On Mon, 1 Apr 2024 17:44:22 +0200 Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: > 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. Good point. I'd forgotten the call was in an optional callback. > > 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 > > 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 = { > > >