diff mbox series

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

Message ID 20241213211909.40896-2-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

Commit Message

Lothar Rubusch Dec. 13, 2024, 9:19 p.m. UTC 
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 | 42 +++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 12 deletions(-)

Comments

Christophe JAILLET Dec. 14, 2024, 11:33 a.m. UTC | #1 
Le 13/12/2024 à 22:19, Lothar Rubusch a écrit :
> 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.

...

> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> +{
> +	unsigned int val = 0;

Nitpick: useless init

> +
> +	val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;

Nitpick: useless () around en.

> +	return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> +}

...
Jonathan Cameron Dec. 14, 2024, 12:02 p.m. UTC | #2 
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.

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;
>
Lothar Rubusch Dec. 15, 2024, 9:32 a.m. UTC | #3 
On Sat, Dec 14, 2024 at 12:33 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 13/12/2024 à 22:19, Lothar Rubusch a écrit :
> > 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.
>
> ...
>
> > +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > +{
> > +     unsigned int val = 0;
>
> Nitpick: useless init
>
> > +
> > +     val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
>
> Nitpick: useless () around en.

Thank you for pointing out. I agree. This is better just in one line,
initialization directly and no parens. Anyway, since I already can see
the patch on the iio branch, I'll probably better leave it as is for
now (?).

Question: Since I'm currently about to build up tooling for linters
and static checkers. I'm doing checkpatch. I'm running Dan's smatch,
now. I'm correcting indention/spaces/tabs by some emacs settings.
DT/bindings I was doing wrong, and just figured out why after
submitting the last patch (...).

What is the best approach to find such kind of nitpick issues automatically?

Best,
L

>
> > +     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > +}
>
> ...
Lothar Rubusch Dec. 15, 2024, 9:41 a.m. UTC | #4 
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"?

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;
> >
>
Christophe JAILLET Dec. 15, 2024, 10:21 a.m. UTC | #5 
Le 15/12/2024 à 10:32, Lothar Rubusch a écrit :
> On Sat, Dec 14, 2024 at 12:33 PM Christophe JAILLET
> <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org> wrote:
>>
>> Le 13/12/2024 à 22:19, Lothar Rubusch a écrit :
>>> 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.
>>
>> ...
>>
>>> +static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
>>> +{
>>> +     unsigned int val = 0;
>>
>> Nitpick: useless init
>>
>>> +
>>> +     val = (en) ? ADXL345_POWER_CTL_MEASURE : ADXL345_POWER_CTL_STANDBY;
>>
>> Nitpick: useless () around en.
> 
> Thank you for pointing out. I agree. This is better just in one line,
> initialization directly and no parens. Anyway, since I already can see
> the patch on the iio branch, I'll probably better leave it as is for
> now (?).

It is just a nitpick, it can stay as-is forever.

> 
> Question: Since I'm currently about to build up tooling for linters
> and static checkers. I'm doing checkpatch. I'm running Dan's smatch,
> now. I'm correcting indention/spaces/tabs by some emacs settings.
> DT/bindings I was doing wrong, and just figured out why after
> submitting the last patch (...).
> 
> What is the best approach to find such kind of nitpick issues automatically?

I don't know any.
I only uses ./scripts/checkpatch.pl --strict

I guess that some coccinelle scripts could also spot some things, (the 
unneeded init in this case), but I don't really think that the effort 
would worth it.

Maybe using -Wall and or -Wextra gcc gcc would also trigger additional 
warning that could look relevant?


Adding Dan if he has more ideas.

CJ

> 
> Best,
> L
> 
>>
>>> +     return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
>>> +}
>>
>> ...
> 
>
Jonathan Cameron Dec. 15, 2024, 2:10 p.m. UTC | #6 
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;
> > >  
> >
diff mbox series

Patch

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;