On Mon, 27 Feb 2023 19:44:57 +0100
Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
> Hi Jonathan,
>
> On 23-02-26, Jonathan Cameron wrote:
> > On Mon, 20 Feb 2023 13:25:49 +0100
> > Marco Felsch <m.felsch@xxxxxxxxxxxxxx> wrote:
> >
> > > Don't error if the device-id found don't match the device-id for the
> > > TMP117 sensor since other TMPxxx might be compatible to the TMP117. The
> > > fallback mechanism tries to gather the required information from the
> > > of_device_id or from the i2c_client information.
> > >
> > > The commit also prepares the driver for adding new devices more easily
> > > by making use of switch-case at the relevant parts.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > Hi Marco,
> >
> > Thanks for doing this. A small things inline.
>
> please see my comments below.
>
> > > ---
> > > v4:
> > > - new patch to implement possible fallback (Jonathan)
> > >
> > > drivers/iio/temperature/tmp117.c | 67 +++++++++++++++++++++-----------
> > > 1 file changed, 44 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
> > > index f9b8f2b570f6b..4ddb8cf9a29ab 100644
> > > --- a/drivers/iio/temperature/tmp117.c
> > > +++ b/drivers/iio/temperature/tmp117.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/types.h>
> > > #include <linux/kernel.h>
> > > #include <linux/limits.h>
> > > +#include <linux/property.h>
> > >
> > > #include <linux/iio/iio.h>
> > >
> > > @@ -113,32 +114,60 @@ static const struct iio_info tmp117_info = {
> > > .write_raw = tmp117_write_raw,
> > > };
> > >
> > > +static const struct of_device_id tmp117_of_match[] = {
> > > + { .compatible = "ti,tmp117", .data = (void *)TMP117_DEVICE_ID },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, tmp117_of_match);
> > > +
> > > +static const struct i2c_device_id tmp117_id[] = {
> > > + { "tmp117", TMP117_DEVICE_ID },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, tmp117_id);
> >
> > As below. There is an easy way to avoid having to move these.
> >
> > > +
> > > static int tmp117_identify(struct i2c_client *client)
> > > {
> > > + unsigned long match_data;
> > > int dev_id;
> > >
> > > dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID);
> > > if (dev_id < 0)
> > > return dev_id;
> > > - if (dev_id != TMP117_DEVICE_ID) {
> > > - dev_err(&client->dev, "TMP117 not found\n");
> > > - return -ENODEV;
> > > +
> > > + switch (dev_id) {
> > > + case TMP117_DEVICE_ID:
> > > + return dev_id;
> > > }
> > > - return 0;
> > > +
> > > + dev_info(&client->dev, "Unknown device id (0x%x), use fallback compatible\n",
> > > + dev_id);
> > > +
> > > + match_data = (uintptr_t)device_get_match_data(&client->dev);
> > > + if (match_data)
> > > + return match_data;
> > > +
> > > + match_data = i2c_match_id(tmp117_id, client)->driver_data;
> >
> > Whilst correct, i2c_client_get_device_id() avoids the need
> > to move tmp117_id up to where you have by getting to that table via
> > the driver structure. That will simplify this patch a fair bit.
> >
> > > + if (match_data)
> > > + return match_data;
> > > +
> > > + dev_err(&client->dev, "error: No valid fallback found\n");
> >
> > This is a little misleading as fallback only applies to the device tree
> > path.
>
> Since we support the i2c_device_id table as well, this is not 100% true.
I guess it's semantics, but the error message implies that a fallback is
possible. That only exists for one of the possible paths, all of which
failed to get to here. Hence suggestion of more generic message.
>
> > Also, not a lot of point in putting error in the text of
> > a dev_err. Perhaps just "Unsupported device".
>
> dev_err() does not print a error on the commandline. If something went
> wrong I tend to "dmesg|grep -i err" or "dmesg|grep -i fail". Therefore I
> added the error keyword here. But I can change the message to "Error:
> unsupported device" if this is okay for you.
Ok. I guess I was assuming some parsing that used the log level.
>
> Regards,
> Marco
>
> > > +
> > > + return -ENODEV;
> > > }
> > >
> > > static int tmp117_probe(struct i2c_client *client)
> > > {
> > > struct tmp117_data *data;
> > > struct iio_dev *indio_dev;
> > > - int ret;
> > > + int dev_id;
> > >
> > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > > return -EOPNOTSUPP;
> > >
> > > - ret = tmp117_identify(client);
> > > - if (ret < 0)
> > > - return ret;
> > > + dev_id = tmp117_identify(client);
> > > + if (dev_id < 0)
> > > + return dev_id;
> >
> > I'd keep it in ret until you know it's good. Reduces churn and is nicer
> > code in general, though one more line.
> >
> > dev_id = ret;
> >
> > >
> > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > if (!indio_dev)
> > > @@ -148,28 +177,20 @@ static int tmp117_probe(struct i2c_client *client)
> > > data->client = client;
> > > data->calibbias = 0;
> > >
> > > - indio_dev->name = "tmp117";
> > > indio_dev->modes = INDIO_DIRECT_MODE;
> > > indio_dev->info = &tmp117_info;
> > >
> > > - indio_dev->channels = tmp117_channels;
> > > - indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> > > + switch (dev_id) {
> > > + case TMP117_DEVICE_ID:
> > > + indio_dev->channels = tmp117_channels;
> > > + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
> > > + indio_dev->name = "tmp117";
> > > + break;
> > > + }
> > >
> > > return devm_iio_device_register(&client->dev, indio_dev);
> > > }
> > >
> > > -static const struct of_device_id tmp117_of_match[] = {
> > > - { .compatible = "ti,tmp117", },
> > > - { }
> > > -};
> > > -MODULE_DEVICE_TABLE(of, tmp117_of_match);
> > > -
> > > -static const struct i2c_device_id tmp117_id[] = {
> > > - { "tmp117", 0 },
> > > - { }
> > > -};
> > > -MODULE_DEVICE_TABLE(i2c, tmp117_id);
> > > -
> > > static struct i2c_driver tmp117_driver = {
> > > .driver = {
> > > .name = "tmp117",
> >
> >
