Messages in this thread | | | Date | Wed, 22 Feb 2023 17:46:00 +0100 | From | Robin van der Gracht <> | Subject | Re: [PATCH v1 2/3] auxdisplay: ht16k33: Make use of device_get_match_data() |
| |
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
| |