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
