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

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

[Uwe Kleine-König (The Capable Hub)]

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.

Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
---
Hello,

while being a cleanup that can stand on its own this is also a
preparation for making driver_data an anonymous union that requires that
.driver_data is initialized by name and not by list order. The union
allows to make better use of the C type system and drops the need for
casting which then let the compiler notice a missing const. The change
to the driver will look as follows:

	diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c
	index 1739971478ff..f677d30b4342 100644
	--- a/arch/x86/platform/intel-mid/pwr.c
	+++ b/arch/x86/platform/intel-mid/pwr.c
	@@ -347,7 +347,7 @@ struct mid_pwr_device_info {

	 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;
	@@ -471,8 +471,8 @@ static const struct mid_pwr_device_info tng_info = {

	 /* 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 },
		{}
	 };

Best regards
Uwe

 arch/x86/platform/intel-mid/pwr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/platform/intel-mid/pwr.c b/arch/x86/platform/intel-mid/pwr.c
index cd7e0c71adde..1739971478ff 100644
--- a/arch/x86/platform/intel-mid/pwr.c
+++ b/arch/x86/platform/intel-mid/pwr.c
@@ -471,8 +471,8 @@ static const struct mid_pwr_device_info tng_info = {
 
 /* 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), (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 },
 	{}
 };
 

base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
-- 
2.47.3

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Uwe Kleine-König (The Capable Hub)]

[-- 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 --]

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

[Andy Shevchenko]

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

The same argument may be applied to many macros that use concatenation
(##) in them, this is not an argument at all.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Uwe Kleine-König (The Capable Hub)]

[-- 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 --]

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

[Andy Shevchenko]

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

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

[Uwe Kleine-König (The Capable Hub)]

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

Hello Andy,

On Sat, May 09, 2026 at 09:35:57AM +0300, Andy Shevchenko wrote:
> 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.

I argued that ## results in the disadvantage of being harder to work
with and harder to understand. But as this is not about being absolutely
black or white, this is only one aspect and there has to be a weighing
of the benefits that a macro yields against disadvantages. The absolute
interpretation is yours only. What I agreed to is that is a weighing
has to be applied for every macro using ##.

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

So this is not a contradiction.

> And that's my point, PCI_DEVICE_DATA() is not bad and grepping for the
> IDs is quite a niche task.

Note that my argument is about using ## *plus* the hidden assignment in
PCI_VDEVICE() (that PCI_DEVICE_DATA() also does BTW). *Plus* for
PCI_DEVICE_DATA() that it hides ".driver_data" (and in your suggestion
later ".driver_data_ptr" using _Generic).

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

Every additional indirection added to some code makes understanding "only"
one indirection harder. But these sum up, and you either have to grep
for INTEL and sort out all the false positives (

$ git grep INTEL | wc -l
20756

hmm, that's much, then maybe:

$ git grep -E '#define.*INTEL' | wc -l
3451

. But maybe it's not a cpp define but an enum? That is much harder to
grep for. And even if you are able to identify the definition of
PCI_VENDOR_ID_INTEL in that haystack, you cannot be sure that's the used
one without understanding PCI_VDEVICE), or you have to look at the
definition of PCI_VDEVICE (or PCI_DEVICE_DATA) and understand that. With
an explicit PCI_VENDOR_ID_INTEL you can just invoke your editor function
to jump to its definition, or a mouse-over shows its value directly in
your editor. And it goes further with PCI_VENDOR_ID_PHILIPS being
available for code completion while PHILIPS most probably isn't.

(OK, you could argue that then not using any macro at all is the way to
go with my argumentation. That would also be fine for me. The advantage
of using a macro (and preferably exactly one and not the plethora of
macros we have today) is that this makes these easier greppable without
getting sdio_device_ids or virtio_device_ids into the mix as these also
have a .vendor and a .device.

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

Using PCI_DEVICE_DATA is more compact, agreed. But someone who is
proficient in C directly understands what is happening when an array of
structs is initialized using names without having to lookup a macro
definition to understand where the driver data parameter ends up.
This is what I consider easier. Less beauty to the eye maybe, but
instantly detectable what actually happens.

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

Best regards
Uwe

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