Received-SPF: pass (google.com: domain of linux-kernel+bounces-123911-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240327220320.15509-1-l.rubusch@gmail.com> <20240327220320.15509-8-l.rubusch@gmail.com>
 <20240328133927.7e49f3bf@jic23-huawei>
In-Reply-To: <20240328133927.7e49f3bf@jic23-huawei>
From: Lothar Rubusch <l.rubusch@gmail.com>
Date: Fri, 29 Mar 2024 01:33:01 +0100
Message-ID: <CAFXKEHZva5AE=sinx_i0Gec2FbB4ZfyEu8mWg52omzGBvr5uXw@mail.gmail.com>
Subject: Re: [PATCH v5 7/7] iio: accel: adxl345: Add spi-3wire option
To: Jonathan Cameron <jic23@kernel.org>
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 28, 2024 at 2:39=E2=80=AFPM Jonathan Cameron <jic23@kernel.org>=
 wrote:
>
> On Wed, 27 Mar 2024 22:03:20 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a setup function implementation to the spi module to enable spi-3wi=
re
> > as option when specified in the device-tree.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h     |  2 ++
> >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 4ea9341d4..e6bc3591c 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -30,6 +30,8 @@
> >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> >  #define ADXL345_POWER_CTL_STANDBY    0x00
> >
> > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode=
 */
> > +
> >  #define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0) /* Set the g range =
*/
> >  #define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2) /* Left-justified (MSB) mo=
de */
> >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolutio=
n */
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl34=
5_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_regmap=
_config =3D {
> >       .read_flag_mask =3D BIT(7) | BIT(6),
> >  };
> >
> > +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap=
)
> > +{
> > +     struct spi_device *spi =3D container_of(dev, struct spi_device, d=
ev);
> > +
> > +     if (spi->mode & SPI_3WIRE)
> > +             return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > +                                 ADXL345_DATA_FORMAT_SPI_3WIRE);
> Your earlier patch carefully (I think) left one or two fields alone, then
> this write just comes in and changes them. In particular INT_INVERT.
>
Why do you refer here to INT_INVERT? In this code above I try to set
SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
it happens in adxl345_spi.c. Passing this function to the bus
independent adxl345_core.c file allows to configure the bus first.
Therefore, I'm using the update function in core masking out the SPI
filag.

My reason why I try to keep INT_INVERT out is different. There is
another driver for the same part in the kernel:
/drivers/input/misc/adxl34x.c - This is a input driver, using the
interrupts of the adxl345 for the input device implementation. I
assumed, that in the iio implementation there won't be interrupt
handling for the adx345, since it is not an input device. Does this
make sense?

> If it doesn't makes sense to write it there, either write that bit
> every time here, or leave it alone every time.  Not decide on whether
> to write the bit based on SPI_3WIRE or not.  As far as I know they
> are unconnected features.
>
I think I did not understand. Could you please specify a bit more?
When spi-3wire is configured in the DT it has to be parsed and handled
in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
SPI_3WIRE there. I don't want to place SPI specific code in the core
file.

> > +     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 i=
nitializing 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 {
>