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.
> 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.
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",
>
>
