[PATCH] iio: adc: Add support for TI ADC1x8s102

[PATCH] iio: adc: Add support for TI ADC1x8s102

[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.

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>
---
 drivers/iio/adc/Kconfig                  |  12 +
 drivers/iio/adc/Makefile                 |   1 +
 drivers/iio/adc/ti-adc1x8s102.c          | 408 +++++++++++++++++++++++++++++++
 include/linux/platform_data/adc1x8s102.h |  28 +++
 4 files changed, 449 insertions(+)
 create mode 100644 drivers/iio/adc/ti-adc1x8s102.c
 create mode 100644 include/linux/platform_data/adc1x8s102.h

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dedae7adbce9..edb7254a648c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -582,6 +582,18 @@ config TI_ADC128S052
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc128s052.
 
+config TI_ADC1x8S102
+	tristate "Texas Instruments ADC1x8S102 driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Texas Instruments ADC1x8S102 ADC.
+	  Provides direct access via sysfs.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called ti-adc1x8s102
+
 config TI_ADC161S626
 	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d0012620cd1c..d5d913bc1263 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
+obj-$(CONFIG_TI_ADC1x8S102) += ti-adc1x8s102.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
 obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
diff --git a/drivers/iio/adc/ti-adc1x8s102.c b/drivers/iio/adc/ti-adc1x8s102.c
new file mode 100644
index 000000000000..4f94c371489d
--- /dev/null
+++ b/drivers/iio/adc/ti-adc1x8s102.c
@@ -0,0 +1,408 @@
+/*
+ * TI ADC1x8S102 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 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/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/spi/spi.h>
+
+#include <linux/platform_data/adc1x8s102.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/delay.h>
+#include <linux/acpi.h>
+#include <linux/property.h>
+#include <linux/gpio.h>
+
+#include <linux/spi/pxa2xx_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 ADC1x8S102_BITS		12
+#define ADC1x8S102_MAX_CHANNELS	8
+
+/* 16-bit SPI command format:
+ *   [15:14] Ignored
+ *   [13:11] 3-bit channel address
+ *   [10:0]  Ignored
+ */
+#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))
+
+/*
+ * 16-bit SPI response format:
+ *   [15:12] Zeros
+ *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
+ */
+#define ADC1x8S102_RES_DATA(res)	(res & ((1 << ADC1x8S102_BITS) - 1))
+
+#define ADC1x8S102_GALILEO2_CS	8
+
+struct adc1x8s102_state {
+	struct spi_device		*spi;
+	struct regulator		*reg;
+	u16				ext_vin;
+	/* 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];
+};
+
+#define ADC1X8S102_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 = ADC1x8S102_BITS,			\
+			.storagebits = 16,				\
+			.endianness = IIO_BE,				\
+		},							\
+	}
+
+static const struct iio_chan_spec adc1x8s102_channels[] = {
+	ADC1X8S102_V_CHAN(0),
+	ADC1X8S102_V_CHAN(1),
+	ADC1X8S102_V_CHAN(2),
+	ADC1X8S102_V_CHAN(3),
+	ADC1X8S102_V_CHAN(4),
+	ADC1X8S102_V_CHAN(5),
+	ADC1X8S102_V_CHAN(6),
+	ADC1X8S102_V_CHAN(7),
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+};
+
+static int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
+		unsigned long const *active_scan_mask)
+{
+	struct adc1x8s102_state *st;
+	int i, j;
+
+	st = iio_priv(indio_dev);
+
+	/* Fill in the first x shorts of tx_buf with the number of channels
+	 * enabled for sampling by the triggered buffer
+	 */
+	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
+		if (test_bit(i, active_scan_mask)) {
+			st->tx_buf[j] = cpu_to_be16(ADC1x8S102_CMD(i));
+			j++;
+		}
+	}
+	/* One dummy command added, to clock in the last response */
+	st->tx_buf[j] = 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 = (j + 1) * sizeof(__be16);
+
+	spi_message_init(&st->ring_msg);
+	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
+
+	return 0;
+}
+
+static irqreturn_t adc1x8s102_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev;
+	struct adc1x8s102_state *st;
+	s64 time_ns = 0;
+	int b_sent;
+
+	indio_dev = pf->indio_dev;
+	st = iio_priv(indio_dev);
+
+	b_sent = spi_sync(st->spi, &st->ring_msg);
+	if (b_sent == 0) {
+		if (indio_dev->scan_timestamp) {
+			time_ns = iio_get_time_ns(indio_dev);
+			memcpy((u8 *)st->rx_buf + st->ring_xfer.len, &time_ns,
+			       sizeof(time_ns));
+		}
+
+		/* Skip the dummy response in the first slot */
+		iio_push_to_buffers(indio_dev, (u8 *)&st->rx_buf[1]);
+	}
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int adc1x8s102_scan_direct(struct adc1x8s102_state *st, unsigned int ch)
+{
+	int ret;
+
+	st->tx_buf[0] = cpu_to_be16(ADC1x8S102_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 adc1x8s102_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	int ret;
+	struct adc1x8s102_state *st;
+
+	st = iio_priv(indio_dev);
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
+			ret = -EBUSY;
+			dev_warn(&st->spi->dev,
+			 "indio_dev->currentmode is INDIO_BUFFER_TRIGGERED\n");
+		} else {
+			ret = adc1x8s102_scan_direct(st, chan->address);
+		}
+		mutex_unlock(&indio_dev->mlock);
+
+		if (ret < 0)
+			return ret;
+		*val = ADC1x8S102_RES_DATA(ret);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			if (st->reg)
+				*val = regulator_get_voltage(st->reg) / 1000;
+			else
+				*val = st->ext_vin;
+
+			*val2 = chan->scan_type.realbits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			dev_warn(&st->spi->dev,
+				 "Invalid channel type %u for channel %d\n",
+				 chan->type, chan->channel);
+			return -EINVAL;
+		}
+	default:
+		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO: %lu\n", m);
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info adc1x8s102_info = {
+	.read_raw		= &adc1x8s102_read_raw,
+	.update_scan_mode	= &adc1x8s102_update_scan_mode,
+	.driver_module		= THIS_MODULE,
+};
+
+#ifdef CONFIG_ACPI
+typedef int (*acpi_setup_handler)(struct spi_device *,
+				  const struct adc1x8s102_platform_data **);
+
+static const struct adc1x8s102_platform_data int3495_platform_data = {
+	.ext_vin = 5000,	/* 5 V */
+};
+
+/* Galileo Gen 2 SPI setup */
+static int
+adc1x8s102_setup_int3495(struct spi_device *spi,
+			 const struct adc1x8s102_platform_data **pdata)
+{
+	struct pxa2xx_spi_chip *chip_data;
+
+	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data), GFP_KERNEL);
+	if (!chip_data)
+		return -ENOMEM;
+
+	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
+	spi->controller_data = chip_data;
+	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
+		 chip_data->gpio_cs);
+	spi_setup(spi);
+
+	*pdata = &int3495_platform_data;
+
+	return 0;
+}
+
+static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
+	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
+#endif
+
+static int adc1x8s102_probe(struct spi_device *spi)
+{
+	const struct adc1x8s102_platform_data *pdata = spi->dev.platform_data;
+	struct adc1x8s102_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+#ifdef CONFIG_ACPI
+	if (ACPI_COMPANION(&spi->dev)) {
+		acpi_setup_handler setup_handler;
+		const struct acpi_device_id *id;
+
+		id = acpi_match_device(adc1x8s102_acpi_ids, &spi->dev);
+		if (!id)
+			return -ENODEV;
+
+		setup_handler = (acpi_setup_handler)id->driver_data;
+		if (setup_handler) {
+			ret = setup_handler(spi, &pdata);
+			if (ret)
+				return ret;
+		}
+	}
+#endif
+
+	if (!pdata) {
+		dev_err(&spi->dev, "Cannot get adc1x8s102 platform data\n");
+		return -ENODEV;
+	}
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->ext_vin = pdata->ext_vin;
+
+	/* Use regulator, if available. */
+	st->reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(st->reg)) {
+		dev_err(&spi->dev, "Cannot get 'vref' regulator\n");
+		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 = adc1x8s102_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adc1x8s102_channels);
+	indio_dev->info = &adc1x8s102_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(__be16);
+	st->scan_single_xfer.cs_change = 0;
+
+	spi_message_init(&st->scan_single_msg);
+	spi_message_add_tail(&st->scan_single_xfer, &st->scan_single_msg);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+			&adc1x8s102_trigger_handler, NULL);
+	if (ret)
+		goto error_disable_reg;
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&spi->dev,
+			"Failed to register IIO device\n");
+		goto error_cleanup_ring;
+	}
+	return 0;
+
+error_cleanup_ring:
+	iio_triggered_buffer_cleanup(indio_dev);
+error_disable_reg:
+	regulator_disable(st->reg);
+
+	return ret;
+}
+
+static int adc1x8s102_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct adc1x8s102_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+static const struct spi_device_id adc1x8s102_id[] = {
+	{ "adc1x8s102", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, adc1x8s102_id);
+
+static struct spi_driver adc1x8s102_driver = {
+	.driver = {
+		.name   = "adc1x8s102",
+		.owner	= THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(adc1x8s102_acpi_ids),
+	},
+	.probe		= adc1x8s102_probe,
+	.remove		= adc1x8s102_remove,
+	.id_table	= adc1x8s102_id,
+};
+module_spi_driver(adc1x8s102_driver);
+
+MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
+MODULE_DESCRIPTION("Texas Instruments ADC1x8S102 driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/platform_data/adc1x8s102.h b/include/linux/platform_data/adc1x8s102.h
new file mode 100644
index 000000000000..6ad753c99823
--- /dev/null
+++ b/include/linux/platform_data/adc1x8s102.h
@@ -0,0 +1,28 @@
+/*
+ * ADC1x8S102 SPI ADC driver
+ *
+ * Copyright(c) 2013 Intel Corporation.
+ *
+ * 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.
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_ADC1x8S102_H__
+#define __LINUX_PLATFORM_DATA_ADC1x8S102_H__
+
+/**
+ * struct adc1x8s102_platform_data - Platform data for the adc1x8s102 ADC driver
+ * @ext_vin: External input voltage range for all voltage input channels
+ *	This is the voltage level of pin VA in millivolts
+ **/
+struct adc1x8s102_platform_data {
+	u16  ext_vin;
+};
+
+#endif /* __LINUX_PLATFORM_DATA_ADC1x8S102_H__ */

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Mon, 2017-04-24 at 21:28 +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.
> 
> Original author: Bogdan Pricop <bogdan.pricop@emutex.com>
> Ported from Intel Galileo Gen2 BSP to Intel Yocto kernel:
> Todor Minchev <todor@minchev.co.uk>.

> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/platform_data/adc1x8s102.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/property.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/spi/pxa2xx_spi.h>

Perhaps alphabetical order?

> +
> +/* 16-bit SPI command format:
> + *   [15:14] Ignored
> + *   [13:11] 3-bit channel address
> + *   [10:0]  Ignored
> + */
> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))

I guess ((u16)(ch) << 11) would be slightly better.

> +
> +/*
> + * 16-bit SPI response format:
> + *   [15:12] Zeros
> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be
> 0).
> + */

> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 <<
> ADC1x8S102_BITS) - 1))

GENMASK() and to align with above

((u16)(res) & GENMASK(11, 0))

> +	/* 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];

Would it be better to have tx_buf with ____cache_aligned? (IIUC it's
already by fact of above, though...)

> +};

> tatic int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
> +		unsigned long const *active_scan_mask)
> +{
> +	struct adc1x8s102_state *st;
> +	int i, j;
> +
> +	st = iio_priv(indio_dev);
> +

> +	/* Fill in the first x shorts of tx_buf with the number of
> channels
> +	 * enabled for sampling by the triggered buffer
> +	 */

/*
 * Is it okay style for
 * multi-line comments?
 */

> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
> +		if (test_bit(i, active_scan_mask)) {

for_each_set_bit() 

> +			st->tx_buf[j] =
> cpu_to_be16(ADC1x8S102_CMD(i));
> +			j++;
> +		}
> +	}
> +	/* One dummy command added, to clock in the last response */
> +	st->tx_buf[j] = 0x00;

> +}
> +

> +static int adc1x8s102_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,

> +			   int *val,
> +			   int *val2,
> +			   long m)

One line?

> +{
> +	int ret;
> +	struct adc1x8s102_state *st;
> +
> +	st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> {
> +			ret = -EBUSY;

> +			dev_warn(&st->spi->dev,
> +			 "indio_dev->currentmode is
> INDIO_BUFFER_TRIGGERED\n");

Indentation?

> +		} else {
> +			ret = adc1x8s102_scan_direct(st, chan-
> >address);
> +		}
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret < 0)
> +			return ret;
> +		*val = ADC1x8S102_RES_DATA(ret);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (st->reg)
> +				*val = regulator_get_voltage(st->reg) 
> / 1000;
> +			else
> +				*val = st->ext_vin;
> +
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			dev_warn(&st->spi->dev,
> +				 "Invalid channel type %u for channel
> %d\n",
> +				 chan->type, chan->channel);
> +			return -EINVAL;
> +		}
> +	default:
> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO:
> %lu\n", m);
> +		return -EINVAL;
> +	}
> +}

> +#ifdef CONFIG_ACPI
> +typedef int (*acpi_setup_handler)(struct spi_device *,
> +				  const struct
> adc1x8s102_platform_data **);
> +
> +static const struct adc1x8s102_platform_data int3495_platform_data =
> {
> +	.ext_vin = 5000,	/* 5 V */
> +};
> +

> +/* Galileo Gen 2 SPI setup */
> +static int
> +adc1x8s102_setup_int3495(struct spi_device *spi,
> +			 const struct adc1x8s102_platform_data
> **pdata)
> +{

> +	struct pxa2xx_spi_chip *chip_data;

This one is too big to waste memory on one member.

> +
> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data),
> GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;
> +
> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
> +	spi->controller_data = chip_data;
> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
> +		 chip_data->gpio_cs);
> +	spi_setup(spi);
> +
> +	*pdata = &int3495_platform_data;
> +
> +	return 0;
> +}

This is weird approach.
Moreover, please do not use platform data at all.

> +
> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
> +#endif
> +
> +static int adc1x8s102_probe(struct spi_device *spi)
> +{
> +	const struct adc1x8s102_platform_data *pdata = spi-
> >dev.platform_data;
> +	struct adc1x8s102_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +

> +#ifdef CONFIG_ACPI

No.

> +	if (ACPI_COMPANION(&spi->dev)) {
> +		acpi_setup_handler setup_handler;
> +		const struct acpi_device_id *id;
> +
> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi-
> >dev);
> +		if (!id)
> +			return -ENODEV;
> +

> +		setup_handler = (acpi_setup_handler)id->driver_data;
> +		if (setup_handler) {
> +			ret = setup_handler(spi, &pdata);
> +			if (ret)
> +				return ret;
> +		}

No way.

> +	}
> +#endif
> +
> +	if (!pdata) {
> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform
> data\n");
> +		return -ENODEV;
> +	}
> +

> +error_cleanup_ring:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +error_disable_reg:
> +	regulator_disable(st->reg);

Does devm_() help to get rid of these?

> +
> +	return ret;
> +}

> +
> +static int adc1x8s102_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
> +

> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	regulator_disable(st->reg);

Ditto.

> +
> +	return 0;
> +}
> +

> +++ b/include/linux/platform_data/adc1x8s102.h

It must be no such file at all!
Please, remove it completely.

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Mon, 2017-04-24 at 23:05 +0300, Andy Shevchenko wrote:
> On Mon, 2017-04-24 at 21:28 +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.

> > +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
> > +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);

One more thing, please provide an excerpt from real DSDT that describes
this device.

And just in case one question, is that DSDT carved in stone?

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-24 22:05, Andy Shevchenko wrote:
> On Mon, 2017-04-24 at 21:28 +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.
>>
>> Original author: Bogdan Pricop <bogdan.pricop@emutex.com>
>> Ported from Intel Galileo Gen2 BSP to Intel Yocto kernel:
>> Todor Minchev <todor@minchev.co.uk>.
> 
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <linux/platform_data/adc1x8s102.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/acpi.h>
>> +#include <linux/property.h>
>> +#include <linux/gpio.h>
>> +
>> +#include <linux/spi/pxa2xx_spi.h>
> 
> Perhaps alphabetical order?
> 
>> +
>> +/* 16-bit SPI command format:
>> + *   [15:14] Ignored
>> + *   [13:11] 3-bit channel address
>> + *   [10:0]  Ignored
>> + */
>> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))
> 
> I guess ((u16)(ch) << 11) would be slightly better.
> 
>> +
>> +/*
>> + * 16-bit SPI response format:
>> + *   [15:12] Zeros
>> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be
>> 0).
>> + */
> 
>> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 <<
>> ADC1x8S102_BITS) - 1))
> 
> GENMASK() and to align with above
> 
> ((u16)(res) & GENMASK(11, 0))
> 
>> +	/* 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];
> 
> Would it be better to have tx_buf with ____cache_aligned? (IIUC it's
> already by fact of above, though...)
> 
>> +};
> 
>> tatic int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
>> +		unsigned long const *active_scan_mask)
>> +{
>> +	struct adc1x8s102_state *st;
>> +	int i, j;
>> +
>> +	st = iio_priv(indio_dev);
>> +
> 
>> +	/* Fill in the first x shorts of tx_buf with the number of
>> channels
>> +	 * enabled for sampling by the triggered buffer
>> +	 */
> 
> /*
>  * Is it okay style for
>  * multi-line comments?
>  */
> 
>> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
>> +		if (test_bit(i, active_scan_mask)) {
> 
> for_each_set_bit() 
> 
>> +			st->tx_buf[j] =
>> cpu_to_be16(ADC1x8S102_CMD(i));
>> +			j++;
>> +		}
>> +	}
>> +	/* One dummy command added, to clock in the last response */
>> +	st->tx_buf[j] = 0x00;
> 
>> +}
>> +
> 
>> +static int adc1x8s102_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
> 
>> +			   int *val,
>> +			   int *val2,
>> +			   long m)
> 
> One line?
> 
>> +{
>> +	int ret;
>> +	struct adc1x8s102_state *st;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&indio_dev->mlock);
>> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
>> {
>> +			ret = -EBUSY;
> 
>> +			dev_warn(&st->spi->dev,
>> +			 "indio_dev->currentmode is
>> INDIO_BUFFER_TRIGGERED\n");
> 
> Indentation?
> 

All valid remarks, will address.

>> +		} else {
>> +			ret = adc1x8s102_scan_direct(st, chan-
>>> address);
>> +		}
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = ADC1x8S102_RES_DATA(ret);
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_VOLTAGE:
>> +			if (st->reg)
>> +				*val = regulator_get_voltage(st->reg) 
>> / 1000;
>> +			else
>> +				*val = st->ext_vin;
>> +
>> +			*val2 = chan->scan_type.realbits;
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		default:
>> +			dev_warn(&st->spi->dev,
>> +				 "Invalid channel type %u for channel
>> %d\n",
>> +				 chan->type, chan->channel);
>> +			return -EINVAL;
>> +		}
>> +	default:
>> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO:
>> %lu\n", m);
>> +		return -EINVAL;
>> +	}
>> +}
> 
>> +#ifdef CONFIG_ACPI
>> +typedef int (*acpi_setup_handler)(struct spi_device *,
>> +				  const struct
>> adc1x8s102_platform_data **);
>> +
>> +static const struct adc1x8s102_platform_data int3495_platform_data =
>> {
>> +	.ext_vin = 5000,	/* 5 V */
>> +};
>> +
> 
>> +/* Galileo Gen 2 SPI setup */
>> +static int
>> +adc1x8s102_setup_int3495(struct spi_device *spi,
>> +			 const struct adc1x8s102_platform_data
>> **pdata)
>> +{
> 
>> +	struct pxa2xx_spi_chip *chip_data;
> 
> This one is too big to waste memory on one member.
> 
>> +
>> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data),
>> GFP_KERNEL);
>> +	if (!chip_data)
>> +		return -ENOMEM;
>> +
>> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>> +	spi->controller_data = chip_data;
>> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>> +		 chip_data->gpio_cs);
>> +	spi_setup(spi);
>> +
>> +	*pdata = &int3495_platform_data;
>> +
>> +	return 0;
>> +}
> 
> This is weird approach.

Let me dig deeper if are allowed to pass a static struct here as well.
But the struct is driver-defined.

> Moreover, please do not use platform data at all.

That is just following pre-existing pattern, just look around in the
iio/adc folder, not to speak of others. But I'm open to learn about any
newer pattern there is.

> 
>> +
>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
>> +#endif
>> +
>> +static int adc1x8s102_probe(struct spi_device *spi)
>> +{
>> +	const struct adc1x8s102_platform_data *pdata = spi-
>>> dev.platform_data;
>> +	struct adc1x8s102_state *st;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
> 
>> +#ifdef CONFIG_ACPI
> 
> No.

...because?

> 
>> +	if (ACPI_COMPANION(&spi->dev)) {
>> +		acpi_setup_handler setup_handler;
>> +		const struct acpi_device_id *id;
>> +
>> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi-
>>> dev);
>> +		if (!id)
>> +			return -ENODEV;
>> +
> 
>> +		setup_handler = (acpi_setup_handler)id->driver_data;
>> +		if (setup_handler) {
>> +			ret = setup_handler(spi, &pdata);
>> +			if (ret)
>> +				return ret;
>> +		}
> 
> No way.

Constructive feedback, please.

> 
>> +	}
>> +#endif
>> +
>> +	if (!pdata) {
>> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform
>> data\n");
>> +		return -ENODEV;
>> +	}
>> +
> 
>> +error_cleanup_ring:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +error_disable_reg:
>> +	regulator_disable(st->reg);
> 
> Does devm_() help to get rid of these?

Yep. See also other drivers.

> 
>> +
>> +	return ret;
>> +}
> 
>> +
>> +static int adc1x8s102_remove(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
>> +
> 
>> +	iio_device_unregister(indio_dev);
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +	regulator_disable(st->reg);
> 
> Ditto.
> 
>> +
>> +	return 0;
>> +}
>> +
> 
>> +++ b/include/linux/platform_data/adc1x8s102.h
> 
> It must be no such file at all!
> Please, remove it completely.

Not without explaining what the new style is. As I said, the existing
driver use that as well. The fact that there is no OF binding yet
exploiting this should be no excuse IMHO.

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-24 22:10, Andy Shevchenko wrote:
> On Mon, 2017-04-24 at 23:05 +0300, Andy Shevchenko wrote:
>> On Mon, 2017-04-24 at 21:28 +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.
> 
>>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>>> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
> 
> One more thing, please provide an excerpt from real DSDT that describes
> this device.

If I didn't mess things up here, this should be what the Galileo Gen2
reports:

                Device (ADC2)
                {
                    Name (_HID, "INT3495")  // _HID: Hardware ID
                    Name (_CID, "INT3495")  // _CID: Compatible ID
                    Name (RBUF, ResourceTemplate ()
                    {
                        SpiSerialBus (0x0000, PolarityLow, FourWireMode, 0x10,
                            ControllerInitiated, 0x00FE5178, ClockPolarityHigh,
                            ClockPhaseSecond, "\\_SB.PCI0.SPI0",
                            0x00, ResourceConsumer, ,
                            )
                        GpioIo (Shared, PullDefault, 0x0000, 0x0000, IoRestrictionNone,
                            "\\_SB.PCI0.GIP0.GPO_", 0x00, ResourceConsumer, ,
                            )
                            {   // Pin list
                                0x0008
                            }
                    })
                    Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
                    {
                        Return (RBUF)
                    }

                    Method (_STA, 0, NotSerialized)  // _STA: Status
                    {
                        If (LNotEqual (PTYP, 0x08))
                        {
                            Return (Zero)
                        }

                        Return (0x0F)
                    }
                }


> 
> And just in case one question, is that DSDT carved in stone?
> 

Yes, for the existing devices: The Galieo is shipped for years with this
DSDT, the IOT2000 for about half a year.

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Mon, Apr 24, 2017 at 11:32 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-04-24 22:05, Andy Shevchenko wrote:
>> On Mon, 2017-04-24 at 21:28 +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.

>>> +#ifdef CONFIG_ACPI
>>> +typedef int (*acpi_setup_handler)(struct spi_device *,
>>> +                              const struct
>>> adc1x8s102_platform_data **);
>>> +
>>> +static const struct adc1x8s102_platform_data int3495_platform_data =
>>> {
>>> +    .ext_vin = 5000,        /* 5 V */
>>> +};
>>> +
>>
>>> +/* Galileo Gen 2 SPI setup */
>>> +static int
>>> +adc1x8s102_setup_int3495(struct spi_device *spi,
>>> +                     const struct adc1x8s102_platform_data
>>> **pdata)
>>> +{
>>
>>> +    struct pxa2xx_spi_chip *chip_data;
>>
>> This one is too big to waste memory on one member.
>>
>>> +
>>> +    chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data),
>>> GFP_KERNEL);
>>> +    if (!chip_data)
>>> +            return -ENOMEM;
>>> +
>>> +    chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>>> +    spi->controller_data = chip_data;
>>> +    dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>>> +             chip_data->gpio_cs);
>>> +    spi_setup(spi);
>>> +
>>> +    *pdata = &int3495_platform_data;
>>> +
>>> +    return 0;
>>> +}
>>
>> This is weird approach.
>
> Let me dig deeper if are allowed to pass a static struct here as well.
> But the struct is driver-defined.

We have _DSD for ACPI, that's why I sent another email where I was
asking for DSDT excerpt and if it's already in the wild.

>
>> Moreover, please do not use platform data at all.
>
> That is just following pre-existing pattern, just look around in the
> iio/adc folder, not to speak of others. But I'm open to learn about any
> newer pattern there is.

Unified Device Properties API is your friend. It makes driver to
consume resources in agnostic way.

>>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>>> +    { "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>>> +    { }
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
>>> +#endif
>>> +
>>> +static int adc1x8s102_probe(struct spi_device *spi)
>>> +{
>>> +    const struct adc1x8s102_platform_data *pdata = spi-
>>>> dev.platform_data;
>>> +    struct adc1x8s102_state *st;
>>> +    struct iio_dev *indio_dev;
>>> +    int ret;
>>> +
>>
>>> +#ifdef CONFIG_ACPI
>>
>> No.
>
> ...because?

Because in correctly written ->probe() all ACPI functions have stubs
for !CONFIG_ACPI case. Just no need.

>>> +            setup_handler = (acpi_setup_handler)id->driver_data;
>>> +            if (setup_handler) {
>>> +                    ret = setup_handler(spi, &pdata);
>>> +                    if (ret)
>>> +                            return ret;
>>> +            }
>>
>> No way.
>
> Constructive feedback, please.

See above. We have nowadays mechanisms to provide device properties natively.
Without seeing DSDT I can't tell more.

>>> +++ b/include/linux/platform_data/adc1x8s102.h
>>
>> It must be no such file at all!
>> Please, remove it completely.
>
> Not without explaining what the new style is. As I said, the existing
> driver use that as well.

See above.

> The fact that there is no OF binding yet
> exploiting this should be no excuse IMHO.

...and I'm not talking about it at all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-24 23:25, Andy Shevchenko wrote:
> On Mon, Apr 24, 2017 at 11:32 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-04-24 22:05, Andy Shevchenko wrote:
>>> On Mon, 2017-04-24 at 21:28 +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.
> 
>>>> +#ifdef CONFIG_ACPI
>>>> +typedef int (*acpi_setup_handler)(struct spi_device *,
>>>> +                              const struct
>>>> adc1x8s102_platform_data **);
>>>> +
>>>> +static const struct adc1x8s102_platform_data int3495_platform_data =
>>>> {
>>>> +    .ext_vin = 5000,        /* 5 V */
>>>> +};
>>>> +
>>>
>>>> +/* Galileo Gen 2 SPI setup */
>>>> +static int
>>>> +adc1x8s102_setup_int3495(struct spi_device *spi,
>>>> +                     const struct adc1x8s102_platform_data
>>>> **pdata)
>>>> +{
>>>
>>>> +    struct pxa2xx_spi_chip *chip_data;
>>>
>>> This one is too big to waste memory on one member.
>>>
>>>> +
>>>> +    chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data),
>>>> GFP_KERNEL);
>>>> +    if (!chip_data)
>>>> +            return -ENOMEM;
>>>> +
>>>> +    chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>>>> +    spi->controller_data = chip_data;
>>>> +    dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>>>> +             chip_data->gpio_cs);
>>>> +    spi_setup(spi);
>>>> +
>>>> +    *pdata = &int3495_platform_data;
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> This is weird approach.
>>
>> Let me dig deeper if are allowed to pass a static struct here as well.
>> But the struct is driver-defined.
> 
> We have _DSD for ACPI, that's why I sent another email where I was
> asking for DSDT excerpt and if it's already in the wild.

I don't find any traces of "_DSD" in those DSDTs.

> 
>>
>>> Moreover, please do not use platform data at all.
>>
>> That is just following pre-existing pattern, just look around in the
>> iio/adc folder, not to speak of others. But I'm open to learn about any
>> newer pattern there is.
> 
> Unified Device Properties API is your friend. It makes driver to
> consume resources in agnostic way.

Is that ACPI-only or a generic solution? Where is a good example? Sorry,
I still don't see how to make code out of your comments.

> 
>>>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>>>> +    { "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>>>> +    { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
>>>> +#endif
>>>> +
>>>> +static int adc1x8s102_probe(struct spi_device *spi)
>>>> +{
>>>> +    const struct adc1x8s102_platform_data *pdata = spi-
>>>>> dev.platform_data;
>>>> +    struct adc1x8s102_state *st;
>>>> +    struct iio_dev *indio_dev;
>>>> +    int ret;
>>>> +
>>>
>>>> +#ifdef CONFIG_ACPI
>>>
>>> No.
>>
>> ...because?
> 
> Because in correctly written ->probe() all ACPI functions have stubs
> for !CONFIG_ACPI case. Just no need.

OK, will give that a try. I just don't want to leave much dead code
behind for !CONFIG_ACPI.

> 
>>>> +            setup_handler = (acpi_setup_handler)id->driver_data;
>>>> +            if (setup_handler) {
>>>> +                    ret = setup_handler(spi, &pdata);
>>>> +                    if (ret)
>>>> +                            return ret;
>>>> +            }
>>>
>>> No way.
>>
>> Constructive feedback, please.
> 
> See above. We have nowadays mechanisms to provide device properties natively.
> Without seeing DSDT I can't tell more.

You've seen it, please tell me more now.

> 
>>>> +++ b/include/linux/platform_data/adc1x8s102.h
>>>
>>> It must be no such file at all!
>>> Please, remove it completely.
>>
>> Not without explaining what the new style is. As I said, the existing
>> driver use that as well.
> 
> See above.
> 
>> The fact that there is no OF binding yet
>> exploiting this should be no excuse IMHO.
> 
> ...and I'm not talking about it at all.
> 

But I am: ACPI is not the center of the world (luckily), and this driver
shall not be designed to only work with that way of defining resources.
Therefore, I'm trying to follow driver which include OF support.

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-24 22:05, Andy Shevchenko wrote:
>> +{
>> +	int ret;
>> +	struct adc1x8s102_state *st;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&indio_dev->mlock);
>> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
>> {
>> +			ret = -EBUSY;
> 
>> +			dev_warn(&st->spi->dev,
>> +			 "indio_dev->currentmode is
>> INDIO_BUFFER_TRIGGERED\n");
> 
> Indentation?

Actually, this block needs to be converted to
iio_device_claim_direct_mode / iio_device_release_direct_mode, instead
of doing the old open-coded locking way.

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Peter Meerwald-Stadler]

[-- Attachment #1: Type: text/plain, Size: 16414 bytes --]


> 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.

comments below

naming: don't use placeholders, name after one of the supported chips and 
list them in Kconfig and the driver file

what is the difference between this chip and those supported 
by ti-adc084s021 which was proposed by Mårten Lindahl on April 21?

I think board-specific stuff should not go into the driver -> DT?
 
> 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>
> ---
>  drivers/iio/adc/Kconfig                  |  12 +
>  drivers/iio/adc/Makefile                 |   1 +
>  drivers/iio/adc/ti-adc1x8s102.c          | 408 +++++++++++++++++++++++++++++++
>  include/linux/platform_data/adc1x8s102.h |  28 +++
>  4 files changed, 449 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-adc1x8s102.c
>  create mode 100644 include/linux/platform_data/adc1x8s102.h
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7adbce9..edb7254a648c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -582,6 +582,18 @@ config TI_ADC128S052
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
>  
> +config TI_ADC1x8S102
> +	tristate "Texas Instruments ADC1x8S102 driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Texas Instruments ADC1x8S102 ADC.
> +	  Provides direct access via sysfs.

drop the 'direct access' statement

> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called ti-adc1x8s102

end with .

> +
>  config TI_ADC161S626
>  	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d0012620cd1c..d5d913bc1263 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> +obj-$(CONFIG_TI_ADC1x8S102) += ti-adc1x8s102.o
>  obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> diff --git a/drivers/iio/adc/ti-adc1x8s102.c b/drivers/iio/adc/ti-adc1x8s102.c
> new file mode 100644
> index 000000000000..4f94c371489d
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc1x8s102.c
> @@ -0,0 +1,408 @@
> +/*
> + * TI ADC1x8S102 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 is designed to work with the following

is is

> + * analog to digital converters from Texas Instruments:
> + *  ADC108S102
> + *  ADC128S102
> + * The communication with ADC chip is via the SPI bus (mode 3).
> + */
> +
> +#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/spi/spi.h>
> +
> +#include <linux/platform_data/adc1x8s102.h>

who needs platform data these days :)

> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/property.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/spi/pxa2xx_spi.h>

no, this shouldn't be here

> +
> +/*
> + * 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 ADC1x8S102_BITS		12
> +#define ADC1x8S102_MAX_CHANNELS	8
> +
> +/* 16-bit SPI command format:
> + *   [15:14] Ignored
> + *   [13:11] 3-bit channel address
> + *   [10:0]  Ignored
> + */
> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))

no need to put parenthesis around 8 and 3
why not shift by 11?

> +
> +/*
> + * 16-bit SPI response format:
> + *   [15:12] Zeros
> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
> + */
> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 << ADC1x8S102_BITS) - 1))

could use GENMASK()

> +
> +#define ADC1x8S102_GALILEO2_CS	8

this board-specific detail shouldn't be here

> +
> +struct adc1x8s102_state {
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	u16				ext_vin;

unit?

> +	/* 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:

not a proper multi-line comment

> +	 *  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];
> +};
> +
> +#define ADC1X8S102_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 = ADC1x8S102_BITS,			\

this should be different for the 128 and 108 part, shift missing

most drivers do shifting and don't use _SCALE for that purpose

> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec adc1x8s102_channels[] = {
> +	ADC1X8S102_V_CHAN(0),
> +	ADC1X8S102_V_CHAN(1),
> +	ADC1X8S102_V_CHAN(2),
> +	ADC1X8S102_V_CHAN(3),
> +	ADC1X8S102_V_CHAN(4),
> +	ADC1X8S102_V_CHAN(5),
> +	ADC1X8S102_V_CHAN(6),
> +	ADC1X8S102_V_CHAN(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
> +		unsigned long const *active_scan_mask)
> +{
> +	struct adc1x8s102_state *st;
> +	int i, j;
> +
> +	st = iio_priv(indio_dev);
> +
> +	/* Fill in the first x shorts of tx_buf with the number of channels

not a multi-line comment

> +	 * enabled for sampling by the triggered buffer
> +	 */
> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
> +		if (test_bit(i, active_scan_mask)) {
> +			st->tx_buf[j] = cpu_to_be16(ADC1x8S102_CMD(i));
> +			j++;
> +		}
> +	}
> +	/* One dummy command added, to clock in the last response */
> +	st->tx_buf[j] = 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 = (j + 1) * sizeof(__be16);
> +
> +	spi_message_init(&st->ring_msg);
> +	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t adc1x8s102_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev;
> +	struct adc1x8s102_state *st;
> +	s64 time_ns = 0;

no need to initialize

> +	int b_sent;
> +
> +	indio_dev = pf->indio_dev;
> +	st = iio_priv(indio_dev);
> +
> +	b_sent = spi_sync(st->spi, &st->ring_msg);
> +	if (b_sent == 0) {
> +		if (indio_dev->scan_timestamp) {
> +			time_ns = iio_get_time_ns(indio_dev);
> +			memcpy((u8 *)st->rx_buf + st->ring_xfer.len, &time_ns,
> +			       sizeof(time_ns));
> +		}
> +
> +		/* Skip the dummy response in the first slot */
> +		iio_push_to_buffers(indio_dev, (u8 *)&st->rx_buf[1]);

iio_push_to_buffers_with_timestamp()?

> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adc1x8s102_scan_direct(struct adc1x8s102_state *st, unsigned int ch)
> +{
> +	int ret;
> +
> +	st->tx_buf[0] = cpu_to_be16(ADC1x8S102_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 adc1x8s102_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	int ret;
> +	struct adc1x8s102_state *st;
> +
> +	st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:

iio_device_claim_direct_mode()?

> +		mutex_lock(&indio_dev->mlock);
> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +			ret = -EBUSY;
> +			dev_warn(&st->spi->dev,
> +			 "indio_dev->currentmode is INDIO_BUFFER_TRIGGERED\n");
> +		} else {
> +			ret = adc1x8s102_scan_direct(st, chan->address);
> +		}
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret < 0)
> +			return ret;
> +		*val = ADC1x8S102_RES_DATA(ret);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (st->reg)
> +				*val = regulator_get_voltage(st->reg) / 1000;
> +			else
> +				*val = st->ext_vin;
> +
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			dev_warn(&st->spi->dev,
> +				 "Invalid channel type %u for channel %d\n",
> +				 chan->type, chan->channel);

message necessary?

> +			return -EINVAL;
> +		}
> +	default:
> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO: %lu\n", m);

message necessary?

> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info adc1x8s102_info = {
> +	.read_raw		= &adc1x8s102_read_raw,
> +	.update_scan_mode	= &adc1x8s102_update_scan_mode,
> +	.driver_module		= THIS_MODULE,
> +};
> +
> +#ifdef CONFIG_ACPI
> +typedef int (*acpi_setup_handler)(struct spi_device *,
> +				  const struct adc1x8s102_platform_data **);
> +

no board specific stuff here please

> +static const struct adc1x8s102_platform_data int3495_platform_data = {
> +	.ext_vin = 5000,	/* 5 V */
> +};
> +
> +/* Galileo Gen 2 SPI setup */
> +static int
> +adc1x8s102_setup_int3495(struct spi_device *spi,
> +			 const struct adc1x8s102_platform_data **pdata)
> +{
> +	struct pxa2xx_spi_chip *chip_data;
> +
> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data), GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;
> +
> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
> +	spi->controller_data = chip_data;
> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
> +		 chip_data->gpio_cs);
> +	spi_setup(spi);
> +
> +	*pdata = &int3495_platform_data;
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
> +#endif
> +
> +static int adc1x8s102_probe(struct spi_device *spi)
> +{
> +	const struct adc1x8s102_platform_data *pdata = spi->dev.platform_data;
> +	struct adc1x8s102_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +#ifdef CONFIG_ACPI
> +	if (ACPI_COMPANION(&spi->dev)) {
> +		acpi_setup_handler setup_handler;
> +		const struct acpi_device_id *id;
> +
> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi->dev);
> +		if (!id)
> +			return -ENODEV;
> +
> +		setup_handler = (acpi_setup_handler)id->driver_data;
> +		if (setup_handler) {
> +			ret = setup_handler(spi, &pdata);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +#endif
> +
> +	if (!pdata) {
> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform data\n");
> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->ext_vin = pdata->ext_vin;
> +
> +	/* Use regulator, if available. */
> +	st->reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(st->reg)) {
> +		dev_err(&spi->dev, "Cannot get 'vref' regulator\n");
> +		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 = adc1x8s102_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adc1x8s102_channels);
> +	indio_dev->info = &adc1x8s102_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(__be16);
> +	st->scan_single_xfer.cs_change = 0;
> +
> +	spi_message_init(&st->scan_single_msg);
> +	spi_message_add_tail(&st->scan_single_xfer, &st->scan_single_msg);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +			&adc1x8s102_trigger_handler, NULL);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev,
> +			"Failed to register IIO device\n");
> +		goto error_cleanup_ring;
> +	}
> +	return 0;
> +
> +error_cleanup_ring:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +error_disable_reg:
> +	regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +
> +static int adc1x8s102_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id adc1x8s102_id[] = {
> +	{ "adc1x8s102", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adc1x8s102_id);
> +
> +static struct spi_driver adc1x8s102_driver = {
> +	.driver = {
> +		.name   = "adc1x8s102",
> +		.owner	= THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(adc1x8s102_acpi_ids),
> +	},
> +	.probe		= adc1x8s102_probe,
> +	.remove		= adc1x8s102_remove,
> +	.id_table	= adc1x8s102_id,
> +};
> +module_spi_driver(adc1x8s102_driver);
> +
> +MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADC1x8S102 driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/adc1x8s102.h b/include/linux/platform_data/adc1x8s102.h
> new file mode 100644
> index 000000000000..6ad753c99823
> --- /dev/null
> +++ b/include/linux/platform_data/adc1x8s102.h
> @@ -0,0 +1,28 @@
> +/*
> + * ADC1x8S102 SPI ADC driver
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + *
> + * 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.
> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_ADC1x8S102_H__
> +#define __LINUX_PLATFORM_DATA_ADC1x8S102_H__
> +
> +/**
> + * struct adc1x8s102_platform_data - Platform data for the adc1x8s102 ADC driver
> + * @ext_vin: External input voltage range for all voltage input channels
> + *	This is the voltage level of pin VA in millivolts
> + **/
> +struct adc1x8s102_platform_data {
> +	u16  ext_vin;
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_ADC1x8S102_H__ */
> 

-- 

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Tue, Apr 25, 2017 at 10:31 AM, Peter Meerwald-Stadler
<pmeerw@pmeerw.net> 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.

> I think board-specific stuff should not go into the driver -> DT?

World is not ARM/DT only -> Unified Device Properties, yes.

P.S. I agree with everything else, though it looks a bit overlapping
with my review, which is a good sign to me :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-25 09:31, Peter Meerwald-Stadler 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.
> 
> comments below
> 
> naming: don't use placeholders, name after one of the supported chips and 
> list them in Kconfig and the driver file

No problem.

> 
> what is the difference between this chip and those supported 
> by ti-adc084s021 which was proposed by Mårten Lindahl on April 21?

I'm not an expert in all those variants, I've "just" adopted the driver
where Intel and Yocto apparently dropped it. But let me try to find out
more.

> 
> I think board-specific stuff should not go into the driver -> DT?

Unfortunately, we only have half-baked ACPI on those boards.

>  
>> 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>
>> ---
>>  drivers/iio/adc/Kconfig                  |  12 +
>>  drivers/iio/adc/Makefile                 |   1 +
>>  drivers/iio/adc/ti-adc1x8s102.c          | 408 +++++++++++++++++++++++++++++++
>>  include/linux/platform_data/adc1x8s102.h |  28 +++
>>  4 files changed, 449 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-adc1x8s102.c
>>  create mode 100644 include/linux/platform_data/adc1x8s102.h
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index dedae7adbce9..edb7254a648c 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -582,6 +582,18 @@ config TI_ADC128S052
>>  	  This driver can also be built as a module. If so, the module will be
>>  	  called ti-adc128s052.
>>  
>> +config TI_ADC1x8S102
>> +	tristate "Texas Instruments ADC1x8S102 driver"
>> +	depends on SPI
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>> +	help
>> +	  Say yes here to build support for Texas Instruments ADC1x8S102 ADC.
>> +	  Provides direct access via sysfs.
> 
> drop the 'direct access' statement
> 
>> +
>> +	  To compile this driver as a module, choose M here: the module will
>> +	  be called ti-adc1x8s102
> 
> end with .
> 
>> +
>>  config TI_ADC161S626
>>  	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
>>  	depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index d0012620cd1c..d5d913bc1263 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
>> +obj-$(CONFIG_TI_ADC1x8S102) += ti-adc1x8s102.o
>>  obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
>> diff --git a/drivers/iio/adc/ti-adc1x8s102.c b/drivers/iio/adc/ti-adc1x8s102.c
>> new file mode 100644
>> index 000000000000..4f94c371489d
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-adc1x8s102.c
>> @@ -0,0 +1,408 @@
>> +/*
>> + * TI ADC1x8S102 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 is designed to work with the following
> 
> is is
> 
>> + * analog to digital converters from Texas Instruments:
>> + *  ADC108S102
>> + *  ADC128S102
>> + * The communication with ADC chip is via the SPI bus (mode 3).
>> + */
>> +
>> +#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/spi/spi.h>
>> +
>> +#include <linux/platform_data/adc1x8s102.h>
> 
> who needs platform data these days :)

Apparently, quite a few devices.

But the situation here is:
- Existing hardware has incomplete ACPI description that needs to be
  augmented by software.
- I'm not aware of a complete DT specification for this device. If
  there is any somewhere, I can happily include it.

> 
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/acpi.h>
>> +#include <linux/property.h>
>> +#include <linux/gpio.h>
>> +
>> +#include <linux/spi/pxa2xx_spi.h>
> 
> no, this shouldn't be here
> 
>> +
>> +/*
>> + * 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 ADC1x8S102_BITS		12
>> +#define ADC1x8S102_MAX_CHANNELS	8
>> +
>> +/* 16-bit SPI command format:
>> + *   [15:14] Ignored
>> + *   [13:11] 3-bit channel address
>> + *   [10:0]  Ignored
>> + */
>> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))
> 
> no need to put parenthesis around 8 and 3
> why not shift by 11?
> 
>> +
>> +/*
>> + * 16-bit SPI response format:
>> + *   [15:12] Zeros
>> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
>> + */
>> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 << ADC1x8S102_BITS) - 1))
> 
> could use GENMASK()
> 

Both already fixed locally (Andy noted that as well).

>> +
>> +#define ADC1x8S102_GALILEO2_CS	8
> 
> this board-specific detail shouldn't be here

Where should it go then?

> 
>> +
>> +struct adc1x8s102_state {
>> +	struct spi_device		*spi;
>> +	struct regulator		*reg;
>> +	u16				ext_vin;
> 
> unit?

ext_vin_mv?

> 
>> +	/* 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:
> 
> not a proper multi-line comment

Hmm, checkpatch didn't complain, but your are right.

> 
>> +	 *  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];
>> +};
>> +
>> +#define ADC1X8S102_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 = ADC1x8S102_BITS,			\
> 
> this should be different for the 128 and 108 part, shift missing
> 
> most drivers do shifting and don't use _SCALE for that purpose

Unfortunately, I cannot test the 128 as we only have the 108 built in. I
adjust blindly, of course.

> 
>> +			.storagebits = 16,				\
>> +			.endianness = IIO_BE,				\
>> +		},							\
>> +	}
>> +
>> +static const struct iio_chan_spec adc1x8s102_channels[] = {
>> +	ADC1X8S102_V_CHAN(0),
>> +	ADC1X8S102_V_CHAN(1),
>> +	ADC1X8S102_V_CHAN(2),
>> +	ADC1X8S102_V_CHAN(3),
>> +	ADC1X8S102_V_CHAN(4),
>> +	ADC1X8S102_V_CHAN(5),
>> +	ADC1X8S102_V_CHAN(6),
>> +	ADC1X8S102_V_CHAN(7),
>> +	IIO_CHAN_SOFT_TIMESTAMP(8),
>> +};
>> +
>> +static int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
>> +		unsigned long const *active_scan_mask)
>> +{
>> +	struct adc1x8s102_state *st;
>> +	int i, j;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	/* Fill in the first x shorts of tx_buf with the number of channels
> 
> not a multi-line comment
> 
>> +	 * enabled for sampling by the triggered buffer
>> +	 */
>> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
>> +		if (test_bit(i, active_scan_mask)) {
>> +			st->tx_buf[j] = cpu_to_be16(ADC1x8S102_CMD(i));
>> +			j++;
>> +		}
>> +	}
>> +	/* One dummy command added, to clock in the last response */
>> +	st->tx_buf[j] = 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 = (j + 1) * sizeof(__be16);
>> +
>> +	spi_message_init(&st->ring_msg);
>> +	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t adc1x8s102_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev;
>> +	struct adc1x8s102_state *st;
>> +	s64 time_ns = 0;
> 
> no need to initialize
> 
>> +	int b_sent;
>> +
>> +	indio_dev = pf->indio_dev;
>> +	st = iio_priv(indio_dev);
>> +
>> +	b_sent = spi_sync(st->spi, &st->ring_msg);
>> +	if (b_sent == 0) {
>> +		if (indio_dev->scan_timestamp) {
>> +			time_ns = iio_get_time_ns(indio_dev);
>> +			memcpy((u8 *)st->rx_buf + st->ring_xfer.len, &time_ns,
>> +			       sizeof(time_ns));
>> +		}
>> +
>> +		/* Skip the dummy response in the first slot */
>> +		iio_push_to_buffers(indio_dev, (u8 *)&st->rx_buf[1]);
> 
> iio_push_to_buffers_with_timestamp()?

No idea. If you tell me that this should work, I will put it in.

> 
>> +	}
>> +
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int adc1x8s102_scan_direct(struct adc1x8s102_state *st, unsigned int ch)
>> +{
>> +	int ret;
>> +
>> +	st->tx_buf[0] = cpu_to_be16(ADC1x8S102_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 adc1x8s102_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val,
>> +			   int *val2,
>> +			   long m)
>> +{
>> +	int ret;
>> +	struct adc1x8s102_state *st;
>> +
>> +	st = iio_priv(indio_dev);
>> +
>> +	switch (m) {
>> +	case IIO_CHAN_INFO_RAW:
> 
> iio_device_claim_direct_mode()?

Already found and fixed locally.

> 
>> +		mutex_lock(&indio_dev->mlock);
>> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
>> +			ret = -EBUSY;
>> +			dev_warn(&st->spi->dev,
>> +			 "indio_dev->currentmode is INDIO_BUFFER_TRIGGERED\n");
>> +		} else {
>> +			ret = adc1x8s102_scan_direct(st, chan->address);
>> +		}
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +		*val = ADC1x8S102_RES_DATA(ret);
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_VOLTAGE:
>> +			if (st->reg)
>> +				*val = regulator_get_voltage(st->reg) / 1000;
>> +			else
>> +				*val = st->ext_vin;
>> +
>> +			*val2 = chan->scan_type.realbits;
>> +			return IIO_VAL_FRACTIONAL_LOG2;
>> +		default:
>> +			dev_warn(&st->spi->dev,
>> +				 "Invalid channel type %u for channel %d\n",
>> +				 chan->type, chan->channel);
> 
> message necessary?

No idea. The original code contained some "defensive" checks that I
removed. If this case is prevented by the IIO core, I'll drop it.

> 
>> +			return -EINVAL;
>> +		}
>> +	default:
>> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO: %lu\n", m);
> 
> message necessary?

Same here.

> 
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct iio_info adc1x8s102_info = {
>> +	.read_raw		= &adc1x8s102_read_raw,
>> +	.update_scan_mode	= &adc1x8s102_update_scan_mode,
>> +	.driver_module		= THIS_MODULE,
>> +};
>> +
>> +#ifdef CONFIG_ACPI
>> +typedef int (*acpi_setup_handler)(struct spi_device *,
>> +				  const struct adc1x8s102_platform_data **);
>> +
> 
> no board specific stuff here please

Needed to make it work. If there is a better file to keep that, I'll
move it.

> 
>> +static const struct adc1x8s102_platform_data int3495_platform_data = {
>> +	.ext_vin = 5000,	/* 5 V */
>> +};
>> +
>> +/* Galileo Gen 2 SPI setup */
>> +static int
>> +adc1x8s102_setup_int3495(struct spi_device *spi,
>> +			 const struct adc1x8s102_platform_data **pdata)
>> +{
>> +	struct pxa2xx_spi_chip *chip_data;
>> +
>> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data), GFP_KERNEL);
>> +	if (!chip_data)
>> +		return -ENOMEM;
>> +
>> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>> +	spi->controller_data = chip_data;
>> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>> +		 chip_data->gpio_cs);
>> +	spi_setup(spi);
>> +
>> +	*pdata = &int3495_platform_data;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
>> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
>> +#endif
>> +
>> +static int adc1x8s102_probe(struct spi_device *spi)
>> +{
>> +	const struct adc1x8s102_platform_data *pdata = spi->dev.platform_data;
>> +	struct adc1x8s102_state *st;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +#ifdef CONFIG_ACPI
>> +	if (ACPI_COMPANION(&spi->dev)) {
>> +		acpi_setup_handler setup_handler;
>> +		const struct acpi_device_id *id;
>> +
>> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi->dev);
>> +		if (!id)
>> +			return -ENODEV;
>> +
>> +		setup_handler = (acpi_setup_handler)id->driver_data;
>> +		if (setup_handler) {
>> +			ret = setup_handler(spi, &pdata);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +#endif
>> +
>> +	if (!pdata) {
>> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform data\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	st = iio_priv(indio_dev);
>> +	st->ext_vin = pdata->ext_vin;
>> +
>> +	/* Use regulator, if available. */
>> +	st->reg = devm_regulator_get(&spi->dev, "vref");
>> +	if (IS_ERR(st->reg)) {
>> +		dev_err(&spi->dev, "Cannot get 'vref' regulator\n");
>> +		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 = adc1x8s102_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(adc1x8s102_channels);
>> +	indio_dev->info = &adc1x8s102_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(__be16);
>> +	st->scan_single_xfer.cs_change = 0;
>> +
>> +	spi_message_init(&st->scan_single_msg);
>> +	spi_message_add_tail(&st->scan_single_xfer, &st->scan_single_msg);
>> +
>> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +			&adc1x8s102_trigger_handler, NULL);
>> +	if (ret)
>> +		goto error_disable_reg;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret) {
>> +		dev_err(&spi->dev,
>> +			"Failed to register IIO device\n");
>> +		goto error_cleanup_ring;
>> +	}
>> +	return 0;
>> +
>> +error_cleanup_ring:
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +error_disable_reg:
>> +	regulator_disable(st->reg);
>> +
>> +	return ret;
>> +}
>> +
>> +static int adc1x8s102_remove(struct spi_device *spi)
>> +{
>> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +	regulator_disable(st->reg);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct spi_device_id adc1x8s102_id[] = {
>> +	{ "adc1x8s102", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(spi, adc1x8s102_id);
>> +
>> +static struct spi_driver adc1x8s102_driver = {
>> +	.driver = {
>> +		.name   = "adc1x8s102",
>> +		.owner	= THIS_MODULE,
>> +		.acpi_match_table = ACPI_PTR(adc1x8s102_acpi_ids),
>> +	},
>> +	.probe		= adc1x8s102_probe,
>> +	.remove		= adc1x8s102_remove,
>> +	.id_table	= adc1x8s102_id,
>> +};
>> +module_spi_driver(adc1x8s102_driver);
>> +
>> +MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
>> +MODULE_DESCRIPTION("Texas Instruments ADC1x8S102 driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/platform_data/adc1x8s102.h b/include/linux/platform_data/adc1x8s102.h
>> new file mode 100644
>> index 000000000000..6ad753c99823
>> --- /dev/null
>> +++ b/include/linux/platform_data/adc1x8s102.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * ADC1x8S102 SPI ADC driver
>> + *
>> + * Copyright(c) 2013 Intel Corporation.
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __LINUX_PLATFORM_DATA_ADC1x8S102_H__
>> +#define __LINUX_PLATFORM_DATA_ADC1x8S102_H__
>> +
>> +/**
>> + * struct adc1x8s102_platform_data - Platform data for the adc1x8s102 ADC driver
>> + * @ext_vin: External input voltage range for all voltage input channels
>> + *	This is the voltage level of pin VA in millivolts
>> + **/
>> +struct adc1x8s102_platform_data {
>> +	u16  ext_vin;
>> +};
>> +
>> +#endif /* __LINUX_PLATFORM_DATA_ADC1x8S102_H__ */
>>
> 

Thanks,
Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

+Cc: Mika

On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-04-24 23:25, Andy Shevchenko wrote:
>> On Mon, Apr 24, 2017 at 11:32 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-04-24 22:05, Andy Shevchenko wrote:
>>>> On Mon, 2017-04-24 at 21:28 +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.

>>>>> +    chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>>>>> +    spi->controller_data = chip_data;
>>>>> +    dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>>>>> +             chip_data->gpio_cs);
>>>>> +    spi_setup(spi);
>>>>> +
>>>>> +    *pdata = &int3495_platform_data;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>
>>>> This is weird approach.
>>>
>>> Let me dig deeper if are allowed to pass a static struct here as well.
>>> But the struct is driver-defined.
>>
>> We have _DSD for ACPI, that's why I sent another email where I was
>> asking for DSDT excerpt and if it's already in the wild.
>
> I don't find any traces of "_DSD" in those DSDTs.

Yes, and looking into the DSDT you don't need them.

>>>> Moreover, please do not use platform data at all.
>>>
>>> That is just following pre-existing pattern, just look around in the
>>> iio/adc folder, not to speak of others. But I'm open to learn about any
>>> newer pattern there is.
>>
>> Unified Device Properties API is your friend. It makes driver to
>> consume resources in agnostic way.
>
> Is that ACPI-only or a generic solution?

It's generic as one may assume from the title.

> Where is a good example? Sorry,
> I still don't see how to make code out of your comments.

Mostly remove those ugly hacks and start over.

>> See above. We have nowadays mechanisms to provide device properties natively.
>> Without seeing DSDT I can't tell more.
>
> You've seen it, please tell me more now.

DSDT is wrong. So, it's another bug in the table. If you able to fix
it in your firmware, do it ASAP.

CS is a property of the host controller, not the slave devices.

Once I pointed to Mika's work for Galileo, perhaps you forgot. The
below is an example how to fix ACPI table using

https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/galileo/spi.asl

It's done for SPI1, but you easily can convert it to SPI0 and
corresponding properties.

Btw, we welcome any contribution to meta-acpi repository!

>> ...and I'm not talking about it at all.

> But I am: ACPI is not the center of the world (luckily), and this driver
> shall not be designed to only work with that way of defining resources.

Yes, that's correct.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-25 11:42, Andy Shevchenko wrote:
> +Cc: Mika
> 
> On Tue, Apr 25, 2017 at 8:44 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-04-24 23:25, Andy Shevchenko wrote:
>>> On Mon, Apr 24, 2017 at 11:32 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2017-04-24 22:05, Andy Shevchenko wrote:
>>>>> On Mon, 2017-04-24 at 21:28 +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.
> 
>>>>>> +    chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
>>>>>> +    spi->controller_data = chip_data;
>>>>>> +    dev_info(&spi->dev, "setting GPIO CS value to %d\n",
>>>>>> +             chip_data->gpio_cs);
>>>>>> +    spi_setup(spi);
>>>>>> +
>>>>>> +    *pdata = &int3495_platform_data;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>
>>>>> This is weird approach.
>>>>
>>>> Let me dig deeper if are allowed to pass a static struct here as well.
>>>> But the struct is driver-defined.
>>>
>>> We have _DSD for ACPI, that's why I sent another email where I was
>>> asking for DSDT excerpt and if it's already in the wild.
>>
>> I don't find any traces of "_DSD" in those DSDTs.
> 
> Yes, and looking into the DSDT you don't need them.
> 
>>>>> Moreover, please do not use platform data at all.
>>>>
>>>> That is just following pre-existing pattern, just look around in the
>>>> iio/adc folder, not to speak of others. But I'm open to learn about any
>>>> newer pattern there is.
>>>
>>> Unified Device Properties API is your friend. It makes driver to
>>> consume resources in agnostic way.
>>
>> Is that ACPI-only or a generic solution?
> 
> It's generic as one may assume from the title.
> 
>> Where is a good example? Sorry,
>> I still don't see how to make code out of your comments.
> 
> Mostly remove those ugly hacks and start over.

Still not a constructive answer.

> 
>>> See above. We have nowadays mechanisms to provide device properties natively.
>>> Without seeing DSDT I can't tell more.
>>
>> You've seen it, please tell me more now.
> 
> DSDT is wrong. So, it's another bug in the table. If you able to fix
> it in your firmware, do it ASAP.

Well, fixing future versions is one thing, addressing existing hardware
another...

> 
> CS is a property of the host controller, not the slave devices.
> 
> Once I pointed to Mika's work for Galileo, perhaps you forgot. The
> below is an example how to fix ACPI table using
> 
> https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-tables/samples/galileo/spi.asl
> 
> It's done for SPI1, but you easily can convert it to SPI0 and
> corresponding properties.

So that information would be picked up by the existing SPI host
controller driver, and we don't need anything beyond basic ACPI support
in this driver? That is indeed appealing. Maybe we can make the board
patch private then, until a firmware update is available. I'll split
that part off.

> 
> Btw, we welcome any contribution to meta-acpi repository!

Shipping own DSDTs is no long-term path: we would be forced to provide
separate images due to a single parameter being different in the DSDTs
of the 2020 and 2040. And you cannot provide any overlay to adjust the
table after boot, i.e. once we know on which board we are.

The dependency on meta-intel is also suboptimal (we will switch to a
long-term supported kernel source soon), but that would probably be fixable.

Thanks,
Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Tue, 2017-04-25 at 11:32 +0200, Jan Kiszka wrote:
> On 2017-04-25 09:31, Peter Meerwald-Stadler wrote:

+Cc: Mika.

> > I think board-specific stuff should not go into the driver -> DT?
> 
> Unfortunately, we only have half-baked ACPI on those boards.

That's the main problem and still no excuse for uglifying code.

> > > +
> > > +#include <linux/platform_data/adc1x8s102.h>
> > 
> > who needs platform data these days :)
> 
> Apparently, quite a few devices.

New drivers are not supposed to use platform data.

> But the situation here is:
> - Existing hardware has incomplete ACPI description that needs to be
>   augmented by software.

Yes, and this software is called DSDT table in BIOS. Linux kernel has a
support for properly formed table already for few releases.

> - I'm not aware of a complete DT specification for this device. If
>   there is any somewhere, I can happily include it.

Looking to proposed code there is only one property for it, if there is
an existing binding for the same property you may just simple re-use it.

> > > +
> > > +#define ADC1x8S102_GALILEO2_CS	8
> > 
> > this board-specific detail shouldn't be here
> 
> Where should it go then?

Obviously in (properly formed) ACPI.

> no board specific stuff here please
> 
> Needed to make it work. If there is a better file to keep that, I'll
> move it.

Ideally you need BIOS fixed for that.

Otherwise you may do a separate code which would provide CS GPIO look up
table.

Mika, what do you think about fixing this in the C code for existing
devices?

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Tue, 2017-04-25 at 12:53 +0200, Jan Kiszka wrote:
> On 2017-04-25 11:42, Andy Shevchenko wrote:


> > > Where is a good example? Sorry,
> > > I still don't see how to make code out of your comments.
> > 
> > Mostly remove those ugly hacks and start over.
> 
> Still not a constructive answer.

You just didn't get.

Provide a driver without that ugly code and then we may think how to
make it work on existing boards without too much intrusion here or
there.

> > CS is a property of the host controller, not the slave devices.
> > 
> > Once I pointed to Mika's work for Galileo, perhaps you forgot. The
> > below is an example how to fix ACPI table using
> > 
> > https://github.com/westeri/meta-acpi/blob/master/recipes-bsp/acpi-ta
> > bles/samples/galileo/spi.asl
> > 
> > It's done for SPI1, but you easily can convert it to SPI0 and
> > corresponding properties.
> 
> So that information would be picked up by the existing SPI host
> controller driver, and we don't need anything beyond basic ACPI
> support
> in this driver?

Correct!

>  That is indeed appealing. Maybe we can make the board
> patch private then, until a firmware update is available. I'll split
> that part off.

Yes, that is what I meant.

> > Btw, we welcome any contribution to meta-acpi repository!
> 
> Shipping own DSDTs is no long-term path: we would be forced to provide
> separate images due to a single parameter being different in the DSDTs
> of the 2020 and 2040. And you cannot provide any overlay to adjust the
> table after boot, i.e. once we know on which board we are.
> 
> The dependency on meta-intel is also suboptimal (we will switch to a
> long-term supported kernel source soon), but that would probably be
> fixable.

Mika, do you have anything to comment on the above?

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Mika Westerberg]

On Tue, Apr 25, 2017 at 02:27:10PM +0300, Andy Shevchenko wrote:
> > Shipping own DSDTs is no long-term path: we would be forced to provide
> > separate images due to a single parameter being different in the DSDTs
> > of the 2020 and 2040. And you cannot provide any overlay to adjust the
> > table after boot, i.e. once we know on which board we are.
> > 
> > The dependency on meta-intel is also suboptimal (we will switch to a
> > long-term supported kernel source soon), but that would probably be
> > fixable.
> 
> Mika, do you have anything to comment on the above?

You don't need to depend on meta-intel or meta-acpi. There are other
ways to add SSDT overlays than initrd. For example your boards can stick
them to EFI variable, the kernel can find them there. Then you don't
need to ship a separate image per board type.

For more information see Documentation/acpi/ssdt-overlays.txt.

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-25 13:35, Mika Westerberg wrote:
> On Tue, Apr 25, 2017 at 02:27:10PM +0300, Andy Shevchenko wrote:
>>> Shipping own DSDTs is no long-term path: we would be forced to provide
>>> separate images due to a single parameter being different in the DSDTs
>>> of the 2020 and 2040. And you cannot provide any overlay to adjust the
>>> table after boot, i.e. once we know on which board we are.
>>>
>>> The dependency on meta-intel is also suboptimal (we will switch to a
>>> long-term supported kernel source soon), but that would probably be
>>> fixable.
>>
>> Mika, do you have anything to comment on the above?
> 
> You don't need to depend on meta-intel or meta-acpi. There are other
> ways to add SSDT overlays than initrd. For example your boards can stick
> them to EFI variable, the kernel can find them there. Then you don't
> need to ship a separate image per board type.
> 
> For more information see Documentation/acpi/ssdt-overlays.txt.

I'm not ACPI guru: How do we come from a SSDT to information that is
carried in the DSDT so far? How can we overload wrong information in the
built in DSDT this way? I'm all ears if we could fix our (and also the
Galileo) quirks like that!

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Mika Westerberg]

On Tue, Apr 25, 2017 at 02:23:48PM +0300, Andy Shevchenko wrote:
> > Needed to make it work. If there is a better file to keep that, I'll
> > move it.
> 
> Ideally you need BIOS fixed for that.
> 
> Otherwise you may do a separate code which would provide CS GPIO look up
> table.
> 
> Mika, what do you think about fixing this in the C code for existing
> devices?

If nothing else helps but I would first try to explore the SSDT overlay
stuff if it can be used here instead.

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Mika Westerberg]

On Tue, Apr 25, 2017 at 02:17:23PM +0200, Jan Kiszka wrote:
> I'm not ACPI guru: How do we come from a SSDT to information that is
> carried in the DSDT so far? How can we overload wrong information in the
> built in DSDT this way? I'm all ears if we could fix our (and also the
> Galileo) quirks like that!

SSDT stands for Secondary System Description table which basically adds
stuff to DSDT (the main table). Main use for SSDTs is to add devices but
you can also amend an existing device in DSDT by adding methods and so
forth.

In case of Galileo the SPI1 host controller happens to miss _CRS method
so we can use SSDT like below to add that method there:

Scope (\_SB.PCI0.SPI1)
{
    Name (_CRS, ResourceTemplate () {
        GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
                "\\_SB.PCI0.GIP0.GPO", 0) {2} // MUX6_IO
    })

    Name (_DSD, Package () {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () {
                "cs-gpios", Package () {^SPI1, 0, 0, 0},
            },
        }
    })
}

This effectively means that once the table is parsed we find the SPI1
device with two new methods, _CRS and _DSD and the kernel is happy to
handle the rest.

Important thing here is the

	Scope (\_SB.PCI0.SPI1)

which allows us to reference an object in DSDT.

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-25 14:30, Mika Westerberg wrote:
> On Tue, Apr 25, 2017 at 02:17:23PM +0200, Jan Kiszka wrote:
>> I'm not ACPI guru: How do we come from a SSDT to information that is
>> carried in the DSDT so far? How can we overload wrong information in the
>> built in DSDT this way? I'm all ears if we could fix our (and also the
>> Galileo) quirks like that!
> 
> SSDT stands for Secondary System Description table which basically adds
> stuff to DSDT (the main table). Main use for SSDTs is to add devices but
> you can also amend an existing device in DSDT by adding methods and so
> forth.
> 
> In case of Galileo the SPI1 host controller happens to miss _CRS method
> so we can use SSDT like below to add that method there:
> 
> Scope (\_SB.PCI0.SPI1)
> {
>     Name (_CRS, ResourceTemplate () {
>         GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
>                 "\\_SB.PCI0.GIP0.GPO", 0) {2} // MUX6_IO
>     })
> 
>     Name (_DSD, Package () {
>         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>             Package () {
>                 "cs-gpios", Package () {^SPI1, 0, 0, 0},
>             },
>         }
>     })
> }
> 
> This effectively means that once the table is parsed we find the SPI1
> device with two new methods, _CRS and _DSD and the kernel is happy to
> handle the rest.
> 
> Important thing here is the
> 
> 	Scope (\_SB.PCI0.SPI1)
> 
> which allows us to reference an object in DSDT.
> 

Ah, now I recall: I think we discussed this before, but for a case were
we would need to patch an existing element that contains one wrong value
- that won't work. This case should, though. I'll give that a try.

Thanks,
Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-25 15:47, Jan Kiszka wrote:
> On 2017-04-25 14:30, Mika Westerberg wrote:
>> On Tue, Apr 25, 2017 at 02:17:23PM +0200, Jan Kiszka wrote:
>>> I'm not ACPI guru: How do we come from a SSDT to information that is
>>> carried in the DSDT so far? How can we overload wrong information in the
>>> built in DSDT this way? I'm all ears if we could fix our (and also the
>>> Galileo) quirks like that!
>>
>> SSDT stands for Secondary System Description table which basically adds
>> stuff to DSDT (the main table). Main use for SSDTs is to add devices but
>> you can also amend an existing device in DSDT by adding methods and so
>> forth.
>>
>> In case of Galileo the SPI1 host controller happens to miss _CRS method
>> so we can use SSDT like below to add that method there:
>>
>> Scope (\_SB.PCI0.SPI1)
>> {
>>     Name (_CRS, ResourceTemplate () {
>>         GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionOutputOnly,
>>                 "\\_SB.PCI0.GIP0.GPO", 0) {2} // MUX6_IO
>>     })
>>
>>     Name (_DSD, Package () {
>>         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>>         Package () {
>>             Package () {
>>                 "cs-gpios", Package () {^SPI1, 0, 0, 0},
>>             },
>>         }
>>     })
>> }
>>
>> This effectively means that once the table is parsed we find the SPI1
>> device with two new methods, _CRS and _DSD and the kernel is happy to
>> handle the rest.
>>
>> Important thing here is the
>>
>> 	Scope (\_SB.PCI0.SPI1)
>>
>> which allows us to reference an object in DSDT.
>>
> 
> Ah, now I recall: I think we discussed this before, but for a case were
> we would need to patch an existing element that contains one wrong value
> - that won't work. This case should, though. I'll give that a try.
> 

Works fine via an SSDT overlay - perfect!

Now, do you also a suggestion where to put that 5 V reference voltage
value that is hard-coded (via wiring) on the target boards for the ADC?
That is now device-specific, not a controller parameter. Is there an
ACPI-way to express such a parameter?

How would that be done for DTs?

Plan B would be hard-coding in the code for now, waiting for a second,
non-ACPI user to address it.

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jan Kiszka]

On 2017-04-25 09:31, Peter Meerwald-Stadler 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.
> 
> comments below
> 
> naming: don't use placeholders, name after one of the supported chips and 
> list them in Kconfig and the driver file
> 
> what is the difference between this chip and those supported 
> by ti-adc084s021 which was proposed by Mårten Lindahl on April 21?

I've cross-read that driver, and it looks fairly different to me.

> 
> I think board-specific stuff should not go into the driver -> DT?

Still looking for suggestions how to provide the external reference
voltage as parameter. Chip select is gone now.

Also, should I suggest a device tree binding even though I have no test
case for it? My current feeling is to better leave this exercise to the
first user on a DT platform.

[...]

>> +
>> +/*
>> + * 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 ADC1x8S102_BITS		12

[...]

>> +#define ADC1X8S102_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 = ADC1x8S102_BITS,			\
> 
> this should be different for the 128 and 108 part, shift missing
> 
> most drivers do shifting and don't use _SCALE for that purpose

What would be the difference when following your suggestion?

To my understanding, which is based on the comment above, the 108 simply
has its two least significant bits cleared, i.e. it provides a value
with the exact same scale, just with lower resolution.

> 
>> +			.storagebits = 16,				\
>> +			.endianness = IIO_BE,				\
>> +		},							\
>> +	}

Jan

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

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Mika Westerberg]

On Tue, Apr 25, 2017 at 06:12:12PM +0200, Jan Kiszka wrote:
> Now, do you also a suggestion where to put that 5 V reference voltage
> value that is hard-coded (via wiring) on the target boards for the ADC?
> That is now device-specific, not a controller parameter. Is there an
> ACPI-way to express such a parameter?

You may put it into device property using _DSD (as long as you don't try
to represent regulators or so).

> How would that be done for DTs?

I don't know but you may look under Documentation/devicetree/bindings if
there is anything.

> Plan B would be hard-coding in the code for now, waiting for a second,
> non-ACPI user to address it.

Sounds reasonable.

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Andy Shevchenko]

On Wed, Apr 26, 2017 at 8:37 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-04-25 09:31, Peter Meerwald-Stadler wrote:

>> I think board-specific stuff should not go into the driver -> DT?
>
> Still looking for suggestions how to provide the external reference
> voltage as parameter. Chip select is gone now.

Unified Device Properties API.

Just
ret = device_property_read_u16();
if (ret)
 ...use default...

> Also, should I suggest a device tree binding even though I have no test
> case for it? My current feeling is to better leave this exercise to the
> first user on a DT platform.

But I prefer this one. One problem at a time.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jonathan Cameron]

On 26/04/17 10:01, Mika Westerberg wrote:
> On Tue, Apr 25, 2017 at 06:12:12PM +0200, Jan Kiszka wrote:
>> Now, do you also a suggestion where to put that 5 V reference voltage
>> value that is hard-coded (via wiring) on the target boards for the ADC?
>> That is now device-specific, not a controller parameter. Is there an
>> ACPI-way to express such a parameter?
> 
> You may put it into device property using _DSD (as long as you don't try
> to represent regulators or so).
> 
>> How would that be done for DTs?
> 
> I don't know but you may look under Documentation/devicetree/bindings if
> there is anything.
Under DT it would be done using a fixed voltage regulator.
> 
>> Plan B would be hard-coding in the code for now, waiting for a second,
>> non-ACPI user to address it.
> 
> Sounds reasonable.
Somewhat of a pain to basically use a random value as the default going
forward.  Presumably this isn't the first ever ACPI table to need to
tell use about a reference voltage...

Mark, seen anything similar?

I see https://www.kernel.org/doc/Documentation/arm64/arm-acpi.txt suggests
that mapping to regulators isn't expected to ever happen...

Jonathan
> --
> 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] iio: adc: Add support for TI ADC1x8s102

[Jonathan Cameron]

On 27/04/17 07:01, Jonathan Cameron wrote:
> On 26/04/17 10:01, Mika Westerberg wrote:
>> On Tue, Apr 25, 2017 at 06:12:12PM +0200, Jan Kiszka wrote:
>>> Now, do you also a suggestion where to put that 5 V reference voltage
>>> value that is hard-coded (via wiring) on the target boards for the ADC?
>>> That is now device-specific, not a controller parameter. Is there an
>>> ACPI-way to express such a parameter?
>>
>> You may put it into device property using _DSD (as long as you don't try
>> to represent regulators or so).
>>
>>> How would that be done for DTs?
>>
>> I don't know but you may look under Documentation/devicetree/bindings if
>> there is anything.
> Under DT it would be done using a fixed voltage regulator.
Though having actually looked at the driver, you presumably already know this
given you are requesting an appropriate regulator...
>>
>>> Plan B would be hard-coding in the code for now, waiting for a second,
>>> non-ACPI user to address it.
>>
>> Sounds reasonable.
> Somewhat of a pain to basically use a random value as the default going
> forward.  Presumably this isn't the first ever ACPI table to need to
> tell use about a reference voltage...
> 
> Mark, seen anything similar?
> 
> I see https://www.kernel.org/doc/Documentation/arm64/arm-acpi.txt suggests
> that mapping to regulators isn't expected to ever happen...
> 
> Jonathan
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jonathan Cameron]

On 24/04/17 20:28, 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.
> 
> 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 more bits from me.

Jonathan
> ---
>  drivers/iio/adc/Kconfig                  |  12 +
>  drivers/iio/adc/Makefile                 |   1 +
>  drivers/iio/adc/ti-adc1x8s102.c          | 408 +++++++++++++++++++++++++++++++
>  include/linux/platform_data/adc1x8s102.h |  28 +++
>  4 files changed, 449 insertions(+)
>  create mode 100644 drivers/iio/adc/ti-adc1x8s102.c
>  create mode 100644 include/linux/platform_data/adc1x8s102.h
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7adbce9..edb7254a648c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -582,6 +582,18 @@ config TI_ADC128S052
>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-adc128s052.
>  
> +config TI_ADC1x8S102
> +	tristate "Texas Instruments ADC1x8S102 driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Texas Instruments ADC1x8S102 ADC.
> +	  Provides direct access via sysfs.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called ti-adc1x8s102
> +
>  config TI_ADC161S626
>  	tristate "Texas Instruments ADC161S626 1-channel differential ADC"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d0012620cd1c..d5d913bc1263 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
> +obj-$(CONFIG_TI_ADC1x8S102) += ti-adc1x8s102.o
>  obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
>  obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
>  obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
> diff --git a/drivers/iio/adc/ti-adc1x8s102.c b/drivers/iio/adc/ti-adc1x8s102.c
> new file mode 100644
> index 000000000000..4f94c371489d
> --- /dev/null
> +++ b/drivers/iio/adc/ti-adc1x8s102.c
> @@ -0,0 +1,408 @@
> +/*
> + * TI ADC1x8S102 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 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/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/spi/spi.h>
> +
> +#include <linux/platform_data/adc1x8s102.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +#include <linux/property.h>
> +#include <linux/gpio.h>
> +
> +#include <linux/spi/pxa2xx_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 ADC1x8S102_BITS		12
> +#define ADC1x8S102_MAX_CHANNELS	8
> +
> +/* 16-bit SPI command format:
> + *   [15:14] Ignored
> + *   [13:11] 3-bit channel address
> + *   [10:0]  Ignored
> + */
> +#define ADC1x8S102_CMD(ch)		(((ch) << (8)) << (3))
> +
> +/*
> + * 16-bit SPI response format:
> + *   [15:12] Zeros
> + *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
> + */
> +#define ADC1x8S102_RES_DATA(res)	(res & ((1 << ADC1x8S102_BITS) - 1))
> +
> +#define ADC1x8S102_GALILEO2_CS	8
> +
> +struct adc1x8s102_state {
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	u16				ext_vin;
> +	/* 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];
> +};
> +
> +#define ADC1X8S102_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 = ADC1x8S102_BITS,			\
> +			.storagebits = 16,				\
> +			.endianness = IIO_BE,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec adc1x8s102_channels[] = {
> +	ADC1X8S102_V_CHAN(0),
> +	ADC1X8S102_V_CHAN(1),
> +	ADC1X8S102_V_CHAN(2),
> +	ADC1X8S102_V_CHAN(3),
> +	ADC1X8S102_V_CHAN(4),
> +	ADC1X8S102_V_CHAN(5),
> +	ADC1X8S102_V_CHAN(6),
> +	ADC1X8S102_V_CHAN(7),
> +	IIO_CHAN_SOFT_TIMESTAMP(8),
> +};
> +
> +static int adc1x8s102_update_scan_mode(struct iio_dev *indio_dev,
> +		unsigned long const *active_scan_mask)
> +{
> +	struct adc1x8s102_state *st;
> +	int i, j;
> +
> +	st = iio_priv(indio_dev);
> +
> +	/* Fill in the first x shorts of tx_buf with the number of channels
> +	 * enabled for sampling by the triggered buffer
> +	 */
> +	for (i = 0, j = 0; i < ADC1x8S102_MAX_CHANNELS; i++) {
> +		if (test_bit(i, active_scan_mask)) {
> +			st->tx_buf[j] = cpu_to_be16(ADC1x8S102_CMD(i));
> +			j++;
> +		}
> +	}
> +	/* One dummy command added, to clock in the last response */
> +	st->tx_buf[j] = 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 = (j + 1) * sizeof(__be16);
> +
> +	spi_message_init(&st->ring_msg);
> +	spi_message_add_tail(&st->ring_xfer, &st->ring_msg);
The init with transfers version to do this in one line please.
> +
> +	return 0;
> +}
> +
> +static irqreturn_t adc1x8s102_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev;
> +	struct adc1x8s102_state *st;
> +	s64 time_ns = 0;
> +	int b_sent;
> +
> +	indio_dev = pf->indio_dev;
> +	st = iio_priv(indio_dev);
> +
> +	b_sent = spi_sync(st->spi, &st->ring_msg);
I'd handle the error path with a goto rather than indenting the
main flow like this.
> +	if (b_sent == 0) {
> +		if (indio_dev->scan_timestamp) {
> +			time_ns = iio_get_time_ns(indio_dev);
> +			memcpy((u8 *)st->rx_buf + st->ring_xfer.len, &time_ns,
> +			       sizeof(time_ns));
> +		}
> +
> +		/* Skip the dummy response in the first slot */
> +		iio_push_to_buffers(indio_dev, (u8 *)&st->rx_buf[1]);
It's already been pointed out but iio_push_to_buffers_with_timstamp handles the
above for you.
> +	}
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adc1x8s102_scan_direct(struct adc1x8s102_state *st, unsigned int ch)
> +{
> +	int ret;
> +
> +	st->tx_buf[0] = cpu_to_be16(ADC1x8S102_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 adc1x8s102_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +	int ret;
> +	struct adc1x8s102_state *st;
> +
> +	st = iio_priv(indio_dev);
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
the direct mode claim stuff please.
> +		mutex_lock(&indio_dev->mlock);
> +		if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> +			ret = -EBUSY;
> +			dev_warn(&st->spi->dev,
> +			 "indio_dev->currentmode is INDIO_BUFFER_TRIGGERED\n");
> +		} else {
> +			ret = adc1x8s102_scan_direct(st, chan->address);
> +		}
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (ret < 0)
> +			return ret;
> +		*val = ADC1x8S102_RES_DATA(ret);
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (st->reg)
> +				*val = regulator_get_voltage(st->reg) / 1000;
> +			else
> +				*val = st->ext_vin;
> +
> +			*val2 = chan->scan_type.realbits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		default:
> +			dev_warn(&st->spi->dev,
> +				 "Invalid channel type %u for channel %d\n",
> +				 chan->type, chan->channel);
> +			return -EINVAL;
> +		}
> +	default:
> +		dev_warn(&st->spi->dev, "Invalid IIO_CHAN_INFO: %lu\n", m);
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info adc1x8s102_info = {
> +	.read_raw		= &adc1x8s102_read_raw,
> +	.update_scan_mode	= &adc1x8s102_update_scan_mode,
> +	.driver_module		= THIS_MODULE,
> +};
> +
> +#ifdef CONFIG_ACPI
> +typedef int (*acpi_setup_handler)(struct spi_device *,
> +				  const struct adc1x8s102_platform_data **);
> +
> +static const struct adc1x8s102_platform_data int3495_platform_data = {
> +	.ext_vin = 5000,	/* 5 V */
> +};
> +
> +/* Galileo Gen 2 SPI setup */
> +static int
> +adc1x8s102_setup_int3495(struct spi_device *spi,
> +			 const struct adc1x8s102_platform_data **pdata)
> +{
> +	struct pxa2xx_spi_chip *chip_data;
Hmm. As has been observed this stuff doesn't belong in a driver...
> +
> +	chip_data = devm_kzalloc(&spi->dev, sizeof(*chip_data), GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;
> +
> +	chip_data->gpio_cs = ADC1x8S102_GALILEO2_CS;
> +	spi->controller_data = chip_data;
> +	dev_info(&spi->dev, "setting GPIO CS value to %d\n",
> +		 chip_data->gpio_cs);
> +	spi_setup(spi);
> +
> +	*pdata = &int3495_platform_data;
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id adc1x8s102_acpi_ids[] = {
> +	{ "INT3495",  (kernel_ulong_t)&adc1x8s102_setup_int3495 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, adc1x8s102_acpi_ids);
> +#endif
> +
> +static int adc1x8s102_probe(struct spi_device *spi)
> +{
> +	const struct adc1x8s102_platform_data *pdata = spi->dev.platform_data;
> +	struct adc1x8s102_state *st;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +#ifdef CONFIG_ACPI
> +	if (ACPI_COMPANION(&spi->dev)) {
> +		acpi_setup_handler setup_handler;
> +		const struct acpi_device_id *id;
> +
> +		id = acpi_match_device(adc1x8s102_acpi_ids, &spi->dev);
> +		if (!id)
> +			return -ENODEV;
> +
> +		setup_handler = (acpi_setup_handler)id->driver_data;
> +		if (setup_handler) {
> +			ret = setup_handler(spi, &pdata);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +#endif
> +
> +	if (!pdata) {
> +		dev_err(&spi->dev, "Cannot get adc1x8s102 platform data\n");
> +		return -ENODEV;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->ext_vin = pdata->ext_vin;
> +
> +	/* Use regulator, if available. */
> +	st->reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(st->reg)) {
> +		dev_err(&spi->dev, "Cannot get 'vref' regulator\n");
> +		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 = adc1x8s102_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adc1x8s102_channels);
> +	indio_dev->info = &adc1x8s102_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(__be16);
would prefer this as 2* sizeof(st->tx_buf[0]) or something like that.

> +	st->scan_single_xfer.cs_change = 0;
no need to set cs_change to the default..
> +
> +	spi_message_init(&st->scan_single_msg);
> +	spi_message_add_tail(&st->scan_single_xfer, &st->scan_single_msg);
spi_message_init_with_transfers
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +			&adc1x8s102_trigger_handler, NULL);
> +	if (ret)
> +		goto error_disable_reg;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev,
> +			"Failed to register IIO device\n");
> +		goto error_cleanup_ring;
> +	}
> +	return 0;
> +
> +error_cleanup_ring:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +error_disable_reg:
> +	regulator_disable(st->reg);
> +
> +	return ret;
> +}
> +
> +static int adc1x8s102_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct adc1x8s102_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id adc1x8s102_id[] = {
> +	{ "adc1x8s102", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, adc1x8s102_id);
> +
> +static struct spi_driver adc1x8s102_driver = {
> +	.driver = {
> +		.name   = "adc1x8s102",
> +		.owner	= THIS_MODULE,
> +		.acpi_match_table = ACPI_PTR(adc1x8s102_acpi_ids),
> +	},
> +	.probe		= adc1x8s102_probe,
> +	.remove		= adc1x8s102_remove,
> +	.id_table	= adc1x8s102_id,
> +};
> +module_spi_driver(adc1x8s102_driver);
> +
> +MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADC1x8S102 driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/adc1x8s102.h b/include/linux/platform_data/adc1x8s102.h
> new file mode 100644
> index 000000000000..6ad753c99823
> --- /dev/null
> +++ b/include/linux/platform_data/adc1x8s102.h
> @@ -0,0 +1,28 @@
> +/*
> + * ADC1x8S102 SPI ADC driver
> + *
> + * Copyright(c) 2013 Intel Corporation.
> + *
> + * 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.
> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_ADC1x8S102_H__
> +#define __LINUX_PLATFORM_DATA_ADC1x8S102_H__
> +
> +/**
> + * struct adc1x8s102_platform_data - Platform data for the adc1x8s102 ADC driver
> + * @ext_vin: External input voltage range for all voltage input channels
> + *	This is the voltage level of pin VA in millivolts
> + **/
> +struct adc1x8s102_platform_data {
> +	u16  ext_vin;
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_ADC1x8S102_H__ */
> 

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Thu, Apr 27, 2017 at 07:01:07AM +0100, Jonathan Cameron wrote:

> Somewhat of a pain to basically use a random value as the default going
> forward.  Presumably this isn't the first ever ACPI table to need to
> tell use about a reference voltage...

> Mark, seen anything similar?

> I see https://www.kernel.org/doc/Documentation/arm64/arm-acpi.txt suggests
> that mapping to regulators isn't expected to ever happen...

There's multiple different camps trying to use ACPI for very different
things.  There's the server people on both x86 and ARM who want to use
standard ACPI and nothing but with the power management all hidden in
the AML but there's also the embedded x86 people who have the same needs
as DT platforms but find themselves unable to use DT so have to map all
the DT support into ACPI.  This has been accepted in areas that clearly
don't overlap with areas where there are existing ACPI bindings for
things, power management is one area where there are clear bindings
though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Jonathan Cameron]

On 19/05/17 17:01, Mark Brown wrote:
> On Thu, Apr 27, 2017 at 07:01:07AM +0100, Jonathan Cameron wrote:
> 
>> Somewhat of a pain to basically use a random value as the default going
>> forward.  Presumably this isn't the first ever ACPI table to need to
>> tell use about a reference voltage...
> 
>> Mark, seen anything similar?
> 
>> I see https://www.kernel.org/doc/Documentation/arm64/arm-acpi.txt suggests
>> that mapping to regulators isn't expected to ever happen...
> 
> There's multiple different camps trying to use ACPI for very different
> things.  There's the server people on both x86 and ARM who want to use
> standard ACPI and nothing but with the power management all hidden in
> the AML but there's also the embedded x86 people who have the same needs
> as DT platforms but find themselves unable to use DT so have to map all
> the DT support into ACPI.  This has been accepted in areas that clearly
> don't overlap with areas where there are existing ACPI bindings for
> things, power management is one area where there are clear bindings
> though.
Thanks Mark,

Just to clarify what do we do here, where a regulator is providing a
reference voltage and there appears to be no information on it in ACPI?

Right now we are just going with a fixed value that matches the board
someone has, but that's hardly sustainable.

Jonathan
> 

Re: [PATCH] iio: adc: Add support for TI ADC1x8s102

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

On Sat, May 20, 2017 at 05:26:30PM +0100, Jonathan Cameron wrote:

> Just to clarify what do we do here, where a regulator is providing a
> reference voltage and there appears to be no information on it in ACPI?

> Right now we are just going with a fixed value that matches the board
> someone has, but that's hardly sustainable.

Yes, the ACPI people need to come up with some standard regulator
bindings for this sort of application.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]