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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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

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

-- 
With Best Regards,
Andy Shevchenko