linux-kernel - Re: [PATCH v3 14/15] iio: accel: adxl345: add coupling detection for activity/inactivity

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: <CAFXKEHZDo=AYFswbM48T7syNzk03rPa0PbLceh3C_HPUkKKxQw@mail.gmail.com>
Date: Thu, 13 Mar 2025 17:39:17 +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 14/15] iio: accel: adxl345: add coupling detection for activity/inactivity

On Tue, Mar 4, 2025 at 2:59 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu, 20 Feb 2025 10:42:33 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Add coupling activity/inactivity detection by the AC/DC bit. This is an
> > addititional enhancement for the detection of activity states and
> > completes the activity / inactivity feature of the ADXL345.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> > @@ -337,6 +356,69 @@ static int adxl345_write_act_axis(struct adxl345_state *st,
> >       return 0;
> >  }
> >
> > +static int adxl345_is_act_inact_ac(struct adxl345_state *st,
> > +                                enum adxl345_activity_type type, bool *ac)
> > +{
> > +     unsigned int regval;
> > +     int ret;
> > +
> > +     ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &regval);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (type == ADXL345_ACTIVITY)
> > +             *ac = (FIELD_GET(ADXL345_REG_ACT_ACDC_MSK, regval) > 0) ? true : false;
> > +     else
> > +             *ac = (FIELD_GET(ADXL345_REG_INACT_ACDC_MSK, regval) > 0) ? true : false;
> > +
> Why the ternaries?
>                 *ac = FIELD_GET() > 0;
> is a boolean right hand side anyway.
>
> > +     return 0;
> > +}
> > +
> > +static int adxl345_set_act_inact_ac(struct adxl345_state *st,
> > +                                 enum adxl345_activity_type type, bool ac)
> > +{
> > +     unsigned int act_inact_ac = ac ? 0xff : 0x00;
> > +
> > +     /*
> > +      * A setting of 0 selects dc-coupled operation, and a setting of 1
> false and true rather than 0 / 1?

I don't see where "false" and "true" here has an advantage over 0 and
1, since the text speaks
about setting a bit to 1 or 0. Anyway I changed this in v4.

BTW. in v4 I re-added the comments about the channels for /* single
tap */ and /* double tap */ although this is possible to see from the
iio channel. Since I add comments for freefall, for activity, etc. it
looked cleaner to me, if definitely not ok, let me know please.

> > +      * enables ac-coupled operation. In dc-coupled operation, the current
> > +      * acceleration magnitude is compared directly with
> > +      * ADXL345_REG_THRESH_ACT and ADXL345_REG_THRESH_INACT to determine
> > +      * whether activity or inactivity is detected.
> ...
> >  static int adxl345_is_act_inact_en(struct adxl345_state *st,
> >                                  enum adxl345_activity_type type, bool *en)
> >  {
> > @@ -695,6 +777,11 @@ static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> >       if (ret)
> >               return ret;
> >
> > +     /* update inactivity time by ODR */
> > +     ret = adxl345_set_inact_time_s(st, 0);
> > +     if (ret)
> > +             return ret;
> > +
> >       return 0;
>
> return adxl345_set_inact_time_s()
>
> >  }
> >
> > @@ -718,14 +805,54 @@ static int adxl345_find_range(struct adxl345_state *st, int val, int val2,
> >
> >  static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> >  {
> > +     unsigned int act_threshold, inact_threshold;
> > +     unsigned int range_old;
> > +     unsigned int regval;
> >       int ret;
> >
> > +     ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, &regval);
> > +     if (ret)
> > +             return ret;
> > +     range_old = FIELD_GET(ADXL345_DATA_FORMAT_RANGE, regval);
> > +
> > +     ret = regmap_read(st->regmap,
> > +                       adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > +                       &act_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_read(st->regmap,
> > +                       adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > +                       &inact_threshold);
> > +     if (ret)
> > +             return 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;
> >
> > +     act_threshold = act_threshold
> > +             * adxl345_range_factor_tbl[range_old]
> > +             / adxl345_range_factor_tbl[range];
> > +     act_threshold = min(255, max(1, inact_threshold));
> > +
> > +     inact_threshold = inact_threshold
> > +             * adxl345_range_factor_tbl[range_old]
> > +             / adxl345_range_factor_tbl[range];
> > +     inact_threshold = min(255, max(1, inact_threshold));
> > +
> > +     ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > +                        act_threshold);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > +                        inact_threshold);
> > +     if (ret)
> > +             return ret;
> return regmap_write()
> > +
> >       return 0;
> >  }
>