[PATCH 1/2] iio: ti-ads124s08: Add DT binding documentation

[PATCH 1/2] iio: ti-ads124s08: Add DT binding documentation

[Dan Murphy]

Adding binding documentation for Texas Instruments ADS124S08
and ADS124S06 ADC.

S08 is a 12 channel ADC
S06 is a 6 channel ADC

Datesheet can be found here:
http://www.ti.com/lit/gpn/ads124s08

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../bindings/iio/adc/ti-ads124s08.txt         | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt b/Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt
new file mode 100644
index 000000000000..41ab67379ee1
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt
@@ -0,0 +1,25 @@
+* Texas Instruments' ads124s08 and ads124s06 ADC chip
+
+Required properties:
+ - compatible: Should be "ti,ads124s08" or "ti,ads124s06"
+ - reg: spi chip select number for the device
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+		Documentation/devicetree/bindings/spi/spi-bus.txt
+ - spi-cpha: Definition as per
+		Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Optional properties:
+ - vref-supply: The regulator supply for ADC reference voltage
+ - reset-gpios: GPIO pin used to reset the device.
+
+Example:
+adc@0 {
+	compatible = "ti,ads8688";
+	reg = <0>;
+	vref-supply = <&vdd_supply>;
+	spi-max-frequency = <1000000>;
+	spi-cpha;
+	reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
+};
-- 
2.20.0.rc1.10.g7068cbc4ab

[PATCH 2/2] iio: adc: Add the TI ads124s08 ADC code

[Dan Murphy]

Introduce the TI ADS124S08 and the ADS124S06 ADC
devices from TI.  The ADS124S08 is the 12 channel ADC
and the ADS124S06 is the 6 channel ADC device

These devices share a common datasheet:
http://www.ti.com/lit/gpn/ads124s08

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/iio/adc/Kconfig        |  10 +
 drivers/iio/adc/Makefile       |   1 +
 drivers/iio/adc/ti-ads124s08.c | 437 +++++++++++++++++++++++++++++++++
 3 files changed, 448 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads124s08.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a52fea8749a9..e8c5686e6716 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -887,6 +887,16 @@ config TI_ADS8688
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads8688.
 
+config TI_ADS124S08
+	tristate "Texas Instruments ADS124S08"
+	depends on SPI && OF
+	help
+	  If you say yes here you get support for Texas Instruments ADS124S08
+	  and ADS124S06 ADC chips
+
+	  This driver can also be built as a module. If so, the module will be
+	  called ti-ads124s08.
+
 config TI_AM335X_ADC
 	tristate "TI's AM335X ADC driver"
 	depends on MFD_TI_AM335X_TSCADC && HAS_DMA
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a6e6a0b659e2..d70293abfdba 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
 obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
 obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
+obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
 obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
new file mode 100644
index 000000000000..b6fc93f37355
--- /dev/null
+++ b/drivers/iio/adc/ti-ads124s08.c
@@ -0,0 +1,437 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI ADS124S0X chip family driver
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/err.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/sysfs.h>
+
+#define ADS124S08_ID		0x00
+#define ADS124S06_ID		0x01
+
+/* Commands */
+#define ADS124S_CMD_NOP		0x00
+#define ADS124S_CMD_WAKEUP	0x02
+#define ADS124S_CMD_PWRDWN	0x04
+#define ADS124S_CMD_RESET	0x06
+#define ADS124S_CMD_START	0x08
+#define ADS124S_CMD_STOP	0x0a
+#define ADS124S_CMD_SYOCAL	0x16
+#define ADS124S_CMD_SYGCAL	0x17
+#define ADS124S_CMD_SFOCAL	0x19
+#define ADS124S_CMD_RDATA	0x12
+#define ADS124S_CMD_RREG	0x20
+#define ADS124S_CMD_WREG	0x40
+
+/* Registers */
+#define ADS124S0X_ID		0x00
+#define ADS124S0X_STATUS	0x01
+#define ADS124S0X_INPUT_MUX	0x02
+#define ADS124S0X_PGA		0x03
+#define ADS124S0X_DATA_RATE	0x04
+#define ADS124S0X_REF		0x05
+#define ADS124S0X_IDACMAG	0x06
+#define ADS124S0X_IDACMUX	0x07
+#define ADS124S0X_VBIAS		0x08
+#define ADS124S0X_SYS		0x09
+#define ADS124S0X_OFCAL0	0x0a
+#define ADS124S0X_OFCAL1	0x0b
+#define ADS124S0X_OFCAL2	0x0c
+#define ADS124S0X_FSCAL0	0x0d
+#define ADS124S0X_FSCAL1	0x0e
+#define ADS124S0X_FSCAL2	0x0f
+#define ADS124S0X_GPIODAT	0x10
+#define ADS124S0X_GPIOCON	0x11
+
+/* ADS124S0x common channels */
+#define ADS124S0X_AIN0		0x00
+#define ADS124S0X_AIN1		0x01
+#define ADS124S0X_AIN2		0x02
+#define ADS124S0X_AIN3		0x03
+#define ADS124S0X_AIN4		0x04
+#define ADS124S0X_AIN5		0x05
+#define ADS124S08_AINCOM	0x0c
+/* ADS124S08 only channels */
+#define ADS124S08_AIN6		0x06
+#define ADS124S08_AIN7		0x07
+#define ADS124S08_AIN8		0x08
+#define ADS124S08_AIN9		0x09
+#define ADS124S08_AIN10		0x0a
+#define ADS124S08_AIN11		0x0b
+#define ADS124S0X_MAX_CHANNELS	12
+
+#define ADS124S0X_POS_MUX_SHIFT	0x04
+#define ADS124S_INT_REF		0x09
+
+#define ADS124S_START_REG_MASK	0x1f
+#define ADS124S_NUM_BYTES_MASK	0x1f
+
+#define ADS124S_START_CONV	0x01
+#define ADS124S_STOP_CONV	0x00
+
+struct ads124s_chip_info {
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+};
+
+struct ads124s_private {
+	const struct ads124s_chip_info	*chip_info;
+	struct gpio_desc *reset_gpio;
+	struct spi_device *spi;
+	struct regulator *reg;
+	struct mutex lock;
+	unsigned int vref_mv;
+	u8 data[ADS124S0X_MAX_CHANNELS];
+};
+
+#define ADS124S_CHAN(index)					\
+{								\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = index,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.scan_index = index,					\
+	.scan_type = {						\
+		.sign = 'u',					\
+		.realbits = 24,					\
+		.storagebits = 24,				\
+		.endianness = IIO_BE,				\
+	},							\
+}
+
+static const struct iio_chan_spec ads124s06_channels[] = {
+	ADS124S_CHAN(0),
+	ADS124S_CHAN(1),
+	ADS124S_CHAN(2),
+	ADS124S_CHAN(3),
+	ADS124S_CHAN(4),
+	ADS124S_CHAN(5),
+};
+
+static const struct iio_chan_spec ads124s08_channels[] = {
+	ADS124S_CHAN(0),
+	ADS124S_CHAN(1),
+	ADS124S_CHAN(2),
+	ADS124S_CHAN(3),
+	ADS124S_CHAN(4),
+	ADS124S_CHAN(5),
+	ADS124S_CHAN(6),
+	ADS124S_CHAN(7),
+	ADS124S_CHAN(8),
+	ADS124S_CHAN(9),
+	ADS124S_CHAN(10),
+	ADS124S_CHAN(11),
+};
+
+static const struct ads124s_chip_info ads124s_chip_info_tbl[] = {
+	[ADS124S08_ID] = {
+		.channels = ads124s08_channels,
+		.num_channels = ARRAY_SIZE(ads124s08_channels),
+	},
+	[ADS124S06_ID] = {
+		.channels = ads124s06_channels,
+		.num_channels = ARRAY_SIZE(ads124s06_channels),
+	},
+};
+
+static void ads124s_fill_nop(u8 *data, int start, int count)
+{
+	int i;
+
+	for (i = start; i <= count; i++)
+		data[i] = ADS124S_CMD_NOP;
+
+};
+
+static int ads124s_write_cmd(struct iio_dev *indio_dev, u8 command)
+{
+	struct ads124s_private *priv = iio_priv(indio_dev);
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = &priv->data[0],
+			.len = 1,
+		}
+	};
+
+	priv->data[0] = command;
+
+	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
+}
+
+static int ads124s_write_reg(struct iio_dev *indio_dev, u8 reg, u8 data)
+{
+	struct ads124s_private *priv = iio_priv(indio_dev);
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = &priv->data[0],
+			.len = 3,
+		}
+	};
+
+	priv->data[0] = ADS124S_CMD_WREG | reg;
+	priv->data[1] = 0x0;
+	priv->data[2] = data;
+
+	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
+}
+
+static int ads124s_reset(struct iio_dev *indio_dev)
+{
+	struct ads124s_private *priv = iio_priv(indio_dev);
+
+	if (priv->reset_gpio) {
+		gpiod_direction_output(priv->reset_gpio, 0);
+		udelay(200);
+		gpiod_direction_output(priv->reset_gpio, 1);
+	} else {
+		return ads124s_write_cmd(indio_dev, ADS124S_CMD_RESET);
+	}
+
+	return 0;
+};
+
+static int ads124s_read(struct iio_dev *indio_dev, unsigned int chan)
+{
+	struct ads124s_private *priv = iio_priv(indio_dev);
+	int ret;
+	u32 tmp;
+	struct spi_transfer t[] = {
+		{
+			.tx_buf = &priv->data[0],
+			.len = 4,
+			.cs_change = 1,
+		}, {
+			.tx_buf = &priv->data[1],
+			.rx_buf = &priv->data[1],
+			.len = 4,
+		},
+	};
+
+	priv->data[0] = ADS124S_CMD_RDATA;
+	ads124s_fill_nop(priv->data, 1, 3);
+
+	ret = spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
+	if (ret < 0)
+		return ret;
+
+	tmp = priv->data[2] << 16 | priv->data[3] << 8 | priv->data[4];
+
+	return tmp;
+}
+
+static int ads124s_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long m)
+{
+	struct ads124s_private *priv = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&priv->lock);
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX,
+					chan->channel);
+		if (ret) {
+			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
+			goto out;
+		}
+
+		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
+		if (ret) {
+			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
+			goto out;
+		}
+
+		ret = ads124s_read(indio_dev, chan->channel);
+		if (ret < 0) {
+			dev_err(&priv->spi->dev, "Read ADC failed\n");
+			goto out;
+		} else
+			*val = ret;
+
+		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
+		if (ret) {
+			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
+			goto out;
+		}
+
+		mutex_unlock(&priv->lock);
+		if (ret < 0)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		break;
+	}
+out:
+	mutex_unlock(&priv->lock);
+	return -EINVAL;
+}
+
+static const struct iio_info ads124s_info = {
+	.read_raw = &ads124s_read_raw,
+};
+
+static irqreturn_t ads124s_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ads124s_private *priv = iio_priv(indio_dev);
+	u16 buffer[ADS124S0X_MAX_CHANNELS];
+	int i, j = 0;
+	int ret;
+
+	for (i = 0; i < indio_dev->masklength; i++) {
+		if (!test_bit(i, indio_dev->active_scan_mask))
+			continue;
+
+		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, i);
+		if (ret)
+			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
+
+		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
+		if (ret)
+			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
+
+		buffer[j] = ads124s_read(indio_dev, i);
+		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
+		if (ret)
+			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
+
+		j++;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
+			pf->timestamp);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int ads124s_probe(struct spi_device *spi)
+{
+	struct ads124s_private *ads124s_priv;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*ads124s_priv));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	ads124s_priv = iio_priv(indio_dev);
+
+	ads124s_priv->reg = devm_regulator_get_optional(&spi->dev, "vref");
+	if (!IS_ERR(ads124s_priv->reg)) {
+		ret = regulator_enable(ads124s_priv->reg);
+		if (ret)
+			return ret;
+
+		ads124s_priv->vref_mv = regulator_get_voltage(ads124s_priv->reg);
+		if (ads124s_priv->vref_mv <= 0)
+			goto err_regulator_disable;
+	} else {
+		/* Use internal reference */
+		ads124s_priv->vref_mv = 0;
+	}
+
+	ads124s_priv->reset_gpio = devm_gpiod_get_optional(&spi->dev,
+						   "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ads124s_priv->reset_gpio))
+		dev_info(&spi->dev, "Reset GPIO not defined\n");
+
+	ads124s_priv->chip_info = &ads124s_chip_info_tbl[spi_get_device_id(spi)->driver_data];
+
+	spi_set_drvdata(spi, indio_dev);
+
+	ads124s_priv->spi = spi;
+
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->dev.of_node = spi->dev.of_node;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ads124s_priv->chip_info->channels;
+	indio_dev->num_channels = ads124s_priv->chip_info->num_channels;
+	indio_dev->info = &ads124s_info;
+
+	mutex_init(&ads124s_priv->lock);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 ads124s_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
+		goto err_regulator_disable;
+	}
+
+	ads124s_reset(indio_dev);
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err_buffer_cleanup;
+
+	return 0;
+
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+err_regulator_disable:
+	if (!IS_ERR(ads124s_priv->reg))
+		regulator_disable(ads124s_priv->reg);
+
+	return ret;
+}
+
+static int ads124s_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 spi_device_id ads124s_id[] = {
+	{ "ads124s08", ADS124S08_ID },
+	{ "ads124s06", ADS124S06_ID },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ads124s_id);
+
+static const struct of_device_id ads124s_of_table[] = {
+	{ .compatible = "ti,ads124s08" },
+	{ .compatible = "ti,ads124s06" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ads124s_of_table);
+
+static struct spi_driver ads124s_driver = {
+	.driver = {
+		.name	= "ads124s",
+		.of_match_table = ads124s_of_table,
+	},
+	.probe		= ads124s_probe,
+	.remove		= ads124s_remove,
+	.id_table	= ads124s_id,
+};
+module_spi_driver(ads124s_driver);
+
+MODULE_AUTHOR("Dan Murphy <dmuprhy@ti.com>");
+MODULE_DESCRIPTION("TI TI_ADS12S0X ADC");
+MODULE_LICENSE("GPL v2");
-- 
2.20.0.rc1.10.g7068cbc4ab

Re: [PATCH 1/2] iio: ti-ads124s08: Add DT binding documentation

[Dan Murphy]

Hello

On 12/04/2018 11:59 AM, Dan Murphy wrote:
> Adding binding documentation for Texas Instruments ADS124S08
> and ADS124S06 ADC.
> 
> S08 is a 12 channel ADC
> S06 is a 6 channel ADC
> 
> Datesheet can be found here:
> http://www.ti.com/lit/gpn/ads124s08
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../bindings/iio/adc/ti-ads124s08.txt         | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt b/Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt
> new file mode 100644
> index 000000000000..41ab67379ee1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti-ads124s08.txt
> @@ -0,0 +1,25 @@
> +* Texas Instruments' ads124s08 and ads124s06 ADC chip
> +
> +Required properties:
> + - compatible: Should be "ti,ads124s08" or "ti,ads124s06"
> + - reg: spi chip select number for the device
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> +		Documentation/devicetree/bindings/spi/spi-bus.txt
> + - spi-cpha: Definition as per
> +		Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Optional properties:
> + - vref-supply: The regulator supply for ADC reference voltage
> + - reset-gpios: GPIO pin used to reset the device.
> +
> +Example:
> +adc@0 {
> +	compatible = "ti,ads8688";

This is a copy/paste error I will fix in v2.

> +	reg = <0>;
> +	vref-supply = <&vdd_supply>;
> +	spi-max-frequency = <1000000>;
> +	spi-cpha;
> +	reset-gpios = <&gpio1 16 GPIO_ACTIVE_LOW>;
> +};
> 


-- 
------------------
Dan Murphy

Re: [PATCH 2/2] iio: adc: Add the TI ads124s08 ADC code

[Jonathan Cameron]

On Tue, 4 Dec 2018 11:59:55 -0600
Dan Murphy <dmurphy@ti.com> wrote:

> Introduce the TI ADS124S08 and the ADS124S06 ADC
> devices from TI.  The ADS124S08 is the 12 channel ADC
> and the ADS124S06 is the 6 channel ADC device
> 
> These devices share a common datasheet:
> http://www.ti.com/lit/gpn/ads124s08
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
Various minor things inline.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig        |  10 +
>  drivers/iio/adc/Makefile       |   1 +
>  drivers/iio/adc/ti-ads124s08.c | 437 +++++++++++++++++++++++++++++++++
>  3 files changed, 448 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-ads124s08.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index a52fea8749a9..e8c5686e6716 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -887,6 +887,16 @@ config TI_ADS8688
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-ads8688.
>  
> +config TI_ADS124S08
> +	tristate "Texas Instruments ADS124S08"
> +	depends on SPI && OF
> +	help
> +	  If you say yes here you get support for Texas Instruments ADS124S08
> +	  and ADS124S06 ADC chips
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called ti-ads124s08.
> +
>  config TI_AM335X_ADC
>  	tristate "TI's AM335X ADC driver"
>  	depends on MFD_TI_AM335X_TSCADC && HAS_DMA
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a6e6a0b659e2..d70293abfdba 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> +obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
>  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
> new file mode 100644
> index 000000000000..b6fc93f37355
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads124s08.c
> @@ -0,0 +1,437 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// TI ADS124S0X chip family driver
> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
An oddity of current kernel style is that typically only the spdx
line is // 

The rest are a normal kernel multiline comment.

> +
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define ADS124S08_ID		0x00
> +#define ADS124S06_ID		0x01

Use an enum for these as the actual values don't matter, only that
they are unique.

> +
> +/* Commands */
> +#define ADS124S_CMD_NOP		0x00
Why this shorter prefix? Are all ADS124S parts
compatible with this list? 

> +#define ADS124S_CMD_WAKEUP	0x02
> +#define ADS124S_CMD_PWRDWN	0x04
> +#define ADS124S_CMD_RESET	0x06
> +#define ADS124S_CMD_START	0x08
> +#define ADS124S_CMD_STOP	0x0a
> +#define ADS124S_CMD_SYOCAL	0x16
> +#define ADS124S_CMD_SYGCAL	0x17
> +#define ADS124S_CMD_SFOCAL	0x19
> +#define ADS124S_CMD_RDATA	0x12
> +#define ADS124S_CMD_RREG	0x20
> +#define ADS124S_CMD_WREG	0x40
> +
> +/* Registers */
> +#define ADS124S0X_ID		0x00
Avoid wild cards in naming.  It always goes wrong when
along comes another part that is similar but not quite the
same yet fits in your naming scheme.  Just pick a part
and name after that.

> +#define ADS124S0X_STATUS	0x01
> +#define ADS124S0X_INPUT_MUX	0x02
> +#define ADS124S0X_PGA		0x03
> +#define ADS124S0X_DATA_RATE	0x04
> +#define ADS124S0X_REF		0x05
> +#define ADS124S0X_IDACMAG	0x06
> +#define ADS124S0X_IDACMUX	0x07
> +#define ADS124S0X_VBIAS		0x08
> +#define ADS124S0X_SYS		0x09
> +#define ADS124S0X_OFCAL0	0x0a
> +#define ADS124S0X_OFCAL1	0x0b
> +#define ADS124S0X_OFCAL2	0x0c
> +#define ADS124S0X_FSCAL0	0x0d
> +#define ADS124S0X_FSCAL1	0x0e
> +#define ADS124S0X_FSCAL2	0x0f
> +#define ADS124S0X_GPIODAT	0x10
> +#define ADS124S0X_GPIOCON	0x11
> +
> +/* ADS124S0x common channels */
> +#define ADS124S0X_AIN0		0x00
> +#define ADS124S0X_AIN1		0x01
> +#define ADS124S0X_AIN2		0x02
> +#define ADS124S0X_AIN3		0x03
> +#define ADS124S0X_AIN4		0x04
> +#define ADS124S0X_AIN5		0x05
> +#define ADS124S08_AINCOM	0x0c
> +/* ADS124S08 only channels */
> +#define ADS124S08_AIN6		0x06
> +#define ADS124S08_AIN7		0x07
> +#define ADS124S08_AIN8		0x08
> +#define ADS124S08_AIN9		0x09
> +#define ADS124S08_AIN10		0x0a
> +#define ADS124S08_AIN11		0x0b
> +#define ADS124S0X_MAX_CHANNELS	12
> +
> +#define ADS124S0X_POS_MUX_SHIFT	0x04
> +#define ADS124S_INT_REF		0x09
> +
> +#define ADS124S_START_REG_MASK	0x1f
> +#define ADS124S_NUM_BYTES_MASK	0x1f
> +
> +#define ADS124S_START_CONV	0x01
> +#define ADS124S_STOP_CONV	0x00
> +
> +struct ads124s_chip_info {
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
> +};
> +
> +struct ads124s_private {
> +	const struct ads124s_chip_info	*chip_info;
> +	struct gpio_desc *reset_gpio;
> +	struct spi_device *spi;
> +	struct regulator *reg;
> +	struct mutex lock;
> +	unsigned int vref_mv;
> +	u8 data[ADS124S0X_MAX_CHANNELS];
This will break in horrible ways on spi controllers using DMA.
The data buffer needs to be in it's own cacheline to avoid
possibly overwriting data in the rest of this structure.

___cache_line_aligned is your friend.   Wolfram did a very
good talk on this issue at elce. 
https://events.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf

There's a video on youtube as well.

He was addressing i2c, but the arguments still hold and unlike
i2c, spi has always had dma controllers who assume they have
dma safe buffers.

> +};
> +
> +#define ADS124S_CHAN(index)					\
> +{								\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = index,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\

It looks like you started adding scale but didn't finish doing so.
It's definitely a good thing to have so please see if that can be
added for V2.

> +	.scan_index = index,					\
> +	.scan_type = {						\
> +		.sign = 'u',					\
> +		.realbits = 24,					\
> +		.storagebits = 24,				\
> +		.endianness = IIO_BE,				\
> +	},							\
> +}
> +
> +static const struct iio_chan_spec ads124s06_channels[] = {
> +	ADS124S_CHAN(0),
> +	ADS124S_CHAN(1),
> +	ADS124S_CHAN(2),
> +	ADS124S_CHAN(3),
> +	ADS124S_CHAN(4),
> +	ADS124S_CHAN(5),
> +};
> +
> +static const struct iio_chan_spec ads124s08_channels[] = {
> +	ADS124S_CHAN(0),
> +	ADS124S_CHAN(1),
> +	ADS124S_CHAN(2),
> +	ADS124S_CHAN(3),
> +	ADS124S_CHAN(4),
> +	ADS124S_CHAN(5),
> +	ADS124S_CHAN(6),
> +	ADS124S_CHAN(7),
> +	ADS124S_CHAN(8),
> +	ADS124S_CHAN(9),
> +	ADS124S_CHAN(10),
> +	ADS124S_CHAN(11),
> +};
> +
> +static const struct ads124s_chip_info ads124s_chip_info_tbl[] = {
> +	[ADS124S08_ID] = {
> +		.channels = ads124s08_channels,
> +		.num_channels = ARRAY_SIZE(ads124s08_channels),
> +	},
> +	[ADS124S06_ID] = {
> +		.channels = ads124s06_channels,
> +		.num_channels = ARRAY_SIZE(ads124s06_channels),
> +	},
> +};
> +
> +static void ads124s_fill_nop(u8 *data, int start, int count)
> +{
> +	int i;
> +
> +	for (i = start; i <= count; i++)
> +		data[i] = ADS124S_CMD_NOP;

memset

> +
> +};
> +
> +static int ads124s_write_cmd(struct iio_dev *indio_dev, u8 command)
> +{
> +	struct ads124s_private *priv = iio_priv(indio_dev);
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &priv->data[0],
> +			.len = 1,
> +		}
> +	};
> +
> +	priv->data[0] = command;
> +
> +	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));

spi_write?

> +}
> +
> +static int ads124s_write_reg(struct iio_dev *indio_dev, u8 reg, u8 data)
> +{
> +	struct ads124s_private *priv = iio_priv(indio_dev);
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &priv->data[0],
> +			.len = 3,
> +		}
> +	};
> +
> +	priv->data[0] = ADS124S_CMD_WREG | reg;
> +	priv->data[1] = 0x0;
> +	priv->data[2] = data;
> +
> +	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));

spi_write?

> +}
> +
> +static int ads124s_reset(struct iio_dev *indio_dev)
> +{
> +	struct ads124s_private *priv = iio_priv(indio_dev);
> +
> +	if (priv->reset_gpio) {
> +		gpiod_direction_output(priv->reset_gpio, 0);
I'd expect that gpio to always be in the output
direction, so just being controlled here.

So gpiod_set_value

> +		udelay(200);
> +		gpiod_direction_output(priv->reset_gpio, 1);
> +	} else {
> +		return ads124s_write_cmd(indio_dev, ADS124S_CMD_RESET);
> +	}
> +
> +	return 0;
> +};
> +
> +static int ads124s_read(struct iio_dev *indio_dev, unsigned int chan)
> +{
> +	struct ads124s_private *priv = iio_priv(indio_dev);
> +	int ret;
> +	u32 tmp;
> +	struct spi_transfer t[] = {
> +		{
> +			.tx_buf = &priv->data[0],
> +			.len = 4,
> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = &priv->data[1],
> +			.rx_buf = &priv->data[1],
> +			.len = 4,
> +		},
> +	};

Pity we don't have a spi_write_the_read_with_cs or
something like that.  I wonder how common this structure is?

> +
> +	priv->data[0] = ADS124S_CMD_RDATA;
> +	ads124s_fill_nop(priv->data, 1, 3);
> +
> +	ret = spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
> +	if (ret < 0)
> +		return ret;
> +
> +	tmp = priv->data[2] << 16 | priv->data[3] << 8 | priv->data[4];
> +
> +	return tmp;
> +}
> +
> +static int ads124s_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long m)
> +{
> +	struct ads124s_private *priv = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX,
> +					chan->channel);
> +		if (ret) {
> +			dev_err(&priv->spi->dev, "Set ADC CH failed\n");

You are eating a bus error in favour of -EINVAL.  Please don't!

> +			goto out;
> +		}
> +
> +		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
> +		if (ret) {
> +			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
> +			goto out;
> +		}
> +
> +		ret = ads124s_read(indio_dev, chan->channel);
> +		if (ret < 0) {
> +			dev_err(&priv->spi->dev, "Read ADC failed\n");
> +			goto out;
> +		} else
> +			*val = ret;

This is the same code block as found in the buffered version below.  Factor
it out to a utility function and use it both paths.

> +
> +		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
> +		if (ret) {
> +			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
> +			goto out;
> +		}
> +
> +		mutex_unlock(&priv->lock);
> +		if (ret < 0)
> +			return ret;
Given you need to unlock in both this path and the error path, cleaner
to just set ret here and break.  Then return ret below having set
it appropriately in the error paths.
> +
> +		return IIO_VAL_INT;
> +	default:
> +		break;
> +	}
> +out:
> +	mutex_unlock(&priv->lock);
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info ads124s_info = {
> +	.read_raw = &ads124s_read_raw,
> +};
> +
> +static irqreturn_t ads124s_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ads124s_private *priv = iio_priv(indio_dev);
> +	u16 buffer[ADS124S0X_MAX_CHANNELS];

There is an unfortunate oddity in how push_to_buffers_with_timestamp
works in that it builds the data for the kfifo inline in the buffer
provided.  Thus that buffer has to have room for the channels and
the 64 bit timestamp.

> +	int i, j = 0;
> +	int ret;
> +
> +	for (i = 0; i < indio_dev->masklength; i++) {
> +		if (!test_bit(i, indio_dev->active_scan_mask))

for_each_bit_set 

> +			continue;
> +
> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, i);

Does this need to be rewritten if the channel is already correct?

> +		if (ret)
> +			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
> +
> +		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
> +		if (ret)
> +			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
> +
> +		buffer[j] = ads124s_read(indio_dev, i);
> +		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
> +		if (ret)
> +			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
> +
> +		j++;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> +			pf->timestamp);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ads124s_probe(struct spi_device *spi)
> +{
> +	struct ads124s_private *ads124s_priv;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*ads124s_priv));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	ads124s_priv = iio_priv(indio_dev);
> +
> +	ads124s_priv->reg = devm_regulator_get_optional(&spi->dev, "vref");
> +	if (!IS_ERR(ads124s_priv->reg)) {
> +		ret = regulator_enable(ads124s_priv->reg);
> +		if (ret)
> +			return ret;

	As mentioned below, use devm_add_action_or_reset to provide
	automatic disabling of this regulator letting you used managed
	cleanup throughout. 

> +
> +		ads124s_priv->vref_mv = regulator_get_voltage(ads124s_priv->reg);

This doesn't actually seem to be used...  As a general rule the only time you 
want to read this is to provide a scale value.  As that isn't a fast path
it's better to read it where needed in case the value is changing (not unusual
during boot up if the voltage is being used for several things).

> +		if (ads124s_priv->vref_mv <= 0)
> +			goto err_regulator_disable;
> +	} else {
> +		/* Use internal reference */
> +		ads124s_priv->vref_mv = 0;
> +	}
> +
> +	ads124s_priv->reset_gpio = devm_gpiod_get_optional(&spi->dev,
> +						   "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ads124s_priv->reset_gpio))
> +		dev_info(&spi->dev, "Reset GPIO not defined\n");
> +
> +	ads124s_priv->chip_info = &ads124s_chip_info_tbl[spi_get_device_id(spi)->driver_data];
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	ads124s_priv->spi = spi;
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->dev.of_node = spi->dev.of_node;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ads124s_priv->chip_info->channels;
> +	indio_dev->num_channels = ads124s_priv->chip_info->num_channels;
> +	indio_dev->info = &ads124s_info;
> +
> +	mutex_init(&ads124s_priv->lock);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 ads124s_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
> +		goto err_regulator_disable;
> +	}
> +
> +	ads124s_reset(indio_dev);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err_buffer_cleanup;
> +
> +	return 0;
> +
> +err_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +err_regulator_disable:
> +	if (!IS_ERR(ads124s_priv->reg))
> +		regulator_disable(ads124s_priv->reg);
> +
> +	return ret;
> +}
> +
> +static int ads124s_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);
Both of these functions can be automatically cleaned
up if you use the devm_ variants of the registration functions
which makes me suspicious..  Ah. The regulator disable
is missing.

You can move to fully managed if you use devm_add_action_or_reset
to turn the regulator off.

> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id ads124s_id[] = {
> +	{ "ads124s08", ADS124S08_ID },
> +	{ "ads124s06", ADS124S06_ID },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ads124s_id);
> +
> +static const struct of_device_id ads124s_of_table[] = {
> +	{ .compatible = "ti,ads124s08" },
> +	{ .compatible = "ti,ads124s06" },

It's trivial but numerical order preferred as it makes it
obvious where to add new devices later.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ads124s_of_table);
> +
> +static struct spi_driver ads124s_driver = {
> +	.driver = {
> +		.name	= "ads124s",
> +		.of_match_table = ads124s_of_table,
> +	},
> +	.probe		= ads124s_probe,
> +	.remove		= ads124s_remove,
> +	.id_table	= ads124s_id,
> +};
> +module_spi_driver(ads124s_driver);
> +
> +MODULE_AUTHOR("Dan Murphy <dmuprhy@ti.com>");
> +MODULE_DESCRIPTION("TI TI_ADS12S0X ADC");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 2/2] iio: adc: Add the TI ads124s08 ADC code

[Dan Murphy]

Jonathan

Thanks for the review

On 12/08/2018 05:56 AM, Jonathan Cameron wrote:
> On Tue, 4 Dec 2018 11:59:55 -0600
> Dan Murphy <dmurphy@ti.com> wrote:
> 
>> Introduce the TI ADS124S08 and the ADS124S06 ADC
>> devices from TI.  The ADS124S08 is the 12 channel ADC
>> and the ADS124S06 is the 6 channel ADC device
>>
>> These devices share a common datasheet:
>> http://www.ti.com/lit/gpn/ads124s08
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> Various minor things inline.
> 

No worries.  I believe I used the ADS8688 driver as a reference so that driver
may need to be updated as well

> Thanks,
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/Kconfig        |  10 +
>>  drivers/iio/adc/Makefile       |   1 +
>>  drivers/iio/adc/ti-ads124s08.c | 437 +++++++++++++++++++++++++++++++++
>>  3 files changed, 448 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-ads124s08.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index a52fea8749a9..e8c5686e6716 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -887,6 +887,16 @@ config TI_ADS8688
>>  	  This driver can also be built as a module. If so, the module will be
>>  	  called ti-ads8688.
>>  
>> +config TI_ADS124S08
>> +	tristate "Texas Instruments ADS124S08"
>> +	depends on SPI && OF
>> +	help
>> +	  If you say yes here you get support for Texas Instruments ADS124S08
>> +	  and ADS124S06 ADC chips
>> +
>> +	  This driver can also be built as a module. If so, the module will be
>> +	  called ti-ads124s08.
>> +
>>  config TI_AM335X_ADC
>>  	tristate "TI's AM335X ADC driver"
>>  	depends on MFD_TI_AM335X_TSCADC && HAS_DMA
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index a6e6a0b659e2..d70293abfdba 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -79,6 +79,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
>>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>> +obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>>  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
>>  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
>> diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
>> new file mode 100644
>> index 000000000000..b6fc93f37355
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads124s08.c
>> @@ -0,0 +1,437 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// TI ADS124S0X chip family driver
>> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
> An oddity of current kernel style is that typically only the spdx
> line is // 
> 
> The rest are a normal kernel multiline comment.

Ack.  Finding it is up to the maintainer here on the preference so I will change it.

> 
>> +
>> +#include <linux/err.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define ADS124S08_ID		0x00
>> +#define ADS124S06_ID		0x01
> 
> Use an enum for these as the actual values don't matter, only that
> they are unique.
> 

Ack.  Is there a preference on using the sentinnal or not?

>> +
>> +/* Commands */
>> +#define ADS124S_CMD_NOP		0x00
> Why this shorter prefix? Are all ADS124S parts
> compatible with this list? 
> 

Ack.  Yes the 124S are all compatible since there are only 2 ATM.


>> +#define ADS124S_CMD_WAKEUP	0x02
>> +#define ADS124S_CMD_PWRDWN	0x04
>> +#define ADS124S_CMD_RESET	0x06
>> +#define ADS124S_CMD_START	0x08
>> +#define ADS124S_CMD_STOP	0x0a
>> +#define ADS124S_CMD_SYOCAL	0x16
>> +#define ADS124S_CMD_SYGCAL	0x17
>> +#define ADS124S_CMD_SFOCAL	0x19
>> +#define ADS124S_CMD_RDATA	0x12
>> +#define ADS124S_CMD_RREG	0x20
>> +#define ADS124S_CMD_WREG	0x40
>> +
>> +/* Registers */
>> +#define ADS124S0X_ID		0x00
> Avoid wild cards in naming.  It always goes wrong when
> along comes another part that is similar but not quite the
> same yet fits in your naming scheme.  Just pick a part
> and name after that.

The issue is that there are some registers below that are S08 only
and do not apply to the S06.

This is why I choose the wild card.  I can use the 08 since that has a master set.
Unless I can keep the 0X

> 
>> +#define ADS124S0X_STATUS	0x01
>> +#define ADS124S0X_INPUT_MUX	0x02
>> +#define ADS124S0X_PGA		0x03
>> +#define ADS124S0X_DATA_RATE	0x04
>> +#define ADS124S0X_REF		0x05
>> +#define ADS124S0X_IDACMAG	0x06
>> +#define ADS124S0X_IDACMUX	0x07
>> +#define ADS124S0X_VBIAS		0x08
>> +#define ADS124S0X_SYS		0x09
>> +#define ADS124S0X_OFCAL0	0x0a
>> +#define ADS124S0X_OFCAL1	0x0b
>> +#define ADS124S0X_OFCAL2	0x0c
>> +#define ADS124S0X_FSCAL0	0x0d
>> +#define ADS124S0X_FSCAL1	0x0e
>> +#define ADS124S0X_FSCAL2	0x0f
>> +#define ADS124S0X_GPIODAT	0x10
>> +#define ADS124S0X_GPIOCON	0x11
>> +
>> +/* ADS124S0x common channels */
>> +#define ADS124S0X_AIN0		0x00
>> +#define ADS124S0X_AIN1		0x01
>> +#define ADS124S0X_AIN2		0x02
>> +#define ADS124S0X_AIN3		0x03
>> +#define ADS124S0X_AIN4		0x04
>> +#define ADS124S0X_AIN5		0x05
>> +#define ADS124S08_AINCOM	0x0c
>> +/* ADS124S08 only channels */
>> +#define ADS124S08_AIN6		0x06
>> +#define ADS124S08_AIN7		0x07
>> +#define ADS124S08_AIN8		0x08
>> +#define ADS124S08_AIN9		0x09
>> +#define ADS124S08_AIN10		0x0a
>> +#define ADS124S08_AIN11		0x0b
>> +#define ADS124S0X_MAX_CHANNELS	12
>> +
>> +#define ADS124S0X_POS_MUX_SHIFT	0x04
>> +#define ADS124S_INT_REF		0x09
>> +
>> +#define ADS124S_START_REG_MASK	0x1f
>> +#define ADS124S_NUM_BYTES_MASK	0x1f
>> +
>> +#define ADS124S_START_CONV	0x01
>> +#define ADS124S_STOP_CONV	0x00
>> +
>> +struct ads124s_chip_info {
>> +	const struct iio_chan_spec *channels;
>> +	unsigned int num_channels;
>> +};
>> +
>> +struct ads124s_private {
>> +	const struct ads124s_chip_info	*chip_info;
>> +	struct gpio_desc *reset_gpio;
>> +	struct spi_device *spi;
>> +	struct regulator *reg;
>> +	struct mutex lock;
>> +	unsigned int vref_mv;
>> +	u8 data[ADS124S0X_MAX_CHANNELS];
> This will break in horrible ways on spi controllers using DMA.
> The data buffer needs to be in it's own cacheline to avoid
> possibly overwriting data in the rest of this structure.
> 
> ___cache_line_aligned is your friend.   Wolfram did a very
> good talk on this issue at elce. 
> https://events.linuxfoundation.org/wp-content/uploads/2017/12/20181023-Wolfram-Sang-ELCE18-safe_dma_buffers.pdf
> 
> There's a video on youtube as well.
> 
> He was addressing i2c, but the arguments still hold and unlike
> i2c, spi has always had dma controllers who assume they have
> dma safe buffers.
> 

ACK.  I will look into this.  Probably will do something similar to the 8688 buffer

>> +};
>> +
>> +#define ADS124S_CHAN(index)					\
>> +{								\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.channel = index,					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> 
> It looks like you started adding scale but didn't finish doing so.
> It's definitely a good thing to have so please see if that can be
> added for V2.

Yes I did but could not figure out how to apply it to this part. I will investigate this in v2

> 
>> +	.scan_index = index,					\
>> +	.scan_type = {						\
>> +		.sign = 'u',					\
>> +		.realbits = 24,					\
>> +		.storagebits = 24,				\
>> +		.endianness = IIO_BE,				\
>> +	},							\
>> +}
>> +
>> +static const struct iio_chan_spec ads124s06_channels[] = {
>> +	ADS124S_CHAN(0),
>> +	ADS124S_CHAN(1),
>> +	ADS124S_CHAN(2),
>> +	ADS124S_CHAN(3),
>> +	ADS124S_CHAN(4),
>> +	ADS124S_CHAN(5),
>> +};
>> +
>> +static const struct iio_chan_spec ads124s08_channels[] = {
>> +	ADS124S_CHAN(0),
>> +	ADS124S_CHAN(1),
>> +	ADS124S_CHAN(2),
>> +	ADS124S_CHAN(3),
>> +	ADS124S_CHAN(4),
>> +	ADS124S_CHAN(5),
>> +	ADS124S_CHAN(6),
>> +	ADS124S_CHAN(7),
>> +	ADS124S_CHAN(8),
>> +	ADS124S_CHAN(9),
>> +	ADS124S_CHAN(10),
>> +	ADS124S_CHAN(11),
>> +};
>> +
>> +static const struct ads124s_chip_info ads124s_chip_info_tbl[] = {
>> +	[ADS124S08_ID] = {
>> +		.channels = ads124s08_channels,
>> +		.num_channels = ARRAY_SIZE(ads124s08_channels),
>> +	},
>> +	[ADS124S06_ID] = {
>> +		.channels = ads124s06_channels,
>> +		.num_channels = ARRAY_SIZE(ads124s06_channels),
>> +	},
>> +};
>> +
>> +static void ads124s_fill_nop(u8 *data, int start, int count)
>> +{
>> +	int i;
>> +
>> +	for (i = start; i <= count; i++)
>> +		data[i] = ADS124S_CMD_NOP;
> 
> memset
> 

ACK

>> +
>> +};
>> +
>> +static int ads124s_write_cmd(struct iio_dev *indio_dev, u8 command)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	struct spi_transfer t[] = {
>> +		{
>> +			.tx_buf = &priv->data[0],
>> +			.len = 1,
>> +		}
>> +	};
>> +
>> +	priv->data[0] = command;
>> +
>> +	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
> 
> spi_write?

ACK

> 
>> +}
>> +
>> +static int ads124s_write_reg(struct iio_dev *indio_dev, u8 reg, u8 data)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	struct spi_transfer t[] = {
>> +		{
>> +			.tx_buf = &priv->data[0],
>> +			.len = 3,
>> +		}
>> +	};
>> +
>> +	priv->data[0] = ADS124S_CMD_WREG | reg;
>> +	priv->data[1] = 0x0;
>> +	priv->data[2] = data;
>> +
>> +	return spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
> 
> spi_write?
> 

ACK

>> +}
>> +
>> +static int ads124s_reset(struct iio_dev *indio_dev)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +
>> +	if (priv->reset_gpio) {
>> +		gpiod_direction_output(priv->reset_gpio, 0);
> I'd expect that gpio to always be in the output
> direction, so just being controlled here.
> 
> So gpiod_set_value

ACK

> 
>> +		udelay(200);
>> +		gpiod_direction_output(priv->reset_gpio, 1);
>> +	} else {
>> +		return ads124s_write_cmd(indio_dev, ADS124S_CMD_RESET);
>> +	}
>> +
>> +	return 0;
>> +};
>> +
>> +static int ads124s_read(struct iio_dev *indio_dev, unsigned int chan)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	int ret;
>> +	u32 tmp;
>> +	struct spi_transfer t[] = {
>> +		{
>> +			.tx_buf = &priv->data[0],
>> +			.len = 4,
>> +			.cs_change = 1,
>> +		}, {
>> +			.tx_buf = &priv->data[1],
>> +			.rx_buf = &priv->data[1],
>> +			.len = 4,
>> +		},
>> +	};
> 
> Pity we don't have a spi_write_the_read_with_cs or
> something like that.  I wonder how common this structure is?

This is common with this part and the ads8688 and another spi part I am working on for CAN.

I just don't know if there are any parts that hold CS for more then one data xfer.

> 
>> +
>> +	priv->data[0] = ADS124S_CMD_RDATA;
>> +	ads124s_fill_nop(priv->data, 1, 3);
>> +
>> +	ret = spi_sync_transfer(priv->spi, t, ARRAY_SIZE(t));
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	tmp = priv->data[2] << 16 | priv->data[3] << 8 | priv->data[4];
>> +
>> +	return tmp;
>> +}
>> +
>> +static int ads124s_read_raw(struct iio_dev *indio_dev,
>> +			    struct iio_chan_spec const *chan,
>> +			    int *val, int *val2, long m)
>> +{
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	mutex_lock(&priv->lock);
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX,
>> +					chan->channel);
>> +		if (ret) {
>> +			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
> 
> You are eating a bus error in favour of -EINVAL.  Please don't!

ACK.  I was debating on setting ret here so I will set ret and return that instead.
Rinse and repeat for the ones below too.

> 
>> +			goto out;
>> +		}
>> +
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
>> +		if (ret) {
>> +			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
>> +			goto out;
>> +		}
>> +
>> +		ret = ads124s_read(indio_dev, chan->channel);
>> +		if (ret < 0) {
>> +			dev_err(&priv->spi->dev, "Read ADC failed\n");
>> +			goto out;
>> +		} else
>> +			*val = ret;
> 
> This is the same code block as found in the buffered version below.  Factor
> it out to a utility function and use it both paths.

It is not exactly the same but I will see where I can consolidate the code.
The buffered version loops through and reads every channel this reads just one.

> 
>> +
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
>> +		if (ret) {
>> +			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
>> +			goto out;
>> +		}
>> +
>> +		mutex_unlock(&priv->lock);
>> +		if (ret < 0)
>> +			return ret;
> Given you need to unlock in both this path and the error path, cleaner
> to just set ret here and break.  Then return ret below having set
> it appropriately in the error paths.

ACK.  Similar to the above.

>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		break;
>> +	}
>> +out:
>> +	mutex_unlock(&priv->lock);
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info ads124s_info = {
>> +	.read_raw = &ads124s_read_raw,
>> +};
>> +
>> +static irqreturn_t ads124s_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct ads124s_private *priv = iio_priv(indio_dev);
>> +	u16 buffer[ADS124S0X_MAX_CHANNELS];
> 
> There is an unfortunate oddity in how push_to_buffers_with_timestamp
> works in that it builds the data for the kfifo inline in the buffer
> provided.  Thus that buffer has to have room for the channels and
> the 64 bit timestamp.

Hmmm.  This is like the 8688.  I am starting to think the 8688 needs to be updated.

> 
>> +	int i, j = 0;
>> +	int ret;
>> +
>> +	for (i = 0; i < indio_dev->masklength; i++) {
>> +		if (!test_bit(i, indio_dev->active_scan_mask))
> 
> for_each_bit_set 

Same as above.  But I can change it.

> 
>> +			continue;
>> +
>> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, i);
> 
> Does this need to be rewritten if the channel is already correct?

This is an iterative loop so the channel should be different each time 0 - 12

> 
>> +		if (ret)
>> +			dev_err(&priv->spi->dev, "Set ADC CH failed\n");
>> +
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_START_CONV);
>> +		if (ret)
>> +			dev_err(&priv->spi->dev, "Start ADC converions failed\n");
>> +
>> +		buffer[j] = ads124s_read(indio_dev, i);
>> +		ret = ads124s_write_cmd(indio_dev, ADS124S_STOP_CONV);
>> +		if (ret)
>> +			dev_err(&priv->spi->dev, "Stop ADC converions failed\n");
>> +
>> +		j++;
>> +	}
>> +
>> +	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>> +			pf->timestamp);
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ads124s_probe(struct spi_device *spi)
>> +{
>> +	struct ads124s_private *ads124s_priv;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*ads124s_priv));
>> +	if (indio_dev == NULL)
>> +		return -ENOMEM;
>> +
>> +	ads124s_priv = iio_priv(indio_dev);
>> +
>> +	ads124s_priv->reg = devm_regulator_get_optional(&spi->dev, "vref");
>> +	if (!IS_ERR(ads124s_priv->reg)) {
>> +		ret = regulator_enable(ads124s_priv->reg);
>> +		if (ret)
>> +			return ret;
> 
> 	As mentioned below, use devm_add_action_or_reset to provide
> 	automatic disabling of this regulator letting you used managed
> 	cleanup throughout. 

ACK.  will look into these APIs

> 
>> +
>> +		ads124s_priv->vref_mv = regulator_get_voltage(ads124s_priv->reg);
> 
> This doesn't actually seem to be used...  As a general rule the only time you 
> want to read this is to provide a scale value.  As that isn't a fast path
> it's better to read it where needed in case the value is changing (not unusual
> during boot up if the voltage is being used for several things).

I will look into this if I add the scale otherwise I will remove it.

> 
>> +		if (ads124s_priv->vref_mv <= 0)
>> +			goto err_regulator_disable;
>> +	} else {
>> +		/* Use internal reference */
>> +		ads124s_priv->vref_mv = 0;
>> +	}
>> +
>> +	ads124s_priv->reset_gpio = devm_gpiod_get_optional(&spi->dev,
>> +						   "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ads124s_priv->reset_gpio))
>> +		dev_info(&spi->dev, "Reset GPIO not defined\n");
>> +
>> +	ads124s_priv->chip_info = &ads124s_chip_info_tbl[spi_get_device_id(spi)->driver_data];
>> +
>> +	spi_set_drvdata(spi, indio_dev);
>> +
>> +	ads124s_priv->spi = spi;
>> +
>> +	indio_dev->name = spi_get_device_id(spi)->name;
>> +	indio_dev->dev.parent = &spi->dev;
>> +	indio_dev->dev.of_node = spi->dev.of_node;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = ads124s_priv->chip_info->channels;
>> +	indio_dev->num_channels = ads124s_priv->chip_info->num_channels;
>> +	indio_dev->info = &ads124s_info;
>> +
>> +	mutex_init(&ads124s_priv->lock);
>> +
>> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +					 ads124s_trigger_handler, NULL);
>> +	if (ret < 0) {
>> +		dev_err(&spi->dev, "iio triggered buffer setup failed\n");
>> +		goto err_regulator_disable;
>> +	}
>> +
>> +	ads124s_reset(indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret)
>> +		goto err_buffer_cleanup;
>> +
>> +	return 0;
>> +
>> +err_buffer_cleanup:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +err_regulator_disable:
>> +	if (!IS_ERR(ads124s_priv->reg))
>> +		regulator_disable(ads124s_priv->reg);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ads124s_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);
> Both of these functions can be automatically cleaned
> up if you use the devm_ variants of the registration functions
> which makes me suspicious..  Ah. The regulator disable
> is missing.

Ack.  Regulator is based on above

> 
> You can move to fully managed if you use devm_add_action_or_reset
> to turn the regulator off.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id ads124s_id[] = {
>> +	{ "ads124s08", ADS124S08_ID },
>> +	{ "ads124s06", ADS124S06_ID },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, ads124s_id);
>> +
>> +static const struct of_device_id ads124s_of_table[] = {
>> +	{ .compatible = "ti,ads124s08" },
>> +	{ .compatible = "ti,ads124s06" },
> 
> It's trivial but numerical order preferred as it makes it
> obvious where to add new devices later.

Actually I based these off the ID read from the device.
The S08 ID is 0x0 and the S06 is 0x1.  Bit I can reorder this since it just the compatible.

Dan

> 
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, ads124s_of_table);
>> +
>> +static struct spi_driver ads124s_driver = {
>> +	.driver = {
>> +		.name	= "ads124s",
>> +		.of_match_table = ads124s_of_table,
>> +	},
>> +	.probe		= ads124s_probe,
>> +	.remove		= ads124s_remove,
>> +	.id_table	= ads124s_id,
>> +};
>> +module_spi_driver(ads124s_driver);
>> +
>> +MODULE_AUTHOR("Dan Murphy <dmuprhy@ti.com>");
>> +MODULE_DESCRIPTION("TI TI_ADS12S0X ADC");
>> +MODULE_LICENSE("GPL v2");
> 


-- 
------------------
Dan Murphy

Re: [PATCH 2/2] iio: adc: Add the TI ads124s08 ADC code

[Jonathan Cameron]

Hi Dan,

Replies to queries inline.

On Mon, 10 Dec 2018 14:14:55 -0600
Dan Murphy <dmurphy@ti.com> wrote:

> Jonathan
> 
> Thanks for the review
> 
> On 12/08/2018 05:56 AM, Jonathan Cameron wrote:
> > On Tue, 4 Dec 2018 11:59:55 -0600
> > Dan Murphy <dmurphy@ti.com> wrote:
> >   
> >> Introduce the TI ADS124S08 and the ADS124S06 ADC
> >> devices from TI.  The ADS124S08 is the 12 channel ADC
> >> and the ADS124S06 is the 6 channel ADC device
> >>
> >> These devices share a common datasheet:
> >> http://www.ti.com/lit/gpn/ads124s08
> >>
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>  
> > Various minor things inline.
> >   
> 
> No worries.  I believe I used the ADS8688 driver as a reference so that driver
> may need to be updated as well

Great if you don't mind fixing the buffer size issue in there as well.

> 
> > Thanks,
> > 
> > Jonathan
> >   
> >> ---
> >>  drivers/iio/adc/Kconfig        |  10 +
> >>  drivers/iio/adc/Makefile       |   1 +
> >>  drivers/iio/adc/ti-ads124s08.c | 437 +++++++++++++++++++++++++++++++++
> >>  3 files changed, 448 insertions(+)
> >>  create mode 100644 drivers/iio/adc/ti-ads124s08.c
> >>
> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >> index a52fea8749a9..e8c5686e6716 100644
> >> --- a/drivers/iio/adc/Kconfig
> >> +++ b/drivers/iio/adc/Kconfig
> >> @@ -887,6 +887,16 @@ config TI_ADS8688
> >>  	  This driver can also be built as a module. If so, the module will be
> >>  	  called ti-ads8688.
> >>  
> >> +config TI_ADS124S08
> >> +	tristate "Texas Instruments ADS124S08"
> >> +	depends on SPI && OF
> >> +	help
> >> +	  If you say yes here you get support for Texas Instruments ADS124S08
> >> +	  and ADS124S06 ADC chips
> >> +
> >> +	  This driver can also be built as a module. If so, the module will be
> >> +	  called ti-ads124s08.
> >> +
> >>  config TI_AM335X_ADC
> >>  	tristate "TI's AM335X ADC driver"
> >>  	depends on MFD_TI_AM335X_TSCADC && HAS_DMA
> >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> index a6e6a0b659e2..d70293abfdba 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -79,6 +79,7 @@ obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
> >>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> >>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> >>  obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> >> +obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> >>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> >>  obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
> >>  obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> >> diff --git a/drivers/iio/adc/ti-ads124s08.c b/drivers/iio/adc/ti-ads124s08.c
> >> new file mode 100644
> >> index 000000000000..b6fc93f37355
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/ti-ads124s08.c
> >> @@ -0,0 +1,437 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// TI ADS124S0X chip family driver
> >> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/  
> > An oddity of current kernel style is that typically only the spdx
> > line is // 
> > 
> > The rest are a normal kernel multiline comment.  
> 
> Ack.  Finding it is up to the maintainer here on the preference so I will change it.
> 
> >   
> >> +
> >> +#include <linux/err.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/device.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_gpio.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/sysfs.h>
> >> +
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/regulator/consumer.h>
> >> +#include <linux/spi/spi.h>
> >> +
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/iio/buffer.h>
> >> +#include <linux/iio/trigger_consumer.h>
> >> +#include <linux/iio/triggered_buffer.h>
> >> +#include <linux/iio/sysfs.h>
> >> +
> >> +#define ADS124S08_ID		0x00
> >> +#define ADS124S06_ID		0x01  
> > 
> > Use an enum for these as the actual values don't matter, only that
> > they are unique.
> >   
> 
> Ack.  Is there a preference on using the sentinnal or not?

If the sentinel is useful for a loop or range check then yes, if not
then don't bother.

> 
> >> +
> >> +/* Commands */
> >> +#define ADS124S_CMD_NOP		0x00  
> > Why this shorter prefix? Are all ADS124S parts
> > compatible with this list? 
> >   
> 
> Ack.  Yes the 124S are all compatible since there are only 2 ATM.
> 
> 
> >> +#define ADS124S_CMD_WAKEUP	0x02
> >> +#define ADS124S_CMD_PWRDWN	0x04
> >> +#define ADS124S_CMD_RESET	0x06
> >> +#define ADS124S_CMD_START	0x08
> >> +#define ADS124S_CMD_STOP	0x0a
> >> +#define ADS124S_CMD_SYOCAL	0x16
> >> +#define ADS124S_CMD_SYGCAL	0x17
> >> +#define ADS124S_CMD_SFOCAL	0x19
> >> +#define ADS124S_CMD_RDATA	0x12
> >> +#define ADS124S_CMD_RREG	0x20
> >> +#define ADS124S_CMD_WREG	0x40
> >> +
> >> +/* Registers */
> >> +#define ADS124S0X_ID		0x00  
> > Avoid wild cards in naming.  It always goes wrong when
> > along comes another part that is similar but not quite the
> > same yet fits in your naming scheme.  Just pick a part
> > and name after that.  
> 
> The issue is that there are some registers below that are S08 only
> and do not apply to the S06.
> 
> This is why I choose the wild card.  I can use the 08 since that has a master set.
> Unless I can keep the 0X

Use 08 seems like a good solution as they are definitely valid for that.

...

> >> +
> >> +static int ads124s_read(struct iio_dev *indio_dev, unsigned int chan)
> >> +{
> >> +	struct ads124s_private *priv = iio_priv(indio_dev);
> >> +	int ret;
> >> +	u32 tmp;
> >> +	struct spi_transfer t[] = {
> >> +		{
> >> +			.tx_buf = &priv->data[0],
> >> +			.len = 4,
> >> +			.cs_change = 1,
> >> +		}, {
> >> +			.tx_buf = &priv->data[1],
> >> +			.rx_buf = &priv->data[1],
> >> +			.len = 4,
> >> +		},
> >> +	};  
> > 
> > Pity we don't have a spi_write_the_read_with_cs or
> > something like that.  I wonder how common this structure is?  
> 
> This is common with this part and the ads8688 and another spi part I am working on for CAN.
> 
> I just don't know if there are any parts that hold CS for more then one data xfer.

There definitely are though I couldn't name one right now.
Some devices use the CS to reset a simple state machine.  If you drop
it then the state machine resets, even mid read.

...
> 
> >   
> >> +
> >> +static const struct iio_info ads124s_info = {
> >> +	.read_raw = &ads124s_read_raw,
> >> +};
> >> +
> >> +static irqreturn_t ads124s_trigger_handler(int irq, void *p)
> >> +{
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct ads124s_private *priv = iio_priv(indio_dev);
> >> +	u16 buffer[ADS124S0X_MAX_CHANNELS];  
> > 
> > There is an unfortunate oddity in how push_to_buffers_with_timestamp
> > works in that it builds the data for the kfifo inline in the buffer
> > provided.  Thus that buffer has to have room for the channels and
> > the 64 bit timestamp.  
> 
> Hmmm.  This is like the 8688.  I am starting to think the 8688 needs to be updated.

Yes that looks wrong.  Thanks!

> 
> >   
> >> +	int i, j = 0;
> >> +	int ret;
> >> +
> >> +	for (i = 0; i < indio_dev->masklength; i++) {
> >> +		if (!test_bit(i, indio_dev->active_scan_mask))  
> > 
> > for_each_bit_set   
> 
> Same as above.  But I can change it.
> 
> >   
> >> +			continue;
> >> +
> >> +		ret = ads124s_write_reg(indio_dev, ADS124S0X_INPUT_MUX, i);  
> > 
> > Does this need to be rewritten if the channel is already correct?  
> 
> This is an iterative loop so the channel should be different each time 0 - 12

doh.  I'm not sure how I misread that.  However, not true for the raw
read above I think?
   
...
> > 
> > You can move to fully managed if you use devm_add_action_or_reset
> > to turn the regulator off.
> >   
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct spi_device_id ads124s_id[] = {
> >> +	{ "ads124s08", ADS124S08_ID },
> >> +	{ "ads124s06", ADS124S06_ID },
> >> +	{ }
> >> +};
> >> +MODULE_DEVICE_TABLE(spi, ads124s_id);
> >> +
> >> +static const struct of_device_id ads124s_of_table[] = {
> >> +	{ .compatible = "ti,ads124s08" },
> >> +	{ .compatible = "ti,ads124s06" },  
> > 
> > It's trivial but numerical order preferred as it makes it
> > obvious where to add new devices later.  
> 
> Actually I based these off the ID read from the device.
> The S08 ID is 0x0 and the S06 is 0x1.  Bit I can reorder this since it just the compatible.

Given that reason for the order is hidden by the defines
I'd reorder to avoid someone else coming and tidying it up later!

> 
> Dan
...

Jonathan