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

[Peter Stuge <peter@stuge.se>]

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.

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

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.

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.


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?


Thanks a lot and kind regards

//Peter