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