| Message ID | 20250309193515.2974-1-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/1] iio: accel: adxl367: fix setting odr for activity time update | expand |
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)
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) >
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)
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(-)