Re: [PATCH] iio: accel: mma8452: Add single pulse/tap event detection

[Martin Kepplinger]

Am 14.11.2017 05:36 schrieb harinath Nampally:
> > > This patch adds following related changes:
> > > - defines pulse event related registers
> > > - enables and handles single pulse interrupt for fxls8471
> > > - handles IIO_EV_DIR_EITHER in read/write callbacks (because
> > >   event direction for pulse is either rising or falling)
> > > - configures read/write event value for pulse latency register
> > >   using IIO_EV_INFO_HYSTERESIS
> > > - adds multiple events like pulse and tranient event spec
> > >   as elements of event_spec array named 'mma8452_accel_events'
> > >
> > > Except mma8653 chip all other chips like mma845x and
> > > fxls8471 have single tap detection feature.
> > > Tested thoroughly using iio_event_monitor application on
> > > imx6ul-evk board which has fxls8471.
> > >
> > > Signed-off-by: Harinath Nampally <harinath922@xxxxxxxxx>
> > > ---
> > What tree is this written against? It doesn't apply to the current 
> > -next
> > anyways.
> Thanks for the review.
> It is actually against 'testing' branch, I think two of my earlier
> patches are not yet applied to
> any branch, that might be reason this patch is not good against
> current -next or 'togreg'.
>
> > I think the defintions would deserve to be in a separate patch, but
> > that's debatable.
> Yes, I would argue that definitions are not a logical change.

I would argue definitions don't break the build and maybe slightly 
better
support features like bisect or revert :)

> > >               .type = IIO_EV_TYPE_MAG,
> > >               .dir = IIO_EV_DIR_RISING,
> > >               .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > @@ -1139,6 +1274,15 @@ static const struct iio_event_spec mma8452_transient_event[] = {
> > >                                       BIT(IIO_EV_INFO_PERIOD) |
> > >                                       BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB)
> > >       },
> > > +     {
> > > +             //pulse event
> > > +             .type = IIO_EV_TYPE_MAG,
> > > +             .dir = IIO_EV_DIR_EITHER,
> > > +             .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > > +             .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > > +                                     BIT(IIO_EV_INFO_PERIOD) |
> > > +                                     BIT(IIO_EV_INFO_HYSTERESIS)
> > > +     },
> > >  };
> > >
> > >  static const struct iio_event_spec mma8452_motion_event[] = {
> > > @@ -1202,8 +1346,8 @@ static struct attribute_group mma8452_event_attribute_group = {
> > >               .shift = 16 - (bits), \
> > >               .endianness = IIO_BE, \
> > >       }, \
> > > -     .event_spec = mma8452_transient_event, \
> > > -     .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
> > > +     .event_spec = mma8452_accel_events, \
> > > +     .num_event_specs = ARRAY_SIZE(mma8452_accel_events), \
> > that would go in the mentioned separate renaming-patch
> OK so I will make a patch set; patch 1/2 to just rename
> 'mma8452_transient_event[]'
> to 'mma8452_accel_events[]'(without adding pulse event).
> and everything else would go in 2/2. Does that makes sense?

It does to me.