linux-kernel - Re: [PATCH v3 11/15] iio: accel: adxl345: add g-range configuration

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] [day] [month] [year] [list]

Message-ID: <CAFXKEHYNAL2vRgBo6H8JRHemvGj2vT30y01T_0-jhY-hkumyMA@mail.gmail.com>
Date: Thu, 13 Mar 2025 17:47:08 +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 11/15] iio: accel: adxl345: add g-range configuration

On Tue, Mar 4, 2025 at 2:40 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:30 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Introduce means to configure and work with the available g-ranges
> > keeping the precision of 13 digits.
> >
> > This is in preparation for the activity/inactivity feature.
>
> I'm not really following why adding range control is anything
> much to do with that. Mostly we do this to improve accuracy for
> low accelerations.
>

As you probably saw the connection comes a bit over the link in
adjusting the activity/inactivity
parameters (times and threshold) by a given range in the follow up patches.

If the question is rather why at all adding this g-range control. My
idea was that adjusting i.e. lowering precision, less odr, etc might
also help adjusting power consumption. In other words
from a user perspective I assume there is more configuration
possibility. I did not pretend to tune
the implementation for lowest possible power consumption, though. It
was just an idea.

[Also, I was curious about implementing it here. My patch here is
rather meant as a proposal,
if you strongly oppose the idea, pls let me know.]

> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
>
>
> > @@ -483,12 +518,48 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >       return 0;
> >  }
> >
> > +static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> > +                           enum adxl345_range *range)
> > +{
> > +     int i;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(adxl345_fullres_range_tbl); i++)
> > +             if (val == adxl345_fullres_range_tbl[i][0] &&
> > +                 val2 == adxl345_fullres_range_tbl[i][1])
> > +                     break;
> Similar to case in earlier patch, maybe set *range and return in here
> so that any finish of the loop is an error.
> > +
> > +     if (i == ARRAY_SIZE(adxl345_fullres_range_tbl))
> > +             return -EINVAL;
> > +
> > +     *range = i;
> > +
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
> > +                              ADXL345_DATA_FORMAT_RANGE,
> > +                              FIELD_PREP(ADXL345_DATA_FORMAT_RANGE, range));
> > +     if (ret)
> > +             return ret;
> > +
>
> return regmap_update_bits() unless this gets more complex in later patch.
>
> > +     return 0;
> > +}
> > +
>
> > @@ -558,6 +634,7 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >                            int val, int val2, long mask)
> >  {
> >       struct adxl345_state *st = iio_priv(indio_dev);
> > +     enum adxl345_range range;
> >       enum adxl345_odr odr;
> >       int ret;
> >
> > @@ -581,6 +658,12 @@ static int adxl345_write_raw(struct iio_dev *indio_dev,
> >                       return ret;
> >               ret = adxl345_set_odr(st, odr);
> >               break;
> > +     case IIO_CHAN_INFO_SCALE:
> > +             ret = adxl345_find_range(st, val, val2, &range);
> > +             if (ret)
> > +                     return ret;
> > +             ret = adxl345_set_range(st, range);
> as in previous I'd have the
>                 if (ret)
>                         return ret;
> here for consistency with the other one immediately above this.
> > +             break;
> >       default:
> >               return -EINVAL;
> >       }
>