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

[Lothar Rubusch]

On Sat, Dec 14, 2024 at 1:02 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 13 Dec 2024 21:19:03 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> 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@xxxxxxxxx>
> 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"?

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