Hi,
On 10/22/20 12:57 PM, Thomas Zimmermann wrote:
Hi Hans
On 22.10.20 12:17, Hans de Goede wrote:
Hi,
On 10/22/20 11:30 AM, Thomas Zimmermann wrote:
Hi
On 22.10.20 11:20, Hans de Goede wrote:
Hi,
On 10/21/20 3:07 PM, Thomas Zimmermann wrote:
The drivers gm12u320 and udl operate on USB devices. They leave the PCI device in struct drm_device empty and store the USB device in their own driver structure.
Fix this special case and save a few bytes by putting the USB device into an anonymous union with the PCI data. It's expected that DRM core and helpers only touch the PCI-device field for actual PCI devices.
Thomas Zimmermann (3): drm: Add reference to USB device to struct drm_device drm/tiny/gm12u320: Store USB device in struct drm_device.udev drm/udl: Store USB device in struct drm_device.udev
This series looks good to me:
Reviewed-by: Hans de Goede hdegoede@redhat.com
Thanks a lot. Following Daniel's request, I'll drop patch 1 and instead do an upcast from drm_device.dev to the USB device structure. The driver patches 2 and 3 will be slightly different. Unless you object, I''ll take the r-b into the new patches.
I somehow missed Daniel's reply about this.
With that said, hmm that is going to be an interesting up-cast, at least for the gm12u320, that is going to look something like this:
struct usb_device *udev = interface_to_usbdev(to_usb_interface(drm_dev->dev));
(I wrote drm_dev instead of dev to make it more clear what is going on)
For the DRM_DEV_ERROR() macro you can just use gm12u320->dev.dev , that will make the errors be printed with the in usb-interface device-name as prefix instead of the usb-device device-name, but that is fine.
I wonder of this is all worth it then though, just to save those few bytes ?
Daniel and I briefly discussed on IRC if we could make drm_device.pdev (the PCI dev ) a legacy field in the longer term. All Linux devices would be retrieved from drm_device.dev then.
I see. Then I guess the fancy cast which I gave above is the right way to move forward with this.
The first version made some sense since it made how drm devices with usb resp. pci parents are handled consistent. Now it seems to make the code somewhat harder to understand just to save the storage for a single pointer...
What's the implication of setting drm_device.dev to the value of struct usb_device.dev ? That's the device all the code interacts with.
USB drivers bind to USB interfaces, not to the USB device (the parent of the interfaces). A single USB device may have multiple interfaces, e.g. an USB headset may implement the USB audio class for the microphone and headphones function, while having a second interface implementing the HID interface for things like volume buttons, a play/pause button, etc.
So from the USB device model's pov making the USB device itself the parent of the drm device is just wrong. I guess it may not cause much issues, but it very much goes against how USB devices / interfaces are supposed to be used inside the kernel.
So I guess we should just move forward with a v2 with the long / fancy casts.
As for keeping my Reviewed-by for that v2, yes that is fine.
Regards,
Hans