[PATCH v1 0/2] iio: Add single and double tap events support

[PATCH v1 0/2] iio: Add single and double tap events support

[Jagath Jog J]

This patch series adds new event type for tap called gesture and direction
is used to differentiate single and double tap. This series adds single
and double tap support for bma400 accelerometer device driver.

Changes since RFC
1. Corrected the "quite" typo to "quiet".
2. Added proper reference and name of the section from datasheet.
3. Changed the ABI documentation to make it more generic.
4. Added ABI documentation for double tap quiet period.
5. Added available list by registering new event attribute for tap
   threshold values and double tap quiet period values.
6. Sending both single and double tap events separately.
8. Removed checking for tap enabled while changing data rate.
9. Returning invalid with error message if the input data rate is not
   200Hz while enabling tap interrupts.
7. Added datasheet reference for interrupt engine overrun.

Jagath Jog J (2):
  iio: Add new event type gesture and use direction for single and
    double tap
  iio: accel: bma400: Add support for single and double tap events

 Documentation/ABI/testing/sysfs-bus-iio |  24 +++
 drivers/iio/accel/bma400.h              |  11 ++
 drivers/iio/accel/bma400_core.c         | 210 ++++++++++++++++++++++--
 drivers/iio/industrialio-event.c        |   5 +-
 include/uapi/linux/iio/types.h          |   3 +
 tools/iio/iio_event_monitor.c           |   8 +-
 6 files changed, 249 insertions(+), 12 deletions(-)


base-commit: 9c50b2e70d55e088400b828d441b42254eae94b4
-- 
2.17.1

[PATCH v1 1/2] iio: Add new event type gesture and use direction for single and double tap

[Jagath Jog J]

Add new event type for tap called gesture and the direction can be used
to differentiate single and double tap. This may be used by accelerometer
sensors to express single and double tap events. For directional tap,
modifiers like IIO_MOD_(X/Y/Z) can be used along with singletap and
doubletap direction.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 24 ++++++++++++++++++++++++
 drivers/iio/industrialio-event.c        |  5 ++++-
 include/uapi/linux/iio/types.h          |  3 +++
 tools/iio/iio_event_monitor.c           |  8 +++++++-
 4 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 3e00d7f7ee22..4eaf85e01911 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2035,3 +2035,27 @@ Description:
 		Available range for the forced calibration value, expressed as:
 
 		- a range specified as "[min step max]"
+
+What:		/sys/.../events/in_accel_gesture_singletap_en
+What:		/sys/.../events/in_accel_gesture_doubletap_en
+KernelVersion:	5.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Device generates an event on a single or double tap.
+
+What:		/sys/.../events/in_accel_gesture_singletap_value
+What:		/sys/.../events/in_accel_gesture_doubletap_value
+KernelVersion:	5.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Specifies the threshold value that the device is comparing
+		against to generate the tap gesture event. Units and exact
+		meaning of value are device specific.
+
+What:		/sys/.../events/in_accel_gesture_doubletap_period
+KernelVersion:	5.19
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Minimum time period between before and after the double tap
+		event. Units and exact meaning of period value are device
+		specific.
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index b5e059e15b0a..22d59eb0a8a2 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -231,12 +231,15 @@ static const char * const iio_ev_type_text[] = {
 	[IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
 	[IIO_EV_TYPE_CHANGE] = "change",
 	[IIO_EV_TYPE_MAG_REFERENCED] = "mag_referenced",
+	[IIO_EV_TYPE_GESTURE] = "gesture",
 };
 
 static const char * const iio_ev_dir_text[] = {
 	[IIO_EV_DIR_EITHER] = "either",
 	[IIO_EV_DIR_RISING] = "rising",
-	[IIO_EV_DIR_FALLING] = "falling"
+	[IIO_EV_DIR_FALLING] = "falling",
+	[IIO_EV_DIR_SINGLETAP] = "singletap",
+	[IIO_EV_DIR_DOUBLETAP] = "doubletap",
 };
 
 static const char * const iio_ev_info_text[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 472cead10d8d..913864221ac4 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -105,6 +105,7 @@ enum iio_event_type {
 	IIO_EV_TYPE_MAG_ADAPTIVE,
 	IIO_EV_TYPE_CHANGE,
 	IIO_EV_TYPE_MAG_REFERENCED,
+	IIO_EV_TYPE_GESTURE,
 };
 
 enum iio_event_direction {
@@ -112,6 +113,8 @@ enum iio_event_direction {
 	IIO_EV_DIR_RISING,
 	IIO_EV_DIR_FALLING,
 	IIO_EV_DIR_NONE,
+	IIO_EV_DIR_SINGLETAP,
+	IIO_EV_DIR_DOUBLETAP,
 };
 
 #endif /* _UAPI_IIO_TYPES_H_ */
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index 2f4581658859..b3b3ea399f67 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -69,12 +69,15 @@ static const char * const iio_ev_type_text[] = {
 	[IIO_EV_TYPE_MAG_ADAPTIVE] = "mag_adaptive",
 	[IIO_EV_TYPE_CHANGE] = "change",
 	[IIO_EV_TYPE_MAG_REFERENCED] = "mag_referenced",
+	[IIO_EV_TYPE_GESTURE] = "gesture",
 };
 
 static const char * const iio_ev_dir_text[] = {
 	[IIO_EV_DIR_EITHER] = "either",
 	[IIO_EV_DIR_RISING] = "rising",
-	[IIO_EV_DIR_FALLING] = "falling"
+	[IIO_EV_DIR_FALLING] = "falling",
+	[IIO_EV_DIR_SINGLETAP] = "singletap",
+	[IIO_EV_DIR_DOUBLETAP] = "doubletap",
 };
 
 static const char * const iio_modifier_names[] = {
@@ -227,6 +230,7 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_EV_TYPE_THRESH_ADAPTIVE:
 	case IIO_EV_TYPE_MAG_ADAPTIVE:
 	case IIO_EV_TYPE_CHANGE:
+	case IIO_EV_TYPE_GESTURE:
 		break;
 	default:
 		return false;
@@ -236,6 +240,8 @@ static bool event_is_known(struct iio_event_data *event)
 	case IIO_EV_DIR_EITHER:
 	case IIO_EV_DIR_RISING:
 	case IIO_EV_DIR_FALLING:
+	case IIO_EV_DIR_SINGLETAP:
+	case IIO_EV_DIR_DOUBLETAP:
 	case IIO_EV_DIR_NONE:
 		break;
 	default:
-- 
2.17.1

Re: [PATCH v1 1/2] iio: Add new event type gesture and use direction for single and double tap

[Jonathan Cameron]

On Tue, 14 Jun 2022 00:47:05 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Add new event type for tap called gesture and the direction can be used
> to differentiate single and double tap. This may be used by accelerometer
> sensors to express single and double tap events. For directional tap,
> modifiers like IIO_MOD_(X/Y/Z) can be used along with singletap and
> doubletap direction.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>

Hi,

With fresh eyes I think we need to rethink the use of _period to make sure
we have 'space' for another very like ABI element which is the maximum
time between events for them to be considered a double tap.

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 24 ++++++++++++++++++++++++
>  drivers/iio/industrialio-event.c        |  5 ++++-
>  include/uapi/linux/iio/types.h          |  3 +++
>  tools/iio/iio_event_monitor.c           |  8 +++++++-
>  4 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 3e00d7f7ee22..4eaf85e01911 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -2035,3 +2035,27 @@ Description:
>  		Available range for the forced calibration value, expressed as:
>  
>  		- a range specified as "[min step max]"
> +
> +What:		/sys/.../events/in_accel_gesture_singletap_en
> +What:		/sys/.../events/in_accel_gesture_doubletap_en
> +KernelVersion:	5.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Device generates an event on a single or double tap.
> +
> +What:		/sys/.../events/in_accel_gesture_singletap_value
> +What:		/sys/.../events/in_accel_gesture_doubletap_value
> +KernelVersion:	5.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Specifies the threshold value that the device is comparing
> +		against to generate the tap gesture event. Units and exact
> +		meaning of value are device specific.

I'm fine with this one being device specific, as likely a complex alg
involved.

> +
> +What:		/sys/.../events/in_accel_gesture_doubletap_period
> +KernelVersion:	5.19
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Minimum time period between before and after the double tap
> +		event. Units and exact meaning of period value are device
> +		specific.

I think the units need to be standard.  Also would this work better
as a description?

	Minimum time in seconds between the two taps making up a double
        tap event.

Raises a question though. How would we specify the maximum time? I.e.
if taps are further apart than Xseconds, they are reported as two single
taps.

Maybe reusing period here isn't a good idea and we need to have new ABI for
this?

Re: [PATCH v1 1/2] iio: Add new event type gesture and use direction for single and double tap

[Jagath Jog J]

Hi Jonathan,

Sorry for the delay in replying.

On Sun, Jun 19, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 14 Jun 2022 00:47:05 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Add new event type for tap called gesture and the direction can be used
> > to differentiate single and double tap. This may be used by accelerometer
> > sensors to express single and double tap events. For directional tap,
> > modifiers like IIO_MOD_(X/Y/Z) can be used along with singletap and
> > doubletap direction.
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
>
> Hi,
>
> With fresh eyes I think we need to rethink the use of _period to make sure
> we have 'space' for another very like ABI element which is the maximum
> time between events for them to be considered a double tap.
>
> Jonathan
>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 24 ++++++++++++++++++++++++
> >  drivers/iio/industrialio-event.c        |  5 ++++-
> >  include/uapi/linux/iio/types.h          |  3 +++
> >  tools/iio/iio_event_monitor.c           |  8 +++++++-
> >  4 files changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index 3e00d7f7ee22..4eaf85e01911 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -2035,3 +2035,27 @@ Description:
> >               Available range for the forced calibration value, expressed as:
> >
> >               - a range specified as "[min step max]"
> > +
> > +What:                /sys/.../events/in_accel_gesture_singletap_en
> > +What:                /sys/.../events/in_accel_gesture_doubletap_en
> > +KernelVersion:       5.19
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Device generates an event on a single or double tap.
> > +
> > +What:                /sys/.../events/in_accel_gesture_singletap_value
> > +What:                /sys/.../events/in_accel_gesture_doubletap_value
> > +KernelVersion:       5.19
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Specifies the threshold value that the device is comparing
> > +             against to generate the tap gesture event. Units and exact
> > +             meaning of value are device specific.
>
> I'm fine with this one being device specific, as likely a complex alg
> involved.
>
> > +
> > +What:                /sys/.../events/in_accel_gesture_doubletap_period
> > +KernelVersion:       5.19
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Minimum time period between before and after the double tap
> > +             event. Units and exact meaning of period value are device
> > +             specific.
>
> I think the units need to be standard.  Also would this work better
> as a description?
>
>         Minimum time in seconds between the two taps making up a double
>         tap event.

The values for the time between the two taps are not in terms of seconds,
here period value is in terms of data samples which depends on
the data rate.

>
> Raises a question though. How would we specify the maximum time? I.e.
> if taps are further apart than Xseconds, they are reported as two single
> taps.
>
> Maybe reusing period here isn't a good idea and we need to have new ABI for
> this?
>
>
>
>

Re: [PATCH v1 1/2] iio: Add new event type gesture and use direction for single and double tap

[Jonathan Cameron]

On Sun, 26 Jun 2022 18:51:54 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Jonathan,
> 
> Sorry for the delay in replying.
> 
> On Sun, Jun 19, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 14 Jun 2022 00:47:05 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >  
> > > Add new event type for tap called gesture and the direction can be used
> > > to differentiate single and double tap. This may be used by accelerometer
> > > sensors to express single and double tap events. For directional tap,
> > > modifiers like IIO_MOD_(X/Y/Z) can be used along with singletap and
> > > doubletap direction.
> > >
> > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>  
> >
> > Hi,
> >
> > With fresh eyes I think we need to rethink the use of _period to make sure
> > we have 'space' for another very like ABI element which is the maximum
> > time between events for them to be considered a double tap.
> >
> > Jonathan
> >  
> > > ---
> > >  Documentation/ABI/testing/sysfs-bus-iio | 24 ++++++++++++++++++++++++
> > >  drivers/iio/industrialio-event.c        |  5 ++++-
> > >  include/uapi/linux/iio/types.h          |  3 +++
> > >  tools/iio/iio_event_monitor.c           |  8 +++++++-
> > >  4 files changed, 38 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index 3e00d7f7ee22..4eaf85e01911 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -2035,3 +2035,27 @@ Description:
> > >               Available range for the forced calibration value, expressed as:
> > >
> > >               - a range specified as "[min step max]"
> > > +
> > > +What:                /sys/.../events/in_accel_gesture_singletap_en
> > > +What:                /sys/.../events/in_accel_gesture_doubletap_en
> > > +KernelVersion:       5.19
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Device generates an event on a single or double tap.
> > > +
> > > +What:                /sys/.../events/in_accel_gesture_singletap_value
> > > +What:                /sys/.../events/in_accel_gesture_doubletap_value
> > > +KernelVersion:       5.19
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Specifies the threshold value that the device is comparing
> > > +             against to generate the tap gesture event. Units and exact
> > > +             meaning of value are device specific.  
> >
> > I'm fine with this one being device specific, as likely a complex alg
> > involved.
> >  
> > > +
> > > +What:                /sys/.../events/in_accel_gesture_doubletap_period
> > > +KernelVersion:       5.19
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Minimum time period between before and after the double tap
> > > +             event. Units and exact meaning of period value are device
> > > +             specific.  
> >
> > I think the units need to be standard.  Also would this work better
> > as a description?
> >
> >         Minimum time in seconds between the two taps making up a double
> >         tap event.  
> 
> The values for the time between the two taps are not in terms of seconds,
> here period value is in terms of data samples which depends on
> the data rate.

These time based controls in IIO are always defined in seconds. You'll have
to correct for the data rate in the driver.

Either just make the available options change with data rate, or have the
driver do a 'nearest possible' choice based on what was requested and what
is possible at the current data rate.

It's more complex but it gives a consistent ABI across devices that work
in many different ways.

Jonathan

> 
> >
> > Raises a question though. How would we specify the maximum time? I.e.
> > if taps are further apart than Xseconds, they are reported as two single
> > taps.
> >
> > Maybe reusing period here isn't a good idea and we need to have new ABI for
> > this?
> >
> >
> >
> >  

Re: [PATCH v1 1/2] iio: Add new event type gesture and use direction for single and double tap

[Jagath Jog J]

Hi Jonathan,

On Sat, Jul 16, 2022 at 8:16 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 26 Jun 2022 18:51:54 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Hi Jonathan,
> >
> > Sorry for the delay in replying.
> >
> > On Sun, Jun 19, 2022 at 6:15 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Tue, 14 Jun 2022 00:47:05 +0530
> > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > >
> > > > Add new event type for tap called gesture and the direction can be used
> > > > to differentiate single and double tap. This may be used by accelerometer
> > > > sensors to express single and double tap events. For directional tap,
> > > > modifiers like IIO_MOD_(X/Y/Z) can be used along with singletap and
> > > > doubletap direction.
> > > >
> > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > >
> > > Hi,
> > >
> > > With fresh eyes I think we need to rethink the use of _period to make sure
> > > we have 'space' for another very like ABI element which is the maximum
> > > time between events for them to be considered a double tap.
> > >
> > > Jonathan
> > >
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-bus-iio | 24 ++++++++++++++++++++++++
> > > >  drivers/iio/industrialio-event.c        |  5 ++++-
> > > >  include/uapi/linux/iio/types.h          |  3 +++
> > > >  tools/iio/iio_event_monitor.c           |  8 +++++++-
> > > >  4 files changed, 38 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > > index 3e00d7f7ee22..4eaf85e01911 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > > @@ -2035,3 +2035,27 @@ Description:
> > > >               Available range for the forced calibration value, expressed as:
> > > >
> > > >               - a range specified as "[min step max]"
> > > > +
> > > > +What:                /sys/.../events/in_accel_gesture_singletap_en
> > > > +What:                /sys/.../events/in_accel_gesture_doubletap_en
> > > > +KernelVersion:       5.19
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             Device generates an event on a single or double tap.
> > > > +
> > > > +What:                /sys/.../events/in_accel_gesture_singletap_value
> > > > +What:                /sys/.../events/in_accel_gesture_doubletap_value
> > > > +KernelVersion:       5.19
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             Specifies the threshold value that the device is comparing
> > > > +             against to generate the tap gesture event. Units and exact
> > > > +             meaning of value are device specific.
> > >
> > > I'm fine with this one being device specific, as likely a complex alg
> > > involved.
> > >
> > > > +
> > > > +What:                /sys/.../events/in_accel_gesture_doubletap_period
> > > > +KernelVersion:       5.19
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             Minimum time period between before and after the double tap
> > > > +             event. Units and exact meaning of period value are device
> > > > +             specific.
> > >
> > > I think the units need to be standard.  Also would this work better
> > > as a description?
> > >
> > >         Minimum time in seconds between the two taps making up a double
> > >         tap event.
> >
> > The values for the time between the two taps are not in terms of seconds,
> > here period value is in terms of data samples which depends on
> > the data rate.
>
> These time based controls in IIO are always defined in seconds. You'll have
> to correct for the data rate in the driver.
>
> Either just make the available options change with data rate, or have the
> driver do a 'nearest possible' choice based on what was requested and what
> is possible at the current data rate.
>
> It's more complex but it gives a consistent ABI across devices that work
> in many different ways.

TAP interrupts work with a 200 Hz data rate, so all the time-related
config values
are converted to seconds and provided the available options.
I have prepared the v2 and soon I will send the same.

Thank you,
Jagath


>
> Jonathan
>
> >
> > >
> > > Raises a question though. How would we specify the maximum time? I.e.
> > > if taps are further apart than Xseconds, they are reported as two single
> > > taps.
> > >
> > > Maybe reusing period here isn't a good idea and we need to have new ABI for
> > > this?
> > >
> > >
> > >
> > >
>

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

[Jagath Jog J]

Add support for single and double tap events based on the tap threshold
value and minimum quiet time value between the taps. The INT1 pin is used
to interrupt and event is pushed to userspace.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 drivers/iio/accel/bma400.h      |  11 ++
 drivers/iio/accel/bma400_core.c | 210 ++++++++++++++++++++++++++++++--
 2 files changed, 211 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
index e8f802a82300..d05edd9b009a 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_QUIET_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 c31bdd9b168e..f8945dc0de04 100644
--- a/drivers/iio/accel/bma400_core.c
+++ b/drivers/iio/accel/bma400_core.c
@@ -29,6 +29,7 @@
 #include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/iio/sysfs.h>
 
 #include "bma400.h"
 
@@ -88,6 +89,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 +218,36 @@ 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) |
+				       BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+/* List of sensitivity values available to configure tap interrupts */
+static IIO_CONST_ATTR(in_accel_gesture_value_available, "0 1 2 3 4 5 6 7");
+
+/* List of minimum quiet time before and after double tap, in data samples. */
+static IIO_CONST_ATTR(in_accel_gesture_doubletap_period_available,
+		      "60 80 100 120");
+
+static struct attribute *bma400_event_attributes[] = {
+	&iio_const_attr_in_accel_gesture_value_available.dev_attr.attr,
+	&iio_const_attr_in_accel_gesture_doubletap_period_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group bma400_event_attribute_group = {
+	.attrs = bma400_event_attributes,
 };
 
 #define BMA400_ACC_CHANNEL(_index, _axis) { \
@@ -1012,6 +1044,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 +1137,80 @@ static int bma400_activity_event_en(struct bma400_data *data,
 	return 0;
 }
 
+static int bma400_tap_event_en(struct bma400_data *data,
+			       enum iio_event_direction dir, int state)
+{
+	int ret;
+	unsigned int mask, field_value;
+
+	/*
+	 * Tap interrupts can be configured only in normal mode.
+	 * See table in section 4.3 "Power modes - performance modes" of
+	 * datasheet v1.2.
+	 */
+	if (data->power_mode != POWER_MODE_NORMAL)
+		return -EINVAL;
+
+	/*
+	 * Tap interrupts are operating with the data rate of 200Hz.
+	 * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
+	 */
+	if (data->sample_freq.hz != 200) {
+		dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
+				 BMA400_S_TAP_MSK,
+				 FIELD_PREP(BMA400_S_TAP_MSK, state));
+	if (ret)
+		return ret;
+
+	switch (dir) {
+	case IIO_EV_DIR_SINGLETAP:
+		mask = BMA400_S_TAP_MSK;
+		set_mask_bits(&field_value, BMA400_S_TAP_MSK,
+			      FIELD_PREP(BMA400_S_TAP_MSK, state));
+		break;
+	case IIO_EV_DIR_DOUBLETAP:
+		mask = BMA400_D_TAP_MSK;
+		set_mask_bits(&field_value, BMA400_D_TAP_MSK,
+			      FIELD_PREP(BMA400_D_TAP_MSK, state));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
+				 field_value);
+	if (ret)
+		return ret;
+
+	set_mask_bits(&data->tap_event_en, mask, field_value);
+
+	return 0;
+}
+
+static int bma400_disable_adv_interrupt(struct bma400_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, BMA400_INT_CONFIG0_REG, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, BMA400_INT_CONFIG1_REG, 0);
+	if (ret)
+		return ret;
+
+	data->tap_event_en = 0;
+	data->generic_event_en = 0;
+	data->step_event_en = 0;
+	data->activity_event_en = 0;
+
+	return 0;
+}
+
 static int bma400_write_event_config(struct iio_dev *indio_dev,
 				     const struct iio_chan_spec *chan,
 				     enum iio_event_type type,
@@ -1111,10 +1221,20 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
 
 	switch (chan->type) {
 	case IIO_ACCEL:
-		mutex_lock(&data->mutex);
-		ret = bma400_activity_event_en(data, dir, state);
-		mutex_unlock(&data->mutex);
-		return ret;
+		switch (type) {
+		case IIO_EV_TYPE_MAG:
+			mutex_lock(&data->mutex);
+			ret = bma400_activity_event_en(data, dir, state);
+			mutex_unlock(&data->mutex);
+			return ret;
+		case IIO_EV_TYPE_GESTURE:
+			mutex_lock(&data->mutex);
+			ret = bma400_tap_event_en(data, dir, state);
+			mutex_unlock(&data->mutex);
+			return ret;
+		default:
+			return -EINVAL;
+		}
 	case IIO_STEPS:
 		mutex_lock(&data->mutex);
 		ret = bma400_steps_event_enable(data, state);
@@ -1159,8 +1279,8 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 	struct bma400_data *data = iio_priv(indio_dev);
 	int ret, reg;
 
-	switch (chan->type) {
-	case IIO_ACCEL:
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
 		reg = get_gen_config_reg(dir);
 		if (reg < 0)
 			return -EINVAL;
@@ -1196,6 +1316,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_EV_TYPE_GESTURE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
+					  val);
+			if (ret)
+				return ret;
+			*val = FIELD_GET(BMA400_TAP_SEN_MSK, *val);
+			return IIO_VAL_INT;
+		case IIO_EV_INFO_PERIOD:
+			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
+					  val);
+			if (ret)
+				return ret;
+			*val = FIELD_GET(BMA400_TAP_QUIET_MSK, *val);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -1209,10 +1348,10 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 				    int val, int val2)
 {
 	struct bma400_data *data = iio_priv(indio_dev);
-	int reg, ret;
+	int reg, ret, quiet_period;
 
-	switch (chan->type) {
-	case IIO_ACCEL:
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
 		reg = get_gen_config_reg(dir);
 		if (reg < 0)
 			return -EINVAL;
@@ -1228,7 +1367,6 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 		case IIO_EV_INFO_PERIOD:
 			if (val < 1 || val > 65535)
 				return -EINVAL;
-
 			mutex_lock(&data->mutex);
 			put_unaligned_be16(val, &data->duration);
 			ret = regmap_bulk_write(data->regmap,
@@ -1248,6 +1386,31 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
 		default:
 			return -EINVAL;
 		}
+	case IIO_EV_TYPE_GESTURE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			if (val < 0 || val > 7)
+				return -EINVAL;
+
+			return regmap_update_bits(data->regmap,
+						  BMA400_TAP_CONFIG,
+						  BMA400_TAP_SEN_MSK,
+						  FIELD_PREP(BMA400_TAP_SEN_MSK,
+							     val));
+
+		case IIO_EV_INFO_PERIOD:
+			quiet_period = (val / 20) - 3;
+			if (quiet_period < 0 || quiet_period > 3)
+				return -EINVAL;
+
+			return regmap_update_bits(data->regmap,
+						  BMA400_TAP_CONFIG1,
+						  BMA400_TAP_QUIET_MSK,
+						  FIELD_PREP(BMA400_TAP_QUIET_MSK,
+							     quiet_period));
+		default:
+			return -EINVAL;
+		}
 	default:
 		return -EINVAL;
 	}
@@ -1287,6 +1450,7 @@ static const struct iio_info bma400_info = {
 	.write_event_config = bma400_write_event_config,
 	.write_event_value = bma400_write_event_value,
 	.read_event_value = bma400_read_event_value,
+	.event_attrs = &bma400_event_attribute_group,
 };
 
 static const struct iio_trigger_ops bma400_trigger_ops = {
@@ -1350,6 +1514,32 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
 	if (ret || !data->status)
 		goto unlock_err;
 
+	/*
+	 * Disable all advance interrupts if interrupt engine overrun occurs.
+	 * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
+	 */
+	if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
+		bma400_disable_adv_interrupt(data);
+		dev_err(data->dev, "Interrupt engine overrun\n");
+		goto unlock_err;
+	}
+
+	if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
+		iio_push_event(indio_dev,
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+						  IIO_MOD_X_OR_Y_OR_Z,
+						  IIO_EV_TYPE_GESTURE,
+						  IIO_EV_DIR_SINGLETAP),
+			       timestamp);
+
+	if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
+		iio_push_event(indio_dev,
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+						  IIO_MOD_X_OR_Y_OR_Z,
+						  IIO_EV_TYPE_GESTURE,
+						  IIO_EV_DIR_DOUBLETAP),
+			       timestamp);
+
 	if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
 		ev_dir = IIO_EV_DIR_RISING;
 
-- 
2.17.1

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

[Andy Shevchenko]

On Mon, Jun 13, 2022 at 9:17 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Add support for single and double tap events based on the tap threshold
> value and minimum quiet time value between the taps. The INT1 pin is used
> to interrupt and event is pushed to userspace.

the event

...

>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/sysfs.h>

s is before t

...

>         bool step_event_en;
>         bool activity_event_en;
>         unsigned int generic_event_en;
> +       unsigned int tap_event_en;

Should it be boolean? Or i.o.w. why does it need to be an unsigned int?

...

> +       data->step_event_en = 0;
> +       data->activity_event_en = 0;

These are booleans.

-- 
With Best Regards,
Andy Shevchenko

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

[Jagath Jog J]

Hi Andy,

On Tue, Jun 14, 2022 at 2:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Jun 13, 2022 at 9:17 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > Add support for single and double tap events based on the tap threshold
> > value and minimum quiet time value between the taps. The INT1 pin is used
> > to interrupt and event is pushed to userspace.
>
> the event
>
> ...
>
> >  #include <linux/iio/trigger.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/sysfs.h>
>
> s is before t

Sure I will arrange this in alphabetical order.

>
> ...
>
> >         bool step_event_en;
> >         bool activity_event_en;
> >         unsigned int generic_event_en;
> > +       unsigned int tap_event_en;
>
> Should it be boolean? Or i.o.w. why does it need to be an unsigned int?

tap_event_en is used as a bitmask. Bit 2 and 3 of tap_event_en
are used to tell the state of the single and double tap events.
For example bit 2 is used to indicate single tap is enabled or not.

set_mask_bits() and FIELD_GET()  is used to set and retrieve the
required bit value so I used unsigned int.



>
> ...
>
> > +       data->step_event_en = 0;
> > +       data->activity_event_en = 0;
>
> These are booleans.
>
> --
> With Best Regards,
> Andy Shevchenko

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

[Jonathan Cameron]

> >
> > ...
> >  
> > >         bool step_event_en;
> > >         bool activity_event_en;
> > >         unsigned int generic_event_en;
> > > +       unsigned int tap_event_en;  
> >
> > Should it be boolean? Or i.o.w. why does it need to be an unsigned int?  
> 
> tap_event_en is used as a bitmask. Bit 2 and 3 of tap_event_en
> are used to tell the state of the single and double tap events.
> For example bit 2 is used to indicate single tap is enabled or not.
> 
> set_mask_bits() and FIELD_GET()  is used to set and retrieve the
> required bit value so I used unsigned int.

Perhaps make that obvious by calling it

tap_event_en_bitmask ?

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

[Jonathan Cameron]

On Tue, 14 Jun 2022 00:47:06 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Add support for single and double tap events based on the tap threshold
> value and minimum quiet time value between the taps. The INT1 pin is used
> to interrupt and event is pushed to userspace.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
>  drivers/iio/accel/bma400.h      |  11 ++
>  drivers/iio/accel/bma400_core.c | 210 ++++++++++++++++++++++++++++++--
>  2 files changed, 211 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> index e8f802a82300..d05edd9b009a 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_QUIET_MSK        GENMASK(3, 2)

So following up on the 'what is the maximum time between double taps
question I took a look at the datasheet.

What about quiet_dt and tics_th?

quiet_dt seems to be min time between taps in a double tap, and tics_th
is about the 'tap' algorithm itself.

Interesting, tics_th is kind of the opposite of what we'd normally use period
for.  We only get event if the max to min of tap occur closer together than
this value.

quiet_dt seems a closer fit to what you documented period for.  quiet
controls min space between single taps.  So related but indirectly to
double tap detection.
If I have understood this correctly.

* If two taps are more that quiet apart then we get 2 single taps.
* If two taps are more than quiet_dt apart but less than quiet apart then
  we get a double tap.
* If two taps are less than quiet_dt apart, then we probably detect them
  as a single tap, as long as the tics_th timing condition is met.

Whilst quiet effects both single and double tap I think we would have
to define it as controls on both of them (linked by changing value of
one effecting the other).

Period doesn't fit any of these but I'm struggling to come up with
good ABI.

Maybe use "holdoff" for single tap to indicate minimum time after
first tap before we can have another one.  (lifted from naming of
controls on oscilloscope triggers).

What to call the same time for double taps (which it's a limit on how
far apart they can be) isn't obvious.  Nor is what would map to quiet_dt
a they are both very specific to double taps. 
Maybe "period" makes sense for quiet (not quiet_dt) when applied to a double
tap as it's time within which the event must have completed (got 2 taps)
but it's a stetch.

Anyhow, this needs more thought.  All suggestions welcome!


A few trivial things inline as well.



> +
>  /*
>   * 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 c31bdd9b168e..f8945dc0de04 100644
> --- a/drivers/iio/accel/bma400_core.c
> +++ b/drivers/iio/accel/bma400_core.c
> @@ -29,6 +29,7 @@
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/sysfs.h>
>  
>  #include "bma400.h"
>  
> @@ -88,6 +89,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 +218,36 @@ 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) |
> +				       BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +/* List of sensitivity values available to configure tap interrupts */
> +static IIO_CONST_ATTR(in_accel_gesture_value_available, "0 1 2 3 4 5 6 7");
> +
> +/* List of minimum quiet time before and after double tap, in data samples. */
> +static IIO_CONST_ATTR(in_accel_gesture_doubletap_period_available,
> +		      "60 80 100 120");
> +
> +static struct attribute *bma400_event_attributes[] = {
> +	&iio_const_attr_in_accel_gesture_value_available.dev_attr.attr,
> +	&iio_const_attr_in_accel_gesture_doubletap_period_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group bma400_event_attribute_group = {
> +	.attrs = bma400_event_attributes,
>  };
>  
>  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> @@ -1012,6 +1044,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 +1137,80 @@ static int bma400_activity_event_en(struct bma400_data *data,
>  	return 0;
>  }
>  
> +static int bma400_tap_event_en(struct bma400_data *data,
> +			       enum iio_event_direction dir, int state)
> +{
> +	int ret;
> +	unsigned int mask, field_value;
> +
> +	/*
> +	 * Tap interrupts can be configured only in normal mode.
> +	 * See table in section 4.3 "Power modes - performance modes" of
> +	 * datasheet v1.2.
> +	 */
> +	if (data->power_mode != POWER_MODE_NORMAL)
> +		return -EINVAL;
> +
> +	/*
> +	 * Tap interrupts are operating with the data rate of 200Hz.
> +	 * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> +	 */
> +	if (data->sample_freq.hz != 200) {
> +		dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
> +				 BMA400_S_TAP_MSK,
> +				 FIELD_PREP(BMA400_S_TAP_MSK, state));
> +	if (ret)
> +		return ret;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_SINGLETAP:
> +		mask = BMA400_S_TAP_MSK;
> +		set_mask_bits(&field_value, BMA400_S_TAP_MSK,
> +			      FIELD_PREP(BMA400_S_TAP_MSK, state));
> +		break;
> +	case IIO_EV_DIR_DOUBLETAP:
> +		mask = BMA400_D_TAP_MSK;
> +		set_mask_bits(&field_value, BMA400_D_TAP_MSK,
> +			      FIELD_PREP(BMA400_D_TAP_MSK, state));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
> +				 field_value);
> +	if (ret)
> +		return ret;
> +
> +	set_mask_bits(&data->tap_event_en, mask, field_value);
> +
> +	return 0;
> +}
> +
> +static int bma400_disable_adv_interrupt(struct bma400_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, BMA400_INT_CONFIG0_REG, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, BMA400_INT_CONFIG1_REG, 0);
> +	if (ret)
> +		return ret;
> +
> +	data->tap_event_en = 0;
> +	data->generic_event_en = 0;
> +	data->step_event_en = 0;
> +	data->activity_event_en = 0;
> +
> +	return 0;
> +}
> +
>  static int bma400_write_event_config(struct iio_dev *indio_dev,
>  				     const struct iio_chan_spec *chan,
>  				     enum iio_event_type type,
> @@ -1111,10 +1221,20 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
>  
>  	switch (chan->type) {
>  	case IIO_ACCEL:
> -		mutex_lock(&data->mutex);
> -		ret = bma400_activity_event_en(data, dir, state);
> -		mutex_unlock(&data->mutex);
> -		return ret;
> +		switch (type) {
> +		case IIO_EV_TYPE_MAG:
> +			mutex_lock(&data->mutex);
> +			ret = bma400_activity_event_en(data, dir, state);
> +			mutex_unlock(&data->mutex);
> +			return ret;
> +		case IIO_EV_TYPE_GESTURE:
> +			mutex_lock(&data->mutex);
> +			ret = bma400_tap_event_en(data, dir, state);
> +			mutex_unlock(&data->mutex);
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
>  	case IIO_STEPS:
>  		mutex_lock(&data->mutex);
>  		ret = bma400_steps_event_enable(data, state);
> @@ -1159,8 +1279,8 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
>  	struct bma400_data *data = iio_priv(indio_dev);
>  	int ret, reg;
>  
> -	switch (chan->type) {
> -	case IIO_ACCEL:
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
>  		reg = get_gen_config_reg(dir);
>  		if (reg < 0)
>  			return -EINVAL;
> @@ -1196,6 +1316,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
> +					  val);
> +			if (ret)
> +				return ret;
> +			*val = FIELD_GET(BMA400_TAP_SEN_MSK, *val);

I'd prefer a local variable for regval into which the regmap_read goes.

Slightly more readable that way as *val then only ever means one thing.

> +			return IIO_VAL_INT;
> +		case IIO_EV_INFO_PERIOD:
> +			ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> +					  val);
> +			if (ret)
> +				return ret;
> +			*val = FIELD_GET(BMA400_TAP_QUIET_MSK, *val);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1209,10 +1348,10 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
>  				    int val, int val2)
>  {
>  	struct bma400_data *data = iio_priv(indio_dev);
> -	int reg, ret;
> +	int reg, ret, quiet_period;
>  
> -	switch (chan->type) {
> -	case IIO_ACCEL:
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
>  		reg = get_gen_config_reg(dir);
>  		if (reg < 0)
>  			return -EINVAL;
> @@ -1228,7 +1367,6 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
>  		case IIO_EV_INFO_PERIOD:
>  			if (val < 1 || val > 65535)
>  				return -EINVAL;
> -

Clear out this noise.

>  			mutex_lock(&data->mutex);
>  			put_unaligned_be16(val, &data->duration);
>  			ret = regmap_bulk_write(data->regmap,
> @@ -1248,6 +1386,31 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
>  		default:
>  			return -EINVAL;
>  		}
> +	case IIO_EV_TYPE_GESTURE:
> +		switch (info) {
> +		case IIO_EV_INFO_VALUE:
> +			if (val < 0 || val > 7)
> +				return -EINVAL;
> +
> +			return regmap_update_bits(data->regmap,
> +						  BMA400_TAP_CONFIG,
> +						  BMA400_TAP_SEN_MSK,
> +						  FIELD_PREP(BMA400_TAP_SEN_MSK,
> +							     val));
> +
> +		case IIO_EV_INFO_PERIOD:
> +			quiet_period = (val / 20) - 3;
> +			if (quiet_period < 0 || quiet_period > 3)
> +				return -EINVAL;
> +
> +			return regmap_update_bits(data->regmap,
> +						  BMA400_TAP_CONFIG1,
> +						  BMA400_TAP_QUIET_MSK,
> +						  FIELD_PREP(BMA400_TAP_QUIET_MSK,
> +							     quiet_period));
> +		default:
> +			return -EINVAL;
> +		}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1287,6 +1450,7 @@ static const struct iio_info bma400_info = {
>  	.write_event_config = bma400_write_event_config,
>  	.write_event_value = bma400_write_event_value,
>  	.read_event_value = bma400_read_event_value,
> +	.event_attrs = &bma400_event_attribute_group,
>  };
>  
>  static const struct iio_trigger_ops bma400_trigger_ops = {
> @@ -1350,6 +1514,32 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
>  	if (ret || !data->status)
>  		goto unlock_err;
>  
> +	/*
> +	 * Disable all advance interrupts if interrupt engine overrun occurs.
> +	 * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
> +	 */
> +	if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
> +		bma400_disable_adv_interrupt(data);
> +		dev_err(data->dev, "Interrupt engine overrun\n");
> +		goto unlock_err;
> +	}
> +
> +	if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
> +		iio_push_event(indio_dev,
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +						  IIO_MOD_X_OR_Y_OR_Z,
> +						  IIO_EV_TYPE_GESTURE,
> +						  IIO_EV_DIR_SINGLETAP),
> +			       timestamp);
> +
> +	if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
> +		iio_push_event(indio_dev,
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> +						  IIO_MOD_X_OR_Y_OR_Z,
> +						  IIO_EV_TYPE_GESTURE,
> +						  IIO_EV_DIR_DOUBLETAP),
> +			       timestamp);
> +
>  	if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
>  		ev_dir = IIO_EV_DIR_RISING;
>  

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

[Jagath Jog J]

Hi Jonathan,

On Sun, Jun 19, 2022 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 14 Jun 2022 00:47:06 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Add support for single and double tap events based on the tap threshold
> > value and minimum quiet time value between the taps. The INT1 pin is used
> > to interrupt and event is pushed to userspace.
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > ---
> >  drivers/iio/accel/bma400.h      |  11 ++
> >  drivers/iio/accel/bma400_core.c | 210 ++++++++++++++++++++++++++++++--
> >  2 files changed, 211 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > index e8f802a82300..d05edd9b009a 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_QUIET_MSK        GENMASK(3, 2)
>
> So following up on the 'what is the maximum time between double taps
> question I took a look at the datasheet.
>
> What about quiet_dt and tics_th?

I kept both of them as default values since there is only _period attribute
which matches these values.

>
> quiet_dt seems to be min time between taps in a double tap, and tics_th
> is about the 'tap' algorithm itself.
>
> Interesting, tics_th is kind of the opposite of what we'd normally use period
> for.  We only get event if the max to min of tap occur closer together than
> this value.
>
> quiet_dt seems a closer fit to what you documented period for.  quiet
> controls min space between single taps.  So related but indirectly to
> double tap detection.
> If I have understood this correctly.
>
> * If two taps are more that quiet apart then we get 2 single taps.
> * If two taps are more than quiet_dt apart but less than quiet apart then
>   we get a double tap.
> * If two taps are less than quiet_dt apart, then we probably detect them
>   as a single tap, as long as the tics_th timing condition is met.
>
> Whilst quiet effects both single and double tap I think we would have
> to define it as controls on both of them (linked by changing value of
> one effecting the other).
>
> Period doesn't fit any of these but I'm struggling to come up with
> good ABI.
>
> Maybe use "holdoff" for single tap to indicate minimum time after
> first tap before we can have another one.  (lifted from naming of
> controls on oscilloscope triggers).
>
> What to call the same time for double taps (which it's a limit on how
> far apart they can be) isn't obvious.  Nor is what would map to quiet_dt
> a they are both very specific to double taps.
> Maybe "period" makes sense for quiet (not quiet_dt) when applied to a double
> tap as it's time within which the event must have completed (got 2 taps)
> but it's a stetch.
>
> Anyhow, this needs more thought.  All suggestions welcome!

Thanks for the suggestion,
For double-tap, the quiet value represents window time which means both taps
should occur within this period of time.

Please give me suggestions for the name "pulse window" or only "window" for
double tap quiet value and "latency" for the quiet_dt.

Most of the accelerometer sensors which support tap feature define
quite, quite_dt
register field with different names then, can I add these attributes in the core
with the final suggested name?

Since tics_th is specific to bma400 can I define this by creating custom
attributes with the same name using IIO_DEVICE_ATTR_RW()?

Thank you,
Jagath


>
>
> A few trivial things inline as well.
>
>
>
> > +
> >  /*
> >   * 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 c31bdd9b168e..f8945dc0de04 100644
> > --- a/drivers/iio/accel/bma400_core.c
> > +++ b/drivers/iio/accel/bma400_core.c
> > @@ -29,6 +29,7 @@
> >  #include <linux/iio/trigger.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/sysfs.h>
> >
> >  #include "bma400.h"
> >
> > @@ -88,6 +89,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 +218,36 @@ 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) |
> > +                                    BIT(IIO_EV_INFO_ENABLE),
> > +     },
> > +};
> > +
> > +/* List of sensitivity values available to configure tap interrupts */
> > +static IIO_CONST_ATTR(in_accel_gesture_value_available, "0 1 2 3 4 5 6 7");
> > +
> > +/* List of minimum quiet time before and after double tap, in data samples. */
> > +static IIO_CONST_ATTR(in_accel_gesture_doubletap_period_available,
> > +                   "60 80 100 120");
> > +
> > +static struct attribute *bma400_event_attributes[] = {
> > +     &iio_const_attr_in_accel_gesture_value_available.dev_attr.attr,
> > +     &iio_const_attr_in_accel_gesture_doubletap_period_available.dev_attr.attr,
> > +     NULL
> > +};
> > +
> > +static const struct attribute_group bma400_event_attribute_group = {
> > +     .attrs = bma400_event_attributes,
> >  };
> >
> >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > @@ -1012,6 +1044,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 +1137,80 @@ static int bma400_activity_event_en(struct bma400_data *data,
> >       return 0;
> >  }
> >
> > +static int bma400_tap_event_en(struct bma400_data *data,
> > +                            enum iio_event_direction dir, int state)
> > +{
> > +     int ret;
> > +     unsigned int mask, field_value;
> > +
> > +     /*
> > +      * Tap interrupts can be configured only in normal mode.
> > +      * See table in section 4.3 "Power modes - performance modes" of
> > +      * datasheet v1.2.
> > +      */
> > +     if (data->power_mode != POWER_MODE_NORMAL)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * Tap interrupts are operating with the data rate of 200Hz.
> > +      * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> > +      */
> > +     if (data->sample_freq.hz != 200) {
> > +             dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
> > +                              BMA400_S_TAP_MSK,
> > +                              FIELD_PREP(BMA400_S_TAP_MSK, state));
> > +     if (ret)
> > +             return ret;
> > +
> > +     switch (dir) {
> > +     case IIO_EV_DIR_SINGLETAP:
> > +             mask = BMA400_S_TAP_MSK;
> > +             set_mask_bits(&field_value, BMA400_S_TAP_MSK,
> > +                           FIELD_PREP(BMA400_S_TAP_MSK, state));
> > +             break;
> > +     case IIO_EV_DIR_DOUBLETAP:
> > +             mask = BMA400_D_TAP_MSK;
> > +             set_mask_bits(&field_value, BMA400_D_TAP_MSK,
> > +                           FIELD_PREP(BMA400_D_TAP_MSK, state));
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
> > +                              field_value);
> > +     if (ret)
> > +             return ret;
> > +
> > +     set_mask_bits(&data->tap_event_en, mask, field_value);
> > +
> > +     return 0;
> > +}
> > +
> > +static int bma400_disable_adv_interrupt(struct bma400_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG0_REG, 0);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG1_REG, 0);
> > +     if (ret)
> > +             return ret;
> > +
> > +     data->tap_event_en = 0;
> > +     data->generic_event_en = 0;
> > +     data->step_event_en = 0;
> > +     data->activity_event_en = 0;
> > +
> > +     return 0;
> > +}
> > +
> >  static int bma400_write_event_config(struct iio_dev *indio_dev,
> >                                    const struct iio_chan_spec *chan,
> >                                    enum iio_event_type type,
> > @@ -1111,10 +1221,20 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> >
> >       switch (chan->type) {
> >       case IIO_ACCEL:
> > -             mutex_lock(&data->mutex);
> > -             ret = bma400_activity_event_en(data, dir, state);
> > -             mutex_unlock(&data->mutex);
> > -             return ret;
> > +             switch (type) {
> > +             case IIO_EV_TYPE_MAG:
> > +                     mutex_lock(&data->mutex);
> > +                     ret = bma400_activity_event_en(data, dir, state);
> > +                     mutex_unlock(&data->mutex);
> > +                     return ret;
> > +             case IIO_EV_TYPE_GESTURE:
> > +                     mutex_lock(&data->mutex);
> > +                     ret = bma400_tap_event_en(data, dir, state);
> > +                     mutex_unlock(&data->mutex);
> > +                     return ret;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> >       case IIO_STEPS:
> >               mutex_lock(&data->mutex);
> >               ret = bma400_steps_event_enable(data, state);
> > @@ -1159,8 +1279,8 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> >       struct bma400_data *data = iio_priv(indio_dev);
> >       int ret, reg;
> >
> > -     switch (chan->type) {
> > -     case IIO_ACCEL:
> > +     switch (type) {
> > +     case IIO_EV_TYPE_MAG:
> >               reg = get_gen_config_reg(dir);
> >               if (reg < 0)
> >                       return -EINVAL;
> > @@ -1196,6 +1316,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> >               default:
> >                       return -EINVAL;
> >               }
> > +     case IIO_EV_TYPE_GESTURE:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
> > +                                       val);
> > +                     if (ret)
> > +                             return ret;
> > +                     *val = FIELD_GET(BMA400_TAP_SEN_MSK, *val);
>
> I'd prefer a local variable for regval into which the regmap_read goes.
>
> Slightly more readable that way as *val then only ever means one thing.
>
> > +                     return IIO_VAL_INT;
> > +             case IIO_EV_INFO_PERIOD:
> > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> > +                                       val);
> > +                     if (ret)
> > +                             return ret;
> > +                     *val = FIELD_GET(BMA400_TAP_QUIET_MSK, *val);
> > +                     return IIO_VAL_INT;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -1209,10 +1348,10 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> >                                   int val, int val2)
> >  {
> >       struct bma400_data *data = iio_priv(indio_dev);
> > -     int reg, ret;
> > +     int reg, ret, quiet_period;
> >
> > -     switch (chan->type) {
> > -     case IIO_ACCEL:
> > +     switch (type) {
> > +     case IIO_EV_TYPE_MAG:
> >               reg = get_gen_config_reg(dir);
> >               if (reg < 0)
> >                       return -EINVAL;
> > @@ -1228,7 +1367,6 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> >               case IIO_EV_INFO_PERIOD:
> >                       if (val < 1 || val > 65535)
> >                               return -EINVAL;
> > -
>
> Clear out this noise.
>
> >                       mutex_lock(&data->mutex);
> >                       put_unaligned_be16(val, &data->duration);
> >                       ret = regmap_bulk_write(data->regmap,
> > @@ -1248,6 +1386,31 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> >               default:
> >                       return -EINVAL;
> >               }
> > +     case IIO_EV_TYPE_GESTURE:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     if (val < 0 || val > 7)
> > +                             return -EINVAL;
> > +
> > +                     return regmap_update_bits(data->regmap,
> > +                                               BMA400_TAP_CONFIG,
> > +                                               BMA400_TAP_SEN_MSK,
> > +                                               FIELD_PREP(BMA400_TAP_SEN_MSK,
> > +                                                          val));
> > +
> > +             case IIO_EV_INFO_PERIOD:
> > +                     quiet_period = (val / 20) - 3;
> > +                     if (quiet_period < 0 || quiet_period > 3)
> > +                             return -EINVAL;
> > +
> > +                     return regmap_update_bits(data->regmap,
> > +                                               BMA400_TAP_CONFIG1,
> > +                                               BMA400_TAP_QUIET_MSK,
> > +                                               FIELD_PREP(BMA400_TAP_QUIET_MSK,
> > +                                                          quiet_period));
> > +             default:
> > +                     return -EINVAL;
> > +             }
> >       default:
> >               return -EINVAL;
> >       }
> > @@ -1287,6 +1450,7 @@ static const struct iio_info bma400_info = {
> >       .write_event_config = bma400_write_event_config,
> >       .write_event_value = bma400_write_event_value,
> >       .read_event_value = bma400_read_event_value,
> > +     .event_attrs = &bma400_event_attribute_group,
> >  };
> >
> >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > @@ -1350,6 +1514,32 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> >       if (ret || !data->status)
> >               goto unlock_err;
> >
> > +     /*
> > +      * Disable all advance interrupts if interrupt engine overrun occurs.
> > +      * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
> > +      */
> > +     if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
> > +             bma400_disable_adv_interrupt(data);
> > +             dev_err(data->dev, "Interrupt engine overrun\n");
> > +             goto unlock_err;
> > +     }
> > +
> > +     if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
> > +             iio_push_event(indio_dev,
> > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > +                                               IIO_EV_TYPE_GESTURE,
> > +                                               IIO_EV_DIR_SINGLETAP),
> > +                            timestamp);
> > +
> > +     if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
> > +             iio_push_event(indio_dev,
> > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > +                                               IIO_EV_TYPE_GESTURE,
> > +                                               IIO_EV_DIR_DOUBLETAP),
> > +                            timestamp);
> > +
> >       if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
> >               ev_dir = IIO_EV_DIR_RISING;
> >
>

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

[Jonathan Cameron]

On Sun, 26 Jun 2022 18:53:11 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sun, Jun 19, 2022 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 14 Jun 2022 00:47:06 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >  
> > > Add support for single and double tap events based on the tap threshold
> > > value and minimum quiet time value between the taps. The INT1 pin is used
> > > to interrupt and event is pushed to userspace.
> > >
> > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > > ---
> > >  drivers/iio/accel/bma400.h      |  11 ++
> > >  drivers/iio/accel/bma400_core.c | 210 ++++++++++++++++++++++++++++++--
> > >  2 files changed, 211 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > index e8f802a82300..d05edd9b009a 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_QUIET_MSK        GENMASK(3, 2)  
> >
> > So following up on the 'what is the maximum time between double taps
> > question I took a look at the datasheet.
> >
> > What about quiet_dt and tics_th?  
> 
> I kept both of them as default values since there is only _period attribute
> which matches these values.

Understood.  Definitely feels like we are stretching that ABI too far.

> 
> >
> > quiet_dt seems to be min time between taps in a double tap, and tics_th
> > is about the 'tap' algorithm itself.
> >
> > Interesting, tics_th is kind of the opposite of what we'd normally use period
> > for.  We only get event if the max to min of tap occur closer together than
> > this value.
> >
> > quiet_dt seems a closer fit to what you documented period for.  quiet
> > controls min space between single taps.  So related but indirectly to
> > double tap detection.
> > If I have understood this correctly.
> >
> > * If two taps are more that quiet apart then we get 2 single taps.
> > * If two taps are more than quiet_dt apart but less than quiet apart then
> >   we get a double tap.
> > * If two taps are less than quiet_dt apart, then we probably detect them
> >   as a single tap, as long as the tics_th timing condition is met.
> >
> > Whilst quiet effects both single and double tap I think we would have
> > to define it as controls on both of them (linked by changing value of
> > one effecting the other).
> >
> > Period doesn't fit any of these but I'm struggling to come up with
> > good ABI.
> >
> > Maybe use "holdoff" for single tap to indicate minimum time after
> > first tap before we can have another one.  (lifted from naming of
> > controls on oscilloscope triggers).
> >
> > What to call the same time for double taps (which it's a limit on how
> > far apart they can be) isn't obvious.  Nor is what would map to quiet_dt
> > a they are both very specific to double taps.
> > Maybe "period" makes sense for quiet (not quiet_dt) when applied to a double
> > tap as it's time within which the event must have completed (got 2 taps)
> > but it's a stetch.
> >
> > Anyhow, this needs more thought.  All suggestions welcome!  
> 
> Thanks for the suggestion,
> For double-tap, the quiet value represents window time which means both taps
> should occur within this period of time.
> 
> Please give me suggestions for the name "pulse window" or only "window" for
> double tap quiet value and "latency" for the quiet_dt.

Window doesn't really describe anything - it's too vague. 

> 
> Most of the accelerometer sensors which support tap feature define
> quite, quite_dt
> register field with different names then, can I add these attributes in the core
> with the final suggested name?

ABI definition is tricky.  Normal trick is to look at other similar sorts of things
(like oscilloscope triggers). https://download.tek.com/document/55W_17291_6_0.pdf (1st
hit in google)
So, one option is to use the concept of triggers resetting.

Hence quiet might work as
gesture_singletap_resettimeout (in seconds)  Until this time has passed the
detector is not reset to look for another event.

It's a bit of a stretch to use this for quiet_dt. There is a similar concept
in the above scope manual, but it's defined as AB triggers and that's complex
and doesn't map great to simple naming.
gesture_doubletap_tap2mindelay (in seconds)
maybe?  Note these are only vague suggestions.  There are a various userspace
folk who are often kind enough to give us feedback on things affecting ABI. 
So +CC
Bastien Nocera <hadess@hadess.net>
Hans de Goede <hdegoede@redhat.com>

(for Bastien and Hans: the aim here is to finally add ABI to IIO for single and double
 tap detection from accelerometers - in particular trying to come up with sensible tunables
 that are suitable for the algorithms various devices implement).

> 
> Since tics_th is specific to bma400 can I define this by creating custom
> attributes with the same name using IIO_DEVICE_ATTR_RW()?

No.  Custom ABI is fine for a driver if needed like here, but it needs to be inline with
the standard ABI such that if we make it standard later, we can hopefully make
use of it.   I'm not sure this will ever be standards, but let's aim for
something readable(ish).

_maxtomin_time  (in seconds)  might work perhaps?

I've not looked at what the input subsystem does for touch screens. Feels like they'll
have at least some of the same controls.  Perhaps there is something to be learnt
from there?

Jonathan


> 
> Thank you,
> Jagath
> 
> 
> >
> >
> > A few trivial things inline as well.
> >
> >
> >  
> > > +
> > >  /*
> > >   * 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 c31bdd9b168e..f8945dc0de04 100644
> > > --- a/drivers/iio/accel/bma400_core.c
> > > +++ b/drivers/iio/accel/bma400_core.c
> > > @@ -29,6 +29,7 @@
> > >  #include <linux/iio/trigger.h>
> > >  #include <linux/iio/trigger_consumer.h>
> > >  #include <linux/iio/triggered_buffer.h>
> > > +#include <linux/iio/sysfs.h>
> > >
> > >  #include "bma400.h"
> > >
> > > @@ -88,6 +89,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 +218,36 @@ 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) |
> > > +                                    BIT(IIO_EV_INFO_ENABLE),
> > > +     },
> > > +};
> > > +
> > > +/* List of sensitivity values available to configure tap interrupts */
> > > +static IIO_CONST_ATTR(in_accel_gesture_value_available, "0 1 2 3 4 5 6 7");
> > > +
> > > +/* List of minimum quiet time before and after double tap, in data samples. */
> > > +static IIO_CONST_ATTR(in_accel_gesture_doubletap_period_available,
> > > +                   "60 80 100 120");
> > > +
> > > +static struct attribute *bma400_event_attributes[] = {
> > > +     &iio_const_attr_in_accel_gesture_value_available.dev_attr.attr,
> > > +     &iio_const_attr_in_accel_gesture_doubletap_period_available.dev_attr.attr,
> > > +     NULL
> > > +};
> > > +
> > > +static const struct attribute_group bma400_event_attribute_group = {
> > > +     .attrs = bma400_event_attributes,
> > >  };
> > >
> > >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > @@ -1012,6 +1044,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 +1137,80 @@ static int bma400_activity_event_en(struct bma400_data *data,
> > >       return 0;
> > >  }
> > >
> > > +static int bma400_tap_event_en(struct bma400_data *data,
> > > +                            enum iio_event_direction dir, int state)
> > > +{
> > > +     int ret;
> > > +     unsigned int mask, field_value;
> > > +
> > > +     /*
> > > +      * Tap interrupts can be configured only in normal mode.
> > > +      * See table in section 4.3 "Power modes - performance modes" of
> > > +      * datasheet v1.2.
> > > +      */
> > > +     if (data->power_mode != POWER_MODE_NORMAL)
> > > +             return -EINVAL;
> > > +
> > > +     /*
> > > +      * Tap interrupts are operating with the data rate of 200Hz.
> > > +      * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> > > +      */
> > > +     if (data->sample_freq.hz != 200) {
> > > +             dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
> > > +                              BMA400_S_TAP_MSK,
> > > +                              FIELD_PREP(BMA400_S_TAP_MSK, state));
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     switch (dir) {
> > > +     case IIO_EV_DIR_SINGLETAP:
> > > +             mask = BMA400_S_TAP_MSK;
> > > +             set_mask_bits(&field_value, BMA400_S_TAP_MSK,
> > > +                           FIELD_PREP(BMA400_S_TAP_MSK, state));
> > > +             break;
> > > +     case IIO_EV_DIR_DOUBLETAP:
> > > +             mask = BMA400_D_TAP_MSK;
> > > +             set_mask_bits(&field_value, BMA400_D_TAP_MSK,
> > > +                           FIELD_PREP(BMA400_D_TAP_MSK, state));
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
> > > +                              field_value);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     set_mask_bits(&data->tap_event_en, mask, field_value);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int bma400_disable_adv_interrupt(struct bma400_data *data)
> > > +{
> > > +     int ret;
> > > +
> > > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG0_REG, 0);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG1_REG, 0);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     data->tap_event_en = 0;
> > > +     data->generic_event_en = 0;
> > > +     data->step_event_en = 0;
> > > +     data->activity_event_en = 0;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  static int bma400_write_event_config(struct iio_dev *indio_dev,
> > >                                    const struct iio_chan_spec *chan,
> > >                                    enum iio_event_type type,
> > > @@ -1111,10 +1221,20 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > >
> > >       switch (chan->type) {
> > >       case IIO_ACCEL:
> > > -             mutex_lock(&data->mutex);
> > > -             ret = bma400_activity_event_en(data, dir, state);
> > > -             mutex_unlock(&data->mutex);
> > > -             return ret;
> > > +             switch (type) {
> > > +             case IIO_EV_TYPE_MAG:
> > > +                     mutex_lock(&data->mutex);
> > > +                     ret = bma400_activity_event_en(data, dir, state);
> > > +                     mutex_unlock(&data->mutex);
> > > +                     return ret;
> > > +             case IIO_EV_TYPE_GESTURE:
> > > +                     mutex_lock(&data->mutex);
> > > +                     ret = bma400_tap_event_en(data, dir, state);
> > > +                     mutex_unlock(&data->mutex);
> > > +                     return ret;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > >       case IIO_STEPS:
> > >               mutex_lock(&data->mutex);
> > >               ret = bma400_steps_event_enable(data, state);
> > > @@ -1159,8 +1279,8 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> > >       struct bma400_data *data = iio_priv(indio_dev);
> > >       int ret, reg;
> > >
> > > -     switch (chan->type) {
> > > -     case IIO_ACCEL:
> > > +     switch (type) {
> > > +     case IIO_EV_TYPE_MAG:
> > >               reg = get_gen_config_reg(dir);
> > >               if (reg < 0)
> > >                       return -EINVAL;
> > > @@ -1196,6 +1316,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> > >               default:
> > >                       return -EINVAL;
> > >               }
> > > +     case IIO_EV_TYPE_GESTURE:
> > > +             switch (info) {
> > > +             case IIO_EV_INFO_VALUE:
> > > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
> > > +                                       val);
> > > +                     if (ret)
> > > +                             return ret;
> > > +                     *val = FIELD_GET(BMA400_TAP_SEN_MSK, *val);  
> >
> > I'd prefer a local variable for regval into which the regmap_read goes.
> >
> > Slightly more readable that way as *val then only ever means one thing.
> >  
> > > +                     return IIO_VAL_INT;
> > > +             case IIO_EV_INFO_PERIOD:
> > > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> > > +                                       val);
> > > +                     if (ret)
> > > +                             return ret;
> > > +                     *val = FIELD_GET(BMA400_TAP_QUIET_MSK, *val);
> > > +                     return IIO_VAL_INT;
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > >       default:
> > >               return -EINVAL;
> > >       }
> > > @@ -1209,10 +1348,10 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > >                                   int val, int val2)
> > >  {
> > >       struct bma400_data *data = iio_priv(indio_dev);
> > > -     int reg, ret;
> > > +     int reg, ret, quiet_period;
> > >
> > > -     switch (chan->type) {
> > > -     case IIO_ACCEL:
> > > +     switch (type) {
> > > +     case IIO_EV_TYPE_MAG:
> > >               reg = get_gen_config_reg(dir);
> > >               if (reg < 0)
> > >                       return -EINVAL;
> > > @@ -1228,7 +1367,6 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > >               case IIO_EV_INFO_PERIOD:
> > >                       if (val < 1 || val > 65535)
> > >                               return -EINVAL;
> > > -  
> >
> > Clear out this noise.
> >  
> > >                       mutex_lock(&data->mutex);
> > >                       put_unaligned_be16(val, &data->duration);
> > >                       ret = regmap_bulk_write(data->regmap,
> > > @@ -1248,6 +1386,31 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > >               default:
> > >                       return -EINVAL;
> > >               }
> > > +     case IIO_EV_TYPE_GESTURE:
> > > +             switch (info) {
> > > +             case IIO_EV_INFO_VALUE:
> > > +                     if (val < 0 || val > 7)
> > > +                             return -EINVAL;
> > > +
> > > +                     return regmap_update_bits(data->regmap,
> > > +                                               BMA400_TAP_CONFIG,
> > > +                                               BMA400_TAP_SEN_MSK,
> > > +                                               FIELD_PREP(BMA400_TAP_SEN_MSK,
> > > +                                                          val));
> > > +
> > > +             case IIO_EV_INFO_PERIOD:
> > > +                     quiet_period = (val / 20) - 3;
> > > +                     if (quiet_period < 0 || quiet_period > 3)
> > > +                             return -EINVAL;
> > > +
> > > +                     return regmap_update_bits(data->regmap,
> > > +                                               BMA400_TAP_CONFIG1,
> > > +                                               BMA400_TAP_QUIET_MSK,
> > > +                                               FIELD_PREP(BMA400_TAP_QUIET_MSK,
> > > +                                                          quiet_period));
> > > +             default:
> > > +                     return -EINVAL;
> > > +             }
> > >       default:
> > >               return -EINVAL;
> > >       }
> > > @@ -1287,6 +1450,7 @@ static const struct iio_info bma400_info = {
> > >       .write_event_config = bma400_write_event_config,
> > >       .write_event_value = bma400_write_event_value,
> > >       .read_event_value = bma400_read_event_value,
> > > +     .event_attrs = &bma400_event_attribute_group,
> > >  };
> > >
> > >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > > @@ -1350,6 +1514,32 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > >       if (ret || !data->status)
> > >               goto unlock_err;
> > >
> > > +     /*
> > > +      * Disable all advance interrupts if interrupt engine overrun occurs.
> > > +      * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
> > > +      */
> > > +     if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
> > > +             bma400_disable_adv_interrupt(data);
> > > +             dev_err(data->dev, "Interrupt engine overrun\n");
> > > +             goto unlock_err;
> > > +     }
> > > +
> > > +     if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
> > > +             iio_push_event(indio_dev,
> > > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > > +                                               IIO_EV_TYPE_GESTURE,
> > > +                                               IIO_EV_DIR_SINGLETAP),
> > > +                            timestamp);
> > > +
> > > +     if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
> > > +             iio_push_event(indio_dev,
> > > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > > +                                               IIO_EV_TYPE_GESTURE,
> > > +                                               IIO_EV_DIR_DOUBLETAP),
> > > +                            timestamp);
> > > +
> > >       if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
> > >               ev_dir = IIO_EV_DIR_RISING;
> > >  
> >  

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

[Jagath Jog J]

 Hi Jonathan,

On Sat, Jul 16, 2022 at 8:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 26 Jun 2022 18:53:11 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Hi Jonathan,
> >
> > On Sun, Jun 19, 2022 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Tue, 14 Jun 2022 00:47:06 +0530
> > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > >
> > > > Add support for single and double tap events based on the tap threshold
> > > > value and minimum quiet time value between the taps. The INT1 pin is used
> > > > to interrupt and event is pushed to userspace.
> > > >
> > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > > > ---
> > > >  drivers/iio/accel/bma400.h      |  11 ++
> > > >  drivers/iio/accel/bma400_core.c | 210 ++++++++++++++++++++++++++++++--
> > > >  2 files changed, 211 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > > index e8f802a82300..d05edd9b009a 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_QUIET_MSK        GENMASK(3, 2)
> > >
> > > So following up on the 'what is the maximum time between double taps
> > > question I took a look at the datasheet.
> > >
> > > What about quiet_dt and tics_th?
> >
> > I kept both of them as default values since there is only _period attribute
> > which matches these values.
>
> Understood.  Definitely feels like we are stretching that ABI too far.
>
> >
> > >
> > > quiet_dt seems to be min time between taps in a double tap, and tics_th
> > > is about the 'tap' algorithm itself.
> > >
> > > Interesting, tics_th is kind of the opposite of what we'd normally use period
> > > for.  We only get event if the max to min of tap occur closer together than
> > > this value.
> > >
> > > quiet_dt seems a closer fit to what you documented period for.  quiet
> > > controls min space between single taps.  So related but indirectly to
> > > double tap detection.
> > > If I have understood this correctly.
> > >
> > > * If two taps are more that quiet apart then we get 2 single taps.
> > > * If two taps are more than quiet_dt apart but less than quiet apart then
> > >   we get a double tap.
> > > * If two taps are less than quiet_dt apart, then we probably detect them
> > >   as a single tap, as long as the tics_th timing condition is met.
> > >
> > > Whilst quiet effects both single and double tap I think we would have
> > > to define it as controls on both of them (linked by changing value of
> > > one effecting the other).
> > >
> > > Period doesn't fit any of these but I'm struggling to come up with
> > > good ABI.
> > >
> > > Maybe use "holdoff" for single tap to indicate minimum time after
> > > first tap before we can have another one.  (lifted from naming of
> > > controls on oscilloscope triggers).
> > >
> > > What to call the same time for double taps (which it's a limit on how
> > > far apart they can be) isn't obvious.  Nor is what would map to quiet_dt
> > > a they are both very specific to double taps.
> > > Maybe "period" makes sense for quiet (not quiet_dt) when applied to a double
> > > tap as it's time within which the event must have completed (got 2 taps)
> > > but it's a stetch.
> > >
> > > Anyhow, this needs more thought.  All suggestions welcome!
> >
> > Thanks for the suggestion,
> > For double-tap, the quiet value represents window time which means both taps
> > should occur within this period of time.
> >
> > Please give me suggestions for the name "pulse window" or only "window" for
> > double tap quiet value and "latency" for the quiet_dt.
>
> Window doesn't really describe anything - it's too vague.
>
> >
> > Most of the accelerometer sensors which support tap feature define
> > quite, quite_dt
> > register field with different names then, can I add these attributes in the core
> > with the final suggested name?
>
> ABI definition is tricky.  Normal trick is to look at other similar sorts of things
> (like oscilloscope triggers). https://download.tek.com/document/55W_17291_6_0.pdf (1st
> hit in google)
> So, one option is to use the concept of triggers resetting.
>
> Hence quiet might work as
> gesture_singletap_resettimeout (in seconds)  Until this time has passed the
> detector is not reset to look for another event.

There is already IIO_EV_INFO_TIMEOUT defined which almost maps to the
given suggestion. Your thoughts on reusing the same for quiet
(gesture_singletap_timeout)?

>
> It's a bit of a stretch to use this for quiet_dt. There is a similar concept
> in the above scope manual, but it's defined as AB triggers and that's complex
> and doesn't map great to simple naming.
> gesture_doubletap_tap2mindelay (in seconds)

Thanks for the suggestions.
I will send v2 soon so that I can get more comments on this from others also.

> maybe?  Note these are only vague suggestions.  There are a various userspace
> folk who are often kind enough to give us feedback on things affecting ABI.
> So +CC
> Bastien Nocera <hadess@hadess.net>
> Hans de Goede <hdegoede@redhat.com>
>
> (for Bastien and Hans: the aim here is to finally add ABI to IIO for single and double
>  tap detection from accelerometers - in particular trying to come up with sensible tunables
>  that are suitable for the algorithms various devices implement).
>
> >
> > Since tics_th is specific to bma400 can I define this by creating custom
> > attributes with the same name using IIO_DEVICE_ATTR_RW()?
>
> No.  Custom ABI is fine for a driver if needed like here, but it needs to be inline with
> the standard ABI such that if we make it standard later, we can hopefully make
> use of it.   I'm not sure this will ever be standards, but let's aim for
> something readable(ish).
>
> _maxtomin_time  (in seconds)  might work perhaps?

This tics_th (_maxtomin_time) in seconds is more specific to bma400 so I thought
of providing a custom ABI for this, but the values are in floating
point. How can
I decode these floating point values (string) in the custom *_show and *_store?

To avoid floating point operations, is there any helper similar to kstrtoint()?

For other ABI like _tap2mindelay and _resettimeout are also floating
point values
but the core will convert into micro since they are added in core as
standard ABI.

>
> I've not looked at what the input subsystem does for touch screens. Feels like they'll
> have at least some of the same controls.  Perhaps there is something to be learnt
> from there?

Input subsystem is using MT protocol for reporting multi-touch and events are
reported to userspace, but I didn't find attributes related to tap
configurations
that are being read or written from userspace.

There is an adxl34x accelerometer driver in the input subsystem which supports
tap but the configuration values for tap are hardcoded in the driver.

Thanks
Jagath

Jagath
>
> Jonathan
>
>
> >
> > Thank you,
> > Jagath
> >
> >
> > >
> > >
> > > A few trivial things inline as well.
> > >
> > >
> > >
> > > > +
> > > >  /*
> > > >   * 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 c31bdd9b168e..f8945dc0de04 100644
> > > > --- a/drivers/iio/accel/bma400_core.c
> > > > +++ b/drivers/iio/accel/bma400_core.c
> > > > @@ -29,6 +29,7 @@
> > > >  #include <linux/iio/trigger.h>
> > > >  #include <linux/iio/trigger_consumer.h>
> > > >  #include <linux/iio/triggered_buffer.h>
> > > > +#include <linux/iio/sysfs.h>
> > > >
> > > >  #include "bma400.h"
> > > >
> > > > @@ -88,6 +89,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 +218,36 @@ 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) |
> > > > +                                    BIT(IIO_EV_INFO_ENABLE),
> > > > +     },
> > > > +};
> > > > +
> > > > +/* List of sensitivity values available to configure tap interrupts */
> > > > +static IIO_CONST_ATTR(in_accel_gesture_value_available, "0 1 2 3 4 5 6 7");
> > > > +
> > > > +/* List of minimum quiet time before and after double tap, in data samples. */
> > > > +static IIO_CONST_ATTR(in_accel_gesture_doubletap_period_available,
> > > > +                   "60 80 100 120");
> > > > +
> > > > +static struct attribute *bma400_event_attributes[] = {
> > > > +     &iio_const_attr_in_accel_gesture_value_available.dev_attr.attr,
> > > > +     &iio_const_attr_in_accel_gesture_doubletap_period_available.dev_attr.attr,
> > > > +     NULL
> > > > +};
> > > > +
> > > > +static const struct attribute_group bma400_event_attribute_group = {
> > > > +     .attrs = bma400_event_attributes,
> > > >  };
> > > >
> > > >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > > @@ -1012,6 +1044,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 +1137,80 @@ static int bma400_activity_event_en(struct bma400_data *data,
> > > >       return 0;
> > > >  }
> > > >
> > > > +static int bma400_tap_event_en(struct bma400_data *data,
> > > > +                            enum iio_event_direction dir, int state)
> > > > +{
> > > > +     int ret;
> > > > +     unsigned int mask, field_value;
> > > > +
> > > > +     /*
> > > > +      * Tap interrupts can be configured only in normal mode.
> > > > +      * See table in section 4.3 "Power modes - performance modes" of
> > > > +      * datasheet v1.2.
> > > > +      */
> > > > +     if (data->power_mode != POWER_MODE_NORMAL)
> > > > +             return -EINVAL;
> > > > +
> > > > +     /*
> > > > +      * Tap interrupts are operating with the data rate of 200Hz.
> > > > +      * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> > > > +      */
> > > > +     if (data->sample_freq.hz != 200) {
> > > > +             dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
> > > > +                              BMA400_S_TAP_MSK,
> > > > +                              FIELD_PREP(BMA400_S_TAP_MSK, state));
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     switch (dir) {
> > > > +     case IIO_EV_DIR_SINGLETAP:
> > > > +             mask = BMA400_S_TAP_MSK;
> > > > +             set_mask_bits(&field_value, BMA400_S_TAP_MSK,
> > > > +                           FIELD_PREP(BMA400_S_TAP_MSK, state));
> > > > +             break;
> > > > +     case IIO_EV_DIR_DOUBLETAP:
> > > > +             mask = BMA400_D_TAP_MSK;
> > > > +             set_mask_bits(&field_value, BMA400_D_TAP_MSK,
> > > > +                           FIELD_PREP(BMA400_D_TAP_MSK, state));
> > > > +             break;
> > > > +     default:
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
> > > > +                              field_value);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     set_mask_bits(&data->tap_event_en, mask, field_value);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int bma400_disable_adv_interrupt(struct bma400_data *data)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG0_REG, 0);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG1_REG, 0);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     data->tap_event_en = 0;
> > > > +     data->generic_event_en = 0;
> > > > +     data->step_event_en = 0;
> > > > +     data->activity_event_en = 0;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > >                                    const struct iio_chan_spec *chan,
> > > >                                    enum iio_event_type type,
> > > > @@ -1111,10 +1221,20 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > >
> > > >       switch (chan->type) {
> > > >       case IIO_ACCEL:
> > > > -             mutex_lock(&data->mutex);
> > > > -             ret = bma400_activity_event_en(data, dir, state);
> > > > -             mutex_unlock(&data->mutex);
> > > > -             return ret;
> > > > +             switch (type) {
> > > > +             case IIO_EV_TYPE_MAG:
> > > > +                     mutex_lock(&data->mutex);
> > > > +                     ret = bma400_activity_event_en(data, dir, state);
> > > > +                     mutex_unlock(&data->mutex);
> > > > +                     return ret;
> > > > +             case IIO_EV_TYPE_GESTURE:
> > > > +                     mutex_lock(&data->mutex);
> > > > +                     ret = bma400_tap_event_en(data, dir, state);
> > > > +                     mutex_unlock(&data->mutex);
> > > > +                     return ret;
> > > > +             default:
> > > > +                     return -EINVAL;
> > > > +             }
> > > >       case IIO_STEPS:
> > > >               mutex_lock(&data->mutex);
> > > >               ret = bma400_steps_event_enable(data, state);
> > > > @@ -1159,8 +1279,8 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > >       struct bma400_data *data = iio_priv(indio_dev);
> > > >       int ret, reg;
> > > >
> > > > -     switch (chan->type) {
> > > > -     case IIO_ACCEL:
> > > > +     switch (type) {
> > > > +     case IIO_EV_TYPE_MAG:
> > > >               reg = get_gen_config_reg(dir);
> > > >               if (reg < 0)
> > > >                       return -EINVAL;
> > > > @@ -1196,6 +1316,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > >               default:
> > > >                       return -EINVAL;
> > > >               }
> > > > +     case IIO_EV_TYPE_GESTURE:
> > > > +             switch (info) {
> > > > +             case IIO_EV_INFO_VALUE:
> > > > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
> > > > +                                       val);
> > > > +                     if (ret)
> > > > +                             return ret;
> > > > +                     *val = FIELD_GET(BMA400_TAP_SEN_MSK, *val);
> > >
> > > I'd prefer a local variable for regval into which the regmap_read goes.
> > >
> > > Slightly more readable that way as *val then only ever means one thing.
> > >
> > > > +                     return IIO_VAL_INT;
> > > > +             case IIO_EV_INFO_PERIOD:
> > > > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> > > > +                                       val);
> > > > +                     if (ret)
> > > > +                             return ret;
> > > > +                     *val = FIELD_GET(BMA400_TAP_QUIET_MSK, *val);
> > > > +                     return IIO_VAL_INT;
> > > > +             default:
> > > > +                     return -EINVAL;
> > > > +             }
> > > >       default:
> > > >               return -EINVAL;
> > > >       }
> > > > @@ -1209,10 +1348,10 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > >                                   int val, int val2)
> > > >  {
> > > >       struct bma400_data *data = iio_priv(indio_dev);
> > > > -     int reg, ret;
> > > > +     int reg, ret, quiet_period;
> > > >
> > > > -     switch (chan->type) {
> > > > -     case IIO_ACCEL:
> > > > +     switch (type) {
> > > > +     case IIO_EV_TYPE_MAG:
> > > >               reg = get_gen_config_reg(dir);
> > > >               if (reg < 0)
> > > >                       return -EINVAL;
> > > > @@ -1228,7 +1367,6 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > >               case IIO_EV_INFO_PERIOD:
> > > >                       if (val < 1 || val > 65535)
> > > >                               return -EINVAL;
> > > > -
> > >
> > > Clear out this noise.
> > >
> > > >                       mutex_lock(&data->mutex);
> > > >                       put_unaligned_be16(val, &data->duration);
> > > >                       ret = regmap_bulk_write(data->regmap,
> > > > @@ -1248,6 +1386,31 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > >               default:
> > > >                       return -EINVAL;
> > > >               }
> > > > +     case IIO_EV_TYPE_GESTURE:
> > > > +             switch (info) {
> > > > +             case IIO_EV_INFO_VALUE:
> > > > +                     if (val < 0 || val > 7)
> > > > +                             return -EINVAL;
> > > > +
> > > > +                     return regmap_update_bits(data->regmap,
> > > > +                                               BMA400_TAP_CONFIG,
> > > > +                                               BMA400_TAP_SEN_MSK,
> > > > +                                               FIELD_PREP(BMA400_TAP_SEN_MSK,
> > > > +                                                          val));
> > > > +
> > > > +             case IIO_EV_INFO_PERIOD:
> > > > +                     quiet_period = (val / 20) - 3;
> > > > +                     if (quiet_period < 0 || quiet_period > 3)
> > > > +                             return -EINVAL;
> > > > +
> > > > +                     return regmap_update_bits(data->regmap,
> > > > +                                               BMA400_TAP_CONFIG1,
> > > > +                                               BMA400_TAP_QUIET_MSK,
> > > > +                                               FIELD_PREP(BMA400_TAP_QUIET_MSK,
> > > > +                                                          quiet_period));
> > > > +             default:
> > > > +                     return -EINVAL;
> > > > +             }
> > > >       default:
> > > >               return -EINVAL;
> > > >       }
> > > > @@ -1287,6 +1450,7 @@ static const struct iio_info bma400_info = {
> > > >       .write_event_config = bma400_write_event_config,
> > > >       .write_event_value = bma400_write_event_value,
> > > >       .read_event_value = bma400_read_event_value,
> > > > +     .event_attrs = &bma400_event_attribute_group,
> > > >  };
> > > >
> > > >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > > > @@ -1350,6 +1514,32 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > > >       if (ret || !data->status)
> > > >               goto unlock_err;
> > > >
> > > > +     /*
> > > > +      * Disable all advance interrupts if interrupt engine overrun occurs.
> > > > +      * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
> > > > +      */
> > > > +     if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
> > > > +             bma400_disable_adv_interrupt(data);
> > > > +             dev_err(data->dev, "Interrupt engine overrun\n");
> > > > +             goto unlock_err;
> > > > +     }
> > > > +
> > > > +     if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
> > > > +             iio_push_event(indio_dev,
> > > > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > > > +                                               IIO_EV_TYPE_GESTURE,
> > > > +                                               IIO_EV_DIR_SINGLETAP),
> > > > +                            timestamp);
> > > > +
> > > > +     if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
> > > > +             iio_push_event(indio_dev,
> > > > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > > > +                                               IIO_EV_TYPE_GESTURE,
> > > > +                                               IIO_EV_DIR_DOUBLETAP),
> > > > +                            timestamp);
> > > > +
> > > >       if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
> > > >               ev_dir = IIO_EV_DIR_RISING;
> > > >
> > >
>

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

[Jonathan Cameron]

On Sat, 23 Jul 2022 14:29:33 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

>  Hi Jonathan,
> 
> On Sat, Jul 16, 2022 at 8:40 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 26 Jun 2022 18:53:11 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >  
> > > Hi Jonathan,
> > >
> > > On Sun, Jun 19, 2022 at 6:38 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Tue, 14 Jun 2022 00:47:06 +0530
> > > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > > >  
> > > > > Add support for single and double tap events based on the tap threshold
> > > > > value and minimum quiet time value between the taps. The INT1 pin is used
> > > > > to interrupt and event is pushed to userspace.
> > > > >
> > > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > > > > ---
> > > > >  drivers/iio/accel/bma400.h      |  11 ++
> > > > >  drivers/iio/accel/bma400_core.c | 210 ++++++++++++++++++++++++++++++--
> > > > >  2 files changed, 211 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/accel/bma400.h b/drivers/iio/accel/bma400.h
> > > > > index e8f802a82300..d05edd9b009a 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_QUIET_MSK        GENMASK(3, 2)  
> > > >
> > > > So following up on the 'what is the maximum time between double taps
> > > > question I took a look at the datasheet.
> > > >
> > > > What about quiet_dt and tics_th?  
> > >
> > > I kept both of them as default values since there is only _period attribute
> > > which matches these values.  
> >
> > Understood.  Definitely feels like we are stretching that ABI too far.
> >  
> > >  
> > > >
> > > > quiet_dt seems to be min time between taps in a double tap, and tics_th
> > > > is about the 'tap' algorithm itself.
> > > >
> > > > Interesting, tics_th is kind of the opposite of what we'd normally use period
> > > > for.  We only get event if the max to min of tap occur closer together than
> > > > this value.
> > > >
> > > > quiet_dt seems a closer fit to what you documented period for.  quiet
> > > > controls min space between single taps.  So related but indirectly to
> > > > double tap detection.
> > > > If I have understood this correctly.
> > > >
> > > > * If two taps are more that quiet apart then we get 2 single taps.
> > > > * If two taps are more than quiet_dt apart but less than quiet apart then
> > > >   we get a double tap.
> > > > * If two taps are less than quiet_dt apart, then we probably detect them
> > > >   as a single tap, as long as the tics_th timing condition is met.
> > > >
> > > > Whilst quiet effects both single and double tap I think we would have
> > > > to define it as controls on both of them (linked by changing value of
> > > > one effecting the other).
> > > >
> > > > Period doesn't fit any of these but I'm struggling to come up with
> > > > good ABI.
> > > >
> > > > Maybe use "holdoff" for single tap to indicate minimum time after
> > > > first tap before we can have another one.  (lifted from naming of
> > > > controls on oscilloscope triggers).
> > > >
> > > > What to call the same time for double taps (which it's a limit on how
> > > > far apart they can be) isn't obvious.  Nor is what would map to quiet_dt
> > > > a they are both very specific to double taps.
> > > > Maybe "period" makes sense for quiet (not quiet_dt) when applied to a double
> > > > tap as it's time within which the event must have completed (got 2 taps)
> > > > but it's a stetch.
> > > >
> > > > Anyhow, this needs more thought.  All suggestions welcome!  
> > >
> > > Thanks for the suggestion,
> > > For double-tap, the quiet value represents window time which means both taps
> > > should occur within this period of time.
> > >
> > > Please give me suggestions for the name "pulse window" or only "window" for
> > > double tap quiet value and "latency" for the quiet_dt.  
> >
> > Window doesn't really describe anything - it's too vague.
> >  
> > >
> > > Most of the accelerometer sensors which support tap feature define
> > > quite, quite_dt
> > > register field with different names then, can I add these attributes in the core
> > > with the final suggested name?  
> >
> > ABI definition is tricky.  Normal trick is to look at other similar sorts of things
> > (like oscilloscope triggers). https://download.tek.com/document/55W_17291_6_0.pdf (1st
> > hit in google)
> > So, one option is to use the concept of triggers resetting.
> >
> > Hence quiet might work as
> > gesture_singletap_resettimeout (in seconds)  Until this time has passed the
> > detector is not reset to look for another event.  
> 
> There is already IIO_EV_INFO_TIMEOUT defined which almost maps to the
> given suggestion. Your thoughts on reusing the same for quiet
> (gesture_singletap_timeout)?

Despite similar name, I'm not sure it's a similar concept.  That timeout is
about reset of a tracking value (typically low pass filtered version of the
data) used for dynamic thresholding.  Same concept of 'something being reset' but
in that case reset just means set to the current _raw value which is rather different
from reenabling the detector.

Probably shouldn't have used such a generic name for the existing _timeout, but
that's been there a very long time IIRC :(

> 
> >
> > It's a bit of a stretch to use this for quiet_dt. There is a similar concept
> > in the above scope manual, but it's defined as AB triggers and that's complex
> > and doesn't map great to simple naming.
> > gesture_doubletap_tap2mindelay (in seconds)  
> 
> Thanks for the suggestions.
> I will send v2 soon so that I can get more comments on this from others also.
> 
> > maybe?  Note these are only vague suggestions.  There are a various userspace
> > folk who are often kind enough to give us feedback on things affecting ABI.
> > So +CC
> > Bastien Nocera <hadess@hadess.net>
> > Hans de Goede <hdegoede@redhat.com>
> >
> > (for Bastien and Hans: the aim here is to finally add ABI to IIO for single and double
> >  tap detection from accelerometers - in particular trying to come up with sensible tunables
> >  that are suitable for the algorithms various devices implement).
> >  
> > >
> > > Since tics_th is specific to bma400 can I define this by creating custom
> > > attributes with the same name using IIO_DEVICE_ATTR_RW()?  
> >
> > No.  Custom ABI is fine for a driver if needed like here, but it needs to be inline with
> > the standard ABI such that if we make it standard later, we can hopefully make
> > use of it.   I'm not sure this will ever be standards, but let's aim for
> > something readable(ish).
> >
> > _maxtomin_time  (in seconds)  might work perhaps?  
> 
> This tics_th (_maxtomin_time) in seconds is more specific to bma400 so I thought
> of providing a custom ABI for this, but the values are in floating
> point. How can
> I decode these floating point values (string) in the custom *_show and *_store?

Actual floating point or something we can map to fixed point?
The IIO format extraction routines can be used for non standard ABI and take 
IIO_VAL_INT_PLUS_MICRO etc which often have enough precision for this sort of thing.
> 
> To avoid floating point operations, is there any helper similar to kstrtoint()?
Nope, just the ones in IIO that I know of. May be others hiding elsewhere in the
kernel, but they will all map to some fixed point representation I think.

> 
> For other ABI like _tap2mindelay and _resettimeout are also floating
> point values
> but the core will convert into micro since they are added in core as
> standard ABI.

Yup, that ABI is used in a few non core IIO places though as not uncommon to
need something similar for custom attributes. Look at iio_str_to_fixpoint()
usage in drivers.  e.g. (picking at random)
https://elixir.bootlin.com/linux/latest/source/drivers/iio/proximity/srf08.c#L300


> 
> >
> > I've not looked at what the input subsystem does for touch screens. Feels like they'll
> > have at least some of the same controls.  Perhaps there is something to be learnt
> > from there?  
> 
> Input subsystem is using MT protocol for reporting multi-touch and events are
> reported to userspace, but I didn't find attributes related to tap
> configurations
> that are being read or written from userspace.

Curious.  I guess maybe for touchscreens it is mostly something device specific.
There is probably some argument that these could be firmware things for IIO as well, but
much like you can adjust things about mouse double click detection from userspace, it
makes sense to provide the controls.  Maybe for touch screens it's all done in userspace
(so there is no built in double tap detection?)

Jonathan

> 
> There is an adxl34x accelerometer driver in the input subsystem which supports
> tap but the configuration values for tap are hardcoded in the driver.
> 
> Thanks
> Jagath
> 
> Jagath
> >
> > Jonathan
> >
> >  
> > >
> > > Thank you,
> > > Jagath
> > >
> > >  
> > > >
> > > >
> > > > A few trivial things inline as well.
> > > >
> > > >
> > > >  
> > > > > +
> > > > >  /*
> > > > >   * 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 c31bdd9b168e..f8945dc0de04 100644
> > > > > --- a/drivers/iio/accel/bma400_core.c
> > > > > +++ b/drivers/iio/accel/bma400_core.c
> > > > > @@ -29,6 +29,7 @@
> > > > >  #include <linux/iio/trigger.h>
> > > > >  #include <linux/iio/trigger_consumer.h>
> > > > >  #include <linux/iio/triggered_buffer.h>
> > > > > +#include <linux/iio/sysfs.h>
> > > > >
> > > > >  #include "bma400.h"
> > > > >
> > > > > @@ -88,6 +89,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 +218,36 @@ 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) |
> > > > > +                                    BIT(IIO_EV_INFO_ENABLE),
> > > > > +     },
> > > > > +};
> > > > > +
> > > > > +/* List of sensitivity values available to configure tap interrupts */
> > > > > +static IIO_CONST_ATTR(in_accel_gesture_value_available, "0 1 2 3 4 5 6 7");
> > > > > +
> > > > > +/* List of minimum quiet time before and after double tap, in data samples. */
> > > > > +static IIO_CONST_ATTR(in_accel_gesture_doubletap_period_available,
> > > > > +                   "60 80 100 120");
> > > > > +
> > > > > +static struct attribute *bma400_event_attributes[] = {
> > > > > +     &iio_const_attr_in_accel_gesture_value_available.dev_attr.attr,
> > > > > +     &iio_const_attr_in_accel_gesture_doubletap_period_available.dev_attr.attr,
> > > > > +     NULL
> > > > > +};
> > > > > +
> > > > > +static const struct attribute_group bma400_event_attribute_group = {
> > > > > +     .attrs = bma400_event_attributes,
> > > > >  };
> > > > >
> > > > >  #define BMA400_ACC_CHANNEL(_index, _axis) { \
> > > > > @@ -1012,6 +1044,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 +1137,80 @@ static int bma400_activity_event_en(struct bma400_data *data,
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > +static int bma400_tap_event_en(struct bma400_data *data,
> > > > > +                            enum iio_event_direction dir, int state)
> > > > > +{
> > > > > +     int ret;
> > > > > +     unsigned int mask, field_value;
> > > > > +
> > > > > +     /*
> > > > > +      * Tap interrupts can be configured only in normal mode.
> > > > > +      * See table in section 4.3 "Power modes - performance modes" of
> > > > > +      * datasheet v1.2.
> > > > > +      */
> > > > > +     if (data->power_mode != POWER_MODE_NORMAL)
> > > > > +             return -EINVAL;
> > > > > +
> > > > > +     /*
> > > > > +      * Tap interrupts are operating with the data rate of 200Hz.
> > > > > +      * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> > > > > +      */
> > > > > +     if (data->sample_freq.hz != 200) {
> > > > > +             dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
> > > > > +                              BMA400_S_TAP_MSK,
> > > > > +                              FIELD_PREP(BMA400_S_TAP_MSK, state));
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     switch (dir) {
> > > > > +     case IIO_EV_DIR_SINGLETAP:
> > > > > +             mask = BMA400_S_TAP_MSK;
> > > > > +             set_mask_bits(&field_value, BMA400_S_TAP_MSK,
> > > > > +                           FIELD_PREP(BMA400_S_TAP_MSK, state));
> > > > > +             break;
> > > > > +     case IIO_EV_DIR_DOUBLETAP:
> > > > > +             mask = BMA400_D_TAP_MSK;
> > > > > +             set_mask_bits(&field_value, BMA400_D_TAP_MSK,
> > > > > +                           FIELD_PREP(BMA400_D_TAP_MSK, state));
> > > > > +             break;
> > > > > +     default:
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
> > > > > +                              field_value);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     set_mask_bits(&data->tap_event_en, mask, field_value);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static int bma400_disable_adv_interrupt(struct bma400_data *data)
> > > > > +{
> > > > > +     int ret;
> > > > > +
> > > > > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG0_REG, 0);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = regmap_write(data->regmap, BMA400_INT_CONFIG1_REG, 0);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     data->tap_event_en = 0;
> > > > > +     data->generic_event_en = 0;
> > > > > +     data->step_event_en = 0;
> > > > > +     data->activity_event_en = 0;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > > >                                    const struct iio_chan_spec *chan,
> > > > >                                    enum iio_event_type type,
> > > > > @@ -1111,10 +1221,20 @@ static int bma400_write_event_config(struct iio_dev *indio_dev,
> > > > >
> > > > >       switch (chan->type) {
> > > > >       case IIO_ACCEL:
> > > > > -             mutex_lock(&data->mutex);
> > > > > -             ret = bma400_activity_event_en(data, dir, state);
> > > > > -             mutex_unlock(&data->mutex);
> > > > > -             return ret;
> > > > > +             switch (type) {
> > > > > +             case IIO_EV_TYPE_MAG:
> > > > > +                     mutex_lock(&data->mutex);
> > > > > +                     ret = bma400_activity_event_en(data, dir, state);
> > > > > +                     mutex_unlock(&data->mutex);
> > > > > +                     return ret;
> > > > > +             case IIO_EV_TYPE_GESTURE:
> > > > > +                     mutex_lock(&data->mutex);
> > > > > +                     ret = bma400_tap_event_en(data, dir, state);
> > > > > +                     mutex_unlock(&data->mutex);
> > > > > +                     return ret;
> > > > > +             default:
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > >       case IIO_STEPS:
> > > > >               mutex_lock(&data->mutex);
> > > > >               ret = bma400_steps_event_enable(data, state);
> > > > > @@ -1159,8 +1279,8 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > > >       struct bma400_data *data = iio_priv(indio_dev);
> > > > >       int ret, reg;
> > > > >
> > > > > -     switch (chan->type) {
> > > > > -     case IIO_ACCEL:
> > > > > +     switch (type) {
> > > > > +     case IIO_EV_TYPE_MAG:
> > > > >               reg = get_gen_config_reg(dir);
> > > > >               if (reg < 0)
> > > > >                       return -EINVAL;
> > > > > @@ -1196,6 +1316,25 @@ static int bma400_read_event_value(struct iio_dev *indio_dev,
> > > > >               default:
> > > > >                       return -EINVAL;
> > > > >               }
> > > > > +     case IIO_EV_TYPE_GESTURE:
> > > > > +             switch (info) {
> > > > > +             case IIO_EV_INFO_VALUE:
> > > > > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG,
> > > > > +                                       val);
> > > > > +                     if (ret)
> > > > > +                             return ret;
> > > > > +                     *val = FIELD_GET(BMA400_TAP_SEN_MSK, *val);  
> > > >
> > > > I'd prefer a local variable for regval into which the regmap_read goes.
> > > >
> > > > Slightly more readable that way as *val then only ever means one thing.
> > > >  
> > > > > +                     return IIO_VAL_INT;
> > > > > +             case IIO_EV_INFO_PERIOD:
> > > > > +                     ret = regmap_read(data->regmap, BMA400_TAP_CONFIG1,
> > > > > +                                       val);
> > > > > +                     if (ret)
> > > > > +                             return ret;
> > > > > +                     *val = FIELD_GET(BMA400_TAP_QUIET_MSK, *val);
> > > > > +                     return IIO_VAL_INT;
> > > > > +             default:
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > >       default:
> > > > >               return -EINVAL;
> > > > >       }
> > > > > @@ -1209,10 +1348,10 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > > >                                   int val, int val2)
> > > > >  {
> > > > >       struct bma400_data *data = iio_priv(indio_dev);
> > > > > -     int reg, ret;
> > > > > +     int reg, ret, quiet_period;
> > > > >
> > > > > -     switch (chan->type) {
> > > > > -     case IIO_ACCEL:
> > > > > +     switch (type) {
> > > > > +     case IIO_EV_TYPE_MAG:
> > > > >               reg = get_gen_config_reg(dir);
> > > > >               if (reg < 0)
> > > > >                       return -EINVAL;
> > > > > @@ -1228,7 +1367,6 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > > >               case IIO_EV_INFO_PERIOD:
> > > > >                       if (val < 1 || val > 65535)
> > > > >                               return -EINVAL;
> > > > > -  
> > > >
> > > > Clear out this noise.
> > > >  
> > > > >                       mutex_lock(&data->mutex);
> > > > >                       put_unaligned_be16(val, &data->duration);
> > > > >                       ret = regmap_bulk_write(data->regmap,
> > > > > @@ -1248,6 +1386,31 @@ static int bma400_write_event_value(struct iio_dev *indio_dev,
> > > > >               default:
> > > > >                       return -EINVAL;
> > > > >               }
> > > > > +     case IIO_EV_TYPE_GESTURE:
> > > > > +             switch (info) {
> > > > > +             case IIO_EV_INFO_VALUE:
> > > > > +                     if (val < 0 || val > 7)
> > > > > +                             return -EINVAL;
> > > > > +
> > > > > +                     return regmap_update_bits(data->regmap,
> > > > > +                                               BMA400_TAP_CONFIG,
> > > > > +                                               BMA400_TAP_SEN_MSK,
> > > > > +                                               FIELD_PREP(BMA400_TAP_SEN_MSK,
> > > > > +                                                          val));
> > > > > +
> > > > > +             case IIO_EV_INFO_PERIOD:
> > > > > +                     quiet_period = (val / 20) - 3;
> > > > > +                     if (quiet_period < 0 || quiet_period > 3)
> > > > > +                             return -EINVAL;
> > > > > +
> > > > > +                     return regmap_update_bits(data->regmap,
> > > > > +                                               BMA400_TAP_CONFIG1,
> > > > > +                                               BMA400_TAP_QUIET_MSK,
> > > > > +                                               FIELD_PREP(BMA400_TAP_QUIET_MSK,
> > > > > +                                                          quiet_period));
> > > > > +             default:
> > > > > +                     return -EINVAL;
> > > > > +             }
> > > > >       default:
> > > > >               return -EINVAL;
> > > > >       }
> > > > > @@ -1287,6 +1450,7 @@ static const struct iio_info bma400_info = {
> > > > >       .write_event_config = bma400_write_event_config,
> > > > >       .write_event_value = bma400_write_event_value,
> > > > >       .read_event_value = bma400_read_event_value,
> > > > > +     .event_attrs = &bma400_event_attribute_group,
> > > > >  };
> > > > >
> > > > >  static const struct iio_trigger_ops bma400_trigger_ops = {
> > > > > @@ -1350,6 +1514,32 @@ static irqreturn_t bma400_interrupt(int irq, void *private)
> > > > >       if (ret || !data->status)
> > > > >               goto unlock_err;
> > > > >
> > > > > +     /*
> > > > > +      * Disable all advance interrupts if interrupt engine overrun occurs.
> > > > > +      * See section 4.7 "Interrupt engine overrun" in datasheet v1.2.
> > > > > +      */
> > > > > +     if (FIELD_GET(BMA400_INT_ENG_OVRUN_MSK, le16_to_cpu(data->status))) {
> > > > > +             bma400_disable_adv_interrupt(data);
> > > > > +             dev_err(data->dev, "Interrupt engine overrun\n");
> > > > > +             goto unlock_err;
> > > > > +     }
> > > > > +
> > > > > +     if (FIELD_GET(BMA400_INT_S_TAP_MSK, le16_to_cpu(data->status)))
> > > > > +             iio_push_event(indio_dev,
> > > > > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > > > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > > > > +                                               IIO_EV_TYPE_GESTURE,
> > > > > +                                               IIO_EV_DIR_SINGLETAP),
> > > > > +                            timestamp);
> > > > > +
> > > > > +     if (FIELD_GET(BMA400_INT_D_TAP_MSK, le16_to_cpu(data->status)))
> > > > > +             iio_push_event(indio_dev,
> > > > > +                            IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > > > +                                               IIO_MOD_X_OR_Y_OR_Z,
> > > > > +                                               IIO_EV_TYPE_GESTURE,
> > > > > +                                               IIO_EV_DIR_DOUBLETAP),
> > > > > +                            timestamp);
> > > > > +
> > > > >       if (FIELD_GET(BMA400_INT_GEN1_MSK, le16_to_cpu(data->status)))
> > > > >               ev_dir = IIO_EV_DIR_RISING;
> > > > >  
> > > >  
> >