[PATCH 0/3] drm: Store USB device in struct drm_device

[Thomas Zimmermann]

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 at 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.

> 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.

Best regards
Thomas

> 
> Regards,
> 
> Hans
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer