linux-kernel - Re: [PATCH v3 09/15] iio: accel: adxl345: add freefall feature

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: <CAFXKEHb3GehCyNyLh3Z_8yVPNda9V_8KN_RF+hZvHdW8u9zr0Q@mail.gmail.com>
Date: Thu, 13 Mar 2025 17:34:32 +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 v3 09/15] iio: accel: adxl345: add freefall feature

On Tue, Mar 4, 2025 at 2:23 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:28 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Add the freefall detection of the sensor together with a threshold and
> > time parameter. A freefall event is detected if the measuring signal
> > falls below the threshold.
> >
> > Introduce a freefall threshold stored in regmap cache, and a freefall
> > time, having the scaled time value stored as a member variable in the
> > state instance.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> Hi, one thing inline.
>
> > @@ -855,6 +958,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> >                       return ret;
> >       }
> >
> > +     if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> > +             ret = iio_push_event(indio_dev,
> > +                                  IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                                     IIO_MOD_X_OR_Y_OR_Z,
> > +                                                     IIO_EV_TYPE_MAG,
> > +                                                     IIO_EV_DIR_FALLING),
> > +                                  ts);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
>
> Seems unlikely to be right. Pushed an event without error yet this function
> is returning an error here?
>
> >       return -ENOENT;
> >  }
> >

"it worked on my machine" - Of course, you're right. So, I tried to
understand why this "worked". In consequence, I think the best
solution will be to put also fifo handling based on int_stat into this
function, which I currently made a separate patch, you'll see that in
v4.

> > @@ -954,6 +1068,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >                                        ADXL345_DATA_FORMAT_FULL_RES |
> >                                        ADXL345_DATA_FORMAT_SELF_TEST);
> >       unsigned int tap_threshold;
> > +     unsigned int ff_threshold;
> >       int ret;
> >
> >       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > @@ -973,6 +1088,9 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >       st->tap_window_us = 64;                 /*   64 [0x40] -> .080    */
> >       st->tap_latent_us = 16;                 /*   16 [0x10] -> .020    */
> >
> > +     ff_threshold = 8;                       /*    8 [0x08]            */
> > +     st->ff_time_ms = 32;                    /*   32 [0x20] -> 0.16    */
> > +
> >       indio_dev->name = st->info->name;
> >       indio_dev->info = &adxl345_info;
> >       indio_dev->modes = INDIO_DIRECT_MODE;
> > @@ -1049,6 +1167,10 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >               if (ret)
> >                       return ret;
> >
> > +             ret = regmap_write(st->regmap, ADXL345_REG_THRESH_FF, ff_threshold);
> > +             if (ret)
> > +                     return ret;
> > +
> >               /* FIFO_STREAM mode is going to be activated later */
> >               ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops);
> >               if (ret)
>