[v3,3/3] iio: adc: new driver to support Linear technology's ltc2496
diff mbox series

Message ID 20191121210007.25646-4-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • [v3,1/3] iio: adc: ltc2496: provide device tree binding document
Related show

Commit Message

Uwe Kleine-König Nov. 21, 2019, 9 p.m. UTC
This chip is similar to the LTC2497 ADC, it just uses SPI instead of I2C
and so has a slightly different protocol. Only the actual hardware
access is different. The spi protocol is different enough to not be able
to map the differences via a regmap.

Also generalize the entry in MAINTAINER to cover the newly introduced
file.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 MAINTAINERS               |   2 +-
 drivers/iio/adc/Kconfig   |  10 ++++
 drivers/iio/adc/Makefile  |   1 +
 drivers/iio/adc/ltc2496.c | 108 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/adc/ltc2496.c

Comments

Jonathan Cameron Nov. 23, 2019, 5:12 p.m. UTC | #1
On Thu, 21 Nov 2019 22:00:07 +0100
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> This chip is similar to the LTC2497 ADC, it just uses SPI instead of I2C
> and so has a slightly different protocol. Only the actual hardware
> access is different. The spi protocol is different enough to not be able
> to map the differences via a regmap.
> 
> Also generalize the entry in MAINTAINER to cover the newly introduced
> file.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
looks good with the exception of the now overly protected DMA buffers.

See inline.  As that's all I'm seeing that needs fixing I'll just
fix this up whilst applying.

I'd like the series to sit a little longer on the list though to give
devicetree maintainers time to look at the bindings.

Thanks,

Jonathan

> ---
>  MAINTAINERS               |   2 +-
>  drivers/iio/adc/Kconfig   |  10 ++++
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/ltc2496.c | 108 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 120 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iio/adc/ltc2496.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eb19fad370d7..8b1211038617 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1029,7 +1029,7 @@ S:	Supported
>  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523
>  F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350
>  F:	drivers/iio/*/ad*
> -F:	drivers/iio/adc/ltc2497*
> +F:	drivers/iio/adc/ltc249*
>  X:	drivers/iio/*/adjd*
>  F:	drivers/staging/iio/*/ad*
>  
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f0af3a42f53c..deb86f6039b3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -492,6 +492,16 @@ config LTC2485
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2485.
>  
> +config LTC2496
> +	tristate "Linear Technology LTC2496 ADC driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Linear Technology LTC2496
> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ltc2496.
> +
>  config LTC2497
>  	tristate "Linear Technology LTC2497 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ee0c8dcfb501..c3dc2a12766a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -47,6 +47,7 @@ obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>  obj-$(CONFIG_LTC2471) += ltc2471.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
> +obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
>  obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
> diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
> new file mode 100644
> index 000000000000..a7659c8f9cc9
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2496.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ltc2496.c - Driver for Analog Devices/Linear Technology LTC2496 ADC
> + *
> + * Based on ltc2497.c which has
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2496fc.pdf
> + */
> +
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +
> +#include "ltc2497.h"
> +
> +struct ltc2496_driverdata {
> +	/* this must be the first member */
> +	struct ltc2497core_driverdata common_ddata;
> +	struct spi_device *spi;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	unsigned char rxbuf[3] ____cacheline_aligned;
> +	unsigned char txbuf[3] ____cacheline_aligned;
Ah.  I've not explained this clearly enough.  Upshot is you only need
to ensure that the buffers used for dma are not shared with any other
usage.  the __cacheline_aligned marker pads the structure to ensure
the element so marked is at the start of a new cacheline.  This means
there is no sharing with non DMA related elements which may be accidentally
reset when the DMA transfer ends.

Imagine we had:
struct bob {
	int a; //used for all sorts of fun things not related to dma and not
       	       //protected from happening concurrently with dma.
	unsigned char rx_buf[3];
	unsigned char tx_buf[3]
};

The buffers are used for DMA.  The DMA engine takes a copy of the cacheline
to start doing it's magic.

Along comes other activity and writes to 'a'.

DMA completes, then engine pushes the cacheline back to the memory including
writing back what it had as a copy of a.  Thus the update to 'a' is lost.

Now the guarantee we make use of is that DMA engines are not allowed to
copy cachelines that do not contain the buffers they are using (all sorts
of things would break if they were).

However, there is no need to separate rx_buf and tx_buf as they are being
used by the same DMA engine and nothing else is going to update them whilst
they are in use.

A fun side note is that i2c never uses buffers provided to it for DMA
transfers except when using the specific dmasafe functions (which isn't
happening here).

Thanks,

Jonathan

> +};
> +
> +static int ltc2496_result_and_measure(struct ltc2497core_driverdata *ddata,
> +				      u8 address, int *val)
> +{
> +	struct ltc2496_driverdata *st =
> +		container_of(ddata, struct ltc2496_driverdata, common_ddata);
> +	struct spi_transfer t = {
> +		.tx_buf = st->txbuf,
> +		.rx_buf = st->rxbuf,
> +		.len = sizeof(st->txbuf),
> +	};
> +	int ret;
> +
> +	st->txbuf[0] = LTC2497_ENABLE | address;
> +
> +	ret = spi_sync_transfer(st->spi, &t, 1);
> +	if (ret < 0)  {
> +		dev_err(&st->spi->dev, "spi_sync_transfer failed: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	if (val)
> +		*val = ((st->rxbuf[0] & 0x3f) << 12 |
> +			st->rxbuf[1] << 4 | st->rxbuf[2] >> 4) -
> +			(1 << 17);
> +
> +	return 0;
> +}
> +
> +static int ltc2496_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc2496_driverdata *st;
> +	struct device *dev = &spi->dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);
> +	st->spi = spi;
> +	st->common_ddata.result_and_measure = ltc2496_result_and_measure;
> +
> +	return ltc2497core_probe(dev, indio_dev);
> +}
> +
> +static int ltc2496_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> +	ltc2497core_remove(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ltc2496_of_match[] = {
> +	{ .compatible = "lltc,ltc2496", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ltc2496_of_match);
> +
> +static struct spi_driver ltc2496_driver = {
> +	.driver = {
> +		.name = "ltc2496",
> +		.of_match_table = of_match_ptr(ltc2496_of_match),
> +	},
> +	.probe = ltc2496_probe,
> +	.remove = ltc2496_remove,
> +};
> +module_spi_driver(ltc2496_driver);
> +
> +MODULE_AUTHOR("Uwe Kleine-König <u.kleine-könig@pengutronix.de>");
> +MODULE_DESCRIPTION("Linear Technology LTC2496 ADC driver");
> +MODULE_LICENSE("GPL v2");
Uwe Kleine-König Nov. 24, 2019, 7:11 p.m. UTC | #2
On Sat, Nov 23, 2019 at 05:12:04PM +0000, Jonathan Cameron wrote:
> On Thu, 21 Nov 2019 22:00:07 +0100
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > This chip is similar to the LTC2497 ADC, it just uses SPI instead of I2C
> > and so has a slightly different protocol. Only the actual hardware
> > access is different. The spi protocol is different enough to not be able
> > to map the differences via a regmap.
> > 
> > Also generalize the entry in MAINTAINER to cover the newly introduced
> > file.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> looks good with the exception of the now overly protected DMA buffers.
> 
> See inline.  As that's all I'm seeing that needs fixing I'll just
> fix this up whilst applying.
> 
> I'd like the series to sit a little longer on the list though to give
> devicetree maintainers time to look at the bindings.

ok.

> > +struct ltc2496_driverdata {
> > +	/* this must be the first member */
> > +	struct ltc2497core_driverdata common_ddata;
> > +	struct spi_device *spi;
> > +
> > +	/*
> > +	 * DMA (thus cache coherency maintenance) requires the
> > +	 * transfer buffers to live in their own cache lines.
> > +	 */
> > +	unsigned char rxbuf[3] ____cacheline_aligned;
> > +	unsigned char txbuf[3] ____cacheline_aligned;
> Ah.  I've not explained this clearly enough.  Upshot is you only need
> to ensure that the buffers used for dma are not shared with any other
> usage.  the __cacheline_aligned marker pads the structure to ensure
> the element so marked is at the start of a new cacheline.  This means
> there is no sharing with non DMA related elements which may be accidentally
> reset when the DMA transfer ends.
> 
> Imagine we had:
> struct bob {
> 	int a; //used for all sorts of fun things not related to dma and not
>        	       //protected from happening concurrently with dma.
> 	unsigned char rx_buf[3];
> 	unsigned char tx_buf[3]
> };
> 
> The buffers are used for DMA.  The DMA engine takes a copy of the cacheline
> to start doing it's magic.
> 
> Along comes other activity and writes to 'a'.
> 
> DMA completes, then engine pushes the cacheline back to the memory including
> writing back what it had as a copy of a.  Thus the update to 'a' is lost.
> 
> Now the guarantee we make use of is that DMA engines are not allowed to
> copy cachelines that do not contain the buffers they are using (all sorts
> of things would break if they were).
> 
> However, there is no need to separate rx_buf and tx_buf as they are being
> used by the same DMA engine and nothing else is going to update them whilst
> they are in use.

Yeah, I thought about that when adding the second annotation but then
forgot to mention that in my cover letter.

So I assume you will just drop the 2nd ____cacheline_aligned? That's
fine for me; thanks.

Best regards
Uwe
Jonathan Cameron Dec. 1, 2019, 11 a.m. UTC | #3
On Sun, 24 Nov 2019 20:11:01 +0100
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> On Sat, Nov 23, 2019 at 05:12:04PM +0000, Jonathan Cameron wrote:
> > On Thu, 21 Nov 2019 22:00:07 +0100
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> >   
> > > This chip is similar to the LTC2497 ADC, it just uses SPI instead of I2C
> > > and so has a slightly different protocol. Only the actual hardware
> > > access is different. The spi protocol is different enough to not be able
> > > to map the differences via a regmap.
> > > 
> > > Also generalize the entry in MAINTAINER to cover the newly introduced
> > > file.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>  
> > looks good with the exception of the now overly protected DMA buffers.
> > 
> > See inline.  As that's all I'm seeing that needs fixing I'll just
> > fix this up whilst applying.
> > 
> > I'd like the series to sit a little longer on the list though to give
> > devicetree maintainers time to look at the bindings.  
> 
> ok.
> 
> > > +struct ltc2496_driverdata {
> > > +	/* this must be the first member */
> > > +	struct ltc2497core_driverdata common_ddata;
> > > +	struct spi_device *spi;
> > > +
> > > +	/*
> > > +	 * DMA (thus cache coherency maintenance) requires the
> > > +	 * transfer buffers to live in their own cache lines.
> > > +	 */
> > > +	unsigned char rxbuf[3] ____cacheline_aligned;
> > > +	unsigned char txbuf[3] ____cacheline_aligned;  
> > Ah.  I've not explained this clearly enough.  Upshot is you only need
> > to ensure that the buffers used for dma are not shared with any other
> > usage.  the __cacheline_aligned marker pads the structure to ensure
> > the element so marked is at the start of a new cacheline.  This means
> > there is no sharing with non DMA related elements which may be accidentally
> > reset when the DMA transfer ends.
> > 
> > Imagine we had:
> > struct bob {
> > 	int a; //used for all sorts of fun things not related to dma and not
> >        	       //protected from happening concurrently with dma.
> > 	unsigned char rx_buf[3];
> > 	unsigned char tx_buf[3]
> > };
> > 
> > The buffers are used for DMA.  The DMA engine takes a copy of the cacheline
> > to start doing it's magic.
> > 
> > Along comes other activity and writes to 'a'.
> > 
> > DMA completes, then engine pushes the cacheline back to the memory including
> > writing back what it had as a copy of a.  Thus the update to 'a' is lost.
> > 
> > Now the guarantee we make use of is that DMA engines are not allowed to
> > copy cachelines that do not contain the buffers they are using (all sorts
> > of things would break if they were).
> > 
> > However, there is no need to separate rx_buf and tx_buf as they are being
> > used by the same DMA engine and nothing else is going to update them whilst
> > they are in use.  
> 
> Yeah, I thought about that when adding the second annotation but then
> forgot to mention that in my cover letter.
> 
> So I assume you will just drop the 2nd ____cacheline_aligned? That's
> fine for me; thanks.
Yup.

Jonathan

> 
> Best regards
> Uwe
>

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index eb19fad370d7..8b1211038617 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1029,7 +1029,7 @@  S:	Supported
 F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-ad9523
 F:	Documentation/ABI/testing/sysfs-bus-iio-frequency-adf4350
 F:	drivers/iio/*/ad*
-F:	drivers/iio/adc/ltc2497*
+F:	drivers/iio/adc/ltc249*
 X:	drivers/iio/*/adjd*
 F:	drivers/staging/iio/*/ad*
 
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f0af3a42f53c..deb86f6039b3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -492,6 +492,16 @@  config LTC2485
 	  To compile this driver as a module, choose M here: the module will be
 	  called ltc2485.
 
+config LTC2496
+	tristate "Linear Technology LTC2496 ADC driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Linear Technology LTC2496
+	  16-Bit 8-/16-Channel Delta Sigma ADC.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ltc2496.
+
 config LTC2497
 	tristate "Linear Technology LTC2497 ADC driver"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ee0c8dcfb501..c3dc2a12766a 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -47,6 +47,7 @@  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
 obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
 obj-$(CONFIG_LTC2471) += ltc2471.o
 obj-$(CONFIG_LTC2485) += ltc2485.o
+obj-$(CONFIG_LTC2496) += ltc2496.o ltc2497-core.o
 obj-$(CONFIG_LTC2497) += ltc2497.o ltc2497-core.o
 obj-$(CONFIG_MAX1027) += max1027.o
 obj-$(CONFIG_MAX11100) += max11100.o
diff --git a/drivers/iio/adc/ltc2496.c b/drivers/iio/adc/ltc2496.c
new file mode 100644
index 000000000000..a7659c8f9cc9
--- /dev/null
+++ b/drivers/iio/adc/ltc2496.c
@@ -0,0 +1,108 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ltc2496.c - Driver for Analog Devices/Linear Technology LTC2496 ADC
+ *
+ * Based on ltc2497.c which has
+ * Copyright (C) 2017 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/2496fc.pdf
+ */
+
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/driver.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include "ltc2497.h"
+
+struct ltc2496_driverdata {
+	/* this must be the first member */
+	struct ltc2497core_driverdata common_ddata;
+	struct spi_device *spi;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	unsigned char rxbuf[3] ____cacheline_aligned;
+	unsigned char txbuf[3] ____cacheline_aligned;
+};
+
+static int ltc2496_result_and_measure(struct ltc2497core_driverdata *ddata,
+				      u8 address, int *val)
+{
+	struct ltc2496_driverdata *st =
+		container_of(ddata, struct ltc2496_driverdata, common_ddata);
+	struct spi_transfer t = {
+		.tx_buf = st->txbuf,
+		.rx_buf = st->rxbuf,
+		.len = sizeof(st->txbuf),
+	};
+	int ret;
+
+	st->txbuf[0] = LTC2497_ENABLE | address;
+
+	ret = spi_sync_transfer(st->spi, &t, 1);
+	if (ret < 0)  {
+		dev_err(&st->spi->dev, "spi_sync_transfer failed: %pe\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	if (val)
+		*val = ((st->rxbuf[0] & 0x3f) << 12 |
+			st->rxbuf[1] << 4 | st->rxbuf[2] >> 4) -
+			(1 << 17);
+
+	return 0;
+}
+
+static int ltc2496_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct ltc2496_driverdata *st;
+	struct device *dev = &spi->dev;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	spi_set_drvdata(spi, indio_dev);
+	st->spi = spi;
+	st->common_ddata.result_and_measure = ltc2496_result_and_measure;
+
+	return ltc2497core_probe(dev, indio_dev);
+}
+
+static int ltc2496_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	ltc2497core_remove(indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id ltc2496_of_match[] = {
+	{ .compatible = "lltc,ltc2496", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ltc2496_of_match);
+
+static struct spi_driver ltc2496_driver = {
+	.driver = {
+		.name = "ltc2496",
+		.of_match_table = of_match_ptr(ltc2496_of_match),
+	},
+	.probe = ltc2496_probe,
+	.remove = ltc2496_remove,
+};
+module_spi_driver(ltc2496_driver);
+
+MODULE_AUTHOR("Uwe Kleine-König <u.kleine-könig@pengutronix.de>");
+MODULE_DESCRIPTION("Linear Technology LTC2496 ADC driver");
+MODULE_LICENSE("GPL v2");