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: 3844 bytes --]

Hello Andy,

On Fri, May 08, 2026 at 10:15:51AM +0300, Andy Shevchenko wrote:
> 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@baylibre.com> wrote:
> > > >  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,

I fully agree!

> this is not an argument at all.

I don't understand how you deduce this then however.

Not every macro using ## is bad, but every such macro comes at a cost
that you have to consider when justifying the usefulness of the macro.

Of course there is some subjective weighing involved. In my subjective
weighing using both ## and assignments to .class and .class_mask makes
PCI_VDEVICE *at least* questionable. (When all initializers are using
names, the 2nd can be fixed.)

PCI_DEVICE_DATA using ## and the outlook of it using a _Generic
construct is IMHO even worse and so I hesitate going that route.

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

If you ignore the cost of hiding stuff, there is indeed no benefit.

It seems we have to agree to not agree here.

Best regards
Uwe

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