Re: [PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option

[Lothar Rubusch]

On Sat, Mar 30, 2024 at 4:30 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri, 29 Mar 2024 00:40:30 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
>
> > Add a setup function implementation to the spi module to enable spi-3wire
> > when specified in the device-tree.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > ---
> >  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 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_SPI_3WIRE        BIT(6)  /* 3-wire SPI mode */
> >  #define ADXL345_DATA_FORMAT_SELF_TEST        BIT(7)  /* Enable a self test */
> >
> >  #define ADXL345_DATA_FORMAT_2G               0
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_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 = {
> >       .read_flag_mask = BIT(7) | BIT(6),
> >  };
> >
> > +static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
> > +{
> > +     struct spi_device *spi = 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);
> 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 known.
> 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.
>

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.

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_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.

Happy Easter!
Lothar

> Jonathan
>
> > +     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 = {
>