On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
>
> Enhance the interrupt handler to process inactivity events. Introduce
> functions to configure the threshold and period registers for
> inactivity detection, as well as to enable or disable the inactivity
> feature. Extend the fake IIO channel to handle inactivity events by
> combining the x, y, and z axes using a logical AND operation.
...
> - axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> + /* Check if axis for activity are enabled */
> + switch (type) {
> + case ADXL313_ACTIVITY:
> + axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
> + break;
> + case ADXL313_INACTIVITY:
> + axis_en = FIELD_GET(ADXL313_INACT_XYZ_EN, axis_ctrl);
> + break;
> + default:
> + return -EINVAL;
> + }
>
> if (!axis_en)
> return false;
So, this looks better without a variable?
case FOO:
if (!FIELD_GET(...))
return false;
break;
On the first glance it seems that next changes don't affect this.
...
> - /* Start modifying configuration registers */
Stray change, the next patch restores this. So why change to begin with?
> ret = adxl313_set_measure_en(data, false);
> if (ret)
> return ret;
...
> /* Enable axis according to the command */
> - axis_ctrl = ADXL313_ACT_XYZ_EN;
> + switch (type) {
I was wondering if you can use switch-case earlier in the series.
> + case ADXL313_ACTIVITY:
> + axis_ctrl = ADXL313_ACT_XYZ_EN;
> + break;
> + case ADXL313_INACTIVITY:
> + axis_ctrl = ADXL313_INACT_XYZ_EN;
> + break;
> + default:
> + return -EINVAL;
> + }
...
> - if (info != IIO_EV_INFO_VALUE)
> - return -EINVAL;
> -
> - switch (dir) {
> - case IIO_EV_DIR_RISING:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + ret = regmap_read(data->regmap,
> + adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> + &act_threshold);
> + if (ret)
> + return ret;
> + *val = act_threshold * 15625;
> + *val2 = MICRO;
> + return IIO_VAL_FRACTIONAL;
> + case IIO_EV_DIR_FALLING:
> + ret = regmap_read(data->regmap,
> + adxl313_act_thresh_reg[ADXL313_INACTIVITY],
> + &inact_threshold);
> + if (ret)
> + return ret;
> + *val = inact_threshold * 15625;
> + *val2 = MICRO;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> + case IIO_EV_INFO_PERIOD:
> ret = regmap_read(data->regmap,
> - adxl313_act_thresh_reg[ADXL313_ACTIVITY],
> - &act_threshold);
> + ADXL313_REG_TIME_INACT,
> + &inact_time_s);
> if (ret)
> return ret;
> - *val = act_threshold * 15625;
> - *val2 = MICRO;
> - return IIO_VAL_FRACTIONAL;
> + *val = inact_time_s;
> + return IIO_VAL_INT;
> default:
> return -EINVAL;
> }
I still don't get what's wrong with helpers for nested switches?
Instead of doing ping-pong with so many lines (due to indentation
changes), just create a helper from the beginning. In this case this
will look more like
if (nfo == IIO_EV_INFO_VALUE)
return my_cool_helper_for_THIS_case(...);
Note, I admit that not all the cases may be done like this, but just
look at this again and perhaps something can be optimised.
--
With Best Regards,
Andy Shevchenko
