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

Andy Shevchenko
2026-04-29T06:55:14Z

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/cover.1776579304.git.u.kleine-koenig@baylibre.com/)
> 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