Message-ID: <aEmbLG0S5P7m35a-@smile.fi.intel.com>
Date: Wed, 11 Jun 2025 18:05:16 +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,
	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, Jun 11, 2025 at 04:49:34PM +0200, Lothar Rubusch wrote:
> 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:

...

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

Right, but wouldn't be better to update at the same time when this new context
appears?

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

Basically I use the following rule of thumb: I made an approach and look at
the each patch separately and at the series as a whole (with the end result).
If it's too much of rewriting (yes, I admit, that in some cases it's
unavoidable to have some changes as we do feature-by-feature incremental
changes), I try to rethink. Repeat, until the result looks good enough.
I.o.w. you, as the author of this code, can propose something better based on
your knowledge of the HW and vision of what you want at the end.

Yeah, this might require time and a few attempts which one can argue would be
waste of time. But at least this is my personal experience and flow with my
own patches.

> > >         }
> > >
> > >         /* 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.

I will do my best. Thanks for your patience!

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