linux-kernel - Re: [PATCH v5 5/8] 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: <CAHp75VdWtY6nxFFTucoCRiq+tM5caL5N1yp_15=FSAZ7Vyk_7Q@mail.gmail.com>
Date: Mon, 16 Jun 2025 13:58:34 +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 v5 5/8] iio: accel: adxl313: add inactivity sensing

On Mon, Jun 16, 2025 at 1:23 AM Lothar Rubusch <l.rubusch@...il.com> 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