Re: [PATCH v4 07/11] iio: accel: adxl313: add activity sensing

[Andy Shevchenko]

On Wed, Jun 11, 2025 at 04:15:04PM +0100, Jonathan Cameron wrote:
> On Wed, 11 Jun 2025 16:49:34 +0200
> Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> > On Sun, Jun 1, 2025 at 9:38 PM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:  

...

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

But at the same time if switch becomes nested and 2+ levels, it's better
to split the inner parts to the helpr functions or so. Doing a switch
with 2+ levels looks ugly independently on the approach taken.

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

Indeed.

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

-- 
With Best Regards,
Andy Shevchenko