[PATCH 0/2] iio: temperature: tmp006: support for drdy irq

[PATCH 0/2] iio: temperature: tmp006: support for drdy irq

[Antoni Pokusinski]

This patch series adds support for the data ready interrupt of tmp006
sensor. The interrupt line is pulled down once there is a measurement
available to be read. Hence, triggered buffers are used in order to
support continuous data capture for the sensor.

Antoni Pokusinski (2):
  iio: temperature: tmp006: add triggered buffer support
  dt-bindings: iio: temperature: tmp006: document interrupt

 .../bindings/iio/temperature/ti,tmp006.yaml   |   6 +
 drivers/iio/temperature/Kconfig               |   2 +
 drivers/iio/temperature/tmp006.c              | 116 ++++++++++++++++--
 3 files changed, 117 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/2] iio: temperature: tmp006: add triggered buffer support

[Antoni Pokusinski]

Add support for continuous data capture using triggered buffers for the
tmp006 sensor. The device features a "data ready" interrupt line which
is pulled down once a new measurement is ready to be read.

Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
Note:
For some reason, the checkpatch.pl returns a warning for this patch:

  WARNING: Missing a blank line after declarations
  #156: FILE: drivers/iio/temperature/tmp006.c:253:
  +               s16 channels[2];
  +               s64 ts __aligned(8);

It also suggests that the following code with a blank line is correct:

	struct {
		s16 channels[2];

		s64 ts __aligned(8);
	} scan;

I left the code as it was (with the WARNING) since it feels obviously
more natural.
---
 drivers/iio/temperature/Kconfig  |   2 +
 drivers/iio/temperature/tmp006.c | 116 +++++++++++++++++++++++++++++--
 2 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index ed0e4963362f..1244d8e17d50 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -91,6 +91,8 @@ config MLX90635
 config TMP006
 	tristate "TMP006 infrared thermopile sensor"
 	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	help
 	  If you say yes here you get support for the Texas Instruments
 	  TMP006 infrared thermopile sensor.
diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
index 6d8d661f0c82..183b30a41573 100644
--- a/drivers/iio/temperature/tmp006.c
+++ b/drivers/iio/temperature/tmp006.c
@@ -7,8 +7,6 @@
  * Driver for the Texas Instruments I2C 16-bit IR thermopile sensor
  *
  * (7-bit I2C slave address 0x40, changeable via ADR pins)
- *
- * TODO: data ready irq
  */
 
 #include <linux/err.h>
@@ -21,6 +19,9 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #define TMP006_VOBJECT 0x00
 #define TMP006_TAMBIENT 0x01
@@ -45,6 +46,7 @@
 struct tmp006_data {
 	struct i2c_client *client;
 	u16 config;
+	struct iio_trigger *drdy_trig;
 };
 
 static int tmp006_read_measurement(struct tmp006_data *data, u8 reg)
@@ -81,15 +83,21 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
 		if (channel->type == IIO_VOLTAGE) {
 			/* LSB is 156.25 nV */
 			ret = tmp006_read_measurement(data, TMP006_VOBJECT);
+			iio_device_release_direct_mode(indio_dev);
 			if (ret < 0)
 				return ret;
 			*val = sign_extend32(ret, 15);
 		} else if (channel->type == IIO_TEMP) {
 			/* LSB is 0.03125 degrees Celsius */
 			ret = tmp006_read_measurement(data, TMP006_TAMBIENT);
+			iio_device_release_direct_mode(indio_dev);
 			if (ret < 0)
 				return ret;
 			*val = sign_extend32(ret, 15) >> TMP006_TAMBIENT_SHIFT;
@@ -128,7 +136,7 @@ static int tmp006_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct tmp006_data *data = iio_priv(indio_dev);
-	int i;
+	int ret, i;
 
 	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
 		return -EINVAL;
@@ -136,13 +144,19 @@ static int tmp006_write_raw(struct iio_dev *indio_dev,
 	for (i = 0; i < ARRAY_SIZE(tmp006_freqs); i++)
 		if ((val == tmp006_freqs[i][0]) &&
 		    (val2 == tmp006_freqs[i][1])) {
+			ret = iio_device_claim_direct_mode(indio_dev);
+			if (ret)
+				return ret;
+
 			data->config &= ~TMP006_CONFIG_CR_MASK;
 			data->config |= i << TMP006_CONFIG_CR_SHIFT;
 
-			return i2c_smbus_write_word_swapped(data->client,
-							    TMP006_CONFIG,
-							    data->config);
+			ret = i2c_smbus_write_word_swapped(data->client,
+							   TMP006_CONFIG,
+							   data->config);
 
+			iio_device_release_direct_mode(indio_dev);
+			return ret;
 		}
 	return -EINVAL;
 }
@@ -164,13 +178,29 @@ static const struct iio_chan_spec tmp006_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_BE
+		}
 	},
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
-	}
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 14,
+			.storagebits = 16,
+			.shift = TMP006_TAMBIENT_SHIFT,
+			.endianness = IIO_BE
+		}
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2)
 };
 
 static const struct iio_info tmp006_info = {
@@ -213,6 +243,49 @@ static void tmp006_powerdown_cleanup(void *dev)
 	tmp006_power(dev, false);
 }
 
+static irqreturn_t tmp006_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct tmp006_data *data = iio_priv(indio_dev);
+	struct {
+		s16 channels[2];
+		s64 ts __aligned(8);
+	} scan;
+
+	scan.channels[0] = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT);
+	if (scan.channels[0] < 0)
+		goto err;
+
+	scan.channels[1] = i2c_smbus_read_word_data(data->client, TMP006_TAMBIENT);
+	if (scan.channels[1] < 0)
+		goto err;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
+					   iio_get_time_ns(indio_dev));
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
+static int tmp006_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct tmp006_data *data = iio_priv(indio_dev);
+
+	if (state)
+		data->config |= TMP006_CONFIG_DRDY_EN;
+	else
+		data->config &= ~TMP006_CONFIG_DRDY_EN;
+
+	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
+		data->config);
+}
+
+static const struct iio_trigger_ops tmp006_trigger_ops = {
+	.set_trigger_state = tmp006_set_trigger_state,
+};
+
 static int tmp006_probe(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev;
@@ -258,6 +331,35 @@ static int tmp006_probe(struct i2c_client *client)
 	if (ret < 0)
 		return ret;
 
+	if (client->irq > 0) {
+		data->drdy_trig = devm_iio_trigger_alloc(&client->dev,
+							 "%s-dev%d",
+							 indio_dev->name,
+							 iio_device_id(indio_dev));
+		if (!data->drdy_trig)
+			return -ENOMEM;
+
+		data->drdy_trig->ops = &tmp006_trigger_ops;
+		iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
+		ret = iio_trigger_register(data->drdy_trig);
+		if (ret)
+			return ret;
+
+		indio_dev->trig = iio_trigger_get(data->drdy_trig);
+
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						iio_trigger_generic_data_rdy_poll,
+						NULL,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						"tmp006_irq",
+						data->drdy_trig);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
+					      tmp006_trigger_handler, NULL);
+
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
-- 
2.25.1

[PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt

[Antoni Pokusinski]

Update the example with interrupt bindings and add "interrupts" property

Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
---
 .../devicetree/bindings/iio/temperature/ti,tmp006.yaml      | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp006.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp006.yaml
index d43002b9bfdc..590f50ba3a31 100644
--- a/Documentation/devicetree/bindings/iio/temperature/ti,tmp006.yaml
+++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp006.yaml
@@ -23,6 +23,9 @@ properties:
   vdd-supply:
     description: provide VDD power to the sensor.
 
+  interrupts:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -31,6 +34,7 @@ additionalProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/irq.h>
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -38,5 +42,7 @@ examples:
             compatible = "ti,tmp006";
             reg = <0x40>;
             vdd-supply = <&ldo4_reg>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
         };
     };
-- 
2.25.1

Re: [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt

[Krzysztof Kozlowski]

On 02/09/2024 14:59, Antoni Pokusinski wrote:
> Update the example with interrupt bindings and add "interrupts" property

But why? Commit msg should answer this question. E.g. because hardware
has interrupts or any other hardware-related reason

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt

[Krzysztof Kozlowski]

On Mon, Sep 02, 2024 at 06:04:15PM +0200, Krzysztof Kozlowski wrote:
> On 02/09/2024 14:59, Antoni Pokusinski wrote:
> > Update the example with interrupt bindings and add "interrupts" property
> 
> But why? Commit msg should answer this question. E.g. because hardware
> has interrupts or any other hardware-related reason
> 

And please point me which pin on the diagram/schematics is the interrupt
pin.

Best regards,
Krzysztof

Re: [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support

[Jonathan Cameron]

On Mon,  2 Sep 2024 14:59:47 +0200
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> Add support for continuous data capture using triggered buffers for the
> tmp006 sensor. The device features a "data ready" interrupt line which
> is pulled down once a new measurement is ready to be read.
> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
> ---
> Note:
> For some reason, the checkpatch.pl returns a warning for this patch:

Don't worry - it's a well known checkpatch false positive but the case is obscure
enough no one ever bothered 'fixing' it.

Ignore it.

However, I just merged a patch from Andy providing
aligned_s64 as a type so feel free to use that which should
get rid of this message.


> 
>   WARNING: Missing a blank line after declarations
>   #156: FILE: drivers/iio/temperature/tmp006.c:253:
>   +               s16 channels[2];
>   +               s64 ts __aligned(8);
> 
> It also suggests that the following code with a blank line is correct:
> 
> 	struct {
> 		s16 channels[2];
> 
> 		s64 ts __aligned(8);
> 	} scan;
> 
> I left the code as it was (with the WARNING) since it feels obviously
> more natural.

Various comments inline.

Jonathan

> ---
>  drivers/iio/temperature/Kconfig  |   2 +
>  drivers/iio/temperature/tmp006.c | 116 +++++++++++++++++++++++++++++--
>  2 files changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index ed0e4963362f..1244d8e17d50 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -91,6 +91,8 @@ config MLX90635
>  config TMP006
>  	tristate "TMP006 infrared thermopile sensor"
>  	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  TMP006 infrared thermopile sensor.
> diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
> index 6d8d661f0c82..183b30a41573 100644
> --- a/drivers/iio/temperature/tmp006.c

>  #define TMP006_VOBJECT 0x00
>  #define TMP006_TAMBIENT 0x01
> @@ -45,6 +46,7 @@
>  struct tmp006_data {
>  	struct i2c_client *client;
>  	u16 config;
> +	struct iio_trigger *drdy_trig;
>  };
>  
>  static int tmp006_read_measurement(struct tmp006_data *data, u8 reg)
> @@ -81,15 +83,21 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);

I'd suggest the scoped variant of this as you
are effectively taking and releasing locks in different scopes.
The code is write, but not nice to read.

> +		if (ret)
> +			return ret;
> +
>  		if (channel->type == IIO_VOLTAGE) {
>  			/* LSB is 156.25 nV */
>  			ret = tmp006_read_measurement(data, TMP006_VOBJECT);
> +			iio_device_release_direct_mode(indio_dev);
>  			if (ret < 0)
>  				return ret;
>  			*val = sign_extend32(ret, 15);
>  		} else if (channel->type == IIO_TEMP) {
>  			/* LSB is 0.03125 degrees Celsius */
>  			ret = tmp006_read_measurement(data, TMP006_TAMBIENT);
> +			iio_device_release_direct_mode(indio_dev);
>  			if (ret < 0)
>  				return ret;
>  			*val = sign_extend32(ret, 15) >> TMP006_TAMBIENT_SHIFT;

> @@ -164,13 +178,29 @@ static const struct iio_chan_spec tmp006_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_BE
trailing comma wanted here.

> +		}
>  	},
>  	{
>  		.type = IIO_TEMP,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_SCALE),
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> -	}
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 14,
> +			.storagebits = 16,
> +			.shift = TMP006_TAMBIENT_SHIFT,
> +			.endianness = IIO_BE
add trailing comma. New fields may be added in future and need setting
so having ht comma will make that easier to do.

> +		}
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2)

Add a comma.  In is possible that other channels will follow this.
Doesn't often happen but there have been cases.

>  };
>  
>  static const struct iio_info tmp006_info = {
> @@ -213,6 +243,49 @@ static void tmp006_powerdown_cleanup(void *dev)
>  	tmp006_power(dev, false);
>  }
>  
> +static irqreturn_t tmp006_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct tmp006_data *data = iio_priv(indio_dev);
> +	struct {
> +		s16 channels[2];
> +		s64 ts __aligned(8);
> +	} scan;
> +
> +	scan.channels[0] = i2c_smbus_read_word_data(data->client, TMP006_VOBJECT);

read word_data uses an s32 return type to leave space for errors.  
What you have here will cause problems if the top bit happens to be set in
the register (i.e. it's a negative value).

So use a local s32 first then assign it to the buffer
data after checking for negative values.

Note you currently don't ensure both channels are enabled, so if the
user requests only the second channel they will get the wrong data.

Either make this code handle or set available_scan_masks appropriately
to let the core IIO code reorganize the data to what the user asked for.

> +	if (scan.channels[0] < 0)
> +		goto err;
> +
> +	scan.channels[1] = i2c_smbus_read_word_data(data->client, TMP006_TAMBIENT);
> +	if (scan.channels[1] < 0)
> +		goto err;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan,
> +					   iio_get_time_ns(indio_dev));
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static int tmp006_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct tmp006_data *data = iio_priv(indio_dev);
> +
> +	if (state)
> +		data->config |= TMP006_CONFIG_DRDY_EN;
> +	else
> +		data->config &= ~TMP006_CONFIG_DRDY_EN;
> +
> +	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
> +		data->config);

Align with data on the line above, so just after the (

That's the preferred kernel style except when we are up against
really long lines or similar.

> +}
> +
> +static const struct iio_trigger_ops tmp006_trigger_ops = {
> +	.set_trigger_state = tmp006_set_trigger_state,
> +};
> +
>  static int tmp006_probe(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev;
> @@ -258,6 +331,35 @@ static int tmp006_probe(struct i2c_client *client)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq > 0) {
> +		data->drdy_trig = devm_iio_trigger_alloc(&client->dev,
> +							 "%s-dev%d",
> +							 indio_dev->name,
> +							 iio_device_id(indio_dev));
> +		if (!data->drdy_trig)
> +			return -ENOMEM;
> +
> +		data->drdy_trig->ops = &tmp006_trigger_ops;
> +		iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> +		ret = iio_trigger_register(data->drdy_trig);
> +		if (ret)
> +			return ret;
> +
> +		indio_dev->trig = iio_trigger_get(data->drdy_trig);
> +
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						iio_trigger_generic_data_rdy_poll,
> +						NULL,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
Type of interrupt should come from firmware.   We wrongly used to set it
in drivers (and are now stuck with that) but that overly constrains
things for no useful purpose.  So IRQF_ONESHOT only here.

> +						"tmp006_irq",
> +						data->drdy_trig);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> +					      tmp006_trigger_handler, NULL);
> +
check ret.

>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>