linux-kernel - Re: [PATCH v5 1/7] iio: accel: adxl345: Make data

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: <CAFXKEHaPcbRMVJTWW0Atg37jqb97JBdPoSmm3gAZEO1Q=SZm9w@mail.gmail.com>
Date: Fri, 29 Mar 2024 01:03:29 +0100
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, eraretuya@...il.com
Subject: Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete

On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Wed, 27 Mar 2024 22:03:14 +0000
> Lothar Rubusch <l.rubusch@...il.com> wrote:
>
> > Replace write() data_format by regmap_update_bits(), because bus specific
> > pre-configuration may have happened before on the same register. For
> > further updates to the data_format register then bus pre-configuration
> > needs to be masked out.
> >
> > Remove the data_range field from the struct adxl345_data, because it is
> > not used anymore.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> > ---
> >  drivers/iio/accel/adxl345_core.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 8bd30a23e..35df5e372 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -37,7 +37,15 @@
> >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> >  #define ADXL345_POWER_CTL_STANDBY    0x00
> >
> > +#define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0) /* Set the g range */
> > +#define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2) /* Left-justified (MSB) mode */
> >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > +#define ADXL345_DATA_FORMAT_SELF_TEST        BIT(7) /* Enable a self test */
> > +#define ADXL345_DATA_FORMAT_MSK              (ADXL345_DATA_FORMAT_RANGE | \
> > +                                      ADXL345_DATA_FORMAT_JUSTIFY |  \
> > +                                      ADXL345_DATA_FORMAT_FULL_RES | \
> > +                                      ADXL345_DATA_FORMAT_SELF_TEST)
> This needs renaming.  It's not a mask of everything in the register, or
> even just of everything related to format.
>
> Actually I'd just not have this definition.  Use the build up value
> from all the submasks at the call site.  Then we are just making it clear
> only a subset of fields are being cleared.
>
I understand this solution was not very useful. I'm not sure, I
understood you correctly. Please have a look into v6 if this matches
your comment.
Now, I remove the mask, instead I use a local variable in core's
probe() for the update mask. I added a comment. Nevertheless, I keep
the used flags for FORMAT_DATA. Does this go into the direction of
using the build up value from the submasks at the call site?

> Jonathan
>
> > +
> >  #define ADXL345_DATA_FORMAT_2G               0
> >  #define ADXL345_DATA_FORMAT_4G               1
> >  #define ADXL345_DATA_FORMAT_8G               2
> > @@ -48,7 +56,6 @@
> >  struct adxl345_data {
> >       const struct adxl345_chip_info *info;
> >       struct regmap *regmap;
> > -     u8 data_range;
> >  };
> >
> >  #define ADXL345_CHANNEL(index, axis) {                                       \
> > @@ -218,15 +225,14 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
> >
> >       data = iio_priv(indio_dev);
> >       data->regmap = regmap;
> > -     /* Enable full-resolution mode */
> > -     data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> >       data->info = device_get_match_data(dev);
> >       if (!data->info)
> >               return -ENODEV;
> >
> > -     ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> > -                        data->data_range);
> > -     if (ret < 0)
> > +     /* Enable full-resolution mode */
> > +     ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> > +                              ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
> > +     if (ret)
> >               return dev_err_probe(dev, ret, "Failed to set data range\n");
> >
> >       indio_dev->name = data->info->name;
>