Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Sat, 4 Jun 2022 15:43:38 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Jagath Jog J <jagathjog1996@gmail.com>
Cc:     andy.shevchenko@gmail.com, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC 1/2] iio: Add new event type gesture and use direction for
 single and double tap
Message-ID: <20220604154338.26a9ddf7@jic23-huawei>
In-Reply-To: <20220529040153.4878-2-jagathjog1996@gmail.com>
References: <20220529040153.4878-1-jagathjog1996@gmail.com>
        <20220529040153.4878-2-jagathjog1996@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 29 May 2022 09:31:52 +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>

Looks good to me in general. A few comments on the documentation. 

I briefly thought about whether we should make the direction enum dependent
on the type of channel, but then I checked and we have 7 bits for that
fields, so 'hopefully' we will always have plenty of space.

If people start defining 100s of gestures we may need to rethink.

This is bold new userspace ABI, so I definitely want to give this
more time on the list for others to have the opportunity to take a look
and think about whether it works for their devices etc.

Thanks,

Jonathan

> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 19 +++++++++++++++++++
>  drivers/iio/industrialio-event.c        |  5 ++++-
>  include/uapi/linux/iio/types.h          |  3 +++
>  tools/iio/iio_event_monitor.c           |  8 +++++++-
>  4 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 3e00d7f7ee22..2d4866203ccf 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -2035,3 +2035,22 @@ 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:
> +		Accelerometer device detects single or double taps and generate
> +		events when threshold for minimum tap amplitide passes.

I'd keep this a little more vague as different accelerometers have different
magic algorithms to detect this and we don't want to care about how they do
it.

> +		E.g. a single tap event is generated when acceleration value
> +		crosses the minimum tap amplitude value set. Where tap threshold
> +		value is set by using in_accel_gesture_singletap_value.

That's a very simplistic tap detector. Given the control on the threshold
on the bma400 is a whole 3 bits, I'd not what to hazard any guess to
what 'tap sensitivity is'.

So I'd make this description much less specific. (even drop the device type)
Perhaps:

"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.

This is why 'gesture' detection is a pain.   What are the units of this
value?  (typically tap detection is done via some mixture of jerk and
thresholds on the raw acceleration plus temporal constraints).  

Perhaps for now, we just add a sentence that says.

'Units and exact meaning 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: