Re: [PATCH] hwmon: Initialize i2c_device_id structures by name

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Dec 07, 2024 at 07:32:43AM -0800, Guenter Roeck wrote:
> On 12/7/24 01:13, Uwe Kleine-König wrote:
> > Hello Guenter,
> > 
> > [dropped Jose Ramon San Buenaventura from Cc: who's address bounced]
> > 
> > On Thu, Dec 05, 2024 at 08:27:15AM -0800, Guenter Roeck wrote:
> > > On Thu, Dec 05, 2024 at 04:28:33PM +0100, Uwe Kleine-König wrote:
> > > > I intend to change the definition of struct i2c_device_id to look as
> > > > follows:
> > > > 
> > > >          struct i2c_device_id {
> > > >                 char name[I2C_NAME_SIZE];
> > > >                 /* Data private to the driver */
> > > >                 union {
> > > >                         kernel_ulong_t driver_data;
> > > >                         const void *driver_data_ptr;
> > > >                 };
> > > >          };
> > > > 
> > > > That the initializers for these structures also work with this new
> > > > definition, they must use named members.
> > > > 
> > > > The motivation for that change is to be able to drop many casts from
> > > > pointer to kernel_ulong_t. So once the definition is updated,
> > > > initializers that today use:
> > > > 
> > > > 	{"adp4000", (kernel_ulong_t)&pmbus_info_one},
> > > > 
> > > > can be changed to
> > > > 
> > > >          { .name = "adp4000", .driver_data_ptr = &pmbus_info_one },
> > > > 
> > > 
> > > How about introducing a macro for that instead, similar to I3C_DEVICE() ?
> > 
> > OK, for now we'd have then:
> > 
> > 	#define I2C_DEVICE_ID_PTR(_name, _driver_data_ptr)	\
> > 		{ .name = (_name), .driver_data = (kernel_ulong_t)_driver_data_ptr }
> > 
> > 	#define I2C_DEVICE_ID_ULONG(_name, _driver_data)	\
> > 		{ .name = (_name), .driver_data = _driver_data }
> > 
> > plus maybe:
> > 
> > 	#define I2C_DEVICE_ID(_name)	\
> > 		{ .name = (_name) }
> > 
> > for the drivers that don't need driver data?
> > 
> 
> Something like that, though I'd even hide the parameter type and just have
> 
> 	#define I2C_DEVICE_ID_DATA(_name, _data)	\
> 		{ .name = (_name), .driver_data = (kernel_ulong_t)_data }
> 
> 	#define I2C_DEVICE_ID(_name)	\
> 		{ .name = (_name) }

It's better, if we go this way, to keep this in sync with above by changing as

	#define I2C_DEVICE_ID(_name)	I2C_DEVICE_ID_DATA(_name, 0)

> where I2C_DEVICE_ID_DATA() would accept any type.
> 
> > When all drivers are converted accordingly, we could change the
> > definition of i2c_device_id in a commit that only touches i2c core
> > things to introduce the stronger type checking.
> 
> The stronger type checking would not be possible with that, though.
> Does that really add value ? I personally prefer the opaque style where
> driver users can provide (almost) any type they want/need.

-- 
With Best Regards,
Andy Shevchenko