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

[Jonathan Cameron <jic23@kernel.org>]

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