[RFC v4 0/3] iio: potentiostat: add LMP91000 support

[RFC v4 0/3] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

Changes from v1:
 * Rename ti-adc1x1s.c driver to ti-adc161s626.c
 * Switch from iio_channel_read() to using the industrialio-buffer-cb
   interface
 * Process data in ADC trigger handler for buffer callbacks
 * Set the mux trigger to the ADC referenced by io-channels by default

Changes from v2:
 * Fix configuration index shifting values
 * Switch from wait_for_completion_interruptible to wait_for_completion_timeout

Changes from v3:
 * Fix order of probe function error rollback

Matt Ranostay (3):
  iio: buffer-callback: allow getting underlying iio_dev
  iio: adc: ti-adc161s626: add support for TI 1-channel differential
    ADCs
  iio: potentiostat: add LMP91000 support

 .../devicetree/bindings/iio/adc/ti-adc161s626.txt  |  17 +
 .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/adc/Kconfig                            |  12 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ti-adc161s626.c                    | 248 ++++++++++++++
 drivers/iio/buffer/industrialio-buffer-cb.c        |  24 +-
 drivers/iio/potentiostat/Kconfig                   |  22 ++
 drivers/iio/potentiostat/Makefile                  |   6 +
 drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
 include/linux/iio/consumer.h                       |  12 +
 12 files changed, 741 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
 create mode 100644 Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
 create mode 100644 drivers/iio/adc/ti-adc161s626.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 v4 1/3] iio: buffer-callback: allow getting underlying iio_dev

[Matt Ranostay]

Add iio_channel_cb_get_iio_dev function to allow getting the
underlying iio_dev. This is useful for setting the trigger of the
consumer ADC device.

Signed-off-by: Matt Ranostay <mranostay@gmail.com>
---
 drivers/iio/buffer/industrialio-buffer-cb.c | 24 ++++++++++++++----------
 include/linux/iio/consumer.h                | 12 ++++++++++++
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
index 323079c3ccce..b8f550e47d3d 100644
--- a/drivers/iio/buffer/industrialio-buffer-cb.c
+++ b/drivers/iio/buffer/industrialio-buffer-cb.c
@@ -18,6 +18,7 @@ struct iio_cb_buffer {
 	int (*cb)(const void *data, void *private);
 	void *private;
 	struct iio_channel *channels;
+	struct iio_dev *indio_dev;
 };
 
 static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer)
@@ -52,7 +53,6 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
 {
 	int ret;
 	struct iio_cb_buffer *cb_buff;
-	struct iio_dev *indio_dev;
 	struct iio_channel *chan;
 
 	cb_buff = kzalloc(sizeof(*cb_buff), GFP_KERNEL);
@@ -72,17 +72,17 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
 		goto error_free_cb_buff;
 	}
 
-	indio_dev = cb_buff->channels[0].indio_dev;
+	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
 	cb_buff->buffer.scan_mask
-		= kcalloc(BITS_TO_LONGS(indio_dev->masklength), sizeof(long),
-			  GFP_KERNEL);
+		= kcalloc(BITS_TO_LONGS(cb_buff->indio_dev->masklength),
+			  sizeof(long), GFP_KERNEL);
 	if (cb_buff->buffer.scan_mask == NULL) {
 		ret = -ENOMEM;
 		goto error_release_channels;
 	}
 	chan = &cb_buff->channels[0];
 	while (chan->indio_dev) {
-		if (chan->indio_dev != indio_dev) {
+		if (chan->indio_dev != cb_buff->indio_dev) {
 			ret = -EINVAL;
 			goto error_free_scan_mask;
 		}
@@ -105,17 +105,14 @@ EXPORT_SYMBOL_GPL(iio_channel_get_all_cb);
 
 int iio_channel_start_all_cb(struct iio_cb_buffer *cb_buff)
 {
-	return iio_update_buffers(cb_buff->channels[0].indio_dev,
-				  &cb_buff->buffer,
+	return iio_update_buffers(cb_buff->indio_dev, &cb_buff->buffer,
 				  NULL);
 }
 EXPORT_SYMBOL_GPL(iio_channel_start_all_cb);
 
 void iio_channel_stop_all_cb(struct iio_cb_buffer *cb_buff)
 {
-	iio_update_buffers(cb_buff->channels[0].indio_dev,
-			   NULL,
-			   &cb_buff->buffer);
+	iio_update_buffers(cb_buff->indio_dev, NULL, &cb_buff->buffer);
 }
 EXPORT_SYMBOL_GPL(iio_channel_stop_all_cb);
 
@@ -133,6 +130,13 @@ struct iio_channel
 }
 EXPORT_SYMBOL_GPL(iio_channel_cb_get_channels);
 
+struct iio_dev
+*iio_channel_cb_get_iio_dev(const struct iio_cb_buffer *cb_buffer)
+{
+	return cb_buffer->indio_dev;
+}
+EXPORT_SYMBOL_GPL(iio_channel_cb_get_iio_dev);
+
 MODULE_AUTHOR("Jonathan Cameron <jic23@kernel.org>");
 MODULE_DESCRIPTION("Industrial I/O callback buffer");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 3d672f72e7ec..9edccfba1ffb 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -165,6 +165,18 @@ struct iio_channel
 *iio_channel_cb_get_channels(const struct iio_cb_buffer *cb_buffer);
 
 /**
+ * iio_channel_cb_get_iio_dev() - get access to the underlying device.
+ * @cb_buffer:		The callback buffer from whom we want the device
+ *			information.
+ *
+ * This function allows one to obtain information about the device.
+ * The primary aim is to allow drivers that are consuming a device to query
+ * things like current trigger.
+ */
+struct iio_dev
+*iio_channel_cb_get_iio_dev(const struct iio_cb_buffer *cb_buffer);
+
+/**
  * iio_read_channel_raw() - read from a given channel
  * @chan:		The channel being queried.
  * @val:		Value read back.
-- 
2.7.4

[RFC v4 2/3] iio: adc: ti-adc161s626: 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-adc161s626.txt  |  17 ++
 drivers/iio/adc/Kconfig                            |  12 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ti-adc161s626.c                    | 248 +++++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
 create mode 100644 drivers/iio/adc/ti-adc161s626.c

diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
new file mode 100644
index 000000000000..2bdad2d13d6f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
@@ -0,0 +1,17 @@
+* Texas Instruments ADC141S626, and ADC161S626 chip
+
+Required properties:
+ - compatible: Should be "ti,adc141s626" or "ti,adc161s626"
+ - 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 1de31bdd4ce4..2658f2f69bad 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -426,6 +426,18 @@ config TI_ADC128S052
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc128s052.
 
+config TI_ADC161S626
+	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
+	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-adc161s626.
+
 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 0ba0d500eedb..1d805770c1a0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -41,6 +41,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_ADC161S626) += ti-adc161s626.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-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
new file mode 100644
index 000000000000..f94b69f9c288
--- /dev/null
+++ b/drivers/iio/adc/ti-adc161s626.c
@@ -0,0 +1,248 @@
+/*
+ * ti-adc161s626.c - Texas Instruments ADC161S626 1-channel differential ADC
+ *
+ * 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	"ti-adc161s626"
+
+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 = 16,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+struct ti_adc_data {
+	struct iio_dev *indio_dev;
+	struct spi_device *spi;
+	u8 read_size;
+	u8 shift;
+
+	u8 buffer[16] ____cacheline_aligned;
+};
+
+static int ti_adc_read_measurement(struct ti_adc_data *data,
+				   struct iio_chan_spec const *chan, int *val)
+{
+	int ret;
+
+	switch (data->read_size) {
+	case 2: {
+		__be16 buf;
+
+		ret = spi_read(data->spi, (void *) &buf, 2);
+		if (ret)
+			return ret;
+
+		*val = be16_to_cpu(buf);
+		break;
+	}
+	case 3: {
+		__be32 buf;
+
+		ret = spi_read(data->spi, (void *) &buf, 3);
+		if (ret)
+			return ret;
+
+		*val = be32_to_cpu(buf) >> 8;
+		break;
+	}
+	default:
+		return -EINVAL;
+	}
+
+	*val = sign_extend32(*val >> data->shift, chan->scan_type.realbits - 1);
+
+	return 0;
+}
+
+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 = ti_adc_read_measurement(data, &indio_dev->channels[0],
+				     (int *) &data->buffer);
+	if (!ret)
+		iio_push_to_buffers_with_timestamp(indio_dev,
+					data->buffer,
+					iio_get_time_ns(indio_dev));
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+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;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	ret = ti_adc_read_measurement(data, chan, val);
+	iio_device_release_direct_mode(indio_dev);
+
+	if (!ret)
+		return IIO_VAL_INT;
+
+	return 0;
+}
+
+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->shift = 0;
+		data->read_size = 2;
+		break;
+	case TI_ADC161S626:
+		indio_dev->channels = ti_adc161s626_channels;
+		indio_dev->num_channels = ARRAY_SIZE(ti_adc161s626_channels);
+		data->shift = 6;
+		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

[RFC v4 3/3] 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         |  30 ++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/Makefile                               |   1 +
 drivers/iio/potentiostat/Kconfig                   |  22 ++
 drivers/iio/potentiostat/Makefile                  |   6 +
 drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
 6 files changed, 437 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..b9b621e94cd7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
@@ -0,0 +1,30 @@
+* 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 of the iio provider
+
+  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
+    needs to be set to signal that an external resistor value is being used.
+
+Optional properties:
+
+  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
+    amplifier. Must be 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 = <&adc>;
+};
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..1e3baf2cc97d
--- /dev/null
+++ b/drivers/iio/potentiostat/Kconfig
@@ -0,0 +1,22 @@
+#
+# 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_BUFFER_CB
+	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..5046d2d3da98
--- /dev/null
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -0,0 +1,377 @@
+/*
+ * 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.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_trigger *trig;
+	struct iio_cb_buffer *cb_buffer;
+
+	struct completion completion;
+	u8 chan_select;
+
+	u32 buffer[4]; /* 64-bit data + 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);
+
+	data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
+
+	iio_trigger_poll_chained(data->trig);
+
+	ret = wait_for_completion_timeout(&data->completion, HZ);
+	reinit_completion(&data->completion);
+
+	if (!ret)
+		return -ETIMEDOUT;
+
+	*val = data->buffer[data->chan_select];
+
+	return 0;
+}
+
+static irqreturn_t lmp91000_buffer_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;
+
+	memset(data->buffer, 0, sizeof(data->buffer));
+
+	ret = iio_channel_start_all_cb(data->cb_buffer);
+	if (ret)
+		goto error_out;
+
+	ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
+	iio_channel_stop_all_cb(data->cb_buffer);
+
+	if (!ret) {
+		data->buffer[0] = val;
+		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+						   iio_get_time_ns(indio_dev));
+	}
+
+error_out:
+	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;
+
+	ret = iio_channel_start_all_cb(data->cb_buffer);
+	if (ret)
+		return ret;
+
+	ret = lmp91000_read(data, chan->address, val);
+
+	iio_channel_stop_all_cb(data->cb_buffer);
+
+	if (!ret)
+		return IIO_VAL_INT;
+
+	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;
+	unsigned int reg, val;
+	int i, ret;
+
+	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
+	if (ret) {
+		if (of_property_read_bool(np, "ti,external-tia-resistor"))
+			val = 0;
+		else {
+			dev_err(dev, "no ti,tia-gain-ohm defined");
+			return ret;
+		}
+	}
+
+	ret = -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
+		if (lmp91000_tia_gain[i] == val) {
+			reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "ti,rload-ohm", &val);
+	if (ret) {
+		val = 100;
+		dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
+	}
+
+	ret = -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
+		if (lmp91000_rload[i] == val) {
+			reg |= i;
+			ret = 0;
+			break;
+		}
+	}
+
+	if (ret) {
+		dev_err(dev, "invalid ti,rload-ohm %d\n", val);
+		return ret;
+	}
+
+	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
+	regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
+	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_buffer_cb(const void *val, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct lmp91000_data *data = iio_priv(indio_dev);
+
+	data->buffer[data->chan_select] = *((int *)val);
+	complete_all(&data->completion);
+
+	return 0;
+}
+
+static const struct iio_trigger_ops lmp91000_trigger_ops = {
+	.owner = THIS_MODULE,
+};
+
+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;
+	int ret;
+
+	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->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);
+	}
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
+					    indio_dev->name, indio_dev->id);
+	if (!data->trig) {
+		dev_err(dev, "cannot allocate iio trigger.\n");
+		return -ENOMEM;
+	}
+
+	data->trig->ops = &lmp91000_trigger_ops;
+	data->trig->dev.parent = dev;
+	init_completion(&data->completion);
+
+	ret = lmp91000_read_config(data);
+	if (ret)
+		return ret;
+
+	data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
+						 indio_dev);
+	if (IS_ERR(data->cb_buffer)) {
+		if (PTR_ERR(data->cb_buffer) == -ENODEV)
+			return -EPROBE_DEFER;
+		return PTR_ERR(data->cb_buffer);
+	}
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 &lmp91000_buffer_handler, NULL);
+	if (ret)
+		goto error_unreg_cb_buffer;
+
+	ret = iio_trigger_register(data->trig);
+	if (ret) {
+		dev_err(dev, "cannot register iio trigger.\n");
+		goto error_unreg_cb_buffer;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto error_unreg_buffer;
+
+	iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
+					iio_trigger_get(data->trig);
+
+	return 0;
+
+error_unreg_buffer:
+	iio_triggered_buffer_cleanup(indio_dev);
+	iio_trigger_unregister(data->trig);
+
+error_unreg_cb_buffer:
+	iio_channel_release_all_cb(data->cb_buffer);
+
+	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_channel_stop_all_cb(data->cb_buffer);
+	iio_channel_release_all_cb(data->cb_buffer);
+
+	iio_triggered_buffer_cleanup(indio_dev);
+	iio_trigger_unregister(data->trig);
+
+	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 v4 2/3] iio: adc: ti-adc161s626: add support for TI 1-channel differential ADCs

[Marek Vasut]

On 08/20/2016 05:17 AM, Matt Ranostay wrote:
> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.

Hi,

[...]

> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
> @@ -0,0 +1,17 @@
> +* Texas Instruments ADC141S626, and ADC161S626 chip

I think you should drop the comma and use plural (chips) here.

> +Required properties:
> + - compatible: Should be "ti,adc141s626" or "ti,adc161s626"
> + - 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 1de31bdd4ce4..2658f2f69bad 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -426,6 +426,18 @@ config TI_ADC128S052
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
>  
> +config TI_ADC161S626
> +	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
> +	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

... be built ... :-)

> as a module. If so, the module will be
> +	  called ti-adc161s626.
> +
>  config TI_ADS1015
>  	tristate "Texas Instruments ADS1015 ADC"
>  	depends on I2C && !SENSORS_ADS1015

The code looks OK.

-- 
Best regards,
Marek Vasut

Re: [RFC v4 3/3] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 20/08/16 04:17, Matt Ranostay wrote:
> Add support for the LMP91000 potentiostat which is used for chemical
> sensing applications.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
As this is 'unusual' I'd appreciate more docs in general of what is
going on.

Mostly looking good but few bits and bobs that I think need tweaking inline.

I'd be inclined to factor out the relevant code in industrialio-trigger.c
(iio_trigger_write_current) to do validation etc as well on this.
It's not strickly necessary but might be nice. Also you can then put
a 'exclusive' mode check in there to prevent sysfs writes changing the
trigger on you.

> ---
>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>  drivers/iio/Kconfig                                |   1 +
>  drivers/iio/Makefile                               |   1 +
>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>  drivers/iio/potentiostat/Makefile                  |   6 +
>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>  6 files changed, 437 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..b9b621e94cd7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
> @@ -0,0 +1,30 @@
> +* 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 of the iio provider
> +
> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
> +    needs to be set to signal that an external resistor value is being used.
> +
> +Optional properties:
> +
> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
> +    amplifier. Must be 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 = <&adc>;
> +};
> 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..1e3baf2cc97d
> --- /dev/null
> +++ b/drivers/iio/potentiostat/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# 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_BUFFER_CB
> +	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..5046d2d3da98
> --- /dev/null
> +++ b/drivers/iio/potentiostat/lmp91000.c
> @@ -0,0 +1,377 @@
> +/*
> + * 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.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_trigger *trig;
> +	struct iio_cb_buffer *cb_buffer;
> +
> +	struct completion completion;
> +	u8 chan_select;
> +
> +	u32 buffer[4]; /* 64-bit data + 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);
> +
> +	data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
> +
I'd like a comment here to explain that this is causing the sample to
be taken.

To get it clear in my head:
1) Calls handle_nested_irq (should rename this function in general as my
understanding of the meaning of chained interrupts was actually wrong when
I came up with this name).
2) poll func of the ADC is called (bottom half only, but that's fine).
3) This calls iio_push_to_buffer.
4) The demux plucks on the relevant channel (only one enabled so that's easy
:) and passes it on to the callback buffer.
5) The completion below finishes, data is copied out and pushed into the
buffer of this device with a timestamp added etc. Job done.

> +	iio_trigger_poll_chained(data->trig);
> +
> +	ret = wait_for_completion_timeout(&data->completion, HZ);
> +	reinit_completion(&data->completion);
> +
> +	if (!ret)
> +		return -ETIMEDOUT;
> +
> +	*val = data->buffer[data->chan_select];
> +
> +	return 0;
> +}
> +
> +static irqreturn_t lmp91000_buffer_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;
> +
> +	memset(data->buffer, 0, sizeof(data->buffer));
> +
> +	ret = iio_channel_start_all_cb(data->cb_buffer);
> +	if (ret)
> +		goto error_out;
Why the need for the explicit start each time?  You have control of the
trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
operation so best avoided if we can keep it running across multiple scans.

Would have thought you can just do this in the postenable callback. Then
disable in the predisable.
> +
> +	ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
> +	iio_channel_stop_all_cb(data->cb_buffer);
> +
> +	if (!ret) {
> +		data->buffer[0] = val;
> +		iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +						   iio_get_time_ns(indio_dev));
> +	}
> +
> +error_out:
> +	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;
> +
I'd protect this with claim_direct.  We don't want this occuring
mid way through a buffered read - or two of these happening at
once.

> +	ret = iio_channel_start_all_cb(data->cb_buffer);
> +	if (ret)
> +		return ret;
> +
> +	ret = lmp91000_read(data, chan->address, val);
> +
> +	iio_channel_stop_all_cb(data->cb_buffer);
> +
> +	if (!ret)
> +		return IIO_VAL_INT;
> +
> +	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;
> +	unsigned int reg, val;
> +	int i, ret;
> +
> +	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
> +	if (ret) {
> +		if (of_property_read_bool(np, "ti,external-tia-resistor"))
> +			val = 0;
> +		else {
> +			dev_err(dev, "no ti,tia-gain-ohm defined");
> +			return ret;
> +		}
> +	}
> +
> +	ret = -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
> +		if (lmp91000_tia_gain[i] == val) {
> +			reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (ret) {
> +		dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "ti,rload-ohm", &val);
> +	if (ret) {
> +		val = 100;
> +		dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
> +	}
> +
> +	ret = -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
> +		if (lmp91000_rload[i] == val) {
> +			reg |= i;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	if (ret) {
> +		dev_err(dev, "invalid ti,rload-ohm %d\n", val);
> +		return ret;
> +	}
> +
> +	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
> +	regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
> +	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_buffer_cb(const void *val, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct lmp91000_data *data = iio_priv(indio_dev);
> +
> +	data->buffer[data->chan_select] = *((int *)val);
> +	complete_all(&data->completion);
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
> +	.owner = THIS_MODULE,
> +};
> +
> +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;
> +	int ret;
> +
> +	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->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);
> +	}
> +
> +	data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
> +					    indio_dev->name, indio_dev->id);
> +	if (!data->trig) {
> +		dev_err(dev, "cannot allocate iio trigger.\n");
> +		return -ENOMEM;
> +	}
> +
> +	data->trig->ops = &lmp91000_trigger_ops;
> +	data->trig->dev.parent = dev;
> +	init_completion(&data->completion);
> +
> +	ret = lmp91000_read_config(data);
> +	if (ret)
> +		return ret;
> +
> +	data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
> +						 indio_dev);
> +	if (IS_ERR(data->cb_buffer)) {
> +		if (PTR_ERR(data->cb_buffer) == -ENODEV)
> +			return -EPROBE_DEFER;
> +		return PTR_ERR(data->cb_buffer);
> +	}
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 &lmp91000_buffer_handler, NULL);
> +	if (ret)
> +		goto error_unreg_cb_buffer;
> +
> +	ret = iio_trigger_register(data->trig);
> +	if (ret) {
> +		dev_err(dev, "cannot register iio trigger.\n");
> +		goto error_unreg_cb_buffer;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto error_unreg_buffer;
> +

> +	iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
> +					iio_trigger_get(data->trig);

Hmm. Is this really all there is to it?

* What prevents the trigger from being changed?  The buffer is only enabled
  currently during each reading so there is plenty of time to 'steal' it.
  We need some for of 'exclusive' lock on this. As we are explicitly clocking
  the ADC capture from here I can't see where a non exclusive mode would work
  without some very very fiddly handling.

* I'd prefer this happening through a utility function as it's more
  that possible it'll be used in drivers that don't know about struct iio_dev.
  So keep that opaque.

It's racey with the iio_device_register... So probably needs to be
prior to that.  I'd even be inclined to work out how to do the cb_buffer
start in the probe and stop in the remove.  I'm not 100% sure all the
infrastructure is in place before the iio_device_register though. If it's
not we may need ot think about adding another stage for this usecase.
Might just push races into iio_device_register itself.

> +
> +	return 0;
> +
> +error_unreg_buffer:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	iio_trigger_unregister(data->trig);
> +
> +error_unreg_cb_buffer:
> +	iio_channel_release_all_cb(data->cb_buffer);
> +
> +	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_channel_stop_all_cb(data->cb_buffer);
> +	iio_channel_release_all_cb(data->cb_buffer);
> +
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	iio_trigger_unregister(data->trig);
> +
> +	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 v4 1/3] iio: buffer-callback: allow getting underlying iio_dev

[Jonathan Cameron]

On 20/08/16 04:17, Matt Ranostay wrote:
> Add iio_channel_cb_get_iio_dev function to allow getting the
> underlying iio_dev. This is useful for setting the trigger of the
> consumer ADC device.
> 
> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
Clearly going to be a necessary bit of infrastructure.

Applied to the togreg branch of iio.git - initially pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/buffer/industrialio-buffer-cb.c | 24 ++++++++++++++----------
>  include/linux/iio/consumer.h                | 12 ++++++++++++
>  2 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c
> index 323079c3ccce..b8f550e47d3d 100644
> --- a/drivers/iio/buffer/industrialio-buffer-cb.c
> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c
> @@ -18,6 +18,7 @@ struct iio_cb_buffer {
>  	int (*cb)(const void *data, void *private);
>  	void *private;
>  	struct iio_channel *channels;
> +	struct iio_dev *indio_dev;
>  };
>  
>  static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer)
> @@ -52,7 +53,6 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  {
>  	int ret;
>  	struct iio_cb_buffer *cb_buff;
> -	struct iio_dev *indio_dev;
>  	struct iio_channel *chan;
>  
>  	cb_buff = kzalloc(sizeof(*cb_buff), GFP_KERNEL);
> @@ -72,17 +72,17 @@ struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  		goto error_free_cb_buff;
>  	}
>  
> -	indio_dev = cb_buff->channels[0].indio_dev;
> +	cb_buff->indio_dev = cb_buff->channels[0].indio_dev;
>  	cb_buff->buffer.scan_mask
> -		= kcalloc(BITS_TO_LONGS(indio_dev->masklength), sizeof(long),
> -			  GFP_KERNEL);
> +		= kcalloc(BITS_TO_LONGS(cb_buff->indio_dev->masklength),
> +			  sizeof(long), GFP_KERNEL);
>  	if (cb_buff->buffer.scan_mask == NULL) {
>  		ret = -ENOMEM;
>  		goto error_release_channels;
>  	}
>  	chan = &cb_buff->channels[0];
>  	while (chan->indio_dev) {
> -		if (chan->indio_dev != indio_dev) {
> +		if (chan->indio_dev != cb_buff->indio_dev) {
>  			ret = -EINVAL;
>  			goto error_free_scan_mask;
>  		}
> @@ -105,17 +105,14 @@ EXPORT_SYMBOL_GPL(iio_channel_get_all_cb);
>  
>  int iio_channel_start_all_cb(struct iio_cb_buffer *cb_buff)
>  {
> -	return iio_update_buffers(cb_buff->channels[0].indio_dev,
> -				  &cb_buff->buffer,
> +	return iio_update_buffers(cb_buff->indio_dev, &cb_buff->buffer,
>  				  NULL);
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_start_all_cb);
>  
>  void iio_channel_stop_all_cb(struct iio_cb_buffer *cb_buff)
>  {
> -	iio_update_buffers(cb_buff->channels[0].indio_dev,
> -			   NULL,
> -			   &cb_buff->buffer);
> +	iio_update_buffers(cb_buff->indio_dev, NULL, &cb_buff->buffer);
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_stop_all_cb);
>  
> @@ -133,6 +130,13 @@ struct iio_channel
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_cb_get_channels);
>  
> +struct iio_dev
> +*iio_channel_cb_get_iio_dev(const struct iio_cb_buffer *cb_buffer)
> +{
> +	return cb_buffer->indio_dev;
> +}
> +EXPORT_SYMBOL_GPL(iio_channel_cb_get_iio_dev);
> +
>  MODULE_AUTHOR("Jonathan Cameron <jic23@kernel.org>");
>  MODULE_DESCRIPTION("Industrial I/O callback buffer");
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 3d672f72e7ec..9edccfba1ffb 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -165,6 +165,18 @@ struct iio_channel
>  *iio_channel_cb_get_channels(const struct iio_cb_buffer *cb_buffer);
>  
>  /**
> + * iio_channel_cb_get_iio_dev() - get access to the underlying device.
> + * @cb_buffer:		The callback buffer from whom we want the device
> + *			information.
> + *
> + * This function allows one to obtain information about the device.
> + * The primary aim is to allow drivers that are consuming a device to query
> + * things like current trigger.
> + */
> +struct iio_dev
> +*iio_channel_cb_get_iio_dev(const struct iio_cb_buffer *cb_buffer);
> +
> +/**
>   * iio_read_channel_raw() - read from a given channel
>   * @chan:		The channel being queried.
>   * @val:		Value read back.
> 

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

[Jonathan Cameron]

On 20/08/16 17:47, Marek Vasut wrote:
> On 08/20/2016 05:17 AM, Matt Ranostay wrote:
>> Add support for Texas Instruments ADC141S626, and ADC161S626 chips.
> 
> Hi,
> 
> [...]
> 
>> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc161s626.txt
>> @@ -0,0 +1,17 @@
>> +* Texas Instruments ADC141S626, and ADC161S626 chip
> 
> I think you should drop the comma and use plural (chips) here.
> 
>> +Required properties:
>> + - compatible: Should be "ti,adc141s626" or "ti,adc161s626"
>> + - 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 1de31bdd4ce4..2658f2f69bad 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -426,6 +426,18 @@ config TI_ADC128S052
>>  	  This driver can also be built as a module. If so, the module will be
>>  	  called ti-adc128s052.
>>  
>> +config TI_ADC161S626
>> +	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
>> +	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
> 
> ... be built ... :-)
> 
>> as a module. If so, the module will be
>> +	  called ti-adc161s626.
>> +
>>  config TI_ADS1015
>>  	tristate "Texas Instruments ADS1015 ADC"
>>  	depends on I2C && !SENSORS_ADS1015
> 
> The code looks OK.
> 
Agreed - I fixed the above up and applied to the togreg branch of iio.git.
This is a good little driver in it's own right.  No need to wait
for the consumer you have for it Matt to be ready!

Jonathan

Re: [RFC v4 3/3] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 20/08/16 04:17, Matt Ranostay wrote:
>> Add support for the LMP91000 potentiostat which is used for chemical
>> sensing applications.
>>
>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
> As this is 'unusual' I'd appreciate more docs in general of what is
> going on.
>
> Mostly looking good but few bits and bobs that I think need tweaking inline.
>
> I'd be inclined to factor out the relevant code in industrialio-trigger.c
> (iio_trigger_write_current) to do validation etc as well on this.
> It's not strickly necessary but might be nice. Also you can then put
> a 'exclusive' mode check in there to prevent sysfs writes changing the
> trigger on you.
>
>> ---
>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>  drivers/iio/Kconfig                                |   1 +
>>  drivers/iio/Makefile                               |   1 +
>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>  6 files changed, 437 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..b9b621e94cd7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>> @@ -0,0 +1,30 @@
>> +* 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 of the iio provider
>> +
>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>> +    needs to be set to signal that an external resistor value is being used.
>> +
>> +Optional properties:
>> +
>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>> +    amplifier. Must be 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 = <&adc>;
>> +};
>> 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..1e3baf2cc97d
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/Kconfig
>> @@ -0,0 +1,22 @@
>> +#
>> +# 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_BUFFER_CB
>> +     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..5046d2d3da98
>> --- /dev/null
>> +++ b/drivers/iio/potentiostat/lmp91000.c
>> @@ -0,0 +1,377 @@
>> +/*
>> + * 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.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_trigger *trig;
>> +     struct iio_cb_buffer *cb_buffer;
>> +
>> +     struct completion completion;
>> +     u8 chan_select;
>> +
>> +     u32 buffer[4]; /* 64-bit data + 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);
>> +
>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>> +
> I'd like a comment here to explain that this is causing the sample to
> be taken.
>
> To get it clear in my head:
> 1) Calls handle_nested_irq (should rename this function in general as my
> understanding of the meaning of chained interrupts was actually wrong when
> I came up with this name).

Yeah the naming kinda doesn't make sense, and to be honest wasn't that
clear to me initially.

> 2) poll func of the ADC is called (bottom half only, but that's fine).
> 3) This calls iio_push_to_buffer.
> 4) The demux plucks on the relevant channel (only one enabled so that's easy
> :) and passes it on to the callback buffer.
> 5) The completion below finishes, data is copied out and pushed into the
> buffer of this device with a timestamp added etc. Job done.

Correct!

>
>> +     iio_trigger_poll_chained(data->trig);
>> +
>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>> +     reinit_completion(&data->completion);
>> +
>> +     if (!ret)
>> +             return -ETIMEDOUT;
>> +
>> +     *val = data->buffer[data->chan_select];
>> +
>> +     return 0;
>> +}
>> +
>> +static irqreturn_t lmp91000_buffer_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;
>> +
>> +     memset(data->buffer, 0, sizeof(data->buffer));
>> +
>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>> +     if (ret)
>> +             goto error_out;
> Why the need for the explicit start each time?  You have control of the
> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
> operation so best avoided if we can keep it running across multiple scans.

There is a few issues with locking in the code doing that...
indio_dev->mlock is already held before iio_channel_start_all_cb. So
we need to really rework a few things or have this hack for now.

>
> Would have thought you can just do this in the postenable callback. Then
> disable in the predisable.
>> +
>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>> +     iio_channel_stop_all_cb(data->cb_buffer);
>> +
>> +     if (!ret) {
>> +             data->buffer[0] = val;
>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>> +                                                iio_get_time_ns(indio_dev));
>> +     }
>> +
>> +error_out:
>> +     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;
>> +
> I'd protect this with claim_direct.  We don't want this occuring
> mid way through a buffered read - or two of these happening at
> once.
>
>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = lmp91000_read(data, chan->address, val);
>> +
>> +     iio_channel_stop_all_cb(data->cb_buffer);
>> +
>> +     if (!ret)
>> +             return IIO_VAL_INT;
>> +
>> +     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;
>> +     unsigned int reg, val;
>> +     int i, ret;
>> +
>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>> +     if (ret) {
>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>> +                     val = 0;
>> +             else {
>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     ret = -EINVAL;
>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>> +             if (lmp91000_tia_gain[i] == val) {
>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ret) {
>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>> +             return ret;
>> +     }
>> +
>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>> +     if (ret) {
>> +             val = 100;
>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>> +     }
>> +
>> +     ret = -EINVAL;
>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>> +             if (lmp91000_rload[i] == val) {
>> +                     reg |= i;
>> +                     ret = 0;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (ret) {
>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>> +             return ret;
>> +     }
>> +
>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>> +     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_buffer_cb(const void *val, void *private)
>> +{
>> +     struct iio_dev *indio_dev = private;
>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>> +
>> +     data->buffer[data->chan_select] = *((int *)val);
>> +     complete_all(&data->completion);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>> +     .owner = THIS_MODULE,
>> +};
>> +
>> +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;
>> +     int ret;
>> +
>> +     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->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);
>> +     }
>> +
>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>> +                                         indio_dev->name, indio_dev->id);
>> +     if (!data->trig) {
>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>> +             return -ENOMEM;
>> +     }
>> +
>> +     data->trig->ops = &lmp91000_trigger_ops;
>> +     data->trig->dev.parent = dev;
>> +     init_completion(&data->completion);
>> +
>> +     ret = lmp91000_read_config(data);
>> +     if (ret)
>> +             return ret;
>> +
>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>> +                                              indio_dev);
>> +     if (IS_ERR(data->cb_buffer)) {
>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>> +                     return -EPROBE_DEFER;
>> +             return PTR_ERR(data->cb_buffer);
>> +     }
>> +
>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                                      &lmp91000_buffer_handler, NULL);
>> +     if (ret)
>> +             goto error_unreg_cb_buffer;
>> +
>> +     ret = iio_trigger_register(data->trig);
>> +     if (ret) {
>> +             dev_err(dev, "cannot register iio trigger.\n");
>> +             goto error_unreg_cb_buffer;
>> +     }
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_unreg_buffer;
>> +
>
>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>> +                                     iio_trigger_get(data->trig);
>
> Hmm. Is this really all there is to it?
>
> * What prevents the trigger from being changed?  The buffer is only enabled
>   currently during each reading so there is plenty of time to 'steal' it.
>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>   the ADC capture from here I can't see where a non exclusive mode would work
>   without some very very fiddly handling.
>

Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
So do we need to have a flag in the iio_dev struct that signals the
trigger isn't changeable?

> * I'd prefer this happening through a utility function as it's more
>   that possible it'll be used in drivers that don't know about struct iio_dev.
>   So keep that opaque.
>
> It's racey with the iio_device_register... So probably needs to be
> prior to that.  I'd even be inclined to work out how to do the cb_buffer
> start in the probe and stop in the remove.  I'm not 100% sure all the
> infrastructure is in place before the iio_device_register though. If it's
> not we may need ot think about adding another stage for this usecase.
> Might just push races into iio_device_register itself.
>

See locking issues noted above :).

>> +
>> +     return 0;
>> +
>> +error_unreg_buffer:
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +     iio_trigger_unregister(data->trig);
>> +
>> +error_unreg_cb_buffer:
>> +     iio_channel_release_all_cb(data->cb_buffer);
>> +
>> +     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_channel_stop_all_cb(data->cb_buffer);
>> +     iio_channel_release_all_cb(data->cb_buffer);
>> +
>> +     iio_triggered_buffer_cleanup(indio_dev);
>> +     iio_trigger_unregister(data->trig);
>> +
>> +     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 v4 3/3] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 26/08/16 04:09, Matt Ranostay wrote:
> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 20/08/16 04:17, Matt Ranostay wrote:
>>> Add support for the LMP91000 potentiostat which is used for chemical
>>> sensing applications.
>>>
>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>> As this is 'unusual' I'd appreciate more docs in general of what is
>> going on.
>>
>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>
>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>> (iio_trigger_write_current) to do validation etc as well on this.
>> It's not strickly necessary but might be nice. Also you can then put
>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>> trigger on you.
>>
>>> ---
>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>  drivers/iio/Kconfig                                |   1 +
>>>  drivers/iio/Makefile                               |   1 +
>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>>  6 files changed, 437 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..b9b621e94cd7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>> @@ -0,0 +1,30 @@
>>> +* 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 of the iio provider
>>> +
>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>> +    needs to be set to signal that an external resistor value is being used.
>>> +
>>> +Optional properties:
>>> +
>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>> +    amplifier. Must be 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 = <&adc>;
>>> +};
>>> 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..1e3baf2cc97d
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/Kconfig
>>> @@ -0,0 +1,22 @@
>>> +#
>>> +# 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_BUFFER_CB
>>> +     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..5046d2d3da98
>>> --- /dev/null
>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>> @@ -0,0 +1,377 @@
>>> +/*
>>> + * 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.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_trigger *trig;
>>> +     struct iio_cb_buffer *cb_buffer;
>>> +
>>> +     struct completion completion;
>>> +     u8 chan_select;
>>> +
>>> +     u32 buffer[4]; /* 64-bit data + 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);
>>> +
>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>> +
>> I'd like a comment here to explain that this is causing the sample to
>> be taken.
>>
>> To get it clear in my head:
>> 1) Calls handle_nested_irq (should rename this function in general as my
>> understanding of the meaning of chained interrupts was actually wrong when
>> I came up with this name).
> 
> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
> clear to me initially.
Hmm. If you fancy fixing that feel free ;)
> 
>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>> 3) This calls iio_push_to_buffer.
>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>> :) and passes it on to the callback buffer.
>> 5) The completion below finishes, data is copied out and pushed into the
>> buffer of this device with a timestamp added etc. Job done.
> 
> Correct!
> 
>>
>>> +     iio_trigger_poll_chained(data->trig);
>>> +
>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>> +     reinit_completion(&data->completion);
>>> +
>>> +     if (!ret)
>>> +             return -ETIMEDOUT;
>>> +
>>> +     *val = data->buffer[data->chan_select];
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static irqreturn_t lmp91000_buffer_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;
>>> +
>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>> +
>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>> +     if (ret)
>>> +             goto error_out;
>> Why the need for the explicit start each time?  You have control of the
>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>> operation so best avoided if we can keep it running across multiple scans.
> 
> There is a few issues with locking in the code doing that...
> indio_dev->mlock is already held before iio_channel_start_all_cb. So
> we need to really rework a few things or have this hack for now.
I'm not sure I follow.  

Talk me through the locking issue with doing the start in the preenable for
the buffer and the stop in the post disable?
> 
>>
>> Would have thought you can just do this in the postenable callback. Then
>> disable in the predisable.
>>> +
>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>> +
>>> +     if (!ret) {
>>> +             data->buffer[0] = val;
>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>> +                                                iio_get_time_ns(indio_dev));
>>> +     }
>>> +
>>> +error_out:
>>> +     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;
>>> +
>> I'd protect this with claim_direct.  We don't want this occuring
>> mid way through a buffered read - or two of these happening at
>> once.
>>
>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = lmp91000_read(data, chan->address, val);
>>> +
>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>> +
>>> +     if (!ret)
>>> +             return IIO_VAL_INT;
>>> +
>>> +     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;
>>> +     unsigned int reg, val;
>>> +     int i, ret;
>>> +
>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>> +     if (ret) {
>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>> +                     val = 0;
>>> +             else {
>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>> +                     return ret;
>>> +             }
>>> +     }
>>> +
>>> +     ret = -EINVAL;
>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>> +             if (lmp91000_tia_gain[i] == val) {
>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>> +     if (ret) {
>>> +             val = 100;
>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>> +     }
>>> +
>>> +     ret = -EINVAL;
>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>> +             if (lmp91000_rload[i] == val) {
>>> +                     reg |= i;
>>> +                     ret = 0;
>>> +                     break;
>>> +             }
>>> +     }
>>> +
>>> +     if (ret) {
>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>> +             return ret;
>>> +     }
>>> +
>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>> +     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_buffer_cb(const void *val, void *private)
>>> +{
>>> +     struct iio_dev *indio_dev = private;
>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>> +
>>> +     data->buffer[data->chan_select] = *((int *)val);
>>> +     complete_all(&data->completion);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>> +     .owner = THIS_MODULE,
>>> +};
>>> +
>>> +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;
>>> +     int ret;
>>> +
>>> +     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->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);
>>> +     }
>>> +
>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>> +                                         indio_dev->name, indio_dev->id);
>>> +     if (!data->trig) {
>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>> +             return -ENOMEM;
>>> +     }
>>> +
>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>> +     data->trig->dev.parent = dev;
>>> +     init_completion(&data->completion);
>>> +
>>> +     ret = lmp91000_read_config(data);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>> +                                              indio_dev);
>>> +     if (IS_ERR(data->cb_buffer)) {
>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>> +                     return -EPROBE_DEFER;
>>> +             return PTR_ERR(data->cb_buffer);
>>> +     }
>>> +
>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                      &lmp91000_buffer_handler, NULL);
>>> +     if (ret)
>>> +             goto error_unreg_cb_buffer;
>>> +
>>> +     ret = iio_trigger_register(data->trig);
>>> +     if (ret) {
>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>> +             goto error_unreg_cb_buffer;
>>> +     }
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret)
>>> +             goto error_unreg_buffer;
>>> +
>>
>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>> +                                     iio_trigger_get(data->trig);
>>
>> Hmm. Is this really all there is to it?
>>
>> * What prevents the trigger from being changed?  The buffer is only enabled
>>   currently during each reading so there is plenty of time to 'steal' it.
>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>   the ADC capture from here I can't see where a non exclusive mode would work
>>   without some very very fiddly handling.
>>
> 
> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
> So do we need to have a flag in the iio_dev struct that signals the
> trigger isn't changeable?
Yes, something along those lines I think.
> 
>> * I'd prefer this happening through a utility function as it's more
>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>   So keep that opaque.
>>
>> It's racey with the iio_device_register... So probably needs to be
>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>> start in the probe and stop in the remove.  I'm not 100% sure all the
>> infrastructure is in place before the iio_device_register though. If it's
>> not we may need ot think about adding another stage for this usecase.
>> Might just push races into iio_device_register itself.
>>
> 
> See locking issues noted above :).
We need to fix those...  Should not be the case.
> 
>>> +
>>> +     return 0;
>>> +
>>> +error_unreg_buffer:
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +     iio_trigger_unregister(data->trig);
>>> +
>>> +error_unreg_cb_buffer:
>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>> +
>>> +     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_channel_stop_all_cb(data->cb_buffer);
>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>> +
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>> +     iio_trigger_unregister(data->trig);
>>> +
>>> +     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");
>>>
>>
> --
> 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 v4 3/3] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 26/08/16 04:09, Matt Ranostay wrote:
>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 20/08/16 04:17, Matt Ranostay wrote:
>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>> sensing applications.
>>>>
>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>> As this is 'unusual' I'd appreciate more docs in general of what is
>>> going on.
>>>
>>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>>
>>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>>> (iio_trigger_write_current) to do validation etc as well on this.
>>> It's not strickly necessary but might be nice. Also you can then put
>>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>>> trigger on you.
>>>
>>>> ---
>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>>  drivers/iio/Kconfig                                |   1 +
>>>>  drivers/iio/Makefile                               |   1 +
>>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>>>  6 files changed, 437 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..b9b621e94cd7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>> @@ -0,0 +1,30 @@
>>>> +* 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 of the iio provider
>>>> +
>>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>>> +    needs to be set to signal that an external resistor value is being used.
>>>> +
>>>> +Optional properties:
>>>> +
>>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>>> +    amplifier. Must be 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 = <&adc>;
>>>> +};
>>>> 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..1e3baf2cc97d
>>>> --- /dev/null
>>>> +++ b/drivers/iio/potentiostat/Kconfig
>>>> @@ -0,0 +1,22 @@
>>>> +#
>>>> +# 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_BUFFER_CB
>>>> +     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..5046d2d3da98
>>>> --- /dev/null
>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>> @@ -0,0 +1,377 @@
>>>> +/*
>>>> + * 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.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_trigger *trig;
>>>> +     struct iio_cb_buffer *cb_buffer;
>>>> +
>>>> +     struct completion completion;
>>>> +     u8 chan_select;
>>>> +
>>>> +     u32 buffer[4]; /* 64-bit data + 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);
>>>> +
>>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>>> +
>>> I'd like a comment here to explain that this is causing the sample to
>>> be taken.
>>>
>>> To get it clear in my head:
>>> 1) Calls handle_nested_irq (should rename this function in general as my
>>> understanding of the meaning of chained interrupts was actually wrong when
>>> I came up with this name).
>>
>> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
>> clear to me initially.
> Hmm. If you fancy fixing that feel free ;)
>>
>>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>>> 3) This calls iio_push_to_buffer.
>>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>>> :) and passes it on to the callback buffer.
>>> 5) The completion below finishes, data is copied out and pushed into the
>>> buffer of this device with a timestamp added etc. Job done.
>>
>> Correct!
>>
>>>
>>>> +     iio_trigger_poll_chained(data->trig);
>>>> +
>>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>>> +     reinit_completion(&data->completion);
>>>> +
>>>> +     if (!ret)
>>>> +             return -ETIMEDOUT;
>>>> +
>>>> +     *val = data->buffer[data->chan_select];
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t lmp91000_buffer_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;
>>>> +
>>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>>> +
>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>> +     if (ret)
>>>> +             goto error_out;
>>> Why the need for the explicit start each time?  You have control of the
>>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>>> operation so best avoided if we can keep it running across multiple scans.
>>
>> There is a few issues with locking in the code doing that...
>> indio_dev->mlock is already held before iio_channel_start_all_cb. So
>> we need to really rework a few things or have this hack for now.
> I'm not sure I follow.
>
> Talk me through the locking issue with doing the start in the preenable for
> the buffer and the stop in the post disable?
>>

Yeah you get this doing that.... seems multiple attempts to get mlock
mutex with the update_buffers...


   77.959336] =============================================
[   77.963451] [ INFO: possible recursive locking detected ]
[   77.967575] 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15 Not tainted
[   77.972212] ---------------------------------------------
[   77.976335] sh/321 is trying to acquire lock:
[   77.979406]  (&dev->mlock){+.+.+.}, at: [<c0630ce0>]
iio_update_buffers+0x38/0xd4
[   77.985700]
but task is already holding lock:
[   77.988940]  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
iio_buffer_store_enable+0x34/0x98
[   77.995632]
other info that might help us debug this:
[   77.999572]  Possible unsafe locking scenario:
[   78.002899]        CPU0
[   78.004045]        ----
[   78.005190]   lock(&dev->mlock);
[   78.007142]   lock(&dev->mlock);
[   78.009095]
 *** DEADLOCK ***
[   78.011115]  May be due to missing lock nesting notation
[   78.015317] 5 locks held by sh/321:
[   78.017511]  #0:  (sb_writers#4){.+.+.+}, at: [<c02967d0>]
__sb_start_write+0x8c/0xb0
[   78.024143]  #1:  (&of->mutex){+.+.+.}, at: [<c0314184>]
kernfs_fop_write+0x88/0x1f0
[   78.030677]  #2:  (s_active#80){.+.+.+}, at: [<c031418c>]
kernfs_fop_write+0x90/0x1f0
[   78.037300]  #3:  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
iio_buffer_store_enable+0x34/0x98
[   78.044435]  #4:  (&dev->info_exist_lock){+.+...}, at: [<c0630cd4>]
iio_update_buffers+0x2c/0xd4
[   78.052006]
stack backtrace:
[   78.053773] PU: 0 PID: 321 Comm: sh Not tainted
4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15
[   78.060591] Hardware name: Generic AM33XX (Flattened Device Tree)
[   78.065428] [<c0110060>] (unwind_backtrace) from [<c010c1f8>]
(show_stack+0x10/0x14)
[   78.071919] [<c010c1f8>] (show_stack) from [<c0485904>]
(dump_stack+0xac/0xe0)
[   78.077884] [<c0485904>] (dump_stack) from [<c0190d38>]
(__lock_acquire+0xf0c/0x1880)
[   78.084450] [<c0190d38>] (__lock_acquire) from [<c0191ad4>]
(lock_acquire+0xcc/0x238)
[   78.091027] [<c0191ad4>] (lock_acquire) from [<c0751118>]
(mutex_lock_nested+0x3c/0x3d4)
[   78.097856] [<c0751118>] (mutex_lock_nested) from [<c0630ce0>]
(iio_update_buffers+0x38/0xd4)
[   78.105120] [<c0630ce0>] (iio_update_buffers) from [<c0630c50>]
(__iio_update_buffers+0x440/0x498)
[   78.112820] [<c0630c50>] (__iio_update_buffers) from [<c0630de4>]
(iio_buffer_store_enable+0x68/0x98)
[   78.120784] [<c0630de4>] (iio_buffer_store_enable) from
[<c03141c0>] (kernfs_fop_write+0xc4/0x1f0)
[   78.128486] [<c03141c0>] (kernfs_fop_write) from [<c02927b8>]
(__vfs_write+0x1c/0x114)
[   78.135136] [<c02927b8>] (__vfs_write) from [<c02936f8>]
(vfs_write+0xa0/0x180)
[   78.141176] [<c02936f8>] (vfs_write) from [<c0294450>] (SyS_write+0x3c/0x90)
[   78.146965] [<c0294450>] (SyS_write) from [<c0107840>]
(ret_fast_syscall+0x0/0x1c)

>>>
>>> Would have thought you can just do this in the postenable callback. Then
>>> disable in the predisable.
>>>> +
>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>> +
>>>> +     if (!ret) {
>>>> +             data->buffer[0] = val;
>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>> +                                                iio_get_time_ns(indio_dev));
>>>> +     }
>>>> +
>>>> +error_out:
>>>> +     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;
>>>> +
>>> I'd protect this with claim_direct.  We don't want this occuring
>>> mid way through a buffered read - or two of these happening at
>>> once.
>>>
>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     ret = lmp91000_read(data, chan->address, val);
>>>> +
>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>> +
>>>> +     if (!ret)
>>>> +             return IIO_VAL_INT;
>>>> +
>>>> +     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;
>>>> +     unsigned int reg, val;
>>>> +     int i, ret;
>>>> +
>>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>> +     if (ret) {
>>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>> +                     val = 0;
>>>> +             else {
>>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>>> +                     return ret;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     ret = -EINVAL;
>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>>> +             if (lmp91000_tia_gain[i] == val) {
>>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>>> +                     ret = 0;
>>>> +                     break;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (ret) {
>>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>>> +     if (ret) {
>>>> +             val = 100;
>>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>>> +     }
>>>> +
>>>> +     ret = -EINVAL;
>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>>> +             if (lmp91000_rload[i] == val) {
>>>> +                     reg |= i;
>>>> +                     ret = 0;
>>>> +                     break;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (ret) {
>>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>>> +     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_buffer_cb(const void *val, void *private)
>>>> +{
>>>> +     struct iio_dev *indio_dev = private;
>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>> +
>>>> +     data->buffer[data->chan_select] = *((int *)val);
>>>> +     complete_all(&data->completion);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>>> +     .owner = THIS_MODULE,
>>>> +};
>>>> +
>>>> +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;
>>>> +     int ret;
>>>> +
>>>> +     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->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);
>>>> +     }
>>>> +
>>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>>> +                                         indio_dev->name, indio_dev->id);
>>>> +     if (!data->trig) {
>>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>>> +             return -ENOMEM;
>>>> +     }
>>>> +
>>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>>> +     data->trig->dev.parent = dev;
>>>> +     init_completion(&data->completion);
>>>> +
>>>> +     ret = lmp91000_read_config(data);
>>>> +     if (ret)
>>>> +             return ret;
>>>> +
>>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>>> +                                              indio_dev);
>>>> +     if (IS_ERR(data->cb_buffer)) {
>>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>>> +                     return -EPROBE_DEFER;
>>>> +             return PTR_ERR(data->cb_buffer);
>>>> +     }
>>>> +
>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>> +                                      &lmp91000_buffer_handler, NULL);
>>>> +     if (ret)
>>>> +             goto error_unreg_cb_buffer;
>>>> +
>>>> +     ret = iio_trigger_register(data->trig);
>>>> +     if (ret) {
>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>> +             goto error_unreg_cb_buffer;
>>>> +     }
>>>> +
>>>> +     ret = iio_device_register(indio_dev);
>>>> +     if (ret)
>>>> +             goto error_unreg_buffer;
>>>> +
>>>
>>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>>> +                                     iio_trigger_get(data->trig);
>>>
>>> Hmm. Is this really all there is to it?
>>>
>>> * What prevents the trigger from being changed?  The buffer is only enabled
>>>   currently during each reading so there is plenty of time to 'steal' it.
>>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>>   the ADC capture from here I can't see where a non exclusive mode would work
>>>   without some very very fiddly handling.
>>>
>>
>> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
>> So do we need to have a flag in the iio_dev struct that signals the
>> trigger isn't changeable?
> Yes, something along those lines I think.
>>
>>> * I'd prefer this happening through a utility function as it's more
>>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>>   So keep that opaque.
>>>
>>> It's racey with the iio_device_register... So probably needs to be
>>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>>> start in the probe and stop in the remove.  I'm not 100% sure all the
>>> infrastructure is in place before the iio_device_register though. If it's
>>> not we may need ot think about adding another stage for this usecase.
>>> Might just push races into iio_device_register itself.
>>>
>>
>> See locking issues noted above :).
> We need to fix those...  Should not be the case.
>>
>>>> +
>>>> +     return 0;
>>>> +
>>>> +error_unreg_buffer:
>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>> +     iio_trigger_unregister(data->trig);
>>>> +
>>>> +error_unreg_cb_buffer:
>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>> +
>>>> +     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_channel_stop_all_cb(data->cb_buffer);
>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>> +
>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>> +     iio_trigger_unregister(data->trig);
>>>> +
>>>> +     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");
>>>>
>>>
>> --
>> 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 v4 3/3] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 30/08/16 07:01, Matt Ranostay wrote:
> On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 26/08/16 04:09, Matt Ranostay wrote:
>>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 20/08/16 04:17, Matt Ranostay wrote:
>>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>>> sensing applications.
>>>>>
>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>> As this is 'unusual' I'd appreciate more docs in general of what is
>>>> going on.
>>>>
>>>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>>>
>>>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>>>> (iio_trigger_write_current) to do validation etc as well on this.
>>>> It's not strickly necessary but might be nice. Also you can then put
>>>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>>>> trigger on you.
>>>>
>>>>> ---
>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>>>>  6 files changed, 437 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..b9b621e94cd7
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>> @@ -0,0 +1,30 @@
>>>>> +* 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 of the iio provider
>>>>> +
>>>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>>>> +    needs to be set to signal that an external resistor value is being used.
>>>>> +
>>>>> +Optional properties:
>>>>> +
>>>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>>>> +    amplifier. Must be 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 = <&adc>;
>>>>> +};
>>>>> 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..1e3baf2cc97d
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/potentiostat/Kconfig
>>>>> @@ -0,0 +1,22 @@
>>>>> +#
>>>>> +# 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_BUFFER_CB
>>>>> +     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..5046d2d3da98
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>>> @@ -0,0 +1,377 @@
>>>>> +/*
>>>>> + * 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.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_trigger *trig;
>>>>> +     struct iio_cb_buffer *cb_buffer;
>>>>> +
>>>>> +     struct completion completion;
>>>>> +     u8 chan_select;
>>>>> +
>>>>> +     u32 buffer[4]; /* 64-bit data + 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);
>>>>> +
>>>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>>>> +
>>>> I'd like a comment here to explain that this is causing the sample to
>>>> be taken.
>>>>
>>>> To get it clear in my head:
>>>> 1) Calls handle_nested_irq (should rename this function in general as my
>>>> understanding of the meaning of chained interrupts was actually wrong when
>>>> I came up with this name).
>>>
>>> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
>>> clear to me initially.
>> Hmm. If you fancy fixing that feel free ;)
>>>
>>>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>>>> 3) This calls iio_push_to_buffer.
>>>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>>>> :) and passes it on to the callback buffer.
>>>> 5) The completion below finishes, data is copied out and pushed into the
>>>> buffer of this device with a timestamp added etc. Job done.
>>>
>>> Correct!
>>>
>>>>
>>>>> +     iio_trigger_poll_chained(data->trig);
>>>>> +
>>>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>>>> +     reinit_completion(&data->completion);
>>>>> +
>>>>> +     if (!ret)
>>>>> +             return -ETIMEDOUT;
>>>>> +
>>>>> +     *val = data->buffer[data->chan_select];
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t lmp91000_buffer_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;
>>>>> +
>>>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>>>> +
>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>> +     if (ret)
>>>>> +             goto error_out;
>>>> Why the need for the explicit start each time?  You have control of the
>>>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>>>> operation so best avoided if we can keep it running across multiple scans.
>>>
>>> There is a few issues with locking in the code doing that...
>>> indio_dev->mlock is already held before iio_channel_start_all_cb. So
>>> we need to really rework a few things or have this hack for now.
>> I'm not sure I follow.
>>
>> Talk me through the locking issue with doing the start in the preenable for
>> the buffer and the stop in the post disable?
>>>
> 
> Yeah you get this doing that.... seems multiple attempts to get mlock
> mutex with the update_buffers...

Hmm. Let me try and get my head around this. I'm going to call the
front end device the outer, and the adc the inner device..

We call enable on the outer buffer. (iio_buffer_store_enable).
This takes the outer mlock and calls
__iio_update_buffers which calls iio_enable_buffers.
This calls indio_dev->setup_ops->preenable (for the outer device).

This preenable will call the iio_channel_start_all_cb which looks
dodgy as it'll call a nested iio_update_buffers which takes the
lock.  However, it should be calling it on the inner device whose
mlock we haven't taken yet.

So why is it going wrong?  I can't immediately see...

If I get a chance later I'll mock this up and see if I can track down
why it's a problem.  May well not get back to it this weekend though.

If you get a chance to dig some more that would be great!

Jonathan

> 
> 
>    77.959336] =============================================
> [   77.963451] [ INFO: possible recursive locking detected ]
> [   77.967575] 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15 Not tainted
> [   77.972212] ---------------------------------------------
> [   77.976335] sh/321 is trying to acquire lock:
> [   77.979406]  (&dev->mlock){+.+.+.}, at: [<c0630ce0>]
> iio_update_buffers+0x38/0xd4
> [   77.985700]
> but task is already holding lock:
> [   77.988940]  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
> iio_buffer_store_enable+0x34/0x98
> [   77.995632]
> other info that might help us debug this:
> [   77.999572]  Possible unsafe locking scenario:
> [   78.002899]        CPU0
> [   78.004045]        ----
> [   78.005190]   lock(&dev->mlock);
> [   78.007142]   lock(&dev->mlock);
> [   78.009095]
>  *** DEADLOCK ***
> [   78.011115]  May be due to missing lock nesting notation
> [   78.015317] 5 locks held by sh/321:
> [   78.017511]  #0:  (sb_writers#4){.+.+.+}, at: [<c02967d0>]
> __sb_start_write+0x8c/0xb0
> [   78.024143]  #1:  (&of->mutex){+.+.+.}, at: [<c0314184>]
> kernfs_fop_write+0x88/0x1f0
> [   78.030677]  #2:  (s_active#80){.+.+.+}, at: [<c031418c>]
> kernfs_fop_write+0x90/0x1f0
> [   78.037300]  #3:  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
> iio_buffer_store_enable+0x34/0x98
> [   78.044435]  #4:  (&dev->info_exist_lock){+.+...}, at: [<c0630cd4>]
> iio_update_buffers+0x2c/0xd4
> [   78.052006]
> stack backtrace:
> [   78.053773] PU: 0 PID: 321 Comm: sh Not tainted
> 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15
> [   78.060591] Hardware name: Generic AM33XX (Flattened Device Tree)
> [   78.065428] [<c0110060>] (unwind_backtrace) from [<c010c1f8>]
> (show_stack+0x10/0x14)
> [   78.071919] [<c010c1f8>] (show_stack) from [<c0485904>]
> (dump_stack+0xac/0xe0)
> [   78.077884] [<c0485904>] (dump_stack) from [<c0190d38>]
> (__lock_acquire+0xf0c/0x1880)
> [   78.084450] [<c0190d38>] (__lock_acquire) from [<c0191ad4>]
> (lock_acquire+0xcc/0x238)
> [   78.091027] [<c0191ad4>] (lock_acquire) from [<c0751118>]
> (mutex_lock_nested+0x3c/0x3d4)
> [   78.097856] [<c0751118>] (mutex_lock_nested) from [<c0630ce0>]
> (iio_update_buffers+0x38/0xd4)
> [   78.105120] [<c0630ce0>] (iio_update_buffers) from [<c0630c50>]
> (__iio_update_buffers+0x440/0x498)
> [   78.112820] [<c0630c50>] (__iio_update_buffers) from [<c0630de4>]
> (iio_buffer_store_enable+0x68/0x98)
> [   78.120784] [<c0630de4>] (iio_buffer_store_enable) from
> [<c03141c0>] (kernfs_fop_write+0xc4/0x1f0)
> [   78.128486] [<c03141c0>] (kernfs_fop_write) from [<c02927b8>]
> (__vfs_write+0x1c/0x114)
> [   78.135136] [<c02927b8>] (__vfs_write) from [<c02936f8>]
> (vfs_write+0xa0/0x180)
> [   78.141176] [<c02936f8>] (vfs_write) from [<c0294450>] (SyS_write+0x3c/0x90)
> [   78.146965] [<c0294450>] (SyS_write) from [<c0107840>]
> (ret_fast_syscall+0x0/0x1c)
> 
>>>>
>>>> Would have thought you can just do this in the postenable callback. Then
>>>> disable in the predisable.
>>>>> +
>>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>> +
>>>>> +     if (!ret) {
>>>>> +             data->buffer[0] = val;
>>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>> +                                                iio_get_time_ns(indio_dev));
>>>>> +     }
>>>>> +
>>>>> +error_out:
>>>>> +     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;
>>>>> +
>>>> I'd protect this with claim_direct.  We don't want this occuring
>>>> mid way through a buffered read - or two of these happening at
>>>> once.
>>>>
>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     ret = lmp91000_read(data, chan->address, val);
>>>>> +
>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>> +
>>>>> +     if (!ret)
>>>>> +             return IIO_VAL_INT;
>>>>> +
>>>>> +     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;
>>>>> +     unsigned int reg, val;
>>>>> +     int i, ret;
>>>>> +
>>>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>>> +     if (ret) {
>>>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>>> +                     val = 0;
>>>>> +             else {
>>>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>>>> +                     return ret;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     ret = -EINVAL;
>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>>>> +             if (lmp91000_tia_gain[i] == val) {
>>>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>>>> +                     ret = 0;
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     if (ret) {
>>>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>>>> +     if (ret) {
>>>>> +             val = 100;
>>>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>>>> +     }
>>>>> +
>>>>> +     ret = -EINVAL;
>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>>>> +             if (lmp91000_rload[i] == val) {
>>>>> +                     reg |= i;
>>>>> +                     ret = 0;
>>>>> +                     break;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     if (ret) {
>>>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>>>> +     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_buffer_cb(const void *val, void *private)
>>>>> +{
>>>>> +     struct iio_dev *indio_dev = private;
>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>> +
>>>>> +     data->buffer[data->chan_select] = *((int *)val);
>>>>> +     complete_all(&data->completion);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>>>> +     .owner = THIS_MODULE,
>>>>> +};
>>>>> +
>>>>> +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;
>>>>> +     int ret;
>>>>> +
>>>>> +     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->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);
>>>>> +     }
>>>>> +
>>>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>>>> +                                         indio_dev->name, indio_dev->id);
>>>>> +     if (!data->trig) {
>>>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>>>> +             return -ENOMEM;
>>>>> +     }
>>>>> +
>>>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>>>> +     data->trig->dev.parent = dev;
>>>>> +     init_completion(&data->completion);
>>>>> +
>>>>> +     ret = lmp91000_read_config(data);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>>>> +                                              indio_dev);
>>>>> +     if (IS_ERR(data->cb_buffer)) {
>>>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>>>> +                     return -EPROBE_DEFER;
>>>>> +             return PTR_ERR(data->cb_buffer);
>>>>> +     }
>>>>> +
>>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>> +                                      &lmp91000_buffer_handler, NULL);
>>>>> +     if (ret)
>>>>> +             goto error_unreg_cb_buffer;
>>>>> +
>>>>> +     ret = iio_trigger_register(data->trig);
>>>>> +     if (ret) {
>>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>>> +             goto error_unreg_cb_buffer;
>>>>> +     }
>>>>> +
>>>>> +     ret = iio_device_register(indio_dev);
>>>>> +     if (ret)
>>>>> +             goto error_unreg_buffer;
>>>>> +
>>>>
>>>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>>>> +                                     iio_trigger_get(data->trig);
>>>>
>>>> Hmm. Is this really all there is to it?
>>>>
>>>> * What prevents the trigger from being changed?  The buffer is only enabled
>>>>   currently during each reading so there is plenty of time to 'steal' it.
>>>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>>>   the ADC capture from here I can't see where a non exclusive mode would work
>>>>   without some very very fiddly handling.
>>>>
>>>
>>> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
>>> So do we need to have a flag in the iio_dev struct that signals the
>>> trigger isn't changeable?
>> Yes, something along those lines I think.
>>>
>>>> * I'd prefer this happening through a utility function as it's more
>>>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>>>   So keep that opaque.
>>>>
>>>> It's racey with the iio_device_register... So probably needs to be
>>>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>>>> start in the probe and stop in the remove.  I'm not 100% sure all the
>>>> infrastructure is in place before the iio_device_register though. If it's
>>>> not we may need ot think about adding another stage for this usecase.
>>>> Might just push races into iio_device_register itself.
>>>>
>>>
>>> See locking issues noted above :).
>> We need to fix those...  Should not be the case.
>>>
>>>>> +
>>>>> +     return 0;
>>>>> +
>>>>> +error_unreg_buffer:
>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>> +     iio_trigger_unregister(data->trig);
>>>>> +
>>>>> +error_unreg_cb_buffer:
>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>> +
>>>>> +     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_channel_stop_all_cb(data->cb_buffer);
>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>> +
>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>> +     iio_trigger_unregister(data->trig);
>>>>> +
>>>>> +     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");
>>>>>
>>>>
>>> --
>>> 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 v4 3/3] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

On Sat, Sep 3, 2016 at 7:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 30/08/16 07:01, Matt Ranostay wrote:
>> On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 26/08/16 04:09, Matt Ranostay wrote:
>>>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 20/08/16 04:17, Matt Ranostay wrote:
>>>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>>>> sensing applications.
>>>>>>
>>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>>> As this is 'unusual' I'd appreciate more docs in general of what is
>>>>> going on.
>>>>>
>>>>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>>>>
>>>>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>>>>> (iio_trigger_write_current) to do validation etc as well on this.
>>>>> It's not strickly necessary but might be nice. Also you can then put
>>>>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>>>>> trigger on you.
>>>>>
>>>>>> ---
>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>>>>>  6 files changed, 437 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..b9b621e94cd7
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>> @@ -0,0 +1,30 @@
>>>>>> +* 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 of the iio provider
>>>>>> +
>>>>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>>>>> +    needs to be set to signal that an external resistor value is being used.
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +
>>>>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>>>>> +    amplifier. Must be 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 = <&adc>;
>>>>>> +};
>>>>>> 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..1e3baf2cc97d
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/iio/potentiostat/Kconfig
>>>>>> @@ -0,0 +1,22 @@
>>>>>> +#
>>>>>> +# 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_BUFFER_CB
>>>>>> +     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..5046d2d3da98
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>>>> @@ -0,0 +1,377 @@
>>>>>> +/*
>>>>>> + * 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.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_trigger *trig;
>>>>>> +     struct iio_cb_buffer *cb_buffer;
>>>>>> +
>>>>>> +     struct completion completion;
>>>>>> +     u8 chan_select;
>>>>>> +
>>>>>> +     u32 buffer[4]; /* 64-bit data + 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);
>>>>>> +
>>>>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>>>>> +
>>>>> I'd like a comment here to explain that this is causing the sample to
>>>>> be taken.
>>>>>
>>>>> To get it clear in my head:
>>>>> 1) Calls handle_nested_irq (should rename this function in general as my
>>>>> understanding of the meaning of chained interrupts was actually wrong when
>>>>> I came up with this name).
>>>>
>>>> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
>>>> clear to me initially.
>>> Hmm. If you fancy fixing that feel free ;)
>>>>
>>>>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>>>>> 3) This calls iio_push_to_buffer.
>>>>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>>>>> :) and passes it on to the callback buffer.
>>>>> 5) The completion below finishes, data is copied out and pushed into the
>>>>> buffer of this device with a timestamp added etc. Job done.
>>>>
>>>> Correct!
>>>>
>>>>>
>>>>>> +     iio_trigger_poll_chained(data->trig);
>>>>>> +
>>>>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>>>>> +     reinit_completion(&data->completion);
>>>>>> +
>>>>>> +     if (!ret)
>>>>>> +             return -ETIMEDOUT;
>>>>>> +
>>>>>> +     *val = data->buffer[data->chan_select];
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static irqreturn_t lmp91000_buffer_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;
>>>>>> +
>>>>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>>>>> +
>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>> +     if (ret)
>>>>>> +             goto error_out;
>>>>> Why the need for the explicit start each time?  You have control of the
>>>>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>>>>> operation so best avoided if we can keep it running across multiple scans.
>>>>
>>>> There is a few issues with locking in the code doing that...
>>>> indio_dev->mlock is already held before iio_channel_start_all_cb. So
>>>> we need to really rework a few things or have this hack for now.
>>> I'm not sure I follow.
>>>
>>> Talk me through the locking issue with doing the start in the preenable for
>>> the buffer and the stop in the post disable?
>>>>
>>
>> Yeah you get this doing that.... seems multiple attempts to get mlock
>> mutex with the update_buffers...
>
> Hmm. Let me try and get my head around this. I'm going to call the
> front end device the outer, and the adc the inner device..
>
> We call enable on the outer buffer. (iio_buffer_store_enable).
> This takes the outer mlock and calls
> __iio_update_buffers which calls iio_enable_buffers.
> This calls indio_dev->setup_ops->preenable (for the outer device).
>
> This preenable will call the iio_channel_start_all_cb which looks
> dodgy as it'll call a nested iio_update_buffers which takes the
> lock.  However, it should be calling it on the inner device whose
> mlock we haven't taken yet.
>
> So why is it going wrong?  I can't immediately see...
>
> If I get a chance later I'll mock this up and see if I can track down
> why it's a problem.  May well not get back to it this weekend though.
>

I am really perplexed as well since it should be different indio_dev->mlock's

> If you get a chance to dig some more that would be great!
>
> Jonathan
>
>>
>>
>>    77.959336] =============================================
>> [   77.963451] [ INFO: possible recursive locking detected ]
>> [   77.967575] 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15 Not tainted
>> [   77.972212] ---------------------------------------------
>> [   77.976335] sh/321 is trying to acquire lock:
>> [   77.979406]  (&dev->mlock){+.+.+.}, at: [<c0630ce0>]
>> iio_update_buffers+0x38/0xd4
>> [   77.985700]
>> but task is already holding lock:
>> [   77.988940]  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>> iio_buffer_store_enable+0x34/0x98
>> [   77.995632]
>> other info that might help us debug this:
>> [   77.999572]  Possible unsafe locking scenario:
>> [   78.002899]        CPU0
>> [   78.004045]        ----
>> [   78.005190]   lock(&dev->mlock);
>> [   78.007142]   lock(&dev->mlock);
>> [   78.009095]
>>  *** DEADLOCK ***
>> [   78.011115]  May be due to missing lock nesting notation
>> [   78.015317] 5 locks held by sh/321:
>> [   78.017511]  #0:  (sb_writers#4){.+.+.+}, at: [<c02967d0>]
>> __sb_start_write+0x8c/0xb0
>> [   78.024143]  #1:  (&of->mutex){+.+.+.}, at: [<c0314184>]
>> kernfs_fop_write+0x88/0x1f0
>> [   78.030677]  #2:  (s_active#80){.+.+.+}, at: [<c031418c>]
>> kernfs_fop_write+0x90/0x1f0
>> [   78.037300]  #3:  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>> iio_buffer_store_enable+0x34/0x98
>> [   78.044435]  #4:  (&dev->info_exist_lock){+.+...}, at: [<c0630cd4>]
>> iio_update_buffers+0x2c/0xd4
>> [   78.052006]
>> stack backtrace:
>> [   78.053773] PU: 0 PID: 321 Comm: sh Not tainted
>> 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15
>> [   78.060591] Hardware name: Generic AM33XX (Flattened Device Tree)
>> [   78.065428] [<c0110060>] (unwind_backtrace) from [<c010c1f8>]
>> (show_stack+0x10/0x14)
>> [   78.071919] [<c010c1f8>] (show_stack) from [<c0485904>]
>> (dump_stack+0xac/0xe0)
>> [   78.077884] [<c0485904>] (dump_stack) from [<c0190d38>]
>> (__lock_acquire+0xf0c/0x1880)
>> [   78.084450] [<c0190d38>] (__lock_acquire) from [<c0191ad4>]
>> (lock_acquire+0xcc/0x238)
>> [   78.091027] [<c0191ad4>] (lock_acquire) from [<c0751118>]
>> (mutex_lock_nested+0x3c/0x3d4)
>> [   78.097856] [<c0751118>] (mutex_lock_nested) from [<c0630ce0>]
>> (iio_update_buffers+0x38/0xd4)
>> [   78.105120] [<c0630ce0>] (iio_update_buffers) from [<c0630c50>]
>> (__iio_update_buffers+0x440/0x498)
>> [   78.112820] [<c0630c50>] (__iio_update_buffers) from [<c0630de4>]
>> (iio_buffer_store_enable+0x68/0x98)
>> [   78.120784] [<c0630de4>] (iio_buffer_store_enable) from
>> [<c03141c0>] (kernfs_fop_write+0xc4/0x1f0)
>> [   78.128486] [<c03141c0>] (kernfs_fop_write) from [<c02927b8>]
>> (__vfs_write+0x1c/0x114)
>> [   78.135136] [<c02927b8>] (__vfs_write) from [<c02936f8>]
>> (vfs_write+0xa0/0x180)
>> [   78.141176] [<c02936f8>] (vfs_write) from [<c0294450>] (SyS_write+0x3c/0x90)
>> [   78.146965] [<c0294450>] (SyS_write) from [<c0107840>]
>> (ret_fast_syscall+0x0/0x1c)
>>
>>>>>
>>>>> Would have thought you can just do this in the postenable callback. Then
>>>>> disable in the predisable.
>>>>>> +
>>>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>> +
>>>>>> +     if (!ret) {
>>>>>> +             data->buffer[0] = val;
>>>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>>> +                                                iio_get_time_ns(indio_dev));
>>>>>> +     }
>>>>>> +
>>>>>> +error_out:
>>>>>> +     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;
>>>>>> +
>>>>> I'd protect this with claim_direct.  We don't want this occuring
>>>>> mid way through a buffered read - or two of these happening at
>>>>> once.
>>>>>
>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>> +     if (ret)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     ret = lmp91000_read(data, chan->address, val);
>>>>>> +
>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>> +
>>>>>> +     if (!ret)
>>>>>> +             return IIO_VAL_INT;
>>>>>> +
>>>>>> +     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;
>>>>>> +     unsigned int reg, val;
>>>>>> +     int i, ret;
>>>>>> +
>>>>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>>>> +     if (ret) {
>>>>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>>>> +                     val = 0;
>>>>>> +             else {
>>>>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>>>>> +                     return ret;
>>>>>> +             }
>>>>>> +     }
>>>>>> +
>>>>>> +     ret = -EINVAL;
>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>>>>> +             if (lmp91000_tia_gain[i] == val) {
>>>>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>>>>> +                     ret = 0;
>>>>>> +                     break;
>>>>>> +             }
>>>>>> +     }
>>>>>> +
>>>>>> +     if (ret) {
>>>>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>>>>> +             return ret;
>>>>>> +     }
>>>>>> +
>>>>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>>>>> +     if (ret) {
>>>>>> +             val = 100;
>>>>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>>>>> +     }
>>>>>> +
>>>>>> +     ret = -EINVAL;
>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>>>>> +             if (lmp91000_rload[i] == val) {
>>>>>> +                     reg |= i;
>>>>>> +                     ret = 0;
>>>>>> +                     break;
>>>>>> +             }
>>>>>> +     }
>>>>>> +
>>>>>> +     if (ret) {
>>>>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>>>>> +             return ret;
>>>>>> +     }
>>>>>> +
>>>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>>>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>>>>> +     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_buffer_cb(const void *val, void *private)
>>>>>> +{
>>>>>> +     struct iio_dev *indio_dev = private;
>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>> +
>>>>>> +     data->buffer[data->chan_select] = *((int *)val);
>>>>>> +     complete_all(&data->completion);
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>>>>> +     .owner = THIS_MODULE,
>>>>>> +};
>>>>>> +
>>>>>> +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;
>>>>>> +     int ret;
>>>>>> +
>>>>>> +     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->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);
>>>>>> +     }
>>>>>> +
>>>>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>>>>> +                                         indio_dev->name, indio_dev->id);
>>>>>> +     if (!data->trig) {
>>>>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>>>>> +             return -ENOMEM;
>>>>>> +     }
>>>>>> +
>>>>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>>>>> +     data->trig->dev.parent = dev;
>>>>>> +     init_completion(&data->completion);
>>>>>> +
>>>>>> +     ret = lmp91000_read_config(data);
>>>>>> +     if (ret)
>>>>>> +             return ret;
>>>>>> +
>>>>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>>>>> +                                              indio_dev);
>>>>>> +     if (IS_ERR(data->cb_buffer)) {
>>>>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>>>>> +                     return -EPROBE_DEFER;
>>>>>> +             return PTR_ERR(data->cb_buffer);
>>>>>> +     }
>>>>>> +
>>>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>> +                                      &lmp91000_buffer_handler, NULL);
>>>>>> +     if (ret)
>>>>>> +             goto error_unreg_cb_buffer;
>>>>>> +
>>>>>> +     ret = iio_trigger_register(data->trig);
>>>>>> +     if (ret) {
>>>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>>>> +             goto error_unreg_cb_buffer;
>>>>>> +     }
>>>>>> +
>>>>>> +     ret = iio_device_register(indio_dev);
>>>>>> +     if (ret)
>>>>>> +             goto error_unreg_buffer;
>>>>>> +
>>>>>
>>>>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>>>>> +                                     iio_trigger_get(data->trig);
>>>>>
>>>>> Hmm. Is this really all there is to it?
>>>>>
>>>>> * What prevents the trigger from being changed?  The buffer is only enabled
>>>>>   currently during each reading so there is plenty of time to 'steal' it.
>>>>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>>>>   the ADC capture from here I can't see where a non exclusive mode would work
>>>>>   without some very very fiddly handling.
>>>>>
>>>>
>>>> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
>>>> So do we need to have a flag in the iio_dev struct that signals the
>>>> trigger isn't changeable?
>>> Yes, something along those lines I think.
>>>>
>>>>> * I'd prefer this happening through a utility function as it's more
>>>>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>>>>   So keep that opaque.
>>>>>
>>>>> It's racey with the iio_device_register... So probably needs to be
>>>>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>>>>> start in the probe and stop in the remove.  I'm not 100% sure all the
>>>>> infrastructure is in place before the iio_device_register though. If it's
>>>>> not we may need ot think about adding another stage for this usecase.
>>>>> Might just push races into iio_device_register itself.
>>>>>
>>>>
>>>> See locking issues noted above :).
>>> We need to fix those...  Should not be the case.
>>>>
>>>>>> +
>>>>>> +     return 0;
>>>>>> +
>>>>>> +error_unreg_buffer:
>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>> +
>>>>>> +error_unreg_cb_buffer:
>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>> +
>>>>>> +     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_channel_stop_all_cb(data->cb_buffer);
>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>> +
>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>> +
>>>>>> +     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");
>>>>>>
>>>>>
>>>> --
>>>> 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 v4 3/3] iio: potentiostat: add LMP91000 support

[Matt Ranostay]

On Sat, Sep 3, 2016 at 9:56 PM, Matt Ranostay <mranostay@gmail.com> wrote:
> On Sat, Sep 3, 2016 at 7:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 30/08/16 07:01, Matt Ranostay wrote:
>>> On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>> On 26/08/16 04:09, Matt Ranostay wrote:
>>>>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>> On 20/08/16 04:17, Matt Ranostay wrote:
>>>>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>>>>> sensing applications.
>>>>>>>
>>>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>>>> As this is 'unusual' I'd appreciate more docs in general of what is
>>>>>> going on.
>>>>>>
>>>>>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>>>>>
>>>>>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>>>>>> (iio_trigger_write_current) to do validation etc as well on this.
>>>>>> It's not strickly necessary but might be nice. Also you can then put
>>>>>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>>>>>> trigger on you.
>>>>>>
>>>>>>> ---
>>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>>>>>>  6 files changed, 437 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..b9b621e94cd7
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>> @@ -0,0 +1,30 @@
>>>>>>> +* 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 of the iio provider
>>>>>>> +
>>>>>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>>>>>> +    needs to be set to signal that an external resistor value is being used.
>>>>>>> +
>>>>>>> +Optional properties:
>>>>>>> +
>>>>>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>>>>>> +    amplifier. Must be 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 = <&adc>;
>>>>>>> +};
>>>>>>> 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..1e3baf2cc97d
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/iio/potentiostat/Kconfig
>>>>>>> @@ -0,0 +1,22 @@
>>>>>>> +#
>>>>>>> +# 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_BUFFER_CB
>>>>>>> +     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..5046d2d3da98
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>>>>> @@ -0,0 +1,377 @@
>>>>>>> +/*
>>>>>>> + * 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.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_trigger *trig;
>>>>>>> +     struct iio_cb_buffer *cb_buffer;
>>>>>>> +
>>>>>>> +     struct completion completion;
>>>>>>> +     u8 chan_select;
>>>>>>> +
>>>>>>> +     u32 buffer[4]; /* 64-bit data + 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);
>>>>>>> +
>>>>>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>>>>>> +
>>>>>> I'd like a comment here to explain that this is causing the sample to
>>>>>> be taken.
>>>>>>
>>>>>> To get it clear in my head:
>>>>>> 1) Calls handle_nested_irq (should rename this function in general as my
>>>>>> understanding of the meaning of chained interrupts was actually wrong when
>>>>>> I came up with this name).
>>>>>
>>>>> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
>>>>> clear to me initially.
>>>> Hmm. If you fancy fixing that feel free ;)
>>>>>
>>>>>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>>>>>> 3) This calls iio_push_to_buffer.
>>>>>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>>>>>> :) and passes it on to the callback buffer.
>>>>>> 5) The completion below finishes, data is copied out and pushed into the
>>>>>> buffer of this device with a timestamp added etc. Job done.
>>>>>
>>>>> Correct!
>>>>>
>>>>>>
>>>>>>> +     iio_trigger_poll_chained(data->trig);
>>>>>>> +
>>>>>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>>>>>> +     reinit_completion(&data->completion);
>>>>>>> +
>>>>>>> +     if (!ret)
>>>>>>> +             return -ETIMEDOUT;
>>>>>>> +
>>>>>>> +     *val = data->buffer[data->chan_select];
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static irqreturn_t lmp91000_buffer_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;
>>>>>>> +
>>>>>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>>>>>> +
>>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>>> +     if (ret)
>>>>>>> +             goto error_out;
>>>>>> Why the need for the explicit start each time?  You have control of the
>>>>>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>>>>>> operation so best avoided if we can keep it running across multiple scans.
>>>>>
>>>>> There is a few issues with locking in the code doing that...
>>>>> indio_dev->mlock is already held before iio_channel_start_all_cb. So
>>>>> we need to really rework a few things or have this hack for now.
>>>> I'm not sure I follow.
>>>>
>>>> Talk me through the locking issue with doing the start in the preenable for
>>>> the buffer and the stop in the post disable?
>>>>>
>>>
>>> Yeah you get this doing that.... seems multiple attempts to get mlock
>>> mutex with the update_buffers...
>>
>> Hmm. Let me try and get my head around this. I'm going to call the
>> front end device the outer, and the adc the inner device..
>>
>> We call enable on the outer buffer. (iio_buffer_store_enable).
>> This takes the outer mlock and calls
>> __iio_update_buffers which calls iio_enable_buffers.
>> This calls indio_dev->setup_ops->preenable (for the outer device).
>>
>> This preenable will call the iio_channel_start_all_cb which looks
>> dodgy as it'll call a nested iio_update_buffers which takes the
>> lock.  However, it should be calling it on the inner device whose
>> mlock we haven't taken yet.
>>
>> So why is it going wrong?  I can't immediately see...
>>
>> If I get a chance later I'll mock this up and see if I can track down
>> why it's a problem.  May well not get back to it this weekend though.
>>
>
> I am really perplexed as well since it should be different indio_dev->mlock's
>
>> If you get a chance to dig some more that would be great!

Hmm is this a possible lockdep issue? Seen a few fixes go into in the
stable branch..

>>
>> Jonathan
>>
>>>
>>>
>>>    77.959336] =============================================
>>> [   77.963451] [ INFO: possible recursive locking detected ]
>>> [   77.967575] 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15 Not tainted
>>> [   77.972212] ---------------------------------------------
>>> [   77.976335] sh/321 is trying to acquire lock:
>>> [   77.979406]  (&dev->mlock){+.+.+.}, at: [<c0630ce0>]
>>> iio_update_buffers+0x38/0xd4
>>> [   77.985700]
>>> but task is already holding lock:
>>> [   77.988940]  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>>> iio_buffer_store_enable+0x34/0x98
>>> [   77.995632]
>>> other info that might help us debug this:
>>> [   77.999572]  Possible unsafe locking scenario:
>>> [   78.002899]        CPU0
>>> [   78.004045]        ----
>>> [   78.005190]   lock(&dev->mlock);
>>> [   78.007142]   lock(&dev->mlock);
>>> [   78.009095]
>>>  *** DEADLOCK ***
>>> [   78.011115]  May be due to missing lock nesting notation
>>> [   78.015317] 5 locks held by sh/321:
>>> [   78.017511]  #0:  (sb_writers#4){.+.+.+}, at: [<c02967d0>]
>>> __sb_start_write+0x8c/0xb0
>>> [   78.024143]  #1:  (&of->mutex){+.+.+.}, at: [<c0314184>]
>>> kernfs_fop_write+0x88/0x1f0
>>> [   78.030677]  #2:  (s_active#80){.+.+.+}, at: [<c031418c>]
>>> kernfs_fop_write+0x90/0x1f0
>>> [   78.037300]  #3:  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>>> iio_buffer_store_enable+0x34/0x98
>>> [   78.044435]  #4:  (&dev->info_exist_lock){+.+...}, at: [<c0630cd4>]
>>> iio_update_buffers+0x2c/0xd4
>>> [   78.052006]
>>> stack backtrace:
>>> [   78.053773] PU: 0 PID: 321 Comm: sh Not tainted
>>> 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15
>>> [   78.060591] Hardware name: Generic AM33XX (Flattened Device Tree)
>>> [   78.065428] [<c0110060>] (unwind_backtrace) from [<c010c1f8>]
>>> (show_stack+0x10/0x14)
>>> [   78.071919] [<c010c1f8>] (show_stack) from [<c0485904>]
>>> (dump_stack+0xac/0xe0)
>>> [   78.077884] [<c0485904>] (dump_stack) from [<c0190d38>]
>>> (__lock_acquire+0xf0c/0x1880)
>>> [   78.084450] [<c0190d38>] (__lock_acquire) from [<c0191ad4>]
>>> (lock_acquire+0xcc/0x238)
>>> [   78.091027] [<c0191ad4>] (lock_acquire) from [<c0751118>]
>>> (mutex_lock_nested+0x3c/0x3d4)
>>> [   78.097856] [<c0751118>] (mutex_lock_nested) from [<c0630ce0>]
>>> (iio_update_buffers+0x38/0xd4)
>>> [   78.105120] [<c0630ce0>] (iio_update_buffers) from [<c0630c50>]
>>> (__iio_update_buffers+0x440/0x498)
>>> [   78.112820] [<c0630c50>] (__iio_update_buffers) from [<c0630de4>]
>>> (iio_buffer_store_enable+0x68/0x98)
>>> [   78.120784] [<c0630de4>] (iio_buffer_store_enable) from
>>> [<c03141c0>] (kernfs_fop_write+0xc4/0x1f0)
>>> [   78.128486] [<c03141c0>] (kernfs_fop_write) from [<c02927b8>]
>>> (__vfs_write+0x1c/0x114)
>>> [   78.135136] [<c02927b8>] (__vfs_write) from [<c02936f8>]
>>> (vfs_write+0xa0/0x180)
>>> [   78.141176] [<c02936f8>] (vfs_write) from [<c0294450>] (SyS_write+0x3c/0x90)
>>> [   78.146965] [<c0294450>] (SyS_write) from [<c0107840>]
>>> (ret_fast_syscall+0x0/0x1c)
>>>
>>>>>>
>>>>>> Would have thought you can just do this in the postenable callback. Then
>>>>>> disable in the predisable.
>>>>>>> +
>>>>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>> +
>>>>>>> +     if (!ret) {
>>>>>>> +             data->buffer[0] = val;
>>>>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>>>> +                                                iio_get_time_ns(indio_dev));
>>>>>>> +     }
>>>>>>> +
>>>>>>> +error_out:
>>>>>>> +     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;
>>>>>>> +
>>>>>> I'd protect this with claim_direct.  We don't want this occuring
>>>>>> mid way through a buffered read - or two of these happening at
>>>>>> once.
>>>>>>
>>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>>> +     if (ret)
>>>>>>> +             return ret;
>>>>>>> +
>>>>>>> +     ret = lmp91000_read(data, chan->address, val);
>>>>>>> +
>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>> +
>>>>>>> +     if (!ret)
>>>>>>> +             return IIO_VAL_INT;
>>>>>>> +
>>>>>>> +     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;
>>>>>>> +     unsigned int reg, val;
>>>>>>> +     int i, ret;
>>>>>>> +
>>>>>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>>>>> +     if (ret) {
>>>>>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>>>>> +                     val = 0;
>>>>>>> +             else {
>>>>>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>>>>>> +                     return ret;
>>>>>>> +             }
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     ret = -EINVAL;
>>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>>>>>> +             if (lmp91000_tia_gain[i] == val) {
>>>>>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>>>>>> +                     ret = 0;
>>>>>>> +                     break;
>>>>>>> +             }
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     if (ret) {
>>>>>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>>>>>> +             return ret;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>>>>>> +     if (ret) {
>>>>>>> +             val = 100;
>>>>>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     ret = -EINVAL;
>>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>>>>>> +             if (lmp91000_rload[i] == val) {
>>>>>>> +                     reg |= i;
>>>>>>> +                     ret = 0;
>>>>>>> +                     break;
>>>>>>> +             }
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     if (ret) {
>>>>>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>>>>>> +             return ret;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>>>>>> +     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_buffer_cb(const void *val, void *private)
>>>>>>> +{
>>>>>>> +     struct iio_dev *indio_dev = private;
>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>> +
>>>>>>> +     data->buffer[data->chan_select] = *((int *)val);
>>>>>>> +     complete_all(&data->completion);
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>>>>>> +     .owner = THIS_MODULE,
>>>>>>> +};
>>>>>>> +
>>>>>>> +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;
>>>>>>> +     int ret;
>>>>>>> +
>>>>>>> +     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->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);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>>>>>> +                                         indio_dev->name, indio_dev->id);
>>>>>>> +     if (!data->trig) {
>>>>>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>>>>>> +             return -ENOMEM;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>>>>>> +     data->trig->dev.parent = dev;
>>>>>>> +     init_completion(&data->completion);
>>>>>>> +
>>>>>>> +     ret = lmp91000_read_config(data);
>>>>>>> +     if (ret)
>>>>>>> +             return ret;
>>>>>>> +
>>>>>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>>>>>> +                                              indio_dev);
>>>>>>> +     if (IS_ERR(data->cb_buffer)) {
>>>>>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>>>>>> +                     return -EPROBE_DEFER;
>>>>>>> +             return PTR_ERR(data->cb_buffer);
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>>> +                                      &lmp91000_buffer_handler, NULL);
>>>>>>> +     if (ret)
>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>> +
>>>>>>> +     ret = iio_trigger_register(data->trig);
>>>>>>> +     if (ret) {
>>>>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     ret = iio_device_register(indio_dev);
>>>>>>> +     if (ret)
>>>>>>> +             goto error_unreg_buffer;
>>>>>>> +
>>>>>>
>>>>>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>>>>>> +                                     iio_trigger_get(data->trig);
>>>>>>
>>>>>> Hmm. Is this really all there is to it?
>>>>>>
>>>>>> * What prevents the trigger from being changed?  The buffer is only enabled
>>>>>>   currently during each reading so there is plenty of time to 'steal' it.
>>>>>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>>>>>   the ADC capture from here I can't see where a non exclusive mode would work
>>>>>>   without some very very fiddly handling.
>>>>>>
>>>>>
>>>>> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
>>>>> So do we need to have a flag in the iio_dev struct that signals the
>>>>> trigger isn't changeable?
>>>> Yes, something along those lines I think.
>>>>>
>>>>>> * I'd prefer this happening through a utility function as it's more
>>>>>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>>>>>   So keep that opaque.
>>>>>>
>>>>>> It's racey with the iio_device_register... So probably needs to be
>>>>>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>>>>>> start in the probe and stop in the remove.  I'm not 100% sure all the
>>>>>> infrastructure is in place before the iio_device_register though. If it's
>>>>>> not we may need ot think about adding another stage for this usecase.
>>>>>> Might just push races into iio_device_register itself.
>>>>>>
>>>>>
>>>>> See locking issues noted above :).
>>>> We need to fix those...  Should not be the case.
>>>>>
>>>>>>> +
>>>>>>> +     return 0;
>>>>>>> +
>>>>>>> +error_unreg_buffer:
>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>>> +
>>>>>>> +error_unreg_cb_buffer:
>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>> +
>>>>>>> +     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_channel_stop_all_cb(data->cb_buffer);
>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>> +
>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>>> +
>>>>>>> +     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");
>>>>>>>
>>>>>>
>>>>> --
>>>>> 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 v4 3/3] iio: potentiostat: add LMP91000 support

[Jonathan Cameron]

On 06/09/16 02:58, Matt Ranostay wrote:
> On Sat, Sep 3, 2016 at 9:56 PM, Matt Ranostay <mranostay@gmail.com> wrote:
>> On Sat, Sep 3, 2016 at 7:59 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>> On 30/08/16 07:01, Matt Ranostay wrote:
>>>> On Mon, Aug 29, 2016 at 10:38 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>> On 26/08/16 04:09, Matt Ranostay wrote:
>>>>>> On Sun, Aug 21, 2016 at 3:46 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>>>>>> On 20/08/16 04:17, Matt Ranostay wrote:
>>>>>>>> Add support for the LMP91000 potentiostat which is used for chemical
>>>>>>>> sensing applications.
>>>>>>>>
>>>>>>>> Signed-off-by: Matt Ranostay <mranostay@gmail.com>
>>>>>>> As this is 'unusual' I'd appreciate more docs in general of what is
>>>>>>> going on.
>>>>>>>
>>>>>>> Mostly looking good but few bits and bobs that I think need tweaking inline.
>>>>>>>
>>>>>>> I'd be inclined to factor out the relevant code in industrialio-trigger.c
>>>>>>> (iio_trigger_write_current) to do validation etc as well on this.
>>>>>>> It's not strickly necessary but might be nice. Also you can then put
>>>>>>> a 'exclusive' mode check in there to prevent sysfs writes changing the
>>>>>>> trigger on you.
>>>>>>>
>>>>>>>> ---
>>>>>>>>  .../bindings/iio/potentiostat/lmp91000.txt         |  30 ++
>>>>>>>>  drivers/iio/Kconfig                                |   1 +
>>>>>>>>  drivers/iio/Makefile                               |   1 +
>>>>>>>>  drivers/iio/potentiostat/Kconfig                   |  22 ++
>>>>>>>>  drivers/iio/potentiostat/Makefile                  |   6 +
>>>>>>>>  drivers/iio/potentiostat/lmp91000.c                | 377 +++++++++++++++++++++
>>>>>>>>  6 files changed, 437 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..b9b621e94cd7
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/potentiostat/lmp91000.txt
>>>>>>>> @@ -0,0 +1,30 @@
>>>>>>>> +* 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 of the iio provider
>>>>>>>> +
>>>>>>>> +  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>>>>>>> +    needs to be set to signal that an external resistor value is being used.
>>>>>>>> +
>>>>>>>> +Optional properties:
>>>>>>>> +
>>>>>>>> +  - ti,tia-gain-ohm: ohm value of the internal resistor for the transimpedance
>>>>>>>> +    amplifier. Must be 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 = <&adc>;
>>>>>>>> +};
>>>>>>>> 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..1e3baf2cc97d
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/iio/potentiostat/Kconfig
>>>>>>>> @@ -0,0 +1,22 @@
>>>>>>>> +#
>>>>>>>> +# 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_BUFFER_CB
>>>>>>>> +     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..5046d2d3da98
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/iio/potentiostat/lmp91000.c
>>>>>>>> @@ -0,0 +1,377 @@
>>>>>>>> +/*
>>>>>>>> + * 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.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_trigger *trig;
>>>>>>>> +     struct iio_cb_buffer *cb_buffer;
>>>>>>>> +
>>>>>>>> +     struct completion completion;
>>>>>>>> +     u8 chan_select;
>>>>>>>> +
>>>>>>>> +     u32 buffer[4]; /* 64-bit data + 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);
>>>>>>>> +
>>>>>>>> +     data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
>>>>>>>> +
>>>>>>> I'd like a comment here to explain that this is causing the sample to
>>>>>>> be taken.
>>>>>>>
>>>>>>> To get it clear in my head:
>>>>>>> 1) Calls handle_nested_irq (should rename this function in general as my
>>>>>>> understanding of the meaning of chained interrupts was actually wrong when
>>>>>>> I came up with this name).
>>>>>>
>>>>>> Yeah the naming kinda doesn't make sense, and to be honest wasn't that
>>>>>> clear to me initially.
>>>>> Hmm. If you fancy fixing that feel free ;)
>>>>>>
>>>>>>> 2) poll func of the ADC is called (bottom half only, but that's fine).
>>>>>>> 3) This calls iio_push_to_buffer.
>>>>>>> 4) The demux plucks on the relevant channel (only one enabled so that's easy
>>>>>>> :) and passes it on to the callback buffer.
>>>>>>> 5) The completion below finishes, data is copied out and pushed into the
>>>>>>> buffer of this device with a timestamp added etc. Job done.
>>>>>>
>>>>>> Correct!
>>>>>>
>>>>>>>
>>>>>>>> +     iio_trigger_poll_chained(data->trig);
>>>>>>>> +
>>>>>>>> +     ret = wait_for_completion_timeout(&data->completion, HZ);
>>>>>>>> +     reinit_completion(&data->completion);
>>>>>>>> +
>>>>>>>> +     if (!ret)
>>>>>>>> +             return -ETIMEDOUT;
>>>>>>>> +
>>>>>>>> +     *val = data->buffer[data->chan_select];
>>>>>>>> +
>>>>>>>> +     return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static irqreturn_t lmp91000_buffer_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;
>>>>>>>> +
>>>>>>>> +     memset(data->buffer, 0, sizeof(data->buffer));
>>>>>>>> +
>>>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>>>> +     if (ret)
>>>>>>>> +             goto error_out;
>>>>>>> Why the need for the explicit start each time?  You have control of the
>>>>>>> trigger so it shouldn't capture otherwise.  This is a fairly heavy weight
>>>>>>> operation so best avoided if we can keep it running across multiple scans.
>>>>>>
>>>>>> There is a few issues with locking in the code doing that...
>>>>>> indio_dev->mlock is already held before iio_channel_start_all_cb. So
>>>>>> we need to really rework a few things or have this hack for now.
>>>>> I'm not sure I follow.
>>>>>
>>>>> Talk me through the locking issue with doing the start in the preenable for
>>>>> the buffer and the stop in the post disable?
>>>>>>
>>>>
>>>> Yeah you get this doing that.... seems multiple attempts to get mlock
>>>> mutex with the update_buffers...
>>>
>>> Hmm. Let me try and get my head around this. I'm going to call the
>>> front end device the outer, and the adc the inner device..
>>>
>>> We call enable on the outer buffer. (iio_buffer_store_enable).
>>> This takes the outer mlock and calls
>>> __iio_update_buffers which calls iio_enable_buffers.
>>> This calls indio_dev->setup_ops->preenable (for the outer device).
>>>
>>> This preenable will call the iio_channel_start_all_cb which looks
>>> dodgy as it'll call a nested iio_update_buffers which takes the
>>> lock.  However, it should be calling it on the inner device whose
>>> mlock we haven't taken yet.
>>>
>>> So why is it going wrong?  I can't immediately see...
>>>
>>> If I get a chance later I'll mock this up and see if I can track down
>>> why it's a problem.  May well not get back to it this weekend though.
>>>
>>
>> I am really perplexed as well since it should be different indio_dev->mlock's
>>
>>> If you get a chance to dig some more that would be great!
> 
> Hmm is this a possible lockdep issue? Seen a few fixes go into in the
> stable branch..
Honestly, I have no idea!

Jonathan
> 
>>>
>>> Jonathan
>>>
>>>>
>>>>
>>>>    77.959336] =============================================
>>>> [   77.963451] [ INFO: possible recursive locking detected ]
>>>> [   77.967575] 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15 Not tainted
>>>> [   77.972212] ---------------------------------------------
>>>> [   77.976335] sh/321 is trying to acquire lock:
>>>> [   77.979406]  (&dev->mlock){+.+.+.}, at: [<c0630ce0>]
>>>> iio_update_buffers+0x38/0xd4
>>>> [   77.985700]
>>>> but task is already holding lock:
>>>> [   77.988940]  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>>>> iio_buffer_store_enable+0x34/0x98
>>>> [   77.995632]
>>>> other info that might help us debug this:
>>>> [   77.999572]  Possible unsafe locking scenario:
>>>> [   78.002899]        CPU0
>>>> [   78.004045]        ----
>>>> [   78.005190]   lock(&dev->mlock);
>>>> [   78.007142]   lock(&dev->mlock);
>>>> [   78.009095]
>>>>  *** DEADLOCK ***
>>>> [   78.011115]  May be due to missing lock nesting notation
>>>> [   78.015317] 5 locks held by sh/321:
>>>> [   78.017511]  #0:  (sb_writers#4){.+.+.+}, at: [<c02967d0>]
>>>> __sb_start_write+0x8c/0xb0
>>>> [   78.024143]  #1:  (&of->mutex){+.+.+.}, at: [<c0314184>]
>>>> kernfs_fop_write+0x88/0x1f0
>>>> [   78.030677]  #2:  (s_active#80){.+.+.+}, at: [<c031418c>]
>>>> kernfs_fop_write+0x90/0x1f0
>>>> [   78.037300]  #3:  (&dev->mlock){+.+.+.}, at: [<c0630db0>]
>>>> iio_buffer_store_enable+0x34/0x98
>>>> [   78.044435]  #4:  (&dev->info_exist_lock){+.+...}, at: [<c0630cd4>]
>>>> iio_update_buffers+0x2c/0xd4
>>>> [   78.052006]
>>>> stack backtrace:
>>>> [   78.053773] PU: 0 PID: 321 Comm: sh Not tainted
>>>> 4.7.0-rc4-00044-g0e4912e9bd3b-dirty #15
>>>> [   78.060591] Hardware name: Generic AM33XX (Flattened Device Tree)
>>>> [   78.065428] [<c0110060>] (unwind_backtrace) from [<c010c1f8>]
>>>> (show_stack+0x10/0x14)
>>>> [   78.071919] [<c010c1f8>] (show_stack) from [<c0485904>]
>>>> (dump_stack+0xac/0xe0)
>>>> [   78.077884] [<c0485904>] (dump_stack) from [<c0190d38>]
>>>> (__lock_acquire+0xf0c/0x1880)
>>>> [   78.084450] [<c0190d38>] (__lock_acquire) from [<c0191ad4>]
>>>> (lock_acquire+0xcc/0x238)
>>>> [   78.091027] [<c0191ad4>] (lock_acquire) from [<c0751118>]
>>>> (mutex_lock_nested+0x3c/0x3d4)
>>>> [   78.097856] [<c0751118>] (mutex_lock_nested) from [<c0630ce0>]
>>>> (iio_update_buffers+0x38/0xd4)
>>>> [   78.105120] [<c0630ce0>] (iio_update_buffers) from [<c0630c50>]
>>>> (__iio_update_buffers+0x440/0x498)
>>>> [   78.112820] [<c0630c50>] (__iio_update_buffers) from [<c0630de4>]
>>>> (iio_buffer_store_enable+0x68/0x98)
>>>> [   78.120784] [<c0630de4>] (iio_buffer_store_enable) from
>>>> [<c03141c0>] (kernfs_fop_write+0xc4/0x1f0)
>>>> [   78.128486] [<c03141c0>] (kernfs_fop_write) from [<c02927b8>]
>>>> (__vfs_write+0x1c/0x114)
>>>> [   78.135136] [<c02927b8>] (__vfs_write) from [<c02936f8>]
>>>> (vfs_write+0xa0/0x180)
>>>> [   78.141176] [<c02936f8>] (vfs_write) from [<c0294450>] (SyS_write+0x3c/0x90)
>>>> [   78.146965] [<c0294450>] (SyS_write) from [<c0107840>]
>>>> (ret_fast_syscall+0x0/0x1c)
>>>>
>>>>>>>
>>>>>>> Would have thought you can just do this in the postenable callback. Then
>>>>>>> disable in the predisable.
>>>>>>>> +
>>>>>>>> +     ret = lmp91000_read(data, LMP91000_REG_MODECN_3LEAD, &val);
>>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>>> +
>>>>>>>> +     if (!ret) {
>>>>>>>> +             data->buffer[0] = val;
>>>>>>>> +             iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
>>>>>>>> +                                                iio_get_time_ns(indio_dev));
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +error_out:
>>>>>>>> +     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;
>>>>>>>> +
>>>>>>> I'd protect this with claim_direct.  We don't want this occuring
>>>>>>> mid way through a buffered read - or two of these happening at
>>>>>>> once.
>>>>>>>
>>>>>>>> +     ret = iio_channel_start_all_cb(data->cb_buffer);
>>>>>>>> +     if (ret)
>>>>>>>> +             return ret;
>>>>>>>> +
>>>>>>>> +     ret = lmp91000_read(data, chan->address, val);
>>>>>>>> +
>>>>>>>> +     iio_channel_stop_all_cb(data->cb_buffer);
>>>>>>>> +
>>>>>>>> +     if (!ret)
>>>>>>>> +             return IIO_VAL_INT;
>>>>>>>> +
>>>>>>>> +     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;
>>>>>>>> +     unsigned int reg, val;
>>>>>>>> +     int i, ret;
>>>>>>>> +
>>>>>>>> +     ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>>>>>> +     if (ret) {
>>>>>>>> +             if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>>>>>> +                     val = 0;
>>>>>>>> +             else {
>>>>>>>> +                     dev_err(dev, "no ti,tia-gain-ohm defined");
>>>>>>>> +                     return ret;
>>>>>>>> +             }
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = -EINVAL;
>>>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_tia_gain); i++) {
>>>>>>>> +             if (lmp91000_tia_gain[i] == val) {
>>>>>>>> +                     reg = i << LMP91000_REG_TIACN_GAIN_SHIFT;
>>>>>>>> +                     ret = 0;
>>>>>>>> +                     break;
>>>>>>>> +             }
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     if (ret) {
>>>>>>>> +             dev_err(dev, "invalid ti,tia-gain-ohm %d\n", val);
>>>>>>>> +             return ret;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = of_property_read_u32(np, "ti,rload-ohm", &val);
>>>>>>>> +     if (ret) {
>>>>>>>> +             val = 100;
>>>>>>>> +             dev_info(dev, "no ti,rload-ohm defined, default to %d\n", val);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = -EINVAL;
>>>>>>>> +     for (i = 0; i < ARRAY_SIZE(lmp91000_rload); i++) {
>>>>>>>> +             if (lmp91000_rload[i] == val) {
>>>>>>>> +                     reg |= i;
>>>>>>>> +                     ret = 0;
>>>>>>>> +                     break;
>>>>>>>> +             }
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     if (ret) {
>>>>>>>> +             dev_err(dev, "invalid ti,rload-ohm %d\n", val);
>>>>>>>> +             return ret;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
>>>>>>>> +     regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
>>>>>>>> +     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_buffer_cb(const void *val, void *private)
>>>>>>>> +{
>>>>>>>> +     struct iio_dev *indio_dev = private;
>>>>>>>> +     struct lmp91000_data *data = iio_priv(indio_dev);
>>>>>>>> +
>>>>>>>> +     data->buffer[data->chan_select] = *((int *)val);
>>>>>>>> +     complete_all(&data->completion);
>>>>>>>> +
>>>>>>>> +     return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static const struct iio_trigger_ops lmp91000_trigger_ops = {
>>>>>>>> +     .owner = THIS_MODULE,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +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;
>>>>>>>> +     int ret;
>>>>>>>> +
>>>>>>>> +     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->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);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-mux%d",
>>>>>>>> +                                         indio_dev->name, indio_dev->id);
>>>>>>>> +     if (!data->trig) {
>>>>>>>> +             dev_err(dev, "cannot allocate iio trigger.\n");
>>>>>>>> +             return -ENOMEM;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     data->trig->ops = &lmp91000_trigger_ops;
>>>>>>>> +     data->trig->dev.parent = dev;
>>>>>>>> +     init_completion(&data->completion);
>>>>>>>> +
>>>>>>>> +     ret = lmp91000_read_config(data);
>>>>>>>> +     if (ret)
>>>>>>>> +             return ret;
>>>>>>>> +
>>>>>>>> +     data->cb_buffer = iio_channel_get_all_cb(dev, &lmp91000_buffer_cb,
>>>>>>>> +                                              indio_dev);
>>>>>>>> +     if (IS_ERR(data->cb_buffer)) {
>>>>>>>> +             if (PTR_ERR(data->cb_buffer) == -ENODEV)
>>>>>>>> +                     return -EPROBE_DEFER;
>>>>>>>> +             return PTR_ERR(data->cb_buffer);
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = iio_triggered_buffer_setup(indio_dev, NULL,
>>>>>>>> +                                      &lmp91000_buffer_handler, NULL);
>>>>>>>> +     if (ret)
>>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>>> +
>>>>>>>> +     ret = iio_trigger_register(data->trig);
>>>>>>>> +     if (ret) {
>>>>>>>> +             dev_err(dev, "cannot register iio trigger.\n");
>>>>>>>> +             goto error_unreg_cb_buffer;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     ret = iio_device_register(indio_dev);
>>>>>>>> +     if (ret)
>>>>>>>> +             goto error_unreg_buffer;
>>>>>>>> +
>>>>>>>
>>>>>>>> +     iio_channel_cb_get_iio_dev(data->cb_buffer)->trig =
>>>>>>>> +                                     iio_trigger_get(data->trig);
>>>>>>>
>>>>>>> Hmm. Is this really all there is to it?
>>>>>>>
>>>>>>> * What prevents the trigger from being changed?  The buffer is only enabled
>>>>>>>   currently during each reading so there is plenty of time to 'steal' it.
>>>>>>>   We need some for of 'exclusive' lock on this. As we are explicitly clocking
>>>>>>>   the ADC capture from here I can't see where a non exclusive mode would work
>>>>>>>   without some very very fiddly handling.
>>>>>>>
>>>>>>
>>>>>> Ok yeah this is a bit ugly... and you are correct the trigger could be changed.
>>>>>> So do we need to have a flag in the iio_dev struct that signals the
>>>>>> trigger isn't changeable?
>>>>> Yes, something along those lines I think.
>>>>>>
>>>>>>> * I'd prefer this happening through a utility function as it's more
>>>>>>>   that possible it'll be used in drivers that don't know about struct iio_dev.
>>>>>>>   So keep that opaque.
>>>>>>>
>>>>>>> It's racey with the iio_device_register... So probably needs to be
>>>>>>> prior to that.  I'd even be inclined to work out how to do the cb_buffer
>>>>>>> start in the probe and stop in the remove.  I'm not 100% sure all the
>>>>>>> infrastructure is in place before the iio_device_register though. If it's
>>>>>>> not we may need ot think about adding another stage for this usecase.
>>>>>>> Might just push races into iio_device_register itself.
>>>>>>>
>>>>>>
>>>>>> See locking issues noted above :).
>>>>> We need to fix those...  Should not be the case.
>>>>>>
>>>>>>>> +
>>>>>>>> +     return 0;
>>>>>>>> +
>>>>>>>> +error_unreg_buffer:
>>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>>>> +
>>>>>>>> +error_unreg_cb_buffer:
>>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>>> +
>>>>>>>> +     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_channel_stop_all_cb(data->cb_buffer);
>>>>>>>> +     iio_channel_release_all_cb(data->cb_buffer);
>>>>>>>> +
>>>>>>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>>>>>>> +     iio_trigger_unregister(data->trig);
>>>>>>>> +
>>>>>>>> +     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");
>>>>>>>>
>>>>>>>
>>>>>> --
>>>>>> 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
>>>>>>
>>>>>
>>>