| 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: <CAFXKEHY59F54Wq7Z7MXc4cnDBq3E2gCNM6Js7CS7K+CarbwSiQ@mail.gmail.com>
Date: Mon, 14 Apr 2025 13:58:05 +0200
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 v5 08/11] iio: accel: adxl345: add activity event feature
On Mon, Mar 31, 2025 at 12:40 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Tue, 18 Mar 2025 23:08:40 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Make the sensor detect and issue interrupts at activity. Activity
> > events are configured by a threshold stored in regmap cache. Initialize
> > the activity threshold register to a reasonable default value in probe.
> > The value is taken from the older ADXL345 input driver, to provide a
> > similar behavior. Reset the activity/inactivity direction enabling
> > register in probe. Reset and initialization shall bring the sensor in a
> > defined initial state to prevent dangling settings when warm restarting
> > the sensor.
> >
> > Activity, ODR configuration together with the range setting prepare the
> > activity/inactivity hystersesis setup, implemented in a follow up patch.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
>
> > +static int adxl345_is_act_inact_en(struct adxl345_state *st,
> > + enum iio_modifier axis,
> > + enum adxl345_activity_type type, bool *en)
> > +{
> > + unsigned int regval;
> > + bool axis_en;
> > + u32 axis_ctrl;
> > + int ret;
> > +
> > + ret = regmap_read(st->regmap, ADXL345_REG_ACT_INACT_CTRL, &axis_ctrl);
> > + if (ret)
> > + return ret;
> > +
> > + if (type == ADXL345_ACTIVITY) {
> > + switch (axis) {
> > + case IIO_MOD_X:
> > + axis_en = FIELD_GET(ADXL345_ACT_X_EN, axis_ctrl);
> > + break;
> > + case IIO_MOD_Y:
> > + axis_en = FIELD_GET(ADXL345_ACT_Y_EN, axis_ctrl);
> > + break;
> > + case IIO_MOD_Z:
> > + axis_en = FIELD_GET(ADXL345_ACT_Z_EN, axis_ctrl);
> Same as in earlier patch; axis_en is never used.
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + ret = regmap_read(st->regmap, ADXL345_REG_INT_ENABLE, ®val);
> > + if (ret)
> > + return ret;
> > +
> > + *en = (adxl345_act_int_reg[type] & regval) > 0;
> > +
> > + return 0;
> > +}
> > +
> > +static int adxl345_set_act_inact_en(struct adxl345_state *st,
> > + enum iio_modifier axis,
> > + enum adxl345_activity_type type,
> > + bool cmd_en)
> > +{
> > + bool axis_en, en;
> > + unsigned int threshold;
> > + u32 axis_ctrl = 0;
> > + int ret;
> > +
> > + if (type == ADXL345_ACTIVITY) {
> > + switch (axis) {
> > + case IIO_MOD_X:
> > + axis_ctrl = ADXL345_ACT_X_EN;
> > + break;
> > + case IIO_MOD_Y:
> > + axis_ctrl = ADXL345_ACT_Y_EN;
> > + break;
> > + case IIO_MOD_Z:
> > + axis_ctrl = ADXL345_ACT_Z_EN;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + if (cmd_en)
> > + ret = regmap_set_bits(st->regmap,
> > + ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
> > + else
> > + ret = regmap_clear_bits(st->regmap,
> > + ADXL345_REG_ACT_INACT_CTRL, axis_ctrl);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_read(st->regmap, adxl345_act_thresh_reg[type], &threshold);
> > + if (ret)
> > + return ret;
> > +
> > + en = false;
> > +
> > + if (type == ADXL345_ACTIVITY) {
> > + axis_en = FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) > 0;
> The > 0 doesn't add anything as this can't be negative.
>
> Drag declaration of axis_en down here as only used in this block.
> or just combine with previous and next bit as
> en = (type === ADXL345_ACTIVITY) &&
> FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, axis_ctrl) &&
> (threshold > 0);
>
> > + en = axis_en && threshold > 0;
> > + }
> > +
> > + return regmap_update_bits(st->regmap, ADXL345_REG_INT_ENABLE,
> > + adxl345_act_int_reg[type],
> > + en ? adxl345_act_int_reg[type] : 0);
> > +}
> > +
> > /* tap */
> >
>
>
>
> > @@ -1347,6 +1542,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > if (ret)
> > return ret;
> >
> > + ret = regmap_write(st->regmap, ADXL345_REG_ACT_INACT_CTRL, 0);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_write(st->regmap, ADXL345_REG_THRESH_ACT, 6);
>
> 6 is a fairly random number. Add a comment for why this default.
>
My general intention is to provide +/- reasonable default configs,
rather than leave it all to 0 or undefined, to allow to turn the event
on and already catch at least something. In many cases those
will be the default values from the older input driver, to keep a bit
of a compatibility.
Particular cases have actually recommendations in the datasheet and
differ slightly to the input
driver. I'm aware that the input driver probably is not a golden
standard, but it worked at least for
my tests. I may leave a general comment on the section, pls have a
look into v6 if this is ok.
> > + if (ret)
> > + return ret;
> > +
> > ret = regmap_write(st->regmap, ADXL345_REG_THRESH_TAP, tap_threshold);
> > if (ret)
> > return ret;
>
Powered by blists - more mailing lists