Re: [PATCH v1] regulator: Use named initializers for arrays of i2c_device_data

[Woody Douglass <wdouglass@carnegierobotics.com>]

On 5/15/26 08:28, Laurent Pinchart wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.
>
>
> Hi Uwe,
>
> Thank you for the patch.
>
> On Fri, May 15, 2026 at 12:31:50PM +0200, Uwe Kleine-König (The Capable Hub) 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.
>>
>> While touching all these arrays, unify usage of whitespace and commas.
>>
>> 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>
>> ---
>> Hello,
>>
>> the mentioned change to i2c_device_id is the following:
>>
>>        diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>>        index 23ff24080dfd..aebd3a5e90af 100644
>>        --- a/include/linux/mod_devicetable.h
>>        +++ b/include/linux/mod_devicetable.h
>>        @@ -477,7 +477,11 @@ struct rpmsg_device_id {
>>
>>         struct i2c_device_id {
>>                char name[I2C_NAME_SIZE];
>>        -       kernel_ulong_t driver_data;     /* Data private to the driver */
>>        +       union {
>>        +               /* Data private to the driver */
>>        +               kernel_ulong_t driver_data;
>>        +               const void *driver_data_ptr;
>>        +       };
>>         };
>>
>>         /* pci_epf */
>>
>> and this requires that .driver_data is assigned via a named initializer
>> for static data. This requirement isn't a bad one because named
>> initializers are also much better readable than list initializers.
>>
>> The union added to struct i2c_device_id enables further cleanups like:
>>
>>        diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
>>        index 0123ca8157a8..dfb0b07500a7 100644
>>        --- a/drivers/regulator/ad5398.c
>>        +++ b/drivers/regulator/ad5398.c
>>        @@ -207,8 +207,8 @@ struct ad5398_current_data_format {
>>         static const struct ad5398_current_data_format df_10_4_120 = {10, 4, 0, 120000};
>>
>>         static const struct i2c_device_id ad5398_id[] = {
>>        -       { .name = "ad5398", .driver_data = (kernel_ulong_t)&df_10_4_120 },
>>        -       { .name = "ad5821", .driver_data = (kernel_ulong_t)&df_10_4_120 },
>>        +       { .name = "ad5398", .driver_data_ptr = &df_10_4_120 },
>>        +       { .name = "ad5821", .driver_data_ptr = &df_10_4_120 },
>>                { }
>>         };
>>         MODULE_DEVICE_TABLE(i2c, ad5398_id);
>>        @@ -219,8 +219,7 @@ static int ad5398_probe(struct i2c_client *client)
>>                struct regulator_init_data *init_data = dev_get_platdata(&client->dev);
>>                struct regulator_config config = { };
>>                struct ad5398_chip_info *chip;
>>        -       const struct ad5398_current_data_format *df =
>>        -                       (struct ad5398_current_data_format *)id->driver_data;
>>        +       const struct ad5398_current_data_format *df = id->driver_data;
>>
>>                chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
>>                if (!chip)
>>
>> that are an improvement for readability (again!) and it keeps some
>> properties of the pointers (here: being const) without having to pay
>> attention for that.
>>
>> My additional motivation for this effort is CHERI[1]. This is a hardware
>> extension that uses 128 bit pointers but unsigned long is still 64 bit.
>> So with CHERI you cannot store pointers in unsigned long variables.
>>
>> Best regards
>> Uwe
>>
>> [1] https://cheri-alliance.org/discover-cheri/
>>      https://lwn.net/Articles/1037974/
>>
>>   drivers/regulator/88pg86x.c            |  4 +--
>>   drivers/regulator/ad5398.c             |  4 +--
>>   drivers/regulator/da9121-regulator.c   | 20 +++++++--------
>>   drivers/regulator/da9210-regulator.c   |  4 +--
>>   drivers/regulator/da9211-regulator.c   | 18 +++++++-------
>>   drivers/regulator/fan53880.c           |  4 +--
>>   drivers/regulator/isl9305.c            |  4 +--
>>   drivers/regulator/lp3971.c             |  2 +-
>>   drivers/regulator/lp3972.c             |  2 +-
>>   drivers/regulator/lp872x.c             | 34 +++++++++++++-------------
>>   drivers/regulator/lp8755.c             |  4 +--
>>   drivers/regulator/ltc3589.c            |  6 ++---
>>   drivers/regulator/ltc3676.c            |  2 +-
>>   drivers/regulator/max1586.c            |  2 +-
>>   drivers/regulator/max20086-regulator.c |  8 +++---
>>   drivers/regulator/max20411-regulator.c |  2 +-
>>   drivers/regulator/max77503-regulator.c |  2 +-
>>   drivers/regulator/max77675-regulator.c |  2 +-
>>   drivers/regulator/max77826-regulator.c |  2 +-
>>   drivers/regulator/max77838-regulator.c |  2 +-
>>   drivers/regulator/max77857-regulator.c |  8 +++---
>>   drivers/regulator/max8649.c            |  2 +-
>>   drivers/regulator/max8893.c            |  2 +-
>>   drivers/regulator/max8952.c            |  2 +-
>>   drivers/regulator/mcp16502.c           |  2 +-
>>   drivers/regulator/mp5416.c             |  6 ++---
>>   drivers/regulator/mp8859.c             |  4 +--
>>   drivers/regulator/mp886x.c             |  6 ++---
>>   drivers/regulator/mpq7920.c            |  4 +--
>>   drivers/regulator/mt6311-regulator.c   |  4 +--
>>   drivers/regulator/pf530x-regulator.c   |  8 +++---
>>   drivers/regulator/pf8x00-regulator.c   |  8 +++---
>>   drivers/regulator/pv88060-regulator.c  |  4 +--
>>   drivers/regulator/pv88080-regulator.c  |  8 +++---
>>   drivers/regulator/pv88090-regulator.c  |  4 +--
>>   drivers/regulator/slg51000-regulator.c |  4 +--
>>   drivers/regulator/sy8106a-regulator.c  |  2 +-
>>   drivers/regulator/sy8824x.c            |  8 +++---
>>   drivers/regulator/sy8827n.c            |  4 +--
>>   drivers/regulator/tps6286x-regulator.c | 10 ++++----
>>   drivers/regulator/tps6287x-regulator.c | 10 ++++----
>>   41 files changed, 119 insertions(+), 119 deletions(-)
> [snip]
>
>> diff --git a/drivers/regulator/pf530x-regulator.c b/drivers/regulator/pf530x-regulator.c
>> index f789c4b6a499..e7b13d60106b 100644
>> --- a/drivers/regulator/pf530x-regulator.c
>> +++ b/drivers/regulator/pf530x-regulator.c
>> @@ -353,10 +353,10 @@ static const struct of_device_id pf530x_dt_ids[] = {
>>   MODULE_DEVICE_TABLE(of, pf530x_dt_ids);
>>
>>   static const struct i2c_device_id pf530x_i2c_id[] = {
>> -     { "pf5300", 0 },
>> -     { "pf5301", 0 },
>> -     { "pf5302", 0 },
>> -     {},
>> +     { .name = "pf5300", .driver_data = 0 },
>> +     { .name = "pf5301", .driver_data = 0 },
>> +     { .name = "pf5302", .driver_data = 0 },
> I think you can drop driver_data here. It doesn't appear to be used by
> the driver.

I can confirm this, the pf530x driver does not use the driver_data 
member. This is an improvement, thank you!

Reviewed-by: Woodrow Douglass <wdouglass@carnegierobotics.com>

> I like the result overall. With this small issue addressed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
>> +     { }
>>   };
>>   MODULE_DEVICE_TABLE(i2c, pf530x_i2c_id);
>>
> [snip]
>
> --
> Regards,
>
> Laurent Pinchart