[PATCH v1 0/3] i2c: stop using i2c_of_match_device()

[PATCH v1 0/3] i2c: stop using i2c_of_match_device()

[Andy Shevchenko]

i2c_of_match_device() is used by core and a couple of drivers.
Instead, convert those drivers to use device_get_match_data()
and unexport i2c_of_match_device().

Andy Shevchenko (3):
  usb: typec: stusb160x: Make use of device_get_match_data()
  auxdisplay: ht16k33: Make use of device_get_match_data()
  i2c: Unexport i2c_of_match_device()

 drivers/auxdisplay/ht16k33.c  | 15 ++++++++++-----
 drivers/i2c/i2c-core-of.c     |  1 -
 drivers/i2c/i2c-core.h        |  9 +++++++++
 drivers/usb/typec/stusb160x.c |  8 ++++----
 include/linux/i2c.h           | 11 -----------
 5 files changed, 23 insertions(+), 21 deletions(-)

-- 
2.39.1

[PATCH v1 1/3] usb: typec: stusb160x: Make use of device_get_match_data()

[Andy Shevchenko]

Switching to use device_get_match_data() helps getting of
i2c_of_match_device() API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/typec/stusb160x.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/stusb160x.c b/drivers/usb/typec/stusb160x.c
index 494b371151e0..70d9f68d99c9 100644
--- a/drivers/usb/typec/stusb160x.c
+++ b/drivers/usb/typec/stusb160x.c
@@ -11,6 +11,7 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/usb/role.h>
@@ -633,9 +634,8 @@ MODULE_DEVICE_TABLE(of, stusb160x_of_match);
 
 static int stusb160x_probe(struct i2c_client *client)
 {
+	const struct regmap_config *regmap_config;
 	struct stusb160x *chip;
-	const struct of_device_id *match;
-	struct regmap_config *regmap_config;
 	struct fwnode_handle *fwnode;
 	int ret;
 
@@ -645,8 +645,8 @@ static int stusb160x_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, chip);
 
-	match = i2c_of_match_device(stusb160x_of_match, client);
-	regmap_config = (struct regmap_config *)match->data;
+	regmap_config = device_get_match_data(&client->dev);
+
 	chip->regmap = devm_regmap_init_i2c(client, regmap_config);
 	if (IS_ERR(chip->regmap)) {
 		ret = PTR_ERR(chip->regmap);
-- 
2.39.1

Re: [PATCH v1 1/3] usb: typec: stusb160x: Make use of device_get_match_data()

[Heikki Krogerus]

On Tue, Feb 21, 2023 at 03:33:05PM +0200, Andy Shevchenko wrote:
> Switching to use device_get_match_data() helps getting of
> i2c_of_match_device() API.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/stusb160x.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/stusb160x.c b/drivers/usb/typec/stusb160x.c
> index 494b371151e0..70d9f68d99c9 100644
> --- a/drivers/usb/typec/stusb160x.c
> +++ b/drivers/usb/typec/stusb160x.c
> @@ -11,6 +11,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/property.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/usb/role.h>
> @@ -633,9 +634,8 @@ MODULE_DEVICE_TABLE(of, stusb160x_of_match);
>  
>  static int stusb160x_probe(struct i2c_client *client)
>  {
> +	const struct regmap_config *regmap_config;
>  	struct stusb160x *chip;
> -	const struct of_device_id *match;
> -	struct regmap_config *regmap_config;
>  	struct fwnode_handle *fwnode;
>  	int ret;
>  
> @@ -645,8 +645,8 @@ static int stusb160x_probe(struct i2c_client *client)
>  
>  	i2c_set_clientdata(client, chip);
>  
> -	match = i2c_of_match_device(stusb160x_of_match, client);
> -	regmap_config = (struct regmap_config *)match->data;
> +	regmap_config = device_get_match_data(&client->dev);
> +
>  	chip->regmap = devm_regmap_init_i2c(client, regmap_config);
>  	if (IS_ERR(chip->regmap)) {
>  		ret = PTR_ERR(chip->regmap);

thanks,

-- 
heikki

[PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

Switching to use device_get_match_data() helps getting of
i2c_of_match_device() API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/auxdisplay/ht16k33.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 02425991c159..8e2fd2291ea4 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -60,7 +60,8 @@
 #define HT16K33_FB_SIZE		(HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW)
 
 enum display_type {
-	DISP_MATRIX = 0,
+	DISP_UNKNOWN = 0,
+	DISP_MATRIX,
 	DISP_QUAD_7SEG,
 	DISP_QUAD_14SEG,
 };
@@ -675,6 +676,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 		return err;
 
 	switch (priv->type) {
+	default:
 	case DISP_MATRIX:
 		/* not handled here */
 		err = -EINVAL;
@@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
 static int ht16k33_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	const struct of_device_id *id;
 	struct ht16k33_priv *priv;
 	uint32_t dft_brightness;
 	int err;
@@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	priv->client = client;
-	id = i2c_of_match_device(dev->driver->of_match_table, client);
-	if (id)
-		priv->type = (uintptr_t)id->data;
+	priv->type = (uintptr_t)device_get_match_data(dev);
+
 	i2c_set_clientdata(client, priv);
 
 	err = ht16k33_initialize(priv);
@@ -771,6 +771,9 @@ static int ht16k33_probe(struct i2c_client *client)
 		/* Segment Display */
 		err = ht16k33_seg_probe(dev, priv, dft_brightness);
 		break;
+	default:
+		/* Unknown display; enumerated via user space? */
+		err = 0;
 	}
 	return err;
 }
@@ -795,6 +798,8 @@ static void ht16k33_remove(struct i2c_client *client)
 		device_remove_file(&client->dev, &dev_attr_map_seg7);
 		device_remove_file(&client->dev, &dev_attr_map_seg14);
 		break;
+	default:
+		break;
 	}
 }
 
-- 
2.39.1

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:
> Switching to use device_get_match_data() helps getting of
> i2c_of_match_device() API.

...

> -	id = i2c_of_match_device(dev->driver->of_match_table, client);
> -	if (id)
> -		priv->type = (uintptr_t)id->data;
> +	priv->type = (uintptr_t)device_get_match_data(dev);

Looking closer the I²C ID table should provide DISP_MATRIX to keep default and
this needs to be not dropped.

So, the question is what to do with unknown type then, return -EINVAL from
probe()?

P.S. I would like to collect other comments anyway, so I will send a v2 later.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Robin van der Gracht]

Hello Andy,

On 2023-02-21 14:40, Andy Shevchenko wrote:
> On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:
>> Switching to use device_get_match_data() helps getting of
>> i2c_of_match_device() API.
> 
> ...
> 
>> -	id = i2c_of_match_device(dev->driver->of_match_table, client);
>> -	if (id)
>> -		priv->type = (uintptr_t)id->data;
>> +	priv->type = (uintptr_t)device_get_match_data(dev);
> 
> Looking closer the I²C ID table should provide DISP_MATRIX to keep 
> default and
> this needs to be not dropped.
> 
> So, the question is what to do with unknown type then, return -EINVAL 
> from
> probe()?

If you leave out your addition of the DISP_UNKNOWN type, the default 
type will
be DISP_MATRIX if no match is found, which is as it is now.

In that case the following change should suffice:

@@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev, 
struct ht16k33_priv *priv,
  static int ht16k33_probe(struct i2c_client *client)
  {
      struct device *dev = &client->dev;
-    const struct of_device_id *id;
      struct ht16k33_priv *priv;
      uint32_t dft_brightness;
      int err;
@@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client *client)
          return -ENOMEM;

      priv->client = client;
-    id = i2c_of_match_device(dev->driver->of_match_table, client);
-    if (id)
-        priv->type = (uintptr_t)id->data;
+    priv->type = (uintptr_t)device_get_match_data(dev);
+
      i2c_set_clientdata(client, priv);

      err = ht16k33_initialize(priv);

Or do you think falling back to DISP_MATRIX if no match is found is 
wrong?

Kind regards,
Robin

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

On Tue, Feb 21, 2023 at 05:10:00PM +0100, Robin van der Gracht wrote:
> On 2023-02-21 14:40, Andy Shevchenko wrote:
> > On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:
> > > Switching to use device_get_match_data() helps getting of
> > > i2c_of_match_device() API.

...

> > > -	id = i2c_of_match_device(dev->driver->of_match_table, client);
> > > -	if (id)
> > > -		priv->type = (uintptr_t)id->data;
> > > +	priv->type = (uintptr_t)device_get_match_data(dev);
> > 
> > Looking closer the I²C ID table should provide DISP_MATRIX to keep
> > default and this needs to be not dropped.
> > 
> > So, the question is what to do with unknown type then, return -EINVAL
> > from probe()?
> 
> If you leave out your addition of the DISP_UNKNOWN type, the default type
> will be DISP_MATRIX if no match is found, which is as it is now.
> 
> In that case the following change should suffice:
> 
> @@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev, struct
> ht16k33_priv *priv,
>  static int ht16k33_probe(struct i2c_client *client)
>  {
>      struct device *dev = &client->dev;
> -    const struct of_device_id *id;
>      struct ht16k33_priv *priv;
>      uint32_t dft_brightness;
>      int err;
> @@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client *client)
>          return -ENOMEM;
> 
>      priv->client = client;
> -    id = i2c_of_match_device(dev->driver->of_match_table, client);
> -    if (id)
> -        priv->type = (uintptr_t)id->data;
> +    priv->type = (uintptr_t)device_get_match_data(dev);
> +
>      i2c_set_clientdata(client, priv);
> 
>      err = ht16k33_initialize(priv);
> 
> Or do you think falling back to DISP_MATRIX if no match is found is wrong?

First of all, the I²C ID table should actually use DISP_MATRIX.

Second, there are two points:

- It would be nice to check if the OF ID table doesn't provide a setting
  (shouldn't we try I²C ID table and then, if still nothing, bail out?)

- The I²C ID table can be extended in the future with another entry which
  may want to have different default


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Robin van der Gracht]

On 2023-02-21 18:48, Andy Shevchenko wrote:
> On Tue, Feb 21, 2023 at 05:10:00PM +0100, Robin van der Gracht wrote:
>> On 2023-02-21 14:40, Andy Shevchenko wrote:
>> > On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:
>> > > Switching to use device_get_match_data() helps getting of
>> > > i2c_of_match_device() API.
> 
> ...
> 
>> > > -	id = i2c_of_match_device(dev->driver->of_match_table, client);
>> > > -	if (id)
>> > > -		priv->type = (uintptr_t)id->data;
>> > > +	priv->type = (uintptr_t)device_get_match_data(dev);
>> >
>> > Looking closer the I²C ID table should provide DISP_MATRIX to keep
>> > default and this needs to be not dropped.
>> >
>> > So, the question is what to do with unknown type then, return -EINVAL
>> > from probe()?
>> 
>> If you leave out your addition of the DISP_UNKNOWN type, the default 
>> type
>> will be DISP_MATRIX if no match is found, which is as it is now.
>> 
>> In that case the following change should suffice:
>> 
>> @@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev, 
>> struct
>> ht16k33_priv *priv,
>>  static int ht16k33_probe(struct i2c_client *client)
>>  {
>>      struct device *dev = &client->dev;
>> -    const struct of_device_id *id;
>>      struct ht16k33_priv *priv;
>>      uint32_t dft_brightness;
>>      int err;
>> @@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client 
>> *client)
>>          return -ENOMEM;
>> 
>>      priv->client = client;
>> -    id = i2c_of_match_device(dev->driver->of_match_table, client);
>> -    if (id)
>> -        priv->type = (uintptr_t)id->data;
>> +    priv->type = (uintptr_t)device_get_match_data(dev);
>> +
>>      i2c_set_clientdata(client, priv);
>> 
>>      err = ht16k33_initialize(priv);
>> 
>> Or do you think falling back to DISP_MATRIX if no match is found is 
>> wrong?
> 
> First of all, the I²C ID table should actually use DISP_MATRIX.
> 
> Second, there are two points:
> 
> - It would be nice to check if the OF ID table doesn't provide a 
> setting
>   (shouldn't we try I²C ID table and then, if still nothing, bail out?)
> 
> - The I²C ID table can be extended in the future with another entry 
> which
>   may want to have different default

For my understanding, please correct me if I'm wrong;

For all methods of instantiation during ht16k33 probe, 
i2c_of_match_device()
matches the compatible strings in the OF ID table due to a call to
i2c_of_match_device_sysfs().

device_get_match_data() only matches the compatible strings in the OF ID
table for devicetree instantiation because of_match_device() won't match
is there is no actual of_node.

So with only device_get_match_data() and a non devicetree instantiation,
priv->type will always be (uintptr_t)NULL = 0 = DISP_MATRIX.

Which effectively breaks i.e. user-space instantiation for other display
types which now do work due to i2c_of_match_device().
(so my suggestion above is not sufficient).

Are you proposing extending and searching the I2C ID table to work 
around that?

Kind regards,

-- 
Robin van der Gracht
Protonic Holland
Factorij 36
1689AL Zwaag
+31 (0)229 212928
https://www.protonic.nl

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

On Wed, Feb 22, 2023 at 05:46:00PM +0100, Robin van der Gracht wrote:
> On 2023-02-21 18:48, Andy Shevchenko wrote:
> > On Tue, Feb 21, 2023 at 05:10:00PM +0100, Robin van der Gracht wrote:
> > > On 2023-02-21 14:40, Andy Shevchenko wrote:
> > > > On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:

...

> > > > > -	id = i2c_of_match_device(dev->driver->of_match_table, client);
> > > > > -	if (id)
> > > > > -		priv->type = (uintptr_t)id->data;
> > > > > +	priv->type = (uintptr_t)device_get_match_data(dev);
> > > >
> > > > Looking closer the I²C ID table should provide DISP_MATRIX to keep
> > > > default and

> > > > this needs to be not dropped.

^^^^^ (1)

> > > > So, the question is what to do with unknown type then, return -EINVAL
> > > > from probe()?
> > > 
> > > If you leave out your addition of the DISP_UNKNOWN type, the default
> > > type
> > > will be DISP_MATRIX if no match is found, which is as it is now.
> > > 
> > > In that case the following change should suffice:
> > > 
> > > @@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev,
> > > struct
> > > ht16k33_priv *priv,
> > >  static int ht16k33_probe(struct i2c_client *client)
> > >  {
> > >      struct device *dev = &client->dev;
> > > -    const struct of_device_id *id;
> > >      struct ht16k33_priv *priv;
> > >      uint32_t dft_brightness;
> > >      int err;
> > > @@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client
> > > *client)
> > >          return -ENOMEM;
> > > 
> > >      priv->client = client;
> > > -    id = i2c_of_match_device(dev->driver->of_match_table, client);
> > > -    if (id)
> > > -        priv->type = (uintptr_t)id->data;
> > > +    priv->type = (uintptr_t)device_get_match_data(dev);
> > > +
> > >      i2c_set_clientdata(client, priv);
> > > 
> > >      err = ht16k33_initialize(priv);
> > > 
> > > Or do you think falling back to DISP_MATRIX if no match is found is
> > > wrong?
> > 
> > First of all, the I²C ID table should actually use DISP_MATRIX.
> > 
> > Second, there are two points:
> > 
> > - It would be nice to check if the OF ID table doesn't provide a setting
> >   (shouldn't we try I²C ID table and then, if still nothing, bail out?)
> > 
> > - The I²C ID table can be extended in the future with another entry
> > which
> >   may want to have different default
> 
> For my understanding, please correct me if I'm wrong;
> 
> For all methods of instantiation during ht16k33 probe, i2c_of_match_device()
> matches the compatible strings in the OF ID table due to a call to
> i2c_of_match_device_sysfs().
> 
> device_get_match_data() only matches the compatible strings in the OF ID
> table for devicetree instantiation because of_match_device() won't match
> is there is no actual of_node.

That's half-true. On ACPI based platforms we may have no of_node and match
against OF ID table.

> So with only device_get_match_data() and a non devicetree instantiation,
> priv->type will always be (uintptr_t)NULL = 0 = DISP_MATRIX.

Yes.

> Which effectively breaks i.e. user-space instantiation for other display
> types which now do work due to i2c_of_match_device().
> (so my suggestion above is not sufficient).
> 
> Are you proposing extending and searching the I2C ID table to work around
> that?

See (1) above. This is the downside I have noticed after sending this series.
So, the I²C ID table match has to be restored, but the above mentioned issues
with existing table are not gone, hence they need to be addressed in the next
version.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

+ Cc: OF bindings people for the mess with the IDs.

On Wed, Feb 22, 2023 at 07:01:40PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 22, 2023 at 05:46:00PM +0100, Robin van der Gracht wrote:
> > On 2023-02-21 18:48, Andy Shevchenko wrote:
> > > On Tue, Feb 21, 2023 at 05:10:00PM +0100, Robin van der Gracht wrote:
> > > > On 2023-02-21 14:40, Andy Shevchenko wrote:
> > > > > On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:

...

> > > > > > -	id = i2c_of_match_device(dev->driver->of_match_table, client);
> > > > > > -	if (id)
> > > > > > -		priv->type = (uintptr_t)id->data;
> > > > > > +	priv->type = (uintptr_t)device_get_match_data(dev);
> > > > >
> > > > > Looking closer the I²C ID table should provide DISP_MATRIX to keep
> > > > > default and
> 
> > > > > this needs to be not dropped.
> 
> ^^^^^ (1)
> 
> > > > > So, the question is what to do with unknown type then, return -EINVAL
> > > > > from probe()?
> > > > 
> > > > If you leave out your addition of the DISP_UNKNOWN type, the default
> > > > type
> > > > will be DISP_MATRIX if no match is found, which is as it is now.
> > > > 
> > > > In that case the following change should suffice:
> > > > 
> > > > @@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev,
> > > > struct
> > > > ht16k33_priv *priv,
> > > >  static int ht16k33_probe(struct i2c_client *client)
> > > >  {
> > > >      struct device *dev = &client->dev;
> > > > -    const struct of_device_id *id;
> > > >      struct ht16k33_priv *priv;
> > > >      uint32_t dft_brightness;
> > > >      int err;
> > > > @@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client
> > > > *client)
> > > >          return -ENOMEM;
> > > > 
> > > >      priv->client = client;
> > > > -    id = i2c_of_match_device(dev->driver->of_match_table, client);
> > > > -    if (id)
> > > > -        priv->type = (uintptr_t)id->data;
> > > > +    priv->type = (uintptr_t)device_get_match_data(dev);
> > > > +
> > > >      i2c_set_clientdata(client, priv);
> > > > 
> > > >      err = ht16k33_initialize(priv);
> > > > 
> > > > Or do you think falling back to DISP_MATRIX if no match is found is
> > > > wrong?
> > > 
> > > First of all, the I²C ID table should actually use DISP_MATRIX.
> > > 
> > > Second, there are two points:
> > > 
> > > - It would be nice to check if the OF ID table doesn't provide a setting
> > >   (shouldn't we try I²C ID table and then, if still nothing, bail out?)
> > > 
> > > - The I²C ID table can be extended in the future with another entry
> > > which
> > >   may want to have different default
> > 
> > For my understanding, please correct me if I'm wrong;
> > 
> > For all methods of instantiation during ht16k33 probe, i2c_of_match_device()
> > matches the compatible strings in the OF ID table due to a call to
> > i2c_of_match_device_sysfs().
> > 
> > device_get_match_data() only matches the compatible strings in the OF ID
> > table for devicetree instantiation because of_match_device() won't match
> > is there is no actual of_node.
> 
> That's half-true. On ACPI based platforms we may have no of_node and match
> against OF ID table.
> 
> > So with only device_get_match_data() and a non devicetree instantiation,
> > priv->type will always be (uintptr_t)NULL = 0 = DISP_MATRIX.
> 
> Yes.
> 
> > Which effectively breaks i.e. user-space instantiation for other display
> > types which now do work due to i2c_of_match_device().
> > (so my suggestion above is not sufficient).
> > 
> > Are you proposing extending and searching the I2C ID table to work around
> > that?
> 
> See (1) above. This is the downside I have noticed after sending this series.
> So, the I²C ID table match has to be restored, but the above mentioned issues
> with existing table are not gone, hence they need to be addressed in the next
> version.

I see now what you mean. So, we have even more issues in this driver:
- I²C table is not in sync with all devices supported
- the OF ID table seems has something really badly formed for adafruit
  (just a number after a comma)

The latter shows how broken it is. The I²C ID table mechanism is used as
a backward compatibility to the OF. Unfortunately, user space may not provide
the data except in form of DT overlays, so for the legacy enumeration we
have only device name, which is a set of 4 digits for adafruit case.

Now imagine if by some reason we will get adafruit2 (you name it) with
the same schema. How I²C framework can understand that you meant adafruit
and not adafruit2? Or did I miss something?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Krzysztof Kozlowski]

On 22/02/2023 18:20, Andy Shevchenko wrote:
>>
>>> Which effectively breaks i.e. user-space instantiation for other display
>>> types which now do work due to i2c_of_match_device().
>>> (so my suggestion above is not sufficient).
>>>
>>> Are you proposing extending and searching the I2C ID table to work around
>>> that?
>>
>> See (1) above. This is the downside I have noticed after sending this series.
>> So, the I²C ID table match has to be restored, but the above mentioned issues
>> with existing table are not gone, hence they need to be addressed in the next
>> version.
> 
> I see now what you mean. So, we have even more issues in this driver:
> - I²C table is not in sync with all devices supported

Does anything actually rely on i2c_device_id table? ACPI would match
either via ACPI or OF tables. All modern ARM systems (e.g. imx6) are
DT-based. Maybe just drop the I2C ID table?

> - the OF ID table seems has something really badly formed for adafruit
>   (just a number after a comma)

Maybe it is a model number? It was documented:
Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml

> 
> The latter shows how broken it is. The I²C ID table mechanism is used as
> a backward compatibility to the OF. Unfortunately, user space may not provide
> the data except in form of DT overlays, so for the legacy enumeration we
> have only device name, which is a set of 4 digits for adafruit case.
> 
> Now imagine if by some reason we will get adafruit2 (you name it) with
> the same schema. How I²C framework can understand that you meant adafruit
> and not adafruit2? Or did I miss something?
> 

Best regards,
Krzysztof

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

On Wed, Feb 22, 2023 at 07:46:25PM +0100, Krzysztof Kozlowski wrote:
> On 22/02/2023 18:20, Andy Shevchenko wrote:
> >>
> >>> Which effectively breaks i.e. user-space instantiation for other display
> >>> types which now do work due to i2c_of_match_device().
> >>> (so my suggestion above is not sufficient).
> >>>
> >>> Are you proposing extending and searching the I2C ID table to work around
> >>> that?
> >>
> >> See (1) above. This is the downside I have noticed after sending this series.
> >> So, the I²C ID table match has to be restored, but the above mentioned issues
> >> with existing table are not gone, hence they need to be addressed in the next
> >> version.
> > 
> > I see now what you mean. So, we have even more issues in this driver:
> > - I²C table is not in sync with all devices supported
> 
> Does anything actually rely on i2c_device_id table? ACPI would match
> either via ACPI or OF tables. All modern ARM systems (e.g. imx6) are
> DT-based. Maybe just drop the I2C ID table?

For I²C it's still possible to enumerate the device via sysfs, which is ABI.

> > - the OF ID table seems has something really badly formed for adafruit
> >   (just a number after a comma)
> 
> Maybe it is a model number? It was documented:
> Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml

Yes, it's not a problem for ACPI/DT platforms, the problem is for the above
way of enumeration, so if we have more than 1 manufacturer that uses plain
numbers for the model, I²C framework may not distinguish which driver to use.

I.o.w. the part after comma in the compatible strings of the I²C devices must
be unique globally to make that enumeration disambiguous.

> > The latter shows how broken it is. The I²C ID table mechanism is used as
> > a backward compatibility to the OF. Unfortunately, user space may not provide
> > the data except in form of DT overlays, so for the legacy enumeration we
> > have only device name, which is a set of 4 digits for adafruit case.
> > 
> > Now imagine if by some reason we will get adafruit2 (you name it) with
> > the same schema. How I²C framework can understand that you meant adafruit
> > and not adafruit2? Or did I miss something?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Geert Uytterhoeven]

Hi Andy,

On Wed, Feb 22, 2023 at 8:21 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Feb 22, 2023 at 07:46:25PM +0100, Krzysztof Kozlowski wrote:
> > On 22/02/2023 18:20, Andy Shevchenko wrote:
> > >>> Which effectively breaks i.e. user-space instantiation for other display
> > >>> types which now do work due to i2c_of_match_device().
> > >>> (so my suggestion above is not sufficient).
> > >>>
> > >>> Are you proposing extending and searching the I2C ID table to work around
> > >>> that?
> > >>
> > >> See (1) above. This is the downside I have noticed after sending this series.
> > >> So, the I²C ID table match has to be restored, but the above mentioned issues
> > >> with existing table are not gone, hence they need to be addressed in the next
> > >> version.
> > >
> > > I see now what you mean. So, we have even more issues in this driver:
> > > - I²C table is not in sync with all devices supported
> >
> > Does anything actually rely on i2c_device_id table? ACPI would match
> > either via ACPI or OF tables. All modern ARM systems (e.g. imx6) are
> > DT-based. Maybe just drop the I2C ID table?
>
> For I²C it's still possible to enumerate the device via sysfs, which is ABI.

Yes, and AFAIK, that worked fine. E.g.

    echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device

Cfr. https://lore.kernel.org/all/20211019144520.3613926-3-geert@linux-m68k.org/

Note that that example actually includes the manufacturer.
I didn't check whether the I2C core takes that part into account when
matching, or just strips it.

> > > - the OF ID table seems has something really badly formed for adafruit
> > >   (just a number after a comma)
> >
> > Maybe it is a model number? It was documented:
> > Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
>
> Yes, it's not a problem for ACPI/DT platforms, the problem is for the above
> way of enumeration, so if we have more than 1 manufacturer that uses plain
> numbers for the model, I²C framework may not distinguish which driver to use.
>
> I.o.w. the part after comma in the compatible strings of the I²C devices must
> be unique globally to make that enumeration disambiguous.

Which is not unique to this driver?
I bet you can find other compatible values that become non-unique
after stripping the manufacturer.

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 v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Andy Shevchenko]

On Thu, Feb 23, 2023 at 10:55:15AM +0100, Geert Uytterhoeven wrote:
> On Wed, Feb 22, 2023 at 8:21 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Wed, Feb 22, 2023 at 07:46:25PM +0100, Krzysztof Kozlowski wrote:

...

> > I.o.w. the part after comma in the compatible strings of the I²C devices must
> > be unique globally to make that enumeration disambiguous.
> 
> Which is not unique to this driver?
> I bet you can find other compatible values that become non-unique
> after stripping the manufacturer.

Yes, exactly my point.
So this all schema is error prone. Hence I would not rely on it at all.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data()

[Robin van der Gracht]

On 2023-02-22 18:20, Andy Shevchenko wrote:
> + Cc: OF bindings people for the mess with the IDs.
> 
> On Wed, Feb 22, 2023 at 07:01:40PM +0200, Andy Shevchenko wrote:
>> On Wed, Feb 22, 2023 at 05:46:00PM +0100, Robin van der Gracht wrote:
>> > On 2023-02-21 18:48, Andy Shevchenko wrote:
>> > > On Tue, Feb 21, 2023 at 05:10:00PM +0100, Robin van der Gracht wrote:
>> > > > On 2023-02-21 14:40, Andy Shevchenko wrote:
>> > > > > On Tue, Feb 21, 2023 at 03:33:06PM +0200, Andy Shevchenko wrote:
> 
> ...
> 
>> > > > > > -	id = i2c_of_match_device(dev->driver->of_match_table, client);
>> > > > > > -	if (id)
>> > > > > > -		priv->type = (uintptr_t)id->data;
>> > > > > > +	priv->type = (uintptr_t)device_get_match_data(dev);
>> > > > >
>> > > > > Looking closer the I²C ID table should provide DISP_MATRIX to keep
>> > > > > default and
>> 
>> > > > > this needs to be not dropped.
>> 
>> ^^^^^ (1)
>> 
>> > > > > So, the question is what to do with unknown type then, return -EINVAL
>> > > > > from probe()?
>> > > >
>> > > > If you leave out your addition of the DISP_UNKNOWN type, the default
>> > > > type
>> > > > will be DISP_MATRIX if no match is found, which is as it is now.
>> > > >
>> > > > In that case the following change should suffice:
>> > > >
>> > > > @@ -713,7 +715,6 @@ static int ht16k33_seg_probe(struct device *dev,
>> > > > struct
>> > > > ht16k33_priv *priv,
>> > > >  static int ht16k33_probe(struct i2c_client *client)
>> > > >  {
>> > > >      struct device *dev = &client->dev;
>> > > > -    const struct of_device_id *id;
>> > > >      struct ht16k33_priv *priv;
>> > > >      uint32_t dft_brightness;
>> > > >      int err;
>> > > > @@ -728,9 +729,8 @@ static int ht16k33_probe(struct i2c_client
>> > > > *client)
>> > > >          return -ENOMEM;
>> > > >
>> > > >      priv->client = client;
>> > > > -    id = i2c_of_match_device(dev->driver->of_match_table, client);
>> > > > -    if (id)
>> > > > -        priv->type = (uintptr_t)id->data;
>> > > > +    priv->type = (uintptr_t)device_get_match_data(dev);
>> > > > +
>> > > >      i2c_set_clientdata(client, priv);
>> > > >
>> > > >      err = ht16k33_initialize(priv);
>> > > >
>> > > > Or do you think falling back to DISP_MATRIX if no match is found is
>> > > > wrong?
>> > >
>> > > First of all, the I²C ID table should actually use DISP_MATRIX.
>> > >
>> > > Second, there are two points:
>> > >
>> > > - It would be nice to check if the OF ID table doesn't provide a setting
>> > >   (shouldn't we try I²C ID table and then, if still nothing, bail out?)
>> > >
>> > > - The I²C ID table can be extended in the future with another entry
>> > > which
>> > >   may want to have different default
>> >
>> > For my understanding, please correct me if I'm wrong;
>> >
>> > For all methods of instantiation during ht16k33 probe, i2c_of_match_device()
>> > matches the compatible strings in the OF ID table due to a call to
>> > i2c_of_match_device_sysfs().
>> >
>> > device_get_match_data() only matches the compatible strings in the OF ID
>> > table for devicetree instantiation because of_match_device() won't match
>> > is there is no actual of_node.
>> 
>> That's half-true. On ACPI based platforms we may have no of_node and 
>> match
>> against OF ID table.
>> 
>> > So with only device_get_match_data() and a non devicetree instantiation,
>> > priv->type will always be (uintptr_t)NULL = 0 = DISP_MATRIX.
>> 
>> Yes.
>> 
>> > Which effectively breaks i.e. user-space instantiation for other display
>> > types which now do work due to i2c_of_match_device().
>> > (so my suggestion above is not sufficient).
>> >
>> > Are you proposing extending and searching the I2C ID table to work around
>> > that?
>> 
>> See (1) above. This is the downside I have noticed after sending this 
>> series.
>> So, the I²C ID table match has to be restored, but the above mentioned 
>> issues
>> with existing table are not gone, hence they need to be addressed in 
>> the next
>> version.
> 
> I see now what you mean. So, we have even more issues in this driver:
> - I²C table is not in sync with all devices supported
> - the OF ID table seems has something really badly formed for adafruit
>   (just a number after a comma)
> 
> The latter shows how broken it is. The I²C ID table mechanism is used 
> as
> a backward compatibility to the OF. Unfortunately, user space may not 
> provide
> the data except in form of DT overlays, so for the legacy enumeration 
> we
> have only device name, which is a set of 4 digits for adafruit case.
> 
> Now imagine if by some reason we will get adafruit2 (you name it) with
> the same schema. How I²C framework can understand that you meant 
> adafruit
> and not adafruit2? Or did I miss something?

I agree.

I've added Geert Uytterhoeven to the CC. He added support for the 
adafruit
segment displays. Maybe he has a comment on this.

Kind regards,
Robin van der Gracht

[PATCH v1 3/3] i2c: Unexport i2c_of_match_device()

[Andy Shevchenko]

i2c_of_match_device() is not used anymore outside of I²C framework,
unexport it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/i2c-core-of.c |  1 -
 drivers/i2c/i2c-core.h    |  9 +++++++++
 include/linux/i2c.h       | 11 -----------
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core-of.c b/drivers/i2c/i2c-core-of.c
index bce6b796e04c..29102c882635 100644
--- a/drivers/i2c/i2c-core-of.c
+++ b/drivers/i2c/i2c-core-of.c
@@ -157,7 +157,6 @@ const struct of_device_id
 
 	return i2c_of_match_device_sysfs(matches, client);
 }
-EXPORT_SYMBOL_GPL(i2c_of_match_device);
 
 #if IS_ENABLED(CONFIG_OF_DYNAMIC)
 static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 1247e6e6e975..3e22a0645e2f 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -82,8 +82,17 @@ static inline void i2c_acpi_remove_space_handler(struct i2c_adapter *adapter) {
 
 #ifdef CONFIG_OF
 void of_i2c_register_devices(struct i2c_adapter *adap);
+const struct of_device_id *i2c_of_match_device(const struct of_device_id *matches,
+					       struct i2c_client *client);
+
 #else
 static inline void of_i2c_register_devices(struct i2c_adapter *adap) { }
+static inline
+const struct of_device_id *i2c_of_match_device(const struct of_device_id *matches,
+					       struct i2c_client *client)
+{
+	return NULL;
+}
 #endif
 extern struct notifier_block i2c_of_notifier;
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 500404d85141..17b2cbb40a55 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -993,10 +993,6 @@ static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node
 	return i2c_get_adapter_by_fwnode(of_fwnode_handle(node));
 }
 
-const struct of_device_id
-*i2c_of_match_device(const struct of_device_id *matches,
-		     struct i2c_client *client);
-
 int of_i2c_get_board_info(struct device *dev, struct device_node *node,
 			  struct i2c_board_info *info);
 
@@ -1017,13 +1013,6 @@ static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node
 	return NULL;
 }
 
-static inline const struct of_device_id
-*i2c_of_match_device(const struct of_device_id *matches,
-		     struct i2c_client *client)
-{
-	return NULL;
-}
-
 static inline int of_i2c_get_board_info(struct device *dev,
 					struct device_node *node,
 					struct i2c_board_info *info)
-- 
2.39.1

Re: [PATCH v1 0/3] i2c: stop using i2c_of_match_device()

[Andy Shevchenko]

On Tue, Feb 21, 2023 at 03:33:04PM +0200, Andy Shevchenko wrote:
> i2c_of_match_device() is used by core and a couple of drivers.
> Instead, convert those drivers to use device_get_match_data()
> and unexport i2c_of_match_device().

After a good discussion and reading a bit deeper some code,
self-NAK for this series. We seems need to be more smart.

-- 
With Best Regards,
Andy Shevchenko