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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4783 bytes --]

> 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...

Hi Jonathan,

so far we support just magnetometers on sensor-hub (LIS2MDL and LIS3MDL).
Both LIS2MDL and LIS3MDL have 3 axis, each of them is le16, so it is fine as it
is for the moment. Do you prefer to be more generic and take into account new
possible sensors? I am not sure when they will arrive :)

Regards,
Lorenzo

> 
> 
> > +	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;
> > +}
> > +
> ...
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]