[PATCH v1 1/1] auxdisplay: ht16k33: Make use of i2c_get_match_data()

[PATCH v1 1/1] auxdisplay: ht16k33: Make use of i2c_get_match_data()

[Andy Shevchenko]

Get matching data in one step by switching to use i2c_get_match_data().
As a positive side effect the I²C ID table is in sync of OF one.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

I believe this is correct update. And here is why.

1) The documentation of the I2C user space device instantiation does not
mention a compatible string. it relies on the term 'name', which I believe
has direct link to the field .name in the struct i2c_device_id.

2) The above mentioned documentation says explicitly when user space
instantiation should be used. And for this driver it seems the only last
piece might be the case, i.e. prototyping / DIY configuration. For this
we don't need to rely on vendor ID anyway as in new supported hardware
the DT/ACPI emumeration is assumed.

3) Moreover, the currently used i2c_of_match_device() seems never be
considered to be used outside of i2c subsystem. Note, that it's being
used for device matching and probe, meaning firmware tables and board
info.

Also note, that the other (yes, it's ONLY 2 drivers call this API)
user of that API is going to be updated as well. Taking 3) into account
I think soon we remove that API from bein exported to the modules.

 drivers/auxdisplay/ht16k33.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index b00012a556fb..96ad9e972bd7 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16bf3a36c62fd8k33.c
@@ -657,7 +657,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;
@@ -672,9 +671,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)i2c_get_match_data(client);
+
 	i2c_set_clientdata(client, priv);
 
 	err = ht16k33_initialize(priv);
@@ -747,7 +745,9 @@ static void ht16k33_remove(struct i2c_client *client)
 }
 
 static const struct i2c_device_id ht16k33_i2c_match[] = {
-	{ "ht16k33" },
+	{ "3108", DISP_QUAD_7SEG },
+	{ "3130", DISP_QUAD_14SEG },
+	{ "ht16k33", DISP_MATRIX },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ht16k33_i2c_match);
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] auxdisplay: ht16k33: Make use of i2c_get_match_data()

[Robin van der Gracht]

Hi Andy,

On Mon, 30 Sep 2024 16:26:42 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Get matching data in one step by switching to use i2c_get_match_data().
> As a positive side effect the I²C ID table is in sync of OF one.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> I believe this is correct update. And here is why.
> 
> 1) The documentation of the I2C user space device instantiation does not
> mention a compatible string. it relies on the term 'name', which I believe
> has direct link to the field .name in the struct i2c_device_id.

Ack.

> 2) The above mentioned documentation says explicitly when user space
> instantiation should be used. And for this driver it seems the only last
> piece might be the case, i.e. prototyping / DIY configuration. For this
> we don't need to rely on vendor ID anyway as in new supported hardware
> the DT/ACPI emumeration is assumed.

Not sure what you mean here. It's statically declared in the device tree for the
imx6dl-victgo board [1].

> 3) Moreover, the currently used i2c_of_match_device() seems never be
> considered to be used outside of i2c subsystem. Note, that it's being
> used for device matching and probe, meaning firmware tables and board
> info.

4) It seems your change will also allows matching the device when it's
enumerated through ACPI.

> Also note, that the other (yes, it's ONLY 2 drivers call this API)
> user of that API is going to be updated as well. Taking 3) into account
> I think soon we remove that API from bein exported to the modules.

i2c_of_match_device_sysfs() also matches the user input with the driver
compatible string(s). Which is no longer true when i2c_get_match_data() is used.

Not sure if it make sense to match against the compatible string at this point
though. Because I'm not sure if the device can be instantiated through
user space by using the compatible string in the first place. If so, there would
only be 2 drives that can get their match data... 

The documentation doesn't mention the compatible string for user space
instantiating just to use "the name of the I2C device".

So maybe it's a good thing to remove that API or at least part of it.

Regardless, the change looks good to me.

Reviewed-by: Robin van der Gracht <robin@protonic.nl>

Regards,
Robin

1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/nxp/imx/imx6dl-victgo.dts?h=v6.12-rc1#n278

Re: [PATCH v1 1/1] auxdisplay: ht16k33: Make use of i2c_get_match_data()

[Andy Shevchenko]

On Wed, Oct 02, 2024 at 02:08:55PM +0200, Robin van der Gracht wrote:
> On Mon, 30 Sep 2024 16:26:42 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> > 2) The above mentioned documentation says explicitly when user space
> > instantiation should be used. And for this driver it seems the only last
> > piece might be the case, i.e. prototyping / DIY configuration. For this
> > we don't need to rely on vendor ID anyway as in new supported hardware
> > the DT/ACPI emumeration is assumed.
> 
> Not sure what you mean here. It's statically declared in the device tree for
> the imx6dl-victgo board [1].

It's about documentation that listed 4 cases when the user may instantiate
the I²C target device via sysfs.

> > 3) Moreover, the currently used i2c_of_match_device() seems never be
> > considered to be used outside of i2c subsystem. Note, that it's being
> > used for device matching and probe, meaning firmware tables and board
> > info.
> 
> 4) It seems your change will also allows matching the device when it's
> enumerated through ACPI.

Yes.

> > Also note, that the other (yes, it's ONLY 2 drivers call this API)
> > user of that API is going to be updated as well. Taking 3) into account
> > I think soon we remove that API from bein exported to the modules.
> 
> i2c_of_match_device_sysfs() also matches the user input with the driver
> compatible string(s). Which is no longer true when i2c_get_match_data() is used.

Yes, and that's not needed. And again, the documentation does not mentioned
that compatible strings are allowed or have to be recognized by the driver
when the instantiation is done via sysfs.

> Not sure if it make sense to match against the compatible string at this point
> though. Because I'm not sure if the device can be instantiated through
> user space by using the compatible string in the first place. If so, there would
> only be 2 drives that can get their match data... 

Yes, strictly speaking it makes a little sense, esp. if the device needs some
(mandatory) device properties. In any case this change doesn't prevent user from
doing that, just without vendor prefix.

> The documentation doesn't mention the compatible string for user space
> instantiating just to use "the name of the I2C device".

Exactly!

> So maybe it's a good thing to remove that API or at least part of it.

We can't remove it, but we can hide it from being used outside of I²C core.
That's what I mentioned in 3) above.

> Regardless, the change looks good to me.
> 
> Reviewed-by: Robin van der Gracht <robin@protonic.nl>

Thank you for the review!

> 1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/nxp/imx/imx6dl-victgo.dts?h=v6.12-rc1#n278

-- 
With Best Regards,
Andy Shevchenko