[PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jan Kiszka]

This is an upstream port of an IIO driver for the TI ADC108S102 and
ADC128S102. The former can be found on the Intel Galileo Gen2 and the
Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
included.

Due to the lack of regulators under ACPI, we need a special device
property to define the voltage provide to the VA pin of the ADC
("va-millivolt"). For DT usage, the regulator "vref-supply" is
requested. Note that DT usage has not been tested.

Original author: Bogdan Pricop <bogdan.pricop@emutex.com>
Ported from Intel Galileo Gen2 BSP to Intel Yocto kernel:
Todor Minchev <todor@minchev.co.uk>.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v3:
 - Reworked reference voltage handling, splitting up the different ACPI
   case from DT usage. This also means that the "va-millivolt"
   (formerly and incorrectly called "ext-vin-microvolt") becomes
   ACPI-only
 - Add DT binding (straightforward, no device specifics, but untested)
 - Address smaller remarks by Andy

 .../devicetree/bindings/iio/adc/ti-adc108s102.txt  |  18 ++
 drivers/iio/adc/Kconfig                            |  12 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/iio/adc/ti-adc108s102.c                    | 338 +++++++++++++++++++++
 4 files changed, 369 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt
 create mode 100644 drivers/iio/adc/ti-adc108s102.c

diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt
new file mode 100644
index 000000000000..bbbbb4a9f58f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt
@@ -0,0 +1,18 @@
+* Texas Instruments' ADC108S102 and ADC128S102 ADC chip
+
+Required properties:
+ - compatible: Should be "ti,adc108s102"
+ - reg: spi chip select number for the device
+ - vref-supply: The regulator supply for ADC reference voltage
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+		Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+adc@0 {
+	compatible = "ti,adc108s102";
+	reg = <0>;
+	vref-supply = <&vdd_supply>;
+	spi-max-frequency = <1000000>;
+};
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dedae7adbce9..8b7dae5604f2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -572,6 +572,18 @@ config TI_ADC12138
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc12138.
 
+config TI_ADC108S102
+	tristate "Texas Instruments ADC108S102 and ADC128S102 driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Texas Instruments ADC108S102 and
+	  ADC128S102 ADC.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called ti-adc108s102.
+
 config TI_ADC128S052
 	tristate "Texas Instruments ADC128S052/ADC122S021/ADC124S021"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d0012620cd1c..e6c20fe3d4eb 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_STM32_ADC) += stm32-adc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
+obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
new file mode 100644
index 000000000000..88dc32dd45a9
--- /dev/null
+++ b/drivers/iio/adc/ti-adc108s102.c
@@ -0,0 +1,338 @@
+/*
+ * TI ADC108S102 SPI ADC driver
+ *
+ * Copyright (c) 2013-2015 Intel Corporation.
+ * Copyright (c) 2017 Siemens AG
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * This IIO device driver is designed to work with the following
+ * analog to digital converters from Texas Instruments:
+ *  ADC108S102
+ *  ADC128S102
+ * The communication with ADC chip is via the SPI bus (mode 3).
+ */
+
+#include <linux/acpi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/types.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+/*
+ * Defining the ADC resolution being 12 bits, we can use the same driver for
+ * both ADC108S102 (10 bits resolution) and ADC128S102 (12 bits resolution)
+ * chips. The ADC108S102 effectively returns a 12-bit result with the 2
+ * least-significant bits unset.
+ */
+#define ADC108S102_BITS		12
+#define ADC108S102_MAX_CHANNELS	8
+
+/*
+ * 16-bit SPI command format:
+ *   [15:14] Ignored
+ *   [13:11] 3-bit channel address
+ *   [10:0]  Ignored
+ */
+#define ADC108S102_CMD(ch)		((u16)(ch) << 11)
+
+/*
+ * 16-bit SPI response format:
+ *   [15:12] Zeros
+ *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
+ */
+#define ADC108S102_RES_DATA(res)	((u16)res & GENMASK(11, 0))
+
+struct adc108s102_state {
+	struct spi_device		*spi;
+	struct regulator		*reg;
+	u32				va_millivolt;
+	/* SPI transfer used by triggered buffer handler*/
+	struct spi_transfer		ring_xfer;
+	/* SPI transfer used by direct scan */
+	struct spi_transfer		scan_single_xfer;
+	/* SPI message used by ring_xfer SPI transfer */
+	struct spi_message		ring_msg;
+	/* SPI message used by scan_single_xfer SPI transfer */
+	struct spi_message		scan_single_msg;
+
+	/*
+	 * SPI message buffers:
+	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
+	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
+	 *
+	 *  tx_buf: 8 channel read commands, plus 1 dummy command
+	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-bit timestamp
+	 */
+	__be16				rx_buf[13] ____cacheline_aligned;
+	__be16				tx_buf[9] ____cacheline_aligned;
+};
+
+#define ADC108S102_V_CHAN(index)					\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = index,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			BIT(IIO_CHAN_INFO_SCALE),			\
+		.address = index,					\
+		.scan_index = index,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = ADC108S102_BITS,			\
+			.storagebits = 16,				\
+			.endianness = IIO_BE,				\
+		},							\
+	}
+
+static const struct iio_chan_spec adc108s102_channels[] = {
+	ADC108S102_V_CHAN(0),
+	ADC108S102_V_CHAN(1),
+	ADC108S102_V_CHAN(2),
+	ADC108S102_V_CHAN(3),
+	ADC108S102_V_CHAN(4),
+	ADC108S102_V_CHAN(5),
+	ADC108S102_V_CHAN(6),
+	ADC108S102_V_CHAN(7),
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+};
+
+static int adc108s102_update_scan_mode(struct iio_dev *indio_dev,
+		unsigned long const *active_scan_mask)
+{
+	struct adc108s102_state *st = iio_priv(indio_dev);
+	unsigned int bit, cmds;
+
+	/*
+	 * Fill in the first x shorts of tx_buf with the number of channels
+	 * enabled for sampling by the triggered buffer.
+	 */
+	cmds = 0;
+	for_each_set_bit(bit, active_scan_mask, ADC108S102_MAX_CHANNELS)
+		st->tx_buf[cmds++] = cpu_to_be16(ADC108S102_CMD(bit));
+
+	/* One dummy command added, to clock in the last response */
+	st->tx_buf[cmds++] = 0x00;
+
+	/* build SPI ring message */
+	st->ring_xfer.tx_buf = &st->tx_buf[0];
+	st->ring_xfer.rx_buf = &st->rx_buf[0];
+	st->ring_xfer.len = cmds * sizeof(st->tx_buf[0]);
+
+	spi_message_init_with_transfers(&st->ring_msg, &st->ring_xfer, 1);
+
+	return 0;
+}
+
+static irqreturn_t adc108s102_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adc108s102_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = spi_sync(st->spi, &st->ring_msg);
+	if (ret < 0)
+		goto out_notify;
+
+	/* Skip the dummy response in the first slot */
+	iio_push_to_buffers_with_timestamp(indio_dev,
+					   (u8 *)&st->rx_buf[1],
+					   iio_get_time_ns(indio_dev));
+
+out_notify:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int adc108s102_scan_direct(struct adc108s102_state *st, unsigned int ch)
+{
+	int ret;
+
+	st->tx_buf[0] = cpu_to_be16(ADC108S102_CMD(ch));
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		return ret;
+
+	/* Skip the dummy response in the first slot */
+	return be16_to_cpu(st->rx_buf[1]);
+}
+
+static int adc108s102_read_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int *val, int *val2, long m)
+{
+	struct adc108s102_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = adc108s102_scan_direct(st, chan->address);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		if (ret < 0)
+			return ret;
+
+		*val = ADC108S102_RES_DATA(ret);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_VOLTAGE) {
+			if (st->reg)
+				*val = regulator_get_voltage(st->reg) / 1000;
+			else
+				*val = st->va_millivolt;
+
+			*val2 = chan->scan_type.realbits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		} else {
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info adc108s102_info = {
+	.read_raw		= &adc108s102_read_raw,
+	.update_scan_mode	= &adc108s102_update_scan_mode,
+	.driver_module		= THIS_MODULE,
+};
+
+static int adc108s102_probe(struct spi_device *spi)
+{
+	struct adc108s102_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	if (ACPI_COMPANION(&spi->dev)) {
+		st->reg = NULL;
+		ret = device_property_read_u32(&spi->dev, "va-millivolt",
+					    &st->va_millivolt);
+		if (ret < 0) {
+			dev_err(&spi->dev,
+				"Missing va-millivolt device property\n");
+			return -ENODEV;
+		}
+	} else {
+		st->reg = devm_regulator_get(&spi->dev, "vref");
+		if (IS_ERR(st->reg))
+			return PTR_ERR(st->reg);
+
+		ret = regulator_enable(st->reg);
+		if (ret < 0) {
+			dev_err(&spi->dev, "Cannot enable vref regulator\n");
+			return ret;
+		}
+	}
+
+	spi_set_drvdata(spi, indio_dev);
+	st->spi = spi;
+
+	indio_dev->name = spi->modalias;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = adc108s102_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adc108s102_channels);
+	indio_dev->info = &adc108s102_info;
+
+	/* Setup default message */
+	st->scan_single_xfer.tx_buf = st->tx_buf;
+	st->scan_single_xfer.rx_buf = st->rx_buf;
+	st->scan_single_xfer.len = 2 * sizeof(st->tx_buf[0]);
+
+	spi_message_init_with_transfers(&st->scan_single_msg,
+					&st->scan_single_xfer, 1);
+
+	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
+			&adc108s102_trigger_handler, NULL);
+	if (ret)
+		goto error_disable_reg;
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register IIO device\n");
+		goto error_disable_reg;
+	}
+	return 0;
+
+error_disable_reg:
+	if (!IS_ERR(st->reg))
+		regulator_disable(st->reg);
+
+	return ret;
+}
+
+static int adc108s102_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct adc108s102_state *st = iio_priv(indio_dev);
+
+	if (!IS_ERR(st->reg))
+		regulator_disable(st->reg);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id adc108s102_of_match[] = {
+	{ .compatible = "ti,adc108s102" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, adc108s102_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id adc108s102_acpi_ids[] = {
+	{ "INT3495", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, adc108s102_acpi_ids);
+#endif
+
+static const struct spi_device_id adc108s102_id[] = {
+	{ "adc108s102", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, adc108s102_id);
+
+static struct spi_driver adc108s102_driver = {
+	.driver = {
+		.name   = "adc108s102",
+		.of_match_table = of_match_ptr(adc108s102_of_match),
+		.acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
+	},
+	.probe		= adc108s102_probe,
+	.remove		= adc108s102_remove,
+	.id_table	= adc108s102_id,
+};
+module_spi_driver(adc108s102_driver);
+
+MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
+MODULE_DESCRIPTION("Texas Instruments ADC108S102 and ADC128S102 driver");
+MODULE_LICENSE("GPL v2");

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Mika Westerberg]

On Fri, May 05, 2017 at 08:31:32AM +0200, Jan Kiszka wrote:
> +static int adc108s102_probe(struct spi_device *spi)
> +{
> +	struct adc108s102_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	if (ACPI_COMPANION(&spi->dev)) {
> +		st->reg = NULL;
> +		ret = device_property_read_u32(&spi->dev, "va-millivolt",
> +					    &st->va_millivolt);

Please try to avoid things like this for now. You can just hard code the
voltage now and we can think how to solve this in ACPI if there will be
an actual user needing anything else than the voltage you are using on
your board.

> +		if (ret < 0) {
> +			dev_err(&spi->dev,
> +				"Missing va-millivolt device property\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		st->reg = devm_regulator_get(&spi->dev, "vref");
> +		if (IS_ERR(st->reg))
> +			return PTR_ERR(st->reg);

This should be an optional regulator and in case of ACPI you just don't
have it.

> +
> +		ret = regulator_enable(st->reg);
> +		if (ret < 0) {
> +			dev_err(&spi->dev, "Cannot enable vref regulator\n");
> +			return ret;
> +		}
> +	}

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Andy Shevchenko]

On Fri, 2017-05-05 at 08:31 +0200, Jan Kiszka wrote:
> This is an upstream port of an IIO driver for the TI ADC108S102 and
> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
> included.
> 
> Due to the lack of regulators under ACPI, we need a special device
> property to define the voltage provide to the VA pin of the ADC
> ("va-millivolt"). For DT usage, the regulator "vref-supply" is
> requested. Note that DT usage has not been tested.

+1 to what Mika commented on this and just some additional information.

Other than that looks pretty good.

> Changes in v3:
>  - Reworked reference voltage handling, splitting up the different
> ACPI
>    case from DT usage. This also means that the "va-millivolt"
>    (formerly and incorrectly called "ext-vin-microvolt") becomes
>    ACPI-only

Just to be clean, there is *no* such thing as *XYZ-only* device
properties. The idea behind them is to provide resource provider
agnostic API to read properties.

> +			if (st->reg)
> +				*val = regulator_get_voltage(st->reg) 
> / 1000;
> +			else
> +				*val = st->va_millivolt;
> +

Another way is to not just hard code the value, but create a fixed
voltage regulator out of it. In this case you will have one way to get
its value.

> +		st->reg = devm_regulator_get(&spi->dev, "vref");

It should be _optional like I mentioned in one of previous review.

> +	if (!IS_ERR(st->reg))

This is redundant

> +		regulator_disable(st->reg);

> +	if (!IS_ERR(st->reg))

Ditto.

> +		regulator_disable(st->reg);


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jan Kiszka]

On 2017-05-05 11:40, Mika Westerberg wrote:
> On Fri, May 05, 2017 at 08:31:32AM +0200, Jan Kiszka wrote:
>> +static int adc108s102_probe(struct spi_device *spi)
>> +{
>> +	struct adc108s102_state *st;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	if (ACPI_COMPANION(&spi->dev)) {
>> +		st->reg = NULL;
>> +		ret = device_property_read_u32(&spi->dev, "va-millivolt",
>> +					    &st->va_millivolt);
> 
> Please try to avoid things like this for now. You can just hard code the
> voltage now and we can think how to solve this in ACPI if there will be
> an actual user needing anything else than the voltage you are using on
> your board.

OK, so I will set a static 5V in case of ACPI. Fine as well.

> 
>> +		if (ret < 0) {
>> +			dev_err(&spi->dev,
>> +				"Missing va-millivolt device property\n");
>> +			return -ENODEV;
>> +		}
>> +	} else {
>> +		st->reg = devm_regulator_get(&spi->dev, "vref");
>> +		if (IS_ERR(st->reg))
>> +			return PTR_ERR(st->reg);
> 
> This should be an optional regulator and in case of ACPI you just don't
> have it.

It's mandatory for this DT case, so I will not change that.

Thanks,
Jan

> 
>> +
>> +		ret = regulator_enable(st->reg);
>> +		if (ret < 0) {
>> +			dev_err(&spi->dev, "Cannot enable vref regulator\n");
>> +			return ret;
>> +		}
>> +	}

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jan Kiszka]

On 2017-05-05 11:54, Andy Shevchenko wrote:
> On Fri, 2017-05-05 at 08:31 +0200, Jan Kiszka wrote:
>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>> included.
>>
>> Due to the lack of regulators under ACPI, we need a special device
>> property to define the voltage provide to the VA pin of the ADC
>> ("va-millivolt"). For DT usage, the regulator "vref-supply" is
>> requested. Note that DT usage has not been tested.
> 
> +1 to what Mika commented on this and just some additional information.
> 
> Other than that looks pretty good.
> 
>> Changes in v3:
>>  - Reworked reference voltage handling, splitting up the different
>> ACPI
>>    case from DT usage. This also means that the "va-millivolt"
>>    (formerly and incorrectly called "ext-vin-microvolt") becomes
>>    ACPI-only
> 
> Just to be clean, there is *no* such thing as *XYZ-only* device
> properties. The idea behind them is to provide resource provider
> agnostic API to read properties.

Well, as along as ACPI does its own thing /wrt regulators, we have that
problem. But let's wait until someone designs an ACPI board with that
chip and a different reference voltage.

> 
>> +			if (st->reg)
>> +				*val = regulator_get_voltage(st->reg) 
>> / 1000;
>> +			else
>> +				*val = st->va_millivolt;
>> +
> 
> Another way is to not just hard code the value, but create a fixed
> voltage regulator out of it. In this case you will have one way to get
> its value.

That's a good idea.

> 
>> +		st->reg = devm_regulator_get(&spi->dev, "vref");
> 
> It should be _optional like I mentioned in one of previous review.
> 
>> +	if (!IS_ERR(st->reg))
> 
> This is redundant

It's not for the existing code because regulator_disable doesn't test
this for us. It will be, though, when I switch back to making it
mandatory for everyone.

Jan

> 
>> +		regulator_disable(st->reg);
> 
>> +	if (!IS_ERR(st->reg))
> 
> Ditto.
> 
>> +		regulator_disable(st->reg);
> 
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Mika Westerberg]

On Fri, May 05, 2017 at 12:23:26PM +0200, Jan Kiszka wrote:
> >> +	} else {
> >> +		st->reg = devm_regulator_get(&spi->dev, "vref");
> >> +		if (IS_ERR(st->reg))
> >> +			return PTR_ERR(st->reg);
> > 
> > This should be an optional regulator and in case of ACPI you just don't
> > have it.
> 
> It's mandatory for this DT case, so I will not change that.

Well, it would be better if you don't need to deviate in the driver like
this. And it clearly is optional because in case of ACPI you don't need
it.

Not my call, though.

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jonathan Cameron]

On 05/05/17 11:40, Mika Westerberg wrote:
> On Fri, May 05, 2017 at 12:23:26PM +0200, Jan Kiszka wrote:
>>>> +	} else {
>>>> +		st->reg = devm_regulator_get(&spi->dev, "vref");
>>>> +		if (IS_ERR(st->reg))
>>>> +			return PTR_ERR(st->reg);
>>>
>>> This should be an optional regulator and in case of ACPI you just don't
>>> have it.
>>
>> It's mandatory for this DT case, so I will not change that.
> 
> Well, it would be better if you don't need to deviate in the driver like
> this. And it clearly is optional because in case of ACPI you don't need
> it.
> 
> Not my call, though.
> 
I'd keep it as mandatory. The ACPI case is more of a workaround of
a lack of information than anything.  The only way you could make it optional
would be to simply not provide the scale if it wasn't there.
(to do that have two channel sets and switch between them depending on its
presence).

We'd do it as optional if there was an existing binding defined where it wasn't
specified (not much use having the fake regulator show up but be unable to
answer the what voltage question ;)

Jonathan

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jonathan Cameron]

On 05/05/17 11:39, Jan Kiszka wrote:
> On 2017-05-05 11:54, Andy Shevchenko wrote:
>> On Fri, 2017-05-05 at 08:31 +0200, Jan Kiszka wrote:
>>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>>> included.
>>>
>>> Due to the lack of regulators under ACPI, we need a special device
>>> property to define the voltage provide to the VA pin of the ADC
>>> ("va-millivolt"). For DT usage, the regulator "vref-supply" is
>>> requested. Note that DT usage has not been tested.
>>
>> +1 to what Mika commented on this and just some additional information.
>>
>> Other than that looks pretty good.
>>
>>> Changes in v3:
>>>  - Reworked reference voltage handling, splitting up the different
>>> ACPI
>>>    case from DT usage. This also means that the "va-millivolt"
>>>    (formerly and incorrectly called "ext-vin-microvolt") becomes
>>>    ACPI-only
>>
>> Just to be clean, there is *no* such thing as *XYZ-only* device
>> properties. The idea behind them is to provide resource provider
>> agnostic API to read properties.
> 
> Well, as along as ACPI does its own thing /wrt regulators, we have that
> problem. But let's wait until someone designs an ACPI board with that
> chip and a different reference voltage.
> 
>>
>>> +			if (st->reg)
>>> +				*val = regulator_get_voltage(st->reg) 
>>> / 1000;
>>> +			else
>>> +				*val = st->va_millivolt;
>>> +
>>
>> Another way is to not just hard code the value, but create a fixed
>> voltage regulator out of it. In this case you will have one way to get
>> its value.
> 
> That's a good idea.
Agreed. Make sure to cc Mark Brown though as I'll need an ack from him
to have a fixed reg hiding in here.
> 
>>
>>> +		st->reg = devm_regulator_get(&spi->dev, "vref");
>>
>> It should be _optional like I mentioned in one of previous review.
>>
>>> +	if (!IS_ERR(st->reg))
>>
>> This is redundant
> 
> It's not for the existing code because regulator_disable doesn't test
> this for us. It will be, though, when I switch back to making it
> mandatory for everyone.
> 
> Jan
> 
>>
>>> +		regulator_disable(st->reg);
>>
>>> +	if (!IS_ERR(st->reg))
>>
>> Ditto.
>>
>>> +		regulator_disable(st->reg);
>>
>>
> 

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jonathan Cameron]

On 05/05/17 07:31, Jan Kiszka wrote:
> This is an upstream port of an IIO driver for the TI ADC108S102 and
> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
> included.
> 
> Due to the lack of regulators under ACPI, we need a special device
> property to define the voltage provide to the VA pin of the ADC
> ("va-millivolt"). For DT usage, the regulator "vref-supply" is
> requested. Note that DT usage has not been tested.
> 
> Original author: Bogdan Pricop <bogdan.pricop@emutex.com>
> Ported from Intel Galileo Gen2 BSP to Intel Yocto kernel:
> Todor Minchev <todor@minchev.co.uk>.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
A few comments inline, but nothing terribly significant.
> ---
> 
> Changes in v3:
>  - Reworked reference voltage handling, splitting up the different ACPI
>    case from DT usage. This also means that the "va-millivolt"
>    (formerly and incorrectly called "ext-vin-microvolt") becomes
>    ACPI-only
>  - Add DT binding (straightforward, no device specifics, but untested)
>  - Address smaller remarks by Andy
> 
>  .../devicetree/bindings/iio/adc/ti-adc108s102.txt  |  18 ++
>  drivers/iio/adc/Kconfig                            |  12 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ti-adc108s102.c                    | 338 +++++++++++++++++++++
>  4 files changed, 369 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt
>  create mode 100644 drivers/iio/adc/ti-adc108s102.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt b/Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt
> new file mode 100644
> index 000000000000..bbbbb4a9f58f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ti-adc108s102.txt
> @@ -0,0 +1,18 @@
> +* Texas Instruments' ADC108S102 and ADC128S102 ADC chip
> +
> +Required properties:
> + - compatible: Should be "ti,adc108s102"
> + - reg: spi chip select number for the device
> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> +		Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +adc@0 {
> +	compatible = "ti,adc108s102";
> +	reg = <0>;
> +	vref-supply = <&vdd_supply>;
> +	spi-max-frequency = <1000000>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7adbce9..8b7dae5604f2 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -572,6 +572,18 @@ config TI_ADC12138
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc12138.
>  
> +config TI_ADC108S102
> +	tristate "Texas Instruments ADC108S102 and ADC128S102 driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Texas Instruments ADC108S102 and
> +	  ADC128S102 ADC.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called ti-adc108s102.
> +
>  config TI_ADC128S052
>  	tristate "Texas Instruments ADC128S052/ADC122S021/ADC124S021"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d0012620cd1c..e6c20fe3d4eb 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -52,6 +52,7 @@ obj-$(CONFIG_STM32_ADC) += stm32-adc.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
> +obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>  obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
> new file mode 100644
> index 000000000000..88dc32dd45a9
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc108s102.c
> @@ -0,0 +1,338 @@
> +/*
> + * TI ADC108S102 SPI ADC driver
> + *
> + * Copyright (c) 2013-2015 Intel Corporation.
> + * Copyright (c) 2017 Siemens AG
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.
> + *
> + * This IIO device driver is designed to work with the following
> + * analog to digital converters from Texas Instruments:
> + *  ADC108S102
> + *  ADC128S102
> + * The communication with ADC chip is via the SPI bus (mode 3).
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/types.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +/*
> + * Defining the ADC resolution being 12 bits, we can use the same driver for
> + * both ADC108S102 (10 bits resolution) and ADC128S102 (12 bits resolution)
> + * chips. The ADC108S102 effectively returns a 12-bit result with the 2
> + * least-significant bits unset.
> + */
> +#define ADC108S102_BITS		12
> +#define ADC108S102_MAX_CHANNELS	8
> +
> +/*
> + * 16-bit SPI command format:
> + *   [15:14] Ignored
> + *   [13:11] 3-bit channel address
> + *   [10:0]  Ignored
> + */
> +#define ADC108S102_CMD(ch)		((u16)(ch) << 11)
> +
> +/*
> + * 16-bit SPI response format:
> + *   [15:12] Zeros
> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
> + */
> +#define ADC108S102_RES_DATA(res)	((u16)res & GENMASK(11, 0))
> +
> +struct adc108s102_state {
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	u32				va_millivolt;
> +	/* SPI transfer used by triggered buffer handler*/
> +	struct spi_transfer		ring_xfer;
> +	/* SPI transfer used by direct scan */
> +	struct spi_transfer		scan_single_xfer;
> +	/* SPI message used by ring_xfer SPI transfer */
> +	struct spi_message		ring_msg;
> +	/* SPI message used by scan_single_xfer SPI transfer */
> +	struct spi_message		scan_single_msg;
> +
> +	/*
> +	 * SPI message buffers:
> +	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
> +	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
> +	 *
> +	 *  tx_buf: 8 channel read commands, plus 1 dummy command
> +	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-bit timestamp
> +	 */
> +	__be16				rx_buf[13] ____cacheline_aligned;
> +	__be16				tx_buf[9] ____cacheline_aligned;
I would have thought the SPI dma wouldn't take itself out so you should be
good with just the one cacheline_aligned?  Maybe I'm missing something.
> +};
> +
> +#define ADC108S102_V_CHAN(index)					\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = index,					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
> +			BIT(IIO_CHAN_INFO_SCALE),			\
> +		.address = index,					\
> +		.scan_index = index,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = ADC108S102_BITS,			\
> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec adc108s102_channels[] = {
> +	ADC108S102_V_CHAN(0),
> +	ADC108S102_V_CHAN(1),
> +	ADC108S102_V_CHAN(2),
> +	ADC108S102_V_CHAN(3),
> +	ADC108S102_V_CHAN(4),
> +	ADC108S102_V_CHAN(5),
> +	ADC108S102_V_CHAN(6),
> +	ADC108S102_V_CHAN(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static int adc108s102_update_scan_mode(struct iio_dev *indio_dev,
> +		unsigned long const *active_scan_mask)
> +{
> +	struct adc108s102_state *st = iio_priv(indio_dev);
> +	unsigned int bit, cmds;
> +
> +	/*
> +	 * Fill in the first x shorts of tx_buf with the number of channels
> +	 * enabled for sampling by the triggered buffer.
> +	 */
> +	cmds = 0;
> +	for_each_set_bit(bit, active_scan_mask, ADC108S102_MAX_CHANNELS)
> +		st->tx_buf[cmds++] = cpu_to_be16(ADC108S102_CMD(bit));
> +
> +	/* One dummy command added, to clock in the last response */
> +	st->tx_buf[cmds++] = 0x00;
> +
> +	/* build SPI ring message */
> +	st->ring_xfer.tx_buf = &st->tx_buf[0];
> +	st->ring_xfer.rx_buf = &st->rx_buf[0];
> +	st->ring_xfer.len = cmds * sizeof(st->tx_buf[0]);
> +
> +	spi_message_init_with_transfers(&st->ring_msg, &st->ring_xfer, 1);
> +
Nice ;)
> +	return 0;
> +}
> +
> +static irqreturn_t adc108s102_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adc108s102_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = spi_sync(st->spi, &st->ring_msg);
> +	if (ret < 0)
> +		goto out_notify;
> +
> +	/* Skip the dummy response in the first slot */
> +	iio_push_to_buffers_with_timestamp(indio_dev,
> +					   (u8 *)&st->rx_buf[1],
> +					   iio_get_time_ns(indio_dev));
> +
> +out_notify:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adc108s102_scan_direct(struct adc108s102_state *st, unsigned int ch)
> +{
> +	int ret;
> +
> +	st->tx_buf[0] = cpu_to_be16(ADC108S102_CMD(ch));
> +	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	if (ret)
> +		return ret;
> +
> +	/* Skip the dummy response in the first slot */
> +	return be16_to_cpu(st->rx_buf[1]);
> +}
> +
> +static int adc108s102_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       int *val, int *val2, long m)
> +{
> +	struct adc108s102_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = adc108s102_scan_direct(st, chan->address);
> +
> +		iio_device_release_direct_mode(indio_dev);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ADC108S102_RES_DATA(ret);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_VOLTAGE) {
> +			if (st->reg)
> +				*val = regulator_get_voltage(st->reg) / 1000;
> +			else
> +				*val = st->va_millivolt;
> +
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		} else {
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info adc108s102_info = {
> +	.read_raw		= &adc108s102_read_raw,
> +	.update_scan_mode	= &adc108s102_update_scan_mode,
> +	.driver_module		= THIS_MODULE,
> +};
> +
> +static int adc108s102_probe(struct spi_device *spi)
> +{
> +	struct adc108s102_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	if (ACPI_COMPANION(&spi->dev)) {
> +		st->reg = NULL;
> +		ret = device_property_read_u32(&spi->dev, "va-millivolt",
> +					    &st->va_millivolt);
> +		if (ret < 0) {
> +			dev_err(&spi->dev,
> +				"Missing va-millivolt device property\n");
> +			return -ENODEV;
> +		}
> +	} else {
> +		st->reg = devm_regulator_get(&spi->dev, "vref");
> +		if (IS_ERR(st->reg))
> +			return PTR_ERR(st->reg);
> +
> +		ret = regulator_enable(st->reg);
Would be nice to do something smarter with the regs to not insist they
are on if no one cares, (i.e. runtime pm stuff) but that can follow later
if you feel like doing it at all!
> +		if (ret < 0) {
> +			dev_err(&spi->dev, "Cannot enable vref regulator\n");
> +			return ret;
> +		}
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	st->spi = spi;
> +
> +	indio_dev->name = spi->modalias;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = adc108s102_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adc108s102_channels);
> +	indio_dev->info = &adc108s102_info;
> +
> +	/* Setup default message */
> +	st->scan_single_xfer.tx_buf = st->tx_buf;
> +	st->scan_single_xfer.rx_buf = st->rx_buf;
> +	st->scan_single_xfer.len = 2 * sizeof(st->tx_buf[0]);
> +
> +	spi_message_init_with_transfers(&st->scan_single_msg,
> +					&st->scan_single_xfer, 1);
> +
> +	ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
> +			&adc108s102_trigger_handler, NULL);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to register IIO device\n");
> +		goto error_disable_reg;
> +	}
> +	return 0;
> +
> +error_disable_reg:
> +	if (!IS_ERR(st->reg))
> +		regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +
> +static int adc108s102_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct adc108s102_state *st = iio_priv(indio_dev);
> +
> +	if (!IS_ERR(st->reg))
> +		regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id adc108s102_of_match[] = {
> +	{ .compatible = "ti,adc108s102" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, adc108s102_of_match);
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id adc108s102_acpi_ids[] = {
> +	{ "INT3495", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc108s102_acpi_ids);
> +#endif
> +
> +static const struct spi_device_id adc108s102_id[] = {
> +	{ "adc108s102", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adc108s102_id);
> +
> +static struct spi_driver adc108s102_driver = {
> +	.driver = {
> +		.name   = "adc108s102",
> +		.of_match_table = of_match_ptr(adc108s102_of_match),
> +		.acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
> +	},
> +	.probe		= adc108s102_probe,
> +	.remove		= adc108s102_remove,
> +	.id_table	= adc108s102_id,
> +};
> +module_spi_driver(adc108s102_driver);
> +
> +MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADC108S102 and ADC128S102 driver");
> +MODULE_LICENSE("GPL v2");
> --
> 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: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Andy Shevchenko]

On Fri, 2017-05-05 at 19:55 +0100, Jonathan Cameron wrote:
> On 05/05/17 07:31, Jan Kiszka wrote:

> > +
> > +	/*
> > +	 * SPI message buffers:
> > +	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
> > +	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
> > +	 *
> > +	 *  tx_buf: 8 channel read commands, plus 1 dummy command
> > +	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-
> > bit timestamp
> > +	 */
> > +	__be16				rx_buf[13]
> > ____cacheline_aligned;
> > +	__be16				tx_buf[9]
> > ____cacheline_aligned;
> 
> I would have thought the SPI dma wouldn't take itself out so you
> should be
> good with just the one cacheline_aligned?  Maybe I'm missing
> something.

It was my idea for sake of consistency. Just to explicitly show that
buffers a cache aligned.

If you insist to remove one, it's your call at the end ;-)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jan Kiszka]

On 2017-05-05 20:52, Jonathan Cameron wrote:
> On 05/05/17 11:39, Jan Kiszka wrote:
>> On 2017-05-05 11:54, Andy Shevchenko wrote:
>>> On Fri, 2017-05-05 at 08:31 +0200, Jan Kiszka wrote:
>>>> This is an upstream port of an IIO driver for the TI ADC108S102 and
>>>> ADC128S102. The former can be found on the Intel Galileo Gen2 and the
>>>> Siemens SIMATIC IOT2000. For those boards, ACPI-based enumeration is
>>>> included.
>>>>
>>>> Due to the lack of regulators under ACPI, we need a special device
>>>> property to define the voltage provide to the VA pin of the ADC
>>>> ("va-millivolt"). For DT usage, the regulator "vref-supply" is
>>>> requested. Note that DT usage has not been tested.
>>>
>>> +1 to what Mika commented on this and just some additional information.
>>>
>>> Other than that looks pretty good.
>>>
>>>> Changes in v3:
>>>>  - Reworked reference voltage handling, splitting up the different
>>>> ACPI
>>>>    case from DT usage. This also means that the "va-millivolt"
>>>>    (formerly and incorrectly called "ext-vin-microvolt") becomes
>>>>    ACPI-only
>>>
>>> Just to be clean, there is *no* such thing as *XYZ-only* device
>>> properties. The idea behind them is to provide resource provider
>>> agnostic API to read properties.
>>
>> Well, as along as ACPI does its own thing /wrt regulators, we have that
>> problem. But let's wait until someone designs an ACPI board with that
>> chip and a different reference voltage.
>>
>>>
>>>> +			if (st->reg)
>>>> +				*val = regulator_get_voltage(st->reg) 
>>>> / 1000;
>>>> +			else
>>>> +				*val = st->va_millivolt;
>>>> +
>>>
>>> Another way is to not just hard code the value, but create a fixed
>>> voltage regulator out of it. In this case you will have one way to get
>>> its value.
>>
>> That's a good idea.
> Agreed. Make sure to cc Mark Brown though as I'll need an ack from him
> to have a fixed reg hiding in here.

After diving deeper, it not longer appears to be a good idea:

- pulls in a non-obvious requirement for CONFIG_REGULATOR on platforms
  that otherwise do not need it

- requires complex life-cycle management so that the fixed regulator is
  instantiated on the first device creation and removed with the last
  one

We better go with the static value assignment.

I'll move that regulator_get_voltage into the probing function which
will simplify things further (va_millivolt will carry the value for both
cases).

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Andy Shevchenko]

On Fri, 2017-05-05 at 22:09 +0200, Jan Kiszka wrote:
> On 2017-05-05 20:52, Jonathan Cameron wrote:
> > On 05/05/17 11:39, Jan Kiszka wrote:
> > > On 2017-05-05 11:54, Andy Shevchenko wrote:
> > > > On Fri, 2017-05-05 at 08:31 +0200, Jan Kiszka wrote:

>>>>> +			if (st->reg)
> > > > > +				*val =
> > > > > regulator_get_voltage(st->reg) 
> > > > > / 1000;
> > > > > +			else
> > > > > +				*val = st->va_millivolt;
> > > > > +
> > > > 
> > > > Another way is to not just hard code the value, but create a
> > > > fixed
> > > > voltage regulator out of it. In this case you will have one way
> > > > to get
> > > > its value.
> > > 
> > > That's a good idea.
> > 
> > Agreed. Make sure to cc Mark Brown though as I'll need an ack from
> > him
> > to have a fixed reg hiding in here.
> 
> After diving deeper, it not longer appears to be a good idea:
> 
> - pulls in a non-obvious requirement for CONFIG_REGULATOR on platforms
>   that otherwise do not need it

Why is it a problem?

> - requires complex life-cycle management so that the fixed regulator
> is
>   instantiated on the first device creation and removed with the last
>   one

Who cares if you register more than one?

> We better go with the static value assignment.
> 
> I'll move that regulator_get_voltage into the probing function which
> will simplify things further (va_millivolt will carry the value for
> both
> cases).

Yes, it would be the way, if system has it's fixed.

But in this case you need to threat regulator as optional if we are
going to enable/disable them for PM.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jonathan Cameron]

On 05/05/17 20:09, Andy Shevchenko wrote:
> On Fri, 2017-05-05 at 19:55 +0100, Jonathan Cameron wrote:
>> On 05/05/17 07:31, Jan Kiszka wrote:
> 
>>> +
>>> +	/*
>>> +	 * SPI message buffers:
>>> +	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
>>> +	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
>>> +	 *
>>> +	 *  tx_buf: 8 channel read commands, plus 1 dummy command
>>> +	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-
>>> bit timestamp
>>> +	 */
>>> +	__be16				rx_buf[13]
>>> ____cacheline_aligned;
>>> +	__be16				tx_buf[9]
>>> ____cacheline_aligned;
>>
>> I would have thought the SPI dma wouldn't take itself out so you
>> should be
>> good with just the one cacheline_aligned?  Maybe I'm missing
>> something.
> 
> It was my idea for sake of consistency. Just to explicitly show that
> buffers a cache aligned.
> 
> If you insist to remove one, it's your call at the end ;-)
> 
As far as I know the only reason the 'need' to be cacheline aligned
is to avoid corruption issues if any other elements sharing the
cacheline (ie.earlier elements in our structure) change whilst the
dma is progress.  Doesn't matter if the second is also so aligned
as we should only have one dma type transfer going on at a time
(plenty of locking in spi to ensure this).

It's not a big point though so doesn't really matter as it is
safe either way!

Jonathan

Re: [PATCH v3] iio: adc: Add support for TI ADC108S102 and ADC128S102

[Jonathan Cameron]

On 05/05/17 21:32, Andy Shevchenko wrote:
> On Fri, 2017-05-05 at 22:09 +0200, Jan Kiszka wrote:
>> On 2017-05-05 20:52, Jonathan Cameron wrote:
>>> On 05/05/17 11:39, Jan Kiszka wrote:
>>>> On 2017-05-05 11:54, Andy Shevchenko wrote:
>>>>> On Fri, 2017-05-05 at 08:31 +0200, Jan Kiszka wrote:
> 
>>>>>> +			if (st->reg)
>>>>>> +				*val =
>>>>>> regulator_get_voltage(st->reg) 
>>>>>> / 1000;
>>>>>> +			else
>>>>>> +				*val = st->va_millivolt;
>>>>>> +
>>>>>
>>>>> Another way is to not just hard code the value, but create a
>>>>> fixed
>>>>> voltage regulator out of it. In this case you will have one way
>>>>> to get
>>>>> its value.
>>>>
>>>> That's a good idea.
>>>
>>> Agreed. Make sure to cc Mark Brown though as I'll need an ack from
>>> him
>>> to have a fixed reg hiding in here.
>>
>> After diving deeper, it not longer appears to be a good idea:
>>
>> - pulls in a non-obvious requirement for CONFIG_REGULATOR on platforms
>>   that otherwise do not need it
> 
> Why is it a problem?
It seems unlikely this is the first ever case of needing proper
regulator support on ACPI platforms.  Mark/Liam, an precedents that you
know of?
> 
>> - requires complex life-cycle management so that the fixed regulator
>> is
>>   instantiated on the first device creation and removed with the last
>>   one
> 
> Who cares if you register more than one?
> 
>> We better go with the static value assignment.
>>
>> I'll move that regulator_get_voltage into the probing function which
>> will simplify things further (va_millivolt will carry the value for
>> both
>> cases).
> 
> Yes, it would be the way, if system has it's fixed.
> 
> But in this case you need to threat regulator as optional if we are
> going to enable/disable them for PM.
>