[PATCH v3 0/4] Add TI TMP116 Support

[PATCH v3 0/4] 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 (4):
  dt-bindings: iio: ti,tmp117: fix documentation link
  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              | 63 +++++++++++++------
 2 files changed, 49 insertions(+), 22 deletions(-)

-- 
2.30.2

[PATCH v3 1/4] 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>
---
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 v3 2/4] 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>
---
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..a2f647fe0760c 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:
@@ -18,6 +19,7 @@ properties:
   compatible:
     enum:
       - ti,tmp117
+      - ti,tmp116
 
   reg:
     maxItems: 1
-- 
2.30.2

Re: [PATCH v3 2/4] dt-bindings: iio: ti,tmp117: add binding for the TMP116

[Krzysztof Kozlowski]

On 17/02/2023 10:37, Marco Felsch wrote:
> The TMP116 is the predecessor of the TMP117.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> 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..a2f647fe0760c 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:
> @@ -18,6 +19,7 @@ properties:
>    compatible:
>      enum:
>        - ti,tmp117
> +      - ti,tmp116

Don't add entries to the end, but keep some order. Less conflicts.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v3 3/4] 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>
---
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 | 46 ++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
index f9b8f2b570f6b..728538fbaf9ba 100644
--- a/drivers/iio/temperature/tmp117.c
+++ b/drivers/iio/temperature/tmp117.c
@@ -31,9 +31,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;
@@ -105,6 +107,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),
 	},
 };
 
@@ -120,25 +129,29 @@ static int tmp117_identify(struct i2c_client *client)
 	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");
+
+	switch (dev_id) {
+	case TMP116_DEVICE_ID:
+	case TMP117_DEVICE_ID:
+		return dev_id;
+	default:
+		dev_err(&client->dev, "TMP116/117 not found\n");
 		return -ENODEV;
 	}
-	return 0;
 }
 
 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,24 +161,35 @@ 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 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);
+		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", },
+	{ .compatible = "ti,tmp116", },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, tmp117_of_match);
 
 static const struct i2c_device_id tmp117_id[] = {
 	{ "tmp117", 0 },
+	{ "tmp116", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tmp117_id);
-- 
2.30.2

Re: [PATCH v3 3/4] iio: temperature: tmp117: add TI TMP116 support

[Jonathan Cameron]

On Fri, 17 Feb 2023 10:37:10 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> 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>

I failed to notice that the handling of fallback compatibles (not
related to the fact we can't use them for the TMP116 as discussed)
is less than ideal in the original driver.

Given it already doesn't work, I'm fine with handling that in a
a separate patch though if you would prefer. If you don't get to
it I might eventually do so.  There are a lot of cases of this
in IIO, but mostly we don't bother fixing them unless we are touching
the driver for other reasons.

I can fix the trivial alphabetical order thing up whilst applying.

Thanks,

Jonathan


> ---
> 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 | 46 ++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c
> index f9b8f2b570f6b..728538fbaf9ba 100644
> --- a/drivers/iio/temperature/tmp117.c
> +++ b/drivers/iio/temperature/tmp117.c
> @@ -31,9 +31,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;
> @@ -105,6 +107,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),
>  	},
>  };
>  
> @@ -120,25 +129,29 @@ static int tmp117_identify(struct i2c_client *client)
>  	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");
> +
> +	switch (dev_id) {
> +	case TMP116_DEVICE_ID:
> +	case TMP117_DEVICE_ID:
> +		return dev_id;
> +	default:
> +		dev_err(&client->dev, "TMP116/117 not found\n");

More a comment on the original code rather than your changes, but we should fix
if whilst here...

So this is always fun.  Ideally if we don't match a known device we should trust
the compatible provided.  The intent here is that if a new device turns up
with a different ID and does indeed have a valid fallback compatible then
the driver should work. It's fine to warn, but we shouldn't fail the probe.

As such a dance is needed.  First we need to associate some data with the
device IDs (see below)

>  		return -ENODEV;
>  	}
> -	return 0;
>  }
>  
>  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)

Fallback ID related. See above. If this happens we should not error out,
but rather fallback on assumption that the provided compatible is correct
even though we don't know the new dev_id.  So look up the data here.
Prefer data from the device_property_get_match_data() and if that fails
fallback to i2c_client_get_device_id() and the data field from that.


> +		return dev_id;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -148,24 +161,35 @@ 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 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);
> +		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", },
> +	{ .compatible = "ti,tmp116", },

Alphabetical order preferred.

Data here as below.

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, tmp117_of_match);
>  
>  static const struct i2c_device_id tmp117_id[] = {
>  	{ "tmp117", 0 },
> +	{ "tmp116", 0 },
Follow on from above: This needs data to distinguish the two so we can
support future fallback compatibles to one of these.
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, tmp117_id);

[PATCH v3 4/4] 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>
---
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 728538fbaf9ba..8fe87b37a0aca 100644
--- a/drivers/iio/temperature/tmp117.c
+++ b/drivers/iio/temperature/tmp117.c
@@ -42,8 +42,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;
@@ -51,7 +51,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);
@@ -59,7 +59,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);
@@ -81,9 +81,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;
@@ -106,7 +105,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