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;
MIME-Version: 1.0
References: <20220505133021.22362-1-jagathjog1996@gmail.com>
 <20220505133021.22362-10-jagathjog1996@gmail.com> <20220507172416.0c74d4cb@jic23-huawei>
In-Reply-To: <20220507172416.0c74d4cb@jic23-huawei>
From:   Jagath Jog J <jagathjog1996@gmail.com>
Date:   Mon, 9 May 2022 10:41:44 +0530
Message-ID: <CAM+2EuLYRGmhgWHiwZOwFSR9ssSOVwgm5gwVBHD=p57OF5e86A@mail.gmail.com>
Subject: Re: [PATCH v5 09/10] iio: Add channel for tap and new modifiers for
 single and double tap
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Dan Robertson <dan@dlrobertson.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Jonathan,

Thanks for accepting the patches.

On Sat, May 7, 2022 at 9:45 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu,  5 May 2022 19:00:20 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> Strangely the last time I can remember discussion around how to
> support tap detection was way back in the early days of IIO, perhaps
> 10 years ago. I don't recall us ever coming to a conclusion on how to do it.
>
> > Add new channel type for tap and also add new modifiers for single and
> > double tap. This channel and modifiers 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 rising and
> > falling direction.
>
> Not sure how that would work seeing as there is only one modifier
> field and it's not a bitmap.
> The event code would need to encode both what type of tap and
> the direction and there aren't two fields in which to do that.
>
> One way I can see this 'might' work would be to use
> the event type to encode tap and the direction could be 'abused'
> to encode single vs double (or other events like this)
>
> in_accel_x_tap_single
> in_accel_x_tap_double
>
> We could possibly be more generic and have the 'type' as
> 'event' or something like that allowing us to use the
> 7 bit direction field to encode different detectable events
> (I'm not that keen on the name event though, could maybe
> map it to gesture which would cover some of the other
> motion pattern detection devices out there)
>
> That would give us
>
> in_accel_x_event_singletap
> in_accel_y_event_doubletap
>
> etc.
>
> How ever we move forwards we do it this want to be in a new series with a nice
> bold title to attract that attention of people who don't really
> care about he bma400 but do care about tap detection; it's
> a common feature of accelerometers.

Sure, I will try to make a new series for tap events with the given
inputs and send the RFC first to get all the comments from everyone.

>
> Jonathan
>
>
>
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
>
>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-iio | 11 +++++++++++
> >  drivers/iio/industrialio-core.c         |  3 +++
> >  include/uapi/linux/iio/types.h          |  3 +++
> >  tools/iio/iio_event_monitor.c           |  6 ++++++
> >  4 files changed, 23 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index d4ccc68fdcf0..bf2d10d6ad9b 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -2030,3 +2030,14 @@ Description:
> >               Available range for the forced calibration value, expressed as:
> >
> >               - a range specified as "[min step max]"
> > +
> > +What:                /sys/.../events/in_tap_single_change_en
> > +What:                /sys/.../events/in_tap_double_change_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.
> > +             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_tap_change_value.
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> > index e1ed44dec2ab..9b0d7bbd07fc 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -87,6 +87,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >       [IIO_POSITIONRELATIVE]  = "positionrelative",
> >       [IIO_PHASE] = "phase",
> >       [IIO_MASSCONCENTRATION] = "massconcentration",
> > +     [IIO_TAP] = "tap"
> >  };
> >
> >  static const char * const iio_modifier_names[] = {
> > @@ -134,6 +135,8 @@ static const char * const iio_modifier_names[] = {
> >       [IIO_MOD_ETHANOL] = "ethanol",
> >       [IIO_MOD_H2] = "h2",
> >       [IIO_MOD_O2] = "o2",
> > +     [IIO_MOD_TAP_SINGLE] = "single",
> > +     [IIO_MOD_TAP_DOUBLE] = "double",
> >  };
> >
> >  /* relies on pairs of these shared then separate */
> > diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> > index 472cead10d8d..d1e61c84e0d5 100644
> > --- a/include/uapi/linux/iio/types.h
> > +++ b/include/uapi/linux/iio/types.h
> > @@ -47,6 +47,7 @@ enum iio_chan_type {
> >       IIO_POSITIONRELATIVE,
> >       IIO_PHASE,
> >       IIO_MASSCONCENTRATION,
> > +     IIO_TAP,
> >  };
> >
> >  enum iio_modifier {
> > @@ -95,6 +96,8 @@ enum iio_modifier {
> >       IIO_MOD_ETHANOL,
> >       IIO_MOD_H2,
> >       IIO_MOD_O2,
> > +     IIO_MOD_TAP_SINGLE,
> > +     IIO_MOD_TAP_DOUBLE,
> >  };
> >
> >  enum iio_event_type {
> > diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> > index 2f4581658859..7fa7d4285f40 100644
> > --- a/tools/iio/iio_event_monitor.c
> > +++ b/tools/iio/iio_event_monitor.c
> > @@ -59,6 +59,7 @@ static const char * const iio_chan_type_name_spec[] = {
> >       [IIO_POSITIONRELATIVE] = "positionrelative",
> >       [IIO_PHASE] = "phase",
> >       [IIO_MASSCONCENTRATION] = "massconcentration",
> > +     [IIO_TAP] = "tap",
> >  };
> >
> >  static const char * const iio_ev_type_text[] = {
> > @@ -122,6 +123,8 @@ static const char * const iio_modifier_names[] = {
> >       [IIO_MOD_PM4] = "pm4",
> >       [IIO_MOD_PM10] = "pm10",
> >       [IIO_MOD_O2] = "o2",
> > +     [IIO_MOD_TAP_SINGLE] = "single",
> > +     [IIO_MOD_TAP_DOUBLE] = "double",
> >  };
> >
> >  static bool event_is_known(struct iio_event_data *event)
> > @@ -164,6 +167,7 @@ static bool event_is_known(struct iio_event_data *event)
> >       case IIO_POSITIONRELATIVE:
> >       case IIO_PHASE:
> >       case IIO_MASSCONCENTRATION:
> > +     case IIO_TAP:
> >               break;
> >       default:
> >               return false;
> > @@ -215,6 +219,8 @@ static bool event_is_known(struct iio_event_data *event)
> >       case IIO_MOD_PM4:
> >       case IIO_MOD_PM10:
> >       case IIO_MOD_O2:
> > +     case IIO_MOD_TAP_SINGLE:
> > +     case IIO_MOD_TAP_DOUBLE:
> >               break;
> >       default:
> >               return false;
>