| Message ID | 20230703103908.27691-1-l.goehrs@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] iio: adc: add buffering support to the TI LMP92064 ADC driver | expand |
Hi Leonard, LGTM, only one comment from me. See below... On Mon, 2023-07-03 at 12:39 +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> > --- > 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; > > --- > drivers/iio/adc/ti-lmp92064.c | 51 +++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/iio/adc/ti-lmp92064.c b/drivers/iio/adc/ti-lmp92064.c > index c30ed824924f3..73504a27a8e22 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,30 @@ 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; > + } data; > + int ret; > + I think you need to explicitly __align(8) your timestamp data. The reason is that (IIRC) for x86 32 bits, a long long is not naturally aligned to 64bits but 32. You also need to explicitly memset(0) your data so we don't leak data to userspace. - Nuno Sá
diff --git a/drivers/iio/adc/ti-lmp92064.c b/drivers/iio/adc/ti-lmp92064.c index c30ed824924f3..73504a27a8e22 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,30 @@ 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; + } data; + int ret; + + 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 +346,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); }
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; --- drivers/iio/adc/ti-lmp92064.c | 51 +++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1