Message-ID: <20250525185222.7f541a28@jic23-huawei>
Date: Sun, 25 May 2025 18:52:22 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
 corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de,
 Michael.Hennerich@...log.com, linux-iio@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 00/12] iio: accel: adxl313: add power-save on
 activity/inactivity

On Sun, 25 May 2025 16:54:11 +0200
Lothar Rubusch <l.rubusch@...il.com> wrote:

> On Sun, May 25, 2025 at 2:50 PM Jonathan Cameron <jic23@...nel.org> wrote:
> >
> > On Fri, 23 May 2025 22:35:11 +0000
> > Lothar Rubusch <l.rubusch@...il.com> wrote:
> >  
> > > The patch set covers the following topics:
> > > - add debug register and regmap cache
> > > - prepare iio channel scan_type and scan_index
> > > - prepare interrupt handling
> > > - implement fifo with watermark
> > > - add activity/inactivity together with auto-sleep with link bit
> > > - documentation
> > >
> > > Since activity and inactivity here are implemented covering all axis, I
> > > assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.  
> >
> > Hi Lothar,
> >
> > I think on this occasion you were a bit too speedy in sending out a new
> > version.  Might have been better to wait at least 1-2 weeks at this point
> > in the cycle, or until you had a few more reviews in at least.
> >
> > I don't mind that much, but it does create noise on the list and tends
> > to reduce the focus patch sets get a little.
> >  
> 
> Hi Jonathan & ML, thank you for this hint.
> 
> I assumed Andy was just "taking over" here. In consequence the rounds
> were based on his reviews. For the future, I better await your (any
> IIO maintainers') reviews, until going into next round?

We don't separate quite like that and I'm always keen to have more
people review code - so when things are going well you'll probably
get 2 or 3 people reviewing a series at some point as it is revised.

Mind you people (including me!) sometimes hide from the controversial
ABI discussions :)

So as a general rule unless we are in a rush (e.g. little tweaks
just prior to a merge) it's better to give some time and gather up
multiple reviews.  IIO tends to move quicker than some parts of the
kernel (as we have a good bunch of reviewers) but I'd still generally
not send more than one version a week for first few versions at least.

> I accept how you like to work on this. Nevertheless, isn't it more
> efficient when I resubmit right after Andy's review (if I can), then
> you review and I re-submit again? In this case I would go through my
> codes thoroughly twice, which usually improves quality of the result,
> IMHO. Since only the most recent version of my patches should actually
> be considered, the older ones could simply be ignored (not sure if it
> is possible to flag this somenow from your maintainer side). I can see
> the point, though, where this increases the number of mails on the
> list. Nvm, just an idea. I'll wait in future.

It's not a clear cut thing but I still tend to scan read through all the
comments on previous versions to avoid getting into an ill informed
argument with another reviewer, so there is little advantage in sending
a new version unless there are either major changes or review feedback
has settled down.

Also there is always testing and a small amount of overhead in a patch
series preparation so it's not free for you either. A bit of batching
up of fixing multiple sets of comments can help there too!


> 
> ADXL313: I neither care much about the number of rounds, nor about the
> split of patches. Thus I split rather a bit too much and you tell me
> how I shall merge (I think that's easier than sending you in a big
> blob patch and figuring out then what and how to separate). Pls, let
> me know if you oppose to this approach?

We all get this balance wrong sometimes and it doesn't matter if it
ends up slightly off.  Patches that just change a few values that
aren't used until quite a few patches later are generally a bad idea.

Also size of patch plays into this.  If it's under 500 lines
it is much less likely you'll get a request to split (as long as
arguably it's all one feature) than if it is larger than that.

> 
> BTW, I also still had a more recent version of the ADXL345 series,
> containing the freefall and inactivity story. Current
> question/proposal: Freefall and inactivity, send out the same MAG
> event. An Idea could be, that userland software simply has logic to
> distinguish on timing, but the kernelspace driver here is doing just
> the same IIO event.
> 
> Long story short - I shifted freefall to the end (also in oder to
> easily rather exclude it from that series). I expect this NOT to be
> the final round. First, there is the freefall situation (actually I
> expect objections from your side. If so, I'll exclude freefall from
> here). Second, by some of Andys reviews I feel I also should improve
> the ADXL345 a bit. I learned about regmap_assign_bits() which comes in
> very handy. So, if you tell me the freefall approach in ADXL345 is
> nonsense, I'll exclude it from this series.

I got to that a bit later in the day.  I was expecting it to take
more thought so left it to nearly the end when catching up.

I've replied in that thread to the freefall proposal. It's not
nonsense of course, I just don't think we have figured out a way
to avoid the expectations built into userspace code that is already
out there.

Note I'm also not sure about the innovative use of IIO_MOD_STILL
(which is a human motion thing to indicate not running, walking etc)
that the cros_ec activity sensor that was posted this week used.
That was rather unexpected. They do have the small advantage over
most drivers that they also control the relevant userspace (chrome OS
I believe). 

Seems like we are in a period of pushing against the boundaries
of the ABI :(

BTW regmap_assign_bits() was new to me too ;)

Have a good remainder of the weekend!

Jonathan
> 
> Best,
> L
> 
> 
> > Jonathan  

Powered by blists - more mailing lists