Received-SPF: pass (google.com: domain of linux-kernel+bounces-125759-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 30 Mar 2024 15:18:17 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Lothar Rubusch <l.rubusch@gmail.com>
Cc: lars@metafoo.de, Michael.Hennerich@analog.com, robh+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, eraretuya@gmail.com
Subject: Re: [PATCH v5 1/7] iio: accel: adxl345: Make data_range obsolete
Message-ID: <20240330151817.25c11a5f@jic23-huawei>
In-Reply-To: <CAFXKEHaPcbRMVJTWW0Atg37jqb97JBdPoSmm3gAZEO1Q=SZm9w@mail.gmail.com>
References: <20240327220320.15509-1-l.rubusch@gmail.com>
	<20240327220320.15509-2-l.rubusch@gmail.com>
	<20240328133720.7dfd46b0@jic23-huawei>
	<CAFXKEHaPcbRMVJTWW0Atg37jqb97JBdPoSmm3gAZEO1Q=SZm9w@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, 29 Mar 2024 01:03:29 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Thu, Mar 28, 2024 at 2:37=E2=80=AFPM Jonathan Cameron <jic23@kernel.or=
g> wrote:
> >
> > On Wed, 27 Mar 2024 22:03:14 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > =20
> > > Replace write() data_format by regmap_update_bits(), because bus spec=
ific
> > > 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@gmail.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/adx=
l345_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 rang=
e */
> > > +#define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2) /* Left-justified (MSB) =
mode */
> > >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolut=
ion */
> > > +#define ADXL345_DATA_FORMAT_SELF_TEST        BIT(7) /* Enable a self=
 test */
> > > +#define ADXL345_DATA_FORMAT_MSK              (ADXL345_DATA_FORMAT_RA=
NGE | \
> > > +                                      ADXL345_DATA_FORMAT_JUSTIFY | =
 \
> > > +                                      ADXL345_DATA_FORMAT_FULL_RES |=
 \
> > > +                                      ADXL345_DATA_FORMAT_SELF_TEST)=
 =20
> > 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 cle=
ar
> > only a subset of fields are being cleared.
> > =20
> 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?
>=20
The new code looks good to me.  A local variable doesn't carry the
same implication of global applicability as the define did.
Thanks,

J
> > Jonathan
> > =20
> > > +
> > >  #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, stru=
ct regmap *regmap)
> > >
> > >       data =3D iio_priv(indio_dev);
> > >       data->regmap =3D regmap;
> > > -     /* Enable full-resolution mode */
> > > -     data->data_range =3D ADXL345_DATA_FORMAT_FULL_RES;
> > >       data->info =3D device_get_match_data(dev);
> > >       if (!data->info)
> > >               return -ENODEV;
> > >
> > > -     ret =3D regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> > > -                        data->data_range);
> > > -     if (ret < 0)
> > > +     /* Enable full-resolution mode */
> > > +     ret =3D 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 rang=
e\n");
> > >
> > >       indio_dev->name =3D data->info->name; =20
> > =20