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

[PATCH v1 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.

Signed-off-by: Lothar Rubusch <l.rubusch@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 v1 1/1] iio: accel: adxl367: fix setting odr for activity time update

[Jonathan Cameron]

On Fri, 21 Feb 2025 20:33:52 +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.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Fixes tag?

Otherwise looks good to me.

> ---
>  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 v1 1/1] iio: accel: adxl367: fix setting odr for activity time update

[Lothar Rubusch]

On Sat, Feb 22, 2025 at 4:03 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 21 Feb 2025 20:33:52 +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.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> Fixes tag?
>

Hi IIO ML readers - Hi Jonathan,
AFAIK there is no tracked bug which I could refer to. Alternatively, I
could refer to
the commit hash of the original commit which introduced the code this
patch is supposed
to fix. Is this ok? Could you please help me here with the process?

> Otherwise looks good to me.
>
> > ---
> >  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 v1 1/1] iio: accel: adxl367: fix setting odr for activity time update

[Marcelo Schmitt]

Hi Lothar,

On 03/08, Lothar Rubusch wrote:
> On Sat, Feb 22, 2025 at 4:03 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 21 Feb 2025 20:33:52 +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.
> > >
...
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
...
> 
> Hi IIO ML readers - Hi Jonathan,
> AFAIK there is no tracked bug which I could refer to. Alternatively, I
> could refer to
> the commit hash of the original commit which introduced the code this
> patch is supposed
> to fix. Is this ok? Could you please help me here with the process?

Yes, the fixes tag should point to the commit that introduced the code that is
being fixed. So, this this patch should carry the following tag

Fixes: cbab791c5e2a ("iio: accel: add ADXL367 driver")

Also,
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

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

[Jonathan Cameron]

On Sat, 8 Mar 2025 22:26:36 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Feb 22, 2025 at 4:03 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 21 Feb 2025 20:33:52 +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.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Fixes tag?
> >  
> 
> Hi IIO ML readers - Hi Jonathan,
> AFAIK there is no tracked bug which I could refer to. Alternatively, I
> could refer to
> the commit hash of the original commit which introduced the code this
> patch is supposed
> to fix. Is this ok? Could you please help me here with the process?

Follow description in the submitting-patches documentation.

https://elixir.bootlin.com/linux/v6.14-rc5/source/Documentation/process/submitting-patches.rst

Here the commit hash + description is what I am after in the appropriate
format as described in that doc.

I don't really care about patch trackers as most bug fixes are never in
them as they come from people noticing issues whilst reading or testing
the code during other development.

Jonathan

> 
> > Otherwise looks good to me.
> >  
> > > ---
> > >  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)  
> >