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

[Andy Shevchenko]

On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@xxxxxxxxx> 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