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.
> ---
> 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. Also, not a lot of point in putting error in the text of
a dev_err. Perhaps just "Unsupported device".
> +
> + 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",
