[PATCH v6 3/3] drm: Add GUD USB Display driver

[Noralf Trønnes]

Den 28.02.2021 02.52, skrev Peter Stuge:
> Noralf Trønnes wrote:
>> Peter, please have a look at this diff and see if I'm on the right track
>> here: https://gist.github.com/notro/a43a93a3aa0cc75d930890b7b254fc0a
> 
> Yes that's exactly what I meant; this way the possibility for contradicting
> sizes is eliminated by protocol and not just by implementation - very nice!
> 
> Some more comments, sorry if this is just because of ongoing work:
> 
> Perhaps the functions taking usb_device + ifnum could take usb_interface
> instead - but I don't know if that would simplify or complicate things.
> Alan mentioned this idea in similar circumstances in another thread.
> I don't feel strongly, but perhaps it's cleaner.
> 

I agree it's cleaner, this way I don't have to store the interface
number in gdrm.

> gud_usb_control_msg() now seems almost redundant, maybe it could be removed.
> 

There are 4 callers so I think it makes sense still.

> In gud_usb_set() if NULL == buf then that's passed to usb_control_msg()
> along with len, which likely crashes if len > 0, so it may be good to
> check or enforce that, maybe with else len=0; before the gud_usb_transfer()
> call.
> 

Ok.

> Finally a small style note that I'd personally change a few if (ret > 0) {
> blocks to have one indent level less and do each check right away, e.g. in
> gud_connector_get_modes():
> 
> ret = gud_usb_get()
> if (ret % EDID_LENGTH) {
> 	drm_err();
> } else if (ret > 0) {
> 	edid_ctx.len = ret;
> 	edid = drm_do_get_edid();
> }
> 
> and later on in the function by the display modes one indent level
> could be saved with a goto:
> 
> if (ret <= 0)
> 	goto out;
> 
> but obviously no huge deal.
> 

It makes for a better read so I'll do that.

> 
> In general it's really helpful for device development to see error messages
> when the device behaves incorrectly, the "Invalid .. size" errors are great
> examples of this, but e.g. gud_get_display_descriptor() returns -EIO without
> a message. Maybe there are opportunities for further helpful error messages?
> 

The message is printed by the caller:

	ret = gud_get_display_descriptor(intf, &desc);
	if (ret) {
		DRM_DEV_DEBUG_DRIVER(dev, "Not a display interface: ret=%d\n", ret);
		return -ENODEV;
	}

It's a debug message enabled by writing to /sys/module/drm/parameters/debug.
The reason for not making it an error message, is that I want the driver
to just ignore non-display vendor class interfaces so they can co-exist
on the device. Someone might make an open protocol gpio (vendor class)
interface driver some day, or adc, i2c, spi, rtc, or...

Thanks,
Noralf.