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. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
