On 23/09/2024 15:53, Andy Shevchenko wrote:
> On Mon, Sep 23, 2024 at 03:42:20PM +0300, Roger Quadros wrote:
>> On 13/09/2024 16:17, Andy Shevchenko wrote:
>>> The PCI vendor ID for Cadence is defined in pci_ids.h. Use it.
>>> While at it, move to PCI_DEVICE() macro and usual pattern for
>>> PCI class and device IDs.
>
> ...
>
>>> +#define PCI_DEVICE_ID_CDNS_USB3 0x0100
>>
>> Why do we need to change this? You did not explain in commit log.
>
> It's explained: "...usual pattern for PCI class and device IDs."
>
>> I would call this PCI_DEVICE_ID_CDNS_USBSS3. Also see later why to differentiate with USBSSP.
>
> It's good to know that there are semantic differences,
> but it is already applied, feel free to update.
>
> ...
>
>>> - { PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
>>> + { PCI_VDEVICE(CDNS, PCI_DEVICE_ID_CDNS_USB3) },
>>
>> For better readability I still prefer
>> PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USBSS3)
>
> I disagree. The PCI_VDEVICE() has less letters and much easier to get
> the vendor from the (less power to parse and decode is required).
>
:)
> ...
>
>>> -#define CDNS_DEVICE_ID 0x0200
>>> -#define CDNS_DRD_ID 0x0100
>>> -#define CDNS_DRD_IF (PCI_CLASS_SERIAL_USB << 8 | 0x80)
>>> +#define PCI_DEVICE_ID_CDNS_USB3 0x0100
>>
>> This is an entirely different card who's device ID should be 0x200?
>> Also I don't think this card supports USB3 so it is a wrong name choice.
>
> Are you stating that 0x0100 in both cases points to the *different* devices?!
> This is unbelievable, however possible abuse of PCI IDs.
I am not entirely sure.
What I do know is that one card should be USBSS (0x100) and other should be USBSSP (0x200). P for super-speed-Plus.
Also please see commit 96b96b2a567f ("usb: cdnsp: changes PCI Device ID to fix conflict with CNDS3 driver")
>
>> I would call this PCI_DEVICE_ID_CDNS_USBSSP 0x200 to match with PCI driver name.
>>
>>> +#define PCI_DEVICE_ID_CDNS_UDC 0x0200
>>
>> UDC is used for Peripheral controller only. Is that really the case here?
>> originally it was called DRD.
>> So how about?
>> PCI_DEVICE_ID_CDNS_DRD 0x0100
>
> I strongly disagree. The same PCI IDs should be named the same independently on
> how many drivers use them.
Agreed.
>
> The only possibility to have what you propose is the complete screwed up PCI
> IDs allocations done by vendor (I do not believe this is the case with Cadence).
>
--
cheers,
-roger
