Re: [PATCH v5 05/11] iio: accel: adxl345: add freefall feature

[Lothar Rubusch]

On Mon, Mar 31, 2025 at 12:28 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Tue, 18 Mar 2025 23:08:37 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> 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@xxxxxxxxx>
> 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