Re: [PATCH v2 7/7] iio: Initialize i2c_device_id arrays using member names
From: Uwe Kleine-König (The Capable Hub)
Date: Thu May 21 2026 - 09:45:03 EST
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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > >
> > > 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:
signature.asc
Description: PGP signature