Re: [PATCH] x86/platform/intel-mid: Use named initializer for pci_device_id array
From: Andy Shevchenko
Date: Fri Jul 17 2026 - 06:20:15 EST
On Fri, Jul 17, 2026 at 11:44:26AM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> On Sat, May 09, 2026 at 03:42:44PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> > > 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.
> >
> > In my approach the first step (U1) is:
> >
> > 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 },
> > {}
> > };
> >
> > (i.e. the patch under discussion) and the second (U2) is
> >
> > static int mid_pwr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > {
> > - struct mid_pwr_device_info *info = (void *)id->driver_data;
> > + const struct mid_pwr_device_info *info = id->driver_data_ptr;
> > struct device *dev = &pdev->dev;
> > struct mid_pwr *pwr;
> > int ret;
> > ...
> > 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 },
> > {}
> > };
> >
> > . With your suggested approach we have instead of U1 the following (A1):
> >
> > 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_DEVICE_DATA(INTEL, PENWELL), &pnw_info) },
> > + { PCI_DEVICE_DATA(INTEL, TANGIER), &tng_info) },
> > {}
> > };
Yes.
> > and to benefit later from the union there is still the first hunk of U2
> > needed (A2):
> >
> > static int mid_pwr_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > {
> > - struct mid_pwr_device_info *info = (void *)id->driver_data;
> > + const struct mid_pwr_device_info *info = id->driver_data_ptr;
> > struct device *dev = &pdev->dev;
> > struct mid_pwr *pwr;
> > int ret;
Yes.
> > . I was under the impression you assumed there is no need for A2, but
> > that's wrong. Additionally U2 is easier to review and judge that is
> > correct than A2 because it uses an indirection less, while A2 depends on
> > .driver_data and .driver_data_ptr holding the same data. With my
> > approach there is always a directly visible 1:1 correspondance between
> > the array and the probe function.
> >
> > Also if A2 at a later point is backported to stable it applies just fine
> > without A1 and the _Generic extention of PCI_DEVICE_DATA() and the added
> > union. With my approach it's more obvious that you also need U1 before
> > you can apply U2. (Yes, it still doesn't fail to apply without the added
> > union to pci_device_id, but that's three potential papercuts with your
> > approach vs. one with mine.)
> >
> > So I don't buy your argument that A1 + A2 is less churn than U1 + U2. In
> > my book U1 + U2 is even less.
>
> You stopped replying to this discussion after my argumentation. Does
> that mean I managed to convince you?
Not at all, I am at strong opinion that PCI_DEVICE_DATA() is the right way to
go with lesser churn. So, I do not like this patch. Please, consider using
PCI_DEVICE_DATA().
> In that case, please consider applying the patch.
I am not in the position of applying this patch, you should ask x86 maintainers
for that.
--
With Best Regards,
Andy Shevchenko