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

Uwe Kleine-König (The Capable Hub)
2026-04-29T10:26:36Z

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

[I dropped a few addresses from Cc: that bounced for me before.]

Hello Andy,

On Wed, Apr 29, 2026 at 09:54:54AM +0300, Andy Shevchenko wrote:
> 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.

Agreed, but I'd consider it out of scope for this patch to drop these
comments. That might also be subjective.

> 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

That was my intention. Will rework.

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

Sounds fair.

> > },
>
> Also seems that you may decrease number of LoC here putting it as
>
> { PCI_VDEVICE(CAVIUM, 0x0091) }, /* 68xx */
>
> and so on...

Agreed if all lines of an array can be compressed like that.

> > { /* 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?

Over engineering? Reworking that also seems to be out of scope for this
patch to me.

> 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! },

That was also my intention. Will rework.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]