Received-SPF: pass (google.com: domain of linux-kernel+bounces-126879-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 1 Apr 2024 17:55:52 +0100
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Lothar Rubusch <l.rubusch@gmail.com>
CC: Jonathan Cameron <jic23@kernel.org>, <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 v6 7/7] iio: accel: adxl345: Add spi-3wire option
Message-ID: <20240401175552.00003665@Huawei.com>
In-Reply-To: <CAFXKEHbM5kcSvfqHmfetzy8Gp_jOK0ZMbOjBK8USkNdxyNAT5w@mail.gmail.com>
References: <20240329004030.16153-1-l.rubusch@gmail.com>
	<20240329004030.16153-8-l.rubusch@gmail.com>
	<20240330152949.7e10ebcc@jic23-huawei>
	<CAFXKEHbM5kcSvfqHmfetzy8Gp_jOK0ZMbOjBK8USkNdxyNAT5w@mail.gmail.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, 1 Apr 2024 17:44:22 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Mar 30, 2024 at 4:30=E2=80=AFPM Jonathan Cameron <jic23@kernel.or=
g> wrote:
> >
> > On Fri, 29 Mar 2024 00:40:30 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > =20
> > > Add a setup function implementation to the spi module to enable spi-3=
wire
> > > when specified in the device-tree.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345.h     |  1 +
> > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > >  2 files changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index e859c01d4..3d5c8719d 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -31,6 +31,7 @@
> > >  #define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0)   /* Set the g ra=
nge */
> > >  #define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2)  /* Left-justified (MSB)=
 mode */
> > >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3)  /* Up to 13-bits resolu=
tion */
> > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6)  /* 3-wire SPI m=
ode */
> > >  #define ADXL345_DATA_FORMAT_SELF_TEST        BIT(7)  /* Enable a sel=
f test */
> > >
> > >  #define ADXL345_DATA_FORMAT_2G               0
> > > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl=
345_spi.c
> > > index 1c0513bd3..f145d5c1d 100644
> > > --- a/drivers/iio/accel/adxl345_spi.c
> > > +++ b/drivers/iio/accel/adxl345_spi.c
> > > @@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regm=
ap_config =3D {
> > >       .read_flag_mask =3D BIT(7) | BIT(6),
> > >  };
> > >
> > > +static int adxl345_spi_setup(struct device *dev, struct regmap *regm=
ap)
> > > +{
> > > +     struct spi_device *spi =3D container_of(dev, struct spi_device,=
 dev);
> > > +
> > > +     if (spi->mode & SPI_3WIRE)
> > > +             return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > > +                                 ADXL345_DATA_FORMAT_SPI_3WIRE); =20
> > My only remaining comment on this patch set is to add equivalent of
> >         else
> >                 return regmap_write(regmap, ADXL345_REG_DATA_FORMAT, 0);
> >
> > If the hardware had some sort of software reset, that was used,
> > this wouldn't be needed as the status of those other bits would be know=
n.
> > If we leave them alone in the non 3wire path we may in the future have
> > subtle issues because some other code left this in an odd state and
> > we clear those other bits only for 3wire mode.
> > =20
>=20
> I see your point. Thinking over it, I came to the following: Given the
> spi-3wire case, if I did a regmap_write(spi-3wire), else I did
> regmap_write(0), in the i2c case I still passed NULL as setup()
> function. So there would still be just a regmap_update() only in the
> core module. Furthermore I see three cases: spi_setup() passed w/
> 3wire, spi_setu() passed w/o 3wire or NULL passed. This means there is
> the same issue and more complexity. Hence, I will not do this. I think
> I found something else.

Good point. I'd forgotten the call was in an optional callback.

>=20
> What do you think about the following approach: If there is a
> spi-3wire set in the device-tree, I pass the setup() function, else I
> pass NULL. Then in the core module, if the setup() function is valid,
> I do a regmap_update(), else the first option will be set with

regmap_update()?

> regmap_write(). This makes up only two cases: setup() passed, or not -
> and in either case the first call will be a regmap_write(). Thus all
> bits are initialized to a defined state. I will update the patchset
> later today, that you can see.
That sounds like a good solution.

Jonathan

>=20
> Happy Easter!
> Lothar
>=20
> > Jonathan
> > =20
> > > +     return 0;
> > > +}
> > > +
> > >  static int adxl345_spi_probe(struct spi_device *spi)
> > >  {
> > >       struct regmap *regmap;
> > > @@ -33,7 +43,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
> > >       if (IS_ERR(regmap))
> > >               return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error=
 initializing regmap\n");
> > >
> > > -     return adxl345_core_probe(&spi->dev, regmap, NULL);
> > > +     return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > >  }
> > >
> > >  static const struct adxl345_chip_info adxl345_spi_info =3D { =20
> > =20
>=20