| 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
| ||
|
Message-ID: <20250331133633.370bc50b@jic23-huawei>
Date: Mon, 31 Mar 2025 13:36:33 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
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 Wed, 19 Mar 2025 00:08:24 +0100
Lothar Rubusch <l.rubusch@...il.com> wrote:
> 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.
> >
>
> Sorry for replying again on this topic. Pls, find my solution in v5.
>
> After some thinking, I implemented it now using returns directly leaving the
> measurement on/off as is. I'm unsure if it actually makes sense, after an error
> here to turn measurement on again? I can imagine a situation where a wrong
> input might result in an error. Nothing is changed, and measurement
> could/should continue. Now, it will probably stop, in case of wrong
> input. But is
> wrong input actually an issue here?
If a userspace control input is out of range etc, then returning an error
but leaving things on makes sense. If what we see is a comms error
it gets less clear on what we should do.
>
> As other alternative, I can think of is to shift measurement on/off
> into the called
> functions directly. I think, this approach was used also in the
> ADXL380 and seems
> to be common. Let me know what you think.
>
Moving it up or down a layer can work by allowing direct returns and always
trying to reenable if that makes sense.
Sadly in error handling there is often not a right answer on what to do!
Jonathan
> > > +
> > > + if (ret)
> > > + return ret; /* measurement stays off */
> > > +
> > > + return adxl345_set_measure_en(st, true);
> > > +}
> >
Powered by blists - more mailing lists