Re: [RFC 2/2] iio: accel: bma400: Add support for single and double tap events
From: Jagath Jog J
Date: Tue Jun 07 2022 - 17:55:00 EST
Hi Jonathan,
On Sat, Jun 4, 2022 at 8:22 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sun, 29 May 2022 09:31:53 +0530
> Jagath Jog J <jagathjog1996@xxxxxxxxx> wrote:
>
> > Add support for single and double tap events based on the tap threshold
> > value and minimum quite time value between the taps. INT1 pin is used to
> > interrupt and event is pushed to userspace.
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@xxxxxxxxx>
>
> Hi Jagath,
>
> A few comments inline.
>
> Thanks,
>
> Jonathan
>
> > ---
> > drivers/iio/accel/bma400.h | 11 ++
> > drivers/iio/accel/bma400_core.c | 186 ++++++++++++++++++++++++++++++--
> > 2 files changed, 188 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index e8f802a82300..7331474433fa 100644
> > --- a/drivers/iio/accel/bma400.h
> > +++ b/drivers/iio/accel/bma400.h
> > @@ -40,6 +40,7 @@
> > #define BMA400_INT_STAT1_REG 0x0f
> > #define BMA400_INT_STAT2_REG 0x10
> > #define BMA400_INT12_MAP_REG 0x23
> > +#define BMA400_INT_ENG_OVRUN_MSK BIT(4)
> >
> > /* Temperature register */
> > #define BMA400_TEMP_DATA_REG 0x11
> > @@ -105,6 +106,16 @@
> > #define BMA400_INT_GEN2_MSK BIT(3)
> > #define BMA400_GEN_HYST_MSK GENMASK(1, 0)
> >
> > +/* TAP config registers */
> > +#define BMA400_TAP_CONFIG 0x57
> > +#define BMA400_TAP_CONFIG1 0x58
> > +#define BMA400_S_TAP_MSK BIT(2)
> > +#define BMA400_D_TAP_MSK BIT(3)
> > +#define BMA400_INT_S_TAP_MSK BIT(10)
> > +#define BMA400_INT_D_TAP_MSK BIT(11)
> > +#define BMA400_TAP_SEN_MSK GENMASK(2, 0)
> > +#define BMA400_TAP_QUITE_MSK GENMASK(3, 2)
> > +
> > /*
> > * BMA400_SCALE_MIN macro value represents m/s^2 for 1 LSB before
> > * converting to micro values for +-2g range.
> > diff --git a/drivers/iio/accel/bma400_core.c b/drivers/iio/accel/bma400_core.c
> > index 517920400df1..2385883707f2 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -88,6 +88,7 @@ struct bma400_data {
> > bool step_event_en;
> > bool activity_event_en;
> > unsigned int generic_event_en;
> > + unsigned int tap_event_en;
> > /* Correct time stamp alignment */
> > struct {
> > __le16 buff[3];
> > @@ -216,6 +217,19 @@ static const struct iio_event_spec bma400_accel_event[] = {
> > BIT(IIO_EV_INFO_HYSTERESIS) |
> > BIT(IIO_EV_INFO_ENABLE),
> > },
> > + {
> > + .type = IIO_EV_TYPE_GESTURE,
> > + .dir = IIO_EV_DIR_SINGLETAP,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_GESTURE,
> > + .dir = IIO_EV_DIR_DOUBLETAP,
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD) |
>
> Feels like period isn't well defined for this case. So probably needs a specific
> ABI entry and period might not be best choice... However, period has no logical
> other meaning in this case (what does 'amount of time a double tap has been true for before
> event mean?') so I think it is fine to use it, as long as ABI docs exist to say what it's
> meaning is for this case.
>
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > };
> >
> > #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > @@ -407,6 +421,14 @@ static int bma400_set_accel_output_data_rate(struct bma400_data *data,
> > unsigned int val;
> > int ret;
> >
> > + /*
> > + * No need to change ODR when tap event is enabled because
>
> Do not change ODR...
>
>
> > + * tap interrupt is operating with the data rate of 200Hz.
> > + * See datasheet page 124.
> > + */
> > + if (data->tap_event_en)
> > + return -EBUSY;
> > +
> > if (hz >= BMA400_ACC_ODR_MIN_WHOLE_HZ) {
> > if (uhz || hz > BMA400_ACC_ODR_MAX_HZ)
> > return -EINVAL;
> > @@ -1012,6 +1034,10 @@ static int bma400_read_event_config(struct iio_dev *indio_dev,
> > case IIO_EV_DIR_FALLING:
> > return FIELD_GET(BMA400_INT_GEN2_MSK,
> > data->generic_event_en);
> > + case IIO_EV_DIR_SINGLETAP:
> > + return FIELD_GET(BMA400_S_TAP_MSK, data->tap_event_en);
> > + case IIO_EV_DIR_DOUBLETAP:
> > + return FIELD_GET(BMA400_D_TAP_MSK, data->tap_event_en);
> > default:
> > return -EINVAL;
> > }
> > @@ -1101,6 +1127,74 @@ static int bma400_activity_event_en(struct bma400_data *data,
> > return 0;
> > }
> >
> > +static int bma400_tap_event_enable(struct bma400_data *data,
> > + enum iio_event_direction dir, int state)
> > +{
> > + int ret;
> > + unsigned int mask, field_value;
> > +
> > + if (data->power_mode == POWER_MODE_SLEEP)
> > + return -EBUSY;
>
> There are existing examples of this in driver, but I can't immediately
> see how we end up in sleep mode. Perhaps a comment on why this might happen?
Currently, only during the driver unwinding the device is put into sleep mode.
I should be checking whether the device is in normal mode or not since
the tap interrupts only in normal mode. I will change this.
>
> > +
> > + /*
> > + * acc_filt1 is the data source for the tap interrupt and it is
> > + * operating on an input data rate of 200Hz.
> > + */
> > + if (!data->tap_event_en) {
>
> Feels like checking the wrong thing. If we need 200Hz, check if the
> data rate is at 200Hz rather than if the tap_event is not enabled.
> Obviously same result, but one seems more obvious.
if (!data->tap_event_en)
As I mentioned in the previous mail this checking is to make sure
not to execute bma400_set_accel_output_data_rate() function
while disabling the tap event.
>
> Also if bma400_set_accel_output_data_rate() is effectively a noop when
> the data rate is unchanged (and it should be with regmap caching) then
> maybe just call it unconditionally?
>
> This might be a nasty surprise for other users though - so if buffered
> output is in use, maybe just don't allow the rate change, even if
> that means not enabling tap detection.
In this case, if the tap is enabled before the buffer do I need to disable
the tap events before enabling buffer?
I have tested tap events with continuous trigger buffer read but only
thing is, it is not possible to change the data rate.
Thanks,
Jagath