Message-ID: <CAFXKEHbxbjzNunauPFCFUwhn3k5F1Ki2sgVFu2y2HxxPot2MfA@mail.gmail.com>
Date: Tue, 18 Mar 2025 11:32:54 +0100
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v4 05/14] iio: accel: adxl345: add single tap feature

On Sun, Mar 16, 2025 at 12:22 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 13 Mar 2025 16:50:40 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Add the single tap feature with a threshold in 62.5mg/LSB points and a
> > scaled duration in us. Keep singletap threshold in regmap cache but
> > the scaled value of duration in us as member variable.
> >
> > Both use IIO channels for individual enable of the x/y/z axis. Initializes
> > threshold and duration with reasonable content. When an interrupt is
> > caught it will be pushed to the according IIO channel.
> >
>
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
>
> Hi Lothar,
>
> A few things in here are from the discussion that was continuing
> on v3 so I may have said more replying to that.
>
> Anyhow, for now I'll hold off on applying from this point on as
> a few more things to respond to inline.
>
> Jonathan
>
> >
> >  #include "adxl345.h"
> > @@ -31,6 +33,33 @@
> >  #define ADXL345_INT1                 0
> >  #define ADXL345_INT2                 1
> >
> > +#define ADXL345_REG_TAP_AXIS_MSK     GENMASK(2, 0)
> > +
> > +enum adxl345_axis {
> > +     ADXL345_Z_EN = BIT(0),
> > +     ADXL345_Y_EN = BIT(1),
> > +     ADXL345_X_EN = BIT(2),
> > +     /* Suppress double tap detection if value > tap threshold */
> > +     ADXL345_TAP_SUPPRESS = BIT(3),
> > +};
> As per feedback (after you sent this!) on v3, I'd drop
> the last value out of the enum, or just use defines and a u8 for
> the one place this is used for local variable storage.
>
>
> > @@ -198,6 +387,132 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >       return -EINVAL;
> >  }
> >
> > +static int adxl345_read_event_config(struct iio_dev *indio_dev,
> > +                                  const struct iio_chan_spec *chan,
> > +                                  enum iio_event_type type,
> > +                                  enum iio_event_direction dir)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     bool int_en;
> > +     int ret = -EFAULT;
> Not used?
>
> > +
> > +     switch (type) {
> > +     case IIO_EV_TYPE_GESTURE:
> > +             switch (dir) {
> > +             case IIO_EV_DIR_SINGLETAP:
> > +                     ret = adxl345_is_tap_en(st, chan->channel2,
> > +                                             ADXL345_SINGLE_TAP, &int_en);
> > +                     if (ret)
> > +                             return ret;
> > +                     return int_en;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
>
> > +static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > +                                  const struct iio_chan_spec *chan,
> > +                                  enum iio_event_type type,
> > +                                  enum iio_event_direction dir,
> > +                                  enum iio_event_info info,
> > +                                  int val, int val2)
> > +{
> > +     struct adxl345_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +
> > +     ret = adxl345_set_measure_en(st, false);
> > +     if (ret)
> > +             return ret;
> > +
> So in my brief reply to the v3 discussion I suggested perhaps
> factoring out everything from here...
> > +     switch (type) {
> > +     case IIO_EV_TYPE_GESTURE:
> > +             switch (info) {
> > +             case IIO_EV_INFO_VALUE:
> > +                     ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP,
> > +                                        min(val, 0xFF));
> > +                     break;
> > +             case IIO_EV_INFO_TIMEOUT:
> > +                     ret = adxl345_set_tap_duration(st, val, val2);
> > +                     break;
> > +             default:
> > +                     ret = -EINVAL;
> > +                     break;
> > +             }
> > +             break;
> > +     default:
> > +             ret = -EINVAL;
> > +             break;
> > +     }
> to here, so as to allow simple direct returns.
>
> I think that will make the code more readable given the need to reenable
> measurements and that you want to leave it off on error.
>
> > +
> > +     if (ret)
> > +             return ret; /* measurement stays off */
> > +
> > +     return adxl345_set_measure_en(st, true);
> > +}
>

Hi Jonathan, my reason to leave this part unchanged was the following:
As you can see above, 'return adxl345_set_measure_en(st, true);' - I'm
actually need to turn off measurement before changing values, then to
turn it on again after. In case of error, doing a simple "return ret"
would keep measurement generally off. I think I should generally try
to turn it on again after.

Alternatively I could do something with goto/label to turn measurement
on, but then return ret. My choice here was to use this ret approach
together with break, at cost of readability. Any advice?

Powered by blists - more mailing lists