| Message ID | 93ae6ff1150b531a9d7a4d3d1b1adb8383613717.1666955685.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: imu: st_lsm6dsx: introduce sw trigger support | expand |
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; > +} > + ...
> 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; > > +} > > + > ... >
... > > > +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 :) Fine as it stands. You've thought about it and decided to postpone such a change until it is necessary and that's fine by me. Applied to the togreg branch of iio.git and pushed out as testing for 0-day to poke at the tree and see if we missed anything. Thanks, Jonathan
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 __maybe_unused const struct iio_event_spec st_lsm6dsx_event = { @@ -456,6 +456,7 @@ int st_lsm6dsx_read_tagged_fifo(struct st_lsm6dsx_hw *hw); int st_lsm6dsx_check_odr(struct st_lsm6dsx_sensor *sensor, u32 odr, u8 *val); int st_lsm6dsx_shub_probe(struct st_lsm6dsx_hw *hw, const char *name); int st_lsm6dsx_shub_set_enable(struct st_lsm6dsx_sensor *sensor, bool enable); +int st_lsm6dsx_shub_read_output(struct st_lsm6dsx_hw *hw, u8 *data, int len); int st_lsm6dsx_set_page(struct st_lsm6dsx_hw *hw, bool enable); 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)); + 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_buffer_preenable(struct iio_dev *iio_dev) +{ + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev); + + return st_lsm6dsx_device_set_enable(sensor, true); +} + +static int st_lsm6dsx_sw_buffer_postdisable(struct iio_dev *iio_dev) +{ + struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev); + + return st_lsm6dsx_device_set_enable(sensor, false); +} + +static const struct iio_buffer_setup_ops st_lsm6dsx_sw_buffer_ops = { + .preenable = st_lsm6dsx_sw_buffer_preenable, + .postdisable = st_lsm6dsx_sw_buffer_postdisable, +}; + +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; +} + static int st_lsm6dsx_init_regulators(struct device *dev) { /* vdd-vddio power regulators */ @@ -2255,6 +2323,16 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, return err; } + if (!hw->irq || !hw->settings->fifo_ops.read_fifo) { + /* + * Rely on sw triggers (e.g. hr-timers) if irq pin is not + * connected of if the device does not support HW FIFO + */ + err = st_lsm6dsx_sw_buffers_setup(hw); + if (err) + return err; + } + err = iio_read_mount_matrix(hw->dev, &hw->orientation); if (err) return err; diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c index 99562ba85ee4..f2b64b4956a3 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_shub.c @@ -170,9 +170,7 @@ static void st_lsm6dsx_shub_wait_complete(struct st_lsm6dsx_hw *hw) * * Read st_lsm6dsx i2c controller register */ -static int -st_lsm6dsx_shub_read_output(struct st_lsm6dsx_hw *hw, u8 *data, - int len) +int st_lsm6dsx_shub_read_output(struct st_lsm6dsx_hw *hw, u8 *data, int len) { const struct st_lsm6dsx_shub_settings *hub_settings; int err;
There are some hw configuration where irq0 and/or irq1 pins are not connected to the SPI or I2C/I3C controller. 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> --- 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(-)