On Sun, 9 Mar 2025 19:35:15 +0000
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> Fix setting the odr value to update activity time based on frequency
> derrived by recent odr, and not by obsolete odr value.
>
> The [small] bug: When _adxl367_set_odr() is called with a new odr value,
> it first writes the new odr value to the hardware register
> ADXL367_REG_FILTER_CTL.
> Second, it calls _adxl367_set_act_time_ms(), which calls
> adxl367_time_ms_to_samples(). Here st->odr still holds the old odr value.
> This st->odr member is used to derrive a frequency value, which is
> applied to update ADXL367_REG_TIME_ACT. Hence, the idea is to update
> activity time, based on possibilities and power consumption by the
> current ODR rate.
> Finally, when the function calls return, again in _adxl367_set_odr() the
> new ODR is assigned to st->odr.
>
> The fix: When setting a new ODR value is set to ADXL367_REG_FILTER_CTL,
> also ADXL367_REG_TIME_ACT should probably be updated with a frequency
> based on the recent ODR value and not the old one. Changing the location
> of the assignment to st->odr fixes this.
>
> Fixes: cbab791c5e2a5 ("iio: accel: add ADXL367 driver")
> Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>
> ---
Change log missing, but I assume it's just the Fixes tag.
If so, I would have been fine with that just being in a reply
to the v1. Anyhow, new patch is fine too so applied to the
fixes-togreg branch of iio.git. I may well queue this up for
the merge window rather than send another pull request this cycle.
Thanks
Jonathan
> drivers/iio/accel/adxl367.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
> index add4053e7a02..0c04b2bb7efb 100644
> --- a/drivers/iio/accel/adxl367.c
> +++ b/drivers/iio/accel/adxl367.c
> @@ -601,18 +601,14 @@ static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
> if (ret)
> return ret;
>
> + st->odr = odr;
> +
> /* Activity timers depend on ODR */
> ret = _adxl367_set_act_time_ms(st, st->act_time_ms);
> if (ret)
> return ret;
>
> - ret = _adxl367_set_inact_time_ms(st, st->inact_time_ms);
> - if (ret)
> - return ret;
> -
> - st->odr = odr;
> -
> - return 0;
> + return _adxl367_set_inact_time_ms(st, st->inact_time_ms);
> }
>
> static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)
