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.
>
> return devm_iio_device_register(dev, indio_dev);
> }
