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.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
v2 -> v3:
- verify keeping trailing comma when it's multi-line assignment [v1 02/12]
- adxl313_set_fifo(): verify have two on one line to make it easier to read [v1 07/12]
- adxl313_fifo_transfer(): verify removal of useless initialization of ret [v1 07/12]
- adxl313_fifo_transfer(): verify usage of array_size() from overflow.h [v1 07/12]
- adxl313_fifo_transfer(): verify return 0 here [v1 07/12]
- adxl313_irq_handler(): verify "Why do we need the label?" / moving the call under the conditional [v1 07/12]
- verify reorganization of half condition for Activity [v1 09/12] and Inactivity [v1 10/12]
- verify usage of MICRO instead of 1000000
- adxl313_is_act_inact_en(): restructure according to return logic value, or negative error
- adxl313_set_act_inact_en(): restructure function, use regmap_assign_bits()
- adxl313_set_act_inact_en(): verify makeing it a logical split [v1 11/12]
- adxl313_fifo_transfer(): change iterator variable type from int to unsigned int [v2 07/12]
- adxl313_fifo_reset(): add comment on why reset status registers does not do error check ("At least comment...") [v2 07/12]
- adxl313_fifo_push(): change iterator variable from int to unsigned int [v2 08/12]
- adxl313_fifo_push(): remove duplicate check for samples being <0 [v2 08/12]
- apply regmap_assign_bits() in several places to replace regmap_update_bits() depending on bools
- adxl313_set_watermark(): rename mask variable to make it more comprehensive
- removal of duplicate blanks in various places (sry, my keyboard died) [v1 07/12]

v1 -> v2:
- usage of units.h
- simplify approach for return values
---
Lothar Rubusch (12):
  iio: accel: adxl313: add debug register
  iio: accel: adxl313: introduce channel scan_index
  iio: accel: adxl313: configure scan type for buffer
  iio: accel: adxl313: make use of regmap cache
  iio: accel: adxl313: add function to enable measurement
  iio: accel: adxl313: prepare interrupt handling
  iio: accel: adxl313: add basic interrupt handling
  iio: accel: adxl313: add FIFO watermark
  iio: accel: adxl313: add activity sensing
  iio: accel: adxl313: add inactivity sensing
  iio: accel: adxl313: implement power-save on inactivity
  docs: iio: add ADXL313 accelerometer

 Documentation/iio/adxl313.rst    | 196 ++++++++++
 Documentation/iio/index.rst      |   1 +
 drivers/iio/accel/adxl313.h      |  35 +-
 drivers/iio/accel/adxl313_core.c | 625 ++++++++++++++++++++++++++++++-
 drivers/iio/accel/adxl313_i2c.c  |   6 +
 drivers/iio/accel/adxl313_spi.c  |   6 +
 6 files changed, 859 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/iio/adxl313.rst

-- 
2.39.5

On Fri, 23 May 2025 22:35:11 +0000
Lothar Rubusch <l.rubusch@gmail.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.

Jonathan

On Sun, May 25, 2025 at 2:50 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 23 May 2025 22:35:11 +0000
> Lothar Rubusch <l.rubusch@gmail.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?
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.

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?

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.

Best,
L


> Jonathan

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

> On Sun, May 25, 2025 at 2:50 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 23 May 2025 22:35:11 +0000
> > Lothar Rubusch <l.rubusch@gmail.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