Message-ID: <20250611161504.56d402e2@jic23-huawei>
Date: Wed, 11 Jun 2025 16:15:04 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: Andy Shevchenko <andy.shevchenko@...il.com>, 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

On Wed, 11 Jun 2025 16:49:34 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

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

I'd do the from the first but with the comment up with where ret is
declared.  

> 
> 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.
Don't do that. Just use the switch from the start.

Sometimes we will give review feedback that doesn't take the whole
series into account (because it takes much longer to review a full series
then reread the feedback to spot anything that turned out to be due
to a later change)  In those cases it is fine to just reply to the
comment with - "The switch gathers additional elements in patches X,Y,Z
and so is introduced in this first patch to reduce churn.

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

The code is correct I think as I said in my review, but it is a little
unusual.  One option is sanity check and return early if none of the
events we support is set.  That removes this path from consideration.

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