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

[Martin Kepplinger <martink@posteo.de>]

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@gmail.com>
>> > ---
>> 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.