| Message ID | 20241205171343.308963-5-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand |
On Thu, 5 Dec 2024 17:13:37 +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> 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); > }
On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 5 Dec 2024 17:13:37 +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> > 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); > > } >
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"); return devm_iio_device_register(dev, indio_dev); }
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> --- drivers/iio/accel/adxl345_core.c | 44 ++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 13 deletions(-)