Message-ID: <CAFXKEHZcS2qpb1zp6kkQm_Pb-MxYHErpjD=q6huuLm1Nq=xjqA@mail.gmail.com>
Date: Wed, 11 Jun 2025 16:49:34 +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 07/11] iio: accel: adxl313: add activity sensing

Hi Andy,

On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
> On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@...il.com> wrote:
> >
> > Add possibilities to set a threshold for activity sensing. Extend the
> > interrupt handler to process activity interrupts. Provide functions to set
> > the activity threshold and to enable/disable activity sensing. Further add
> > a fake channel for having x, y and z axis anded on the iio channel.
>
> IIO
>
> And what does the 'anded' mean?
>
> > This is a preparatory patch. Some of the definitions and functions are
> > supposed to be extended for inactivity later on.
>
> ...
>
> > +static int adxl313_is_act_inact_en(struct adxl313_data *data,
> > +                                  enum adxl313_activity_type type)
> > +{
> > +       unsigned int axis_ctrl;
> > +       unsigned int regval;
> > +       int axis_en, int_en, ret;
> > +
> > +       ret = regmap_read(data->regmap, ADXL313_REG_ACT_INACT_CTL, &axis_ctrl);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /* Check if axis for activity are enabled */
> > +       if (type != ADXL313_ACTIVITY)
> > +               return 0;
> > +
> > +       axis_en = FIELD_GET(ADXL313_ACT_XYZ_EN, axis_ctrl);
>
> If it's false, it will be false anyway. No need to defer the check:
>
>   if (!axis_en)
>     return false;
>
> > +       /* The axis are enabled, now check if specific interrupt is enabled */
> > +       ret = regmap_read(data->regmap, ADXL313_REG_INT_ENABLE, &regval);
> > +       if (ret)
> > +               return ret;
> > +
> > +       int_en = adxl313_act_int_reg[type] & regval;
> > +
> > +       return axis_en && int_en;
>
>   return ... & regval;
>
> > +}
>
> I have already commented on this a couple of times.
>
> ...
>
> > +       /* Scale factor 15.625 mg/LSB */
> > +       regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
>
> I would rather do
>
> val * MICRO + val2
>
> which is read more naturally (we will easily get that the expression
> uses MICRO scale).
>
> ...
>
> > +       int ret = -ENOENT;
> > +
> > +       if (FIELD_GET(ADXL313_INT_ACTIVITY, int_stat)) {
> > +               ret = iio_push_event(indio_dev,
> > +                                    IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > +                                                       IIO_MOD_X_OR_Y_OR_Z,
> > +                                                       IIO_EV_TYPE_MAG,
> > +                                                       IIO_EV_DIR_RISING),
> > +                                    ts);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >
> >         if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> >                 samples = adxl313_get_samples(data);
> >                 if (samples < 0)
> >                         return samples;
> >
> > -               return adxl313_fifo_push(indio_dev, samples);
> > +               ret = adxl313_fifo_push(indio_dev, samples);
>
> This is not needed...
>

IMHO this will be needed, or shall be needed in the follow up context.

The [going to be renamed] function push_events() shall evaluate the
interrupt status register for the events the driver can handle and
also eventually drain the FIFO in case of watermark. It shall
distinguish between failure, events / drain the FIFO which can be
handled, and events which cannot be handled so far. It's not a if /
else, there can be some event, and some fifo data. Therefore I'd like
not a simple return here, but init a ret var.

I interpreted your reviews, to change the particular implementation as
if there was just activity. Then in a follow up patch, rewrite it
again, now to distinguish just bewteen just activity and inactivity
e.g. by if/else. Eventually rewrite it by a third approach to
distinghish activity, inactivity, AC-coupled activity and AC-coupled
inactivity, might be now switch/case. Eventually you might complain
that my patches contain way too much modification of every line in
every patch.

I'd rather like to start right away with the final structure with just
the first element - e.g. "activity" - leads to results like the above.
Less churn among patches, but having just one element looks like
having taken an over-complicated approach.

Perhaps it's my patch split? Unsure, I tried to note in the commit message:
> This is a preparatory patch. Some of the definitions and functions are
> supposed to be extended for inactivity later on.
Perhaps it needs more feedback here?

Another example is seting up the read/write_event_config() or
read/write_event_value() functions. I mean, eventually this will
become a switch/case implementation. Of course with just one element
switch/case seems to be obvious overkill. Going by your advice, I
changed it to if(!..) return, it's definitely cleaner. Definitely in
the follow up patches this will be rewritten, though.

Please, let me know what is the best approach or what I can improve to
avoid such "ping pong patching" as you name it?

Might be that you're right here in this particular case, but then it
would be better to discuss the final structure, isn't it?


> >         }
> >
> >         /* Return error if no event data was pushed to the IIO channel. */
> > -       return -ENOENT;
> > +       return ret;
>
> ...and this looks wrong.

Well, as I said. Each separate if-condition (not just if-else), could
be ok or not. If ok, the function still shall continue, might be at
the end, also a watermark flag is in the status reg and the FIFO needs
to be drained. It also might be, that some event comes which the
driver does still not handle, but not necessarily an error
(missconfiguration). So, draining the FIFO helps in most cases to
bring a derailed sensor back on track. If not doing so, it silmply
stops working, you would need to turn off and on again, or even power
cycle the setup.

Probably you have a better idea here, but pls have a look into the
final setup. I really appreciate your feedbacks. I understand this is
a rather problematic part of the code. To me it makes sense like this,
but I'd highly appreciate your advice.

>
> Before the case was clear, if we have no respective bit set in the
> int_stat, we return ENOENT. Now it depends on the other bit. If this
> is correct behaviour, it needs a comment.
>
> --
> With Best Regards,
> Andy Shevchenko

Powered by blists - more mailing lists