Re: [PATCH v3 11/12] iio: accel: adxl313: implement power-save on inactivity

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 23 May 2025 22:35:22 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Link activity and inactivity to indicate the internal power-saving state.
> Add auto-sleep to be linked to inactivity.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl313.h      |  3 +++
>  drivers/iio/accel/adxl313_core.c | 25 +++++++++++++++++++++++--
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index 374e4bfe6e05..fae2378fa4ed 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -41,6 +41,9 @@
>  #define ADXL313_RATE_BASE		6
>  
>  #define ADXL313_POWER_CTL_MSK		BIT(3)
> +#define ADXL313_POWER_CTL_INACT_MSK	GENMASK(5, 4)
> +#define ADXL313_POWER_CTL_LINK		BIT(5)
> +#define ADXL313_POWER_CTL_AUTO_SLEEP	BIT(4)
>  
>  #define ADXL313_RANGE_MSK		GENMASK(1, 0)
>  #define ADXL313_RANGE_MAX		3
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index f7baca814da7..d65380901f66 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -389,6 +389,7 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
>  	unsigned int axis_ctrl;
>  	unsigned int threshold;
>  	unsigned int inact_time_s;
> +	int act_en, inact_en;
>  	bool en;
>  	int ret;
>  
> @@ -415,8 +416,28 @@ static int adxl313_set_act_inact_en(struct adxl313_data *data,
>  		en = en && inact_time_s;
>  	}
>  
> -	return regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> -				  adxl313_act_int_reg[type], en);
> +	ret = regmap_assign_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> +				 adxl313_act_int_reg[type], en);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Advanced power saving: Set sleep and link bit only when ACT and INACT
> +	 * are set. Check enable against regmap cached values.

No need to mention regmap cache.  That's just an optimization to avoid us
going and asking the device for register contents.  It should not be
a fundamental part of how the driver works - rather it should just allow
us to not bother with our own optimizations when it already does the job.

> +	 */
> +	act_en = adxl313_is_act_inact_en(data, ADXL313_ACTIVITY);
> +	if (act_en < 0)
> +		return act_en;
> +
> +	inact_en = adxl313_is_act_inact_en(data, ADXL313_INACTIVITY);
> +	if (inact_en < 0)
> +		return inact_en;
> +
> +	en = en && act_en && inact_en;
> +
> +	return regmap_assign_bits(data->regmap, ADXL313_REG_POWER_CTL,
> +				  (ADXL313_POWER_CTL_AUTO_SLEEP | ADXL313_POWER_CTL_LINK),
> +				  en);

For the clearing of link there advice in the spec to exit measurement mode and reenter it
that I'm not spotting here.  See "Link bit" section.


>  }
>  
>  static int adxl313_read_raw(struct iio_dev *indio_dev,