| Message ID | 20240329004030.16153-8-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: Add spi-3wire feature | expand |
On Fri, 29 Mar 2024 00:40:30 +0000 Lothar Rubusch <l.rubusch@gmail.com> 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@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 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. 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 = {
On Sat, Mar 30, 2024 at 4:30 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Mar 2024 00:40:30 +0000 > Lothar Rubusch <l.rubusch@gmail.com> 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@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 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 = { >
On Mon, 1 Apr 2024 17:44:22 +0200 Lothar Rubusch <l.rubusch@gmail.com> wrote: > On Sat, Mar 30, 2024 at 4:30 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 29 Mar 2024 00:40:30 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> 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@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 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 = { > > >
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); + 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 = {
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@gmail.com> --- drivers/iio/accel/adxl345.h | 1 + drivers/iio/accel/adxl345_spi.c | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-)