[RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Matt Ranostay]

LMP91000EVM evaluation board has LMP91000 potentiostat along with an
16-bit ADC for chemical sensoring applications.

 * add support for the TI LMP91000 potentiostat
 * add support for ADC141S626 and ADC161S626 ADC chips

Matt Ranostay (2):
  iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
  iio: potentiostat: add LMP91000 support

 .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
 .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/adc/Kconfig                            |  12 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
 drivers/iio/potentiostat/Kconfig                   |  21 ++
 drivers/iio/potentiostat/Makefile                  |   6 +
 drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
 10 files changed, 622 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
 create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
 create mode 100644 drivers/iio/adc/ti-adc1x1s.c
 create mode 100644 drivers/iio/potentiostat/Kconfig
 create mode 100644 drivers/iio/potentiostat/Makefile
 create mode 100644 drivers/iio/potentiostat/lmp91000.c

-- 
2.7.4

[RFC 1/2] iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs

[Matt Ranostay]

Add support for Texas Instruments ADC141S626, and ADC161S626 chips.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
 drivers/iio/adc/Kconfig                            |  12 ++
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ti-adc1x1s.c                       | 233 +++++++++++++++++++++
 4 files changed, 262 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
 create mode 100644 drivers/iio/adc/ti-adc1x1s.c

diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
new file mode 100644
index 000000000000..9cf7db434aee
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
@@ -0,0 +1,16 @@
+* Texas Instruments' ADC141S626, and ADC161S626 chip
+
+Required properties:
+ - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
+ - reg: spi chip select number for the device
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+		Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+adc@0 {
+	compatible = "ti,adc161s626";
+	reg = <0>;
+	spi-max-frequency = <4300000>;
+};
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 25378c5882e2..cb5e4ae3279d 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -414,6 +414,18 @@ config TI_ADC128S052
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc128s052.
 
+config TI_ADC1X1S
+	tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  If you say yes here you get support for Texas Instruments ADC141S626,
+	  and ADC161S626 chips.
+
+	  This driver can also be build as a module. If so, the module will be
+	  called ti-adc1x1s.
+
 config TI_ADS1015
 	tristate "Texas Instruments ADS1015 ADC"
 	depends on I2C && !SENSORS_ADS1015
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 38638d46f972..855ff407fd0d 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
+obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
 obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
new file mode 100644
index 000000000000..889399968694
--- /dev/null
+++ b/drivers/iio/adc/ti-adc1x1s.c
@@ -0,0 +1,233 @@
+/*
+ * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
+ *
+ * ADC Devices Supported:
+ *  adc141s626 - 14-bit ADC
+ *  adc161s626 - 16-bit ADC
+ *
+ * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define TI_ADC_DRV_NAME	"adc1x1s"
+
+enum {
+	TI_ADC141S626,
+	TI_ADC161S626,
+};
+
+static const struct iio_chan_spec ti_adc141s626_channels[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 14,
+			.storagebits = 16,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct iio_chan_spec ti_adc161s626_channels[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 24,
+			.shift = 6,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+struct ti_adc_data {
+	struct iio_dev *indio_dev;
+	struct spi_device *spi;
+	int read_size;
+
+	u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */
+};
+
+static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ti_adc_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = spi_read(data->spi, &data->buffer, data->read_size);
+	if (!ret)
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+						   iio_get_time_ns());
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int ti_adc_read_measurement(struct ti_adc_data *data,
+				   struct iio_chan_spec const *chan, int *val)
+{
+	__be32 buf;
+	int ret;
+
+	ret = spi_read(data->spi, (void *) &buf, data->read_size);
+	if (ret)
+		return ret;
+
+	switch (data->read_size) {
+	case 2:
+		*val = be16_to_cpu(buf);
+		break;
+	case 3:
+		*val = be32_to_cpu(buf) >> 8;
+		break;
+	}
+
+	*val = sign_extend32(*val >> chan->scan_type.shift,
+			     chan->scan_type.realbits - 1);
+
+	return 0;
+}
+
+static int ti_adc_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct ti_adc_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (iio_device_claim_direct_mode(indio_dev))
+		return -EBUSY;
+
+	ret = ti_adc_read_measurement(data, chan, val);
+	if (!ret)
+		ret = IIO_VAL_INT;
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
+}
+
+static const struct iio_info ti_adc_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = ti_adc_read_raw,
+};
+
+static int ti_adc_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct ti_adc_data *data;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->info = &ti_adc_info;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->name = TI_ADC_DRV_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	spi_set_drvdata(spi, indio_dev);
+
+	data = iio_priv(indio_dev);
+	data->spi = spi;
+
+	switch (spi_get_device_id(spi)->driver_data) {
+	case TI_ADC141S626:
+		indio_dev->channels = ti_adc141s626_channels;
+		indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
+		data->read_size = 2;
+		break;
+	case TI_ADC161S626:
+		indio_dev->channels = ti_adc161s626_channels;
+		indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
+		data->read_size = 3;
+		break;
+	}
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 ti_adc_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_unreg_buffer;
+
+	return 0;
+
+error_unreg_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return ret;
+}
+
+static int ti_adc_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id ti_adc_dt_ids[] = {
+	{ .compatible = "ti,adc141s626", },
+	{ .compatible = "ti,adc161s626", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
+
+static const struct spi_device_id ti_adc_id[] = {
+	{"adc141s626", TI_ADC141S626},
+	{"adc161s626", TI_ADC161S626},
+	{},
+};
+MODULE_DEVICE_TABLE(spi, ti_adc_id);
+
+static struct spi_driver ti_adc_driver = {
+	.driver = {
+		.name	= TI_ADC_DRV_NAME,
+		.of_match_table = of_match_ptr(ti_adc_dt_ids),
+	},
+	.probe		= ti_adc_probe,
+	.remove		= ti_adc_remove,
+	.id_table	= ti_adc_id,
+};
+module_spi_driver(ti_adc_driver);
+
+MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
+MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [RFC 1/2] iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs

[Jonathan Cameron]

On 02/07/16 23:05, Matt Ranostay wrote:
> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Few little points inline and: No wild cards in driver names!
(though I'm occasionally half asleep and let one in).

Storage bits should always be a power of 2.  (I let some
of those slip in as well in the past... oops).

Otherwise a few tiny corners where things could, I think,
be a little more readable at the cost of slightly more code.

Basically a nice little driver.

Jonathan
> ---
>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>  drivers/iio/adc/Kconfig                            |  12 ++
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ti-adc1x1s.c                       | 233 +++++++++++++++++++++
>  4 files changed, 262 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
> new file mode 100644
> index 000000000000..9cf7db434aee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
Arguably could go in spi trivial devices binding file (unless you are going
to suplement this soon anyway in which case it may be less churn to keep it
here).
> @@ -0,0 +1,16 @@
> +* Texas Instruments' ADC141S626, and ADC161S626 chip
> +
> +Required properties:
> + - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
> + - reg: spi chip select number for the device
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> +		Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +adc@0 {
> +	compatible = "ti,adc161s626";
> +	reg = <0>;
> +	spi-max-frequency = <4300000>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5882e2..cb5e4ae3279d 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -414,6 +414,18 @@ config TI_ADC128S052
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
>  
> +config TI_ADC1X1S
> +	tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  If you say yes here you get support for Texas Instruments ADC141S626,
> +	  and ADC161S626 chips.
> +
> +	  This driver can also be build as a module. If so, the module will be
> +	  called ti-adc1x1s.
> +
>  config TI_ADS1015
>  	tristate "Texas Instruments ADS1015 ADC"
>  	depends on I2C && !SENSORS_ADS1015
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d46f972..855ff407fd0d 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> +obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
> new file mode 100644
> index 000000000000..889399968694
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc1x1s.c
> @@ -0,0 +1,233 @@
> +/*
> + * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
> + *
> + * ADC Devices Supported:
> + *  adc141s626 - 14-bit ADC
> + *  adc161s626 - 16-bit ADC
> + *
> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/err.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define TI_ADC_DRV_NAME	"adc1x1s"
I really dislike wild cards.... Just pick a part and name it after that.

> +
> +enum {
> +	TI_ADC141S626,
> +	TI_ADC161S626,
> +};
> +
> +static const struct iio_chan_spec ti_adc141s626_channels[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 14,
> +			.storagebits = 16,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static const struct iio_chan_spec ti_adc161s626_channels[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 16,
> +			.storagebits = 24,
Storage bits should be a power of 2.
> +			.shift = 6,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +struct ti_adc_data {
> +	struct iio_dev *indio_dev;
> +	struct spi_device *spi;
> +	int read_size;
> +
enforce cacheline alignment or this is going to cause possible fun on
dma using spi devices.
> +	u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */
> +};
> +
> +static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ti_adc_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = spi_read(data->spi, &data->buffer, data->read_size);
> +	if (!ret)
> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +						   iio_get_time_ns());
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ti_adc_read_measurement(struct ti_adc_data *data,
> +				   struct iio_chan_spec const *chan, int *val)
> +{
> +	__be32 buf;
> +	int ret;
> +
> +	ret = spi_read(data->spi, (void *) &buf, data->read_size);
> +	if (ret)
> +		return ret;
> +
> +	switch (data->read_size) {
> +	case 2:
> +		*val = be16_to_cpu(buf);
Inelegant as buf is clearly marked as 32 bit by it's type. I'd move the
read into the switch statement then read into the appropriate sized
local variable.  More code but easier to read (slightly)
> +		break;
> +	case 3:
> +		*val = be32_to_cpu(buf) >> 8;
> +		break;
> +	}
> +
> +	*val = sign_extend32(*val >> chan->scan_type.shift,
> +			     chan->scan_type.realbits - 1);
> +
> +	return 0;
> +}
> +
> +static int ti_adc_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ti_adc_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	if (iio_device_claim_direct_mode(indio_dev))
> +		return -EBUSY;
Should technically store and return the result of iio_device_claim_direct_mode.
It could in future return something else..
> +
> +	ret = ti_adc_read_measurement(data, chan, val);
> +	if (!ret)
> +		ret = IIO_VAL_INT;
I'd make ti_adc_read_measurement return IIO_VAL_INT if successful. Then
the slightly odd good path handling goes away.
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info ti_adc_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = ti_adc_read_raw,
> +};
> +
> +static int ti_adc_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ti_adc_data *data;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->info = &ti_adc_info;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->name = TI_ADC_DRV_NAME;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	data = iio_priv(indio_dev);
> +	data->spi = spi;
> +
> +	switch (spi_get_device_id(spi)->driver_data) {
> +	case TI_ADC141S626:
> +		indio_dev->channels = ti_adc141s626_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
> +		data->read_size = 2;
> +		break;
> +	case TI_ADC161S626:
> +		indio_dev->channels = ti_adc161s626_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
> +		data->read_size = 3;
> +		break;
> +	}
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 ti_adc_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_unreg_buffer;
> +
> +	return 0;
> +
> +error_unreg_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int ti_adc_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ti_adc_dt_ids[] = {
> +	{ .compatible = "ti,adc141s626", },
> +	{ .compatible = "ti,adc161s626", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
> +
> +static const struct spi_device_id ti_adc_id[] = {
> +	{"adc141s626", TI_ADC141S626},
> +	{"adc161s626", TI_ADC161S626},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(spi, ti_adc_id);
> +
> +static struct spi_driver ti_adc_driver = {
> +	.driver = {
> +		.name	= TI_ADC_DRV_NAME,
> +		.of_match_table = of_match_ptr(ti_adc_dt_ids),
> +	},
> +	.probe		= ti_adc_probe,
> +	.remove		= ti_adc_remove,
> +	.id_table	= ti_adc_id,
> +};
> +module_spi_driver(ti_adc_driver);
> +
> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
> +MODULE_LICENSE("GPL");
> 

Re: [RFC 1/2] iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs

[Matt Ranostay]

On Sun, Jul 3, 2016 at 6:04 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 02/07/16 23:05, Matt Ranostay wrote:
>> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> Few little points inline and: No wild cards in driver names!
> (though I'm occasionally half asleep and let one in).
>
> Storage bits should always be a power of 2.  (I let some
> of those slip in as well in the past... oops).
>
> Otherwise a few tiny corners where things could, I think,
> be a little more readable at the cost of slightly more code.
>
> Basically a nice little driver.
>
> Jonathan
>> ---
>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>  drivers/iio/adc/Kconfig                            |  12 ++
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 +++++++++++++++++++++
>>  4 files changed, 262 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>> new file mode 100644
>> index 000000000000..9cf7db434aee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
> Arguably could go in spi trivial devices binding file (unless you are going
> to suplement this soon anyway in which case it may be less churn to keep it
> here).
>> @@ -0,0 +1,16 @@
>> +* Texas Instruments' ADC141S626, and ADC161S626 chip
>> +
>> +Required properties:
>> + - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
>> + - reg: spi chip select number for the device
>> +
>> +Recommended properties:
>> + - spi-max-frequency: Definition as per
>> +             Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Example:
>> +adc@0 {
>> +     compatible = "ti,adc161s626";
>> +     reg = <0>;
>> +     spi-max-frequency = <4300000>;
>> +};
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 25378c5882e2..cb5e4ae3279d 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -414,6 +414,18 @@ config TI_ADC128S052
>>         This driver can also be built as a module. If so, the module will be
>>         called ti-adc128s052.
>>
>> +config TI_ADC1X1S
>> +     tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
>> +     depends on SPI
>> +     select IIO_BUFFER
>> +     select IIO_TRIGGERED_BUFFER
>> +     help
>> +       If you say yes here you get support for Texas Instruments ADC141S626,
>> +       and ADC161S626 chips.
>> +
>> +       This driver can also be build as a module. If so, the module will be
>> +       called ti-adc1x1s.
>> +
>>  config TI_ADS1015
>>       tristate "Texas Instruments ADS1015 ADC"
>>       depends on I2C && !SENSORS_ADS1015
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 38638d46f972..855ff407fd0d 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>> +obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>> diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
>> new file mode 100644
>> index 000000000000..889399968694
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-adc1x1s.c
>> @@ -0,0 +1,233 @@
>> +/*
>> + * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
>> + *
>> + * ADC Devices Supported:
>> + *  adc141s626 - 14-bit ADC
>> + *  adc161s626 - 16-bit ADC
>> + *
>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +#define TI_ADC_DRV_NAME      "adc1x1s"
> I really dislike wild cards.... Just pick a part and name it after that.
>
>> +
>> +enum {
>> +     TI_ADC141S626,
>> +     TI_ADC161S626,
>> +};
>> +
>> +static const struct iio_chan_spec ti_adc141s626_channels[] = {
>> +     {
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 0,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 's',
>> +                     .realbits = 14,
>> +                     .storagebits = 16,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +static const struct iio_chan_spec ti_adc161s626_channels[] = {
>> +     {
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 0,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 's',
>> +                     .realbits = 16,
>> +                     .storagebits = 24,
> Storage bits should be a power of 2.
>> +                     .shift = 6,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +struct ti_adc_data {
>> +     struct iio_dev *indio_dev;
>> +     struct spi_device *spi;
>> +     int read_size;
>> +
> enforce cacheline alignment or this is going to cause possible fun on
> dma using spi devices.
>> +     u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */

Thought only tx buffers need to cache aligned?

>> +};
>> +
>> +static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
>> +{
>> +     struct iio_poll_func *pf = private;
>> +     struct iio_dev *indio_dev = pf->indio_dev;
>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     ret = spi_read(data->spi, &data->buffer, data->read_size);
>> +     if (!ret)
>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +                                                iio_get_time_ns());
>> +
>> +     iio_trigger_notify_done(indio_dev->trig);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int ti_adc_read_measurement(struct ti_adc_data *data,
>> +                                struct iio_chan_spec const *chan, int *val)
>> +{
>> +     __be32 buf;
>> +     int ret;
>> +
>> +     ret = spi_read(data->spi, (void *) &buf, data->read_size);
>> +     if (ret)
>> +             return ret;
>> +
>> +     switch (data->read_size) {
>> +     case 2:
>> +             *val = be16_to_cpu(buf);
> Inelegant as buf is clearly marked as 32 bit by it's type. I'd move the
> read into the switch statement then read into the appropriate sized
> local variable.  More code but easier to read (slightly)

Okay.

>> +             break;
>> +     case 3:
>> +             *val = be32_to_cpu(buf) >> 8;
>> +             break;
>> +     }
>> +
>> +     *val = sign_extend32(*val >> chan->scan_type.shift,
>> +                          chan->scan_type.realbits - 1);
>> +
>> +     return 0;
>> +}
>> +
>> +static int ti_adc_read_raw(struct iio_dev *indio_dev,
>> +                        struct iio_chan_spec const *chan,
>> +                        int *val, int *val2, long mask)
>> +{
>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     if (iio_device_claim_direct_mode(indio_dev))
>> +             return -EBUSY;
> Should technically store and return the result of iio_device_claim_direct_mode.
> It could in future return something else..
>> +
>> +     ret = ti_adc_read_measurement(data, chan, val);
>> +     if (!ret)
>> +             ret = IIO_VAL_INT;
> I'd make ti_adc_read_measurement return IIO_VAL_INT if successful. Then
> the slightly odd good path handling goes away.

Okay.

>> +
>> +     iio_device_release_direct_mode(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct iio_info ti_adc_info = {
>> +     .driver_module = THIS_MODULE,
>> +     .read_raw = ti_adc_read_raw,
>> +};
>> +
>> +static int ti_adc_probe(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev;
>> +     struct ti_adc_data *data;
>> +     int ret;
>> +
>> +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     indio_dev->info = &ti_adc_info;
>> +     indio_dev->dev.parent = &spi->dev;
>> +     indio_dev->dev.of_node = spi->dev.of_node;
>> +     indio_dev->name = TI_ADC_DRV_NAME;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +     spi_set_drvdata(spi, indio_dev);
>> +
>> +     data = iio_priv(indio_dev);
>> +     data->spi = spi;
>> +
>> +     switch (spi_get_device_id(spi)->driver_data) {
>> +     case TI_ADC141S626:
>> +             indio_dev->channels = ti_adc141s626_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
>> +             data->read_size = 2;
>> +             break;
>> +     case TI_ADC161S626:
>> +             indio_dev->channels = ti_adc161s626_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
>> +             data->read_size = 3;
>> +             break;
>> +     }
>> +
>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                      ti_adc_trigger_handler, NULL);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_unreg_buffer;
>> +
>> +     return 0;
>> +
>> +error_unreg_buffer:
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static int ti_adc_remove(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +
>> +     iio_device_unregister(indio_dev);
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id ti_adc_dt_ids[] = {
>> +     { .compatible = "ti,adc141s626", },
>> +     { .compatible = "ti,adc161s626", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
>> +
>> +static const struct spi_device_id ti_adc_id[] = {
>> +     {"adc141s626", TI_ADC141S626},
>> +     {"adc161s626", TI_ADC161S626},
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(spi, ti_adc_id);
>> +
>> +static struct spi_driver ti_adc_driver = {
>> +     .driver = {
>> +             .name   = TI_ADC_DRV_NAME,
>> +             .of_match_table = of_match_ptr(ti_adc_dt_ids),
>> +     },
>> +     .probe          = ti_adc_probe,
>> +     .remove         = ti_adc_remove,
>> +     .id_table       = ti_adc_id,
>> +};
>> +module_spi_driver(ti_adc_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
>> +MODULE_LICENSE("GPL");
>>
>

Re: [RFC 1/2] iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs

[Jonathan Cameron]

On 03/07/16 22:34, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:04 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/07/16 23:05, Matt Ranostay wrote:
>>> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> Few little points inline and: No wild cards in driver names!
>> (though I'm occasionally half asleep and let one in).
>>
>> Storage bits should always be a power of 2.  (I let some
>> of those slip in as well in the past... oops).
>>
>> Otherwise a few tiny corners where things could, I think,
>> be a little more readable at the cost of slightly more code.
>>
>> Basically a nice little driver.
>>
>> Jonathan
>>> ---
>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>  drivers/iio/adc/Kconfig                            |  12 ++
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 +++++++++++++++++++++
>>>  4 files changed, 262 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>> new file mode 100644
>>> index 000000000000..9cf7db434aee
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>> Arguably could go in spi trivial devices binding file (unless you are going
>> to suplement this soon anyway in which case it may be less churn to keep it
>> here).
>>> @@ -0,0 +1,16 @@
>>> +* Texas Instruments' ADC141S626, and ADC161S626 chip
>>> +
>>> +Required properties:
>>> + - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
>>> + - reg: spi chip select number for the device
>>> +
>>> +Recommended properties:
>>> + - spi-max-frequency: Definition as per
>>> +             Documentation/devicetree/bindings/spi/spi-bus.txt
>>> +
>>> +Example:
>>> +adc@0 {
>>> +     compatible = "ti,adc161s626";
>>> +     reg = <0>;
>>> +     spi-max-frequency = <4300000>;
>>> +};
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 25378c5882e2..cb5e4ae3279d 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -414,6 +414,18 @@ config TI_ADC128S052
>>>         This driver can also be built as a module. If so, the module will be
>>>         called ti-adc128s052.
>>>
>>> +config TI_ADC1X1S
>>> +     tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
>>> +     depends on SPI
>>> +     select IIO_BUFFER
>>> +     select IIO_TRIGGERED_BUFFER
>>> +     help
>>> +       If you say yes here you get support for Texas Instruments ADC141S626,
>>> +       and ADC161S626 chips.
>>> +
>>> +       This driver can also be build as a module. If so, the module will be
>>> +       called ti-adc1x1s.
>>> +
>>>  config TI_ADS1015
>>>       tristate "Texas Instruments ADS1015 ADC"
>>>       depends on I2C && !SENSORS_ADS1015
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 38638d46f972..855ff407fd0d 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>>> +obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
>>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>>> diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
>>> new file mode 100644
>>> index 000000000000..889399968694
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/ti-adc1x1s.c
>>> @@ -0,0 +1,233 @@
>>> +/*
>>> + * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
>>> + *
>>> + * ADC Devices Supported:
>>> + *  adc141s626 - 14-bit ADC
>>> + *  adc161s626 - 16-bit ADC
>>> + *
>>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/err.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +
>>> +#define TI_ADC_DRV_NAME      "adc1x1s"
>> I really dislike wild cards.... Just pick a part and name it after that.
>>
>>> +
>>> +enum {
>>> +     TI_ADC141S626,
>>> +     TI_ADC161S626,
>>> +};
>>> +
>>> +static const struct iio_chan_spec ti_adc141s626_channels[] = {
>>> +     {
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 0,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = 0,
>>> +             .scan_type = {
>>> +                     .sign = 's',
>>> +                     .realbits = 14,
>>> +                     .storagebits = 16,
>>> +             },
>>> +     },
>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ti_adc161s626_channels[] = {
>>> +     {
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 0,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = 0,
>>> +             .scan_type = {
>>> +                     .sign = 's',
>>> +                     .realbits = 16,
>>> +                     .storagebits = 24,
>> Storage bits should be a power of 2.
>>> +                     .shift = 6,
>>> +             },
>>> +     },
>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +};
>>> +
>>> +struct ti_adc_data {
>>> +     struct iio_dev *indio_dev;
>>> +     struct spi_device *spi;
>>> +     int read_size;
>>> +
>> enforce cacheline alignment or this is going to cause possible fun on
>> dma using spi devices.
>>> +     u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */
> 
> Thought only tx buffers need to cache aligned?
As far as I know it's either.  Certainly fits with the explanation in:
https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt

> 
>>> +};
>>> +
>>> +static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
>>> +{
>>> +     struct iio_poll_func *pf = private;
>>> +     struct iio_dev *indio_dev = pf->indio_dev;
>>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     ret = spi_read(data->spi, &data->buffer, data->read_size);
>>> +     if (!ret)
>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> +                                                iio_get_time_ns());
>>> +
>>> +     iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +     return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ti_adc_read_measurement(struct ti_adc_data *data,
>>> +                                struct iio_chan_spec const *chan, int *val)
>>> +{
>>> +     __be32 buf;
>>> +     int ret;
>>> +
>>> +     ret = spi_read(data->spi, (void *) &buf, data->read_size);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     switch (data->read_size) {
>>> +     case 2:
>>> +             *val = be16_to_cpu(buf);
>> Inelegant as buf is clearly marked as 32 bit by it's type. I'd move the
>> read into the switch statement then read into the appropriate sized
>> local variable.  More code but easier to read (slightly)
> 
> Okay.
> 
>>> +             break;
>>> +     case 3:
>>> +             *val = be32_to_cpu(buf) >> 8;
>>> +             break;
>>> +     }
>>> +
>>> +     *val = sign_extend32(*val >> chan->scan_type.shift,
>>> +                          chan->scan_type.realbits - 1);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int ti_adc_read_raw(struct iio_dev *indio_dev,
>>> +                        struct iio_chan_spec const *chan,
>>> +                        int *val, int *val2, long mask)
>>> +{
>>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     if (mask != IIO_CHAN_INFO_RAW)
>>> +             return -EINVAL;
>>> +
>>> +     if (iio_device_claim_direct_mode(indio_dev))
>>> +             return -EBUSY;
>> Should technically store and return the result of iio_device_claim_direct_mode.
>> It could in future return something else..
>>> +
>>> +     ret = ti_adc_read_measurement(data, chan, val);
>>> +     if (!ret)
>>> +             ret = IIO_VAL_INT;
>> I'd make ti_adc_read_measurement return IIO_VAL_INT if successful. Then
>> the slightly odd good path handling goes away.
> 
> Okay.
> 
>>> +
>>> +     iio_device_release_direct_mode(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static const struct iio_info ti_adc_info = {
>>> +     .driver_module = THIS_MODULE,
>>> +     .read_raw = ti_adc_read_raw,
>>> +};
>>> +
>>> +static int ti_adc_probe(struct spi_device *spi)
>>> +{
>>> +     struct iio_dev *indio_dev;
>>> +     struct ti_adc_data *data;
>>> +     int ret;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
>>> +     if (!indio_dev)
>>> +             return -ENOMEM;
>>> +
>>> +     indio_dev->info = &ti_adc_info;
>>> +     indio_dev->dev.parent = &spi->dev;
>>> +     indio_dev->dev.of_node = spi->dev.of_node;
>>> +     indio_dev->name = TI_ADC_DRV_NAME;
>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>> +     spi_set_drvdata(spi, indio_dev);
>>> +
>>> +     data = iio_priv(indio_dev);
>>> +     data->spi = spi;
>>> +
>>> +     switch (spi_get_device_id(spi)->driver_data) {
>>> +     case TI_ADC141S626:
>>> +             indio_dev->channels = ti_adc141s626_channels;
>>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
>>> +             data->read_size = 2;
>>> +             break;
>>> +     case TI_ADC161S626:
>>> +             indio_dev->channels = ti_adc161s626_channels;
>>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
>>> +             data->read_size = 3;
>>> +             break;
>>> +     }
>>> +
>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                      ti_adc_trigger_handler, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret)
>>> +             goto error_unreg_buffer;
>>> +
>>> +     return 0;
>>> +
>>> +error_unreg_buffer:
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int ti_adc_remove(struct spi_device *spi)
>>> +{
>>> +     struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>> +
>>> +     iio_device_unregister(indio_dev);
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct of_device_id ti_adc_dt_ids[] = {
>>> +     { .compatible = "ti,adc141s626", },
>>> +     { .compatible = "ti,adc161s626", },
>>> +     {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
>>> +
>>> +static const struct spi_device_id ti_adc_id[] = {
>>> +     {"adc141s626", TI_ADC141S626},
>>> +     {"adc161s626", TI_ADC161S626},
>>> +     {},
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, ti_adc_id);
>>> +
>>> +static struct spi_driver ti_adc_driver = {
>>> +     .driver = {
>>> +             .name   = TI_ADC_DRV_NAME,
>>> +             .of_match_table = of_match_ptr(ti_adc_dt_ids),
>>> +     },
>>> +     .probe          = ti_adc_probe,
>>> +     .remove         = ti_adc_remove,
>>> +     .id_table       = ti_adc_id,
>>> +};
>>> +module_spi_driver(ti_adc_driver);
>>> +
>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>> +MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
>>> +MODULE_LICENSE("GPL");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 1/2] iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs

[Matt Ranostay]

On Sun, Jul 3, 2016 at 6:04 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 02/07/16 23:05, Matt Ranostay wrote:
>> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> Few little points inline and: No wild cards in driver names!
> (though I'm occasionally half asleep and let one in).
>
> Storage bits should always be a power of 2.  (I let some
> of those slip in as well in the past... oops).
>
> Otherwise a few tiny corners where things could, I think,
> be a little more readable at the cost of slightly more code.
>
> Basically a nice little driver.
>
> Jonathan
>> ---
>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>  drivers/iio/adc/Kconfig                            |  12 ++
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 +++++++++++++++++++++
>>  4 files changed, 262 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>> new file mode 100644
>> index 000000000000..9cf7db434aee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
> Arguably could go in spi trivial devices binding file (unless you are going
> to suplement this soon anyway in which case it may be less churn to keep it
> here).

There doesn't seem to be a trivial-devices.txt for spi device bindings.

>> @@ -0,0 +1,16 @@
>> +* Texas Instruments' ADC141S626, and ADC161S626 chip
>> +
>> +Required properties:
>> + - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
>> + - reg: spi chip select number for the device
>> +
>> +Recommended properties:
>> + - spi-max-frequency: Definition as per
>> +             Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Example:
>> +adc@0 {
>> +     compatible = "ti,adc161s626";
>> +     reg = <0>;
>> +     spi-max-frequency = <4300000>;
>> +};
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 25378c5882e2..cb5e4ae3279d 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -414,6 +414,18 @@ config TI_ADC128S052
>>         This driver can also be built as a module. If so, the module will be
>>         called ti-adc128s052.
>>
>> +config TI_ADC1X1S
>> +     tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
>> +     depends on SPI
>> +     select IIO_BUFFER
>> +     select IIO_TRIGGERED_BUFFER
>> +     help
>> +       If you say yes here you get support for Texas Instruments ADC141S626,
>> +       and ADC161S626 chips.
>> +
>> +       This driver can also be build as a module. If so, the module will be
>> +       called ti-adc1x1s.
>> +
>>  config TI_ADS1015
>>       tristate "Texas Instruments ADS1015 ADC"
>>       depends on I2C && !SENSORS_ADS1015
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 38638d46f972..855ff407fd0d 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>> +obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>> diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
>> new file mode 100644
>> index 000000000000..889399968694
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-adc1x1s.c
>> @@ -0,0 +1,233 @@
>> +/*
>> + * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
>> + *
>> + * ADC Devices Supported:
>> + *  adc141s626 - 14-bit ADC
>> + *  adc161s626 - 16-bit ADC
>> + *
>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/err.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +#define TI_ADC_DRV_NAME      "adc1x1s"
> I really dislike wild cards.... Just pick a part and name it after that.
>
>> +
>> +enum {
>> +     TI_ADC141S626,
>> +     TI_ADC161S626,
>> +};
>> +
>> +static const struct iio_chan_spec ti_adc141s626_channels[] = {
>> +     {
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 0,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 's',
>> +                     .realbits = 14,
>> +                     .storagebits = 16,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +static const struct iio_chan_spec ti_adc161s626_channels[] = {
>> +     {
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 0,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 's',
>> +                     .realbits = 16,
>> +                     .storagebits = 24,
> Storage bits should be a power of 2.
>> +                     .shift = 6,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +struct ti_adc_data {
>> +     struct iio_dev *indio_dev;
>> +     struct spi_device *spi;
>> +     int read_size;
>> +
> enforce cacheline alignment or this is going to cause possible fun on
> dma using spi devices.
>> +     u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */
>> +};
>> +
>> +static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
>> +{
>> +     struct iio_poll_func *pf = private;
>> +     struct iio_dev *indio_dev = pf->indio_dev;
>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     ret = spi_read(data->spi, &data->buffer, data->read_size);
>> +     if (!ret)
>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +                                                iio_get_time_ns());
>> +
>> +     iio_trigger_notify_done(indio_dev->trig);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int ti_adc_read_measurement(struct ti_adc_data *data,
>> +                                struct iio_chan_spec const *chan, int *val)
>> +{
>> +     __be32 buf;
>> +     int ret;
>> +
>> +     ret = spi_read(data->spi, (void *) &buf, data->read_size);
>> +     if (ret)
>> +             return ret;
>> +
>> +     switch (data->read_size) {
>> +     case 2:
>> +             *val = be16_to_cpu(buf);
> Inelegant as buf is clearly marked as 32 bit by it's type. I'd move the
> read into the switch statement then read into the appropriate sized
> local variable.  More code but easier to read (slightly)
>> +             break;
>> +     case 3:
>> +             *val = be32_to_cpu(buf) >> 8;
>> +             break;
>> +     }
>> +
>> +     *val = sign_extend32(*val >> chan->scan_type.shift,
>> +                          chan->scan_type.realbits - 1);
>> +
>> +     return 0;
>> +}
>> +
>> +static int ti_adc_read_raw(struct iio_dev *indio_dev,
>> +                        struct iio_chan_spec const *chan,
>> +                        int *val, int *val2, long mask)
>> +{
>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     if (iio_device_claim_direct_mode(indio_dev))
>> +             return -EBUSY;
> Should technically store and return the result of iio_device_claim_direct_mode.
> It could in future return something else..
>> +
>> +     ret = ti_adc_read_measurement(data, chan, val);
>> +     if (!ret)
>> +             ret = IIO_VAL_INT;
> I'd make ti_adc_read_measurement return IIO_VAL_INT if successful. Then
> the slightly odd good path handling goes away.
>> +
>> +     iio_device_release_direct_mode(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct iio_info ti_adc_info = {
>> +     .driver_module = THIS_MODULE,
>> +     .read_raw = ti_adc_read_raw,
>> +};
>> +
>> +static int ti_adc_probe(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev;
>> +     struct ti_adc_data *data;
>> +     int ret;
>> +
>> +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     indio_dev->info = &ti_adc_info;
>> +     indio_dev->dev.parent = &spi->dev;
>> +     indio_dev->dev.of_node = spi->dev.of_node;
>> +     indio_dev->name = TI_ADC_DRV_NAME;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +     spi_set_drvdata(spi, indio_dev);
>> +
>> +     data = iio_priv(indio_dev);
>> +     data->spi = spi;
>> +
>> +     switch (spi_get_device_id(spi)->driver_data) {
>> +     case TI_ADC141S626:
>> +             indio_dev->channels = ti_adc141s626_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
>> +             data->read_size = 2;
>> +             break;
>> +     case TI_ADC161S626:
>> +             indio_dev->channels = ti_adc161s626_channels;
>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
>> +             data->read_size = 3;
>> +             break;
>> +     }
>> +
>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                      ti_adc_trigger_handler, NULL);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_unreg_buffer;
>> +
>> +     return 0;
>> +
>> +error_unreg_buffer:
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static int ti_adc_remove(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +
>> +     iio_device_unregister(indio_dev);
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id ti_adc_dt_ids[] = {
>> +     { .compatible = "ti,adc141s626", },
>> +     { .compatible = "ti,adc161s626", },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
>> +
>> +static const struct spi_device_id ti_adc_id[] = {
>> +     {"adc141s626", TI_ADC141S626},
>> +     {"adc161s626", TI_ADC161S626},
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(spi, ti_adc_id);
>> +
>> +static struct spi_driver ti_adc_driver = {
>> +     .driver = {
>> +             .name   = TI_ADC_DRV_NAME,
>> +             .of_match_table = of_match_ptr(ti_adc_dt_ids),
>> +     },
>> +     .probe          = ti_adc_probe,
>> +     .remove         = ti_adc_remove,
>> +     .id_table       = ti_adc_id,
>> +};
>> +module_spi_driver(ti_adc_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
>> +MODULE_LICENSE("GPL");
>>
>

Re: [RFC 1/2] iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs

[Jonathan Cameron]

On 04/07/16 04:05, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:04 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/07/16 23:05, Matt Ranostay wrote:
>>> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> Few little points inline and: No wild cards in driver names!
>> (though I'm occasionally half asleep and let one in).
>>
>> Storage bits should always be a power of 2.  (I let some
>> of those slip in as well in the past... oops).
>>
>> Otherwise a few tiny corners where things could, I think,
>> be a little more readable at the cost of slightly more code.
>>
>> Basically a nice little driver.
>>
>> Jonathan
>>> ---
>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>  drivers/iio/adc/Kconfig                            |  12 ++
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 +++++++++++++++++++++
>>>  4 files changed, 262 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>> new file mode 100644
>>> index 000000000000..9cf7db434aee
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>> Arguably could go in spi trivial devices binding file (unless you are going
>> to suplement this soon anyway in which case it may be less churn to keep it
>> here).
> 
> There doesn't seem to be a trivial-devices.txt for spi device bindings.
Hmm. Must have imagined it!  
> 
>>> @@ -0,0 +1,16 @@
>>> +* Texas Instruments' ADC141S626, and ADC161S626 chip
>>> +
>>> +Required properties:
>>> + - compatible: Should be "ti,adc141s626" or "ti,ac161s626"
>>> + - reg: spi chip select number for the device
>>> +
>>> +Recommended properties:
>>> + - spi-max-frequency: Definition as per
>>> +             Documentation/devicetree/bindings/spi/spi-bus.txt
>>> +
>>> +Example:
>>> +adc@0 {
>>> +     compatible = "ti,adc161s626";
>>> +     reg = <0>;
>>> +     spi-max-frequency = <4300000>;
>>> +};
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 25378c5882e2..cb5e4ae3279d 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -414,6 +414,18 @@ config TI_ADC128S052
>>>         This driver can also be built as a module. If so, the module will be
>>>         called ti-adc128s052.
>>>
>>> +config TI_ADC1X1S
>>> +     tristate "Texas Instruments ADC1X1S 1-channel differential ADCs"
>>> +     depends on SPI
>>> +     select IIO_BUFFER
>>> +     select IIO_TRIGGERED_BUFFER
>>> +     help
>>> +       If you say yes here you get support for Texas Instruments ADC141S626,
>>> +       and ADC161S626 chips.
>>> +
>>> +       This driver can also be build as a module. If so, the module will be
>>> +       called ti-adc1x1s.
>>> +
>>>  config TI_ADS1015
>>>       tristate "Texas Instruments ADS1015 ADC"
>>>       depends on I2C && !SENSORS_ADS1015
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 38638d46f972..855ff407fd0d 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -40,6 +40,7 @@ obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
>>>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>>> +obj-$(CONFIG_TI_ADC1X1S) += ti-adc1x1s.o
>>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>>> diff --git a/drivers/iio/adc/ti-adc1x1s.c b/drivers/iio/adc/ti-adc1x1s.c
>>> new file mode 100644
>>> index 000000000000..889399968694
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/ti-adc1x1s.c
>>> @@ -0,0 +1,233 @@
>>> +/*
>>> + * ti-adc1x1s.c - Support for the Texas Instruments 1-channel differential ADCs
>>> + *
>>> + * ADC Devices Supported:
>>> + *  adc141s626 - 14-bit ADC
>>> + *  adc161s626 - 16-bit ADC
>>> + *
>>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/err.h>
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/trigger.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +
>>> +#define TI_ADC_DRV_NAME      "adc1x1s"
>> I really dislike wild cards.... Just pick a part and name it after that.
>>
>>> +
>>> +enum {
>>> +     TI_ADC141S626,
>>> +     TI_ADC161S626,
>>> +};
>>> +
>>> +static const struct iio_chan_spec ti_adc141s626_channels[] = {
>>> +     {
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 0,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = 0,
>>> +             .scan_type = {
>>> +                     .sign = 's',
>>> +                     .realbits = 14,
>>> +                     .storagebits = 16,
>>> +             },
>>> +     },
>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +};
>>> +
>>> +static const struct iio_chan_spec ti_adc161s626_channels[] = {
>>> +     {
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 0,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = 0,
>>> +             .scan_type = {
>>> +                     .sign = 's',
>>> +                     .realbits = 16,
>>> +                     .storagebits = 24,
>> Storage bits should be a power of 2.
>>> +                     .shift = 6,
>>> +             },
>>> +     },
>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +};
>>> +
>>> +struct ti_adc_data {
>>> +     struct iio_dev *indio_dev;
>>> +     struct spi_device *spi;
>>> +     int read_size;
>>> +
>> enforce cacheline alignment or this is going to cause possible fun on
>> dma using spi devices.
>>> +     u8 buffer[16]; /* 3 byte data + 5 byte pad + 8 byte timestamp */
>>> +};
>>> +
>>> +static irqreturn_t ti_adc_trigger_handler(int irq, void *private)
>>> +{
>>> +     struct iio_poll_func *pf = private;
>>> +     struct iio_dev *indio_dev = pf->indio_dev;
>>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     ret = spi_read(data->spi, &data->buffer, data->read_size);
>>> +     if (!ret)
>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> +                                                iio_get_time_ns());
>>> +
>>> +     iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +     return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int ti_adc_read_measurement(struct ti_adc_data *data,
>>> +                                struct iio_chan_spec const *chan, int *val)
>>> +{
>>> +     __be32 buf;
>>> +     int ret;
>>> +
>>> +     ret = spi_read(data->spi, (void *) &buf, data->read_size);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     switch (data->read_size) {
>>> +     case 2:
>>> +             *val = be16_to_cpu(buf);
>> Inelegant as buf is clearly marked as 32 bit by it's type. I'd move the
>> read into the switch statement then read into the appropriate sized
>> local variable.  More code but easier to read (slightly)
>>> +             break;
>>> +     case 3:
>>> +             *val = be32_to_cpu(buf) >> 8;
>>> +             break;
>>> +     }
>>> +
>>> +     *val = sign_extend32(*val >> chan->scan_type.shift,
>>> +                          chan->scan_type.realbits - 1);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int ti_adc_read_raw(struct iio_dev *indio_dev,
>>> +                        struct iio_chan_spec const *chan,
>>> +                        int *val, int *val2, long mask)
>>> +{
>>> +     struct ti_adc_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     if (mask != IIO_CHAN_INFO_RAW)
>>> +             return -EINVAL;
>>> +
>>> +     if (iio_device_claim_direct_mode(indio_dev))
>>> +             return -EBUSY;
>> Should technically store and return the result of iio_device_claim_direct_mode.
>> It could in future return something else..
>>> +
>>> +     ret = ti_adc_read_measurement(data, chan, val);
>>> +     if (!ret)
>>> +             ret = IIO_VAL_INT;
>> I'd make ti_adc_read_measurement return IIO_VAL_INT if successful. Then
>> the slightly odd good path handling goes away.
>>> +
>>> +     iio_device_release_direct_mode(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static const struct iio_info ti_adc_info = {
>>> +     .driver_module = THIS_MODULE,
>>> +     .read_raw = ti_adc_read_raw,
>>> +};
>>> +
>>> +static int ti_adc_probe(struct spi_device *spi)
>>> +{
>>> +     struct iio_dev *indio_dev;
>>> +     struct ti_adc_data *data;
>>> +     int ret;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
>>> +     if (!indio_dev)
>>> +             return -ENOMEM;
>>> +
>>> +     indio_dev->info = &ti_adc_info;
>>> +     indio_dev->dev.parent = &spi->dev;
>>> +     indio_dev->dev.of_node = spi->dev.of_node;
>>> +     indio_dev->name = TI_ADC_DRV_NAME;
>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>> +     spi_set_drvdata(spi, indio_dev);
>>> +
>>> +     data = iio_priv(indio_dev);
>>> +     data->spi = spi;
>>> +
>>> +     switch (spi_get_device_id(spi)->driver_data) {
>>> +     case TI_ADC141S626:
>>> +             indio_dev->channels = ti_adc141s626_channels;
>>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc141s626_channels);
>>> +             data->read_size = 2;
>>> +             break;
>>> +     case TI_ADC161S626:
>>> +             indio_dev->channels = ti_adc161s626_channels;
>>> +             indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
>>> +             data->read_size = 3;
>>> +             break;
>>> +     }
>>> +
>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                      ti_adc_trigger_handler, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret)
>>> +             goto error_unreg_buffer;
>>> +
>>> +     return 0;
>>> +
>>> +error_unreg_buffer:
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int ti_adc_remove(struct spi_device *spi)
>>> +{
>>> +     struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>> +
>>> +     iio_device_unregister(indio_dev);
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct of_device_id ti_adc_dt_ids[] = {
>>> +     { .compatible = "ti,adc141s626", },
>>> +     { .compatible = "ti,adc161s626", },
>>> +     {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
>>> +
>>> +static const struct spi_device_id ti_adc_id[] = {
>>> +     {"adc141s626", TI_ADC141S626},
>>> +     {"adc161s626", TI_ADC161S626},
>>> +     {},
>>> +};
>>> +MODULE_DEVICE_TABLE(spi, ti_adc_id);
>>> +
>>> +static struct spi_driver ti_adc_driver = {
>>> +     .driver = {
>>> +             .name   = TI_ADC_DRV_NAME,
>>> +             .of_match_table = of_match_ptr(ti_adc_dt_ids),
>>> +     },
>>> +     .probe          = ti_adc_probe,
>>> +     .remove         = ti_adc_remove,
>>> +     .id_table       = ti_adc_id,
>>> +};
>>> +module_spi_driver(ti_adc_driver);
>>> +
>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>> +MODULE_DESCRIPTION("Texas Instruments ADC1x1S 1-channel differential ADC");
>>> +MODULE_LICENSE("GPL");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[RFC 2/2] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

Add support for the LMP91000 potentiostat which is used for chemical
sensing applications.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/potentiostat/Kconfig                   |  21 ++
 drivers/iio/potentiostat/Makefile                  |   6 +
 drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
 6 files changed, 360 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
 create mode 100644 drivers/iio/potentiostat/Kconfig
 create mode 100644 drivers/iio/potentiostat/Makefile
 create mode 100644 drivers/iio/potentiostat/lmp91000.c

diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
new file mode 100644
index 000000000000..636c548cedee
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
@@ -0,0 +1,28 @@
+* Texas Instruments LMP91000 potentiostat
+
+http://www.ti.com/lit/ds/symlink/lmp91000.pdf
+
+Required properties:
+
+  - compatible: should be "ti,lmp91000"
+  - reg: the I2C address of the device
+  - io-channels: the phandle and channel of the iio provider
+
+Optional properties:
+
+  - ti,tia-gain-ohm: ohm value on the feedback loop for the transimpedance
+    amplifier. Must be 0 (external resistor, default), 2750, 3500, 7000, 14000,
+    35000, 120000, or 350000 ohms.
+
+  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
+    sensor. Must be 10, 33, 50, or 100 (default) ohms.
+
+Example:
+
+lmp91000@48 {
+	compatible = "ti,lmp91000";
+	reg = <0x48>;
+	ti,tia-gain-ohm = <7500>;
+	ti,rload = <100>;
+	io-channels = <&adc1x1s 0>;
+};
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 6743b18194fb..a31a8cf2c330 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -87,6 +87,7 @@ if IIO_TRIGGER
    source "drivers/iio/trigger/Kconfig"
 endif #IIO_TRIGGER
 source "drivers/iio/potentiometer/Kconfig"
+source "drivers/iio/potentiostat/Kconfig"
 source "drivers/iio/pressure/Kconfig"
 source "drivers/iio/proximity/Kconfig"
 source "drivers/iio/temperature/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 87e4c4369e2f..2b6e2762a886 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -29,6 +29,7 @@ obj-y += light/
 obj-y += magnetometer/
 obj-y += orientation/
 obj-y += potentiometer/
+obj-y += potentiostat/
 obj-y += pressure/
 obj-y += proximity/
 obj-y += temperature/
diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
new file mode 100644
index 000000000000..591682c05ae9
--- /dev/null
+++ b/drivers/iio/potentiostat/Kconfig
@@ -0,0 +1,21 @@
+#
+# Potentiostat drivers
+#
+# When adding new entries keep the list in alphabetical order
+
+menu "Digital potentiostats"
+
+config LMP91000
+	tristate "Texas Instruments LMP91000 potentiostat driver"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for the Texas Instruments
+	  LMP91000 digital potentiostat chip.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called lmp91000
+
+endmenu
diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
new file mode 100644
index 000000000000..64d315ef4449
--- /dev/null
+++ b/drivers/iio/potentiostat/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O potentiostat drivers
+#
+
+# When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_LMP91000) += lmp91000.o
diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
new file mode 100644
index 000000000000..fe90172eac28
--- /dev/null
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -0,0 +1,303 @@
+/*
+ * lmp91000.c - Support for Texas Instruments digital potentiostats
+ *
+ * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * TODO: bias voltage + polarity control, and multiple chip support
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define LMP91000_REG_LOCK		0x01
+#define LMP91000_REG_TIACN		0x10
+#define LMP91000_REG_TIACN_GAIN_SHIFT	2
+
+#define LMP91000_REG_REFCN		0x11
+#define LMP91000_REG_REFCN_EXT_REF	0x20
+#define LMP91000_REG_REFCN_50_ZERO	0x80
+
+#define LMP91000_REG_MODECN		0x12
+#define LMP91000_REG_MODECN_3LEAD	0x03
+#define LMP91000_REG_MODECN_TEMP	0x07
+
+#define LMP91000_DRV_NAME	"lmp91000"
+
+static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
+					 120000, 350000 };
+
+static const int lmp91000_rload[] = { 10, 33, 50, 100 };
+
+static const struct regmap_config lmp91000_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+struct lmp91000_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct iio_channel *vout_chan;
+
+	u8 buffer[16]; /* 32-bit data + 32-bit padding + 64-bit timestamp */
+};
+
+static const struct iio_chan_spec lmp91000_channels[] = {
+	{ /* chemical channel mV */
+		.type = IIO_VOLTAGE,
+		.channel = 0,
+		.indexed = 1,
+		.address = LMP91000_REG_MODECN_3LEAD,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+	{ /* temperature channel mV */
+		.type = IIO_VOLTAGE,
+		.channel = 1,
+		.indexed = 1,
+		.address = LMP91000_REG_MODECN_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = -1,
+	},
+};
+
+static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
+{
+	int state, ret;
+
+	ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
+	if (ret)
+		return -EINVAL;
+
+	ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
+	if (ret)
+		return -EINVAL;
+
+	/* delay till first temperature reading is complete */
+	if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
+		usleep_range(3000, 4000);
+
+	ret = iio_read_channel_raw(data->vout_chan, val);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static irqreturn_t lmp91000_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct lmp91000_data *data = iio_priv(indio_dev);
+	int ret, val;
+
+	ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
+	if (!ret) {
+		*((int *) data->buffer) = val;
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+						   iio_get_time_ns());
+	}
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int lmp91000_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct lmp91000_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (iio_device_claim_direct_mode(indio_dev))
+		return -EBUSY;
+
+	ret = lmp91000_read(data, chan->address, val);
+	if (!ret)
+		ret = IIO_VAL_INT;
+
+	iio_device_release_direct_mode(indio_dev);
+
+	return ret;
+}
+
+static const struct iio_info lmp91000_info = {
+	.driver_module = THIS_MODULE,
+	.read_raw = lmp91000_read_raw,
+};
+
+static int lmp91000_read_config(struct lmp91000_data *data)
+{
+	struct device *dev = data->dev;
+	struct device_node *np = dev->of_node;
+	int i, val1, val2, ret;
+
+	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val1);
+	if (ret) {
+		val1 = 0;
+		dev_info(dev, "no ti,tia-gain-ohm defined, default to external\n");
+	}
+
+	ret = of_property_read_u32(np, "ti,rload-ohm", &val2);
+	if (ret) {
+		val2 = 100;
+		dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val2);
+	}
+
+	ret = -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
+		if (lmp91000_tia_gain[i] == val1) {
+			val1 = i;
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "invalid ti,tia-gain-ohm %d", val1);
+		return ret;
+	}
+
+	ret = -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
+		if (lmp91000_rload[i] == val2) {
+			val2 = i;
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "invalid ti,rload-ohm %d", val2);
+		return ret;
+	}
+
+	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
+	regmap_write(data->regmap, LMP91000_REG_TIACN,
+				(val1 << LMP91000_REG_TIACN_GAIN_SHIFT) | val2);
+	regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
+					| LMP91000_REG_REFCN_50_ZERO);
+	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
+
+	return 0;
+}
+
+static int lmp91000_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct lmp91000_data *data;
+	struct iio_dev *indio_dev;
+	struct iio_channel *vout_chan;
+	int ret;
+
+	vout_chan = iio_channel_get(dev, NULL);
+	if (IS_ERR(vout_chan))
+		return -EPROBE_DEFER;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->info = &lmp91000_info;
+	indio_dev->channels = lmp91000_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
+	indio_dev->name = LMP91000_DRV_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	i2c_set_clientdata(client, indio_dev);
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+	data->vout_chan = vout_chan;
+	data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "regmap initialization failed.\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	ret = lmp91000_read_config(data);
+	if (ret)
+		return ret;
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 lmp91000_trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_unreg_buffer;
+
+	return 0;
+
+error_unreg_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return ret;
+}
+
+static int lmp91000_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct lmp91000_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+	iio_channel_release(data->vout_chan);
+
+	return 0;
+}
+
+static const struct of_device_id lmp91000_of_match[] = {
+	{ .compatible = "ti,lmp91000", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, lmp91000_of_match);
+
+static const struct i2c_device_id lmp91000_id[] = {
+	{ "lmp91000", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, lmp91000_id);
+
+static struct i2c_driver lmp91000_driver = {
+	.driver = {
+		.name = LMP91000_DRV_NAME,
+		.of_match_table = of_match_ptr(lmp91000_of_match),
+	},
+	.probe = lmp91000_probe,
+	.remove = lmp91000_remove,
+	.id_table = lmp91000_id,
+};
+module_i2c_driver(lmp91000_driver);
+
+MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
+MODULE_DESCRIPTION("LMP91000 digital potentiostat");
+MODULE_LICENSE("GPL");
-- 
2.7.4

Re: [RFC 2/2] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 02/07/16 23:05, Matt Ranostay wrote:
> Add support for the LMP91000 potentiostat which is used for chemical
> sensing applications.
Step one.  I had to look up what a potentiostat was... So perhaps a brief
idiots guide in the intro to the patch?
:)

This clearly raises some interesting questions... I'll put comments on that
in the cover letter and more or less stick to reviewing code alone here.


> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> ---
>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>  drivers/iio/Kconfig                                |   1 +
>  drivers/iio/Makefile                               |   1 +
>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>  drivers/iio/potentiostat/Makefile                  |   6 +
>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>  6 files changed, 360 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>  create mode 100644 drivers/iio/potentiostat/Kconfig
>  create mode 100644 drivers/iio/potentiostat/Makefile
>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
> new file mode 100644
> index 000000000000..636c548cedee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
> @@ -0,0 +1,28 @@
> +* Texas Instruments LMP91000 potentiostat
> +
> +http://www.ti.com/lit/ds/symlink/lmp91000.pdf
> +
> +Required properties:
> +
> +  - compatible: should be "ti,lmp91000"
> +  - reg: the I2C address of the device
> +  - io-channels: the phandle and channel of the iio provider
> +
> +Optional properties:
> +
> +  - ti,tia-gain-ohm: ohm value on the feedback loop for the transimpedance
> +    amplifier. Must be 0 (external resistor, default), 2750, 3500, 7000, 14000,
> +    35000, 120000, or 350000 ohms.
Better perhaps to have an explicit entry to say use the external resistor?
If nothing else, it ought to be infinite resistance for that not 0.

Also does it ever make sense to change this at runtime or is it a case of
being matched to a particular probe?
> +
> +  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
> +    sensor. Must be 10, 33, 50, or 100 (default) ohms.
Again, make sense to ever change at runtime?  Guessing there is still a best
value for a given probe even if sometimes it might want 'tweaking'.

No idea - never even seen one of these probes that I know of ;)
> +
> +Example:
> +
> +lmp91000@48 {
> +	compatible = "ti,lmp91000";
> +	reg = <0x48>;
> +	ti,tia-gain-ohm = <7500>;
> +	ti,rload = <100>;
> +	io-channels = <&adc1x1s 0>;
> +};
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 6743b18194fb..a31a8cf2c330 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -87,6 +87,7 @@ if IIO_TRIGGER
>     source "drivers/iio/trigger/Kconfig"
>  endif #IIO_TRIGGER
>  source "drivers/iio/potentiometer/Kconfig"
> +source "drivers/iio/potentiostat/Kconfig"
>  source "drivers/iio/pressure/Kconfig"
>  source "drivers/iio/proximity/Kconfig"
>  source "drivers/iio/temperature/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 87e4c4369e2f..2b6e2762a886 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -29,6 +29,7 @@ obj-y += light/
>  obj-y += magnetometer/
>  obj-y += orientation/
>  obj-y += potentiometer/
> +obj-y += potentiostat/
I wonder if grouping analog front ends in at subdirectory above this
might make sense?  Feels like we may get some more.  We already have
'amplifiers' that could be moved into that directory as well.

Or for now we could just have both this and amplifiers in an
'Analog front ends' directory and break them up into their own directories
if we get too many to handle in a single dir in the future?  I don't really
mind but would like this an amplifiers grouped together in some way.

>  obj-y += pressure/
>  obj-y += proximity/
>  obj-y += temperature/
> diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
> new file mode 100644
> index 000000000000..591682c05ae9
> --- /dev/null
> +++ b/drivers/iio/potentiostat/Kconfig
> @@ -0,0 +1,21 @@
> +#
> +# Potentiostat drivers
> +#
> +# When adding new entries keep the list in alphabetical order
> +
> +menu "Digital potentiostats"
> +
> +config LMP91000
> +	tristate "Texas Instruments LMP91000 potentiostat driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for the Texas Instruments
> +	  LMP91000 digital potentiostat chip.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called lmp91000
> +
> +endmenu
> diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
> new file mode 100644
> index 000000000000..64d315ef4449
> --- /dev/null
> +++ b/drivers/iio/potentiostat/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for industrial I/O potentiostat drivers
> +#
> +
> +# When adding new entries keep the list in alphabetical order
> +obj-$(CONFIG_LMP91000) += lmp91000.o
> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
> new file mode 100644
> index 000000000000..fe90172eac28
> --- /dev/null
> +++ b/drivers/iio/potentiostat/lmp91000.c
> @@ -0,0 +1,303 @@
> +/*
> + * lmp91000.c - Support for Texas Instruments digital potentiostats
> + *
> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * TODO: bias voltage + polarity control, and multiple chip support
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define LMP91000_REG_LOCK		0x01
> +#define LMP91000_REG_TIACN		0x10
> +#define LMP91000_REG_TIACN_GAIN_SHIFT	2
> +
> +#define LMP91000_REG_REFCN		0x11
> +#define LMP91000_REG_REFCN_EXT_REF	0x20
> +#define LMP91000_REG_REFCN_50_ZERO	0x80
> +
> +#define LMP91000_REG_MODECN		0x12
> +#define LMP91000_REG_MODECN_3LEAD	0x03
> +#define LMP91000_REG_MODECN_TEMP	0x07
> +
> +#define LMP91000_DRV_NAME	"lmp91000"
> +
> +static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
> +					 120000, 350000 };
> +
> +static const int lmp91000_rload[] = { 10, 33, 50, 100 };
> +
> +static const struct regmap_config lmp91000_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +struct lmp91000_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct iio_channel *vout_chan;
> +
> +	u8 buffer[16]; /* 32-bit data + 32-bit padding + 64-bit timestamp */
> +};
> +
> +static const struct iio_chan_spec lmp91000_channels[] = {
> +	{ /* chemical channel mV */
> +		.type = IIO_VOLTAGE,
> +		.channel = 0,
> +		.indexed = 1,
> +		.address = LMP91000_REG_MODECN_3LEAD,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 32,
> +			.storagebits = 32,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +	{ /* temperature channel mV */
> +		.type = IIO_VOLTAGE,
> +		.channel = 1,
> +		.indexed = 1,
> +		.address = LMP91000_REG_MODECN_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = -1,
> +	},
> +};
> +
> +static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
> +{
> +	int state, ret;
> +
> +	ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* delay till first temperature reading is complete */
> +	if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
> +		usleep_range(3000, 4000);
> +
> +	ret = iio_read_channel_raw(data->vout_chan, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static irqreturn_t lmp91000_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct lmp91000_data *data = iio_priv(indio_dev);
> +	int ret, val;
> +
> +	ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
> +	if (!ret) {
> +		*((int *) data->buffer) = val;
> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +						   iio_get_time_ns());
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int lmp91000_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct lmp91000_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	if (iio_device_claim_direct_mode(indio_dev))
> +		return -EBUSY;
> +
> +	ret = lmp91000_read(data, chan->address, val);
> +	if (!ret)
> +		ret = IIO_VAL_INT;
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info lmp91000_info = {
> +	.driver_module = THIS_MODULE,
> +	.read_raw = lmp91000_read_raw,
> +};
> +
> +static int lmp91000_read_config(struct lmp91000_data *data)
> +{
> +	struct device *dev = data->dev;
> +	struct device_node *np = dev->of_node;
> +	int i, val1, val2, ret;
> +
> +	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val1);
> +	if (ret) {
> +		val1 = 0;
> +		dev_info(dev, "no ti,tia-gain-ohm defined, default to external\n");
> +	}
> +
> +	ret = of_property_read_u32(np, "ti,rload-ohm", &val2);
> +	if (ret) {
> +		val2 = 100;
> +		dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val2);
> +	}
> +
I'd reorder this to have the property followed by it's use together.
Not terribly keen on variables with names as generic as val1 and val2 either.
> +	ret = -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
> +		if (lmp91000_tia_gain[i] == val1) {
> +			val1 = i;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (ret) {
> +		dev_err(dev, "invalid ti,tia-gain-ohm %d", val1);
> +		return ret;
> +	}
> +
> +	ret = -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
> +		if (lmp91000_rload[i] == val2) {
> +			val2 = i;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (ret) {
> +		dev_err(dev, "invalid ti,rload-ohm %d", val2);
> +		return ret;
> +	}
> +
> +	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
Error handling?
> +	regmap_write(data->regmap, LMP91000_REG_TIACN,
> +				(val1 << LMP91000_REG_TIACN_GAIN_SHIFT) | val2);
> +	regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
> +					| LMP91000_REG_REFCN_50_ZERO);
> +	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
> +
> +	return 0;
> +}
> +
> +static int lmp91000_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct lmp91000_data *data;
> +	struct iio_dev *indio_dev;
> +	struct iio_channel *vout_chan;
> +	int ret;
> +
> +	vout_chan = iio_channel_get(dev, NULL);
> +	if (IS_ERR(vout_chan))
> +		return -EPROBE_DEFER;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->info = &lmp91000_info;
> +	indio_dev->channels = lmp91000_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
> +	indio_dev->name = LMP91000_DRV_NAME;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +	data->vout_chan = vout_chan;
> +	data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "regmap initialization failed.\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	ret = lmp91000_read_config(data);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 lmp91000_trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_unreg_buffer;
> +
> +	return 0;
> +
> +error_unreg_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int lmp91000_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct lmp91000_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	iio_channel_release(data->vout_chan);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lmp91000_of_match[] = {
> +	{ .compatible = "ti,lmp91000", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, lmp91000_of_match);
> +
> +static const struct i2c_device_id lmp91000_id[] = {
> +	{ "lmp91000", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, lmp91000_id);
> +
> +static struct i2c_driver lmp91000_driver = {
> +	.driver = {
> +		.name = LMP91000_DRV_NAME,
> +		.of_match_table = of_match_ptr(lmp91000_of_match),
> +	},
> +	.probe = lmp91000_probe,
> +	.remove = lmp91000_remove,
> +	.id_table = lmp91000_id,
> +};
> +module_i2c_driver(lmp91000_driver);
> +
> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
> +MODULE_DESCRIPTION("LMP91000 digital potentiostat");
> +MODULE_LICENSE("GPL");
> 

Re: [RFC 2/2] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 02/07/16 23:05, Matt Ranostay wrote:
>> Add support for the LMP91000 potentiostat which is used for chemical
>> sensing applications.
> Step one.  I had to look up what a potentiostat was... So perhaps a brief
> idiots guide in the intro to the patch?
> :)
>
> This clearly raises some interesting questions... I'll put comments on that
> in the cover letter and more or less stick to reviewing code alone here.
>
>
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>  drivers/iio/Kconfig                                |   1 +
>>  drivers/iio/Makefile                               |   1 +
>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>  6 files changed, 360 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>> new file mode 100644
>> index 000000000000..636c548cedee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>> @@ -0,0 +1,28 @@
>> +* Texas Instruments LMP91000 potentiostat
>> +
>> +http://www.ti.com/lit/ds/symlink/lmp91000.pdf
>> +
>> +Required properties:
>> +
>> +  - compatible: should be "ti,lmp91000"
>> +  - reg: the I2C address of the device
>> +  - io-channels: the phandle and channel of the iio provider
>> +
>> +Optional properties:
>> +
>> +  - ti,tia-gain-ohm: ohm value on the feedback loop for the transimpedance
>> +    amplifier. Must be 0 (external resistor, default), 2750, 3500, 7000, 14000,
>> +    35000, 120000, or 350000 ohms.
> Better perhaps to have an explicit entry to say use the external resistor?
> If nothing else, it ought to be infinite resistance for that not 0.
>


> Also does it ever make sense to change this at runtime or is it a case of
> being matched to a particular probe?

I mean it doesn't in theory damage anything but probably an un-needed
runtime configuration complexity.

>> +
>> +  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
>> +    sensor. Must be 10, 33, 50, or 100 (default) ohms.
> Again, make sense to ever change at runtime?  Guessing there is still a best
> value for a given probe even if sometimes it might want 'tweaking'.
>
> No idea - never even seen one of these probes that I know of ;)

Actually the load resistor value is documented in the sensor
datasheet, and never changes.
An incorrect value could in theory shorter the life of the sensor.

>> +
>> +Example:
>> +
>> +lmp91000@48 {
>> +     compatible = "ti,lmp91000";
>> +     reg = <0x48>;
>> +     ti,tia-gain-ohm = <7500>;
>> +     ti,rload = <100>;
>> +     io-channels = <&adc1x1s 0>;
>> +};
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 6743b18194fb..a31a8cf2c330 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -87,6 +87,7 @@ if IIO_TRIGGER
>>     source "drivers/iio/trigger/Kconfig"
>>  endif #IIO_TRIGGER
>>  source "drivers/iio/potentiometer/Kconfig"
>> +source "drivers/iio/potentiostat/Kconfig"
>>  source "drivers/iio/pressure/Kconfig"
>>  source "drivers/iio/proximity/Kconfig"
>>  source "drivers/iio/temperature/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 87e4c4369e2f..2b6e2762a886 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -29,6 +29,7 @@ obj-y += light/
>>  obj-y += magnetometer/
>>  obj-y += orientation/
>>  obj-y += potentiometer/
>> +obj-y += potentiostat/
> I wonder if grouping analog front ends in at subdirectory above this
> might make sense?  Feels like we may get some more.  We already have
> 'amplifiers' that could be moved into that directory as well.
>
> Or for now we could just have both this and amplifiers in an
> 'Analog front ends' directory and break them up into their own directories
> if we get too many to handle in a single dir in the future?  I don't really
> mind but would like this an amplifiers grouped together in some way.
>
>>  obj-y += pressure/
>>  obj-y += proximity/
>>  obj-y += temperature/
>> diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
>> new file mode 100644
>> index 000000000000..591682c05ae9
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/Kconfig
>> @@ -0,0 +1,21 @@
>> +#
>> +# Potentiostat drivers
>> +#
>> +# When adding new entries keep the list in alphabetical order
>> +
>> +menu "Digital potentiostats"
>> +
>> +config LMP91000
>> +     tristate "Texas Instruments LMP91000 potentiostat driver"
>> +     depends on I2C
>> +     select REGMAP_I2C
>> +     select IIO_BUFFER
>> +     select IIO_TRIGGERED_BUFFER
>> +     help
>> +       Say yes here to build support for the Texas Instruments
>> +       LMP91000 digital potentiostat chip.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called lmp91000
>> +
>> +endmenu
>> diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
>> new file mode 100644
>> index 000000000000..64d315ef4449
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for industrial I/O potentiostat drivers
>> +#
>> +
>> +# When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_LMP91000) += lmp91000.o
>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>> new file mode 100644
>> index 000000000000..fe90172eac28
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/lmp91000.c
>> @@ -0,0 +1,303 @@
>> +/*
>> + * lmp91000.c - Support for Texas Instruments digital potentiostats
>> + *
>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * TODO: bias voltage + polarity control, and multiple chip support
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/delay.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/consumer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +#define LMP91000_REG_LOCK            0x01
>> +#define LMP91000_REG_TIACN           0x10
>> +#define LMP91000_REG_TIACN_GAIN_SHIFT        2
>> +
>> +#define LMP91000_REG_REFCN           0x11
>> +#define LMP91000_REG_REFCN_EXT_REF   0x20
>> +#define LMP91000_REG_REFCN_50_ZERO   0x80
>> +
>> +#define LMP91000_REG_MODECN          0x12
>> +#define LMP91000_REG_MODECN_3LEAD    0x03
>> +#define LMP91000_REG_MODECN_TEMP     0x07
>> +
>> +#define LMP91000_DRV_NAME    "lmp91000"
>> +
>> +static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
>> +                                      120000, 350000 };
>> +
>> +static const int lmp91000_rload[] = { 10, 33, 50, 100 };
>> +
>> +static const struct regmap_config lmp91000_regmap_config = {
>> +     .reg_bits = 8,
>> +     .val_bits = 8,
>> +};
>> +
>> +struct lmp91000_data {
>> +     struct regmap *regmap;
>> +     struct device *dev;
>> +     struct iio_channel *vout_chan;
>> +
>> +     u8 buffer[16]; /* 32-bit data + 32-bit padding + 64-bit timestamp */
>> +};
>> +
>> +static const struct iio_chan_spec lmp91000_channels[] = {
>> +     { /* chemical channel mV */
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 0,
>> +             .indexed = 1,
>> +             .address = LMP91000_REG_MODECN_3LEAD,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 's',
>> +                     .realbits = 32,
>> +                     .storagebits = 32,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +     { /* temperature channel mV */
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 1,
>> +             .indexed = 1,
>> +             .address = LMP91000_REG_MODECN_TEMP,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = -1,
>> +     },
>> +};
>> +
>> +static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
>> +{
>> +     int state, ret;
>> +
>> +     ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
>> +     if (ret)
>> +             return -EINVAL;
>> +
>> +     ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
>> +     if (ret)
>> +             return -EINVAL;
>> +
>> +     /* delay till first temperature reading is complete */
>> +     if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
>> +             usleep_range(3000, 4000);
>> +
>> +     ret = iio_read_channel_raw(data->vout_chan, val);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +
>> +static irqreturn_t lmp91000_trigger_handler(int irq, void *private)
>> +{
>> +     struct iio_poll_func *pf = private;
>> +     struct iio_dev *indio_dev = pf->indio_dev;
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +     int ret, val;
>> +
>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>> +     if (!ret) {
>> +             *((int *) data->buffer) = val;
>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +                                                iio_get_time_ns());
>> +     }
>> +
>> +     iio_trigger_notify_done(indio_dev->trig);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int lmp91000_read_raw(struct iio_dev *indio_dev,
>> +                          struct iio_chan_spec const *chan,
>> +                          int *val, int *val2, long mask)
>> +{
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     if (iio_device_claim_direct_mode(indio_dev))
>> +             return -EBUSY;
>> +
>> +     ret = lmp91000_read(data, chan->address, val);
>> +     if (!ret)
>> +             ret = IIO_VAL_INT;
>> +
>> +     iio_device_release_direct_mode(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct iio_info lmp91000_info = {
>> +     .driver_module = THIS_MODULE,
>> +     .read_raw = lmp91000_read_raw,
>> +};
>> +
>> +static int lmp91000_read_config(struct lmp91000_data *data)
>> +{
>> +     struct device *dev = data->dev;
>> +     struct device_node *np = dev->of_node;
>> +     int i, val1, val2, ret;
>> +
>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val1);
>> +     if (ret) {
>> +             val1 = 0;
>> +             dev_info(dev, "no ti,tia-gain-ohm defined, default to external\n");
>> +     }
>> +
>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val2);
>> +     if (ret) {
>> +             val2 = 100;
>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val2);
>> +     }
>> +
> I'd reorder this to have the property followed by it's use together.
> Not terribly keen on variables with names as generic as val1 and val2 either.
>> +     ret = -EINVAL;
>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>> +             if (lmp91000_tia_gain[i] == val1) {
>> +                     val1 = i;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ret) {
>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d", val1);
>> +             return ret;
>> +     }
>> +
>> +     ret = -EINVAL;
>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>> +             if (lmp91000_rload[i] == val2) {
>> +                     val2 = i;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ret) {
>> +             dev_err(dev, "invalid ti,rload-ohm %d", val2);
>> +             return ret;
>> +     }
>> +
>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
> Error handling?
>> +     regmap_write(data->regmap, LMP91000_REG_TIACN,
>> +                             (val1 << LMP91000_REG_TIACN_GAIN_SHIFT) | val2);
>> +     regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
>> +                                     | LMP91000_REG_REFCN_50_ZERO);
>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
>> +
>> +     return 0;
>> +}
>> +
>> +static int lmp91000_probe(struct i2c_client *client,
>> +                       const struct i2c_device_id *id)
>> +{
>> +     struct device *dev = &client->dev;
>> +     struct lmp91000_data *data;
>> +     struct iio_dev *indio_dev;
>> +     struct iio_channel *vout_chan;
>> +     int ret;
>> +
>> +     vout_chan = iio_channel_get(dev, NULL);
>> +     if (IS_ERR(vout_chan))
>> +             return -EPROBE_DEFER;
>> +
>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     indio_dev->info = &lmp91000_info;
>> +     indio_dev->channels = lmp91000_channels;
>> +     indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
>> +     indio_dev->name = LMP91000_DRV_NAME;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +     i2c_set_clientdata(client, indio_dev);
>> +
>> +     data = iio_priv(indio_dev);
>> +     data->dev = dev;
>> +     data->vout_chan = vout_chan;
>> +     data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
>> +     if (IS_ERR(data->regmap)) {
>> +             dev_err(dev, "regmap initialization failed.\n");
>> +             return PTR_ERR(data->regmap);
>> +     }
>> +
>> +     ret = lmp91000_read_config(data);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                      lmp91000_trigger_handler, NULL);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_unreg_buffer;
>> +
>> +     return 0;
>> +
>> +error_unreg_buffer:
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static int lmp91000_remove(struct i2c_client *client)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +
>> +     iio_device_unregister(indio_dev);
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +     iio_channel_release(data->vout_chan);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id lmp91000_of_match[] = {
>> +     { .compatible = "ti,lmp91000", },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, lmp91000_of_match);
>> +
>> +static const struct i2c_device_id lmp91000_id[] = {
>> +     { "lmp91000", 0 },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lmp91000_id);
>> +
>> +static struct i2c_driver lmp91000_driver = {
>> +     .driver = {
>> +             .name = LMP91000_DRV_NAME,
>> +             .of_match_table = of_match_ptr(lmp91000_of_match),
>> +     },
>> +     .probe = lmp91000_probe,
>> +     .remove = lmp91000_remove,
>> +     .id_table = lmp91000_id,
>> +};
>> +module_i2c_driver(lmp91000_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("LMP91000 digital potentiostat");
>> +MODULE_LICENSE("GPL");
>>
>

Re: [RFC 2/2] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 03/07/16 22:59, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/07/16 23:05, Matt Ranostay wrote:
>>> Add support for the LMP91000 potentiostat which is used for chemical
>>> sensing applications.
>> Step one.  I had to look up what a potentiostat was... So perhaps a brief
>> idiots guide in the intro to the patch?
>> :)
>>
>> This clearly raises some interesting questions... I'll put comments on that
>> in the cover letter and more or less stick to reviewing code alone here.
>>
>>
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>> ---
>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>  drivers/iio/Kconfig                                |   1 +
>>>  drivers/iio/Makefile                               |   1 +
>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>  6 files changed, 360 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>> new file mode 100644
>>> index 000000000000..636c548cedee
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>> @@ -0,0 +1,28 @@
>>> +* Texas Instruments LMP91000 potentiostat
>>> +
>>> +http://www.ti.com/lit/ds/symlink/lmp91000.pdf
>>> +
>>> +Required properties:
>>> +
>>> +  - compatible: should be "ti,lmp91000"
>>> +  - reg: the I2C address of the device
>>> +  - io-channels: the phandle and channel of the iio provider
>>> +
>>> +Optional properties:
>>> +
>>> +  - ti,tia-gain-ohm: ohm value on the feedback loop for the transimpedance
>>> +    amplifier. Must be 0 (external resistor, default), 2750, 3500, 7000, 14000,
>>> +    35000, 120000, or 350000 ohms.
>> Better perhaps to have an explicit entry to say use the external resistor?
>> If nothing else, it ought to be infinite resistance for that not 0.
>>
> 
> 
>> Also does it ever make sense to change this at runtime or is it a case of
>> being matched to a particular probe?
> 
> I mean it doesn't in theory damage anything but probably an un-needed
> runtime configuration complexity.
> 
>>> +
>>> +  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
>>> +    sensor. Must be 10, 33, 50, or 100 (default) ohms.
>> Again, make sense to ever change at runtime?  Guessing there is still a best
>> value for a given probe even if sometimes it might want 'tweaking'.
>>
>> No idea - never even seen one of these probes that I know of ;)
> 
> Actually the load resistor value is documented in the sensor
> datasheet, and never changes.
> An incorrect value could in theory shorter the life of the sensor.
Cool. Perhaps expand the description to make this clear?
> 
>>> +
>>> +Example:
>>> +
>>> +lmp91000@48 {
>>> +     compatible = "ti,lmp91000";
>>> +     reg = <0x48>;
>>> +     ti,tia-gain-ohm = <7500>;
>>> +     ti,rload = <100>;
>>> +     io-channels = <&adc1x1s 0>;
>>> +};
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 6743b18194fb..a31a8cf2c330 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -87,6 +87,7 @@ if IIO_TRIGGER
>>>     source "drivers/iio/trigger/Kconfig"
>>>  endif #IIO_TRIGGER
>>>  source "drivers/iio/potentiometer/Kconfig"
>>> +source "drivers/iio/potentiostat/Kconfig"
>>>  source "drivers/iio/pressure/Kconfig"
>>>  source "drivers/iio/proximity/Kconfig"
>>>  source "drivers/iio/temperature/Kconfig"
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index 87e4c4369e2f..2b6e2762a886 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -29,6 +29,7 @@ obj-y += light/
>>>  obj-y += magnetometer/
>>>  obj-y += orientation/
>>>  obj-y += potentiometer/
>>> +obj-y += potentiostat/
>> I wonder if grouping analog front ends in at subdirectory above this
>> might make sense?  Feels like we may get some more.  We already have
>> 'amplifiers' that could be moved into that directory as well.
>>
>> Or for now we could just have both this and amplifiers in an
>> 'Analog front ends' directory and break them up into their own directories
>> if we get too many to handle in a single dir in the future?  I don't really
>> mind but would like this an amplifiers grouped together in some way.
>>
>>>  obj-y += pressure/
>>>  obj-y += proximity/
>>>  obj-y += temperature/
>>> diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
>>> new file mode 100644
>>> index 000000000000..591682c05ae9
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/Kconfig
>>> @@ -0,0 +1,21 @@
>>> +#
>>> +# Potentiostat drivers
>>> +#
>>> +# When adding new entries keep the list in alphabetical order
>>> +
>>> +menu "Digital potentiostats"
>>> +
>>> +config LMP91000
>>> +     tristate "Texas Instruments LMP91000 potentiostat driver"
>>> +     depends on I2C
>>> +     select REGMAP_I2C
>>> +     select IIO_BUFFER
>>> +     select IIO_TRIGGERED_BUFFER
>>> +     help
>>> +       Say yes here to build support for the Texas Instruments
>>> +       LMP91000 digital potentiostat chip.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called lmp91000
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
>>> new file mode 100644
>>> index 000000000000..64d315ef4449
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/Makefile
>>> @@ -0,0 +1,6 @@
>>> +#
>>> +# Makefile for industrial I/O potentiostat drivers
>>> +#
>>> +
>>> +# When adding new entries keep the list in alphabetical order
>>> +obj-$(CONFIG_LMP91000) += lmp91000.o
>>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>>> new file mode 100644
>>> index 000000000000..fe90172eac28
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>> @@ -0,0 +1,303 @@
>>> +/*
>>> + * lmp91000.c - Support for Texas Instruments digital potentiostats
>>> + *
>>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * TODO: bias voltage + polarity control, and multiple chip support
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/of.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/consumer.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +
>>> +#define LMP91000_REG_LOCK            0x01
>>> +#define LMP91000_REG_TIACN           0x10
>>> +#define LMP91000_REG_TIACN_GAIN_SHIFT        2
>>> +
>>> +#define LMP91000_REG_REFCN           0x11
>>> +#define LMP91000_REG_REFCN_EXT_REF   0x20
>>> +#define LMP91000_REG_REFCN_50_ZERO   0x80
>>> +
>>> +#define LMP91000_REG_MODECN          0x12
>>> +#define LMP91000_REG_MODECN_3LEAD    0x03
>>> +#define LMP91000_REG_MODECN_TEMP     0x07
>>> +
>>> +#define LMP91000_DRV_NAME    "lmp91000"
>>> +
>>> +static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
>>> +                                      120000, 350000 };
>>> +
>>> +static const int lmp91000_rload[] = { 10, 33, 50, 100 };
>>> +
>>> +static const struct regmap_config lmp91000_regmap_config = {
>>> +     .reg_bits = 8,
>>> +     .val_bits = 8,
>>> +};
>>> +
>>> +struct lmp91000_data {
>>> +     struct regmap *regmap;
>>> +     struct device *dev;
>>> +     struct iio_channel *vout_chan;
>>> +
>>> +     u8 buffer[16]; /* 32-bit data + 32-bit padding + 64-bit timestamp */
>>> +};
>>> +
>>> +static const struct iio_chan_spec lmp91000_channels[] = {
>>> +     { /* chemical channel mV */
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 0,
>>> +             .indexed = 1,
>>> +             .address = LMP91000_REG_MODECN_3LEAD,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = 0,
>>> +             .scan_type = {
>>> +                     .sign = 's',
>>> +                     .realbits = 32,
>>> +                     .storagebits = 32,
>>> +             },
>>> +     },
>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +     { /* temperature channel mV */
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 1,
>>> +             .indexed = 1,
>>> +             .address = LMP91000_REG_MODECN_TEMP,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = -1,
>>> +     },
>>> +};
>>> +
>>> +static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
>>> +{
>>> +     int state, ret;
>>> +
>>> +     ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
>>> +     if (ret)
>>> +             return -EINVAL;
>>> +
>>> +     ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
>>> +     if (ret)
>>> +             return -EINVAL;
>>> +
>>> +     /* delay till first temperature reading is complete */
>>> +     if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
>>> +             usleep_range(3000, 4000);
>>> +
>>> +     ret = iio_read_channel_raw(data->vout_chan, val);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static irqreturn_t lmp91000_trigger_handler(int irq, void *private)
>>> +{
>>> +     struct iio_poll_func *pf = private;
>>> +     struct iio_dev *indio_dev = pf->indio_dev;
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +     int ret, val;
>>> +
>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>> +     if (!ret) {
>>> +             *((int *) data->buffer) = val;
>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> +                                                iio_get_time_ns());
>>> +     }
>>> +
>>> +     iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +     return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int lmp91000_read_raw(struct iio_dev *indio_dev,
>>> +                          struct iio_chan_spec const *chan,
>>> +                          int *val, int *val2, long mask)
>>> +{
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     if (mask != IIO_CHAN_INFO_RAW)
>>> +             return -EINVAL;
>>> +
>>> +     if (iio_device_claim_direct_mode(indio_dev))
>>> +             return -EBUSY;
>>> +
>>> +     ret = lmp91000_read(data, chan->address, val);
>>> +     if (!ret)
>>> +             ret = IIO_VAL_INT;
>>> +
>>> +     iio_device_release_direct_mode(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static const struct iio_info lmp91000_info = {
>>> +     .driver_module = THIS_MODULE,
>>> +     .read_raw = lmp91000_read_raw,
>>> +};
>>> +
>>> +static int lmp91000_read_config(struct lmp91000_data *data)
>>> +{
>>> +     struct device *dev = data->dev;
>>> +     struct device_node *np = dev->of_node;
>>> +     int i, val1, val2, ret;
>>> +
>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val1);
>>> +     if (ret) {
>>> +             val1 = 0;
>>> +             dev_info(dev, "no ti,tia-gain-ohm defined, default to external\n");
>>> +     }
>>> +
>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val2);
>>> +     if (ret) {
>>> +             val2 = 100;
>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val2);
>>> +     }
>>> +
>> I'd reorder this to have the property followed by it's use together.
>> Not terribly keen on variables with names as generic as val1 and val2 either.
>>> +     ret = -EINVAL;
>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>> +             if (lmp91000_tia_gain[i] == val1) {
>>> +                     val1 = i;
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d", val1);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = -EINVAL;
>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>> +             if (lmp91000_rload[i] == val2) {
>>> +                     val2 = i;
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             dev_err(dev, "invalid ti,rload-ohm %d", val2);
>>> +             return ret;
>>> +     }
>>> +
>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>> Error handling?
>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN,
>>> +                             (val1 << LMP91000_REG_TIACN_GAIN_SHIFT) | val2);
>>> +     regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
>>> +                                     | LMP91000_REG_REFCN_50_ZERO);
>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int lmp91000_probe(struct i2c_client *client,
>>> +                       const struct i2c_device_id *id)
>>> +{
>>> +     struct device *dev = &client->dev;
>>> +     struct lmp91000_data *data;
>>> +     struct iio_dev *indio_dev;
>>> +     struct iio_channel *vout_chan;
>>> +     int ret;
>>> +
>>> +     vout_chan = iio_channel_get(dev, NULL);
>>> +     if (IS_ERR(vout_chan))
>>> +             return -EPROBE_DEFER;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>>> +     if (!indio_dev)
>>> +             return -ENOMEM;
>>> +
>>> +     indio_dev->info = &lmp91000_info;
>>> +     indio_dev->channels = lmp91000_channels;
>>> +     indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
>>> +     indio_dev->name = LMP91000_DRV_NAME;
>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>> +     i2c_set_clientdata(client, indio_dev);
>>> +
>>> +     data = iio_priv(indio_dev);
>>> +     data->dev = dev;
>>> +     data->vout_chan = vout_chan;
>>> +     data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
>>> +     if (IS_ERR(data->regmap)) {
>>> +             dev_err(dev, "regmap initialization failed.\n");
>>> +             return PTR_ERR(data->regmap);
>>> +     }
>>> +
>>> +     ret = lmp91000_read_config(data);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                      lmp91000_trigger_handler, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret)
>>> +             goto error_unreg_buffer;
>>> +
>>> +     return 0;
>>> +
>>> +error_unreg_buffer:
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int lmp91000_remove(struct i2c_client *client)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +
>>> +     iio_device_unregister(indio_dev);
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +     iio_channel_release(data->vout_chan);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct of_device_id lmp91000_of_match[] = {
>>> +     { .compatible = "ti,lmp91000", },
>>> +     { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, lmp91000_of_match);
>>> +
>>> +static const struct i2c_device_id lmp91000_id[] = {
>>> +     { "lmp91000", 0 },
>>> +     {}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, lmp91000_id);
>>> +
>>> +static struct i2c_driver lmp91000_driver = {
>>> +     .driver = {
>>> +             .name = LMP91000_DRV_NAME,
>>> +             .of_match_table = of_match_ptr(lmp91000_of_match),
>>> +     },
>>> +     .probe = lmp91000_probe,
>>> +     .remove = lmp91000_remove,
>>> +     .id_table = lmp91000_id,
>>> +};
>>> +module_i2c_driver(lmp91000_driver);
>>> +
>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>> +MODULE_DESCRIPTION("LMP91000 digital potentiostat");
>>> +MODULE_LICENSE("GPL");
>>>
>>

Re: [RFC 2/2] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 02/07/16 23:05, Matt Ranostay wrote:
>> Add support for the LMP91000 potentiostat which is used for chemical
>> sensing applications.
> Step one.  I had to look up what a potentiostat was... So perhaps a brief
> idiots guide in the intro to the patch?
> :)
>
> This clearly raises some interesting questions... I'll put comments on that
> in the cover letter and more or less stick to reviewing code alone here.
>
>
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> ---
>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>  drivers/iio/Kconfig                                |   1 +
>>  drivers/iio/Makefile                               |   1 +
>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>  6 files changed, 360 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>> new file mode 100644
>> index 000000000000..636c548cedee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>> @@ -0,0 +1,28 @@
>> +* Texas Instruments LMP91000 potentiostat
>> +
>> +http://www.ti.com/lit/ds/symlink/lmp91000.pdf
>> +
>> +Required properties:
>> +
>> +  - compatible: should be "ti,lmp91000"
>> +  - reg: the I2C address of the device
>> +  - io-channels: the phandle and channel of the iio provider
>> +
>> +Optional properties:
>> +
>> +  - ti,tia-gain-ohm: ohm value on the feedback loop for the transimpedance
>> +    amplifier. Must be 0 (external resistor, default), 2750, 3500, 7000, 14000,
>> +    35000, 120000, or 350000 ohms.
> Better perhaps to have an explicit entry to say use the external resistor?
> If nothing else, it ought to be infinite resistance for that not 0.

How would it be explicit? Have it check to see if it is a char array
first and the string matches 'external'?
Rather not make the other values have to be strings as well.

>
> Also does it ever make sense to change this at runtime or is it a case of
> being matched to a particular probe?
>> +
>> +  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
>> +    sensor. Must be 10, 33, 50, or 100 (default) ohms.
> Again, make sense to ever change at runtime?  Guessing there is still a best
> value for a given probe even if sometimes it might want 'tweaking'.
>
> No idea - never even seen one of these probes that I know of ;)
>> +
>> +Example:
>> +
>> +lmp91000@48 {
>> +     compatible = "ti,lmp91000";
>> +     reg = <0x48>;
>> +     ti,tia-gain-ohm = <7500>;
>> +     ti,rload = <100>;
>> +     io-channels = <&adc1x1s 0>;
>> +};
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 6743b18194fb..a31a8cf2c330 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -87,6 +87,7 @@ if IIO_TRIGGER
>>     source "drivers/iio/trigger/Kconfig"
>>  endif #IIO_TRIGGER
>>  source "drivers/iio/potentiometer/Kconfig"
>> +source "drivers/iio/potentiostat/Kconfig"
>>  source "drivers/iio/pressure/Kconfig"
>>  source "drivers/iio/proximity/Kconfig"
>>  source "drivers/iio/temperature/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 87e4c4369e2f..2b6e2762a886 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -29,6 +29,7 @@ obj-y += light/
>>  obj-y += magnetometer/
>>  obj-y += orientation/
>>  obj-y += potentiometer/
>> +obj-y += potentiostat/
> I wonder if grouping analog front ends in at subdirectory above this
> might make sense?  Feels like we may get some more.  We already have
> 'amplifiers' that could be moved into that directory as well.
>
> Or for now we could just have both this and amplifiers in an
> 'Analog front ends' directory and break them up into their own directories
> if we get too many to handle in a single dir in the future?  I don't really
> mind but would like this an amplifiers grouped together in some way.
>
>>  obj-y += pressure/
>>  obj-y += proximity/
>>  obj-y += temperature/
>> diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
>> new file mode 100644
>> index 000000000000..591682c05ae9
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/Kconfig
>> @@ -0,0 +1,21 @@
>> +#
>> +# Potentiostat drivers
>> +#
>> +# When adding new entries keep the list in alphabetical order
>> +
>> +menu "Digital potentiostats"
>> +
>> +config LMP91000
>> +     tristate "Texas Instruments LMP91000 potentiostat driver"
>> +     depends on I2C
>> +     select REGMAP_I2C
>> +     select IIO_BUFFER
>> +     select IIO_TRIGGERED_BUFFER
>> +     help
>> +       Say yes here to build support for the Texas Instruments
>> +       LMP91000 digital potentiostat chip.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called lmp91000
>> +
>> +endmenu
>> diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
>> new file mode 100644
>> index 000000000000..64d315ef4449
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/Makefile
>> @@ -0,0 +1,6 @@
>> +#
>> +# Makefile for industrial I/O potentiostat drivers
>> +#
>> +
>> +# When adding new entries keep the list in alphabetical order
>> +obj-$(CONFIG_LMP91000) += lmp91000.o
>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>> new file mode 100644
>> index 000000000000..fe90172eac28
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/lmp91000.c
>> @@ -0,0 +1,303 @@
>> +/*
>> + * lmp91000.c - Support for Texas Instruments digital potentiostats
>> + *
>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * TODO: bias voltage + polarity control, and multiple chip support
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/delay.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/consumer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +#define LMP91000_REG_LOCK            0x01
>> +#define LMP91000_REG_TIACN           0x10
>> +#define LMP91000_REG_TIACN_GAIN_SHIFT        2
>> +
>> +#define LMP91000_REG_REFCN           0x11
>> +#define LMP91000_REG_REFCN_EXT_REF   0x20
>> +#define LMP91000_REG_REFCN_50_ZERO   0x80
>> +
>> +#define LMP91000_REG_MODECN          0x12
>> +#define LMP91000_REG_MODECN_3LEAD    0x03
>> +#define LMP91000_REG_MODECN_TEMP     0x07
>> +
>> +#define LMP91000_DRV_NAME    "lmp91000"
>> +
>> +static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
>> +                                      120000, 350000 };
>> +
>> +static const int lmp91000_rload[] = { 10, 33, 50, 100 };
>> +
>> +static const struct regmap_config lmp91000_regmap_config = {
>> +     .reg_bits = 8,
>> +     .val_bits = 8,
>> +};
>> +
>> +struct lmp91000_data {
>> +     struct regmap *regmap;
>> +     struct device *dev;
>> +     struct iio_channel *vout_chan;
>> +
>> +     u8 buffer[16]; /* 32-bit data + 32-bit padding + 64-bit timestamp */
>> +};
>> +
>> +static const struct iio_chan_spec lmp91000_channels[] = {
>> +     { /* chemical channel mV */
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 0,
>> +             .indexed = 1,
>> +             .address = LMP91000_REG_MODECN_3LEAD,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = 0,
>> +             .scan_type = {
>> +                     .sign = 's',
>> +                     .realbits = 32,
>> +                     .storagebits = 32,
>> +             },
>> +     },
>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>> +     { /* temperature channel mV */
>> +             .type = IIO_VOLTAGE,
>> +             .channel = 1,
>> +             .indexed = 1,
>> +             .address = LMP91000_REG_MODECN_TEMP,
>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +             .scan_index = -1,
>> +     },
>> +};
>> +
>> +static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
>> +{
>> +     int state, ret;
>> +
>> +     ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
>> +     if (ret)
>> +             return -EINVAL;
>> +
>> +     ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
>> +     if (ret)
>> +             return -EINVAL;
>> +
>> +     /* delay till first temperature reading is complete */
>> +     if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
>> +             usleep_range(3000, 4000);
>> +
>> +     ret = iio_read_channel_raw(data->vout_chan, val);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +
>> +static irqreturn_t lmp91000_trigger_handler(int irq, void *private)
>> +{
>> +     struct iio_poll_func *pf = private;
>> +     struct iio_dev *indio_dev = pf->indio_dev;
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +     int ret, val;
>> +
>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>> +     if (!ret) {
>> +             *((int *) data->buffer) = val;
>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +                                                iio_get_time_ns());
>> +     }
>> +
>> +     iio_trigger_notify_done(indio_dev->trig);
>> +
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static int lmp91000_read_raw(struct iio_dev *indio_dev,
>> +                          struct iio_chan_spec const *chan,
>> +                          int *val, int *val2, long mask)
>> +{
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     if (mask != IIO_CHAN_INFO_RAW)
>> +             return -EINVAL;
>> +
>> +     if (iio_device_claim_direct_mode(indio_dev))
>> +             return -EBUSY;
>> +
>> +     ret = lmp91000_read(data, chan->address, val);
>> +     if (!ret)
>> +             ret = IIO_VAL_INT;
>> +
>> +     iio_device_release_direct_mode(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct iio_info lmp91000_info = {
>> +     .driver_module = THIS_MODULE,
>> +     .read_raw = lmp91000_read_raw,
>> +};
>> +
>> +static int lmp91000_read_config(struct lmp91000_data *data)
>> +{
>> +     struct device *dev = data->dev;
>> +     struct device_node *np = dev->of_node;
>> +     int i, val1, val2, ret;
>> +
>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val1);
>> +     if (ret) {
>> +             val1 = 0;
>> +             dev_info(dev, "no ti,tia-gain-ohm defined, default to external\n");
>> +     }
>> +
>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val2);
>> +     if (ret) {
>> +             val2 = 100;
>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val2);
>> +     }
>> +
> I'd reorder this to have the property followed by it's use together.
> Not terribly keen on variables with names as generic as val1 and val2 either.
>> +     ret = -EINVAL;
>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>> +             if (lmp91000_tia_gain[i] == val1) {
>> +                     val1 = i;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ret) {
>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d", val1);
>> +             return ret;
>> +     }
>> +
>> +     ret = -EINVAL;
>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>> +             if (lmp91000_rload[i] == val2) {
>> +                     val2 = i;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ret) {
>> +             dev_err(dev, "invalid ti,rload-ohm %d", val2);
>> +             return ret;
>> +     }
>> +
>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
> Error handling?
>> +     regmap_write(data->regmap, LMP91000_REG_TIACN,
>> +                             (val1 << LMP91000_REG_TIACN_GAIN_SHIFT) | val2);
>> +     regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
>> +                                     | LMP91000_REG_REFCN_50_ZERO);
>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
>> +
>> +     return 0;
>> +}
>> +
>> +static int lmp91000_probe(struct i2c_client *client,
>> +                       const struct i2c_device_id *id)
>> +{
>> +     struct device *dev = &client->dev;
>> +     struct lmp91000_data *data;
>> +     struct iio_dev *indio_dev;
>> +     struct iio_channel *vout_chan;
>> +     int ret;
>> +
>> +     vout_chan = iio_channel_get(dev, NULL);
>> +     if (IS_ERR(vout_chan))
>> +             return -EPROBE_DEFER;
>> +
>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     indio_dev->info = &lmp91000_info;
>> +     indio_dev->channels = lmp91000_channels;
>> +     indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
>> +     indio_dev->name = LMP91000_DRV_NAME;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +     i2c_set_clientdata(client, indio_dev);
>> +
>> +     data = iio_priv(indio_dev);
>> +     data->dev = dev;
>> +     data->vout_chan = vout_chan;
>> +     data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
>> +     if (IS_ERR(data->regmap)) {
>> +             dev_err(dev, "regmap initialization failed.\n");
>> +             return PTR_ERR(data->regmap);
>> +     }
>> +
>> +     ret = lmp91000_read_config(data);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                      lmp91000_trigger_handler, NULL);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_unreg_buffer;
>> +
>> +     return 0;
>> +
>> +error_unreg_buffer:
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static int lmp91000_remove(struct i2c_client *client)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +
>> +     iio_device_unregister(indio_dev);
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +     iio_channel_release(data->vout_chan);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id lmp91000_of_match[] = {
>> +     { .compatible = "ti,lmp91000", },
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(of, lmp91000_of_match);
>> +
>> +static const struct i2c_device_id lmp91000_id[] = {
>> +     { "lmp91000", 0 },
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lmp91000_id);
>> +
>> +static struct i2c_driver lmp91000_driver = {
>> +     .driver = {
>> +             .name = LMP91000_DRV_NAME,
>> +             .of_match_table = of_match_ptr(lmp91000_of_match),
>> +     },
>> +     .probe = lmp91000_probe,
>> +     .remove = lmp91000_remove,
>> +     .id_table = lmp91000_id,
>> +};
>> +module_i2c_driver(lmp91000_driver);
>> +
>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>> +MODULE_DESCRIPTION("LMP91000 digital potentiostat");
>> +MODULE_LICENSE("GPL");
>>
>

Re: [RFC 2/2] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 03/07/16 23:48, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/07/16 23:05, Matt Ranostay wrote:
>>> Add support for the LMP91000 potentiostat which is used for chemical
>>> sensing applications.
>> Step one.  I had to look up what a potentiostat was... So perhaps a brief
>> idiots guide in the intro to the patch?
>> :)
>>
>> This clearly raises some interesting questions... I'll put comments on that
>> in the cover letter and more or less stick to reviewing code alone here.
>>
>>
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>> ---
>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>  drivers/iio/Kconfig                                |   1 +
>>>  drivers/iio/Makefile                               |   1 +
>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>  6 files changed, 360 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>> new file mode 100644
>>> index 000000000000..636c548cedee
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>> @@ -0,0 +1,28 @@
>>> +* Texas Instruments LMP91000 potentiostat
>>> +
>>> +http://www.ti.com/lit/ds/symlink/lmp91000.pdf
>>> +
>>> +Required properties:
>>> +
>>> +  - compatible: should be "ti,lmp91000"
>>> +  - reg: the I2C address of the device
>>> +  - io-channels: the phandle and channel of the iio provider
>>> +
>>> +Optional properties:
>>> +
>>> +  - ti,tia-gain-ohm: ohm value on the feedback loop for the transimpedance
>>> +    amplifier. Must be 0 (external resistor, default), 2750, 3500, 7000, 14000,
>>> +    35000, 120000, or 350000 ohms.
>> Better perhaps to have an explicit entry to say use the external resistor?
>> If nothing else, it ought to be infinite resistance for that not 0.
> 
> How would it be explicit? Have it check to see if it is a char array
> first and the string matches 'external'?
> Rather not make the other values have to be strings as well.
Ah, sorry wasn't clear.  A separate 'ti,tia-external-resistor' attribute to
say that we have an external resistor rather than (or perhaps in parallel with?)
the internal one.
> 
>>
>> Also does it ever make sense to change this at runtime or is it a case of
>> being matched to a particular probe?
>>> +
>>> +  - ti,rload-ohm: ohm value of the internal resistor load applied to the gas
>>> +    sensor. Must be 10, 33, 50, or 100 (default) ohms.
>> Again, make sense to ever change at runtime?  Guessing there is still a best
>> value for a given probe even if sometimes it might want 'tweaking'.
>>
>> No idea - never even seen one of these probes that I know of ;)
>>> +
>>> +Example:
>>> +
>>> +lmp91000@48 {
>>> +     compatible = "ti,lmp91000";
>>> +     reg = <0x48>;
>>> +     ti,tia-gain-ohm = <7500>;
>>> +     ti,rload = <100>;
>>> +     io-channels = <&adc1x1s 0>;
>>> +};
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 6743b18194fb..a31a8cf2c330 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -87,6 +87,7 @@ if IIO_TRIGGER
>>>     source "drivers/iio/trigger/Kconfig"
>>>  endif #IIO_TRIGGER
>>>  source "drivers/iio/potentiometer/Kconfig"
>>> +source "drivers/iio/potentiostat/Kconfig"
>>>  source "drivers/iio/pressure/Kconfig"
>>>  source "drivers/iio/proximity/Kconfig"
>>>  source "drivers/iio/temperature/Kconfig"
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index 87e4c4369e2f..2b6e2762a886 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -29,6 +29,7 @@ obj-y += light/
>>>  obj-y += magnetometer/
>>>  obj-y += orientation/
>>>  obj-y += potentiometer/
>>> +obj-y += potentiostat/
>> I wonder if grouping analog front ends in at subdirectory above this
>> might make sense?  Feels like we may get some more.  We already have
>> 'amplifiers' that could be moved into that directory as well.
>>
>> Or for now we could just have both this and amplifiers in an
>> 'Analog front ends' directory and break them up into their own directories
>> if we get too many to handle in a single dir in the future?  I don't really
>> mind but would like this an amplifiers grouped together in some way.
>>
>>>  obj-y += pressure/
>>>  obj-y += proximity/
>>>  obj-y += temperature/
>>> diff --git a/drivers/iio/potentiostat/Kconfig b/drivers/iio/potentiostat/Kconfig
>>> new file mode 100644
>>> index 000000000000..591682c05ae9
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/Kconfig
>>> @@ -0,0 +1,21 @@
>>> +#
>>> +# Potentiostat drivers
>>> +#
>>> +# When adding new entries keep the list in alphabetical order
>>> +
>>> +menu "Digital potentiostats"
>>> +
>>> +config LMP91000
>>> +     tristate "Texas Instruments LMP91000 potentiostat driver"
>>> +     depends on I2C
>>> +     select REGMAP_I2C
>>> +     select IIO_BUFFER
>>> +     select IIO_TRIGGERED_BUFFER
>>> +     help
>>> +       Say yes here to build support for the Texas Instruments
>>> +       LMP91000 digital potentiostat chip.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called lmp91000
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/potentiostat/Makefile b/drivers/iio/potentiostat/Makefile
>>> new file mode 100644
>>> index 000000000000..64d315ef4449
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/Makefile
>>> @@ -0,0 +1,6 @@
>>> +#
>>> +# Makefile for industrial I/O potentiostat drivers
>>> +#
>>> +
>>> +# When adding new entries keep the list in alphabetical order
>>> +obj-$(CONFIG_LMP91000) += lmp91000.o
>>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>>> new file mode 100644
>>> index 000000000000..fe90172eac28
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>> @@ -0,0 +1,303 @@
>>> +/*
>>> + * lmp91000.c - Support for Texas Instruments digital potentiostats
>>> + *
>>> + * Copyright (C) 2016 Matt Ranostay <mranostay@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * TODO: bias voltage + polarity control, and multiple chip support
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/of.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/buffer.h>
>>> +#include <linux/iio/consumer.h>
>>> +#include <linux/iio/trigger_consumer.h>
>>> +#include <linux/iio/triggered_buffer.h>
>>> +
>>> +#define LMP91000_REG_LOCK            0x01
>>> +#define LMP91000_REG_TIACN           0x10
>>> +#define LMP91000_REG_TIACN_GAIN_SHIFT        2
>>> +
>>> +#define LMP91000_REG_REFCN           0x11
>>> +#define LMP91000_REG_REFCN_EXT_REF   0x20
>>> +#define LMP91000_REG_REFCN_50_ZERO   0x80
>>> +
>>> +#define LMP91000_REG_MODECN          0x12
>>> +#define LMP91000_REG_MODECN_3LEAD    0x03
>>> +#define LMP91000_REG_MODECN_TEMP     0x07
>>> +
>>> +#define LMP91000_DRV_NAME    "lmp91000"
>>> +
>>> +static const int lmp91000_tia_gain[] = { 0, 2750, 3500, 7000, 14000, 35000,
>>> +                                      120000, 350000 };
>>> +
>>> +static const int lmp91000_rload[] = { 10, 33, 50, 100 };
>>> +
>>> +static const struct regmap_config lmp91000_regmap_config = {
>>> +     .reg_bits = 8,
>>> +     .val_bits = 8,
>>> +};
>>> +
>>> +struct lmp91000_data {
>>> +     struct regmap *regmap;
>>> +     struct device *dev;
>>> +     struct iio_channel *vout_chan;
>>> +
>>> +     u8 buffer[16]; /* 32-bit data + 32-bit padding + 64-bit timestamp */
>>> +};
>>> +
>>> +static const struct iio_chan_spec lmp91000_channels[] = {
>>> +     { /* chemical channel mV */
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 0,
>>> +             .indexed = 1,
>>> +             .address = LMP91000_REG_MODECN_3LEAD,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = 0,
>>> +             .scan_type = {
>>> +                     .sign = 's',
>>> +                     .realbits = 32,
>>> +                     .storagebits = 32,
>>> +             },
>>> +     },
>>> +     IIO_CHAN_SOFT_TIMESTAMP(1),
>>> +     { /* temperature channel mV */
>>> +             .type = IIO_VOLTAGE,
>>> +             .channel = 1,
>>> +             .indexed = 1,
>>> +             .address = LMP91000_REG_MODECN_TEMP,
>>> +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +             .scan_index = -1,
>>> +     },
>>> +};
>>> +
>>> +static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
>>> +{
>>> +     int state, ret;
>>> +
>>> +     ret = regmap_read(data->regmap, LMP91000_REG_MODECN, &state);
>>> +     if (ret)
>>> +             return -EINVAL;
>>> +
>>> +     ret = regmap_write(data->regmap, LMP91000_REG_MODECN, channel);
>>> +     if (ret)
>>> +             return -EINVAL;
>>> +
>>> +     /* delay till first temperature reading is complete */
>>> +     if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
>>> +             usleep_range(3000, 4000);
>>> +
>>> +     ret = iio_read_channel_raw(data->vout_chan, val);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static irqreturn_t lmp91000_trigger_handler(int irq, void *private)
>>> +{
>>> +     struct iio_poll_func *pf = private;
>>> +     struct iio_dev *indio_dev = pf->indio_dev;
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +     int ret, val;
>>> +
>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>> +     if (!ret) {
>>> +             *((int *) data->buffer) = val;
>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> +                                                iio_get_time_ns());
>>> +     }
>>> +
>>> +     iio_trigger_notify_done(indio_dev->trig);
>>> +
>>> +     return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int lmp91000_read_raw(struct iio_dev *indio_dev,
>>> +                          struct iio_chan_spec const *chan,
>>> +                          int *val, int *val2, long mask)
>>> +{
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     if (mask != IIO_CHAN_INFO_RAW)
>>> +             return -EINVAL;
>>> +
>>> +     if (iio_device_claim_direct_mode(indio_dev))
>>> +             return -EBUSY;
>>> +
>>> +     ret = lmp91000_read(data, chan->address, val);
>>> +     if (!ret)
>>> +             ret = IIO_VAL_INT;
>>> +
>>> +     iio_device_release_direct_mode(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static const struct iio_info lmp91000_info = {
>>> +     .driver_module = THIS_MODULE,
>>> +     .read_raw = lmp91000_read_raw,
>>> +};
>>> +
>>> +static int lmp91000_read_config(struct lmp91000_data *data)
>>> +{
>>> +     struct device *dev = data->dev;
>>> +     struct device_node *np = dev->of_node;
>>> +     int i, val1, val2, ret;
>>> +
>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val1);
>>> +     if (ret) {
>>> +             val1 = 0;
>>> +             dev_info(dev, "no ti,tia-gain-ohm defined, default to external\n");
>>> +     }
>>> +
>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val2);
>>> +     if (ret) {
>>> +             val2 = 100;
>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val2);
>>> +     }
>>> +
>> I'd reorder this to have the property followed by it's use together.
>> Not terribly keen on variables with names as generic as val1 and val2 either.
>>> +     ret = -EINVAL;
>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>> +             if (lmp91000_tia_gain[i] == val1) {
>>> +                     val1 = i;
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d", val1);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = -EINVAL;
>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>> +             if (lmp91000_rload[i] == val2) {
>>> +                     val2 = i;
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             dev_err(dev, "invalid ti,rload-ohm %d", val2);
>>> +             return ret;
>>> +     }
>>> +
>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>> Error handling?
>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN,
>>> +                             (val1 << LMP91000_REG_TIACN_GAIN_SHIFT) | val2);
>>> +     regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
>>> +                                     | LMP91000_REG_REFCN_50_ZERO);
>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int lmp91000_probe(struct i2c_client *client,
>>> +                       const struct i2c_device_id *id)
>>> +{
>>> +     struct device *dev = &client->dev;
>>> +     struct lmp91000_data *data;
>>> +     struct iio_dev *indio_dev;
>>> +     struct iio_channel *vout_chan;
>>> +     int ret;
>>> +
>>> +     vout_chan = iio_channel_get(dev, NULL);
>>> +     if (IS_ERR(vout_chan))
>>> +             return -EPROBE_DEFER;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>>> +     if (!indio_dev)
>>> +             return -ENOMEM;
>>> +
>>> +     indio_dev->info = &lmp91000_info;
>>> +     indio_dev->channels = lmp91000_channels;
>>> +     indio_dev->num_channels = ARRAY_SIZE(lmp91000_channels);
>>> +     indio_dev->name = LMP91000_DRV_NAME;
>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>> +     i2c_set_clientdata(client, indio_dev);
>>> +
>>> +     data = iio_priv(indio_dev);
>>> +     data->dev = dev;
>>> +     data->vout_chan = vout_chan;
>>> +     data->regmap = devm_regmap_init_i2c(client, &lmp91000_regmap_config);
>>> +     if (IS_ERR(data->regmap)) {
>>> +             dev_err(dev, "regmap initialization failed.\n");
>>> +             return PTR_ERR(data->regmap);
>>> +     }
>>> +
>>> +     ret = lmp91000_read_config(data);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                      lmp91000_trigger_handler, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret)
>>> +             goto error_unreg_buffer;
>>> +
>>> +     return 0;
>>> +
>>> +error_unreg_buffer:
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int lmp91000_remove(struct i2c_client *client)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +
>>> +     iio_device_unregister(indio_dev);
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +     iio_channel_release(data->vout_chan);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct of_device_id lmp91000_of_match[] = {
>>> +     { .compatible = "ti,lmp91000", },
>>> +     { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, lmp91000_of_match);
>>> +
>>> +static const struct i2c_device_id lmp91000_id[] = {
>>> +     { "lmp91000", 0 },
>>> +     {}
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, lmp91000_id);
>>> +
>>> +static struct i2c_driver lmp91000_driver = {
>>> +     .driver = {
>>> +             .name = LMP91000_DRV_NAME,
>>> +             .of_match_table = of_match_ptr(lmp91000_of_match),
>>> +     },
>>> +     .probe = lmp91000_probe,
>>> +     .remove = lmp91000_remove,
>>> +     .id_table = lmp91000_id,
>>> +};
>>> +module_i2c_driver(lmp91000_driver);
>>> +
>>> +MODULE_AUTHOR("Matt Ranostay <mranostay@gmail.com>");
>>> +MODULE_DESCRIPTION("LMP91000 digital potentiostat");
>>> +MODULE_LICENSE("GPL");
>>>
>>

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Matt Ranostay]

On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
> 16-bit ADC for chemical sensoring applications.
>
>  * add support for the TI LMP91000 potentiostat
>  * add support for ADC141S626 and ADC161S626 ADC chips

Probably should have put what I am RFC'ing :).

* does this belong in a new path drivers/iio/potentiostat ?
* first example of a iio consumer within drivers/iio, does it seems sane?
* ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
* Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
16635 is ~VA. Of course true zero is defined by the VREF voltage.

>
> Matt Ranostay (2):
>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>   iio: potentiostat: add LMP91000 support
>
>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>  drivers/iio/Kconfig                                |   1 +
>  drivers/iio/Makefile                               |   1 +
>  drivers/iio/adc/Kconfig                            |  12 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>  drivers/iio/potentiostat/Makefile                  |   6 +
>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>  10 files changed, 622 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>  create mode 100644 drivers/iio/potentiostat/Kconfig
>  create mode 100644 drivers/iio/potentiostat/Makefile
>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>
> --
> 2.7.4
>

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Jonathan Cameron]

On 02/07/16 23:13, Matt Ranostay wrote:
> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>> 16-bit ADC for chemical sensoring applications.
>>
>>  * add support for the TI LMP91000 potentiostat
>>  * add support for ADC141S626 and ADC161S626 ADC chips
> 
> Probably should have put what I am RFC'ing :).
> 
> * does this belong in a new path drivers/iio/potentiostat ?
I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
as well to take the only similar driver we already have.

> * first example of a iio consumer within drivers/iio, does it seems sane?
It's 'interesting'.  You've worked around the whole question of how to handle
a mux by putting a push interface equipped client on top of the polled interface
of the ADC.  It's an elegant solution that I'd never considered.

By the very nature of a mux interface, unless we are piping the mux switching
out on the same trigger system as the read back, the actual read out must be
polled rather than self clocked. Only the mux knows when it is ready.
The triggered version has all sorts of additional complexity even if we had
output buffers already to go (such as requiring the output buffering to
'lead' the input buffering to give the mux time to switch.

Question to my mind is whether this is a generic and flexible enough approach
to use for this sort of device in the future... I think we have two classes
of 'analog device' that we need to support:

1) Simple all channels always there devices such as analog accelerometers
feeding into an ADC with a sequencer (or a software based sequencer).
In that case the data flow is clearly going to go over the buffered interface.
The accelerometer driver is just massaging the data for types / scale adjustments.
It has no influence on the sampling of the data.

2) The 'smart' front end with a mux.  In this case the 'when to read' question
is driven by the front end, not the ADC.  Games could be played to push the
sampling of data over to the ADC, but is it worth doing?

So if we wanted to do this, the AFE could itself export a trigger that is then
used by the ADC which in turn pushes data back to the AFE driver via the buffered
data interface.  The AFE driver would then have to handle the demux of this
data stream into a coherent form to push out in it's own buffer.
This approach gains the following:
 - quick data transfers, particularly if we are dealing with a multiple output
   mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
   (or sequenced) ADC. So in this case if the mux was set to provide all 16
   channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc. 
   The mux driver would then have to recombine these 16 channels before kicking
   them out.

To do this we'd need to add an interface to allow the AFE/mux driver to set
the trigger for the ADC to its own. 

If we want to do this quickish I think that's about the lightest weight option
we can do.

Now, the question is, what are the disadvantages of going with what you
have here for this driver but keeping in mind the above for when it matters?

I'm guessing we never need to run this particularly driver very fast...
I'm inclined to say yes but would like some other opinions on this one!
(hence the expanded Cc list - please do pull in anyone else you think
might be interested!)

> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
Should be some defined. That was easy ;)
> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
err. Odd. Go with signed I think.
> 
>>
>> Matt Ranostay (2):
>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>   iio: potentiostat: add LMP91000 support
>>
>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>  drivers/iio/Kconfig                                |   1 +
>>  drivers/iio/Makefile                               |   1 +
>>  drivers/iio/adc/Kconfig                            |  12 +
>>  drivers/iio/adc/Makefile                           |   1 +
>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>  10 files changed, 622 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>
>> --
>> 2.7.4
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Matt Ranostay]

On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 02/07/16 23:13, Matt Ranostay wrote:
>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>> 16-bit ADC for chemical sensoring applications.
>>>
>>>  * add support for the TI LMP91000 potentiostat
>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>
>> Probably should have put what I am RFC'ing :).
>>
>> * does this belong in a new path drivers/iio/potentiostat ?
> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
> as well to take the only similar driver we already have.
>
>> * first example of a iio consumer within drivers/iio, does it seems sane?
> It's 'interesting'.  You've worked around the whole question of how to handle
> a mux by putting a push interface equipped client on top of the polled interface
> of the ADC.  It's an elegant solution that I'd never considered.
>
> By the very nature of a mux interface, unless we are piping the mux switching
> out on the same trigger system as the read back, the actual read out must be
> polled rather than self clocked. Only the mux knows when it is ready.
> The triggered version has all sorts of additional complexity even if we had
> output buffers already to go (such as requiring the output buffering to
> 'lead' the input buffering to give the mux time to switch.
>
> Question to my mind is whether this is a generic and flexible enough approach
> to use for this sort of device in the future... I think we have two classes
> of 'analog device' that we need to support:
>
> 1) Simple all channels always there devices such as analog accelerometers
> feeding into an ADC with a sequencer (or a software based sequencer).
> In that case the data flow is clearly going to go over the buffered interface.
> The accelerometer driver is just massaging the data for types / scale adjustments.
> It has no influence on the sampling of the data.
>
> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
> is driven by the front end, not the ADC.  Games could be played to push the
> sampling of data over to the ADC, but is it worth doing?
>

Probably over-engineering unless we actually find a need to do this in
the future.

> So if we wanted to do this, the AFE could itself export a trigger that is then
> used by the ADC which in turn pushes data back to the AFE driver via the buffered
> data interface.  The AFE driver would then have to handle the demux of this
> data stream into a coherent form to push out in it's own buffer.
> This approach gains the following:
>  - quick data transfers, particularly if we are dealing with a multiple output
>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>    The mux driver would then have to recombine these 16 channels before kicking
>    them out.

Makes sense but there is a slight issue of the settling time for the
temp channel is 2-3 milliseconds. Can't assume all mux reads are going
to take the same time constant.

>
> To do this we'd need to add an interface to allow the AFE/mux driver to set
> the trigger for the ADC to its own.

Of course in this case the ADC and LMP91000 device are using both the
hrtimer trigger, albeit of course you can't do it at the same time. So
it is polling no matter what.

>
> If we want to do this quickish I think that's about the lightest weight option
> we can do.
>
> Now, the question is, what are the disadvantages of going with what you
> have here for this driver but keeping in mind the above for when it matters?
>
> I'm guessing we never need to run this particularly driver very fast...
> I'm inclined to say yes but would like some other opinions on this one!
> (hence the expanded Cc list - please do pull in anyone else you think
> might be interested!)

Yeah the sample response of the sensor isn't that high speed. Maybe a
few dozen hertz.

>
>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
> Should be some defined. That was easy ;)
>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
> err. Odd. Go with signed I think.
>>
>>>
>>> Matt Ranostay (2):
>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>   iio: potentiostat: add LMP91000 support
>>>
>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>  drivers/iio/Kconfig                                |   1 +
>>>  drivers/iio/Makefile                               |   1 +
>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>  drivers/iio/adc/Makefile                           |   1 +
>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>  10 files changed, 622 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>
>>> --
>>> 2.7.4
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Jonathan Cameron]

On 03/07/16 23:24, Matt Ranostay wrote:
> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 02/07/16 23:13, Matt Ranostay wrote:
>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>> 16-bit ADC for chemical sensoring applications.
>>>>
>>>>  * add support for the TI LMP91000 potentiostat
>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>
>>> Probably should have put what I am RFC'ing :).
>>>
>>> * does this belong in a new path drivers/iio/potentiostat ?
>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>> as well to take the only similar driver we already have.
>>
>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>> It's 'interesting'.  You've worked around the whole question of how to handle
>> a mux by putting a push interface equipped client on top of the polled interface
>> of the ADC.  It's an elegant solution that I'd never considered.
>>
>> By the very nature of a mux interface, unless we are piping the mux switching
>> out on the same trigger system as the read back, the actual read out must be
>> polled rather than self clocked. Only the mux knows when it is ready.
>> The triggered version has all sorts of additional complexity even if we had
>> output buffers already to go (such as requiring the output buffering to
>> 'lead' the input buffering to give the mux time to switch.
>>
>> Question to my mind is whether this is a generic and flexible enough approach
>> to use for this sort of device in the future... I think we have two classes
>> of 'analog device' that we need to support:
>>
>> 1) Simple all channels always there devices such as analog accelerometers
>> feeding into an ADC with a sequencer (or a software based sequencer).
>> In that case the data flow is clearly going to go over the buffered interface.
>> The accelerometer driver is just massaging the data for types / scale adjustments.
>> It has no influence on the sampling of the data.
>>
>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>> is driven by the front end, not the ADC.  Games could be played to push the
>> sampling of data over to the ADC, but is it worth doing?
>>
> 
> Probably over-engineering unless we actually find a need to do this in
> the future.
I get the feeling we'll end up with a high performance system needing this
at some point.
> 
>> So if we wanted to do this, the AFE could itself export a trigger that is then
>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>> data interface.  The AFE driver would then have to handle the demux of this
>> data stream into a coherent form to push out in it's own buffer.
>> This approach gains the following:
>>  - quick data transfers, particularly if we are dealing with a multiple output
>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>    The mux driver would then have to recombine these 16 channels before kicking
>>    them out.
> 
> Makes sense but there is a slight issue of the settling time for the
> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
> to take the same time constant.
That's down to the mux driver to handle it.  Only trigger once it's known
to be stable..
> 
>>
>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>> the trigger for the ADC to its own.
> 
> Of course in this case the ADC and LMP91000 device are using both the
> hrtimer trigger, albeit of course you can't do it at the same time. So
> it is polling no matter what.
Fair point.
> 
>>
>> If we want to do this quickish I think that's about the lightest weight option
>> we can do.
>>
>> Now, the question is, what are the disadvantages of going with what you
>> have here for this driver but keeping in mind the above for when it matters?
>>
>> I'm guessing we never need to run this particularly driver very fast...
>> I'm inclined to say yes but would like some other opinions on this one!
>> (hence the expanded Cc list - please do pull in anyone else you think
>> might be interested!)
> 
> Yeah the sample response of the sensor isn't that high speed. Maybe a
> few dozen hertz.
> 
>>
>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>> Should be some defined. That was easy ;)
>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>> err. Odd. Go with signed I think.
>>>
>>>>
>>>> Matt Ranostay (2):
>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>   iio: potentiostat: add LMP91000 support
>>>>
>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>  drivers/iio/Kconfig                                |   1 +
>>>>  drivers/iio/Makefile                               |   1 +
>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>  10 files changed, 622 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Matt Ranostay]

On Tue, Jul 5, 2016 at 12:56 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 03/07/16 23:24, Matt Ranostay wrote:
>> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 02/07/16 23:13, Matt Ranostay wrote:
>>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>>> 16-bit ADC for chemical sensoring applications.
>>>>>
>>>>>  * add support for the TI LMP91000 potentiostat
>>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>>
>>>> Probably should have put what I am RFC'ing :).
>>>>
>>>> * does this belong in a new path drivers/iio/potentiostat ?
>>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>>> as well to take the only similar driver we already have.
>>>
>>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>>> It's 'interesting'.  You've worked around the whole question of how to handle
>>> a mux by putting a push interface equipped client on top of the polled interface
>>> of the ADC.  It's an elegant solution that I'd never considered.
>>>
>>> By the very nature of a mux interface, unless we are piping the mux switching
>>> out on the same trigger system as the read back, the actual read out must be
>>> polled rather than self clocked. Only the mux knows when it is ready.
>>> The triggered version has all sorts of additional complexity even if we had
>>> output buffers already to go (such as requiring the output buffering to
>>> 'lead' the input buffering to give the mux time to switch.
>>>
>>> Question to my mind is whether this is a generic and flexible enough approach
>>> to use for this sort of device in the future... I think we have two classes
>>> of 'analog device' that we need to support:
>>>
>>> 1) Simple all channels always there devices such as analog accelerometers
>>> feeding into an ADC with a sequencer (or a software based sequencer).
>>> In that case the data flow is clearly going to go over the buffered interface.
>>> The accelerometer driver is just massaging the data for types / scale adjustments.
>>> It has no influence on the sampling of the data.
>>>
>>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>>> is driven by the front end, not the ADC.  Games could be played to push the
>>> sampling of data over to the ADC, but is it worth doing?
>>>
>>
>> Probably over-engineering unless we actually find a need to do this in
>> the future.
> I get the feeling we'll end up with a high performance system needing this
> at some point.
>>
>>> So if we wanted to do this, the AFE could itself export a trigger that is then
>>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>>> data interface.  The AFE driver would then have to handle the demux of this
>>> data stream into a coherent form to push out in it's own buffer.
>>> This approach gains the following:
>>>  - quick data transfers, particularly if we are dealing with a multiple output
>>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>>    The mux driver would then have to recombine these 16 channels before kicking
>>>    them out.
>>
>> Makes sense but there is a slight issue of the settling time for the
>> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
>> to take the same time constant.
> That's down to the mux driver to handle it.  Only trigger once it's known
> to be stable..

Also how would the ADC report the data back it would almost need
dynamically setup iio channels after mux gets setup, correct?

1) ADC driver probe
2) MUX driver probe
3) MUX registers it's data channels
4) ADC driver needs to enumerate them

>>
>>>
>>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>>> the trigger for the ADC to its own.
>>
>> Of course in this case the ADC and LMP91000 device are using both the
>> hrtimer trigger, albeit of course you can't do it at the same time. So
>> it is polling no matter what.
> Fair point.
>>
>>>
>>> If we want to do this quickish I think that's about the lightest weight option
>>> we can do.
>>>
>>> Now, the question is, what are the disadvantages of going with what you
>>> have here for this driver but keeping in mind the above for when it matters?
>>>
>>> I'm guessing we never need to run this particularly driver very fast...
>>> I'm inclined to say yes but would like some other opinions on this one!
>>> (hence the expanded Cc list - please do pull in anyone else you think
>>> might be interested!)
>>
>> Yeah the sample response of the sensor isn't that high speed. Maybe a
>> few dozen hertz.
>>
>>>
>>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>>> Should be some defined. That was easy ;)
>>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>>> err. Odd. Go with signed I think.
>>>>
>>>>>
>>>>> Matt Ranostay (2):
>>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>>   iio: potentiostat: add LMP91000 support
>>>>>
>>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>>  10 files changed, 622 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>>
>>>>> --
>>>>> 2.7.4
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Jonathan Cameron]

On 05/07/16 22:27, Matt Ranostay wrote:
> On Tue, Jul 5, 2016 at 12:56 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 03/07/16 23:24, Matt Ranostay wrote:
>>> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 02/07/16 23:13, Matt Ranostay wrote:
>>>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>>>> 16-bit ADC for chemical sensoring applications.
>>>>>>
>>>>>>  * add support for the TI LMP91000 potentiostat
>>>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>>>
>>>>> Probably should have put what I am RFC'ing :).
>>>>>
>>>>> * does this belong in a new path drivers/iio/potentiostat ?
>>>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>>>> as well to take the only similar driver we already have.
>>>>
>>>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>>>> It's 'interesting'.  You've worked around the whole question of how to handle
>>>> a mux by putting a push interface equipped client on top of the polled interface
>>>> of the ADC.  It's an elegant solution that I'd never considered.
>>>>
>>>> By the very nature of a mux interface, unless we are piping the mux switching
>>>> out on the same trigger system as the read back, the actual read out must be
>>>> polled rather than self clocked. Only the mux knows when it is ready.
>>>> The triggered version has all sorts of additional complexity even if we had
>>>> output buffers already to go (such as requiring the output buffering to
>>>> 'lead' the input buffering to give the mux time to switch.
>>>>
>>>> Question to my mind is whether this is a generic and flexible enough approach
>>>> to use for this sort of device in the future... I think we have two classes
>>>> of 'analog device' that we need to support:
>>>>
>>>> 1) Simple all channels always there devices such as analog accelerometers
>>>> feeding into an ADC with a sequencer (or a software based sequencer).
>>>> In that case the data flow is clearly going to go over the buffered interface.
>>>> The accelerometer driver is just massaging the data for types / scale adjustments.
>>>> It has no influence on the sampling of the data.
>>>>
>>>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>>>> is driven by the front end, not the ADC.  Games could be played to push the
>>>> sampling of data over to the ADC, but is it worth doing?
>>>>
>>>
>>> Probably over-engineering unless we actually find a need to do this in
>>> the future.
>> I get the feeling we'll end up with a high performance system needing this
>> at some point.
>>>
>>>> So if we wanted to do this, the AFE could itself export a trigger that is then
>>>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>>>> data interface.  The AFE driver would then have to handle the demux of this
>>>> data stream into a coherent form to push out in it's own buffer.
>>>> This approach gains the following:
>>>>  - quick data transfers, particularly if we are dealing with a multiple output
>>>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>>>    The mux driver would then have to recombine these 16 channels before kicking
>>>>    them out.
>>>
>>> Makes sense but there is a slight issue of the settling time for the
>>> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
>>> to take the same time constant.
>> That's down to the mux driver to handle it.  Only trigger once it's known
>> to be stable..
> 
> Also how would the ADC report the data back it would almost need
> dynamically setup iio channels after mux gets setup, correct?
> 
> 1) ADC driver probe
> 2) MUX driver probe
> 3) MUX registers it's data channels
> 4) ADC driver needs to enumerate them
Why does the ADC driver care?

The Mux driver is the only bit that knows what the ADC is actually capturing
as it controls both the actual wire connections and the reporting to userspace
of results.

So.
1) ADC driver probe
2) Mux driver probe (gets provided ADC channels - however many it controls
the inputs for).
3) ADC trigger set to trigger provided by Mux.

To give a simple example, lets consider a 2 input single channel mux going into
a single channel ADC. Mux trigger called (imaginatively) mux_trig0

Mux is consumer of the ADC channel.

Setup:
1) Mux registers as a 'buffer' consumer of the ADC channel.
2) Mux has a trigger exposed (which is how it controls the capture.)
3) ADC trigger set to the mux exposed one.

A scan. (triggered say by a high resolution timer trigger).
1) Mux picks channel 1 and waits for it to stabilize.
2) Mux 'fires' trigger to initiate a capture and gets the resulting callback
call with the value.  Stashes it somewhere
3) Mux selects channel 2 and waits for it to stabilize
4) Mux 'fires' trigger to initiate a capture and gets the resulting callback
call with the value. Stashes it somewhere.
5) Mux driver can then combine the two values to form the 'scan' and push
that to it's buffer complete with whatever timestamp makes sense.
This Mux driver controlled buffer is the one userspace uses to get the data.

Missing bit of all this is a consumer being able to control the providers
trigger.  Doubt that would be hard to add.

I think that covers all possible circumstances where we have explicit
control of the mux or at least the ability to set it to a known state.

Jonathan

> 
>>>
>>>>
>>>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>>>> the trigger for the ADC to its own.
>>>
>>> Of course in this case the ADC and LMP91000 device are using both the
>>> hrtimer trigger, albeit of course you can't do it at the same time. So
>>> it is polling no matter what.
>> Fair point.
>>>
>>>>
>>>> If we want to do this quickish I think that's about the lightest weight option
>>>> we can do.
>>>>
>>>> Now, the question is, what are the disadvantages of going with what you
>>>> have here for this driver but keeping in mind the above for when it matters?
>>>>
>>>> I'm guessing we never need to run this particularly driver very fast...
>>>> I'm inclined to say yes but would like some other opinions on this one!
>>>> (hence the expanded Cc list - please do pull in anyone else you think
>>>> might be interested!)
>>>
>>> Yeah the sample response of the sensor isn't that high speed. Maybe a
>>> few dozen hertz.
>>>
>>>>
>>>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>>>> Should be some defined. That was easy ;)
>>>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>>>> err. Odd. Go with signed I think.
>>>>>
>>>>>>
>>>>>> Matt Ranostay (2):
>>>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>>>   iio: potentiostat: add LMP91000 support
>>>>>>
>>>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>>>  10 files changed, 622 insertions(+)
>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>>>
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Matt Ranostay]

On Sun, Jul 10, 2016 at 7:20 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 05/07/16 22:27, Matt Ranostay wrote:
>> On Tue, Jul 5, 2016 at 12:56 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 03/07/16 23:24, Matt Ranostay wrote:
>>>> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 02/07/16 23:13, Matt Ranostay wrote:
>>>>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>>>>> 16-bit ADC for chemical sensoring applications.
>>>>>>>
>>>>>>>  * add support for the TI LMP91000 potentiostat
>>>>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>>>>
>>>>>> Probably should have put what I am RFC'ing :).
>>>>>>
>>>>>> * does this belong in a new path drivers/iio/potentiostat ?
>>>>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>>>>> as well to take the only similar driver we already have.
>>>>>
>>>>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>>>>> It's 'interesting'.  You've worked around the whole question of how to handle
>>>>> a mux by putting a push interface equipped client on top of the polled interface
>>>>> of the ADC.  It's an elegant solution that I'd never considered.
>>>>>
>>>>> By the very nature of a mux interface, unless we are piping the mux switching
>>>>> out on the same trigger system as the read back, the actual read out must be
>>>>> polled rather than self clocked. Only the mux knows when it is ready.
>>>>> The triggered version has all sorts of additional complexity even if we had
>>>>> output buffers already to go (such as requiring the output buffering to
>>>>> 'lead' the input buffering to give the mux time to switch.
>>>>>
>>>>> Question to my mind is whether this is a generic and flexible enough approach
>>>>> to use for this sort of device in the future... I think we have two classes
>>>>> of 'analog device' that we need to support:
>>>>>
>>>>> 1) Simple all channels always there devices such as analog accelerometers
>>>>> feeding into an ADC with a sequencer (or a software based sequencer).
>>>>> In that case the data flow is clearly going to go over the buffered interface.
>>>>> The accelerometer driver is just massaging the data for types / scale adjustments.
>>>>> It has no influence on the sampling of the data.
>>>>>
>>>>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>>>>> is driven by the front end, not the ADC.  Games could be played to push the
>>>>> sampling of data over to the ADC, but is it worth doing?
>>>>>
>>>>
>>>> Probably over-engineering unless we actually find a need to do this in
>>>> the future.
>>> I get the feeling we'll end up with a high performance system needing this
>>> at some point.
>>>>
>>>>> So if we wanted to do this, the AFE could itself export a trigger that is then
>>>>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>>>>> data interface.  The AFE driver would then have to handle the demux of this
>>>>> data stream into a coherent form to push out in it's own buffer.
>>>>> This approach gains the following:
>>>>>  - quick data transfers, particularly if we are dealing with a multiple output
>>>>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>>>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>>>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>>>>    The mux driver would then have to recombine these 16 channels before kicking
>>>>>    them out.
>>>>
>>>> Makes sense but there is a slight issue of the settling time for the
>>>> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
>>>> to take the same time constant.
>>> That's down to the mux driver to handle it.  Only trigger once it's known
>>> to be stable..
>>
>> Also how would the ADC report the data back it would almost need
>> dynamically setup iio channels after mux gets setup, correct?
>>
>> 1) ADC driver probe
>> 2) MUX driver probe
>> 3) MUX registers it's data channels
>> 4) ADC driver needs to enumerate them
> Why does the ADC driver care?
>
> The Mux driver is the only bit that knows what the ADC is actually capturing
> as it controls both the actual wire connections and the reporting to userspace
> of results.
>
> So.
> 1) ADC driver probe
> 2) Mux driver probe (gets provided ADC channels - however many it controls
> the inputs for).
> 3) ADC trigger set to trigger provided by Mux.
>
> To give a simple example, lets consider a 2 input single channel mux going into
> a single channel ADC. Mux trigger called (imaginatively) mux_trig0
>
> Mux is consumer of the ADC channel.
>
> Setup:
> 1) Mux registers as a 'buffer' consumer of the ADC channel.
> 2) Mux has a trigger exposed (which is how it controls the capture.)
> 3) ADC trigger set to the mux exposed one.
>
> A scan. (triggered say by a high resolution timer trigger).
> 1) Mux picks channel 1 and waits for it to stabilize.
> 2) Mux 'fires' trigger to initiate a capture and gets the resulting callback
> call with the value.  Stashes it somewhere
> 3) Mux selects channel 2 and waits for it to stabilize
> 4) Mux 'fires' trigger to initiate a capture and gets the resulting callback
> call with the value. Stashes it somewhere.
> 5) Mux driver can then combine the two values to form the 'scan' and push
> that to it's buffer complete with whatever timestamp makes sense.
> This Mux driver controlled buffer is the one userspace uses to get the data.
>

Only question is how does the callback come into play here with a
trigger? Not sure I have seen this in the API so far.

> Missing bit of all this is a consumer being able to control the providers
> trigger.  Doubt that would be hard to add.
>

Worse case initially it will have to be manually set.

> I think that covers all possible circumstances where we have explicit
> control of the mux or at least the ability to set it to a known state.
>
> Jonathan
>
>>
>>>>
>>>>>
>>>>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>>>>> the trigger for the ADC to its own.
>>>>
>>>> Of course in this case the ADC and LMP91000 device are using both the
>>>> hrtimer trigger, albeit of course you can't do it at the same time. So
>>>> it is polling no matter what.
>>> Fair point.
>>>>
>>>>>
>>>>> If we want to do this quickish I think that's about the lightest weight option
>>>>> we can do.
>>>>>
>>>>> Now, the question is, what are the disadvantages of going with what you
>>>>> have here for this driver but keeping in mind the above for when it matters?
>>>>>
>>>>> I'm guessing we never need to run this particularly driver very fast...
>>>>> I'm inclined to say yes but would like some other opinions on this one!
>>>>> (hence the expanded Cc list - please do pull in anyone else you think
>>>>> might be interested!)
>>>>
>>>> Yeah the sample response of the sensor isn't that high speed. Maybe a
>>>> few dozen hertz.
>>>>
>>>>>
>>>>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>>>>> Should be some defined. That was easy ;)
>>>>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>>>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>>>>> err. Odd. Go with signed I think.
>>>>>>
>>>>>>>
>>>>>>> Matt Ranostay (2):
>>>>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>>>>   iio: potentiostat: add LMP91000 support
>>>>>>>
>>>>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>>>>  10 files changed, 622 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>>>>
>>>>>>> --
>>>>>>> 2.7.4
>>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Jonathan Cameron]

On 14/07/16 04:36, Matt Ranostay wrote:
> On Sun, Jul 10, 2016 at 7:20 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 05/07/16 22:27, Matt Ranostay wrote:
>>> On Tue, Jul 5, 2016 at 12:56 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 03/07/16 23:24, Matt Ranostay wrote:
>>>>> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>> On 02/07/16 23:13, Matt Ranostay wrote:
>>>>>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>>>>>> 16-bit ADC for chemical sensoring applications.
>>>>>>>>
>>>>>>>>  * add support for the TI LMP91000 potentiostat
>>>>>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>>>>>
>>>>>>> Probably should have put what I am RFC'ing :).
>>>>>>>
>>>>>>> * does this belong in a new path drivers/iio/potentiostat ?
>>>>>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>>>>>> as well to take the only similar driver we already have.
>>>>>>
>>>>>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>>>>>> It's 'interesting'.  You've worked around the whole question of how to handle
>>>>>> a mux by putting a push interface equipped client on top of the polled interface
>>>>>> of the ADC.  It's an elegant solution that I'd never considered.
>>>>>>
>>>>>> By the very nature of a mux interface, unless we are piping the mux switching
>>>>>> out on the same trigger system as the read back, the actual read out must be
>>>>>> polled rather than self clocked. Only the mux knows when it is ready.
>>>>>> The triggered version has all sorts of additional complexity even if we had
>>>>>> output buffers already to go (such as requiring the output buffering to
>>>>>> 'lead' the input buffering to give the mux time to switch.
>>>>>>
>>>>>> Question to my mind is whether this is a generic and flexible enough approach
>>>>>> to use for this sort of device in the future... I think we have two classes
>>>>>> of 'analog device' that we need to support:
>>>>>>
>>>>>> 1) Simple all channels always there devices such as analog accelerometers
>>>>>> feeding into an ADC with a sequencer (or a software based sequencer).
>>>>>> In that case the data flow is clearly going to go over the buffered interface.
>>>>>> The accelerometer driver is just massaging the data for types / scale adjustments.
>>>>>> It has no influence on the sampling of the data.
>>>>>>
>>>>>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>>>>>> is driven by the front end, not the ADC.  Games could be played to push the
>>>>>> sampling of data over to the ADC, but is it worth doing?
>>>>>>
>>>>>
>>>>> Probably over-engineering unless we actually find a need to do this in
>>>>> the future.
>>>> I get the feeling we'll end up with a high performance system needing this
>>>> at some point.
>>>>>
>>>>>> So if we wanted to do this, the AFE could itself export a trigger that is then
>>>>>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>>>>>> data interface.  The AFE driver would then have to handle the demux of this
>>>>>> data stream into a coherent form to push out in it's own buffer.
>>>>>> This approach gains the following:
>>>>>>  - quick data transfers, particularly if we are dealing with a multiple output
>>>>>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>>>>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>>>>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>>>>>    The mux driver would then have to recombine these 16 channels before kicking
>>>>>>    them out.
>>>>>
>>>>> Makes sense but there is a slight issue of the settling time for the
>>>>> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
>>>>> to take the same time constant.
>>>> That's down to the mux driver to handle it.  Only trigger once it's known
>>>> to be stable..
>>>
>>> Also how would the ADC report the data back it would almost need
>>> dynamically setup iio channels after mux gets setup, correct?
>>>
>>> 1) ADC driver probe
>>> 2) MUX driver probe
>>> 3) MUX registers it's data channels
>>> 4) ADC driver needs to enumerate them
>> Why does the ADC driver care?
>>
>> The Mux driver is the only bit that knows what the ADC is actually capturing
>> as it controls both the actual wire connections and the reporting to userspace
>> of results.
>>
>> So.
>> 1) ADC driver probe
>> 2) Mux driver probe (gets provided ADC channels - however many it controls
>> the inputs for).
>> 3) ADC trigger set to trigger provided by Mux.
>>
>> To give a simple example, lets consider a 2 input single channel mux going into
>> a single channel ADC. Mux trigger called (imaginatively) mux_trig0
>>
>> Mux is consumer of the ADC channel.
>>
>> Setup:
>> 1) Mux registers as a 'buffer' consumer of the ADC channel.
>> 2) Mux has a trigger exposed (which is how it controls the capture.)
>> 3) ADC trigger set to the mux exposed one.
>>
>> A scan. (triggered say by a high resolution timer trigger).
>> 1) Mux picks channel 1 and waits for it to stabilize.
>> 2) Mux 'fires' trigger to initiate a capture and gets the resulting callback
>> call with the value.  Stashes it somewhere
>> 3) Mux selects channel 2 and waits for it to stabilize
>> 4) Mux 'fires' trigger to initiate a capture and gets the resulting callback
>> call with the value. Stashes it somewhere.
>> 5) Mux driver can then combine the two values to form the 'scan' and push
>> that to it's buffer complete with whatever timestamp makes sense.
>> This Mux driver controlled buffer is the one userspace uses to get the data.
>>
> 
> Only question is how does the callback come into play here with a
> trigger? Not sure I have seen this in the API so far.
The ABI has been there a long time - just not much used :)
See buffers\industrialio-buffer-cb.c

Was original written to support pushing data out to the iio-input
bridge driver (which was last posted in 2012)...

Just got used (slightly wrongly) in the sunxi touchscreen driver
as well.
> 
>> Missing bit of all this is a consumer being able to control the providers
>> trigger.  Doubt that would be hard to add.
>>
> 
> Worse case initially it will have to be manually set.
I think we'd have to automate it or this would all be really clunky
to use from userspace.
> 
>> I think that covers all possible circumstances where we have explicit
>> control of the mux or at least the ability to set it to a known state.
>>
>> Jonathan
>>
>>>
>>>>>
>>>>>>
>>>>>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>>>>>> the trigger for the ADC to its own.
>>>>>
>>>>> Of course in this case the ADC and LMP91000 device are using both the
>>>>> hrtimer trigger, albeit of course you can't do it at the same time. So
>>>>> it is polling no matter what.
>>>> Fair point.
>>>>>
>>>>>>
>>>>>> If we want to do this quickish I think that's about the lightest weight option
>>>>>> we can do.
>>>>>>
>>>>>> Now, the question is, what are the disadvantages of going with what you
>>>>>> have here for this driver but keeping in mind the above for when it matters?
>>>>>>
>>>>>> I'm guessing we never need to run this particularly driver very fast...
>>>>>> I'm inclined to say yes but would like some other opinions on this one!
>>>>>> (hence the expanded Cc list - please do pull in anyone else you think
>>>>>> might be interested!)
>>>>>
>>>>> Yeah the sample response of the sensor isn't that high speed. Maybe a
>>>>> few dozen hertz.
>>>>>
>>>>>>
>>>>>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>>>>>> Should be some defined. That was easy ;)
>>>>>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>>>>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>>>>>> err. Odd. Go with signed I think.
>>>>>>>
>>>>>>>>
>>>>>>>> Matt Ranostay (2):
>>>>>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>>>>>   iio: potentiostat: add LMP91000 support
>>>>>>>>
>>>>>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>>>>>  10 files changed, 622 insertions(+)
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>>>>>
>>>>>>>> --
>>>>>>>> 2.7.4
>>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 0/2] iio: add support for LMP91000EVM potentiostat board

[Matt Ranostay]

On Sun, Jul 24, 2016 at 5:25 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 14/07/16 04:36, Matt Ranostay wrote:
>> On Sun, Jul 10, 2016 at 7:20 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 05/07/16 22:27, Matt Ranostay wrote:
>>>> On Tue, Jul 5, 2016 at 12:56 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 03/07/16 23:24, Matt Ranostay wrote:
>>>>>> On Sun, Jul 3, 2016 at 6:41 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>>> On 02/07/16 23:13, Matt Ranostay wrote:
>>>>>>>> On Sat, Jul 2, 2016 at 3:05 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>>>>>>>>> LMP91000EVM evaluation board has LMP91000 potentiostat along with an
>>>>>>>>> 16-bit ADC for chemical sensoring applications.
>>>>>>>>>
>>>>>>>>>  * add support for the TI LMP91000 potentiostat
>>>>>>>>>  * add support for ADC141S626 and ADC161S626 ADC chips
>>>>>>>>
>>>>>>>> Probably should have put what I am RFC'ing :).
>>>>>>>>
>>>>>>>> * does this belong in a new path drivers/iio/potentiostat ?
>>>>>>> I'm going for drivers/iio/AFE/potentiostat with drivers/iio/AFE/amplifiers
>>>>>>> as well to take the only similar driver we already have.
>>>>>>>
>>>>>>>> * first example of a iio consumer within drivers/iio, does it seems sane?
>>>>>>> It's 'interesting'.  You've worked around the whole question of how to handle
>>>>>>> a mux by putting a push interface equipped client on top of the polled interface
>>>>>>> of the ADC.  It's an elegant solution that I'd never considered.
>>>>>>>
>>>>>>> By the very nature of a mux interface, unless we are piping the mux switching
>>>>>>> out on the same trigger system as the read back, the actual read out must be
>>>>>>> polled rather than self clocked. Only the mux knows when it is ready.
>>>>>>> The triggered version has all sorts of additional complexity even if we had
>>>>>>> output buffers already to go (such as requiring the output buffering to
>>>>>>> 'lead' the input buffering to give the mux time to switch.
>>>>>>>
>>>>>>> Question to my mind is whether this is a generic and flexible enough approach
>>>>>>> to use for this sort of device in the future... I think we have two classes
>>>>>>> of 'analog device' that we need to support:
>>>>>>>
>>>>>>> 1) Simple all channels always there devices such as analog accelerometers
>>>>>>> feeding into an ADC with a sequencer (or a software based sequencer).
>>>>>>> In that case the data flow is clearly going to go over the buffered interface.
>>>>>>> The accelerometer driver is just massaging the data for types / scale adjustments.
>>>>>>> It has no influence on the sampling of the data.
>>>>>>>
>>>>>>> 2) The 'smart' front end with a mux.  In this case the 'when to read' question
>>>>>>> is driven by the front end, not the ADC.  Games could be played to push the
>>>>>>> sampling of data over to the ADC, but is it worth doing?
>>>>>>>
>>>>>>
>>>>>> Probably over-engineering unless we actually find a need to do this in
>>>>>> the future.
>>>>> I get the feeling we'll end up with a high performance system needing this
>>>>> at some point.
>>>>>>
>>>>>>> So if we wanted to do this, the AFE could itself export a trigger that is then
>>>>>>> used by the ADC which in turn pushes data back to the AFE driver via the buffered
>>>>>>> data interface.  The AFE driver would then have to handle the demux of this
>>>>>>> data stream into a coherent form to push out in it's own buffer.
>>>>>>> This approach gains the following:
>>>>>>>  - quick data transfers, particularly if we are dealing with a multiple output
>>>>>>>    mux.  e.g we might have a 16 to 4 mux into a 4 channel simultaneous sampling
>>>>>>>    (or sequenced) ADC. So in this case if the mux was set to provide all 16
>>>>>>>    channels in order we'd do 4 reads of the ADC getting 0 1 2 3, 4 5 6 7 etc.
>>>>>>>    The mux driver would then have to recombine these 16 channels before kicking
>>>>>>>    them out.
>>>>>>
>>>>>> Makes sense but there is a slight issue of the settling time for the
>>>>>> temp channel is 2-3 milliseconds. Can't assume all mux reads are going
>>>>>> to take the same time constant.
>>>>> That's down to the mux driver to handle it.  Only trigger once it's known
>>>>> to be stable..
>>>>
>>>> Also how would the ADC report the data back it would almost need
>>>> dynamically setup iio channels after mux gets setup, correct?
>>>>
>>>> 1) ADC driver probe
>>>> 2) MUX driver probe
>>>> 3) MUX registers it's data channels
>>>> 4) ADC driver needs to enumerate them
>>> Why does the ADC driver care?
>>>
>>> The Mux driver is the only bit that knows what the ADC is actually capturing
>>> as it controls both the actual wire connections and the reporting to userspace
>>> of results.
>>>
>>> So.
>>> 1) ADC driver probe
>>> 2) Mux driver probe (gets provided ADC channels - however many it controls
>>> the inputs for).
>>> 3) ADC trigger set to trigger provided by Mux.
>>>
>>> To give a simple example, lets consider a 2 input single channel mux going into
>>> a single channel ADC. Mux trigger called (imaginatively) mux_trig0
>>>
>>> Mux is consumer of the ADC channel.
>>>
>>> Setup:
>>> 1) Mux registers as a 'buffer' consumer of the ADC channel.
>>> 2) Mux has a trigger exposed (which is how it controls the capture.)
>>> 3) ADC trigger set to the mux exposed one.
>>>
>>> A scan. (triggered say by a high resolution timer trigger).
>>> 1) Mux picks channel 1 and waits for it to stabilize.
>>> 2) Mux 'fires' trigger to initiate a capture and gets the resulting callback
>>> call with the value.  Stashes it somewhere
>>> 3) Mux selects channel 2 and waits for it to stabilize
>>> 4) Mux 'fires' trigger to initiate a capture and gets the resulting callback
>>> call with the value. Stashes it somewhere.
>>> 5) Mux driver can then combine the two values to form the 'scan' and push
>>> that to it's buffer complete with whatever timestamp makes sense.
>>> This Mux driver controlled buffer is the one userspace uses to get the data.
>>>
>>
>> Only question is how does the callback come into play here with a
>> trigger? Not sure I have seen this in the API so far.
> The ABI has been there a long time - just not much used :)
> See buffers\industrialio-buffer-cb.c

Gah right after I came up with a hackish way to do the same thing :)
Ah not ideally documented I see :)

>
> Was original written to support pushing data out to the iio-input
> bridge driver (which was last posted in 2012)...
>
> Just got used (slightly wrongly) in the sunxi touchscreen driver
> as well.
>>
>>> Missing bit of all this is a consumer being able to control the providers
>>> trigger.  Doubt that would be hard to add.
>>>
>>
>> Worse case initially it will have to be manually set.
> I think we'd have to automate it or this would all be really clunky
> to use from userspace.

Yeah but one issue is it possible the ADC may wanted to be used for
another application (has more than a few channels hooked into another
device for instance).

>>
>>> I think that covers all possible circumstances where we have explicit
>>> control of the mux or at least the ability to set it to a known state.
>>>
>>> Jonathan
>>>
>>>>
>>>>>>
>>>>>>>
>>>>>>> To do this we'd need to add an interface to allow the AFE/mux driver to set
>>>>>>> the trigger for the ADC to its own.
>>>>>>
>>>>>> Of course in this case the ADC and LMP91000 device are using both the
>>>>>> hrtimer trigger, albeit of course you can't do it at the same time. So
>>>>>> it is polling no matter what.
>>>>> Fair point.
>>>>>>
>>>>>>>
>>>>>>> If we want to do this quickish I think that's about the lightest weight option
>>>>>>> we can do.
>>>>>>>
>>>>>>> Now, the question is, what are the disadvantages of going with what you
>>>>>>> have here for this driver but keeping in mind the above for when it matters?
>>>>>>>
>>>>>>> I'm guessing we never need to run this particularly driver very fast...
>>>>>>> I'm inclined to say yes but would like some other opinions on this one!
>>>>>>> (hence the expanded Cc list - please do pull in anyone else you think
>>>>>>> might be interested!)
>>>>>>
>>>>>> Yeah the sample response of the sensor isn't that high speed. Maybe a
>>>>>> few dozen hertz.
>>>>>>
>>>>>>>
>>>>>>>> * ADC driver has no IIO_CHAN_INFO_SCALE due to no regulators being defined
>>>>>>> Should be some defined. That was easy ;)
>>>>>>>> * Should ADC value be signed or unsigned?   -16636 is 0V, 0 is 2/VA ,
>>>>>>>> 16635 is ~VA. Of course true zero is defined by the VREF voltage.
>>>>>>> err. Odd. Go with signed I think.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Matt Ranostay (2):
>>>>>>>>>   iio: adc: ti-adc1x1s: add support for TI 1-channel differential ADCs
>>>>>>>>>   iio: potentiostat: add LMP91000 support
>>>>>>>>>
>>>>>>>>>  .../devicetree/bindings/iio/adc/ti-adc1x1s.txt     |  16 ++
>>>>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  28 ++
>>>>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>>>>  drivers/iio/adc/Kconfig                            |  12 +
>>>>>>>>>  drivers/iio/adc/Makefile                           |   1 +
>>>>>>>>>  drivers/iio/adc/ti-adc1x1s.c                       | 233 ++++++++++++++++
>>>>>>>>>  drivers/iio/potentiostat/Kconfig                   |  21 ++
>>>>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 303 +++++++++++++++++++++
>>>>>>>>>  10 files changed, 622 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc1x1s.txt
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>>>  create mode 100644 drivers/iio/adc/ti-adc1x1s.c
>>>>>>>>>  create mode 100644 drivers/iio/potentiostat/Kconfig
>>>>>>>>>  create mode 100644 drivers/iio/potentiostat/Makefile
>>>>>>>>>  create mode 100644 drivers/iio/potentiostat/lmp91000.c
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 2.7.4
>>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>
>>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>