Traditionally a struct i2c_device_id has a kernel_ulong_t member to
store some chip variant data. Some drivers use it to store an enum,
others to store a pointer. In the latter case there is some ugly(?)
casting involved. To improve that, add a void pointer in an anonymous
union together with the integer driver_data.

This way a i2c_device_id requires usage of a designated initializer when
the driver_data or data member should be initialized, but IMHO that's
fine and you might even consider that an advantage.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I faked a base commit hash to hopefully confuse the auto build bots
enough to not send build reports. This patch isn't complete, quite a
few more drivers would need adaption.
I only converted a few to have a showcase to collect feedback if others
consider this a nice idea, too.

I shuffled the order of hunks to have the most interesting ones at the
top. The first change implements the modification to i2c_device_id. The
next drops cast in the i2c core and the following two show how drivers
that use integer and pointer values respectively can benefit.

There is an annoying amount of drivers that use

 static const struct i2c_device_id mydriver_i2c_id[] = {
	{ "chipname", 0 },
 	{ }
 };

and which is broken with this patch. These would all have to be touched,
probably dropping the ", 0".

Do you like the idea? If the feedback is mostly positive, I'd start
tackling this quest, probably by first preparing the affected drivers by
dropping explitit zeros as above and/or using a designated initializer.

A possible continuation is to apply the same idea to all the other
${subsystem}_device_id structs.

Best regards
Uwe

 include/linux/mod_devicetable.h          |  5 ++++-
 drivers/i2c/i2c-core-base.c              |  2 +-
 sound/soc/codecs/tas571x.c               | 12 ++++++------
 sound/soc/codecs/tlv320aic31xx.c         | 16 ++++++++--------
 arch/arm/mach-s3c/mach-crag6410-module.c |  2 +-
 drivers/pwm/pwm-pca9685.c                |  2 +-
 sound/soc/codecs/nau8825.c               |  2 +-
 sound/soc/codecs/rt1015.c                |  2 +-
 sound/soc/codecs/rt5682-i2c.c            |  2 +-
 sound/soc/codecs/src4xxx-i2c.c           |  2 +-
 sound/soc/codecs/tas2764.c               |  2 +-
 11 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7a9a07ea451b..f7e51c142a64 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -477,7 +477,10 @@ struct rpmsg_device_id {
 
 struct i2c_device_id {
 	char name[I2C_NAME_SIZE];
-	kernel_ulong_t driver_data;	/* Data private to the driver */
+	union {
+		kernel_ulong_t driver_data;	/* Data private to the driver */
+		const void *data;
+	};
 };
 
 /* pci_epf */
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index ff5c486a1dbb..ccabc71d0041 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -129,7 +129,7 @@ const void *i2c_get_match_data(const struct i2c_client *client)
 		if (!match)
 			return NULL;
 
-		data = (const void *)match->driver_data;
+		data = match->data;
 	}
 
 	return data;
diff --git a/sound/soc/codecs/tas571x.c b/sound/soc/codecs/tas571x.c
index f249e93e2a4e..0fde0111c0e2 100644
--- a/sound/soc/codecs/tas571x.c
+++ b/sound/soc/codecs/tas571x.c
@@ -949,12 +949,12 @@ static const struct of_device_id tas571x_of_match[] __maybe_unused = {
 MODULE_DEVICE_TABLE(of, tas571x_of_match);
 
 static const struct i2c_device_id tas571x_i2c_id[] = {
-	{ "tas5707", (kernel_ulong_t) &tas5707_chip },
-	{ "tas5711", (kernel_ulong_t) &tas5711_chip },
-	{ "tas5717", (kernel_ulong_t) &tas5717_chip },
-	{ "tas5719", (kernel_ulong_t) &tas5717_chip },
-	{ "tas5721", (kernel_ulong_t) &tas5721_chip },
-	{ "tas5733", (kernel_ulong_t) &tas5733_chip },
+	{ .name = "tas5707", .data = &tas5707_chip },
+	{ .name = "tas5711", .data = &tas5711_chip },
+	{ .name = "tas5717", .data = &tas5717_chip },
+	{ .name = "tas5719", .data = &tas5717_chip },
+	{ .name = "tas5721", .data = &tas5721_chip },
+	{ .name = "tas5733", .data = &tas5733_chip },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tas571x_i2c_id);
diff --git a/sound/soc/codecs/tlv320aic31xx.c b/sound/soc/codecs/tlv320aic31xx.c
index 4d7c5a80c6ed..ed78cb549ace 100644
--- a/sound/soc/codecs/tlv320aic31xx.c
+++ b/sound/soc/codecs/tlv320aic31xx.c
@@ -1627,14 +1627,14 @@ static void aic31xx_configure_ocmv(struct aic31xx_priv *priv)
 }
 
 static const struct i2c_device_id aic31xx_i2c_id[] = {
-	{ "tlv320aic310x", AIC3100 },
-	{ "tlv320aic311x", AIC3110 },
-	{ "tlv320aic3100", AIC3100 },
-	{ "tlv320aic3110", AIC3110 },
-	{ "tlv320aic3120", AIC3120 },
-	{ "tlv320aic3111", AIC3111 },
-	{ "tlv320dac3100", DAC3100 },
-	{ "tlv320dac3101", DAC3101 },
+	{ .name = "tlv320aic310x", .driver_data = AIC3100 },
+	{ .name = "tlv320aic311x", .driver_data = AIC3110 },
+	{ .name = "tlv320aic3100", .driver_data = AIC3100 },
+	{ .name = "tlv320aic3110", .driver_data = AIC3110 },
+	{ .name = "tlv320aic3120", .driver_data = AIC3120 },
+	{ .name = "tlv320aic3111", .driver_data = AIC3111 },
+	{ .name = "tlv320dac3100", .driver_data = DAC3100 },
+	{ .name = "tlv320dac3101", .driver_data = DAC3101 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, aic31xx_i2c_id);
diff --git a/arch/arm/mach-s3c/mach-crag6410-module.c b/arch/arm/mach-s3c/mach-crag6410-module.c
index 2de1a89f6e99..5d320a767f0d 100644
--- a/arch/arm/mach-s3c/mach-crag6410-module.c
+++ b/arch/arm/mach-s3c/mach-crag6410-module.c
@@ -446,7 +446,7 @@ static int wlf_gf_module_probe(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id wlf_gf_module_id[] = {
-	{ "wlf-gf-module", 0 },
+	{ "wlf-gf-module", },
 	{ }
 };
 
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index c5da2a6ed846..15b3481caf72 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -634,7 +634,7 @@ static int __maybe_unused pca9685_pwm_runtime_resume(struct device *dev)
 }
 
 static const struct i2c_device_id pca9685_id[] = {
-	{ "pca9685", 0 },
+	{ "pca9685", },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(i2c, pca9685_id);
diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index cd30ad649bae..bde25bc6909d 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -2934,7 +2934,7 @@ static void nau8825_i2c_remove(struct i2c_client *client)
 {}
 
 static const struct i2c_device_id nau8825_i2c_ids[] = {
-	{ "nau8825", 0 },
+	{ "nau8825" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, nau8825_i2c_ids);
diff --git a/sound/soc/codecs/rt1015.c b/sound/soc/codecs/rt1015.c
index 1250cfaf2adc..0f806dde9c39 100644
--- a/sound/soc/codecs/rt1015.c
+++ b/sound/soc/codecs/rt1015.c
@@ -1097,7 +1097,7 @@ static const struct regmap_config rt1015_regmap = {
 };
 
 static const struct i2c_device_id rt1015_i2c_id[] = {
-	{ "rt1015", 0 },
+	{ "rt1015" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rt1015_i2c_id);
diff --git a/sound/soc/codecs/rt5682-i2c.c b/sound/soc/codecs/rt5682-i2c.c
index 62f26ce9d476..ff9e14fad0cd 100644
--- a/sound/soc/codecs/rt5682-i2c.c
+++ b/sound/soc/codecs/rt5682-i2c.c
@@ -318,7 +318,7 @@ static const struct acpi_device_id rt5682_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, rt5682_acpi_match);
 
 static const struct i2c_device_id rt5682_i2c_id[] = {
-	{"rt5682", 0},
+	{"rt5682"},
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, rt5682_i2c_id);
diff --git a/sound/soc/codecs/src4xxx-i2c.c b/sound/soc/codecs/src4xxx-i2c.c
index 93af8e209b05..55f00ce7c718 100644
--- a/sound/soc/codecs/src4xxx-i2c.c
+++ b/sound/soc/codecs/src4xxx-i2c.c
@@ -19,7 +19,7 @@ static int src4xxx_i2c_probe(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id src4xxx_i2c_ids[] = {
-	{ "src4392", 0 },
+	{ "src4392" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, src4xxx_i2c_ids);
diff --git a/sound/soc/codecs/tas2764.c b/sound/soc/codecs/tas2764.c
index a9838e0738cc..c7eab3289430 100644
--- a/sound/soc/codecs/tas2764.c
+++ b/sound/soc/codecs/tas2764.c
@@ -738,7 +738,7 @@ static int tas2764_i2c_probe(struct i2c_client *client)
 }
 
 static const struct i2c_device_id tas2764_i2c_id[] = {
-	{ "tas2764", 0},
+	{ "tas2764" },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tas2764_i2c_id);

base-commit: 6e4f84f18c4ee9b0ccdc19e39b7de41df21699dd
-- 
2.43.0

On Fri, Apr 26, 2024 at 11:38:33PM +0200, Uwe Kleine-König wrote:
> Traditionally a struct i2c_device_id has a kernel_ulong_t member to
> store some chip variant data. Some drivers use it to store an enum,
> others to store a pointer. In the latter case there is some ugly(?)
> casting involved. To improve that, add a void pointer in an anonymous
> union together with the integer driver_data.
> 
> This way a i2c_device_id requires usage of a designated initializer when
> the driver_data or data member should be initialized, but IMHO that's
> fine and you might even consider that an advantage.

...

>  static const struct i2c_device_id wlf_gf_module_id[] = {
> -	{ "wlf-gf-module", 0 },
> +	{ "wlf-gf-module", },

In such cases the inner comma is redundant as well.

>  	{ }
>  };

...

In general idea might be okay, but I always have the same Q (do we have it
being clarified in the documentation, btw): is an ID table the ABI or not?
In another word, how should we treat the changes there, because ID tables
are being used by the user space tools.

-- 
With Best Regards,
Andy Shevchenko

Hello,

On Mon, Apr 29, 2024 at 11:54:29AM +0300, Andy Shevchenko wrote:
> On Fri, Apr 26, 2024 at 11:38:33PM +0200, Uwe Kleine-König wrote:
> > Traditionally a struct i2c_device_id has a kernel_ulong_t member to
> > store some chip variant data. Some drivers use it to store an enum,
> > others to store a pointer. In the latter case there is some ugly(?)
> > casting involved. To improve that, add a void pointer in an anonymous
> > union together with the integer driver_data.
> > 
> > This way a i2c_device_id requires usage of a designated initializer when
> > the driver_data or data member should be initialized, but IMHO that's
> > fine and you might even consider that an advantage.
> 
> ...
> 
> >  static const struct i2c_device_id wlf_gf_module_id[] = {
> > -	{ "wlf-gf-module", 0 },
> > +	{ "wlf-gf-module", },
> 
> In such cases the inner comma is redundant as well.

I would tend to keep the comma, but no strong opinion on my side.
If another member init is added later, the line has to be touched
anyhow, but in the layout:

	... = {
		{
			"wlf-gf-module",
		},
		{ }
	}

I'd keep it for sure.


> >  	{ }
> >  };
> 
> ...
> 
> In general idea might be okay, but I always have the same Q (do we have it
> being clarified in the documentation, btw): is an ID table the ABI or not?
> In another word, how should we treat the changes there, because ID tables
> are being used by the user space tools.

Note that the layout doesn't change and the traditional interpretation
of the data still works fine. Or do you see something that I miss?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Mon, Apr 29, 2024 at 12:21:05PM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 29, 2024 at 11:54:29AM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 11:38:33PM +0200, Uwe Kleine-König wrote:

...

> > >  static const struct i2c_device_id wlf_gf_module_id[] = {
> > > -	{ "wlf-gf-module", 0 },
> > > +	{ "wlf-gf-module", },
> > 
> > In such cases the inner comma is redundant as well.
> 
> I would tend to keep the comma, but no strong opinion on my side.

It's just a confusing leftover in my opinion.

> If another member init is added later, the line has to be touched
> anyhow, but in the layout:
> 
> 	... = {
> 		{
> 			"wlf-gf-module",
> 		},
> 		{ }
> 	}
> 
> I'd keep it for sure.

That's not what I object. Here I am 100% with you.

> > >  	{ }
> > >  };

...

> > In general idea might be okay, but I always have the same Q (do we have it
> > being clarified in the documentation, btw): is an ID table the ABI or not?
> > In another word, how should we treat the changes there, because ID tables
> > are being used by the user space tools.
> 
> Note that the layout doesn't change and the traditional interpretation
> of the data still works fine. Or do you see something that I miss?

Do we have any configurations / architectures / etc when
sizeof(kernel_ulong_t) != sizeof(void *) ? If not, we are fine.

(Different endianess seems impossible.)

-- 
With Best Regards,
Andy Shevchenko

Hello Andy,

On Mon, Apr 29, 2024 at 01:28:32PM +0300, Andy Shevchenko wrote:
> On Mon, Apr 29, 2024 at 12:21:05PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 29, 2024 at 11:54:29AM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 26, 2024 at 11:38:33PM +0200, Uwe Kleine-König wrote:
> 
> ...
> 
> > > >  static const struct i2c_device_id wlf_gf_module_id[] = {
> > > > -	{ "wlf-gf-module", 0 },
> > > > +	{ "wlf-gf-module", },
> > > 
> > > In such cases the inner comma is redundant as well.
> > 
> > I would tend to keep the comma, but no strong opinion on my side.
> 
> It's just a confusing leftover in my opinion.
> 
> > If another member init is added later, the line has to be touched
> > anyhow, but in the layout:
> > 
> > 	... = {
> > 		{
> > 			"wlf-gf-module",
> > 		},
> > 		{ }
> > 	}
> > 
> > I'd keep it for sure.
> 
> That's not what I object. Here I am 100% with you.

OK, agreed. I'm not sure yet if I prefer

	static const struct i2c_device_id wlf_gf_module_id[] = {
		{ "wlf-gf-module" },
		{ }
	};

or

	static const struct i2c_device_id wlf_gf_module_id[] = {
		{ .name = "wlf-gf-module" },
		{ }
	};

> > > In general idea might be okay, but I always have the same Q (do we have it
> > > being clarified in the documentation, btw): is an ID table the ABI or not?
> > > In another word, how should we treat the changes there, because ID tables
> > > are being used by the user space tools.
> > 
> > Note that the layout doesn't change and the traditional interpretation
> > of the data still works fine. Or do you see something that I miss?
> 
> Do we have any configurations / architectures / etc when
> sizeof(kernel_ulong_t) != sizeof(void *) ? If not, we are fine.

According to https://wiki.debian.org/ArchitectureSpecificsMemo (my goto
address for such questions) we have sizeof(void *) == sizeof(long) on
all archs. Also storing a pointer in today's struct
i2c_device_id::driver_data is so common that it should be safe to assume
that sizeof(void *) <= sizeof(kernel_ulong_t). And that <= is enough
that the union doesn't get bigger.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

On Mon, Apr 29, 2024 at 03:55:57PM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 29, 2024 at 01:28:32PM +0300, Andy Shevchenko wrote:

...

> OK, agreed. I'm not sure yet if I prefer
> 
> 	static const struct i2c_device_id wlf_gf_module_id[] = {
> 		{ "wlf-gf-module" },
> 		{ }
> 	};
> 
> or
> 
> 	static const struct i2c_device_id wlf_gf_module_id[] = {
> 		{ .name = "wlf-gf-module" },
> 		{ }
> 	};

Personally I don't care, but it seems in such cases the .name is too verbose
for no benefit. If one needs to expand this with driver data they will change
that line in any case.

-- 
With Best Regards,
Andy Shevchenko