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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8b4fb5d90af4b5e9d08c9a86993fc72@posteo.de>
Date:   Tue, 14 Nov 2017 08:21:58 +0100
From:   Martin Kepplinger <martink@...teo.de>
To:     harinath Nampally <harinath922@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, knaack.h@....de,
        lars@...afoo.de, Peter Meerwald-Stadler <pmeerw@...erw.net>,
        Greg KH <gregkh@...uxfoundation.org>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        Alison Schofield <amsfield22@...il.com>
Subject: Re: [PATCH] iio: accel: mma8452: Add single pulse/tap event detection

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@...il.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.