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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Apr 2025 16:30:35 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

The actual bit(s) would be my preference.  Whilst I agree it would in theory
be fine to use 0xff that would require me going to check the register
width.  Using ~0 might always work but that is just weird to look at.

Jonathan


> 
> Best,
> L