linux-kernel - Re: [PATCH v5 04/10] 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: <CAFXKEHY4LWm9iJnZ---Zixf9GT6u_Ebk-=sDsrqTjGSR4KCM2g@mail.gmail.com>
Date: Tue, 10 Dec 2024 18:49:45 +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 v5 04/10] iio: accel: adxl345: add function to switch
 measuring mode

On Sun, Dec 8, 2024 at 2:42 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Thu,  5 Dec 2024 17:13:37 +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>
> 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);
> >  }
>