| Message ID | 20190916090222.597444-1-sean@geanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] iio: imu: st_lsm6dsx: filter motion events in driver | expand |
> Do not report non enabled motion events. > Wakeup will still be on all channels as it's not possible to do the > filtering in hw. > > Signed-off-by: Sean Nyekjaer <sean@geanix.com> > --- > Hope it's okay to do this as an RFC. To get the most obvious stuff > reviewed before v9 > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 + > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 20 ++++++++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h > index 938192212485..dd46209f94e8 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h > @@ -362,6 +362,7 @@ struct st_lsm6dsx_hw { > > u8 event_threshold; > bool enable_event; > + u8 event_en_mask; I think we do not need it, you could just use enable_event as bitmask (just convert it in u8) > struct st_lsm6dsx_reg irq_routing; > > u8 *buff; > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > index 7596a6ed7d97..2d66e3758921 100644 > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c > @@ -1377,9 +1377,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, > if (type != IIO_EV_TYPE_THRESH) > return -EINVAL; > > + if (hw->event_en_mask & BIT(chan->channel2)) > + goto out; Why do we need this? > + > /* do not enable events if they are already enabled */ > if (state && hw->enable_event) > - return 0; > + goto out; > > err = st_lsm6dsx_event_setup(hw, state); > if (err < 0) > @@ -1391,6 +1394,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, > > hw->enable_event = state; > > +out: > + if (state) > + hw->event_en_mask |= BIT(chan->channel2); > + else > + hw->event_en_mask &= ~BIT(chan->channel2); you can use enable_event here instead of event_en_mask > + > return 0; > } > > @@ -1746,7 +1755,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data) > { > s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]); > > - if (data & hw->settings->event_settings.wakeup_src_z_mask) > + if (data & hw->settings->event_settings.wakeup_src_z_mask && > + hw->event_en_mask & BIT(IIO_MOD_Z)) please add () if ((data & hw->settings->event_settings.wakeup_src_z_mask) && (hw->event_en_mask & BIT(IIO_MOD_Z)) > iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC], > IIO_MOD_EVENT_CODE(IIO_ACCEL, > 0, > @@ -1755,7 +1765,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data) > IIO_EV_DIR_EITHER), > timestamp); > > - if (data & hw->settings->event_settings.wakeup_src_y_mask) > + if (data & hw->settings->event_settings.wakeup_src_y_mask && > + hw->event_en_mask & BIT(IIO_MOD_Y)) > iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC], > IIO_MOD_EVENT_CODE(IIO_ACCEL, > 0, > @@ -1764,7 +1775,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data) > IIO_EV_DIR_EITHER), > timestamp); > > - if (data & hw->settings->event_settings.wakeup_src_x_mask) > + if (data & hw->settings->event_settings.wakeup_src_x_mask && > + hw->event_en_mask & BIT(IIO_MOD_X)) > iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC], > IIO_MOD_EVENT_CODE(IIO_ACCEL, > 0, > -- > 2.23.0 >
On 16/09/2019 11.16, Lorenzo Bianconi wrote: >> + if (hw->event_en_mask & BIT(chan->channel2)) >> + goto out; > Why do we need this? > Guess we need to check if 0 < hw->enable_event before disabling the sensor... >> + >> /* do not enable events if they are already enabled */ >> if (state && hw->enable_event) >> - return 0; >> + goto out; >> static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, const struct iio_chan_spec *chan, enum iio_event_type type, enum iio_event_direction dir, int state) { struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev); struct st_lsm6dsx_hw *hw = sensor->hw; u8 _enable_event; int err = 0; if (type != IIO_EV_TYPE_THRESH) return -EINVAL; if (state) { _enable_event = hw->enable_event | BIT(chan->channel2); /* do not enable events if they are already enabled */ if (0 < hw->enable_event) goto out; } else { _enable_event = hw->enable_event & ~BIT(chan->channel2); /* only turn off sensor if no events is enabled */ if (0 != _enable_event) goto out; } /* stop here if no changes have been made */ if (hw->enable_event == _enable_event) return 0; err = st_lsm6dsx_event_setup(hw, state); if (err < 0) return err; err = st_lsm6dsx_sensor_set_enable(sensor, state); if (err < 0) return err; out: hw->enable_event = _enable_event; return 0; } Is something like this better?
> > > On 16/09/2019 11.16, Lorenzo Bianconi wrote: > > > + if (hw->event_en_mask & BIT(chan->channel2)) > > > + goto out; > > Why do we need this? > > > > Guess we need to check if 0 < hw->enable_event before disabling the > sensor... > > > + > > > /* do not enable events if they are already enabled */ > > > if (state && hw->enable_event) > > > - return 0; > > > + goto out; > > static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, > const struct iio_chan_spec *chan, > enum iio_event_type type, > enum iio_event_direction dir, > int state) > { > struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev); > struct st_lsm6dsx_hw *hw = sensor->hw; > u8 _enable_event; please use enable_event instead of _enable_event > int err = 0; > > if (type != IIO_EV_TYPE_THRESH) > return -EINVAL; > > if (state) { > _enable_event = hw->enable_event | BIT(chan->channel2); > > /* do not enable events if they are already enabled */ > if (0 < hw->enable_event) > goto out; we do not need this since there is the check: if (hw->enable_event == enable_event) return 0; > } else { > _enable_event = hw->enable_event & ~BIT(chan->channel2); > > /* only turn off sensor if no events is enabled */ > > > > > if (0 != _enable_event) > goto out; we do not need this as well > } > > /* stop here if no changes have been made */ > if (hw->enable_event == _enable_event) > return 0; > > err = st_lsm6dsx_event_setup(hw, state); > if (err < 0) > return err; > > err = st_lsm6dsx_sensor_set_enable(sensor, state); > if (err < 0) > return err; > > out: > hw->enable_event = _enable_event; > > return 0; > } > > Is something like this better?
On 16/09/2019 13.55, Lorenzo Bianconi wrote: >> >> >> On 16/09/2019 11.16, Lorenzo Bianconi wrote: >>>> + if (hw->event_en_mask & BIT(chan->channel2)) >>>> + goto out; >>> Why do we need this? >>> >> >> Guess we need to check if 0 < hw->enable_event before disabling the >> sensor... >>>> + >>>> /* do not enable events if they are already enabled */ >>>> if (state && hw->enable_event) >>>> - return 0; >>>> + goto out; >> >> static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, >> const struct iio_chan_spec *chan, >> enum iio_event_type type, >> enum iio_event_direction dir, >> int state) >> { >> struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev); >> struct st_lsm6dsx_hw *hw = sensor->hw; >> u8 _enable_event; > > please use enable_event instead of _enable_event > >> int err = 0; >> >> if (type != IIO_EV_TYPE_THRESH) >> return -EINVAL; >> >> if (state) { >> _enable_event = hw->enable_event | BIT(chan->channel2); >> >> /* do not enable events if they are already enabled */ >> if (0 < hw->enable_event) >> goto out; > > we do not need this since there is the check: > if (hw->enable_event == enable_event) > return 0; I actually think this is needed as if a channel is enabled and another channel is enabled, then 'state' will be 1 and '0 < hw->enable_event' is true. Then we don't want to do the event_setup again. > >> } else { >> _enable_event = hw->enable_event & ~BIT(chan->channel2); >> >> /* only turn off sensor if no events is enabled */ >> >> >> >> >> if (0 != _enable_event) >> goto out; > > we do not need this as well Like wise here, if we don't have this and to channels is enabled, if you deactivate one of the channels then enable_event indicates that a channel is active but the sensor is disabled. Guess that is not what we want :-) > >> } >> >> /* stop here if no changes have been made */ >> if (hw->enable_event == _enable_event) >> return 0; >> >> err = st_lsm6dsx_event_setup(hw, state); >> if (err < 0) >> return err; >>
On Sep 16, Sean Nyekjaer wrote: > > > On 16/09/2019 13.55, Lorenzo Bianconi wrote: > > > > > > > > > On 16/09/2019 11.16, Lorenzo Bianconi wrote: > > > > > + if (hw->event_en_mask & BIT(chan->channel2)) > > > > > + goto out; > > > > Why do we need this? > > > > > > > > > > Guess we need to check if 0 < hw->enable_event before disabling the > > > sensor... > > > > > + > > > > > /* do not enable events if they are already enabled */ > > > > > if (state && hw->enable_event) > > > > > - return 0; > > > > > + goto out; > > > > > > static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, > > > const struct iio_chan_spec *chan, > > > enum iio_event_type type, > > > enum iio_event_direction dir, > > > int state) > > > { > > > struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev); > > > struct st_lsm6dsx_hw *hw = sensor->hw; > > > u8 _enable_event; > > > > please use enable_event instead of _enable_event > > > > > int err = 0; > > > > > > if (type != IIO_EV_TYPE_THRESH) > > > return -EINVAL; > > > > > > if (state) { > > > _enable_event = hw->enable_event | BIT(chan->channel2); > > > > > > /* do not enable events if they are already enabled */ > > > if (0 < hw->enable_event) > > > goto out; > > > > we do not need this since there is the check: > > if (hw->enable_event == enable_event) > > return 0; > > I actually think this is needed as if a channel is enabled and another > channel is enabled, then 'state' will be 1 and '0 < hw->enable_event' is > true. Then we don't want to do the event_setup again. got what you mean here, right..just please do: if (hw->enable_event) goto out; > > > > > > } else { > > > _enable_event = hw->enable_event & ~BIT(chan->channel2); > > > > > > /* only turn off sensor if no events is enabled */ > > > > > > > > > > > > > > > if (0 != _enable_event) > > > goto out; if (enable_event) goto out; > > > > we do not need this as well > Like wise here, if we don't have this and to channels is enabled, if you > deactivate one of the channels then enable_event indicates that a channel is > active but the sensor is disabled. > Guess that is not what we want :-) > > > > > > } > > > > > > /* stop here if no changes have been made */ > > > if (hw->enable_event == _enable_event) > > > return 0; > > > > > > err = st_lsm6dsx_event_setup(hw, state); > > > if (err < 0) > > > return err; > > > >
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h index 938192212485..dd46209f94e8 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h @@ -362,6 +362,7 @@ struct st_lsm6dsx_hw { u8 event_threshold; bool enable_event; + u8 event_en_mask; struct st_lsm6dsx_reg irq_routing; u8 *buff; diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c index 7596a6ed7d97..2d66e3758921 100644 --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c @@ -1377,9 +1377,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, if (type != IIO_EV_TYPE_THRESH) return -EINVAL; + if (hw->event_en_mask & BIT(chan->channel2)) + goto out; + /* do not enable events if they are already enabled */ if (state && hw->enable_event) - return 0; + goto out; err = st_lsm6dsx_event_setup(hw, state); if (err < 0) @@ -1391,6 +1394,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev, hw->enable_event = state; +out: + if (state) + hw->event_en_mask |= BIT(chan->channel2); + else + hw->event_en_mask &= ~BIT(chan->channel2); + return 0; } @@ -1746,7 +1755,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data) { s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]); - if (data & hw->settings->event_settings.wakeup_src_z_mask) + if (data & hw->settings->event_settings.wakeup_src_z_mask && + hw->event_en_mask & BIT(IIO_MOD_Z)) iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC], IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, @@ -1755,7 +1765,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data) IIO_EV_DIR_EITHER), timestamp); - if (data & hw->settings->event_settings.wakeup_src_y_mask) + if (data & hw->settings->event_settings.wakeup_src_y_mask && + hw->event_en_mask & BIT(IIO_MOD_Y)) iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC], IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, @@ -1764,7 +1775,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data) IIO_EV_DIR_EITHER), timestamp); - if (data & hw->settings->event_settings.wakeup_src_x_mask) + if (data & hw->settings->event_settings.wakeup_src_x_mask && + hw->event_en_mask & BIT(IIO_MOD_X)) iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC], IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
Do not report non enabled motion events. Wakeup will still be on all channels as it's not possible to do the filtering in hw. Signed-off-by: Sean Nyekjaer <sean@geanix.com> --- Hope it's okay to do this as an RFC. To get the most obvious stuff reviewed before v9 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 1 + drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 20 ++++++++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-)