[PATCH v4 0/5] Add TI TMP116 Support

[PATCH v4 0/5] Add TI TMP116 Support

[Marco Felsch]

Hi,

this small series adds the support for the TI TMP116 temperature sensor
which is predecessor of the TMP117 but still in production.

Marco Felsch (5):
  dt-bindings: iio: ti,tmp117: fix documentation link
  iio: temperature: tmp117: improve fallback capabilities
  dt-bindings: iio: ti,tmp117: add binding for the TMP116
  iio: temperature: tmp117: add TI TMP116 support
  iio: temperature: tmp117: cosmetic alignment cleanup

 .../bindings/iio/temperature/ti,tmp117.yaml   |   8 +-
 drivers/iio/temperature/tmp117.c              | 103 ++++++++++++------
 2 files changed, 76 insertions(+), 35 deletions(-)

-- 
2.30.2

[PATCH v4 1/5] dt-bindings: iio: ti,tmp117: fix documentation link

[Marco Felsch]

Fix the broken link to point to the correct homepage.

Fixes: 5e713b25d137 ("dt-bindings: iio: temperature: Add DT bindings for TMP117")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v4:
- no changes
v3:
- no changes
v2:
- added Krzysztof ack

 .../devicetree/bindings/iio/temperature/ti,tmp117.yaml          | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
index 347bc16a4671b..8d1ec4d39b28c 100644
--- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
@@ -9,7 +9,7 @@ title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
 description: |
     TI TMP117 - Digital temperature sensor with integrated NV memory that supports
     I2C interface.
-      https://www.ti.com/lit/gpn/tmp1
+      https://www.ti.com/lit/gpn/tmp117
 
 maintainers:
   - Puranjay Mohan <puranjay12@gmail.com>
-- 
2.30.2

[PATCH v4 2/5] iio: temperature: tmp117: improve fallback capabilities

[Marco Felsch]

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@pengutronix.de>
---
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);
+
 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;
+	if (match_data)
+		return match_data;
+
+	dev_err(&client->dev, "error: No valid fallback found\n");
+
+	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;
 
 	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",
-- 
2.30.2

[PATCH v4 3/5] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Marco Felsch]

The TMP116 is the predecessor of the TMP117.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
v4:
- sort alphabetical (Krzysztof)
- added Krzysztof's ab
v3:
- don't use tmp117 as fallback, therefore I didn't add Krzysztof
  rb.
v2:
- drop items from single enum

 .../devicetree/bindings/iio/temperature/ti,tmp117.yaml      | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
index 8d1ec4d39b28c..75f13cbcd72be 100644
--- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml
@@ -7,8 +7,9 @@ $schema: "http://devicetree.org/meta-schemas/core.yaml#"
 title: "TI TMP117 - Digital temperature sensor with integrated NV memory"
 
 description: |
-    TI TMP117 - Digital temperature sensor with integrated NV memory that supports
-    I2C interface.
+    TI TMP116/117 - Digital temperature sensor with integrated NV memory that
+    supports I2C interface.
+      https://www.ti.com/lit/gpn/tmp116
       https://www.ti.com/lit/gpn/tmp117
 
 maintainers:
@@ -17,6 +18,7 @@ maintainers:
 properties:
   compatible:
     enum:
+      - ti,tmp116
       - ti,tmp117
 
   reg:
-- 
2.30.2

[PATCH v4 4/5] iio: temperature: tmp117: add TI TMP116 support

[Marco Felsch]

The TMP116 is the predecessor of the TMP117. The TMP116 don't support
custom offset calibration data, instead this register is used as generic
EEPROM storage as well.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v4:
- split into two patches
  - 1st) handle fallback (Jonathan)
  - 2nd) this one, adding the support for tmp116

v3:
- use switch case within probe() as well
- don't hide smbus_read error within tmp117_identify()
- add dedicated compatible

v2:
- no changes

 drivers/iio/temperature/tmp117.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
index 4ddb8cf9a29ab..4915aceb66ee2 100644
--- a/drivers/iio/temperature/tmp117.c
+++ b/drivers/iio/temperature/tmp117.c
@@ -32,9 +32,11 @@
 #define TMP117_REG_DEVICE_ID		0xF
 
 #define TMP117_RESOLUTION_10UC		78125
-#define TMP117_DEVICE_ID		0x0117
 #define MICRODEGREE_PER_10MILLIDEGREE	10000
 
+#define TMP116_DEVICE_ID		0x1116
+#define TMP117_DEVICE_ID		0x0117
+
 struct tmp117_data {
 	struct i2c_client *client;
 	s16 calibbias;
@@ -106,6 +108,13 @@ static const struct iio_chan_spec tmp117_channels[] = {
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
+};
+
+static const struct iio_chan_spec tmp116_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
 	},
 };
 
@@ -115,12 +124,14 @@ static const struct iio_info tmp117_info = {
 };
 
 static const struct of_device_id tmp117_of_match[] = {
+	{ .compatible = "ti,tmp116", .data = (void *)TMP116_DEVICE_ID },
 	{ .compatible = "ti,tmp117", .data = (void *)TMP117_DEVICE_ID },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, tmp117_of_match);
 
 static const struct i2c_device_id tmp117_id[] = {
+	{ "tmp116", TMP116_DEVICE_ID },
 	{ "tmp117", TMP117_DEVICE_ID },
 	{ }
 };
@@ -136,6 +147,7 @@ static int tmp117_identify(struct i2c_client *client)
 		return dev_id;
 
 	switch (dev_id) {
+	case TMP116_DEVICE_ID:
 	case TMP117_DEVICE_ID:
 		return dev_id;
 	}
@@ -181,6 +193,11 @@ static int tmp117_probe(struct i2c_client *client)
 	indio_dev->info = &tmp117_info;
 
 	switch (dev_id) {
+	case TMP116_DEVICE_ID:
+		indio_dev->channels = tmp116_channels;
+		indio_dev->num_channels = ARRAY_SIZE(tmp116_channels);
+		indio_dev->name = "tmp116";
+		break;
 	case TMP117_DEVICE_ID:
 		indio_dev->channels = tmp117_channels;
 		indio_dev->num_channels = ARRAY_SIZE(tmp117_channels);
-- 
2.30.2

[PATCH v4 5/5] iio: temperature: tmp117: cosmetic alignment cleanup

[Marco Felsch]

Align the code correctly if possible and align the channel bit mask to
make it easier to read.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
v4:
- no changes
v3:
- no changes
v2:
- no changes

 drivers/iio/temperature/tmp117.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
index 4915aceb66ee2..5bc68c6392ff6 100644
--- a/drivers/iio/temperature/tmp117.c
+++ b/drivers/iio/temperature/tmp117.c
@@ -43,8 +43,8 @@ struct tmp117_data {
 };
 
 static int tmp117_read_raw(struct iio_dev *indio_dev,
-		struct iio_chan_spec const *channel, int *val,
-		int *val2, long mask)
+			   struct iio_chan_spec const *channel, int *val,
+			   int *val2, long mask)
 {
 	struct tmp117_data *data = iio_priv(indio_dev);
 	s32 ret;
@@ -52,7 +52,7 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		ret = i2c_smbus_read_word_swapped(data->client,
-						TMP117_REG_TEMP);
+						  TMP117_REG_TEMP);
 		if (ret < 0)
 			return ret;
 		*val = sign_extend32(ret, 15);
@@ -60,7 +60,7 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_CALIBBIAS:
 		ret = i2c_smbus_read_word_swapped(data->client,
-					TMP117_REG_TEMP_OFFSET);
+						  TMP117_REG_TEMP_OFFSET);
 		if (ret < 0)
 			return ret;
 		*val = sign_extend32(ret, 15);
@@ -82,9 +82,8 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
-static int tmp117_write_raw(struct iio_dev *indio_dev,
-		struct iio_chan_spec const *channel, int val,
-		int val2, long mask)
+static int tmp117_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec
+			    const *channel, int val, int val2, long mask)
 {
 	struct tmp117_data *data = iio_priv(indio_dev);
 	s16 off;
@@ -107,7 +106,9 @@ static const struct iio_chan_spec tmp117_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
+				      BIT(IIO_CHAN_INFO_CALIBBIAS) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
 };
 
 static const struct iio_chan_spec tmp116_channels[] = {
-- 
2.30.2

Re: [PATCH v4 2/5] iio: temperature: tmp117: improve fallback capabilities

[Jonathan Cameron]

On Mon, 20 Feb 2023 13:25:49 +0100
Marco Felsch <m.felsch@pengutronix.de> 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@pengutronix.de>
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",

Re: [PATCH v4 5/5] iio: temperature: tmp117: cosmetic alignment cleanup

[Jonathan Cameron]

On Mon, 20 Feb 2023 13:25:52 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Align the code correctly if possible and align the channel bit mask to
> make it easier to read.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Hi Marco,

Rest of the series looks good to me now.  We are early in the cycle, otherwise
I might just have made those tweaks suggested in patch 2 whilst applying to
make sure we didn't run out of time.  Given lots of time available I'm taking
the lazier approach and bouncing it back to you one last time!

Thanks,

Jonathan

> ---
> v4:
> - no changes
> v3:
> - no changes
> v2:
> - no changes
> 
>  drivers/iio/temperature/tmp117.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
> index 4915aceb66ee2..5bc68c6392ff6 100644
> --- a/drivers/iio/temperature/tmp117.c
> +++ b/drivers/iio/temperature/tmp117.c
> @@ -43,8 +43,8 @@ struct tmp117_data {
>  };
>  
>  static int tmp117_read_raw(struct iio_dev *indio_dev,
> -		struct iio_chan_spec const *channel, int *val,
> -		int *val2, long mask)
> +			   struct iio_chan_spec const *channel, int *val,
> +			   int *val2, long mask)
>  {
>  	struct tmp117_data *data = iio_priv(indio_dev);
>  	s32 ret;
> @@ -52,7 +52,7 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = i2c_smbus_read_word_swapped(data->client,
> -						TMP117_REG_TEMP);
> +						  TMP117_REG_TEMP);
>  		if (ret < 0)
>  			return ret;
>  		*val = sign_extend32(ret, 15);
> @@ -60,7 +60,7 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
>  
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		ret = i2c_smbus_read_word_swapped(data->client,
> -					TMP117_REG_TEMP_OFFSET);
> +						  TMP117_REG_TEMP_OFFSET);
>  		if (ret < 0)
>  			return ret;
>  		*val = sign_extend32(ret, 15);
> @@ -82,9 +82,8 @@ static int tmp117_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> -static int tmp117_write_raw(struct iio_dev *indio_dev,
> -		struct iio_chan_spec const *channel, int val,
> -		int val2, long mask)
> +static int tmp117_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec
> +			    const *channel, int val, int val2, long mask)
>  {
>  	struct tmp117_data *data = iio_priv(indio_dev);
>  	s16 off;
> @@ -107,7 +106,9 @@ static const struct iio_chan_spec tmp117_channels[] = {
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -			BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE),
> +				      BIT(IIO_CHAN_INFO_CALIBBIAS) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
>  };
>  
>  static const struct iio_chan_spec tmp116_channels[] = {

Re: [PATCH v4 2/5] iio: temperature: tmp117: improve fallback capabilities

[Marco Felsch]

Hi Jonathan,

On 23-02-26, Jonathan Cameron wrote:
> On Mon, 20 Feb 2023 13:25:49 +0100
> Marco Felsch <m.felsch@pengutronix.de> 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@pengutronix.de>
> 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",
> 
> 

Re: [PATCH v4 2/5] iio: temperature: tmp117: improve fallback capabilities

[Jonathan Cameron]

On Mon, 27 Feb 2023 19:44:57 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Jonathan,
> 
> On 23-02-26, Jonathan Cameron wrote:
> > On Mon, 20 Feb 2023 13:25:49 +0100
> > Marco Felsch <m.felsch@pengutronix.de> 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@pengutronix.de>  
> > 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",  
> > 
> >