Re: [PATCH v7 1/7] iio: accel: adxl345: add function to switch measuring mode

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 15 Dec 2024 10:41:12 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Dec 14, 2024 at 1:02 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 13 Dec 2024 21:19:03 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Replace the powerup / powerdown functions by a generic function to put
> > > the sensor in STANDBY, or MEASURE mode. When configuring the FIFO for
> > > several features of the accelerometer, it is recommended to put
> > > measuring in STANDBY mode.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > Mostly in the interests of trimming down the queue of patches in flight
> > and because this one has been fine for a few versions without significant
> > comment.
> >
> > Applied this patch to the togreg branch of iio.git and pushed out initially
> > as testing to let 0-day take a look.
> >  
> 
> Question here: you applied this patch now to the iio branch. Now,
> Christophe Jaillet pointed still something out that could be improved,
> the function could be shortened to, e.g.
> 
> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> +{
> +     unsigned int val = en ? ADXL345_POWER_CTL_MEASURE :
> ADXL345_POWER_CTL_STANDBY;
> +
> +     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +}
> 
> Should I present an improved patch? Or, in case this was urgent, would
> it require an additional patch/fix? What would be the way to deal with
> such fixes immediately after "applied"?
> 
Send me a patch on top. I may well squash it into the original (particularly
if you stick a note on that being sensible under the --- for the patch!)

There is always a window in which I'm happy to rebase (or pull a patch
if we get feedback needing bigger changes) because I typically give
at least half a week for 0-day to poke the tree before I make a potential
mess in linux-next.  Ultimately I'll rebase after that for a sufficiently
serious issue but for something like this I'll just apply the patch on top
once it's out for linux-next to pick up.

Jonathan

> Best,
> L
> 
> > Thanks
> >
> > Jonathan
> >  
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 42 +++++++++++++++++++++++---------
> > >  1 file changed, 30 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 88df9547b..b48bc838c 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -138,6 +138,34 @@ static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev,
> > >       }
> > >  }
> > >
> > > +/**
> > > + * adxl345_set_measure_en() - Enable and disable measuring.
> > > + *
> > > + * @st: The device data.
> > > + * @en: Enable measurements, else standby mode.
> > > + *
> > > + * For lowest power operation, standby mode can be used. In standby mode,
> > > + * current consumption is supposed to be reduced to 0.1uA (typical). In this
> > > + * mode no measurements are made. Placing the device into standby mode
> > > + * preserves the contents of FIFO.
> > > + *
> > > + * Return: Returns 0 if successful, or a negative error value.
> > > + */
> > > +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > > +{
> > > +     unsigned int val = 0;
> > > +
> > > +     val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
> > > +     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > > +}
> > > +
> > > +static void adxl345_powerdown(void *ptr)
> > > +{
> > > +     struct adxl345_state *st = ptr;
> > > +
> > > +     adxl345_set_measure_en(st, false);
> > > +}
> > > +
> > >  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> > >  "0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200"
> > >  );
> > > @@ -158,16 +186,6 @@ static const struct iio_info adxl345_info = {
> > >       .write_raw_get_fmt      = adxl345_write_raw_get_fmt,
> > >  };
> > >
> > > -static int adxl345_powerup(void *regmap)
> > > -{
> > > -     return regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_MEASURE);
> > > -}
> > > -
> > > -static void adxl345_powerdown(void *regmap)
> > > -{
> > > -     regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
> > > -}
> > > -
> > >  /**
> > >   * adxl345_core_probe() - Probe and setup for the accelerometer.
> > >   * @dev:     Driver model representation of the device
> > > @@ -237,11 +255,11 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > >                                    regval, ADXL345_DEVID);
> > >
> > >       /* Enable measurement mode */
> > > -     ret = adxl345_powerup(st->regmap);
> > > +     ret = adxl345_set_measure_en(st, true);
> > >       if (ret < 0)
> > >               return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
> > >
> > > -     ret = devm_add_action_or_reset(dev, adxl345_powerdown, st->regmap);
> > > +     ret = devm_add_action_or_reset(dev, adxl345_powerdown, st);
> > >       if (ret < 0)
> > >               return ret;
> > >  
> >