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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, May 08, 2026 at 12:49:41PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> 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.

You fully agreed on the ## for any macro, hence every macro using it is not okay.

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

However, you also telling that not every macro is bad. And that's my point,
PCI_DEVICE_DATA() is not bad and grepping for the IDs is quite a niche task.
One can do partial grep and we all know that the PCI device IDs either in
pci_ids.h or in the same file or same driver folder, so there is really easy to
find the real definition (unlike, for example, le32_encode_bits() and other
using ## to construct their names).

> Of course there is some subjective weighing involved.

Sure.

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

We are talking about the user visibility, which is in the users and not
in the definition of the macro. My point is to put all burden in one place
id est PCI_DEVICE_DATA() macro, your point is to spread a churn all over
the kernel which I disagree with.

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

100%. I am against the unneeded churn which this and tons of other
patches for this topic are doing.

-- 
With Best Regards,
Andy Shevchenko