[PATCH 1/3] iio: adc: add STMPE ADC driver using IIO framework

[PATCH 1/3] iio: adc: add STMPE ADC driver using IIO framework

[Philippe Schenker]

From: Stefan Agner <stefan@agner.ch>

This adds an ADC driver for the STMPE device using the industrial
input/output interface. The driver supports raw reading of values.
The driver depends on the MFD STMPE driver. If the touchscreen
block is enabled too, only four of the 8 ADC channels are available.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---
 drivers/iio/adc/Kconfig              |   7 +
 drivers/iio/adc/Makefile             |   1 +
 drivers/iio/adc/stmpe-adc.c          | 383 +++++++++++++++++++++++++++
 drivers/input/touchscreen/stmpe-ts.c |  11 -
 drivers/mfd/Kconfig                  |   3 +-
 drivers/mfd/stmpe.c                  |  27 ++
 include/linux/mfd/stmpe.h            |  11 +
 7 files changed, 431 insertions(+), 12 deletions(-)
 create mode 100644 drivers/iio/adc/stmpe-adc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a52fea8749a9..d2845472b6c2 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -734,6 +734,13 @@ config STM32_DFSDM_ADC
 	  This driver can also be built as a module.  If so, the module
 	  will be called stm32-dfsdm-adc.
 
+config STMPE_ADC
+	tristate "STMicroelectronics STMPE ADC driver"
+	depends on (OF || COMPILE_TEST || MFD_STMPE)
+	help
+	  Say yes here to build support for ST Microelectronics STMPE
+	  built in ADC block (stmpe811).
+
 config STX104
 	tristate "Apex Embedded Systems STX104 driver"
 	depends on PC104 && X86
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index a6e6a0b659e2..cba889c30bf9 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -69,6 +69,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
 obj-$(CONFIG_STM32_ADC) += stm32-adc.o
 obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o
 obj-$(CONFIG_STM32_DFSDM_ADC) += stm32-dfsdm-adc.o
+obj-$(CONFIG_STMPE_ADC) += stmpe-adc.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
new file mode 100644
index 000000000000..891ad838ab3c
--- /dev/null
+++ b/drivers/iio/adc/stmpe-adc.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *  STMicroelectronics STMPE811 IIO ADC Driver
+ *
+ *  4 channel, 10/12-bit ADC
+ *
+ *  Copyright (C) 2013-2018 Toradex AG <stefan.agner@toradex.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/stmpe.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+#define STMPE_REG_INT_STA	0x0B
+#define STMPE_REG_ADC_INT_EN	0x0E
+#define STMPE_REG_ADC_INT_STA	0x0F
+
+#define STMPE_REG_ADC_CTRL1	0x20
+#define STMPE_REG_ADC_CTRL2	0x21
+#define STMPE_REG_ADC_CAPT	0x22
+#define STMPE_REG_ADC_DATA_CH(channel)	(0x30 + 2 * (channel))
+
+#define STMPE_REG_TEMP_CTRL	0x60
+#define STMPE_START_ONE_TEMP_CONV (0x08 + 0x02 + 0x01)
+#define STMPE_REG_TEMP_DATA	0x61
+#define STMPE_REG_TEMP_TH	0x63
+#define STMPE_MAX_ADC_CH	8
+
+#define STMPE_ADC_CH(channel)	((1 << (channel)) & 0xff)
+
+#define STMPE_ADC_TIMEOUT	(msecs_to_jiffies(1000))
+
+struct stmpe_adc {
+	struct stmpe		*stmpe;
+	struct clk		*clk;
+	struct device		*dev;
+
+	struct iio_chan_spec	stmpe_adc_iio_channels[STMPE_MAX_ADC_CH];
+
+	struct completion	completion;
+
+	u8			channel;
+	u32			value;
+	u8			sample_time;
+	u8			mod_12b;
+	u8			ref_sel;
+	u8			adc_freq;
+	u32			norequest_mask;
+};
+
+static int stmpe_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val,
+				int *val2,
+				long mask)
+{
+	struct stmpe_adc *info = iio_priv(indio_dev);
+	long ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+	case IIO_CHAN_INFO_PROCESSED:
+
+		mutex_lock(&indio_dev->mlock);
+
+		info->channel = (u8)chan->channel;
+
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			if (info->channel > 7)
+				return -ENOENT;
+
+			stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
+					STMPE_ADC_CH(info->channel));
+
+			stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
+					STMPE_ADC_CH(info->channel));
+
+			*val = info->value;
+			break;
+
+		case IIO_TEMP:
+			if (info->channel != 8)
+				return -ENOENT;
+
+			stmpe_reg_write(info->stmpe, STMPE_REG_TEMP_CTRL,
+					STMPE_START_ONE_TEMP_CONV);
+			break;
+		default:
+			mutex_unlock(&indio_dev->mlock);
+			return -EINVAL;
+		}
+
+		ret = wait_for_completion_interruptible_timeout
+			(&info->completion, STMPE_ADC_TIMEOUT);
+
+		if (ret <= 0) {
+			mutex_unlock(&indio_dev->mlock);
+			if (ret == 0)
+				return -ETIMEDOUT;
+			else
+				return ret;
+		}
+
+
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = info->value;
+			break;
+
+		case IIO_TEMP:
+			/*
+			 * absolute temp = +V3.3 * value /7.51 [K]
+			 * scale to [milli °C]
+			 */
+			*val = ((449960l * info->value) / 1024l) - 273150;
+			break;
+		default:
+			break;
+		}
+
+		mutex_unlock(&indio_dev->mlock);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = 3300;
+		*val2 = info->mod_12b ? 12 : 10;
+		return IIO_VAL_FRACTIONAL_LOG2;
+
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static irqreturn_t stmpe_adc_isr(int irq, void *dev_id)
+{
+	struct stmpe_adc *info = (struct stmpe_adc *)dev_id;
+	u8 data[2];
+
+	if (info->channel > 8)
+		return IRQ_NONE;
+
+	if (info->channel < 8) {
+		int int_sta;
+
+		int_sta = stmpe_reg_read(info->stmpe, STMPE_REG_ADC_INT_STA);
+
+		/* Is the interrupt relevant */
+		if (!(int_sta & STMPE_ADC_CH(info->channel)))
+			return IRQ_NONE;
+
+		/* Read value */
+		stmpe_block_read(info->stmpe,
+			STMPE_REG_ADC_DATA_CH(info->channel), 2, data);
+
+		stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_STA, int_sta);
+	} else if (info->channel == 8) {
+		/* Read value */
+		stmpe_block_read(info->stmpe, STMPE_REG_TEMP_DATA, 2, data);
+	}
+
+	info->value = ((u32)data[0] << 8) + data[1];
+	complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info stmpe_adc_iio_info = {
+	.read_raw = &stmpe_read_raw,
+};
+
+static void stmpe_adc_voltage_chan(struct iio_chan_spec *ics, int chan)
+{
+	ics->type = IIO_VOLTAGE;
+	ics->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+	ics->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+	ics->indexed = 1;
+	ics->channel = chan;
+}
+
+static void stmpe_adc_temp_chan(struct iio_chan_spec *ics, int chan)
+{
+	ics->type = IIO_TEMP;
+	ics->info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED);
+	ics->indexed = 1;
+	ics->channel = chan;
+}
+
+static int stmpe_adc_init_hw(struct stmpe_adc *adc)
+{
+	int ret;
+	u8 adc_ctrl1, adc_ctrl1_mask;
+	struct stmpe *stmpe = adc->stmpe;
+	struct device *dev = adc->dev;
+
+	ret = stmpe_enable(stmpe, STMPE_BLOCK_ADC);
+	if (ret) {
+		dev_err(dev, "Could not enable clock for ADC\n");
+		goto err_adc;
+	}
+
+	adc_ctrl1 = SAMPLE_TIME(adc->sample_time) | MOD_12B(adc->mod_12b) |
+			REF_SEL(adc->ref_sel);
+	adc_ctrl1_mask = SAMPLE_TIME(0xff) | MOD_12B(0xff) | REF_SEL(0xff);
+
+	ret = stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL1,
+			adc_ctrl1_mask, adc_ctrl1);
+	if (ret) {
+		dev_err(dev, "Could not setup ADC\n");
+		goto err_adc;
+	}
+
+	ret = stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL2,
+			ADC_FREQ(0xff), ADC_FREQ(adc->adc_freq));
+	if (ret) {
+		dev_err(dev, "Could not setup ADC\n");
+		goto err_adc;
+	}
+
+	/* use temp irq for each conversion completion */
+	stmpe_reg_write(stmpe, STMPE_REG_TEMP_TH, 0);
+	stmpe_reg_write(stmpe, STMPE_REG_TEMP_TH + 1, 0);
+
+	return 0;
+err_adc:
+	stmpe_disable(stmpe, STMPE_BLOCK_ADC);
+
+	return ret;
+}
+
+static void stmpe_adc_get_platform_info(struct platform_device *pdev,
+					struct stmpe_adc *adc)
+{
+	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
+	struct device_node *np = pdev->dev.of_node;
+	u32 val;
+
+	adc->stmpe = stmpe;
+
+	if (!np) {
+		dev_err(&pdev->dev, "no device tree node found\n");
+		return;
+	}
+
+	if (!of_property_read_u32(np, "st,sample-time", &val))
+		adc->sample_time = val;
+	if (!of_property_read_u32(np, "st,mod-12b", &val))
+		adc->mod_12b = val;
+	if (!of_property_read_u32(np, "st,ref-sel", &val))
+		adc->ref_sel = val;
+	if (!of_property_read_u32(np, "st,adc-freq", &val))
+		adc->adc_freq = val;
+	if (!of_property_read_u32(np, "st,norequest-mask", &val))
+		adc->norequest_mask = val;
+}
+
+static int stmpe_adc_probe(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = NULL;
+	struct stmpe_adc *info = NULL;
+	int irq_temp, irq_adc;
+	int num_chan = 0;
+	int i = 0;
+	int ret;
+
+	irq_adc = platform_get_irq_byname(pdev, "STMPE_ADC");
+	if (irq_adc < 0)
+		return irq_adc;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct stmpe_adc));
+	if (!indio_dev) {
+		dev_err(&pdev->dev, "failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	info = iio_priv(indio_dev);
+
+	init_completion(&info->completion);
+	ret = devm_request_threaded_irq(&pdev->dev, irq_adc, NULL,
+					stmpe_adc_isr, IRQF_ONESHOT,
+					"stmpe-adc", info);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
+				irq_adc);
+		return ret;
+	}
+
+	irq_temp = platform_get_irq_byname(pdev, "STMPE_TEMP_SENS");
+	if (irq_temp >= 0) {
+		ret = devm_request_threaded_irq(&pdev->dev, irq_temp, NULL,
+						stmpe_adc_isr, IRQF_ONESHOT,
+						"stmpe-adc", info);
+		if (ret < 0)
+			dev_warn(&pdev->dev, "failed requesting irq for"
+					" temp sensor, irq = %d\n", irq_temp);
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->info = &stmpe_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	stmpe_adc_get_platform_info(pdev, info);
+
+	for_each_clear_bit(i, (unsigned long *) &info->norequest_mask,
+				STMPE_MAX_ADC_CH) {
+		stmpe_adc_voltage_chan(&info->stmpe_adc_iio_channels[num_chan], i);
+		num_chan++;
+	}
+	stmpe_adc_temp_chan(&info->stmpe_adc_iio_channels[num_chan], i);
+	num_chan++;
+	indio_dev->channels = info->stmpe_adc_iio_channels;
+	indio_dev->num_channels = num_chan;
+
+	ret = stmpe_adc_init_hw(info);
+	if (ret)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		return ret;
+
+
+	dev_info(&pdev->dev, "Initialized\n");
+
+	return 0;
+}
+
+static int stmpe_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct stmpe_adc *info = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	stmpe_disable(info->stmpe, STMPE_BLOCK_ADC);
+
+	return 0;
+}
+
+static int __maybe_unused stmpe_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct stmpe_adc *info = iio_priv(indio_dev);
+
+	stmpe_adc_init_hw(info);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(stmpe_adc_pm_ops, NULL, stmpe_adc_resume);
+
+static struct platform_driver stmpe_adc_driver = {
+	.probe		= stmpe_adc_probe,
+	.remove		= stmpe_adc_remove,
+	.driver		= {
+		.name	= "stmpe-adc",
+		.pm	= &stmpe_adc_pm_ops,
+	},
+};
+
+module_platform_driver(stmpe_adc_driver);
+
+MODULE_AUTHOR("Stefan Agner <stefan.agner@toradex.com>");
+MODULE_DESCRIPTION("STMPEXXX ADC driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:stmpe-adc");
diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchscreen/stmpe-ts.c
index 2a78e27b4495..05674e0e233f 100644
--- a/drivers/input/touchscreen/stmpe-ts.c
+++ b/drivers/input/touchscreen/stmpe-ts.c
@@ -49,17 +49,6 @@
 
 #define STMPE_IRQ_TOUCH_DET		0
 
-#define SAMPLE_TIME(x)			((x & 0xf) << 4)
-#define MOD_12B(x)			((x & 0x1) << 3)
-#define REF_SEL(x)			((x & 0x1) << 1)
-#define ADC_FREQ(x)			(x & 0x3)
-#define AVE_CTRL(x)			((x & 0x3) << 6)
-#define DET_DELAY(x)			((x & 0x7) << 3)
-#define SETTLING(x)			(x & 0x7)
-#define FRACTION_Z(x)			(x & 0x7)
-#define I_DRIVE(x)			(x & 0x1)
-#define OP_MODE(x)			((x & 0x7) << 1)
-
 #define STMPE_TS_NAME			"stmpe-ts"
 #define XY_MASK				0xfff
 
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8c5dfdce4326..bba159e8eaa4 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1204,7 +1204,7 @@ config MFD_STMPE
 
 	  Currently supported devices are:
 
-		STMPE811: GPIO, Touchscreen
+		STMPE811: GPIO, Touchscreen, ADC
 		STMPE1601: GPIO, Keypad
 		STMPE1801: GPIO, Keypad
 		STMPE2401: GPIO, Keypad
@@ -1217,6 +1217,7 @@ config MFD_STMPE
 		GPIO: stmpe-gpio
 		Keypad: stmpe-keypad
 		Touchscreen: stmpe-ts
+		ADC: stmpe-adc
 
 menu "STMicroelectronics STMPE Interface Drivers"
 depends on MFD_STMPE
diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 566caca4efd8..00b98f75f7dd 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -463,6 +463,28 @@ static const struct mfd_cell stmpe_ts_cell = {
 	.num_resources	= ARRAY_SIZE(stmpe_ts_resources),
 };
 
+/*
+ * ADC (STMPE811)
+ */
+
+static struct resource stmpe_adc_resources[] = {
+	{
+		.name	= "STMPE_TEMP_SENS",
+		.flags	= IORESOURCE_IRQ,
+	},
+	{
+		.name	= "STMPE_ADC",
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static const struct mfd_cell stmpe_adc_cell = {
+	.name		= "stmpe-adc",
+	.of_compatible	= "st,stmpe-adc",
+	.resources	= stmpe_adc_resources,
+	.num_resources	= ARRAY_SIZE(stmpe_adc_resources),
+};
+
 /*
  * STMPE811 or STMPE610
  */
@@ -497,6 +519,11 @@ static struct stmpe_variant_block stmpe811_blocks[] = {
 		.irq	= STMPE811_IRQ_TOUCH_DET,
 		.block	= STMPE_BLOCK_TOUCHSCREEN,
 	},
+	{
+		.cell	= &stmpe_adc_cell,
+		.irq	= STMPE811_IRQ_TEMP_SENS,
+		.block	= STMPE_BLOCK_ADC,
+	},
 };
 
 static int stmpe811_enable(struct stmpe *stmpe, unsigned int blocks,
diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
index 4a827af17e59..90700013f56d 100644
--- a/include/linux/mfd/stmpe.h
+++ b/include/linux/mfd/stmpe.h
@@ -10,6 +10,17 @@
 
 #include <linux/mutex.h>
 
+#define SAMPLE_TIME(x)		((x & 0xf) << 4)
+#define MOD_12B(x)		((x & 0x1) << 3)
+#define REF_SEL(x)		((x & 0x1) << 1)
+#define ADC_FREQ(x)		(x & 0x3)
+#define AVE_CTRL(x)		((x & 0x3) << 6)
+#define DET_DELAY(x)		((x & 0x7) << 3)
+#define SETTLING(x)		(x & 0x7)
+#define FRACTION_Z(x)		(x & 0x7)
+#define I_DRIVE(x)		(x & 0x1)
+#define OP_MODE(x)		((x & 0x7) << 1)
+
 struct device;
 struct regulator;
 
-- 
2.19.1

[PATCH 2/3] iio: adc: add STMPE ADC devicetree bindings

[Philippe Schenker]

From: Stefan Agner <stefan@agner.ch>

This adds the devicetree bindings for the STMPE ADC.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---
 .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
new file mode 100644
index 000000000000..752ef35a794d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
@@ -0,0 +1,34 @@
+STMPE ADC driver
+----------------
+
+Required properties:
+ - compatible: "st,stmpe-adc"
+
+Optional properties:
+Note that the ADC is shared with the STMPE touchscreen, so if using both the
+settings should be the same.
+If the settings are not the same, the settings of the driver initialized last
+will be active.
+- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks,
+  1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks,
+  5 -> 96 clocks, 6 -> 144 clocks), recommended is 4.
+- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
+- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external
+  reference)
+- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz)
+- st,norequest-mask: bitmask specifying which ADC channels should _not_ be
+  requestable due to different usage (e.g. touch)
+
+Node name must be stmpe_adc and should be child node of stmpe node to
+which it belongs.
+
+Example:
+
+	stmpe_adc {
+		compatible = "st,stmpe-adc";
+		st,sample-time = <4>;
+		st,mod-12b = <1>;
+		st,ref-sel = <0>;
+		st,adc-freq = <1>;
+		st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */
+	};
-- 
2.19.1

[PATCH 3/3] ARM: dts: Add stmpe-adc driver to relevant devicetrees

[Philippe Schenker]

From: Philippe Schenker <philippe.schenker@toradex.com>

Activate the stmpe-adc driver as found on Apalis/Colibri iMX6/T30 modules

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---
 arch/arm/boot/dts/imx6qdl-apalis.dtsi  | 14 ++++++++++++++
 arch/arm/boot/dts/imx6qdl-colibri.dtsi | 14 ++++++++++++++
 arch/arm/boot/dts/tegra30-apalis.dtsi  | 14 ++++++++++++++
 arch/arm/boot/dts/tegra30-colibri.dtsi | 14 ++++++++++++++
 4 files changed, 56 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index 3dc99dd8dde1..10ba925be35a 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -356,6 +356,20 @@
 			/* 5 ms touch detect interrupt delay */
 			st,touch-det-delay = <5>;
 		};
+
+		stmpe_adc {
+			compatible = "st,stmpe-adc";
+			/* 3.25 MHz ADC clock speed */
+			st,adc-freq = <1>;
+			/* 12-bit ADC */
+			st,mod-12b = <1>;
+			/* internal ADC reference */
+			st,ref-sel = <0>;
+			/* ADC converstion time: 80 clocks */
+			st,sample-time = <4>;
+			/* forbid to use ADC channels 3-0 (touch) */
+			st,norequest-mask = <0x0F>;
+		};
 	};
 };
 
diff --git a/arch/arm/boot/dts/imx6qdl-colibri.dtsi b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
index 87e15e7cb32b..2c11dc33526c 100644
--- a/arch/arm/boot/dts/imx6qdl-colibri.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-colibri.dtsi
@@ -287,6 +287,20 @@
 			/* 5 ms touch detect interrupt delay */
 			st,touch-det-delay = <5>;
 		};
+
+		stmpe_adc {
+			compatible = "st,stmpe-adc";
+			/* 3.25 MHz ADC clock speed */
+			st,adc-freq = <1>;
+			/* 12-bit ADC */
+			st,mod-12b = <1>;
+			/* internal ADC reference */
+			st,ref-sel = <0>;
+			/* ADC converstion time: 80 clocks */
+			st,sample-time = <4>;
+			/* forbid to use ADC channels 3-0 (touch) */
+			st,norequest-mask = <0x0F>;
+		};
 	};
 };
 
diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi
index 7f112f192fe9..1fd3c1f60478 100644
--- a/arch/arm/boot/dts/tegra30-apalis.dtsi
+++ b/arch/arm/boot/dts/tegra30-apalis.dtsi
@@ -1001,6 +1001,20 @@
 				/* 5 ms touch detect interrupt delay */
 				st,touch-det-delay = <5>;
 			};
+
+			stmpe_adc {
+				compatible = "st,stmpe-adc";
+				/* 3.25 MHz ADC clock speed */
+				st,adc-freq = <1>;
+				/* 12-bit ADC */
+				st,mod-12b = <1>;
+				/* internal ADC reference */
+				st,ref-sel = <0>;
+				/* ADC converstion time: 80 clocks */
+				st,sample-time = <4>;
+				/* forbid to use ADC channels 3-0 (touch) */
+				st,norequest-mask = <0x0F>;
+			};
 		};
 
 		/*
diff --git a/arch/arm/boot/dts/tegra30-colibri.dtsi b/arch/arm/boot/dts/tegra30-colibri.dtsi
index 35af03ca9e90..d9fe112eb5ad 100644
--- a/arch/arm/boot/dts/tegra30-colibri.dtsi
+++ b/arch/arm/boot/dts/tegra30-colibri.dtsi
@@ -870,6 +870,20 @@
 				/* 5 ms touch detect interrupt delay */
 				st,touch-det-delay = <5>;
 			};
+
+			stmpe_adc {
+				compatible = "st,stmpe-adc";
+				/* 3.25 MHz ADC clock speed */
+				st,adc-freq = <1>;
+				/* 12-bit ADC */
+				st,mod-12b = <1>;
+				/* internal ADC reference */
+				st,ref-sel = <0>;
+				/* ADC converstion time: 80 clocks */
+				st,sample-time = <4>;
+				/* forbid to use ADC channels 3-0 (touch) */
+				st,norequest-mask = <0x0F>;
+			};
 		};
 
 		/*
-- 
2.19.1

Re: [PATCH 2/3] iio: adc: add STMPE ADC devicetree bindings

[Jonathan Cameron]

On Tue,  6 Nov 2018 17:41:15 +0100
Philippe Schenker <dev@pschenker.ch> wrote:

> From: Stefan Agner <stefan@agner.ch>
> 
> This adds the devicetree bindings for the STMPE ADC.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> ---
>  .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> new file mode 100644
> index 000000000000..752ef35a794d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> @@ -0,0 +1,34 @@
> +STMPE ADC driver
> +----------------
> +
> +Required properties:
> + - compatible: "st,stmpe-adc"
> +
> +Optional properties:
> +Note that the ADC is shared with the STMPE touchscreen, so if using both the
> +settings should be the same.
Ah, guessing there is already a binding in place for that which we need to closely
match.  Should there be one overarching device with both as children though if
it is the same hardware?

> +If the settings are not the same, the settings of the driver initialized last
> +will be active.
> +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks,
> +  1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks,
> +  5 -> 96 clocks, 6 -> 144 clocks), recommended is 4.
Could of questions here.  Is this a hardware dictated limit or is it dependent on
application?  Basically I'm asking if it should be in userspace rather than here.

Ideally I'd love to see it in a unit of time, but failing that can we have it
in clock cycles please rather than an enum?  Always nice if a binding is
human readable.  st,sample-clock-cycles would be a better name.



> +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
> +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external
> +  reference)

If there is an external reference, I'd expect to see a regulator providing it.
Usually the mere presence of such an optional regulator does the job of saying if
it should be used.


> +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz)
So this is some internal clock?  What dictates the right setting?  I.e. how
should anyone know which one to pick?

> +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be
> +  requestable due to different usage (e.g. touch)
That's a little bit ugly.  Would prefer to see an explicit set of channels
brought out as children in DT.  There are some patches around standardising
some properties for doing that under review at the moment.

[PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate file

> +
> +Node name must be stmpe_adc and should be child node of stmpe node to
> +which it belongs.
> +
> +Example:
> +
> +	stmpe_adc {

adc {
is the moderately standard naming for ADCS.

> +		compatible = "st,stmpe-adc";
> +		st,sample-time = <4>;
> +		st,mod-12b = <1>;
> +		st,ref-sel = <0>;
> +		st,adc-freq = <1>;
> +		st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */
> +	};

Re: [PATCH 2/3] iio: adc: add STMPE ADC devicetree bindings

[Jonathan Cameron]

On Sun, 11 Nov 2018 15:42:07 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue,  6 Nov 2018 17:41:15 +0100
> Philippe Schenker <dev@pschenker.ch> wrote:
> 
> > From: Stefan Agner <stefan@agner.ch>
> > 
> > This adds the devicetree bindings for the STMPE ADC.
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> >  .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > new file mode 100644
> > index 000000000000..752ef35a794d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > @@ -0,0 +1,34 @@
> > +STMPE ADC driver
> > +----------------
> > +
> > +Required properties:
> > + - compatible: "st,stmpe-adc"
> > +
> > +Optional properties:
> > +Note that the ADC is shared with the STMPE touchscreen, so if using both the
> > +settings should be the same.  
> Ah, guessing there is already a binding in place for that which we need to closely
> match.  Should there be one overarching device with both as children though if
> it is the same hardware?

ah, there is an mfd, can these become elements of the binding for that so they
are only in one place? It's a bit ugly to do that given there are other
children, but certainly better than having them replicated like this...


> 
> > +If the settings are not the same, the settings of the driver initialized last
> > +will be active.
> > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks,
> > +  1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks,
> > +  5 -> 96 clocks, 6 -> 144 clocks), recommended is 4.  
> Could of questions here.  Is this a hardware dictated limit or is it dependent on
> application?  Basically I'm asking if it should be in userspace rather than here.
> 
> Ideally I'd love to see it in a unit of time, but failing that can we have it
> in clock cycles please rather than an enum?  Always nice if a binding is
> human readable.  st,sample-clock-cycles would be a better name.
> 
> 
> 
> > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
> > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external
> > +  reference)  
> 
> If there is an external reference, I'd expect to see a regulator providing it.
> Usually the mere presence of such an optional regulator does the job of saying if
> it should be used.
> 
> 
> > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 -> 6.5 MHz)  
> So this is some internal clock?  What dictates the right setting?  I.e. how
> should anyone know which one to pick?
> 
> > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be
> > +  requestable due to different usage (e.g. touch)  
> That's a little bit ugly.  Would prefer to see an explicit set of channels
> brought out as children in DT.  There are some patches around standardising
> some properties for doing that under review at the moment.
> 
> [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate file
> 
> > +
> > +Node name must be stmpe_adc and should be child node of stmpe node to
> > +which it belongs.
> > +
> > +Example:
> > +
> > +	stmpe_adc {  
> 
> adc {
> is the moderately standard naming for ADCS.
> 
> > +		compatible = "st,stmpe-adc";
> > +		st,sample-time = <4>;
> > +		st,mod-12b = <1>;
> > +		st,ref-sel = <0>;
> > +		st,adc-freq = <1>;
> > +		st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */
> > +	};  
> 

Re: [PATCH 1/3] iio: adc: add STMPE ADC driver using IIO framework

[Jonathan Cameron]

On Tue,  6 Nov 2018 17:41:14 +0100
Philippe Schenker <dev@pschenker.ch> wrote:

> From: Stefan Agner <stefan@agner.ch>
>=20
> This adds an ADC driver for the STMPE device using the industrial
> input/output interface. The driver supports raw reading of values.
> The driver depends on the MFD STMPE driver. If the touchscreen
> block is enabled too, only four of the 8 ADC channels are available.
>=20
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
We are a bit limited in here I think by how it interacts with the mfd
and most of my comments are about the related DT bindings.

A few comments inline.

thanks,

Jonathan

> ---
>  drivers/iio/adc/Kconfig              |   7 +
>  drivers/iio/adc/Makefile             |   1 +
>  drivers/iio/adc/stmpe-adc.c          | 383 +++++++++++++++++++++++++++
>  drivers/input/touchscreen/stmpe-ts.c |  11 -
>  drivers/mfd/Kconfig                  |   3 +-
>  drivers/mfd/stmpe.c                  |  27 ++
>  include/linux/mfd/stmpe.h            |  11 +
>  7 files changed, 431 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/iio/adc/stmpe-adc.c
>=20
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index a52fea8749a9..d2845472b6c2 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -734,6 +734,13 @@ config STM32_DFSDM_ADC
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called stm32-dfsdm-adc.
> =20
> +config STMPE_ADC
> +	tristate "STMicroelectronics STMPE ADC driver"
> +	depends on (OF || COMPILE_TEST || MFD_STMPE)
> +	help
> +	  Say yes here to build support for ST Microelectronics STMPE
> +	  built in ADC block (stmpe811).
> +
>  config STX104
>  	tristate "Apex Embedded Systems STX104 driver"
>  	depends on PC104 && X86
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index a6e6a0b659e2..cba889c30bf9 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_STM32_ADC_CORE) +=3D stm32-adc-core.o
>  obj-$(CONFIG_STM32_ADC) +=3D stm32-adc.o
>  obj-$(CONFIG_STM32_DFSDM_CORE) +=3D stm32-dfsdm-core.o
>  obj-$(CONFIG_STM32_DFSDM_ADC) +=3D stm32-dfsdm-adc.o
> +obj-$(CONFIG_STMPE_ADC) +=3D stmpe-adc.o
>  obj-$(CONFIG_TI_ADC081C) +=3D ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) +=3D ti-adc0832.o
>  obj-$(CONFIG_TI_ADC084S021) +=3D ti-adc084s021.o
> diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
> new file mode 100644
> index 000000000000..891ad838ab3c
> --- /dev/null
> +++ b/drivers/iio/adc/stmpe-adc.c
> @@ -0,0 +1,383 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  STMicroelectronics STMPE811 IIO ADC Driver
> + *
> + *  4 channel, 10/12-bit ADC
> + *
> + *  Copyright (C) 2013-2018 Toradex AG <stefan.agner@toradex.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/stmpe.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
Not seeing any regulators in here.  Please do a quick
sanity check on the other headers as well. This is a
long list for a fairly short bit of code!

> +#include <linux/slab.h>
> +
> +#define STMPE_REG_INT_STA	0x0B
> +#define STMPE_REG_ADC_INT_EN	0x0E
> +#define STMPE_REG_ADC_INT_STA	0x0F
> +
> +#define STMPE_REG_ADC_CTRL1	0x20
> +#define STMPE_REG_ADC_CTRL2	0x21
> +#define STMPE_REG_ADC_CAPT	0x22
> +#define STMPE_REG_ADC_DATA_CH(channel)	(0x30 + 2 * (channel))
> +
> +#define STMPE_REG_TEMP_CTRL	0x60
> +#define STMPE_START_ONE_TEMP_CONV (0x08 + 0x02 + 0x01)

This is oddly put.. Is it a mask or 3 things we can otherwise
define?

> +#define STMPE_REG_TEMP_DATA	0x61
> +#define STMPE_REG_TEMP_TH	0x63
> +#define STMPE_MAX_ADC_CH	8
> +
> +#define STMPE_ADC_CH(channel)	((1 << (channel)) & 0xff)
> +
> +#define STMPE_ADC_TIMEOUT	(msecs_to_jiffies(1000))
Outer brackets don't add anything that I can see.

> +
> +struct stmpe_adc {
personally I always think aligning structure elements is a bad idea as
breaks far too often with later additions.  Still I don't care
that much if you really want to do this...

> +	struct stmpe		*stmpe;
> +	struct clk		*clk;
> +	struct device		*dev;
> +
> +	struct iio_chan_spec	stmpe_adc_iio_channels[STMPE_MAX_ADC_CH];
> +
> +	struct completion	completion;
> +
> +	u8			channel;
> +	u32			value;
> +	u8			sample_time;
> +	u8			mod_12b;
> +	u8			ref_sel;
> +	u8			adc_freq;
> +	u32			norequest_mask;
> +};
> +
> +static int stmpe_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val,
> +				int *val2,
> +				long mask)
> +{
> +	struct stmpe_adc *info =3D iio_priv(indio_dev);
> +	long ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +	case IIO_CHAN_INFO_PROCESSED:
> +
> +		mutex_lock(&indio_dev->mlock);
> +
> +		info->channel =3D (u8)chan->channel;
> +
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			if (info->channel > 7)
lock held.

> +				return -ENOENT;
> +
> +			stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
> +					STMPE_ADC_CH(info->channel));
> +
> +			stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
> +					STMPE_ADC_CH(info->channel));
> +
> +			*val =3D info->value;
> +			break;
> +
> +		case IIO_TEMP:
> +			if (info->channel !=3D 8)
lock held...
> +				return -ENOENT;
> +
> +			stmpe_reg_write(info->stmpe, STMPE_REG_TEMP_CTRL,
> +					STMPE_START_ONE_TEMP_CONV);
> +			break;
> +		default:
> +			mutex_unlock(&indio_dev->mlock);
> +			return -EINVAL;
> +		}
> +
> +		ret =3D wait_for_completion_interruptible_timeout
> +			(&info->completion, STMPE_ADC_TIMEOUT);
> +
> +		if (ret <=3D 0) {
> +			mutex_unlock(&indio_dev->mlock);
> +			if (ret =3D=3D 0)
> +				return -ETIMEDOUT;
> +			else
> +				return ret;
> +		}
> +
> +
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val =3D info->value;
> +			break;
> +
> +		case IIO_TEMP:
> +			/*
> +			 * absolute temp =3D +V3.3 * value /7.51 [K]
> +			 * scale to [milli =C2=B0C]
> +			 */
> +			*val =3D ((449960l * info->value) / 1024l) - 273150;
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		mutex_unlock(&indio_dev->mlock);
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val =3D 3300;
> +		*val2 =3D info->mod_12b ? 12 : 10;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static irqreturn_t stmpe_adc_isr(int irq, void *dev_id)
> +{
> +	struct stmpe_adc *info =3D (struct stmpe_adc *)dev_id;
> +	u8 data[2];
> +
> +	if (info->channel > 8)
> +		return IRQ_NONE;
> +
> +	if (info->channel < 8) {
> +		int int_sta;
> +
> +		int_sta =3D stmpe_reg_read(info->stmpe, STMPE_REG_ADC_INT_STA);
> +
> +		/* Is the interrupt relevant */
> +		if (!(int_sta & STMPE_ADC_CH(info->channel)))
> +			return IRQ_NONE;
> +
> +		/* Read value */
> +		stmpe_block_read(info->stmpe,
> +			STMPE_REG_ADC_DATA_CH(info->channel), 2, data);
> +
> +		stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_STA, int_sta);
> +	} else if (info->channel =3D=3D 8) {

That 8 is pretty magic, perhaps a define given it keeps being used?

> +		/* Read value */
> +		stmpe_block_read(info->stmpe, STMPE_REG_TEMP_DATA, 2, data);
> +	}
> +
> +	info->value =3D ((u32)data[0] << 8) + data[1];
> +	complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_info stmpe_adc_iio_info =3D {
> +	.read_raw =3D &stmpe_read_raw,
> +};
> +
> +static void stmpe_adc_voltage_chan(struct iio_chan_spec *ics, int chan)
> +{
> +	ics->type =3D IIO_VOLTAGE;
> +	ics->info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW);
> +	ics->info_mask_shared_by_type =3D BIT(IIO_CHAN_INFO_SCALE);
> +	ics->indexed =3D 1;
> +	ics->channel =3D chan;
> +}
> +
> +static void stmpe_adc_temp_chan(struct iio_chan_spec *ics, int chan)
> +{
> +	ics->type =3D IIO_TEMP;
> +	ics->info_mask_separate =3D BIT(IIO_CHAN_INFO_PROCESSED);
> +	ics->indexed =3D 1;
> +	ics->channel =3D chan;
> +}
> +
> +static int stmpe_adc_init_hw(struct stmpe_adc *adc)
> +{
> +	int ret;
> +	u8 adc_ctrl1, adc_ctrl1_mask;
> +	struct stmpe *stmpe =3D adc->stmpe;
> +	struct device *dev =3D adc->dev;
> +
> +	ret =3D stmpe_enable(stmpe, STMPE_BLOCK_ADC);
> +	if (ret) {
> +		dev_err(dev, "Could not enable clock for ADC\n");
> +		goto err_adc;
> +	}
> +
> +	adc_ctrl1 =3D SAMPLE_TIME(adc->sample_time) | MOD_12B(adc->mod_12b) |
> +			REF_SEL(adc->ref_sel);
> +	adc_ctrl1_mask =3D SAMPLE_TIME(0xff) | MOD_12B(0xff) | REF_SEL(0xff);
> +
> +	ret =3D stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL1,
> +			adc_ctrl1_mask, adc_ctrl1);
> +	if (ret) {
> +		dev_err(dev, "Could not setup ADC\n");
> +		goto err_adc;
> +	}
> +
> +	ret =3D stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL2,
> +			ADC_FREQ(0xff), ADC_FREQ(adc->adc_freq));
> +	if (ret) {
> +		dev_err(dev, "Could not setup ADC\n");
> +		goto err_adc;
> +	}
> +
> +	/* use temp irq for each conversion completion */
> +	stmpe_reg_write(stmpe, STMPE_REG_TEMP_TH, 0);
> +	stmpe_reg_write(stmpe, STMPE_REG_TEMP_TH + 1, 0);
> +
> +	return 0;
> +err_adc:
> +	stmpe_disable(stmpe, STMPE_BLOCK_ADC);
> +
> +	return ret;
> +}
> +
> +static void stmpe_adc_get_platform_info(struct platform_device *pdev,
> +					struct stmpe_adc *adc)
> +{
> +	struct stmpe *stmpe =3D dev_get_drvdata(pdev->dev.parent);
> +	struct device_node *np =3D pdev->dev.of_node;
> +	u32 val;
> +
> +	adc->stmpe =3D stmpe;
> +
> +	if (!np) {
> +		dev_err(&pdev->dev, "no device tree node found\n");
> +		return;
> +	}
> +
> +	if (!of_property_read_u32(np, "st,sample-time", &val))
> +		adc->sample_time =3D val;
> +	if (!of_property_read_u32(np, "st,mod-12b", &val))
> +		adc->mod_12b =3D val;
> +	if (!of_property_read_u32(np, "st,ref-sel", &val))
> +		adc->ref_sel =3D val;
> +	if (!of_property_read_u32(np, "st,adc-freq", &val))
> +		adc->adc_freq =3D val;
> +	if (!of_property_read_u32(np, "st,norequest-mask", &val))
> +		adc->norequest_mask =3D val;
I commented on these in the dt-binding patch, so I won't mention
them any more here.

> +}
> +
> +static int stmpe_adc_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev =3D NULL;
> +	struct stmpe_adc *info =3D NULL;
> +	int irq_temp, irq_adc;
> +	int num_chan =3D 0;
> +	int i =3D 0;
> +	int ret;
> +
> +	irq_adc =3D platform_get_irq_byname(pdev, "STMPE_ADC");
> +	if (irq_adc < 0)
> +		return irq_adc;
> +
> +	indio_dev =3D devm_iio_device_alloc(&pdev->dev, sizeof(struct stmpe_adc=
));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	info =3D iio_priv(indio_dev);
> +
> +	init_completion(&info->completion);
> +	ret =3D devm_request_threaded_irq(&pdev->dev, irq_adc, NULL,
> +					stmpe_adc_isr, IRQF_ONESHOT,
> +					"stmpe-adc", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed requesting irq, irq =3D %d\n",
> +				irq_adc);
> +		return ret;
> +	}
> +
> +	irq_temp =3D platform_get_irq_byname(pdev, "STMPE_TEMP_SENS");
> +	if (irq_temp >=3D 0) {
> +		ret =3D devm_request_threaded_irq(&pdev->dev, irq_temp, NULL,
> +						stmpe_adc_isr, IRQF_ONESHOT,
> +						"stmpe-adc", info);
> +		if (ret < 0)
> +			dev_warn(&pdev->dev, "failed requesting irq for"
> +					" temp sensor, irq =3D %d\n", irq_temp);
> +	}
I'd like a comment here that perhaps says what the result of not having an
irq for the temperature sensor is?

> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name =3D dev_name(&pdev->dev);
> +	indio_dev->dev.parent =3D &pdev->dev;
> +	indio_dev->info =3D &stmpe_adc_iio_info;
> +	indio_dev->modes =3D INDIO_DIRECT_MODE;
> +
> +	stmpe_adc_get_platform_info(pdev, info);
> +
> +	for_each_clear_bit(i, (unsigned long *) &info->norequest_mask,
> +				STMPE_MAX_ADC_CH) {
> +		stmpe_adc_voltage_chan(&info->stmpe_adc_iio_channels[num_chan], i);
> +		num_chan++;
> +	}
> +	stmpe_adc_temp_chan(&info->stmpe_adc_iio_channels[num_chan], i);
> +	num_chan++;
> +	indio_dev->channels =3D info->stmpe_adc_iio_channels;
> +	indio_dev->num_channels =3D num_chan;
> +
> +	ret =3D stmpe_adc_init_hw(info);
> +	if (ret)
> +		return ret;
> +
> +	ret =3D iio_device_register(indio_dev);
> +	if (ret)
> +		return ret;
one blank line is plenty.
> +
> +
> +	dev_info(&pdev->dev, "Initialized\n");
This is just noise in the log. There are lots of ways of finding
out if this came up if you actually want to so no need for the print here.

> +
> +	return 0;
> +}
> +
> +static int stmpe_adc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev =3D platform_get_drvdata(pdev);
> +	struct stmpe_adc *info =3D iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	stmpe_disable(info->stmpe, STMPE_BLOCK_ADC);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused stmpe_adc_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
> +	struct stmpe_adc *info =3D iio_priv(indio_dev);
> +
> +	stmpe_adc_init_hw(info);
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(stmpe_adc_pm_ops, NULL, stmpe_adc_resume);
> +
> +static struct platform_driver stmpe_adc_driver =3D {
> +	.probe		=3D stmpe_adc_probe,
> +	.remove		=3D stmpe_adc_remove,
> +	.driver		=3D {
> +		.name	=3D "stmpe-adc",
> +		.pm	=3D &stmpe_adc_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(stmpe_adc_driver);
> +
> +MODULE_AUTHOR("Stefan Agner <stefan.agner@toradex.com>");
> +MODULE_DESCRIPTION("STMPEXXX ADC driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:stmpe-adc");
> diff --git a/drivers/input/touchscreen/stmpe-ts.c b/drivers/input/touchsc=
reen/stmpe-ts.c
> index 2a78e27b4495..05674e0e233f 100644
> --- a/drivers/input/touchscreen/stmpe-ts.c
> +++ b/drivers/input/touchscreen/stmpe-ts.c
> @@ -49,17 +49,6 @@
> =20
>  #define STMPE_IRQ_TOUCH_DET		0
> =20
> -#define SAMPLE_TIME(x)			((x & 0xf) << 4)
> -#define MOD_12B(x)			((x & 0x1) << 3)
> -#define REF_SEL(x)			((x & 0x1) << 1)
> -#define ADC_FREQ(x)			(x & 0x3)
> -#define AVE_CTRL(x)			((x & 0x3) << 6)
> -#define DET_DELAY(x)			((x & 0x7) << 3)
> -#define SETTLING(x)			(x & 0x7)
> -#define FRACTION_Z(x)			(x & 0x7)
> -#define I_DRIVE(x)			(x & 0x1)
> -#define OP_MODE(x)			((x & 0x7) << 1)
> -

Given the need for prefixes in the header (see below) I would
do this move as a precursor patch including renames in the
touchscreen driver...

>  #define STMPE_TS_NAME			"stmpe-ts"
>  #define XY_MASK				0xfff
> =20
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8c5dfdce4326..bba159e8eaa4 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1204,7 +1204,7 @@ config MFD_STMPE
> =20
>  	  Currently supported devices are:
> =20
> -		STMPE811: GPIO, Touchscreen
> +		STMPE811: GPIO, Touchscreen, ADC
>  		STMPE1601: GPIO, Keypad
>  		STMPE1801: GPIO, Keypad
>  		STMPE2401: GPIO, Keypad
> @@ -1217,6 +1217,7 @@ config MFD_STMPE
>  		GPIO: stmpe-gpio
>  		Keypad: stmpe-keypad
>  		Touchscreen: stmpe-ts
> +		ADC: stmpe-adc
> =20
>  menu "STMicroelectronics STMPE Interface Drivers"
>  depends on MFD_STMPE
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index 566caca4efd8..00b98f75f7dd 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -463,6 +463,28 @@ static const struct mfd_cell stmpe_ts_cell =3D {
>  	.num_resources	=3D ARRAY_SIZE(stmpe_ts_resources),
>  };
> =20
> +/*
> + * ADC (STMPE811)
> + */
> +
> +static struct resource stmpe_adc_resources[] =3D {
> +	{
> +		.name	=3D "STMPE_TEMP_SENS",
> +		.flags	=3D IORESOURCE_IRQ,
> +	},
> +	{
> +		.name	=3D "STMPE_ADC",
> +		.flags	=3D IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct mfd_cell stmpe_adc_cell =3D {
> +	.name		=3D "stmpe-adc",
> +	.of_compatible	=3D "st,stmpe-adc",
> +	.resources	=3D stmpe_adc_resources,
> +	.num_resources	=3D ARRAY_SIZE(stmpe_adc_resources),
> +};
> +
>  /*
>   * STMPE811 or STMPE610
>   */
> @@ -497,6 +519,11 @@ static struct stmpe_variant_block stmpe811_blocks[] =
=3D {
>  		.irq	=3D STMPE811_IRQ_TOUCH_DET,
>  		.block	=3D STMPE_BLOCK_TOUCHSCREEN,
>  	},
> +	{
> +		.cell	=3D &stmpe_adc_cell,
> +		.irq	=3D STMPE811_IRQ_TEMP_SENS,
> +		.block	=3D STMPE_BLOCK_ADC,
> +	},
>  };
> =20
>  static int stmpe811_enable(struct stmpe *stmpe, unsigned int blocks,
> diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
> index 4a827af17e59..90700013f56d 100644
> --- a/include/linux/mfd/stmpe.h
> +++ b/include/linux/mfd/stmpe.h
> @@ -10,6 +10,17 @@
> =20
>  #include <linux/mutex.h>
> =20
> +#define SAMPLE_TIME(x)		((x & 0xf) << 4)
> +#define MOD_12B(x)		((x & 0x1) << 3)
> +#define REF_SEL(x)		((x & 0x1) << 1)
> +#define ADC_FREQ(x)		(x & 0x3)
> +#define AVE_CTRL(x)		((x & 0x3) << 6)
> +#define DET_DELAY(x)		((x & 0x7) << 3)
> +#define SETTLING(x)		(x & 0x7)
> +#define FRACTION_Z(x)		(x & 0x7)
> +#define I_DRIVE(x)		(x & 0x1)
> +#define OP_MODE(x)		((x & 0x7) << 1)

I assumed that these were in keeping with naming in this file but
they aren't.  Should have a prefix to avoid potential name clashes
in the future.

STMPE_SAMPlE_TIME etc would make sense.

> +
>  struct device;
>  struct regulator;
> =20

Re: [PATCH 1/3] iio: adc: add STMPE ADC driver using IIO framework

[Philippe Schenker]

On Sun, 2018-11-11 at 16:00 +0000, Jonathan Cameron wrote:
> On Tue,  6 Nov 2018 17:41:14 +0100
> Philippe Schenker <dev@pschenker.ch> wrote:
> 
> > From: Stefan Agner <stefan@agner.ch>
> > 
> > This adds an ADC driver for the STMPE device using the industrial
> > input/output interface. The driver supports raw reading of values.
> > The driver depends on the MFD STMPE driver. If the touchscreen
> > block is enabled too, only four of the 8 ADC channels are available.
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> We are a bit limited in here I think by how it interacts with the mfd
> and most of my comments are about the related DT bindings.
> 
> A few comments inline.
> 
> thanks,
> 
> Jonathan

Thank you for these comments. I am preparing a v2 right now with your
suggestions. Please see my comments inline. As soon I have v2 ready I will send
it.

Philippe

> 
> > ---
> >  drivers/iio/adc/Kconfig              |   7 +
> >  drivers/iio/adc/Makefile             |   1 +
> >  drivers/iio/adc/stmpe-adc.c          | 383 +++++++++++++++++++++++++++
> >  drivers/input/touchscreen/stmpe-ts.c |  11 -
> >  drivers/mfd/Kconfig                  |   3 +-
> >  drivers/mfd/stmpe.c                  |  27 ++
> >  include/linux/mfd/stmpe.h            |  11 +
> >  7 files changed, 431 insertions(+), 12 deletions(-)
> >  create mode 100644 drivers/iio/adc/stmpe-adc.c
> > 
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index a52fea8749a9..d2845472b6c2 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -734,6 +734,13 @@ config STM32_DFSDM_ADC
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called stm32-dfsdm-adc.
> >  
> > +config STMPE_ADC
> > +	tristate "STMicroelectronics STMPE ADC driver"
> > +	depends on (OF || COMPILE_TEST || MFD_STMPE)
> > +	help
> > +	  Say yes here to build support for ST Microelectronics STMPE
> > +	  built in ADC block (stmpe811).
> > +
> >  config STX104
> >  	tristate "Apex Embedded Systems STX104 driver"
> >  	depends on PC104 && X86
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index a6e6a0b659e2..cba889c30bf9 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -69,6 +69,7 @@ obj-$(CONFIG_STM32_ADC_CORE) += stm32-adc-core.o
> >  obj-$(CONFIG_STM32_ADC) += stm32-adc.o
> >  obj-$(CONFIG_STM32_DFSDM_CORE) += stm32-dfsdm-core.o
> >  obj-$(CONFIG_STM32_DFSDM_ADC) += stm32-dfsdm-adc.o
> > +obj-$(CONFIG_STMPE_ADC) += stmpe-adc.o
> >  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
> >  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
> >  obj-$(CONFIG_TI_ADC084S021) += ti-adc084s021.o
> > diff --git a/drivers/iio/adc/stmpe-adc.c b/drivers/iio/adc/stmpe-adc.c
> > new file mode 100644
> > index 000000000000..891ad838ab3c
> > --- /dev/null
> > +++ b/drivers/iio/adc/stmpe-adc.c
> > @@ -0,0 +1,383 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  STMicroelectronics STMPE811 IIO ADC Driver
> > + *
> > + *  4 channel, 10/12-bit ADC
> > + *
> > + *  Copyright (C) 2013-2018 Toradex AG <stefan.agner@toradex.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/stmpe.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> Not seeing any regulators in here.  Please do a quick
> sanity check on the other headers as well. This is a
> long list for a fairly short bit of code!
> 
> > +#include <linux/slab.h>
> > +
> > +#define STMPE_REG_INT_STA	0x0B
> > +#define STMPE_REG_ADC_INT_EN	0x0E
> > +#define STMPE_REG_ADC_INT_STA	0x0F
> > +
> > +#define STMPE_REG_ADC_CTRL1	0x20
> > +#define STMPE_REG_ADC_CTRL2	0x21
> > +#define STMPE_REG_ADC_CAPT	0x22
> > +#define STMPE_REG_ADC_DATA_CH(channel)	(0x30 + 2 * (channel))
> > +
> > +#define STMPE_REG_TEMP_CTRL	0x60
> > +#define STMPE_START_ONE_TEMP_CONV (0x08 + 0x02 + 0x01)
> 
> This is oddly put.. Is it a mask or 3 things we can otherwise
> define?
> 
> > +#define STMPE_REG_TEMP_DATA	0x61
> > +#define STMPE_REG_TEMP_TH	0x63
> > +#define STMPE_MAX_ADC_CH	8
> > +
> > +#define STMPE_ADC_CH(channel)	((1 << (channel)) & 0xff)
> > +
> > +#define STMPE_ADC_TIMEOUT	(msecs_to_jiffies(1000))
> Outer brackets don't add anything that I can see.
> 
> > +
> > +struct stmpe_adc {
> personally I always think aligning structure elements is a bad idea as
> breaks far too often with later additions.  Still I don't care
> that much if you really want to do this...
> 
> > +	struct stmpe		*stmpe;
> > +	struct clk		*clk;
> > +	struct device		*dev;
> > +
> > +	struct iio_chan_spec	stmpe_adc_iio_channels[STMPE_MAX_ADC_CH];
> > +
> > +	struct completion	completion;
> > +
> > +	u8			channel;
> > +	u32			value;
> > +	u8			sample_time;
> > +	u8			mod_12b;
> > +	u8			ref_sel;
> > +	u8			adc_freq;
> > +	u32			norequest_mask;
> > +};
> > +
> > +static int stmpe_read_raw(struct iio_dev *indio_dev,
> > +				struct iio_chan_spec const *chan,
> > +				int *val,
> > +				int *val2,
> > +				long mask)
> > +{
> > +	struct stmpe_adc *info = iio_priv(indio_dev);
> > +	long ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +
> > +		mutex_lock(&indio_dev->mlock);
> > +
> > +		info->channel = (u8)chan->channel;
> > +
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			if (info->channel > 7)
> lock held.
> 
> > +				return -ENOENT;
> > +
> > +			stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_EN,
> > +					STMPE_ADC_CH(info->channel));
> > +
> > +			stmpe_reg_write(info->stmpe, STMPE_REG_ADC_CAPT,
> > +					STMPE_ADC_CH(info->channel));
> > +
> > +			*val = info->value;
> > +			break;
> > +
> > +		case IIO_TEMP:
> > +			if (info->channel != 8)
> lock held...
> > +				return -ENOENT;
> > +
> > +			stmpe_reg_write(info->stmpe, STMPE_REG_TEMP_CTRL,
> > +					STMPE_START_ONE_TEMP_CONV);
> > +			break;
> > +		default:
> > +			mutex_unlock(&indio_dev->mlock);
> > +			return -EINVAL;
> > +		}
> > +
> > +		ret = wait_for_completion_interruptible_timeout
> > +			(&info->completion, STMPE_ADC_TIMEOUT);
> > +
> > +		if (ret <= 0) {
> > +			mutex_unlock(&indio_dev->mlock);
> > +			if (ret == 0)
> > +				return -ETIMEDOUT;
> > +			else
> > +				return ret;
> > +		}
> > +
> > +
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			*val = info->value;
> > +			break;
> > +
> > +		case IIO_TEMP:
> > +			/*
> > +			 * absolute temp = +V3.3 * value /7.51 [K]
> > +			 * scale to [milli °C]
> > +			 */
> > +			*val = ((449960l * info->value) / 1024l) - 273150;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		mutex_unlock(&indio_dev->mlock);
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 3300;
> > +		*val2 = info->mod_12b ? 12 : 10;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static irqreturn_t stmpe_adc_isr(int irq, void *dev_id)
> > +{
> > +	struct stmpe_adc *info = (struct stmpe_adc *)dev_id;
> > +	u8 data[2];
> > +
> > +	if (info->channel > 8)
> > +		return IRQ_NONE;
> > +
> > +	if (info->channel < 8) {
> > +		int int_sta;
> > +
> > +		int_sta = stmpe_reg_read(info->stmpe, STMPE_REG_ADC_INT_STA);
> > +
> > +		/* Is the interrupt relevant */
> > +		if (!(int_sta & STMPE_ADC_CH(info->channel)))
> > +			return IRQ_NONE;
> > +
> > +		/* Read value */
> > +		stmpe_block_read(info->stmpe,
> > +			STMPE_REG_ADC_DATA_CH(info->channel), 2, data);
> > +
> > +		stmpe_reg_write(info->stmpe, STMPE_REG_ADC_INT_STA, int_sta);
> > +	} else if (info->channel == 8) {
> 
> That 8 is pretty magic, perhaps a define given it keeps being used?
> 
> > +		/* Read value */
> > +		stmpe_block_read(info->stmpe, STMPE_REG_TEMP_DATA, 2, data);
> > +	}
> > +
> > +	info->value = ((u32)data[0] << 8) + data[1];
> > +	complete(&info->completion);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info stmpe_adc_iio_info = {
> > +	.read_raw = &stmpe_read_raw,
> > +};
> > +
> > +static void stmpe_adc_voltage_chan(struct iio_chan_spec *ics, int chan)
> > +{
> > +	ics->type = IIO_VOLTAGE;
> > +	ics->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +	ics->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
> > +	ics->indexed = 1;
> > +	ics->channel = chan;
> > +}
> > +
> > +static void stmpe_adc_temp_chan(struct iio_chan_spec *ics, int chan)
> > +{
> > +	ics->type = IIO_TEMP;
> > +	ics->info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED);
> > +	ics->indexed = 1;
> > +	ics->channel = chan;
> > +}
> > +
> > +static int stmpe_adc_init_hw(struct stmpe_adc *adc)
> > +{
> > +	int ret;
> > +	u8 adc_ctrl1, adc_ctrl1_mask;
> > +	struct stmpe *stmpe = adc->stmpe;
> > +	struct device *dev = adc->dev;
> > +
> > +	ret = stmpe_enable(stmpe, STMPE_BLOCK_ADC);
> > +	if (ret) {
> > +		dev_err(dev, "Could not enable clock for ADC\n");
> > +		goto err_adc;
> > +	}
> > +
> > +	adc_ctrl1 = SAMPLE_TIME(adc->sample_time) | MOD_12B(adc->mod_12b) |
> > +			REF_SEL(adc->ref_sel);
> > +	adc_ctrl1_mask = SAMPLE_TIME(0xff) | MOD_12B(0xff) | REF_SEL(0xff);
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL1,
> > +			adc_ctrl1_mask, adc_ctrl1);
> > +	if (ret) {
> > +		dev_err(dev, "Could not setup ADC\n");
> > +		goto err_adc;
> > +	}
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL2,
> > +			ADC_FREQ(0xff), ADC_FREQ(adc->adc_freq));
> > +	if (ret) {
> > +		dev_err(dev, "Could not setup ADC\n");
> > +		goto err_adc;
> > +	}
> > +
> > +	/* use temp irq for each conversion completion */
> > +	stmpe_reg_write(stmpe, STMPE_REG_TEMP_TH, 0);
> > +	stmpe_reg_write(stmpe, STMPE_REG_TEMP_TH + 1, 0);
> > +
> > +	return 0;
> > +err_adc:
> > +	stmpe_disable(stmpe, STMPE_BLOCK_ADC);
> > +
> > +	return ret;
> > +}
> > +
> > +static void stmpe_adc_get_platform_info(struct platform_device *pdev,
> > +					struct stmpe_adc *adc)
> > +{
> > +	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	u32 val;
> > +
> > +	adc->stmpe = stmpe;
> > +
> > +	if (!np) {
> > +		dev_err(&pdev->dev, "no device tree node found\n");
> > +		return;
> > +	}
> > +
> > +	if (!of_property_read_u32(np, "st,sample-time", &val))
> > +		adc->sample_time = val;
> > +	if (!of_property_read_u32(np, "st,mod-12b", &val))
> > +		adc->mod_12b = val;
> > +	if (!of_property_read_u32(np, "st,ref-sel", &val))
> > +		adc->ref_sel = val;
> > +	if (!of_property_read_u32(np, "st,adc-freq", &val))
> > +		adc->adc_freq = val;
> > +	if (!of_property_read_u32(np, "st,norequest-mask", &val))
> > +		adc->norequest_mask = val;
> I commented on these in the dt-binding patch, so I won't mention
> them any more here.
> 
> > +}
> > +
> > +static int stmpe_adc_probe(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *indio_dev = NULL;
> > +	struct stmpe_adc *info = NULL;
> > +	int irq_temp, irq_adc;
> > +	int num_chan = 0;
> > +	int i = 0;
> > +	int ret;
> > +
> > +	irq_adc = platform_get_irq_byname(pdev, "STMPE_ADC");
> > +	if (irq_adc < 0)
> > +		return irq_adc;
> > +
> > +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct stmpe_adc));
> > +	if (!indio_dev) {
> > +		dev_err(&pdev->dev, "failed allocating iio device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	info = iio_priv(indio_dev);
> > +
> > +	init_completion(&info->completion);
> > +	ret = devm_request_threaded_irq(&pdev->dev, irq_adc, NULL,
> > +					stmpe_adc_isr, IRQF_ONESHOT,
> > +					"stmpe-adc", info);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
> > +				irq_adc);
> > +		return ret;
> > +	}
> > +
> > +	irq_temp = platform_get_irq_byname(pdev, "STMPE_TEMP_SENS");
> > +	if (irq_temp >= 0) {
> > +		ret = devm_request_threaded_irq(&pdev->dev, irq_temp, NULL,
> > +						stmpe_adc_isr, IRQF_ONESHOT,
> > +						"stmpe-adc", info);
> > +		if (ret < 0)
> > +			dev_warn(&pdev->dev, "failed requesting irq for"
> > +					" temp sensor, irq = %d\n", irq_temp);
> > +	}
> I'd like a comment here that perhaps says what the result of not having an
> irq for the temperature sensor is?

Basically the STMPE device has only one hardware interrupt. This is initialized
in drivers/mfd/stmpe.c (1171). So if there is no error in mfd, we can expect it
to work in here.

> 
> > +
> > +	platform_set_drvdata(pdev, indio_dev);
> > +
> > +	indio_dev->name = dev_name(&pdev->dev);
> > +	indio_dev->dev.parent = &pdev->dev;
> > +	indio_dev->info = &stmpe_adc_iio_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	stmpe_adc_get_platform_info(pdev, info);
> > +
> > +	for_each_clear_bit(i, (unsigned long *) &info->norequest_mask,
> > +				STMPE_MAX_ADC_CH) {
> > +		stmpe_adc_voltage_chan(&info->stmpe_adc_iio_channels[num_chan],
> > i);
> > +		num_chan++;
> > +	}
> > +	stmpe_adc_temp_chan(&info->stmpe_adc_iio_channels[num_chan], i);
> > +	num_chan++;
> > +	indio_dev->channels = info->stmpe_adc_iio_channels;
> > +	indio_dev->num_channels = num_chan;
> > +
> > +	ret = stmpe_adc_init_hw(info);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret)
> > +		return ret;
> one blank line is plenty.
> > +
> > +
> > +	dev_info(&pdev->dev, "Initialized\n");
> This is just noise in the log. There are lots of ways of finding
> out if this came up if you actually want to so no need for the print here.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int stmpe_adc_remove(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > +	struct stmpe_adc *info = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	stmpe_disable(info->stmpe, STMPE_BLOCK_ADC);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused stmpe_adc_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct stmpe_adc *info = iio_priv(indio_dev);
> > +
> > +	stmpe_adc_init_hw(info);
> > +
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(stmpe_adc_pm_ops, NULL, stmpe_adc_resume);
> > +
> > +static struct platform_driver stmpe_adc_driver = {
> > +	.probe		= stmpe_adc_probe,
> > +	.remove		= stmpe_adc_remove,
> > +	.driver		= {
> > +		.name	= "stmpe-adc",
> > +		.pm	= &stmpe_adc_pm_ops,
> > +	},
> > +};
> > +
> > +module_platform_driver(stmpe_adc_driver);
> > +
> > +MODULE_AUTHOR("Stefan Agner <stefan.agner@toradex.com>");
> > +MODULE_DESCRIPTION("STMPEXXX ADC driver");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:stmpe-adc");
> > diff --git a/drivers/input/touchscreen/stmpe-ts.c
> > b/drivers/input/touchscreen/stmpe-ts.c
> > index 2a78e27b4495..05674e0e233f 100644
> > --- a/drivers/input/touchscreen/stmpe-ts.c
> > +++ b/drivers/input/touchscreen/stmpe-ts.c
> > @@ -49,17 +49,6 @@
> >  
> >  #define STMPE_IRQ_TOUCH_DET		0
> >  
> > -#define SAMPLE_TIME(x)			((x & 0xf) << 4)
> > -#define MOD_12B(x)			((x & 0x1) << 3)
> > -#define REF_SEL(x)			((x & 0x1) << 1)
> > -#define ADC_FREQ(x)			(x & 0x3)
> > -#define AVE_CTRL(x)			((x & 0x3) << 6)
> > -#define DET_DELAY(x)			((x & 0x7) << 3)
> > -#define SETTLING(x)			(x & 0x7)
> > -#define FRACTION_Z(x)			(x & 0x7)
> > -#define I_DRIVE(x)			(x & 0x1)
> > -#define OP_MODE(x)			((x & 0x7) << 1)
> > -
> 
> Given the need for prefixes in the header (see below) I would
> do this move as a precursor patch including renames in the
> touchscreen driver...
> 
> >  #define STMPE_TS_NAME			"stmpe-ts"
> >  #define XY_MASK				0xfff
> >  
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8c5dfdce4326..bba159e8eaa4 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1204,7 +1204,7 @@ config MFD_STMPE
> >  
> >  	  Currently supported devices are:
> >  
> > -		STMPE811: GPIO, Touchscreen
> > +		STMPE811: GPIO, Touchscreen, ADC
> >  		STMPE1601: GPIO, Keypad
> >  		STMPE1801: GPIO, Keypad
> >  		STMPE2401: GPIO, Keypad
> > @@ -1217,6 +1217,7 @@ config MFD_STMPE
> >  		GPIO: stmpe-gpio
> >  		Keypad: stmpe-keypad
> >  		Touchscreen: stmpe-ts
> > +		ADC: stmpe-adc
> >  
> >  menu "STMicroelectronics STMPE Interface Drivers"
> >  depends on MFD_STMPE
> > diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> > index 566caca4efd8..00b98f75f7dd 100644
> > --- a/drivers/mfd/stmpe.c
> > +++ b/drivers/mfd/stmpe.c
> > @@ -463,6 +463,28 @@ static const struct mfd_cell stmpe_ts_cell = {
> >  	.num_resources	= ARRAY_SIZE(stmpe_ts_resources),
> >  };
> >  
> > +/*
> > + * ADC (STMPE811)
> > + */
> > +
> > +static struct resource stmpe_adc_resources[] = {
> > +	{
> > +		.name	= "STMPE_TEMP_SENS",
> > +		.flags	= IORESOURCE_IRQ,
> > +	},
> > +	{
> > +		.name	= "STMPE_ADC",
> > +		.flags	= IORESOURCE_IRQ,
> > +	},
> > +};
> > +
> > +static const struct mfd_cell stmpe_adc_cell = {
> > +	.name		= "stmpe-adc",
> > +	.of_compatible	= "st,stmpe-adc",
> > +	.resources	= stmpe_adc_resources,
> > +	.num_resources	= ARRAY_SIZE(stmpe_adc_resources),
> > +};
> > +
> >  /*
> >   * STMPE811 or STMPE610
> >   */
> > @@ -497,6 +519,11 @@ static struct stmpe_variant_block stmpe811_blocks[] = {
> >  		.irq	= STMPE811_IRQ_TOUCH_DET,
> >  		.block	= STMPE_BLOCK_TOUCHSCREEN,
> >  	},
> > +	{
> > +		.cell	= &stmpe_adc_cell,
> > +		.irq	= STMPE811_IRQ_TEMP_SENS,
> > +		.block	= STMPE_BLOCK_ADC,
> > +	},
> >  };
> >  
> >  static int stmpe811_enable(struct stmpe *stmpe, unsigned int blocks,
> > diff --git a/include/linux/mfd/stmpe.h b/include/linux/mfd/stmpe.h
> > index 4a827af17e59..90700013f56d 100644
> > --- a/include/linux/mfd/stmpe.h
> > +++ b/include/linux/mfd/stmpe.h
> > @@ -10,6 +10,17 @@
> >  
> >  #include <linux/mutex.h>
> >  
> > +#define SAMPLE_TIME(x)		((x & 0xf) << 4)
> > +#define MOD_12B(x)		((x & 0x1) << 3)
> > +#define REF_SEL(x)		((x & 0x1) << 1)
> > +#define ADC_FREQ(x)		(x & 0x3)
> > +#define AVE_CTRL(x)		((x & 0x3) << 6)
> > +#define DET_DELAY(x)		((x & 0x7) << 3)
> > +#define SETTLING(x)		(x & 0x7)
> > +#define FRACTION_Z(x)		(x & 0x7)
> > +#define I_DRIVE(x)		(x & 0x1)
> > +#define OP_MODE(x)		((x & 0x7) << 1)
> 
> I assumed that these were in keeping with naming in this file but
> they aren't.  Should have a prefix to avoid potential name clashes
> in the future.
> 
> STMPE_SAMPlE_TIME etc would make sense.
> 
> > +
> >  struct device;
> >  struct regulator;
> >  

Re: [PATCH 2/3] iio: adc: add STMPE ADC devicetree bindings

[Philippe Schenker]

On Sun, 2018-11-11 at 15:42 +0000, Jonathan Cameron wrote:
> On Tue,  6 Nov 2018 17:41:15 +0100
> Philippe Schenker <dev@pschenker.ch> wrote:
> 
> > From: Stefan Agner <stefan@agner.ch>
> > 
> > This adds the devicetree bindings for the STMPE ADC.
> > 
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> >  .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > new file mode 100644
> > index 000000000000..752ef35a794d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > @@ -0,0 +1,34 @@
> > +STMPE ADC driver
> > +----------------
> > +
> > +Required properties:
> > + - compatible: "st,stmpe-adc"
> > +
> > +Optional properties:
> > +Note that the ADC is shared with the STMPE touchscreen, so if using both
> > the
> > +settings should be the same.
> Ah, guessing there is already a binding in place for that which we need to
> closely
> match.  Should there be one overarching device with both as children though if
> it is the same hardware?

I moved ADC settings to the mfd...

> 
> > +If the settings are not the same, the settings of the driver initialized
> > last
> > +will be active.
> > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks,
> > +  1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks,
> > +  5 -> 96 clocks, 6 -> 144 clocks), recommended is 4.
> Could of questions here.  Is this a hardware dictated limit or is it dependent
> on
> application?  Basically I'm asking if it should be in userspace rather than
> here.
> 
> Ideally I'd love to see it in a unit of time, but failing that can we have it
> in clock cycles please rather than an enum?  Always nice if a binding is
> human readable.  st,sample-clock-cycles would be a better name.
> 

This is none of our design-decision. We did it the same way as it was already in
drivers/input/touchscreen/stmpe-ts.c.

> 
> > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
> > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external
> > +  reference)
> 
> If there is an external reference, I'd expect to see a regulator providing it.
> Usually the mere presence of such an optional regulator does the job of saying
> if
> it should be used.

Same here, we did it the same way as it was in stmpe-ts.c

> 
> 
> > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 ->
> > 6.5 MHz)
> So this is some internal clock?  What dictates the right setting?  I.e. how
> should anyone know which one to pick?

I think as a user of this driver at this point you need to read the datasheet
anyway, to be able to decide which parameters you need for the application.

> 
> > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be
> > +  requestable due to different usage (e.g. touch)
> That's a little bit ugly.  Would prefer to see an explicit set of channels
> brought out as children in DT.  There are some patches around standardising
> some properties for doing that under review at the moment.
> 
> [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate
> file

This is also implemented with the "norequest-mask" in stmpe-ts.c. That's why we
did it the same way and I don't like to break others devicetree by changing
stmpe-ts.c

> 
> > +
> > +Node name must be stmpe_adc and should be child node of stmpe node to
> > +which it belongs.
> > +
> > +Example:
> > +
> > +	stmpe_adc {
> 
> adc {
> is the moderately standard naming for ADCS.

This is none of our naming decisions please see line 1311 in drivers/mfd/stmpe.c

> 
> > +		compatible = "st,stmpe-adc";
> > +		st,sample-time = <4>;
> > +		st,mod-12b = <1>;
> > +		st,ref-sel = <0>;
> > +		st,adc-freq = <1>;
> > +		st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */
> > +	};

Re: [PATCH 2/3] iio: adc: add STMPE ADC devicetree bindings

[Jonathan Cameron]

On Thu, 15 Nov 2018 10:38:03 +0100
Philippe Schenker <dev@pschenker.ch> wrote:

> On Sun, 2018-11-11 at 15:42 +0000, Jonathan Cameron wrote:
> > On Tue,  6 Nov 2018 17:41:15 +0100
> > Philippe Schenker <dev@pschenker.ch> wrote:
> >   
> > > From: Stefan Agner <stefan@agner.ch>
> > > 
> > > This adds the devicetree bindings for the STMPE ADC.
> > > 
> > > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > > Signed-off-by: Max Krummenacher <max.krummenacher@toradex.com>
> > > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > > ---
> > >  .../devicetree/bindings/iio/adc/stmpe-adc.txt | 34 +++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > > b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > > new file mode 100644
> > > index 000000000000..752ef35a794d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/stmpe-adc.txt
> > > @@ -0,0 +1,34 @@
> > > +STMPE ADC driver
> > > +----------------
> > > +
> > > +Required properties:
> > > + - compatible: "st,stmpe-adc"
> > > +
> > > +Optional properties:
> > > +Note that the ADC is shared with the STMPE touchscreen, so if using both
> > > the
> > > +settings should be the same.  
> > Ah, guessing there is already a binding in place for that which we need to
> > closely
> > match.  Should there be one overarching device with both as children though if
> > it is the same hardware?  
> 
> I moved ADC settings to the mfd...
> 
> >   
> > > +If the settings are not the same, the settings of the driver initialized
> > > last
> > > +will be active.
> > > +- st,sample-time: ADC conversion time in number of clock. (0 -> 36 clocks,
> > > +  1 -> 44 clocks, 2 -> 56 clocks, 3 -> 64 clocks, 4 -> 80 clocks,
> > > +  5 -> 96 clocks, 6 -> 144 clocks), recommended is 4.  
> > Could of questions here.  Is this a hardware dictated limit or is it dependent
> > on
> > application?  Basically I'm asking if it should be in userspace rather than
> > here.
> > 
> > Ideally I'd love to see it in a unit of time, but failing that can we have it
> > in clock cycles please rather than an enum?  Always nice if a binding is
> > human readable.  st,sample-clock-cycles would be a better name.
> >   
> 
> This is none of our design-decision. We did it the same way as it was already in
> drivers/input/touchscreen/stmpe-ts.c.
> 
> >   
> > > +- st,mod-12b: ADC Bit mode (0 -> 10bit ADC, 1 -> 12bit ADC)
> > > +- st,ref-sel: ADC reference source (0 -> internal reference, 1 -> external
> > > +  reference)  
> > 
> > If there is an external reference, I'd expect to see a regulator providing it.
> > Usually the mere presence of such an optional regulator does the job of saying
> > if
> > it should be used.  
> 
> Same here, we did it the same way as it was in stmpe-ts.c
> 
> > 
> >   
> > > +- st,adc-freq: ADC Clock speed (0 -> 1.625 MHz, 1 -> 3.25 MHz, 2 || 3 ->
> > > 6.5 MHz)  
> > So this is some internal clock?  What dictates the right setting?  I.e. how
> > should anyone know which one to pick?  
> 
> I think as a user of this driver at this point you need to read the datasheet
> anyway, to be able to decide which parameters you need for the application.
> 
> >   
> > > +- st,norequest-mask: bitmask specifying which ADC channels should _not_ be
> > > +  requestable due to different usage (e.g. touch)  
> > That's a little bit ugly.  Would prefer to see an explicit set of channels
> > brought out as children in DT.  There are some patches around standardising
> > some properties for doing that under review at the moment.
> > 
> > [PATCH v4 2/4] dt-bindings: iio: adc: Add common ADCs properties to a separate
> > file  
> 
> This is also implemented with the "norequest-mask" in stmpe-ts.c. That's why we
> did it the same way and I don't like to break others devicetree by changing
> stmpe-ts.c
> 
> >   
> > > +
> > > +Node name must be stmpe_adc and should be child node of stmpe node to
> > > +which it belongs.
> > > +
> > > +Example:
> > > +
> > > +	stmpe_adc {  
> > 
> > adc {
> > is the moderately standard naming for ADCS.  
> 
> This is none of our naming decisions please see line 1311 in drivers/mfd/stmpe.c
Fair enough on all these - we are stuck with less than ideal legacy :(

Jonathan

> 
> >   
> > > +		compatible = "st,stmpe-adc";
> > > +		st,sample-time = <4>;
> > > +		st,mod-12b = <1>;
> > > +		st,ref-sel = <0>;
> > > +		st,adc-freq = <1>;
> > > +		st,norequest-mask = <0x0F>; /* dont use ADC CH3-0 */
> > > +	};  
>