Re: [PATCH v2 7/7] iio: Initialize i2c_device_id arrays using member names

["Uwe Kleine-König (The Capable Hub)" <u.kleine-koenig@baylibre.com>]

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

Hello Jonathan,

On Thu, May 21, 2026 at 01:04:44PM +0100, Jonathan Cameron wrote:
> On Tue, 19 May 2026 21:51:20 +0200
> Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com> wrote:
> > On Tue, May 19, 2026 at 07:39:13PM +0100, Jonathan Cameron wrote:
> > > On Tue, 19 May 2026 10:13:09 +0200
> > > Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com> wrote:
> > >   
> > > > While being less compact, using named initializers allows to more easily
> > > > see which members of the structs are assigned which value without having
> > > > to lookup the declaration of the struct. And it's also more robust
> > > > against changes to the struct definition.
> > > > 
> > > > The mentioned robustness is relevant for a planned change to struct
> > > > i2c_device_id that replaces .driver_data by an anonymous union.
> > > > 
> > > > This patch doesn't modify the compiled arrays, only their representation
> > > > in source form benefits. The former was confirmed with x86 and arm64
> > > > builds.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>  
> > > 
> > > I'd prefer it split into cases you care about (not just name) and the name only ones.
> > > That is unless I'm missing some potential change that breaks initializing
> > > just the first element and hence not the union you plan to add.
> > > 
> > > It's a lot of churn and the name one isn't enabling anything new unless
> > > I'm missing something.  
> > 
> > Today all hunks are about using named initializers to improve
> > readability, so the split into only .name vs. .name+.driver_data feels
> > very artificial to me. But if you think that's the compromise to use,
> > I'll adapt.
> > 
> > > We also get fixes in these annoyingly often so chances are this will mess
> > > up backports.  Might even be worth splitting it up into directories
> > > just to reduce that backport mess.  
> > 
> > In my opinion this is the reason to do this kind of cleanup with one
> > patch per driver. This doesn't only make backports easier, it also
> > allows to better record who reviewed and acked what, it reduces merge
> > conflicts (because if one driver is updated in your tree already since
> > my base, with one subsystem patch you get a merge conflict which makes
> > the whole patch unapplicable, with one patch per driver only one out of
> > (here) 208 fails).
> > 
> > But that isn't popular with most subsystem maintainers and so I went
> > with one patch per subsystem. 🤷
> 
> Meh.  This is going to be painful whatever, so to have it as fresh
> as possible I'll pick it up now as one giant patch.

\o/, thanks.

> Note there are already conflicts... Fixed up:
> light/tsl2772.c (a couple more entries)
> light/vcnl4000.c (data is now all pointers, not enum values).
> 
> Bunch of line changes in other drivers, but otherwise went in fine.

I reproduced the issue, my conflict resolution (on top of next-20260521)
looks as follows:

diff --cc drivers/iio/light/tsl2772.c
index 9ba8140c8bc1,1b1c704f1d6c..000000000000
--- a/drivers/iio/light/tsl2772.c
+++ b/drivers/iio/light/tsl2772.c
@@@ -1900,19 -1888,17 +1900,19 @@@ static int tsl2772_resume(struct devic
  }
  
  static const struct i2c_device_id tsl2772_idtable[] = {
- 	{ "tsl2571", tsl2571 },
- 	{ "tsl2671", tsl2671 },
- 	{ "tmd2671", tmd2671 },
- 	{ "tsl2771", tsl2771 },
- 	{ "tmd2771", tmd2771 },
- 	{ "tsl2572", tsl2572 },
- 	{ "tsl2672", tsl2672 },
- 	{ "tmd2672", tmd2672 },
- 	{ "tsl2772", tsl2772 },
- 	{ "tmd2772", tmd2772 },
- 	{ "apds9900", apds9900 },
- 	{ "apds9901", apds9900 },
- 	{ "apds9930", apds9930 },
+ 	{ .name = "tsl2571", .driver_data = tsl2571 },
+ 	{ .name = "tsl2671", .driver_data = tsl2671 },
+ 	{ .name = "tmd2671", .driver_data = tmd2671 },
+ 	{ .name = "tsl2771", .driver_data = tsl2771 },
+ 	{ .name = "tmd2771", .driver_data = tmd2771 },
+ 	{ .name = "tsl2572", .driver_data = tsl2572 },
+ 	{ .name = "tsl2672", .driver_data = tsl2672 },
+ 	{ .name = "tmd2672", .driver_data = tmd2672 },
+ 	{ .name = "tsl2772", .driver_data = tsl2772 },
+ 	{ .name = "tmd2772", .driver_data = tmd2772 },
++	{ .name = "apds9900", .driver_data = apds9900 },
++	{ .name = "apds9901", .driver_data = apds9900 },
+ 	{ .name = "apds9930", .driver_data = apds9930 },
  	{ }
  };
  
diff --cc drivers/iio/light/vcnl4000.c
index 88fc7424ae35,fc2161d5f3c7..000000000000
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@@ -2039,18 -2142,6 +2039,18 @@@ static int vcnl4000_runtime_resume(stru
  static DEFINE_RUNTIME_DEV_PM_OPS(vcnl4000_pm_ops, vcnl4000_runtime_suspend,
  				 vcnl4000_runtime_resume, NULL);
  
 +static const struct i2c_device_id vcnl4000_id[] = {
- 	{ "cm36672p", (kernel_ulong_t)&cm36672p_spec },
- 	{ "cm36686", (kernel_ulong_t)&vcnl4040_spec },
- 	{ "vcnl4000", (kernel_ulong_t)&vcnl4000_spec },
- 	{ "vcnl4010", (kernel_ulong_t)&vcnl4010_spec },
- 	{ "vcnl4020", (kernel_ulong_t)&vcnl4010_spec },
- 	{ "vcnl4040", (kernel_ulong_t)&vcnl4040_spec },
- 	{ "vcnl4200", (kernel_ulong_t)&vcnl4200_spec },
++	{ .name = "cm36672p", .driver_data = (kernel_ulong_t)&cm36672p_spec },
++	{ .name = "cm36686", .driver_data = (kernel_ulong_t)&vcnl4040_spec },
++	{ .name = "vcnl4000", .driver_data = (kernel_ulong_t)&vcnl4000_spec },
++	{ .name = "vcnl4010", .driver_data = (kernel_ulong_t)&vcnl4010_spec },
++	{ .name = "vcnl4020", .driver_data = (kernel_ulong_t)&vcnl4010_spec },
++	{ .name = "vcnl4040", .driver_data = (kernel_ulong_t)&vcnl4040_spec },
++	{ .name = "vcnl4200", .driver_data = (kernel_ulong_t)&vcnl4200_spec },
 +	{ }
 +};
 +MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
 +
  static struct i2c_driver vcnl4000_driver = {
  	.driver = {
  		.name   = VCNL4000_DRV_NAME,
diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
index e2b1da13c0d3..ae50fb27bac9 100644
--- a/drivers/iio/adc/rtq6056.c
+++ b/drivers/iio/adc/rtq6056.c
@@ -872,8 +872,8 @@ static const struct richtek_dev_data rtq6059_devdata = {
 };
 
 static const struct i2c_device_id rtq6056_id[] = {
-	{ "rtq6056", (kernel_ulong_t)&rtq6056_devdata },
-	{ "rtq6059", (kernel_ulong_t)&rtq6059_devdata },
+	{ .name = "rtq6056", .driver_data = (kernel_ulong_t)&rtq6056_devdata },
+	{ .name = "rtq6059", .driver_data = (kernel_ulong_t)&rtq6059_devdata },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, rtq6056_id);

The last hunk was made necessary by commit
ce80292ead5bb42b50a6b63e44fd95c0edf9d334. And I'm pretty sure that the
following is possible on top:

 static const struct of_device_id rtq6056_device_match[] = {
-	{ .compatible = "richtek,rtq6056", .data = &rtq6056_devdata },
-	{ .compatible = "richtek,rtq6059", .data = &rtq6059_devdata },
+	{ .compatible = "richtek,rtq6056" },
+	{ .compatible = "richtek,rtq6059" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, rtq6056_device_match);

> There are a few new instances in tree, though from a quick glance
> maybe no i2c ones.  Since you sent the first series I've been looking
> out for this in reviews, but some stuff was a already queued.
> 
> Anyhow, new ones can be dealt with in follow up patches.

There will more some more drivers I missed in other subsystems, so I'll
have to reiterate anyhow. If you catch drivers before going in, that's
great, but it's not a problem if you miss a few.

Thanks for your cooperation,
Uwe

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