Message-ID: <CAFXKEHYP6o5vzsSP24SLUSs+Tu2Oqm=oVf71xy8EKKD5hoCQqg@mail.gmail.com>
Date: Wed, 11 Jun 2025 17:36:49 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Andy Shevchenko <andy.shevchenko@...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 9:46 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> 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?).

The point here is, as I mentioned in the other mail:
Initially, I wanted to build up the final switch/case struct i.e.
going by MAG/MAG_ADAPTIVE, then INFO_VALUE -> RISING / FALLING and
PERIOD.

This will distinguish properties for four different types of events,
of course it then also will use separate functions. As I uderstood
your review, why starting with switch/case, do
if (!MAG event) then, return right away. I implemented that as I
understood. For further switch/case-ing, I did the same.
Now, patch by patch, it grows. Thus the if-not-back-out lines will be
moved out and replaced by switch/case. Worse, with every level switch
case, all existing code needs indention, thus reading through the
patches show (too) many changes.

How can I improve to help you reviewing this or make the feedback more
useful for me? Or is my approach wrong? I'd like to start with the
switch case right away, then just add up what comes in with every
other patch. If so, you'd only see the changes, since the final
structure of this is already clear, because very similar to all
iio/accel drivers at least (as you probably know better than me).

>
> > +       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

Powered by blists - more mailing lists