[v4] iio: adc: add buffering support to the TI LMP92064 ADC driver
Commit Message
Enable buffered reading of samples from the LMP92064 ADC.
The main benefit of this change is being able to read out current and
voltage measurements in a single transfer, allowing instantaneous power
measurements.
Reads into the buffer can be triggered by any software triggers, e.g.
the iio-trig-hrtimer:
$ mkdir /sys/kernel/config/iio/triggers/hrtimer/my-trigger
$ cat /sys/bus/iio/devices/iio\:device3/name
lmp92064
$ iio_readdev -t my-trigger -b 16 iio:device3 | hexdump
WARNING: High-speed mode not enabled
0000000 0000 0176 0101 0001 5507 abd5 7645 1768
0000010 0000 016d 0101 0001 ee1e ac6b 7645 1768
...
Signed-off-by: Leonard Göhrs <l.goehrs@pengutronix.de>
---
Changes v1->v2:
- Remove superfluous .shift = 0 initialization in scan_type.
- Replace kmalloc buffer allocation for every read with a stack
allocated structure.
A struct is used to ensure correct alignment of the timestamp field.
See e.g. adc/rockchip_saradc.c for other users of the same pattern.
- Use available_scan_masks and always push both voltage and current
measurements to the buffer.
Changes v2->v3:
- Handle errors returned by lmp92064_read_meas() out-of-line via
a goto err;
Changes v3->v4:
- 8-Byte align the 64 bit timestamp in the buffer structure
and memset() the stack-allocated buffer to prevent leaking
kernel memory to userspace. (Thanks to Nuno Sá)
---
drivers/iio/adc/ti-lmp92064.c | 53 +++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1
Comments
On Fri, 2023-07-07 at 08:36 +0200, Leonard Göhrs wrote:
> Enable buffered reading of samples from the LMP92064 ADC.
> The main benefit of this change is being able to read out current and
> voltage measurements in a single transfer, allowing instantaneous power
> measurements.
>
> Reads into the buffer can be triggered by any software triggers, e.g.
> the iio-trig-hrtimer:
>
> $ mkdir /sys/kernel/config/iio/triggers/hrtimer/my-trigger
> $ cat /sys/bus/iio/devices/iio\:device3/name
> lmp92064
> $ iio_readdev -t my-trigger -b 16 iio:device3 | hexdump
> WARNING: High-speed mode not enabled
> 0000000 0000 0176 0101 0001 5507 abd5 7645 1768
> 0000010 0000 016d 0101 0001 ee1e ac6b 7645 1768
> ...
>
> Signed-off-by: Leonard Göhrs <l.goehrs@pengutronix.de>
> ---
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> Changes v1->v2:
>
> - Remove superfluous .shift = 0 initialization in scan_type.
> - Replace kmalloc buffer allocation for every read with a stack
> allocated structure.
> A struct is used to ensure correct alignment of the timestamp field.
> See e.g. adc/rockchip_saradc.c for other users of the same pattern.
> - Use available_scan_masks and always push both voltage and current
> measurements to the buffer.
>
> Changes v2->v3:
>
> - Handle errors returned by lmp92064_read_meas() out-of-line via
> a goto err;
>
> Changes v3->v4:
>
> - 8-Byte align the 64 bit timestamp in the buffer structure
> and memset() the stack-allocated buffer to prevent leaking
> kernel memory to userspace. (Thanks to Nuno Sá)
> ---
> drivers/iio/adc/ti-lmp92064.c | 53 +++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/iio/adc/ti-lmp92064.c b/drivers/iio/adc/ti-lmp92064.c
> index c30ed824924f3..84ba5c4a0eea7 100644
> --- a/drivers/iio/adc/ti-lmp92064.c
> +++ b/drivers/iio/adc/ti-lmp92064.c
> @@ -16,7 +16,10 @@
> #include <linux/spi/spi.h>
>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/driver.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>
> #define TI_LMP92064_REG_CONFIG_A 0x0000
> #define TI_LMP92064_REG_CONFIG_B 0x0001
> @@ -91,6 +94,12 @@ static const struct iio_chan_spec lmp92064_adc_channels[] =
> {
> .address = TI_LMP92064_CHAN_INC,
> .info_mask_separate =
> BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = TI_LMP92064_CHAN_INC,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 12,
> + .storagebits = 16,
> + },
> .datasheet_name = "INC",
> },
> {
> @@ -98,8 +107,20 @@ static const struct iio_chan_spec lmp92064_adc_channels[]
> = {
> .address = TI_LMP92064_CHAN_INV,
> .info_mask_separate =
> BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = TI_LMP92064_CHAN_INV,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 12,
> + .storagebits = 16,
> + },
> .datasheet_name = "INV",
> },
> + IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +static const unsigned long lmp92064_scan_masks[] = {
> + BIT(TI_LMP92064_CHAN_INC) | BIT(TI_LMP92064_CHAN_INV),
> + 0
> };
>
> static int lmp92064_read_meas(struct lmp92064_adc_priv *priv, u16 *res)
> @@ -171,6 +192,32 @@ static int lmp92064_read_raw(struct iio_dev *indio_dev,
> }
> }
>
> +static irqreturn_t lmp92064_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct lmp92064_adc_priv *priv = iio_priv(indio_dev);
> + struct {
> + u16 values[2];
> + int64_t timestamp __aligned(8);
> + } data;
> + int ret;
> +
> + memset(&data, 0, sizeof(data));
> +
> + ret = lmp92064_read_meas(priv, data.values);
> + if (ret)
> + goto err;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &data,
> + iio_get_time_ns(indio_dev));
> +
> +err:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int lmp92064_reset(struct lmp92064_adc_priv *priv,
> struct gpio_desc *gpio_reset)
> {
> @@ -301,6 +348,12 @@ static int lmp92064_adc_probe(struct spi_device *spi)
> indio_dev->channels = lmp92064_adc_channels;
> indio_dev->num_channels = ARRAY_SIZE(lmp92064_adc_channels);
> indio_dev->info = &lmp92064_adc_info;
> + indio_dev->available_scan_masks = lmp92064_scan_masks;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + lmp92064_trigger_handler, NULL);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup buffered
> read\n");
>
> return devm_iio_device_register(dev, indio_dev);
> }
>
> base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1
On Fri, 7 Jul 2023 08:36:35 +0200
Leonard Göhrs <l.goehrs@pengutronix.de> wrote:
> Enable buffered reading of samples from the LMP92064 ADC.
> The main benefit of this change is being able to read out current and
> voltage measurements in a single transfer, allowing instantaneous power
> measurements.
>
> Reads into the buffer can be triggered by any software triggers, e.g.
> the iio-trig-hrtimer:
>
> $ mkdir /sys/kernel/config/iio/triggers/hrtimer/my-trigger
> $ cat /sys/bus/iio/devices/iio\:device3/name
> lmp92064
> $ iio_readdev -t my-trigger -b 16 iio:device3 | hexdump
> WARNING: High-speed mode not enabled
> 0000000 0000 0176 0101 0001 5507 abd5 7645 1768
> 0000010 0000 016d 0101 0001 ee1e ac6b 7645 1768
> ...
>
> Signed-off-by: Leonard Göhrs <l.goehrs@pengutronix.de>
Applied, with a tweaked commit message (to put the driver name after the iio: adc:
as the driver already exists).
Applied to the togreg branch of iio.git but as we are mid merge window, I'll wait
until I can rebase on rc1. For now just pushed out as testing to see if 0-day
can find any issues that we missed.
Thanks,
Jonathan
> ---
> Changes v1->v2:
>
> - Remove superfluous .shift = 0 initialization in scan_type.
> - Replace kmalloc buffer allocation for every read with a stack
> allocated structure.
> A struct is used to ensure correct alignment of the timestamp field.
> See e.g. adc/rockchip_saradc.c for other users of the same pattern.
> - Use available_scan_masks and always push both voltage and current
> measurements to the buffer.
>
> Changes v2->v3:
>
> - Handle errors returned by lmp92064_read_meas() out-of-line via
> a goto err;
>
> Changes v3->v4:
>
> - 8-Byte align the 64 bit timestamp in the buffer structure
> and memset() the stack-allocated buffer to prevent leaking
> kernel memory to userspace. (Thanks to Nuno Sá)
> ---
> drivers/iio/adc/ti-lmp92064.c | 53 +++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/iio/adc/ti-lmp92064.c b/drivers/iio/adc/ti-lmp92064.c
> index c30ed824924f3..84ba5c4a0eea7 100644
> --- a/drivers/iio/adc/ti-lmp92064.c
> +++ b/drivers/iio/adc/ti-lmp92064.c
> @@ -16,7 +16,10 @@
> #include <linux/spi/spi.h>
>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/driver.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>
> #define TI_LMP92064_REG_CONFIG_A 0x0000
> #define TI_LMP92064_REG_CONFIG_B 0x0001
> @@ -91,6 +94,12 @@ static const struct iio_chan_spec lmp92064_adc_channels[] = {
> .address = TI_LMP92064_CHAN_INC,
> .info_mask_separate =
> BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = TI_LMP92064_CHAN_INC,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 12,
> + .storagebits = 16,
> + },
> .datasheet_name = "INC",
> },
> {
> @@ -98,8 +107,20 @@ static const struct iio_chan_spec lmp92064_adc_channels[] = {
> .address = TI_LMP92064_CHAN_INV,
> .info_mask_separate =
> BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = TI_LMP92064_CHAN_INV,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 12,
> + .storagebits = 16,
> + },
> .datasheet_name = "INV",
> },
> + IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +static const unsigned long lmp92064_scan_masks[] = {
> + BIT(TI_LMP92064_CHAN_INC) | BIT(TI_LMP92064_CHAN_INV),
> + 0
> };
>
> static int lmp92064_read_meas(struct lmp92064_adc_priv *priv, u16 *res)
> @@ -171,6 +192,32 @@ static int lmp92064_read_raw(struct iio_dev *indio_dev,
> }
> }
>
> +static irqreturn_t lmp92064_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct lmp92064_adc_priv *priv = iio_priv(indio_dev);
> + struct {
> + u16 values[2];
> + int64_t timestamp __aligned(8);
> + } data;
> + int ret;
> +
> + memset(&data, 0, sizeof(data));
> +
> + ret = lmp92064_read_meas(priv, data.values);
> + if (ret)
> + goto err;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &data,
> + iio_get_time_ns(indio_dev));
> +
> +err:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int lmp92064_reset(struct lmp92064_adc_priv *priv,
> struct gpio_desc *gpio_reset)
> {
> @@ -301,6 +348,12 @@ static int lmp92064_adc_probe(struct spi_device *spi)
> indio_dev->channels = lmp92064_adc_channels;
> indio_dev->num_channels = ARRAY_SIZE(lmp92064_adc_channels);
> indio_dev->info = &lmp92064_adc_info;
> + indio_dev->available_scan_masks = lmp92064_scan_masks;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + lmp92064_trigger_handler, NULL);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to setup buffered read\n");
>
> return devm_iio_device_register(dev, indio_dev);
> }
>
> base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1
@@ -16,7 +16,10 @@
#include <linux/spi/spi.h>
#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/driver.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
#define TI_LMP92064_REG_CONFIG_A 0x0000
#define TI_LMP92064_REG_CONFIG_B 0x0001
@@ -91,6 +94,12 @@ static const struct iio_chan_spec lmp92064_adc_channels[] = {
.address = TI_LMP92064_CHAN_INC,
.info_mask_separate =
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .scan_index = TI_LMP92064_CHAN_INC,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 12,
+ .storagebits = 16,
+ },
.datasheet_name = "INC",
},
{
@@ -98,8 +107,20 @@ static const struct iio_chan_spec lmp92064_adc_channels[] = {
.address = TI_LMP92064_CHAN_INV,
.info_mask_separate =
BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .scan_index = TI_LMP92064_CHAN_INV,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 12,
+ .storagebits = 16,
+ },
.datasheet_name = "INV",
},
+ IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static const unsigned long lmp92064_scan_masks[] = {
+ BIT(TI_LMP92064_CHAN_INC) | BIT(TI_LMP92064_CHAN_INV),
+ 0
};
static int lmp92064_read_meas(struct lmp92064_adc_priv *priv, u16 *res)
@@ -171,6 +192,32 @@ static int lmp92064_read_raw(struct iio_dev *indio_dev,
}
}
+static irqreturn_t lmp92064_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct lmp92064_adc_priv *priv = iio_priv(indio_dev);
+ struct {
+ u16 values[2];
+ int64_t timestamp __aligned(8);
+ } data;
+ int ret;
+
+ memset(&data, 0, sizeof(data));
+
+ ret = lmp92064_read_meas(priv, data.values);
+ if (ret)
+ goto err;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &data,
+ iio_get_time_ns(indio_dev));
+
+err:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
static int lmp92064_reset(struct lmp92064_adc_priv *priv,
struct gpio_desc *gpio_reset)
{
@@ -301,6 +348,12 @@ static int lmp92064_adc_probe(struct spi_device *spi)
indio_dev->channels = lmp92064_adc_channels;
indio_dev->num_channels = ARRAY_SIZE(lmp92064_adc_channels);
indio_dev->info = &lmp92064_adc_info;
+ indio_dev->available_scan_masks = lmp92064_scan_masks;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+ lmp92064_trigger_handler, NULL);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to setup buffered read\n");
return devm_iio_device_register(dev, indio_dev);
}