Message-ID: <20250622153423.0d8ddcdb@jic23-huawei>
Date: Sun, 22 Jun 2025 15:34:23 +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, bagasdotme@...il.com,
 linux-iio@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 0/8] iio: accel: adxl313: add power-save on
 activity/inactivity

On Sun, 22 Jun 2025 12:29:29 +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
> - add ac coupled activity/inactivity, integrate with auto-sleep and link bit
> - documentation
> 
> Sorry for the fuzz: when I was about to rebase for submitting I
> noticed Jonathan actually already applied parts of this. I'd recommend
> to consider v6 rather over v5.
> 
> Since activity and inactivity here are implemented covering all axis, I
> assumed x&y&z and x|y|z, respectively. Thus the driver uses a fake
> channel for activity/inactiviy. AC-coupling is similar to other Analog Device
> accelerometers, so MAG_ADAPTIVE events are chosen. Combinations are
> documented and functionality tested and verified working.
> 
Given reply to wrong email thread probably meant first few patches of v5 that
I picked up, I've dropped them for now.

Thanks,

J
> Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> ---
> v5 -> v6:
> - `adxl313_core_probe()`: change conditional logic for evaluation of
>   `int_line`
> - `adxl313_core_probe()`: add helper function `_get_int_type()` to simplify
>   logic
> - `adxl313_is_act_inact_en()`: remove blank line; make variable `axis_en` a
>   bool
> - `adxl313_is_act_inact_ac()`: removal of ',' in comment
> - `adxl313_set_act_inact_linkbit()`: regroup logic expressions
> - `adxl313_set_act_inact_en()`: change comment style, and rephrase
> - `adxl313_read/write_mag_config()`, `adxl313_read/write_mag_value()` and
>   `_adxl313_read/write_mag_value()`: avoid nested switch/case statements,
>   add helper functions from start and populate them incrementallay
> - `adxl313_set_act_inact_ac()`: move AC-coupling code into a separate
>   helper
> - [PATCH v5 1/8], [PATCH v5 2/8], [PATCH v5 6/8] and [PATCH v5 8/8]: set
>   "Reviewed-by:.."
> 
> v4 -> v5:
> - [v4 01/11]: applied - debug register                                          
> - [v4 03/11]: applied w/ changed commit message - regmap cache                  
> - refrase all commit messages                                                   
> - merge patches [v4 02/11] [v4 05/11] and [v4 06/11]                            
> - add ADXL313_REG_INT_SOURCE to the initial regmap cache definition             
> - `adxl313_set_watermark()`: replace plain hex numbers by defined bit masks     
> - `adxl313_set_watermark()`: replace `regmap_update_bits()` by
>   `regmap_set_bits()`
> - `adxl313_get_samples()`: remove initialization of variable `samples`          
> - `adxl313_buffer_postenable()`: add comment on turning off measurment          
> - `adxl313_push_event()`: move WATERMARK separate out, focus on pushing events  
> - `adxl313_irq_handler()`: add comment on draining the FIFO                     
> - `adxl313_push_event()`: remove missleading comment on return statement        
> - `adxl313_is_act_inact_en()`: If it's false, it will be false anyway -
>   simplified now
> - change order in multiplication with unit: `val * MICRO` which is read
>   more naturally
> - `adxl313_is_act_inact_en()`: remove check for ADXL313_ACTIVITY in the
>   activity patch
> - `adxl313_write_event_value()`: remove the general turning off measurement mode
> - `adxl313_set_inact_time_s()`: replace plain number 255 by U8_MAX              
> - `adxl313_read/write_event_config()`: encapsulate duplicate code into
>   `adxl313_read/write_mag_config()`
> - `adxl313_read/write_event_value()`: encapsulate duplicate code into
>   `adxl313_read/write_mag_value()`
> - `adxl313_is_act_inact_en()`: apply switch/case rather than if/else for
>   readability; factor out variable `coupling`; convert all remaining `_en`
>   variables there to bool, such that a negative error is evaluated from a
>   `ret`, and     logic operates with `_en` variables
> - `adxl313_set_act_inact_en()`: major rework due to issues discovered by
>   automated testing (also affects related functions)
> - fix kernel-doc issues 
> 
> v3 -> v4:
> - squash patches [v3 02/12 + 03/12]: buffer usage into the patch that adds buffered support
> - squash patches [v3 07/12 + 08/12]: interrupt handler with watermark implementation
> - add patch: (in)activity / AC coupled as `MAG_ADAPTIVE` event
> - `ADXL313_MEASUREMENT_MODE`: adjust commit message on removal of define and adding measurement enable function
> - remove irq variable from driver data struct, make it a local variable
> - `adxl313_core_probe()`: flip logic to condition `int_line != ADXL313_INT_NONE`
> - `adxl313_core_probe()`: change mapping interrupts from 0xff to an explicit local mask
> - `adxl313_core_probe()`: add comment on FIFO bypass mode
> - reduce odd selection of headers to add [`adxl313_core.c`]
> - `adxl313_set_fifo()`: this function was turning measurement off/on before changing `fifo_mode`,
>    called in postenable and predisable this firstly excluded setting of interrupts, and secondly
>    still configured watermark where unnecessary, this function was thus removed (covers unhandled
>    return value, and refactoring of function parameters)
> - `adxl313_fifo_transfer()`: simplify computation of `sizeof(i*count/2)`
> - `adxl313_irq_handler()`: make call of `adxl313_reset_fifo()` conditional to OVERRUN one patch earlier
> - includes: rework adding included headers
> - activity: change to work with or'd axis and related changes to the fake channel and arrays
> - `adxl313_set_act_inact_en()`: generally turn off measurement when adjusting config
>   activity/inactivity related config registers, turn measurement on after
> - doc: adjust code block highlighting and remove links
> 
> 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 (8):
>   iio: accel: adxl313: make use of regmap cache
>   iio: accel: adxl313: add function to enable measurement
>   iio: accel: adxl313: add buffered FIFO watermark with interrupt
>     handling
>   iio: accel: adxl313: add activity sensing
>   iio: accel: adxl313: add inactivity sensing
>   iio: accel: adxl313: implement power-save on inactivity
>   iio: accel: adxl313: add AC coupled activity/inactivity events
>   docs: iio: add ADXL313 accelerometer
> 
>  Documentation/iio/adxl313.rst    | 289 ++++++++++
>  Documentation/iio/index.rst      |   1 +
>  drivers/iio/accel/adxl313.h      |  33 +-
>  drivers/iio/accel/adxl313_core.c | 937 ++++++++++++++++++++++++++++++-
>  drivers/iio/accel/adxl313_i2c.c  |   6 +
>  drivers/iio/accel/adxl313_spi.c  |   6 +
>  6 files changed, 1261 insertions(+), 11 deletions(-)
>  create mode 100644 Documentation/iio/adxl313.rst
> 
> 
> base-commit: d1584d12ec8c35534172882c1713947110564e4c
> prerequisite-patch-id: 263cdbf28524f1edc96717db1461d7a4be2319c2

Powered by blists - more mailing lists