Re: [PATCH net-next] net: Consistently define pci_device_ids using named initializers

On Tue, Apr 28, 2026 at 07:18:44PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> ... and PCI device helpers.
> 
> The various struct pci_device_id arrays were initialized mostly by one
> the PCI_DEVICE macros and then list expressions. The latter isn't easily
> readable if you're not into PCI. Using named initializers is more
> explicit and thus easier to parse.
> 
> Also use PCI_DEVICE* helper macros to assign .vendor, .device,
> .subvendor and .subdevice where appropriate and skip explicit
> assignments of 0 (which the compiler takes care of).
> 
> The secret plan is to make struct pci_device_id::driver_data an
> anonymous union (similar to
> https://lore.kernel.org/all/[email protected]/)
> and that requires named initializers. But it's also a nice cleanup on
> its own.
> 
> This change doesn't introduce changes to the compiled pci_device_id
> arrays. Tested on x86 and arm64.

...

> -	{0,}						/* 0 terminated list. */
> +	{ }						/* 0 terminated list. */

The comments like these are just noises. The rule of thumb is to play with a
trailing comma:
- always drop it in the terminator entry
- always keep it in the normal initialisers when semantically it's not a
terminator

...

>  static const struct pci_device_id liquidio_pci_tbl[] = {
>  	{       /* 68xx */
> -		PCI_VENDOR_ID_CAVIUM, 0x91, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0
> +		PCI_VDEVICE(CAVIUM, 0x91)

Use full fixed-width device id value(s). 0x0091 here and so on...

>  	},

Also seems that you may decrease number of LoC here putting it as

	{ PCI_VDEVICE(CAVIUM, 0x0091) }, /* 68xx */

and so on...

>  	{       /* 66xx */
> -		PCI_VENDOR_ID_CAVIUM, 0x92, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0
> +		PCI_VDEVICE(CAVIUM, 0x92)
>  	},
>  	{       /* 23xx pf */
> -		PCI_VENDOR_ID_CAVIUM, 0x9702, PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0
> +		PCI_VDEVICE(CAVIUM, 0x9702)
>  	},
> -	{
> -		0, 0, 0, 0, 0, 0, 0
> -	}
> +	{ }
>  };

...

>  #define CH_PCI_DEVICE_ID_TABLE_DEFINE_END \
> -		{ 0, } \
> +		{ } \
>  	}

Why do we have this macro at all?


> -#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END { 0, } }
> +#define CH_PCI_DEVICE_ID_TABLE_DEFINE_END { } }

Ditto.

...

>  static const struct pci_device_id de_pci_tbl[] = {
> -	{ PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_TULIP,
> -	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0 },
> -	{ PCI_VENDOR_ID_DEC, PCI_DEVICE_ID_DEC_TULIP_PLUS,
> -	  PCI_ANY_ID, PCI_ANY_ID, 0, 0, 1 },
> +	{ PCI_VDEVICE(DEC, PCI_DEVICE_ID_DEC_TULIP), .driver_data = 0 },
> +	{ PCI_VDEVICE(DEC, PCI_DEVICE_ID_DEC_TULIP_PLUS), .driver_data = 1 },
>  	{ },

Drop comma. I.o.w. please make sure you also unify the style of the ID tables,
including terminator entries.

>  };

...

>  static const struct pci_device_id sis190_pci_tbl[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_SI, 0x0190), 0, 0, 0 },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_SI, 0x0191), 0, 0, 1 },
> -	{ 0, },
> +	{ PCI_VDEVICE(SI, 0x0190), .driver_data = 0 },
> +	{ PCI_VDEVICE(SI, 0x0191), .driver_data = 1 },
> +	{ },

Ditto and so on...

>  };

...

Also I somehow managed to remove, but I remember you had an inner comma in some
cases after the .driver_data, when the full ID entry is located on a single
line. I.o.w. do

	{ PCI_...(), .driver_data = ... // no trailing comma here! },

When it's a single line trailing comma inside helps nothing and just makes
lines longer and harder to read.

-- 
With Best Regards,
Andy Shevchenko