RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi all,

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Tue, 8 Aug 2023 15:18:52 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> > > > On Sun, Aug 06, 2023 at 02:29:50PM +0100, Jonathan Cameron wrote:
> > > > > On Sat, 5 Aug 2023 17:42:21 +0000 Biju Das
> > > > > <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > On Fri,  4 Aug 2023 17:17:24 +0100 Biju Das
> > > > > > > <biju.das.jz@bp.renesas.com> wrote:
> >
> > ...
> >
> > > > > > + * Besides the fact that some drivers abuse the device ID
> > > > > > + driver_data type
> > > > > > + * and claim it to be integer, for the bus specific ID tables
> > > > > > + the driver_data
> > > > > > + * may be defined as kernel_ulong_t. For these tables 0 is a
> > > > > > + valid response,
> > > > > > + * but not for this function. It's recommended to convert
> > > > > > + those either to avoid
> > > > > > + * 0 or use a real pointer to the predefined driver data.
> > > >
> > > > > We still need to maintain consistency across the two tables,
> > > > > which is a stronger requirement than avoiding 0.
> > > >
> > > > True. Any suggestion how to amend the above comment? Because the
> > > > documentation makes sense on its own (may be split from the
> series?).
> > > >
> > > > > Some drivers already do that by forcing the enum used to start
> > > > > at 1 which doesn't solver the different data types issue.
> > > >
> > > > And some maintainers do not want to see non-enum values in i2c ID
> table.
> > > > *Shrug*.
> > >
> > > So in legacy ID lookup path we can safely assume that values below
> > > 4096 are scalars and return NULL from the new
> > > device_get_match_data(). This way current drivers using the values
> > > as indices or doing direct comparisons against them can continue
> > > doing manual look up and using them as they see fit. And we can
> convert the drivers at our leisure.
> >
> > It's a good idea, but I believe will be received as hack.
> > But why not to try? We indeed have an error pointers for the last page
> > and NULL (which is only up to 16 IIRC) and reserved space in the first
> > page. To be more robust I would check all enums that are being used in
> > I2C ID tables for maximum value and if that is less than 16, use
> > ZERO_OR_NULL_PTR() instead of custom stuff.
> >
> See iio/adc/max1363 example that has 37ish.
> 
> Could tidy that one up first and hopefully not find any others that are in
> subsystems not keen on the move away from enums.

If there is no objection, I can fix this using i2c_get_match_data() for iio/adc/max1363

and 

device_match_data() will return ZERO_OR_NULL_PTR() if max enum ID in the ID lookup table is less than 16.

and the drivers that use legacy ID's will fallback to ID lookup.

So that there won't be any regression.

Cheers,
Biju

Re: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > On Tue, 8 Aug 2023 15:18:52 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:

...

> > > > So in legacy ID lookup path we can safely assume that values below
> > > > 4096 are scalars and return NULL from the new
> > > > device_get_match_data(). This way current drivers using the values
> > > > as indices or doing direct comparisons against them can continue
> > > > doing manual look up and using them as they see fit. And we can
> > convert the drivers at our leisure.
> > >
> > > It's a good idea, but I believe will be received as hack.
> > > But why not to try? We indeed have an error pointers for the last page
> > > and NULL (which is only up to 16 IIRC) and reserved space in the first
> > > page. To be more robust I would check all enums that are being used in
> > > I2C ID tables for maximum value and if that is less than 16, use
> > > ZERO_OR_NULL_PTR() instead of custom stuff.
> > >
> > See iio/adc/max1363 example that has 37ish.
> > 
> > Could tidy that one up first and hopefully not find any others that are in
> > subsystems not keen on the move away from enums.
> 
> If there is no objection, I can fix this using i2c_get_match_data() for
> iio/adc/max1363 and device_match_data() will return ZERO_OR_NULL_PTR()
> if max enum ID in the ID lookup table is less than 16. And the drivers
> that use legacy ID's will fallback to ID lookup. So that there won't be
> any regression.

I'm good with this approach, but make sure you checked the whole kernel source
tree for a such.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy Shevchenko,

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > > On Tue, 8 Aug 2023 15:18:52 +0300
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Aug 07, 2023 at 01:37:12PM -0700, Dmitry Torokhov wrote:
> > > > > On Mon, Aug 07, 2023 at 05:54:07PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > So in legacy ID lookup path we can safely assume that values
> > > > > below
> > > > > 4096 are scalars and return NULL from the new
> > > > > device_get_match_data(). This way current drivers using the
> > > > > values as indices or doing direct comparisons against them can
> > > > > continue doing manual look up and using them as they see fit.
> > > > > And we can
> > > convert the drivers at our leisure.
> > > >
> > > > It's a good idea, but I believe will be received as hack.
> > > > But why not to try? We indeed have an error pointers for the last
> > > > page and NULL (which is only up to 16 IIRC) and reserved space in
> > > > the first page. To be more robust I would check all enums that are
> > > > being used in I2C ID tables for maximum value and if that is less
> > > > than 16, use
> > > > ZERO_OR_NULL_PTR() instead of custom stuff.
> > > >
> > > See iio/adc/max1363 example that has 37ish.
> > >
> > > Could tidy that one up first and hopefully not find any others that
> > > are in subsystems not keen on the move away from enums.
> >
> > If there is no objection, I can fix this using i2c_get_match_data()
> > for
> > iio/adc/max1363 and device_match_data() will return ZERO_OR_NULL_PTR()
> > if max enum ID in the ID lookup table is less than 16. And the drivers
> > that use legacy ID's will fallback to ID lookup. So that there won't
> > be any regression.
> 
> I'm good with this approach, but make sure you checked the whole kernel
> source tree for a such.

Checking against 16 is too short I guess??

drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.

/*device enum */
enum inv_devices {
	INV_MPU6050,
	INV_MPU6500,
	INV_MPU6515,
	INV_MPU6880,
	INV_MPU6000,
	INV_MPU9150,
	INV_MPU9250,
	INV_MPU9255,
	INV_ICM20608,
	INV_ICM20608D,
	INV_ICM20609,
	INV_ICM20689,
	INV_ICM20600,
	INV_ICM20602,
	INV_ICM20690,
	INV_IAM20680,
	INV_NUM_PARTS
};

The new helper function

+static bool i2c_is_client_uses_legacy_id_table(const struct i2c_driver *driver)
+{
+	const struct i2c_device_id *id = driver->id_table;
+	kernel_ulong_t max_val = 0;
+
+	if (!id)
+		return FALSE;
+
+	while (id->name[0]) {
+		if (id->driver_data > max_val)
+			max_val = id->driver_data;
+		id++;
+	}
+
+	return ZERO_OR_NULL_PTR(max_val);
+}
+

Cheers,
Biju

Re: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:

...

> > I'm good with this approach, but make sure you checked the whole kernel
> > source tree for a such.
> 
> Checking against 16 is too short I guess??
> 
> drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.

So, what does prevent us from moving that tables to use pointers?

> /*device enum */
> enum inv_devices {
> 	INV_MPU6050,
> 	INV_MPU6500,
> 	INV_MPU6515,
> 	INV_MPU6880,
> 	INV_MPU6000,
> 	INV_MPU9150,
> 	INV_MPU9250,
> 	INV_MPU9255,
> 	INV_ICM20608,
> 	INV_ICM20608D,
> 	INV_ICM20609,
> 	INV_ICM20689,
> 	INV_ICM20600,
> 	INV_ICM20602,
> 	INV_ICM20690,
> 	INV_IAM20680,
> 	INV_NUM_PARTS
> };
> 
> The new helper function

You mean for debugging? We don't need that in production.

I think what you need is a coccinelle script to find these.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy,

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> 
> ...
> 
> > > I'm good with this approach, but make sure you checked the whole
> > > kernel source tree for a such.
> >
> > Checking against 16 is too short I guess??
> >
> > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> 
> So, what does prevent us from moving that tables to use pointers?

I think that will lead to ABI breakage(client->name vs id->name)

	match = device_get_match_data(&client->dev);
	if (match) {
		chip_type = (uintptr_t)match;
		name = client->name;
	} else if (id) {
		chip_type = (enum inv_devices)
			id->driver_data;
		name = id->name;
	} else {
		return -ENOSYS;
	}

> 
> > /*device enum */
> > enum inv_devices {
> > 	INV_MPU6050,
> > 	INV_MPU6500,
> > 	INV_MPU6515,
> > 	INV_MPU6880,
> > 	INV_MPU6000,
> > 	INV_MPU9150,
> > 	INV_MPU9250,
> > 	INV_MPU9255,
> > 	INV_ICM20608,
> > 	INV_ICM20608D,
> > 	INV_ICM20609,
> > 	INV_ICM20689,
> > 	INV_ICM20600,
> > 	INV_ICM20602,
> > 	INV_ICM20690,
> > 	INV_IAM20680,
> > 	INV_NUM_PARTS
> > };
> >
> > The new helper function
> 
> You mean for debugging? We don't need that in production.

That is sample code for iterating through id table to find max enum
and check against ZERO_OR_NULL_PTR

> 
> I think what you need is a coccinelle script to find these.

I need to explore using coccinelle script as I have n't tried before.

Cheers,
Biju 

Re: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Geert Uytterhoeven]

Hi Biju,

On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> >
> > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> >
> > ...
> >
> > > > I'm good with this approach, but make sure you checked the whole
> > > > kernel source tree for a such.
> > >
> > > Checking against 16 is too short I guess??
> > >
> > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> >
> > So, what does prevent us from moving that tables to use pointers?
>
> I think that will lead to ABI breakage(client->name vs id->name)
>
>         match = device_get_match_data(&client->dev);
>         if (match) {
>                 chip_type = (uintptr_t)match;
>                 name = client->name;
>         } else if (id) {
>                 chip_type = (enum inv_devices)
>                         id->driver_data;
>                 name = id->name;
>         } else {
>                 return -ENOSYS;
>         }

I don't consider that part of the ABI, as e.g. converting from board files
to DT would change the name.
In addition, using id->name breaks multiple instances of the same device.

I applaud more unification ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> Hi Biju,
> 
> On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type
> > >
> > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> > >
> > > ...
> > >
> > > > > I'm good with this approach, but make sure you checked the whole
> > > > > kernel source tree for a such.
> > > >
> > > > Checking against 16 is too short I guess??
> > > >
> > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > >
> > > So, what does prevent us from moving that tables to use pointers?
> >
> > I think that will lead to ABI breakage(client->name vs id->name)
> >
> >         match = device_get_match_data(&client->dev);
> >         if (match) {
> >                 chip_type = (uintptr_t)match;
> >                 name = client->name;
> >         } else if (id) {
> >                 chip_type = (enum inv_devices)
> >                         id->driver_data;
> >                 name = id->name;
> >         } else {
> >                 return -ENOSYS;
> >         }
> 
> I don't consider that part of the ABI, as e.g. converting from board files
> to DT would change the name.
> In addition, using id->name breaks multiple instances of the same device.

OK, then according to you this patch is ok [1]?

https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230807172548.258247-2-biju.das.jz@bp.renesas.com/

Cheers,
Biju

Re: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:

...

> > > > I'm good with this approach, but make sure you checked the whole
> > > > kernel source tree for a such.
> > >
> > > Checking against 16 is too short I guess??
> > >
> > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > 
> > So, what does prevent us from moving that tables to use pointers?
> 
> I think that will lead to ABI breakage(client->name vs id->name)
> 
> 	match = device_get_match_data(&client->dev);
> 	if (match) {
> 		chip_type = (uintptr_t)match;
> 		name = client->name;
> 	} else if (id) {
> 		chip_type = (enum inv_devices)
> 			id->driver_data;
> 		name = id->name;
> 	} else {
> 		return -ENOSYS;
> 	}


It's easy to work around (may be better fix can be found, haven't checked, just
what first comes to my mind):

	match ...
	name = match->name;

	/* If enumerated via firmware node, fix the ABI */
	if (dev_fwnode())
		client->name

> > > /*device enum */
> > > enum inv_devices {
> > > 	INV_MPU6050,
> > > 	INV_MPU6500,
> > > 	INV_MPU6515,
> > > 	INV_MPU6880,
> > > 	INV_MPU6000,
> > > 	INV_MPU9150,
> > > 	INV_MPU9250,
> > > 	INV_MPU9255,
> > > 	INV_ICM20608,
> > > 	INV_ICM20608D,
> > > 	INV_ICM20609,
> > > 	INV_ICM20689,
> > > 	INV_ICM20600,
> > > 	INV_ICM20602,
> > > 	INV_ICM20690,
> > > 	INV_IAM20680,
> > > 	INV_NUM_PARTS
> > > };
> > >
> > > The new helper function
> > 
> > You mean for debugging? We don't need that in production.
> 
> That is sample code for iterating through id table to find max enum
> and check against ZERO_OR_NULL_PTR

Much better with a coccinelle. You will find all or almost all occurrences
without too much effort done.

> > I think what you need is a coccinelle script to find these.
> 
> I need to explore using coccinelle script as I have n't tried before.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

Hi Andy Shevchenko,

Thanks for the feedback.

> Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:
> > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> 
> ...
> 
> > > > > I'm good with this approach, but make sure you checked the whole
> > > > > kernel source tree for a such.
> > > >
> > > > Checking against 16 is too short I guess??
> > > >
> > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > >
> > > So, what does prevent us from moving that tables to use pointers?
> >
> > I think that will lead to ABI breakage(client->name vs id->name)
> >
> > 	match = device_get_match_data(&client->dev);
> > 	if (match) {
> > 		chip_type = (uintptr_t)match;
> > 		name = client->name;
> > 	} else if (id) {
> > 		chip_type = (enum inv_devices)
> > 			id->driver_data;
> > 		name = id->name;
> > 	} else {
> > 		return -ENOSYS;
> > 	}
> 
> 
> It's easy to work around (may be better fix can be found, haven't checked,
> just what first comes to my mind):
> 
> 	match ...
> 	name = match->name;

The device_get_match_data()API returns matched data, so we cannot use that one here.

Maybe??

/* If enumerated via ID lookup, fix the ABI */
if (!dev_fwnode())
	name = id->name;

Cheers,
Biju

> 
> 	/* If enumerated via firmware node, fix the ABI */
> 	if (dev_fwnode())
> 		client->name
> 
> > > > /*device enum */
> > > > enum inv_devices {
> > > > 	INV_MPU6050,
> > > > 	INV_MPU6500,
> > > > 	INV_MPU6515,
> > > > 	INV_MPU6880,
> > > > 	INV_MPU6000,
> > > > 	INV_MPU9150,
> > > > 	INV_MPU9250,
> > > > 	INV_MPU9255,
> > > > 	INV_ICM20608,
> > > > 	INV_ICM20608D,
> > > > 	INV_ICM20609,
> > > > 	INV_ICM20689,
> > > > 	INV_ICM20600,
> > > > 	INV_ICM20602,
> > > > 	INV_ICM20690,
> > > > 	INV_IAM20680,
> > > > 	INV_NUM_PARTS
> > > > };
> > > >
> > > > The new helper function
> > >
> > > You mean for debugging? We don't need that in production.
> >
> > That is sample code for iterating through id table to find max enum
> > and check against ZERO_OR_NULL_PTR
> 
> Much better with a coccinelle. You will find all or almost all occurrences
> without too much effort done.
> 
> > > I think what you need is a coccinelle script to find these.
> >
> > I need to explore using coccinelle script as I have n't tried before.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

RE: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Biju Das]

> Subject: RE: [PATCH v7 0/4] Extend device_get_match_data() to struct
> bus_type
> 
> 
> Hi Andy Shevchenko,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> >
> > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:
> > > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to
> > > > struct bus_type On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das
> wrote:
> > > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:
> >
> > ...
> >
> > > > > > I'm good with this approach, but make sure you checked the
> > > > > > whole kernel source tree for a such.
> > > > >
> > > > > Checking against 16 is too short I guess??
> > > > >
> > > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.
> > > >
> > > > So, what does prevent us from moving that tables to use pointers?
> > >
> > > I think that will lead to ABI breakage(client->name vs id->name)
> > >
> > > 	match = device_get_match_data(&client->dev);
> > > 	if (match) {
> > > 		chip_type = (uintptr_t)match;
> > > 		name = client->name;
> > > 	} else if (id) {
> > > 		chip_type = (enum inv_devices)
> > > 			id->driver_data;
> > > 		name = id->name;
> > > 	} else {
> > > 		return -ENOSYS;
> > > 	}
> >
> >
> > It's easy to work around (may be better fix can be found, haven't
> > checked, just what first comes to my mind):
> >
> > 	match ...
> > 	name = match->name;
> 
> The device_get_match_data()API returns matched data, so we cannot use that
> one here.
> 
> Maybe??
> 
> /* If enumerated via ID lookup, fix the ABI */ if (!dev_fwnode())
> 	name = id->name;

Looks this will work.

/* If enumerated via firmware node, fix the ABI */
	if (dev_fwnode())
		name = client->name
	else
		name = id->name

Cheers,
Biju

Re: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 06:58:41AM +0000, Biju Das wrote:
> > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > bus_type
> > On Fri, Aug 11, 2023 at 02:46:10PM +0000, Biju Das wrote:

...

> > It's easy to work around (may be better fix can be found, haven't checked,
> > just what first comes to my mind):
> > 
> > 	match ...
> > 	name = match->name;
> 
> The device_get_match_data()API returns matched data, so we cannot use that one here.
> 
> Maybe??
> 
> /* If enumerated via ID lookup, fix the ABI */
> if (!dev_fwnode())
> 	name = id->name;

Yeah, you got the idea.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 0/4] Extend device_get_match_data() to struct bus_type

[Jonathan Cameron]

On Mon, 14 Aug 2023 15:12:24 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Biju,
> 
> On Fri, Aug 11, 2023 at 4:46 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > Subject: Re: [PATCH v7 0/4] Extend device_get_match_data() to struct
> > > bus_type
> > >
> > > On Fri, Aug 11, 2023 at 01:27:36PM +0000, Biju Das wrote:  
> > > > > On Thu, Aug 10, 2023 at 09:05:10AM +0000, Biju Das wrote:  
> > >
> > > ...
> > >  
> > > > > I'm good with this approach, but make sure you checked the whole
> > > > > kernel source tree for a such.  
> > > >
> > > > Checking against 16 is too short I guess??
> > > >
> > > > drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h has 18 enums.  
> > >
> > > So, what does prevent us from moving that tables to use pointers?  
> >
> > I think that will lead to ABI breakage(client->name vs id->name)
> >
> >         match = device_get_match_data(&client->dev);
> >         if (match) {
> >                 chip_type = (uintptr_t)match;
> >                 name = client->name;
> >         } else if (id) {
> >                 chip_type = (enum inv_devices)
> >                         id->driver_data;
> >                 name = id->name;
> >         } else {
> >                 return -ENOSYS;
> >         }  
> 
> I don't consider that part of the ABI, as e.g. converting from board files
> to DT would change the name.
> In addition, using id->name breaks multiple instances of the same device.

This has always been a mess as I wasn't paying attention a long time back
and we ended up with some client->name entries being used for iio_dev->name
whereas it should be the part number.

Using id->name is correct choice.  This is supposed to be the same for multiple
instances of the same device.  There is label and a bunch of other options
for differentiating them including their parent devices.

Problem is that is exported to userspace and often used as part of the
matching when a userspace tool is trying to find the device.

We could 'give it a go' by setting the name in teh switch statement in the core code
and hope no one notices but it's not ideal
https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c#L1597

Jonathan



> 
> I applaud more unification ;-)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>