On Wed, 8 Jun 2022 00:14:08 +0530
Jagath Jog J <jagathjog1996@xxxxxxxxx> wrote:
> 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.
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
