* [PATCH 0/2] iio: temperature: tmp006: support for drdy irq @ 2024-09-02 12:59 Antoni Pokusinski 2024-09-02 12:59 ` [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support Antoni Pokusinski 2024-09-02 12:59 ` [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt Antoni Pokusinski 0 siblings, 2 replies; 6+ messages in thread From: Antoni Pokusinski @ 2024-09-02 12:59 UTC (permalink / raw) To: jic23, lars, robh, krzk+dt Cc: pmeerw, linux-iio, devicetree, linux-kernel, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support 2024-09-02 12:59 [PATCH 0/2] iio: temperature: tmp006: support for drdy irq Antoni Pokusinski @ 2024-09-02 12:59 ` Antoni Pokusinski 2024-09-07 16:20 ` Jonathan Cameron 2024-09-02 12:59 ` [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt Antoni Pokusinski 1 sibling, 1 reply; 6+ messages in thread From: Antoni Pokusinski @ 2024-09-02 12:59 UTC (permalink / raw) To: jic23, lars, robh, krzk+dt Cc: pmeerw, linux-iio, devicetree, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support 2024-09-02 12:59 ` [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support Antoni Pokusinski @ 2024-09-07 16:20 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2024-09-07 16:20 UTC (permalink / raw) To: Antoni Pokusinski Cc: lars, robh, krzk+dt, pmeerw, linux-iio, devicetree, linux-kernel 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); > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt 2024-09-02 12:59 [PATCH 0/2] iio: temperature: tmp006: support for drdy irq Antoni Pokusinski 2024-09-02 12:59 ` [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support Antoni Pokusinski @ 2024-09-02 12:59 ` Antoni Pokusinski 2024-09-02 16:04 ` Krzysztof Kozlowski 1 sibling, 1 reply; 6+ messages in thread From: Antoni Pokusinski @ 2024-09-02 12:59 UTC (permalink / raw) To: jic23, lars, robh, krzk+dt Cc: pmeerw, linux-iio, devicetree, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt 2024-09-02 12:59 ` [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt Antoni Pokusinski @ 2024-09-02 16:04 ` Krzysztof Kozlowski 2024-09-03 7:05 ` Krzysztof Kozlowski 0 siblings, 1 reply; 6+ messages in thread From: Krzysztof Kozlowski @ 2024-09-02 16:04 UTC (permalink / raw) To: Antoni Pokusinski, jic23, lars, robh, krzk+dt Cc: pmeerw, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt 2024-09-02 16:04 ` Krzysztof Kozlowski @ 2024-09-03 7:05 ` Krzysztof Kozlowski 0 siblings, 0 replies; 6+ messages in thread From: Krzysztof Kozlowski @ 2024-09-03 7:05 UTC (permalink / raw) To: Antoni Pokusinski, jic23, lars, robh, krzk+dt Cc: pmeerw, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-09-07 16:20 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-02 12:59 [PATCH 0/2] iio: temperature: tmp006: support for drdy irq Antoni Pokusinski 2024-09-02 12:59 ` [PATCH 1/2] iio: temperature: tmp006: add triggered buffer support Antoni Pokusinski 2024-09-07 16:20 ` Jonathan Cameron 2024-09-02 12:59 ` [PATCH 2/2] dt-bindings: iio: temperature: tmp006: document interrupt Antoni Pokusinski 2024-09-02 16:04 ` Krzysztof Kozlowski 2024-09-03 7:05 ` Krzysztof Kozlowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox