linux-kernel - Re: [PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

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: <CAHp75Vd=mzfVN_UBUHAkTyj2Ap_tz76AB0LtKEz28pR=WmNzog@mail.gmail.com>
Date: Sun, 1 Jun 2025 22:45:29 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com, 
	andy@...nel.org, corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de, 
	Michael.Hennerich@...log.com, bagasdotme@...il.com, linux-iio@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 08/11] iio: accel: adxl313: add inactivity sensing

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> Extend the interrupt handler to process interrupts as inactivity events.
> Add functions to set threshold and period registers for inactivity. Add
> functions to enable / disable inactivity. Extend the fake iio channel to

IIO

> deal with inactivity events on x, y and z combined with AND.

...

> +static int adxl313_set_inact_time_s(struct adxl313_data *data,
> +                                   unsigned int val_s)
> +{
> +       unsigned int max_boundary = 255;

This is unclear how it's defined. What is the limit behind? Size of a
bit field? Decimal value from the datasheet?

The forms of (BIT(8) - 1) or GENMASK(7, 0) may be better depending on
the answers to the above questions.

> +       unsigned int val = min(val_s, max_boundary);
> +
> +       return regmap_write(data->regmap, ADXL313_REG_TIME_INACT, val);
> +}

...

> -       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +       if (type == ADXL313_ACTIVITY)
> +               axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> +       else
> +               axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);

Even with this change my previous comment stays.

...

> +       en = cmd_en && threshold;
> +       if (type == ADXL313_INACTIVITY) {
> +               ret = regmap_read(data->regmap, ADXL313_REG_TIME_INACT, &inact_time_s);
> +               if (ret)
> +                       return ret;
> +
> +               en = en && inact_time_s;
> +       }

...

> -       if (info != IIO_EV_INFO_VALUE)
> -               return -EINVAL;
> -
> -       /* Scale factor 15.625 mg/LSB */
> -       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> -       switch (dir) {
> -       case IIO_EV_DIR_RISING:
> -               ret = regmap_write(data->regmap,
> -                                  adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> -                                  regval);

Hmm... This was added by the previous patches, right? Why can't it be
done as a switch case to begin with? I remember one of the previous
versions had some nested switch-cases, perhaps you need to rethink on
how to split the code between functions to avoid too much nesting (add
some helper functions?).

> +       switch (info) {
> +       case IIO_EV_INFO_VALUE:
> +               /* Scale factor 15.625 mg/LSB */
> +               regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> +               switch (dir) {
> +               case IIO_EV_DIR_RISING:
> +                       ret = regmap_write(data->regmap,
> +                                          adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> +                                          regval);
> +                       if (ret)
> +                               return ret;
> +                       return adxl313_set_measure_en(data, true);
> +               case IIO_EV_DIR_FALLING:
> +                       ret = regmap_write(data->regmap,
> +                                          adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> +                                          regval);
> +                       if (ret)
> +                               return ret;
> +                       return adxl313_set_measure_en(data, true);
> +               default:
> +                       return -EINVAL;
> +               }
> +       case IIO_EV_INFO_PERIOD:
> +               ret = adxl313_set_inact_time_s(data, val);
>                 if (ret)
>                         return ret;
>                 return adxl313_set_measure_en(data, true);

-- 
With Best Regards,
Andy Shevchenko