| Message ID | 20240625083131.2205302-2-u.kleine-koenig@baylibre.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: Drop explicit initialization of struct i2c_device_id::driver_data to 0 | expand |
> This prepares putting driver_data in an anonymous union which requires > either no initialization or named designators. Any preview/RFC of what you are aiming for with this one?
Hello Wolfram, On Tue, Jun 25, 2024 at 11:47:29AM +0200, Wolfram Sang wrote: > > This prepares putting driver_data in an anonymous union which requires > > either no initialization or named designators. > > Any preview/RFC of what you are aiming for with this one? There is an inconsistency in the different *_device_id structures. Some have a kernel_ulong_t for driver private data, others have a void*. Depending on how it's used in the drivers, one or the other is better. To get the best from both the idea is to do diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index 4338b1b4ac44..594c5ace303a 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 */ which then allows to drop quite a few casts, e.g. diff --git a/drivers/clk/clk-cdce925.c b/drivers/clk/clk-cdce925.c index 2354d12ddad8..6ba5dce0c951 100644 --- a/drivers/clk/clk-cdce925.c +++ b/drivers/clk/clk-cdce925.c @@ -818,10 +818,10 @@ static const struct clk_cdce925_chip_info clk_cdce949_info = { }; static const struct i2c_device_id cdce925_id[] = { - { .name = "cdce913", .driver_data = (kernel_ulong_t)&clk_cdce913_info, }, - { .name = "cdce925", .driver_data = (kernel_ulong_t)&clk_cdce925_info, }, - { .name = "cdce937", .driver_data = (kernel_ulong_t)&clk_cdce937_info, }, - { .name = "cdce949", .driver_data = (kernel_ulong_t)&clk_cdce949_info, }, + { .name = "cdce913", .data = &clk_cdce913_info, }, + { .name = "cdce925", .data = &clk_cdce925_info, }, + { .name = "cdce937", .data = &clk_cdce937_info, }, + { .name = "cdce949", .data = &clk_cdce949_info, }, { } }; MODULE_DEVICE_TABLE(i2c, cdce925_id); diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 49fdcb3eb8f6..d19ffe79681b 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; The only downside is that all initializers have to use either ".data =" or ".driver_data =". IMHO that is OK, as named initializers are more robust and explicit. Best regards Uwe
Hi Uwe, > There is an inconsistency in the different *_device_id structures. Some > have a kernel_ulong_t for driver private data, others have a void*. > Depending on how it's used in the drivers, one or the other is better. > To get the best from both the idea is to do > > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h > index 4338b1b4ac44..594c5ace303a 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 */ > > which then allows to drop quite a few casts, e.g. I personally could live with the casts, even though the above looks clearer, of course. On suggestion, though, can we please have: + const void *data_ptr; There is no obvious difference between 'driver_data' and 'data' to determine what is the ulong type and what is void*. 'data_ptr' makes this instantly recognizable IMO. Happy hacking, Wolfram
On Tue, Jun 25, 2024 at 10:31:31AM GMT, Uwe Kleine-König wrote: > These drivers don't use the driver_data member of struct i2c_device_id, > so don't explicitly initialize this member. > > This prepares putting driver_data in an anonymous union which requires > either no initialization or named designators. But it's also a nice > cleanup on its own. > > While add it, also remove a comma after the sentinel entry. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> Totally buying the 'cleanup' argument :) Applied to for-next, thanks!
On Wed, Jun 26, 2024 at 12:34:05PM +0200, Wolfram Sang wrote: > Hi Uwe, > > > There is an inconsistency in the different *_device_id structures. Some > > have a kernel_ulong_t for driver private data, others have a void*. > > Depending on how it's used in the drivers, one or the other is better. > > To get the best from both the idea is to do > > > > diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h > > index 4338b1b4ac44..594c5ace303a 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 */ > > > > which then allows to drop quite a few casts, e.g. > > I personally could live with the casts, even though the above looks > clearer, of course. On suggestion, though, can we please have: > > + const void *data_ptr; > > There is no obvious difference between 'driver_data' and 'data' to > determine what is the ulong type and what is void*. 'data_ptr' makes > this instantly recognizable IMO. I share that concern, the reason for considering "data" is the output of git grep 'const void *' include/linux/mod_devicetable.h Getting these all into alignment, maybe even picking a better name than "driver_data", would be the eventual goal. IMHO "driver_data" vs "data_ptr" looks strange, maybe I'd prefer "driver_ptr" over "data" and "data_ptr"?! Best regards and thanks for your feedback, Uwe
> Getting these all into alignment, maybe even picking a better name than > "driver_data", would be the eventual goal. IMHO "driver_data" vs > "data_ptr" looks strange, maybe I'd prefer "driver_ptr" over > "data" and "data_ptr"?! driver_data_ptr?
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index db0d1ac82910..49fdcb3eb8f6 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1066,8 +1066,8 @@ EXPORT_SYMBOL(i2c_find_device_by_fwnode); static const struct i2c_device_id dummy_id[] = { - { "dummy", 0 }, - { }, + { "dummy" }, + { } }; static int dummy_probe(struct i2c_client *client) diff --git a/drivers/i2c/i2c-slave-testunit.c b/drivers/i2c/i2c-slave-testunit.c index a49642bbae4b..fd57b9bb2356 100644 --- a/drivers/i2c/i2c-slave-testunit.c +++ b/drivers/i2c/i2c-slave-testunit.c @@ -162,7 +162,7 @@ static void i2c_slave_testunit_remove(struct i2c_client *client) } static const struct i2c_device_id i2c_slave_testunit_id[] = { - { "slave-testunit", 0 }, + { "slave-testunit" }, { } }; MODULE_DEVICE_TABLE(i2c, i2c_slave_testunit_id); diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c index f809f0ef2004..1cb137b9181d 100644 --- a/drivers/i2c/i2c-smbus.c +++ b/drivers/i2c/i2c-smbus.c @@ -160,7 +160,7 @@ static void smbalert_remove(struct i2c_client *ara) } static const struct i2c_device_id smbalert_ids[] = { - { "smbus_alert", 0 }, + { "smbus_alert" }, { /* LIST END */ } }; MODULE_DEVICE_TABLE(i2c, smbalert_ids); diff --git a/drivers/i2c/muxes/i2c-mux-pca9541.c b/drivers/i2c/muxes/i2c-mux-pca9541.c index e28694d991fb..8663c8a7c269 100644 --- a/drivers/i2c/muxes/i2c-mux-pca9541.c +++ b/drivers/i2c/muxes/i2c-mux-pca9541.c @@ -78,7 +78,7 @@ struct pca9541 { }; static const struct i2c_device_id pca9541_id[] = { - {"pca9541", 0}, + { "pca9541" }, {} };
These drivers don't use the driver_data member of struct i2c_device_id, so don't explicitly initialize this member. This prepares putting driver_data in an anonymous union which requires either no initialization or named designators. But it's also a nice cleanup on its own. While add it, also remove a comma after the sentinel entry. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/i2c/i2c-core-base.c | 4 ++-- drivers/i2c/i2c-slave-testunit.c | 2 +- drivers/i2c/i2c-smbus.c | 2 +- drivers/i2c/muxes/i2c-mux-pca9541.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) base-commit: 62c97045b8f720c2eac807a5f38e26c9ed512371