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

["Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>]

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

Hello Andy,

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@baylibre.com> 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.

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.

Best regards
Uwe

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