[PATCH v2 1/1] iio: accel: adxl367: fix setting odr for activity time update

[PATCH v2 1/1] iio: accel: adxl367: fix setting odr for activity time update

[Lothar Rubusch]

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@gmail.com>
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
---
 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)
-- 
2.39.5

Re: [PATCH v2 1/1] iio: accel: adxl367: fix setting odr for activity time update

[Jonathan Cameron]

On Sun,  9 Mar 2025 19:35:15 +0000
Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com>
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> ---
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)

Re: [PATCH v2 1/1] iio: accel: adxl367: fix setting odr for activity time update

[Lothar Rubusch]

Hi Jonathan,

On Mon, Mar 10, 2025 at 9:10 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun,  9 Mar 2025 19:35:15 +0000
> Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com>
> > Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> > ---
> 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.
>

Yes, it is actually just the fixes tag and reviewed-by tag. I'm sorry
for the missing log.

Best,
L

> 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)
>