| 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
| ||
|
Message-ID: <CAFXKEHao9xKsizGLMQxikcLbG5Him9n9i3btLtqK2Orj_39a9Q@mail.gmail.com>
Date: Sat, 21 Jun 2025 20:53:58 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v9 08/11] iio: accel: adxl345: add inactivity feature
On Thu, Jun 12, 2025 at 2:15 PM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> On Tue, Jun 10, 2025 at 09:59:30PM +0000, Lothar Rubusch wrote:
> > Add the inactivity feature of the sensor to the driver. When activity
> > and inactivity are enabled, a link bit will be set linking activity and
> > inactivity handling. Additionally, the auto-sleep mode will be enabled.
> > Due to the link bit the sensor is going to auto-sleep when inactivity
> > was detected.
> >
> > Inactivity detection needs a threshold to be configured and a period of
> > time in seconds. After, it will transition to inactivity state, if
> > measurements stay below inactivity threshold.
> >
> > When a ODR is configured the period for inactivity is adjusted with a
> > corresponding reasonable default value, in order to have higher
> > frequencies, lower inactivity times, and lower sample frequency but
> > give more time until inactivity. Both with reasonable upper and lower
> > boundaries, since many of the sensor's features (e.g. auto-sleep) will
> > need to operate between 12.5 Hz and 400 Hz. This is a default setting
> > when actively changing sample frequency, explicitly setting the time
> > until inactivity will overwrite the default.
> >
> > Similarly, setting the g-range will provide a default value for the
> > activity and inactivity thresholds. Both are implicit defaults, but
> > equally can be overwritten to be explicitly configured.
>
> ...
>
> > +static const struct iio_event_spec adxl345_fake_chan_events[] = {
> > + {
> > + /* inactivity */
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD),
>
> Slightly better
>
> .mask_shared_by_type =
> BIT(IIO_EV_INFO_VALUE) |
> BIT(IIO_EV_INFO_PERIOD),
>
> > + },
> > +};
>
> And the same for other similar cases.
>
> ...
>
> > +/**
> > + * adxl345_set_inact_time - Configure inactivity time explicitly or by ODR.
> > + * @st: The sensor state instance.
> > + * @val_s: A desired time value, between 0 and 255.
> > + *
> > + * Inactivity time can be configured between 1 and 255 sec. If a val_s of 0
> > + * is configured by a user, then a default inactivity time will be computed.
> > + *
> > + * In such case, it should take power consumption into consideration. Thus it
> > + * shall be shorter for higher frequencies and longer for lower frequencies.
> > + * Hence, frequencies above 255 Hz shall default to 10 s and frequencies below
> > + * 10 Hz shall result in 255 s to detect inactivity.
> > + *
> > + * The approach simply subtracts the pre-decimal figure of the configured
> > + * sample frequency from 255 s to compute inactivity time [s]. Sub-Hz are thus
> > + * ignored in this estimation. The recommended ODRs for various features
> > + * (activity/inactivity, sleep modes, free fall, etc.) lie between 12.5 Hz and
> > + * 400 Hz, thus higher or lower frequencies will result in the boundary
> > + * defaults or need to be explicitly specified via val_s.
> > + *
> > + * Return: 0 or error value.
> > + */
> > +static int adxl345_set_inact_time(struct adxl345_state *st, u32 val_s)
> > +{
> > + unsigned int max_boundary = 255;
> > + unsigned int min_boundary = 10;
> > + unsigned int val = min(val_s, max_boundary);
> > + enum adxl345_odr odr;
> > + unsigned int regval;
> > + int ret;
> > +
> > + if (val == 0) {
> > + ret = regmap_read(st->regmap, ADXL345_REG_BW_RATE, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + odr = FIELD_GET(ADXL345_BW_RATE_MSK, regval);
>
> > + val = (adxl345_odr_tbl[odr][0] > max_boundary)
> > + ? min_boundary : max_boundary - adxl345_odr_tbl[odr][0];
>
> clamp() ?
>
Isn't clamp() dealing with signed ints? Also, I'll take the diff from
max_boundary here. So, I'll try staying with the current line and hope
it's fine.
Best,
L
> > + }
> > +
> > + return regmap_write(st->regmap, ADXL345_REG_TIME_INACT, val);
> > +}
>
> ...
>
> > if (type == ADXL345_ACTIVITY) {
> > axis_ctrl = ADXL345_ACT_X_EN | ADXL345_ACT_Y_EN |
> > ADXL345_ACT_Z_EN;
> > } else {
> > - axis_ctrl = 0x00;
> > + axis_ctrl = ADXL345_INACT_X_EN | ADXL345_INACT_Y_EN |
> > + ADXL345_INACT_Z_EN;
> > }
>
> Now this can be as simple as
>
> axis_ctrl = ADXL345_ACT_X_EN;
> if (type == ADXL345_ACTIVITY)
> axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> else
> axis_ctrl |= ADXL345_INACT_Y_EN | ADXL345_INACT_Z_EN;
>
> Yeah, I don't know how to make the diff better (it gets worse), but the end
> result is better.
>
> One way, which I don't like much is to previously have this conditional written as:
>
> axis_ctrl = ADXL345_ACT_X_EN;
> if (type == ADXL345_ACTIVITY)
> axis_ctrl |= ADXL345_ACT_Y_EN | ADXL345_ACT_Z_EN;
> else
> axis_ctrl = 0;
>
> ...
>
> > + ret = regmap_assign_bits(st->regmap, ADXL345_REG_POWER_CTL,
> > + (ADXL345_POWER_CTL_AUTO_SLEEP | ADXL345_POWER_CTL_LINK),
>
> Unneeded parentheses.
>
> > + en);
> > if (ret)
> > return ret;
>
> ...
>
> > static int adxl345_set_odr(struct adxl345_state *st, enum adxl345_odr odr)
> > {
> > - return regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > + int ret;
> > +
> > + ret = regmap_update_bits(st->regmap, ADXL345_REG_BW_RATE,
> > ADXL345_BW_RATE_MSK,
> > FIELD_PREP(ADXL345_BW_RATE_MSK, odr));
> > + if (ret)
> > + return ret;
> > +
> > + /* update inactivity time by ODR */
> > + return adxl345_set_inact_time(st, 0);
>
> Okay, in this case the initial form of
>
> int ret;
>
> ret = ...
> if (ret)
> return ret;
>
> return 0;
>
>
> will be better with the respectful comment (as Jonathan suggested) in that
> change that this is not optimal as standalone change, but it will help reduce
> churn in the next change(s).
>
> > }
>
> ...
>
> > static int adxl345_set_range(struct adxl345_state *st, enum adxl345_range range)
> > {
> > - return regmap_update_bits(st->regmap, ADXL345_REG_DATA_FORMAT,
>
> Same here.
>
> > + unsigned int act_threshold, inact_threshold;
> > + unsigned int range_old;
> > + unsigned int regval;
> > + int ret;
> > +
> > + ret = regmap_read(st->regmap, ADXL345_REG_DATA_FORMAT, ®val);
> > + 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(U8_MAX, max(1, act_threshold));
> > +
> > + inact_threshold = inact_threshold * adxl345_range_factor_tbl[range_old]
> > + / adxl345_range_factor_tbl[range];
> > + inact_threshold = min(U8_MAX, max(1, inact_threshold));
> > +
> > + ret = regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > + act_threshold);
> > + if (ret)
> > + return ret;
> > +
> > + return regmap_write(st->regmap, adxl345_act_thresh_reg[ADXL345_INACTIVITY],
> > + inact_threshold);
> > }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Powered by blists - more mailing lists