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

Uwe Kleine-König (The Capable Hub) <[email protected]> Wed, 29 Apr 2026 12:26:32 +0200

Newsgroups	dev.linux.lists.brcm80211,org.kernel.vger.linux-can,org.kernel.vger.linux-kernel,org.kernel.vger.linux-parisc,org.kernel.vger.linux-rdma,org.kernel.vger.linux-wireless,org.kernel.vger.netdev,org.osuosl.intel-wired-lan
Message-ID	<afHbcwzVucHRYmDW@monoceros>

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

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

signature.asc (application/pgp-signature, 488 B)