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

[PATCH v2] 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>
---

Changes in v2:
- dropped platform code, we are heading for ACPI descriptions
  (SSDT fixup first, then firmware update)
- renamed to ADC108S102
- addressed (hopefully) all other smaller review remarks

 drivers/iio/adc/Kconfig         |  12 ++
 drivers/iio/adc/Makefile        |   1 +
 drivers/iio/adc/ti-adc108s102.c | 342 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 355 insertions(+)
 create mode 100644 drivers/iio/adc/ti-adc108s102.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index dedae7adbce9..8b7dae5604f2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -572,6 +572,18 @@ config TI_ADC12138
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-adc12138.
 
+config TI_ADC108S102
+	tristate "Texas Instruments ADC108S102 and ADC128S102 driver"
+	depends on SPI
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for Texas Instruments ADC108S102 and
+	  ADC128S102 ADC.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called ti-adc108s102.
+
 config TI_ADC128S052
 	tristate "Texas Instruments ADC128S052/ADC122S021/ADC124S021"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d0012620cd1c..e6c20fe3d4eb 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_STM32_ADC) += stm32-adc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
+obj-$(CONFIG_TI_ADC108S102) += ti-adc108s102.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
new file mode 100644
index 000000000000..1d6b27d66d7c
--- /dev/null
+++ b/drivers/iio/adc/ti-adc108s102.c
@@ -0,0 +1,342 @@
+/*
+ * TI ADC108S102 SPI ADC driver
+ *
+ * Copyright (c) 2013-2015 Intel Corporation.
+ * Copyright (c) 2017 Siemens AG
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * This IIO device driver is designed to work with the following
+ * analog to digital converters from Texas Instruments:
+ *  ADC108S102
+ *  ADC128S102
+ * The communication with ADC chip is via the SPI bus (mode 3).
+ */
+
+#include <linux/acpi.h>
+#include <linux/gpio.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/types.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+/*
+ * Defining the ADC resolution being 12 bits, we can use the same driver for
+ * both ADC108S102 (10 bits resolution) and ADC128S102 (12 bits resolution)
+ * chips. The ADC108S102 effectively returns a 12-bit result with the 2
+ * least-significant bits unset.
+ */
+#define ADC108S102_BITS		12
+#define ADC108S102_MAX_CHANNELS	8
+
+/*
+ * 16-bit SPI command format:
+ *   [15:14] Ignored
+ *   [13:11] 3-bit channel address
+ *   [10:0]  Ignored
+ */
+#define ADC108S102_CMD(ch)		((u16)(ch) << 11)
+
+/*
+ * 16-bit SPI response format:
+ *   [15:12] Zeros
+ *   [11:0]  12-bit ADC sample (for ADC108S102, [1:0] will always be 0).
+ */
+#define ADC108S102_RES_DATA(res)	((u16)res & GENMASK(11, 0))
+
+struct adc108s102_state {
+	struct spi_device		*spi;
+	struct regulator		*reg;
+	u16				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:
+	 *  tx_buf: |C0|C1|C2|C3|C4|C5|C6|C7|XX|
+	 *  rx_buf: |XX|R0|R1|R2|R3|R4|R5|R6|R7|tt|tt|tt|tt|
+	 *
+	 *  tx_buf: 8 channel read commands, plus 1 dummy command
+	 *  rx_buf: 1 dummy response, 8 channel responses, plus 64-bit timestamp
+	 */
+	__be16				rx_buf[13] ____cacheline_aligned;
+	__be16				tx_buf[9] ____cacheline_aligned;
+};
+
+#define ADC108S102_V_CHAN(index)					\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = index,					\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		\
+			BIT(IIO_CHAN_INFO_SCALE),			\
+		.address = index,					\
+		.scan_index = index,					\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = ADC108S102_BITS,			\
+			.storagebits = 16,				\
+			.endianness = IIO_BE,				\
+		},							\
+	}
+
+static const struct iio_chan_spec adc108s102_channels[] = {
+	ADC108S102_V_CHAN(0),
+	ADC108S102_V_CHAN(1),
+	ADC108S102_V_CHAN(2),
+	ADC108S102_V_CHAN(3),
+	ADC108S102_V_CHAN(4),
+	ADC108S102_V_CHAN(5),
+	ADC108S102_V_CHAN(6),
+	ADC108S102_V_CHAN(7),
+	IIO_CHAN_SOFT_TIMESTAMP(8),
+};
+
+static int adc108s102_update_scan_mode(struct iio_dev *indio_dev,
+		unsigned long const *active_scan_mask)
+{
+	struct adc108s102_state *st;
+	unsigned int bit, cmds;
+
+	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
+	 */
+	cmds = 0;
+	for_each_set_bit(bit, active_scan_mask, ADC108S102_MAX_CHANNELS)
+		st->tx_buf[cmds++] = cpu_to_be16(ADC108S102_CMD(bit));
+
+	/* One dummy command added, to clock in the last response */
+	st->tx_buf[cmds++] = 0x00;
+
+	/* build SPI ring message */
+	st->ring_xfer.tx_buf = &st->tx_buf[0];
+	st->ring_xfer.rx_buf = &st->rx_buf[0];
+	st->ring_xfer.len = cmds * sizeof(st->tx_buf[0]);
+
+	spi_message_init_with_transfers(&st->ring_msg, &st->ring_xfer, 1);
+
+	return 0;
+}
+
+static irqreturn_t adc108s102_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev;
+	struct adc108s102_state *st;
+	int ret;
+
+	indio_dev = pf->indio_dev;
+	st = iio_priv(indio_dev);
+
+	ret = spi_sync(st->spi, &st->ring_msg);
+	if (ret < 0)
+		goto out;
+
+	/* Skip the dummy response in the first slot */
+	iio_push_to_buffers_with_timestamp(indio_dev,
+					   (u8 *)&st->rx_buf[1],
+					   iio_get_time_ns(indio_dev));
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int adc108s102_scan_direct(struct adc108s102_state *st, unsigned int ch)
+{
+	int ret;
+
+	st->tx_buf[0] = cpu_to_be16(ADC108S102_CMD(ch));
+	ret = spi_sync(st->spi, &st->scan_single_msg);
+	if (ret)
+		return ret;
+
+	/* Skip the dummy response in the first slot */
+	return be16_to_cpu(st->rx_buf[1]);
+}
+
+static int adc108s102_read_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int *val, int *val2, long m)
+{
+	int ret;
+	struct adc108s102_state *st;
+
+	st = iio_priv(indio_dev);
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = adc108s102_scan_direct(st, chan->address);
+
+		iio_device_release_direct_mode(indio_dev);
+
+		if (ret < 0)
+			return ret;
+
+		*val = ADC108S102_RES_DATA(ret);
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			if (st->reg)
+				*val = regulator_get_voltage(st->reg) / 1000;
+			else
+				*val = st->ext_vin_mv;
+
+			*val2 = chan->scan_type.realbits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info adc108s102_info = {
+	.read_raw		= &adc108s102_read_raw,
+	.update_scan_mode	= &adc108s102_update_scan_mode,
+	.driver_module		= THIS_MODULE,
+};
+
+static int adc108s102_probe(struct spi_device *spi)
+{
+	struct adc108s102_state *st;
+	struct iio_dev *indio_dev;
+	u32 val;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
+	if (ret < 0) {
+		dev_err(&spi->dev,
+			"Missing ext-vin-microvolt device property\n");
+		return -ENODEV;
+	}
+	st->ext_vin_mv = (u16)val;
+
+	/* 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 = adc108s102_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adc108s102_channels);
+	indio_dev->info = &adc108s102_info;
+
+	/* Setup default message */
+	st->scan_single_xfer.tx_buf = st->tx_buf;
+	st->scan_single_xfer.rx_buf = st->rx_buf;
+	st->scan_single_xfer.len = 2 * sizeof(st->tx_buf[0]);
+
+	spi_message_init_with_transfers(&st->scan_single_msg,
+					&st->scan_single_xfer, 1);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+			&adc108s102_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 adc108s102_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct adc108s102_state *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	regulator_disable(st->reg);
+
+	return 0;
+}
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id adc108s102_acpi_ids[] = {
+	{ "INT3495", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, adc108s102_acpi_ids);
+#endif
+
+static const struct spi_device_id adc108s102_id[] = {
+	{ "adc108s102", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, adc108s102_id);
+
+static struct spi_driver adc108s102_driver = {
+	.driver = {
+		.name   = "adc108s102",
+		.owner	= THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
+	},
+	.probe		= adc108s102_probe,
+	.remove		= adc108s102_remove,
+	.id_table	= adc108s102_id,
+};
+module_spi_driver(adc108s102_driver);
+
+MODULE_AUTHOR("Bogdan Pricop <bogdan.pricop@emutex.com>");
+MODULE_DESCRIPTION("Texas Instruments ADC108S102 and ADC128S102 driver");
+MODULE_LICENSE("GPL v2");
-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Andy Shevchenko]

On Tue, May 2, 2017 at 11:02 PM, Jan Kiszka <jan.kiszka@siemens.com> 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>.

There are several nitpicks, and one concern about regulator.
After addressing at least the latter

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> +#include <linux/acpi.h>

> +#include <linux/gpio.h>

Redundant I beleive.

> +static int adc108s102_update_scan_mode(struct iio_dev *indio_dev,
> +               unsigned long const *active_scan_mask)
> +{
> +       struct adc108s102_state *st;
> +       unsigned int bit, cmds;

> +
> +       st = iio_priv(indio_dev);
> +

I think it's a quite good pattern to assign such variables above at
definition block.
This also would be done in several functions below (except ->probe() call)

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

Usually in multiline comments even for one sentence we use period at the end.

> +       cmds = 0;
> +       for_each_set_bit(bit, active_scan_mask, ADC108S102_MAX_CHANNELS)
> +               st->tx_buf[cmds++] = cpu_to_be16(ADC108S102_CMD(bit));
> +
> +       /* One dummy command added, to clock in the last response */
> +       st->tx_buf[cmds++] = 0x00;
> +
> +       /* build SPI ring message */

> +       st->ring_xfer.tx_buf = &st->tx_buf[0];
> +       st->ring_xfer.rx_buf = &st->rx_buf[0];

&pointer[0] -> pointer

> +       st->ring_xfer.len = cmds * sizeof(st->tx_buf[0]);
> +
> +       spi_message_init_with_transfers(&st->ring_msg, &st->ring_xfer, 1);
> +
> +       return 0;
> +}
> +
> +static irqreturn_t adc108s102_trigger_handler(int irq, void *p)
> +{
> +       struct iio_poll_func *pf = p;
> +       struct iio_dev *indio_dev;
> +       struct adc108s102_state *st;
> +       int ret;
> +

> +       indio_dev = pf->indio_dev;
> +       st = iio_priv(indio_dev);

Assign them above.

> +out:

I would name it out_notify.

> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}

> +static int adc108s102_read_raw(struct iio_dev *indio_dev,
> +                              struct iio_chan_spec const *chan,
> +                              int *val, int *val2, long m)
> +{

> +       int ret;
> +       struct adc108s102_state *st;

Reversed tree order.

> +
> +       st = iio_priv(indio_dev);
> +

Assign it above.

> +       switch (m) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = iio_device_claim_direct_mode(indio_dev);
> +               if (ret)
> +                       return ret;
> +
> +               ret = adc108s102_scan_direct(st, chan->address);
> +
> +               iio_device_release_direct_mode(indio_dev);
> +
> +               if (ret < 0)
> +                       return ret;
> +
> +               *val = ADC108S102_RES_DATA(ret);
> +
> +               return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_SCALE:
> +               switch (chan->type) {
> +               case IIO_VOLTAGE:
> +                       if (st->reg)
> +                               *val = regulator_get_voltage(st->reg) / 1000;
> +                       else
> +                               *val = st->ext_vin_mv;
> +
> +                       *val2 = chan->scan_type.realbits;
> +                       return IIO_VAL_FRACTIONAL_LOG2;

> +               default:
> +                       return -EINVAL;

Switch-case for one case? Are you expecting more in the future?
Here I have no strong opinion, so, leave what you / maintainers prefer.

> +               }
> +       default:
> +               return -EINVAL;
> +       }
> +}

> +static int adc108s102_probe(struct spi_device *spi)
> +{
> +       struct adc108s102_state *st;
> +       struct iio_dev *indio_dev;
> +       u32 val;
> +       int ret;
> +
> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       st = iio_priv(indio_dev);
> +

> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);

Why not to read u16 here?


> +       if (ret < 0) {
> +               dev_err(&spi->dev,
> +                       "Missing ext-vin-microvolt device property\n");
> +               return -ENODEV;
> +       }

> +       st->ext_vin_mv = (u16)val;

Casting is not needed.

> +
> +       /* Use regulator, if available. */
> +       st->reg = devm_regulator_get(&spi->dev, "vref");

Here is most important concern, how you get this regulator in ACPI
case in the future? To me it sounds more like
devm_regulator_get_optional();

> +       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 = adc108s102_channels;
> +       indio_dev->num_channels = ARRAY_SIZE(adc108s102_channels);
> +       indio_dev->info = &adc108s102_info;
> +
> +       /* Setup default message */
> +       st->scan_single_xfer.tx_buf = st->tx_buf;
> +       st->scan_single_xfer.rx_buf = st->rx_buf;
> +       st->scan_single_xfer.len = 2 * sizeof(st->tx_buf[0]);
> +
> +       spi_message_init_with_transfers(&st->scan_single_msg,
> +                                       &st->scan_single_xfer, 1);
> +

> +       ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +                       &adc108s102_trigger_handler, NULL);
> +       if (ret)
> +               goto error_disable_reg;
> +
> +       ret = iio_device_register(indio_dev);

I suppose some of above is already under devres API (implicitly when
devm_iio_..._alloc() is in use)


> +       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);

Do you need it when devm_ used?

> +error_disable_reg:
> +       regulator_disable(st->reg);

Ditto.

> +
> +       return ret;
> +}
> +
> +static int adc108s102_remove(struct spi_device *spi)
> +{
> +       struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +       struct adc108s102_state *st = iio_priv(indio_dev);
> +

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

Ditto.

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

> +static struct spi_driver adc108s102_driver = {
> +       .driver = {
> +               .name   = "adc108s102",

> +               .owner  = THIS_MODULE,

This is redundant I'm pretty sure.

> +               .acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
> +       },
> +       .probe          = adc108s102_probe,
> +       .remove         = adc108s102_remove,
> +       .id_table       = adc108s102_id,
> +};

-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 2017-05-02 22:53, Andy Shevchenko wrote:
> On Tue, May 2, 2017 at 11:02 PM, Jan Kiszka <jan.kiszka@siemens.com> 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>.
> 
> There are several nitpicks, and one concern about regulator.
> After addressing at least the latter
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
>> +#include <linux/acpi.h>
> 
>> +#include <linux/gpio.h>
> 
> Redundant I beleive.
> 
>> +static int adc108s102_update_scan_mode(struct iio_dev *indio_dev,
>> +               unsigned long const *active_scan_mask)
>> +{
>> +       struct adc108s102_state *st;
>> +       unsigned int bit, cmds;
> 
>> +
>> +       st = iio_priv(indio_dev);
>> +
> 
> I think it's a quite good pattern to assign such variables above at
> definition block.
> This also would be done in several functions below (except ->probe() call)
> 
>> +       /*
>> +        * Fill in the first x shorts of tx_buf with the number of channels
>> +        * enabled for sampling by the triggered buffer
>> +        */
> 
> Usually in multiline comments even for one sentence we use period at the end.
> 
>> +       cmds = 0;
>> +       for_each_set_bit(bit, active_scan_mask, ADC108S102_MAX_CHANNELS)
>> +               st->tx_buf[cmds++] = cpu_to_be16(ADC108S102_CMD(bit));
>> +
>> +       /* One dummy command added, to clock in the last response */
>> +       st->tx_buf[cmds++] = 0x00;
>> +
>> +       /* build SPI ring message */
> 
>> +       st->ring_xfer.tx_buf = &st->tx_buf[0];
>> +       st->ring_xfer.rx_buf = &st->rx_buf[0];
> 
> &pointer[0] -> pointer

This is following patterns in other drivers, expressing you want the
first element here. I'll keep it.

> 
>> +       st->ring_xfer.len = cmds * sizeof(st->tx_buf[0]);
>> +
>> +       spi_message_init_with_transfers(&st->ring_msg, &st->ring_xfer, 1);
>> +
>> +       return 0;
>> +}
>> +
>> +static irqreturn_t adc108s102_trigger_handler(int irq, void *p)
>> +{
>> +       struct iio_poll_func *pf = p;
>> +       struct iio_dev *indio_dev;
>> +       struct adc108s102_state *st;
>> +       int ret;
>> +
> 
>> +       indio_dev = pf->indio_dev;
>> +       st = iio_priv(indio_dev);
> 
> Assign them above.
> 
>> +out:
> 
> I would name it out_notify.
> 
>> +       iio_trigger_notify_done(indio_dev->trig);
>> +
>> +       return IRQ_HANDLED;
>> +}
> 
>> +static int adc108s102_read_raw(struct iio_dev *indio_dev,
>> +                              struct iio_chan_spec const *chan,
>> +                              int *val, int *val2, long m)
>> +{
> 
>> +       int ret;
>> +       struct adc108s102_state *st;
> 
> Reversed tree order.
> 
>> +
>> +       st = iio_priv(indio_dev);
>> +
> 
> Assign it above.
> 
>> +       switch (m) {
>> +       case IIO_CHAN_INFO_RAW:
>> +               ret = iio_device_claim_direct_mode(indio_dev);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               ret = adc108s102_scan_direct(st, chan->address);
>> +
>> +               iio_device_release_direct_mode(indio_dev);
>> +
>> +               if (ret < 0)
>> +                       return ret;
>> +
>> +               *val = ADC108S102_RES_DATA(ret);
>> +
>> +               return IIO_VAL_INT;
>> +       case IIO_CHAN_INFO_SCALE:
>> +               switch (chan->type) {
>> +               case IIO_VOLTAGE:
>> +                       if (st->reg)
>> +                               *val = regulator_get_voltage(st->reg) / 1000;
>> +                       else
>> +                               *val = st->ext_vin_mv;
>> +
>> +                       *val2 = chan->scan_type.realbits;
>> +                       return IIO_VAL_FRACTIONAL_LOG2;
> 
>> +               default:
>> +                       return -EINVAL;
> 
> Switch-case for one case? Are you expecting more in the future?
> Here I have no strong opinion, so, leave what you / maintainers prefer.

I've no expectations on the code as I didn't write it in the first
place. There are both patterns around, but let's got for the more
compact if-else.

> 
>> +               }
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +}
> 
>> +static int adc108s102_probe(struct spi_device *spi)
>> +{
>> +       struct adc108s102_state *st;
>> +       struct iio_dev *indio_dev;
>> +       u32 val;
>> +       int ret;
>> +
>> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>> +       if (!indio_dev)
>> +               return -ENOMEM;
>> +
>> +       st = iio_priv(indio_dev);
>> +
> 
>> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
> 
> Why not to read u16 here?
> 

Can I read a property with arbitrary width? Then this would simplify
things. Or do I have to follow how it was defined in the ACPI or device
tree world?

> 
>> +       if (ret < 0) {
>> +               dev_err(&spi->dev,
>> +                       "Missing ext-vin-microvolt device property\n");
>> +               return -ENODEV;
>> +       }
> 
>> +       st->ext_vin_mv = (u16)val;
> 
> Casting is not needed.

"Yes, I do want to cast this down." It's a recommended style for such
cases. But it will be gone if we can read u16 directly.

> 
>> +
>> +       /* Use regulator, if available. */
>> +       st->reg = devm_regulator_get(&spi->dev, "vref");
> 
> Here is most important concern, how you get this regulator in ACPI
> case in the future? To me it sounds more like
> devm_regulator_get_optional();

Actually, all it takes is CONFIG_REGULATOR=y and an access to
in_voltage*_scale to reveal that the code is already broken today. Will fix.

> 
>> +       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 = adc108s102_channels;
>> +       indio_dev->num_channels = ARRAY_SIZE(adc108s102_channels);
>> +       indio_dev->info = &adc108s102_info;
>> +
>> +       /* Setup default message */
>> +       st->scan_single_xfer.tx_buf = st->tx_buf;
>> +       st->scan_single_xfer.rx_buf = st->rx_buf;
>> +       st->scan_single_xfer.len = 2 * sizeof(st->tx_buf[0]);
>> +
>> +       spi_message_init_with_transfers(&st->scan_single_msg,
>> +                                       &st->scan_single_xfer, 1);
>> +
> 
>> +       ret = iio_triggered_buffer_setup(indio_dev, NULL,
>> +                       &adc108s102_trigger_handler, NULL);
>> +       if (ret)
>> +               goto error_disable_reg;
>> +
>> +       ret = iio_device_register(indio_dev);
> 
> I suppose some of above is already under devres API (implicitly when
> devm_iio_..._alloc() is in use)

Indeed, there are devm variants for both functions.

> 
> 
>> +       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);
> 
> Do you need it when devm_ used?
> 
>> +error_disable_reg:
>> +       regulator_disable(st->reg);
> 
> Ditto.

I do not find traces that devm-created regulators are auto-disabled. So
this remains necessary.

> 
>> +
>> +       return ret;
>> +}
>> +
>> +static int adc108s102_remove(struct spi_device *spi)
>> +{
>> +       struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +       struct adc108s102_state *st = iio_priv(indio_dev);
>> +
> 
>> +       iio_device_unregister(indio_dev);
>> +       iio_triggered_buffer_cleanup(indio_dev);
>> +
>> +       regulator_disable(st->reg);
> 
> Ditto.
> 
>> +
>> +       return 0;
>> +}
>> +
> 
>> +static struct spi_driver adc108s102_driver = {
>> +       .driver = {
>> +               .name   = "adc108s102",
> 
>> +               .owner  = THIS_MODULE,
> 
> This is redundant I'm pretty sure.

Even in 2017, drivers keep being added that carry such assignments. Can
you explain when it is needed and when not? Otherwise, I will leave it in.

> 
>> +               .acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
>> +       },
>> +       .probe          = adc108s102_probe,
>> +       .remove         = adc108s102_remove,
>> +       .id_table       = adc108s102_id,
>> +};
> 

The rest makes sense.

Thanks,
Jan

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

[Andy Shevchenko]

On Wed, May 3, 2017 at 8:35 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-02 22:53, Andy Shevchenko wrote:
>> On Tue, May 2, 2017 at 11:02 PM, Jan Kiszka <jan.kiszka@siemens.com> 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.

>>> +       cmds = 0;
>>> +       for_each_set_bit(bit, active_scan_mask, ADC108S102_MAX_CHANNELS)
>>> +               st->tx_buf[cmds++] = cpu_to_be16(ADC108S102_CMD(bit));
>>> +
>>> +       /* One dummy command added, to clock in the last response */
>>> +       st->tx_buf[cmds++] = 0x00;
>>> +
>>> +       /* build SPI ring message */
>>
>>> +       st->ring_xfer.tx_buf = &st->tx_buf[0];
>>> +       st->ring_xfer.rx_buf = &st->rx_buf[0];
>>
>> &pointer[0] -> pointer
>
> This is following patterns in other drivers, expressing you want the
> first element here. I'll keep it.

It doesn't sound fully correct. You need a pointer to the beginning of
the buffer.
I remember when Greg asked me to do same change and I agree with him
that time that this expression is kinda noisy.
I would agree with you if it would be indeed *one* element to address.

In any case it's a call of IIO maintainers (as I said it's a nitpick
from my side).

>>> +static int adc108s102_probe(struct spi_device *spi)
>>> +{
>>> +       struct adc108s102_state *st;
>>> +       struct iio_dev *indio_dev;
>>> +       u32 val;
>>> +       int ret;
>>> +
>>> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>>> +       if (!indio_dev)
>>> +               return -ENOMEM;
>>> +
>>> +       st = iio_priv(indio_dev);
>>> +
>>
>>> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
>>
>> Why not to read u16 here?
>>
>
> Can I read a property with arbitrary width? Then this would simplify
> things.

device_property_read_u16();

> Or do I have to follow how it was defined in the ACPI or device
> tree world?

For property by the way you have to either follow existing one (by
name and meaning), or you
you need get an Ack from DT people (Rob, for example) to introduce such.

Existing one is called "vref-external". So, definitely you need to
figure out this with DT people.

>>> +       st->ext_vin_mv = (u16)val;
>>
>> Casting is not needed.
>
> "Yes, I do want to cast this down." It's a recommended style for such
> cases.

Can you point to the documentation you are referring to?

> But it will be gone if we can read u16 directly.

Yes, you can do that.

>>> +error_disable_reg:
>>> +       regulator_disable(st->reg);
>>
>> Ditto.
>
> I do not find traces that devm-created regulators are auto-disabled. So
> this remains necessary.

Fair enough.

>>> +static struct spi_driver adc108s102_driver = {
>>> +       .driver = {
>>> +               .name   = "adc108s102",
>>
>>> +               .owner  = THIS_MODULE,
>>
>> This is redundant I'm pretty sure.
>
> Even in 2017, drivers keep being added that carry such assignments. Can
> you explain when it is needed and when not? Otherwise, I will leave it in.

The above I'm 100% sure is not needed. It's needed only in cases when
framework / device core doesn't do this for ya.

In the above case IIRC device core does it once for all.

-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

On 2017-05-03 10:06, Andy Shevchenko wrote:
> On Wed, May 3, 2017 at 8:35 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-02 22:53, Andy Shevchenko wrote:
>>> On Tue, May 2, 2017 at 11:02 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> +static int adc108s102_probe(struct spi_device *spi)
>>>> +{
>>>> +       struct adc108s102_state *st;
>>>> +       struct iio_dev *indio_dev;
>>>> +       u32 val;
>>>> +       int ret;
>>>> +
>>>> +       indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>>>> +       if (!indio_dev)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       st = iio_priv(indio_dev);
>>>> +
>>>
>>>> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
>>>
>>> Why not to read u16 here?
>>>
>>
>> Can I read a property with arbitrary width? Then this would simplify
>> things.
> 
> device_property_read_u16();

By now I found out that ACPI does not care about the property type
width, only DT does. So reading it as u16 under ACPI would be fine. But
with DT, we will need a correctly sized property as well - default is u32.

> 
>> Or do I have to follow how it was defined in the ACPI or device
>> tree world?
> 
> For property by the way you have to either follow existing one (by
> name and meaning), or you
> you need get an Ack from DT people (Rob, for example) to introduce such.
> 
> Existing one is called "vref-external". So, definitely you need to
> figure out this with DT people.

Good point... @Rob, @Jonathon, to avoid a different naming between the
now to be introduced ACPI usage and a potential DT binding later on,
really better define a DT one now? Which name to use for such an
external vref, the one above used by spear-adc already?

>>>> +static struct spi_driver adc108s102_driver = {
>>>> +       .driver = {
>>>> +               .name   = "adc108s102",
>>>
>>>> +               .owner  = THIS_MODULE,
>>>
>>> This is redundant I'm pretty sure.
>>
>> Even in 2017, drivers keep being added that carry such assignments. Can
>> you explain when it is needed and when not? Otherwise, I will leave it in.
> 
> The above I'm 100% sure is not needed. It's needed only in cases when
> framework / device core doesn't do this for ya.
> 
> In the above case IIRC device core does it once for all.
> 

Then this is not consistently filtered out in new driver reviews. I can
remove it, of course.

Jan

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

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

[Andy Shevchenko]

On Wed, May 3, 2017 at 11:24 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-03 10:06, Andy Shevchenko wrote:
>> On Wed, May 3, 2017 at 8:35 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>> On 2017-05-02 22:53, Andy Shevchenko wrote:

>>>>> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
>>>>
>>>> Why not to read u16 here?
>>>>
>>>
>>> Can I read a property with arbitrary width? Then this would simplify
>>> things.
>>
>> device_property_read_u16();
>
> By now I found out that ACPI does not care about the property type
> width, only DT does. So reading it as u16 under ACPI would be fine. But
> with DT, we will need a correctly sized property as well - default is u32.

I'm a bit lost here. ACPI cares and DT cares about property size. You
need to agreed what to use with DT people I think based on two
aspects:
- what existing property is using, if any
- what is preferable size to use

API allows to read any size. If you stick with u32 then perhaps better
to change internal variable to be u32 as well.

>>> Or do I have to follow how it was defined in the ACPI or device
>>> tree world?
>>
>> For property by the way you have to either follow existing one (by
>> name and meaning), or you
>> you need get an Ack from DT people (Rob, for example) to introduce such.
>>
>> Existing one is called "vref-external". So, definitely you need to
>> figure out this with DT people.
>
> Good point... @Rob, @Jonathon, to avoid a different naming between the
> now to be introduced ACPI usage and a potential DT binding later on,
> really better define a DT one now? Which name to use for such an
> external vref, the one above used by spear-adc already?

I forgot to mention you also need to add a binding documentation for
this driver.

>>>>> +               .owner  = THIS_MODULE,
>>>>
>>>> This is redundant I'm pretty sure.
>>>
>>> Even in 2017, drivers keep being added that carry such assignments. Can
>>> you explain when it is needed and when not? Otherwise, I will leave it in.
>>
>> The above I'm 100% sure is not needed. It's needed only in cases when
>> framework / device core doesn't do this for ya.
>>
>> In the above case IIRC device core does it once for all.

> Then this is not consistently filtered out in new driver reviews. I can
> remove it, of course.

Please, remove.

I guess many of new drivers just lack of proper review :-(

-- 
With Best Regards,
Andy Shevchenko

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

[Lars-Peter Clausen]

On 05/03/2017 10:43 AM, Andy Shevchenko wrote:
>>>>>> +               .owner  = THIS_MODULE,
>>>>>
>>>>> This is redundant I'm pretty sure.
>>>>
>>>> Even in 2017, drivers keep being added that carry such assignments. Can
>>>> you explain when it is needed and when not? Otherwise, I will leave it in.
>>>
>>> The above I'm 100% sure is not needed. It's needed only in cases when
>>> framework / device core doesn't do this for ya.
>>>
>>> In the above case IIRC device core does it once for all.
> 
>> Then this is not consistently filtered out in new driver reviews. I can
>> remove it, of course.
> 
> Please, remove.
> 
> I guess many of new drivers just lack of proper review :-(
> 

The kbuild bot will even send automated patches to remove the owner field if
a patch that has the owner field is merged. So definitely remove it.

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

[Jan Kiszka]

On 2017-05-03 10:43, Andy Shevchenko wrote:
> On Wed, May 3, 2017 at 11:24 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-05-03 10:06, Andy Shevchenko wrote:
>>> On Wed, May 3, 2017 at 8:35 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>> On 2017-05-02 22:53, Andy Shevchenko wrote:
> 
>>>>>> +       ret = device_property_read_u32(&spi->dev, "ext-vin-microvolt", &val);
>>>>>
>>>>> Why not to read u16 here?
>>>>>
>>>>
>>>> Can I read a property with arbitrary width? Then this would simplify
>>>> things.
>>>
>>> device_property_read_u16();
>>
>> By now I found out that ACPI does not care about the property type
>> width, only DT does. So reading it as u16 under ACPI would be fine. But
>> with DT, we will need a correctly sized property as well - default is u32.
> 
> I'm a bit lost here. ACPI cares and DT cares about property size.

ACPI_TYPE_INTEGER, used for properties, is always stored and read as
64-bit value and then casted to the requested type.

In contrast, if you restrict a DT property via "/bits/ XX", its storage
format changes, and you are better off using the correct access function
for it. That's what I mean.

Jan

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