On Wed, 10 Jan 2024 19:22:40 +0200
Petre Rodan <petre.rodan@xxxxxxxxxxxxxxx> wrote:
> Add triggered buffer feature.
>
> Signed-off-by: Petre Rodan <petre.rodan@xxxxxxxxxxxxxxx>
Hi Petre,
A few minor things inline. + you almost certainly need some Kconfig
changes to ensure buffered support is built for the IIO core.
> ---
> drivers/iio/pressure/hsc030pa.c | 42 +++++++++++++++++++++++++++++++++
> drivers/iio/pressure/hsc030pa.h | 2 +-
> 2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/pressure/hsc030pa.c b/drivers/iio/pressure/hsc030pa.c
> index 7e3f74d53b47..3faa0fd42201 100644
> --- a/drivers/iio/pressure/hsc030pa.c
> +++ b/drivers/iio/pressure/hsc030pa.c
> @@ -22,8 +22,11 @@
> #include <linux/types.h>
> #include <linux/units.h>
>
> +#include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> #include <asm/unaligned.h>
>
> @@ -297,6 +300,23 @@ static int hsc_get_measurement(struct hsc_data *data)
> return 0;
> }
>
> +static irqreturn_t hsc_trigger_handler(int irq, void *private)
> +{
> + struct iio_poll_func *pf = private;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct hsc_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = hsc_get_measurement(data);
> + if (!ret) {
Prefer error handling out of line + no {} for single statements
if (ret)
goto error;
iio_push...
error:
iio_trigger...
> + iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> + iio_get_time_ns(indio_dev));
> + }
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> /*
> * IIO ABI expects
> * value = (conv + offset) * scale
> @@ -382,13 +402,30 @@ static const struct iio_chan_spec hsc_channels[] = {
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OFFSET),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 14,
> + .storagebits = 16,
> + .shift = 0,
No need to specify shift if it's zero. That's considered the obvious default
and C will ensure it's set to 0 anyway.
> + .endianness = IIO_BE,
> + },
> },
> {
> .type = IIO_TEMP,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE) |
> BIT(IIO_CHAN_INFO_OFFSET),
> + .scan_index = 1,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 11,
> + .storagebits = 16,
> + .shift = 5,
> + .endianness = IIO_BE,
> + },
> },
> + IIO_CHAN_SOFT_TIMESTAMP(2),
> };
>
> static const struct iio_info hsc_info = {
> @@ -485,6 +522,11 @@ int hsc_common_probe(struct device *dev, hsc_recv_fn recv)
> indio_dev->channels = hsc->chip->channels;
> indio_dev->num_channels = hsc->chip->num_channels;
>
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + hsc_trigger_handler, NULL);
> + if (ret)
> + return ret;
> +
> return devm_iio_device_register(dev, indio_dev);
> }
> EXPORT_SYMBOL_NS(hsc_common_probe, IIO_HONEYWELL_HSC030PA);
> diff --git a/drivers/iio/pressure/hsc030pa.h b/drivers/iio/pressure/hsc030pa.h
> index 56dc8e88194b..6c635c42d85d 100644
> --- a/drivers/iio/pressure/hsc030pa.h
> +++ b/drivers/iio/pressure/hsc030pa.h
> @@ -56,7 +56,7 @@ struct hsc_data {
> s32 p_scale_dec;
> s64 p_offset;
> s32 p_offset_dec;
> - u8 buffer[HSC_REG_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> + u8 buffer[16] __aligned(IIO_DMA_MINALIGN);
Justify that size. Normal trick is to use a suitable structure definition with
the channels and a timestamp s64 with force alignment (To deal with te annoying
x86 32bit alignment)
> };
>
> struct hsc_chip_data {
> --
> 2.41.0
>
>
