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.