Re: [PATCH] iio: imu: st_lsm6dsx: introduce sw trigger support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 28 Oct 2022 13:23:42 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> There are some hw configuration where irq0 and/or irq1 pins are not
> connected to the SPI or I2C/I3C controller.

The might be connected to lots of other places on the application processor
(doesn't really matter though - I think your meaning is clear enough!)

> In order to avoid polling
> the output register introduce iio-sw trigger support when irq line is
> not available (or hw FIFO is not supported).
> 
> Suggested-by: Mario Tesi <mario.tesi@st.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>

One comment / question inline.  Otherwise looks good to me.

Jonathan


> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  3 +-
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 78 ++++++++++++++++++++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c |  4 +-
>  3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index 07ad8027de73..6399b0bb6f67 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -424,7 +424,7 @@ struct st_lsm6dsx_hw {
>  	struct {
>  		__le16 channels[3];
>  		s64 ts __aligned(8);
> -	} scan[3];
> +	} scan[ST_LSM6DSX_ID_MAX];
>  };
>  

>  static inline int
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index fe5fa08b68ac..73fd5f038375 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -53,6 +53,8 @@
>  #include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/minmax.h>
> @@ -2117,6 +2119,32 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>  	return fifo_len || event ? IRQ_HANDLED : IRQ_NONE;
>  }
>  
> +static irqreturn_t st_lsm6dsx_sw_trigger_handler_thread(int irq,
> +							void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *iio_dev = pf->indio_dev;
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (sensor->id == ST_LSM6DSX_ID_EXT0 ||
> +	    sensor->id == ST_LSM6DSX_ID_EXT1 ||
> +	    sensor->id == ST_LSM6DSX_ID_EXT2)
> +		st_lsm6dsx_shub_read_output(hw,
> +					    (u8 *)hw->scan[sensor->id].channels,
> +					    sizeof(hw->scan[sensor->id].channels));

Are we guaranteed this particular size of readback?  I'm guessing a bit
as it's been a long time since I looked at this driver in detail, but could
we have sensors with either a different number of axes or different number
of registers per axis?

It might be neater to have two handlers, one for the EXTN cases and one
for the main sensors.  That would push this conditional down to the
point of registration.  I'm not sure it's worth it however so up to you...


> +	else
> +		st_lsm6dsx_read_locked(hw, iio_dev->channels[0].address,
> +				       hw->scan[sensor->id].channels,
> +				       sizeof(hw->scan[sensor->id].channels));
> +
> +	iio_push_to_buffers_with_timestamp(iio_dev, &hw->scan[sensor->id],
> +					   iio_get_time_ns(iio_dev));
> +	iio_trigger_notify_done(iio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
>  {
>  	struct st_sensors_platform_data *pdata;
> @@ -2175,6 +2203,46 @@ static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
>  	return 0;
>  }

> +static int st_lsm6dsx_sw_buffers_setup(struct st_lsm6dsx_hw *hw)
> +{
> +	int i;
> +
> +	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> +		int err;
> +
> +		if (!hw->iio_devs[i])
> +			continue;
> +
> +		err = devm_iio_triggered_buffer_setup(hw->dev,
> +					hw->iio_devs[i], NULL,
> +					st_lsm6dsx_sw_trigger_handler_thread,
> +					&st_lsm6dsx_sw_buffer_ops);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
...