Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220825194604.15645-1-jagathjog1996@gmail.com>
 <20220825194604.15645-3-jagathjog1996@gmail.com> <CAHp75Vc5048aQL5cLy-OfBfnH6tz_7z24sFX2H1oGRz+JUyq9Q@mail.gmail.com>
In-Reply-To: <CAHp75Vc5048aQL5cLy-OfBfnH6tz_7z24sFX2H1oGRz+JUyq9Q@mail.gmail.com>
From:   Jagath Jog J <jagathjog1996@gmail.com>
Date:   Sat, 27 Aug 2022 03:36:23 +0530
Message-ID: <CAM+2Eu+a8LM+XBELAm9H51EWVwCDqcYaxfdj4_-Sk9TtJ0gLJA@mail.gmail.com>
Subject: Re: [PATCH v3 2/2] iio: accel: bma400: Add support for single and
 double tap events
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Bastien Nocera <hadess@hadess.net>,
        Hans de Goede <hdegoede@redhat.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 Andy,

On Fri, Aug 26, 2022 at 1:53 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Aug 25, 2022 at 10:46 PM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > Add support for single and double tap events based on the tap threshold
> > value, minimum quiet time before and after the tap and minimum time
> > between the taps in the double tap. The INT1 pin is used to interrupt
> > and the event is pushed to userspace.
>
> ...
>
> > +static int tap_reset_timeout[] = {
> > +       300000,
> > +       400000,
> > +       500000,
> > +       600000
>
> + Comma and so on for the rest of the similar cases.

This is the terminator case so I have not added a comma in the last.
All three tap configurations have only 4 value options.

>
> > +};
>
> ...
>
> > +static int usec_to_tapreg_raw(int usec, const int *time_list)
> > +{
> > +       int index;
> > +
> > +       for (index = 0; index < 4; index++) {
>
> Magic. Shouldn't be defined?

All tap configuration value arrays are of size 4, I will define a
macro for that.

>
> Also you may add it to each data structure in question.

Do you mean storing these values in the device's private structure?

Tap configuration values are not stored in the device's private
structure because.
- I am directly accessing the device registers in _read_event_value()
and _write_event_value().
- These configuration values are not used in the other parts of
the driver.
- Two of these configurations have a default value so instead of
reading and storing these values in the device's private structure
during device init, I am directly accessing the device's register.

>
> > +               if (usec == time_list[index])
> > +                       return index;
> > +       }
> > +       return -EINVAL;
> > +}
>
> ...
>
> > +       int ret;
> > +       unsigned int mask, field_value;
>
> Reversed xmas tree order?
>
> > +        * Tap interrupts are operating with the data rate of 200Hz.
>
> a data

Sure, I will correct these in the next patch series.

Thank you
Jagath

>
> > +        * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> > +        */
>
> --
> With Best Regards,
> Andy Shevchenko