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

Hello Andy,

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) },
> 		 {}
> 	 };
> 
> 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;
> 
> . 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?

In that case, please consider applying the patch.

Thanks
Uwe

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