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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFXKEHamiip3fVp1HNX4DZSzNnc7bMOQNg1RKsY45NbFymzr7g@mail.gmail.com>
Date: Sun, 15 Dec 2024 10:41:12 +0100
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v7 1/7] iio: accel: adxl345: add function to switch
 measuring mode

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