linux-kernel - Re: [PATCH v5 05/11] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAFXKEHZ6pozMTw_8hT9i7rp_OtPZMaFXEisW925oYgFFJeXv=Q@mail.gmail.com>
Date: Mon, 14 Apr 2025 16:30:35 +0200
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 v5 05/11] iio: accel: adxl345: add freefall feature

On Mon, Mar 31, 2025 at 12:28 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 18 Mar 2025 23:08:37 +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 Lothar,
>
> Apologies for the slow review!  Just catching up after travel
> and I did it reverse order.
>
> > +
> > +static int adxl345_set_ff_en(struct adxl345_state *st, bool cmd_en)
> > +{
> > +     unsigned int regval, ff_threshold;
> > +     const unsigned int freefall_mask = 0x02;
>
> Where did this mask come from?   Feels like it should be a define
> (just use ADXL345_INT_FREE_FALL probably)
> or if not that at lest use BIT(1) to make it clear it's a bit rather
> than the number 2.
>
> > +     bool en;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_THRESH_FF, &ff_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     en = cmd_en && ff_threshold > 0 && st->ff_time_ms > 0;
> > +
> > +     regval = en ? ADXL345_INT_FREE_FALL : 0x00;
> > +
> > +     return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > +                               freefall_mask, regval);
> > +}
>

This raises for me a bit of a general question. I face this situation
quite often when using FIELD_GET() or, like here,
regmap_update_bits(). In general, when I need to apply a mask on a
value to be set or unset.

A fixed version of the above could be this:
 421         regval = en ? ADXL345_INT_FREE_FALL : 0x00;
 422
 423         return regmap_update_bits(st->regmap,
ADXL345_REG_INT_ENABLE,
 424                                   ADXL345_INT_FREE_FALL, regval);

Actually, (suppose we have uint8_t, and mask only masks a single bit),
I tend more and more to prefer 0xff over the particular bit, so...
421         regval = en ? 0xff : 0x00;

...and then apply the mask on it. Is there any opinion on using 0xff
or rather using the exact bit? Or, do you, Jonathan, care more about
one or the other?

Best,
L