On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Thu, 5 Dec 2024 17:13:37 +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>
> Hi Lothar,
>
> One trivial comment inline.
>
> Jonathan
>
> > ---
> > drivers/iio/accel/adxl345_core.c | 44 ++++++++++++++++++++++----------
> > 1 file changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 98ff37271f1..1d020b0d79c 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
> > @@ -236,13 +254,13 @@ 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;
> > + return dev_err_probe(dev, ret, "Failed to add action or reset\n");
> You will never see that message, though arguably that's an implementation detail.
>
> The only error that devm_add_action_or_reset() returns is -ENOMEM;
> dev_err_probe() doesn't print on -ENOMEM because enough screaming occurs at
> other layers.
>
> I normally don't bother commenting on this one if it's introduced as one of
> many messages, but here you are adding just this one so I have commented.
Thank you so much. I highly appreciate all the time you spend on
reviewing and giving
feedback!! For me there is no hurry with this driver. I'd prefer to go
a bit more picky
feedback, according to your patience. Hopefully, I will learn for
future patches. Already
when I take a look on the way from v1 of just this driver. Awesome!
>
> >
> > return devm_iio_device_register(dev, indio_dev);
> > }
>
