Re: [RFC 2/2] iio: accel: bma400: Add support for single and double tap events

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 8 Jun 2022 00:14:08 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

>  Hi Jonathan,
> 
> On Sat, Jun 4, 2022 at 8:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 29 May 2022 09:31:53 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> 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@gmail.com>  
> >
> > 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.
Add more to the comment.  I'll think a bit more on this in next version.
> 
> >
> > 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.
Another option is to just fail the buffer start if if a different frequency
is requested or fail to start the tap detection in the first place
if sampling_frequency isn't set correctly.  (spit out a message to the
log though as it's a bit of an odd corner case!)

Jonathan

> 
> Thanks,
> Jagath