Re: [PATCH v1 1/1] sub: cdns3: Use predefined PCI vendor ID constant

[Roger Quadros <rogerq@kernel.org>]

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