Re: [PATCH] x86/platform/intel-mid: Use named initializer for pci_device_id array

[Andy Shevchenko]

On Fri, May 08, 2026 at 08:25:45AM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> On Thu, May 07, 2026 at 08:24:18PM +0300, Andy Shevchenko wrote:
> > On Thu, May 7, 2026 at 6:40 PM Uwe Kleine-König (The Capable Hub)
> > <u.kleine-koenig@xxxxxxxxxxxx> wrote:
> > >
> > > While being more verbose using a named initializer yields easier to
> > > understand code and doesn't rely on the two hidden zeros in the
> > > PCI_VDEVICE macro.
> > >
> > > This doesn't introduce any changes to the compiled result of the array,
> > > which was confirmed with an ARCH=x86 build.
> > 
> > Instead you can modify PCI_DEVICE_DATA() to accept different types and
> > act accordingly, no need for this churn in the drivers.
> > 
> > >          /* This table should be in sync with the one in drivers/pci/pci-mid.c */
> > >          static const struct pci_device_id mid_pwr_pci_ids[] = {
> > >         -       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data = (kernel_ulong_t)&pnw_info },
> > >         -       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data = (kernel_ulong_t)&tng_info },
> > >         +       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data_ptr = &pnw_info },
> > >         +       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data_ptr = &tng_info },
> > >                 {}
> > >          };

...

> > >  static const struct pci_device_id mid_pwr_pci_ids[] = {
> > > -       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
> > > -       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
> > > +       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), .driver_data = (kernel_ulong_t)&pnw_info },
> > > +       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), .driver_data = (kernel_ulong_t)&tng_info },
> > >         {}
> > >  };
> > 
> > Just use PCI_DEVICE_DATA() here and do whatever you want in the future
> > there, once for all.
> 
> I see quite some benefit in having some things explicit in a driver and
> not hide them behind a macro using _Generic magic. Yes being explicit is
> more verbose and here requires to touch the driver twice. But in my
> subjective view it's better to be able to look at the array in the
> driver and be able to understand instantly what happens without having
> to recurse into macro definitions to understand that. IMHO PCI_VDEVICE
> is already at least borderline in that regard because it composes
> identifier names (with the result that `INTEL` is used to build
> `PCI_VENDOR_ID_INTEL` which takes away the possibility to jump to its
> definition using the usual code editor functions) and it contains two
> zeros initializing .class and .class_mask which also isn't obvious for
> the casual reader.

> Yes PCI_DEVICE_DATA is more compact and for someone who touches PCI code
> daily it's also readable and if your working for Intel you maybe even
> know the value of PCI_VENDOR_ID_INTEL by heart and so never are in the
> situation to want to easily jump to its definition.

The same argument may be applied to many macros that use concatenation
(##) in them, this is not an argument at all.

> But for people who don't know the subsystem and its magic such macros
> raise the bar to understand what happens---and that's bad for long term
> maintainabilty and attacting new developers.
> 
> Additionally I also see benefit in the initializer explicitly using
> .driver_data (or .driver_data_ptr) to match that against the probe
> function that hopefully uses the same member.
> 
> So I hear you and understand the advantages you point out, but in my
> assessment the disadvantages of PCI_DEVICE_DATA still prevail here.

The whole point is to hide whatever you do behind a single (or a few)
macro and don't disrupt kernel with thousands of unneeded churn patches.

I do not see any benefit of doing several steps here. Convert the driver
to use PCI_DEVICE_DATA() once and no need to touch this driver anymore.

-- 
With Best Regards,
Andy Shevchenko