[PATCH 0/3] add support for Allwinner SoCs ADC

[PATCH 0/3] add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen controller
and a thermal sensor. The first four channels can be used either for the ADC or
the touchscreen and the fifth channel is used for the thermal sensor. We
currently have a driver for the two latter functions in
drivers/input/touchscreen/sun4i-ts.c but we don't have access to the ADC feature
at all.

This adds initial support for Allwinner SoCs ADC with all features. Yet, the
touchscreen is not implemented but will be added later. To switch between
touchscreen and ADC modes, you need to poke few bits in registers and
(de)activate an interrupt (pen-up).
A MFD is provided to let the input driver activate the pen-up interrupt through
virtual interrupt, poke few bits via regmap and read data from the ADC driver
while both (and iio_hwmon) are probed by the MFD.

There exists slight modifications between the different SoCs ADC like the
address of some registers and the scale and offset to apply to thermal sensor
raw values. These modifications are done by drivers on different
platform_device_id passed by the MFD when probing subdrivers.

This also modifies iio-hwmon to allow probe deferring when no iio channel is
found. Currently when no iio channel is found, the probing of iio-hwmon fails.
This is problematic when iio-hwmon probes before the iio driver could register
iio channels to share.

Quentin Schulz (3):
  mfd: add support for Allwinner SoCs ADC
  iio: adc: add support for Allwinner SoCs ADC
  hwmon: iio_hwmon: defer probe when no channel is found

 drivers/hwmon/iio_hwmon.c           |   5 +-
 drivers/iio/adc/Kconfig             |  12 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c   | 371 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig                 |  14 ++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 188 ++++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  14 ++
 8 files changed, 606 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
 create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
 create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

-- 
2.5.0

[PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. For now, only the ADC and the thermal
sensor drivers are probed by the MFD, the touchscreen controller support
will be added later.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/mfd/Kconfig                 |  14 +++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 188 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  14 +++
 4 files changed, 218 insertions(+)
 create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
 create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index eea61e3..1663db9 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
 	  by the probe function of this MFD driver according to a device tree
 	  property.
 
+config MFD_SUNXI_ADC
+	tristate "ADC MFD core driver for sunxi platforms"
+	select MFD_CORE
+	select REGMAP_MMIO
+	help
+	  Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
+	  This driver will only map the hardware interrupt and registers, you
+	  have to select individual drivers based on this MFD to be able to use
+	  the ADC or the thermal sensor. This will try to probe the ADC driver
+	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sunxi-gpadc-mfd.
+
 config MFD_ATMEL_HLCDC
 	tristate "Atmel HLCDC (High-end LCD Controller)"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 5eaa6465d..b280d0a 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -199,6 +199,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 
+obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
+
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
new file mode 100644
index 0000000..710e297
--- /dev/null
+++ b/drivers/mfd/sunxi-gpadc-mfd.c
@@ -0,0 +1,188 @@
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/mfd/sunxi-gpadc-mfd.h>
+#include <linux/mfd/core.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#define SUNXI_IRQ_FIFO_DATA	0
+#define SUNXI_IRQ_TEMP_DATA	1
+
+static struct resource adc_resources[] = {
+	{
+		.name	= "FIFO_DATA_PENDING",
+		.start	= SUNXI_IRQ_FIFO_DATA,
+		.end	= SUNXI_IRQ_FIFO_DATA,
+		.flags	= IORESOURCE_IRQ,
+	}, {
+		.name	= "TEMP_DATA_PENDING",
+		.start	= SUNXI_IRQ_TEMP_DATA,
+		.end	= SUNXI_IRQ_TEMP_DATA,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
+	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
+	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
+};
+
+static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
+	.name = "sunxi_gpadc_mfd_irq_chip",
+	.status_base = TP_INT_FIFOS,
+	.ack_base = TP_INT_FIFOS,
+	.mask_base = TP_INT_FIFOC,
+	.init_ack_masked = true,
+	.mask_invert = true,
+	.irqs = sunxi_gpadc_mfd_regmap_irq,
+	.num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
+	.num_regs = 1,
+};
+
+static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
+	{
+		.name	= "sun4i-a10-gpadc-iio",
+		.resources = adc_resources,
+		.num_resources = ARRAY_SIZE(adc_resources),
+	}, {
+		.name = "iio_hwmon",
+	}
+};
+
+static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
+	{
+		.name	= "sun5i-a13-gpadc-iio",
+		.resources = adc_resources,
+		.num_resources = ARRAY_SIZE(adc_resources),
+	}, {
+		.name = "iio_hwmon",
+	},
+};
+
+static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
+	{
+		.name	= "sun6i-a31-gpadc-iio",
+		.resources = adc_resources,
+		.num_resources = ARRAY_SIZE(adc_resources),
+	}, {
+		.name = "iio_hwmon",
+	},
+};
+
+static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
+	struct resource *mem = NULL;
+	unsigned int irq;
+	int ret;
+
+	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
+					   sizeof(*sunxi_gpadc_mfd_dev),
+					   GFP_KERNEL);
+	if (!sunxi_gpadc_mfd_dev)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
+		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
+
+	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
+	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
+
+	sunxi_gpadc_mfd_dev->regmap =
+		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
+				      sunxi_gpadc_mfd_dev->regs,
+				      &sunxi_gpadc_mfd_regmap_config);
+	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
+		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
+				  IRQF_ONESHOT, 0,
+				  &sunxi_gpadc_mfd_regmap_irq_chip,
+				  &sunxi_gpadc_mfd_dev->regmap_irqc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
+		return ret;
+	}
+
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "allwinner,sun4i-a10-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun4i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+	else if (of_device_is_compatible(pdev->dev.of_node,
+					 "allwinner,sun5i-a13-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun5i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+	else if (of_device_is_compatible(pdev->dev.of_node,
+					 "allwinner,sun6i-a31-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun6i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
+		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
+		mfd_remove_devices(&pdev->dev);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "successfully loaded\n");
+
+	return 0;
+}
+
+static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
+	unsigned int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	mfd_remove_devices(&pdev->dev);
+	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
+	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ts" },
+	{ .compatible = "allwinner,sun5i-a13-ts" },
+	{ .compatible = "allwinner,sun6i-a31-ts" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
+
+static struct platform_driver sunxi_gpadc_mfd_driver = {
+	.driver = {
+		.name = "sunxi-adc-mfd",
+		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
+	},
+	.probe = sunxi_gpadc_mfd_probe,
+	.remove = sunxi_gpadc_mfd_remove,
+};
+
+module_platform_driver(sunxi_gpadc_mfd_driver);
+
+MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
new file mode 100644
index 0000000..341f8c3
--- /dev/null
+++ b/include/linux/mfd/sunxi-gpadc-mfd.h
@@ -0,0 +1,14 @@
+#ifndef __SUNXI_GPADC_MFD__H__
+#define __SUNXI_GPADC_MFD__H__
+
+#define TP_INT_FIFOC            0x10
+#define TP_INT_FIFOS            0x14
+
+struct sunxi_gpadc_mfd_dev {
+	void __iomem			*regs;
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct regmap_irq_chip_data	*regmap_irqc;
+};
+
+#endif
-- 
2.5.0

Re: [PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Antoine Tenart]

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

Hello Quentin!

A few comments bellow:

On Tue, Jun 28, 2016 at 10:18:15AM +0200, Quentin Schulz wrote:
>
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..710e297
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,188 @@

You should add a license statement here.

> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

Headers are usually included in the alphabetical order.

[...]

> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
> new file mode 100644
> index 0000000..341f8c3
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,14 @@

You should also add a license statement here.

> +#ifndef __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define TP_INT_FIFOC            0x10
> +#define TP_INT_FIFOS            0x14
> +
> +struct sunxi_gpadc_mfd_dev {
> +	void __iomem			*regs;
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +};
> +
> +#endif

Thanks!

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Antoine Tenart]

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

Hello,

On Tue, Jun 28, 2016 at 10:18:15AM +0200, Quentin Schulz wrote:
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..710e297
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,188 @@

[...]

> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "allwinner,sun4i-a10-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun4i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun5i-a13-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun5i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun6i-a31-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun6i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +		mfd_remove_devices(&pdev->dev);

You don't need to explicitly call mfd_remove_devices() when
mfd_add_devices() fails. See:
http://lxr.free-electrons.com/source/drivers/mfd/mfd-core.c#L298

> +		return ret;
> +	}

Thanks,

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 28/06/16 09:18, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. For now, only the ADC and the thermal
> sensor drivers are probed by the MFD, the touchscreen controller support
> will be added later.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
The code looks fine to me. The 'controversial' bit of this is listing
iio-hwmon as an mfd child to get it to probe as a result of this being
present.  My immediately thought is that it should be separately
described in the devicetree and hence instantiated outside of this driver.

Perhaps you could describe your reasoning for doing it this way?

Thanks,

Jonathan
> ---
>  drivers/mfd/Kconfig                 |  14 +++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 188 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  14 +++
>  4 files changed, 218 insertions(+)
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index eea61e3..1663db9 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
>  	  by the probe function of this MFD driver according to a device tree
>  	  property.
>  
> +config MFD_SUNXI_ADC
> +	tristate "ADC MFD core driver for sunxi platforms"
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	help
> +	  Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC.
> +	  This driver will only map the hardware interrupt and registers, you
> +	  have to select individual drivers based on this MFD to be able to use
> +	  the ADC or the thermal sensor. This will try to probe the ADC driver
> +	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunxi-gpadc-mfd.
> +
>  config MFD_ATMEL_HLCDC
>  	tristate "Atmel HLCDC (High-end LCD Controller)"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5eaa6465d..b280d0a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -199,6 +199,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> +obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
> +
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC)	+= intel_soc_pmic_bxtwc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..710e297
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,188 @@
> +#include <linux/interrupt.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +#include <linux/mfd/core.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#define SUNXI_IRQ_FIFO_DATA	0
> +#define SUNXI_IRQ_TEMP_DATA	1
> +
> +static struct resource adc_resources[] = {
> +	{
> +		.name	= "FIFO_DATA_PENDING",
> +		.start	= SUNXI_IRQ_FIFO_DATA,
> +		.end	= SUNXI_IRQ_FIFO_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	}, {
> +		.name	= "TEMP_DATA_PENDING",
> +		.start	= SUNXI_IRQ_TEMP_DATA,
> +		.end	= SUNXI_IRQ_TEMP_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> +	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> +	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> +	.name = "sunxi_gpadc_mfd_irq_chip",
> +	.status_base = TP_INT_FIFOS,
> +	.ack_base = TP_INT_FIFOS,
> +	.mask_base = TP_INT_FIFOC,
> +	.init_ack_masked = true,
> +	.mask_invert = true,
> +	.irqs = sunxi_gpadc_mfd_regmap_irq,
> +	.num_irqs = ARRAY_SIZE(sunxi_gpadc_mfd_regmap_irq),
> +	.num_regs = 1,
> +};
> +
> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun4i-a10-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	}
> +};
> +
> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun5i-a13-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
> +	},
> +};
> +
> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = {
> +	{
> +		.name	= "sun6i-a31-gpadc-iio",
> +		.resources = adc_resources,
> +		.num_resources = ARRAY_SIZE(adc_resources),
> +	}, {
> +		.name = "iio_hwmon",
Interesting to probe this as part of the mfd as opposed to separately
describing it in the devicetree.
> +	},
> +};
> +
> +static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
> +	struct resource *mem = NULL;
> +	unsigned int irq;
> +	int ret;
> +
> +	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
> +					   sizeof(*sunxi_gpadc_mfd_dev),
> +					   GFP_KERNEL);
> +	if (!sunxi_gpadc_mfd_dev)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
> +		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
> +
> +	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
> +	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
> +
> +	sunxi_gpadc_mfd_dev->regmap =
> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
> +				      sunxi_gpadc_mfd_dev->regs,
> +				      &sunxi_gpadc_mfd_regmap_config);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
> +				  IRQF_ONESHOT, 0,
> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "allwinner,sun4i-a10-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun4i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun5i-a13-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun5i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun6i-a31-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun6i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +		mfd_remove_devices(&pdev->dev);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "successfully loaded\n");
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	unsigned int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	mfd_remove_devices(&pdev->dev);
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
> +	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-ts" },
> +	{ .compatible = "allwinner,sun5i-a13-ts" },
> +	{ .compatible = "allwinner,sun6i-a31-ts" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
> +
> +static struct platform_driver sunxi_gpadc_mfd_driver = {
> +	.driver = {
> +		.name = "sunxi-adc-mfd",
> +		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
> +	},
> +	.probe = sunxi_gpadc_mfd_probe,
> +	.remove = sunxi_gpadc_mfd_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_mfd_driver);
> +
> +MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
> new file mode 100644
> index 0000000..341f8c3
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,14 @@
> +#ifndef __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define TP_INT_FIFOC            0x10
> +#define TP_INT_FIFOS            0x14
> +
> +struct sunxi_gpadc_mfd_dev {
> +	void __iomem			*regs;
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +};
> +
> +#endif
> 

Re: [PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Lars-Peter Clausen]

On 07/03/2016 01:17 PM, Jonathan Cameron wrote:
> On 28/06/16 09:18, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. For now, only the ADC and the thermal
>> sensor drivers are probed by the MFD, the touchscreen controller support
>> will be added later.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> The code looks fine to me. The 'controversial' bit of this is listing
> iio-hwmon as an mfd child to get it to probe as a result of this being
> present.  My immediately thought is that it should be separately
> described in the devicetree and hence instantiated outside of this driver.

The devicetree is a generic description of the hardware. The iio-hwmon
bridge is a software component that translates between two Linux specific
ABIs. In my opinion putting the later in the former is makes no sense, it is
simply not part of the hardware description.

Its quite terrible that we have the bindings in the first place, but I guess
we have to keep them considering they are ABI and there are existing users.
But we should definitely strongly discourage the introduction of new users.

It is policy whether an application wants to access a device using the IIO
or hwmon API. As such it must be managed by userspace, this is not something
that can be done using devicetree nor should it be something that is done on
a driver by driver basis.

- Lars

Re: [PATCH 1/3] mfd: add support for Allwinner SoCs ADC

[Guenter Roeck]

On 07/03/2016 09:49 AM, Lars-Peter Clausen wrote:
> On 07/03/2016 01:17 PM, Jonathan Cameron wrote:
>> On 28/06/16 09:18, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. For now, only the ADC and the thermal
>>> sensor drivers are probed by the MFD, the touchscreen controller support
>>> will be added later.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> The code looks fine to me. The 'controversial' bit of this is listing
>> iio-hwmon as an mfd child to get it to probe as a result of this being
>> present.  My immediately thought is that it should be separately
>> described in the devicetree and hence instantiated outside of this driver.
>
> The devicetree is a generic description of the hardware. The iio-hwmon
> bridge is a software component that translates between two Linux specific
> ABIs. In my opinion putting the later in the former is makes no sense, it is
> simply not part of the hardware description.
>
Actually, this is where people get it wrong.

> Its quite terrible that we have the bindings in the first place, but I guess
> we have to keep them considering they are ABI and there are existing users.
> But we should definitely strongly discourage the introduction of new users.
>

I do agree that the _bindings_ are bad.

The ultimate problem is to find bindings which do describe the hardware
in a way that would be acceptable to the devicetree community and is at the
same time useful for software when trying to determine what to do with that
hardware. _This_ is the exceptionally hard problem.

One example would be an adc channel connected to a board voltage. I would assume
that it should be permissible to describe this relationship in a way that can
be _used_ by software to expose that adc channel as voltage, by whatever
means necessary (it be through hwmon or as a regulator or whatever).

Similar, some pin on a chip may be connected to a thermal sensor. I would
assume that it should be permissible to describe that thermal sensor (and its
location) in a way that can be _used_ by software in a meaningful way, either
for it to be reported as hardware monitoring attribute or to be used by the
thermal subsystem as a thermal input channel.

In addition to that, there are various other properties which _do_ describe
the hardware, but are commonly seen as "software". Examples for that would
be voltage or temperature limits (or any other limits, for that matter).
Such limits _are_ part of the hardware description, but are not commonly
accepted as such.

> It is policy whether an application wants to access a device using the IIO
> or hwmon API. As such it must be managed by userspace, this is not something
> that can be done using devicetree nor should it be something that is done on
> a driver by driver basis.
>

Agreed. However, the connections from one chip to another, and the use of a chip
on a board, _is_ part of the hardware description. It is determined by the
schematics as well as the board layout. A well defined hardware description
needs to provide more than "this is an ADC channel" or "this is a thermal sensor".

Guenter

[PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. This patch adds the ADC driver which is
based on the MFD for the same SoCs ADC.

This also registers the thermal adc channel in the iio map array so
iio_hwmon could use it without modifying the Device Tree.

This driver probes on three different platform_device_id to take into
account slight differences between Allwinner SoCs ADCs.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/iio/adc/Kconfig           |  12 ++
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c | 371 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 384 insertions(+)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 82c718c..b7b566a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -328,6 +328,18 @@ config NAU7802
 	  To compile this driver as a module, choose M here: the
 	  module will be called nau7802.
 
+config SUNXI_ADC
+	tristate "ADC driver for sunxi platforms"
+	depends on IIO
+	depends on MFD_SUNXI_ADC
+	help
+	  Say yes here to build support for Allwinner SoCs (A10, A13 and A31)
+	  SoCs ADC. This ADC provides 4 channels which can be used as an ADC or
+	  as a touchscreen input and one channel for thermal sensor.
+
+          To compile this driver as a module, choose M here: the
+          module will be called sunxi-gpadc-iio.
+
 config PALMAS_GPADC
 	tristate "TI Palmas General Purpose ADC"
 	depends on MFD_PALMAS
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 0cb7921..2996a5b 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
 obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
 obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
 obj-$(CONFIG_NAU7802) += nau7802.o
+obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
 obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
 obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
 obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
new file mode 100644
index 0000000..5840f43
--- /dev/null
+++ b/drivers/iio/adc/sunxi-gpadc-iio.c
@@ -0,0 +1,371 @@
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/mfd/sunxi-gpadc-mfd.h>
+#include <linux/regmap.h>
+
+#define TP_CTRL0			0x00
+#define TP_CTRL1			0x04
+#define TP_CTRL2			0x08
+#define TP_CTRL3			0x0c
+#define TP_TPR				0x18
+#define TP_CDAT				0x1c
+#define TEMP_DATA			0x20
+#define TP_DATA				0x24
+
+/* TP_CTRL0 bits */
+#define ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
+#define ADC_FIRST_DLY_MODE		BIT(23)
+#define ADC_CLK_SELECT			BIT(22)
+#define ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
+#define FS_DIV(x)			((x) << 16) /* 4 bits */
+#define T_ACQ(x)			((x) << 0)  /* 16 bits*/
+
+/* TP_CTRL1 bits */
+#define STYLUS_UP_DEBOUNCE(x)		((x) << 12) /* 8 bits */
+#define STYLUS_UP_DEBOUNCE_EN		BIT(9)
+#define TOUCH_PAN_CALI_EN		BIT(6)
+#define TP_DUAL_EN			BIT(5)
+#define TP_MODE_EN			BIT(4)
+#define TP_ADC_SELECT			BIT(3)
+#define ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
+
+/* TP_CTRL1 bits for sun6i SOCs */
+#define SUN6I_TOUCH_PAN_CALI_EN		BIT(7)
+#define SUN6I_TP_DUAL_EN		BIT(6)
+#define SUN6I_TP_MODE_EN		BIT(5)
+#define SUN6I_TP_ADC_SELECT		BIT(4)
+#define SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */
+
+/* TP_CTRL2 bits */
+#define TP_SENSITIVE_ADJUST(x)		((x) << 28) /* 4 bits */
+#define TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
+#define PRE_MEA_EN			BIT(24)
+#define PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits*/
+
+/* TP_CTRL3 bits */
+#define FILTER_EN			BIT(2)
+#define FILTER_TYPE(x)			((x) << 0)  /* 2 bits */
+
+/* TP_INT_FIFOC irq and fifo mask / control bits */
+#define TEMP_IRQ_EN			BIT(18)
+#define TP_OVERRUN_IRQ_EN		BIT(17)
+#define TP_DATA_IRQ_EN			BIT(16)
+#define TP_DATA_XY_CHANGE		BIT(13)
+#define TP_FIFO_TRIG_LEVEL(x)		((x) << 8)  /* 5 bits */
+#define TP_DATA_DRQ_EN			BIT(7)
+#define TP_FIFO_FLUSH			BIT(4)
+#define TP_UP_IRQ_EN			BIT(1)
+#define TP_DOWN_IRQ_EN			BIT(0)
+
+/* TP_INT_FIFOS irq and fifo status bits */
+#define TEMP_DATA_PENDING		BIT(18)
+#define FIFO_OVERRUN_PENDING		BIT(17)
+#define FIFO_DATA_PENDING		BIT(16)
+#define TP_IDLE_FLG			BIT(2)
+#define TP_UP_PENDING			BIT(1)
+#define TP_DOWN_PENDING			BIT(0)
+
+/* TP_TPR bits */
+#define TEMP_ENABLE(x)			((x) << 16)
+#define TEMP_PERIOD(x)			((x) << 0)  /*t = x * 256 * 16 / clkin*/
+
+#define ARCH_SUN4I			BIT(0)
+#define ARCH_SUN5I			BIT(1)
+#define ARCH_SUN6I			BIT(2)
+
+struct sunxi_gpadc_dev {
+	void __iomem			*regs;
+	struct completion		completion;
+	int				temp_data;
+	u32				adc_data;
+	struct regmap			*regmap;
+	unsigned int			fifo_data_irq;
+	unsigned int			temp_data_irq;
+	unsigned int			flags;
+};
+
+#define ADC_CHANNEL(_channel, _name) {				\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.datasheet_name = _name,				\
+}
+
+static struct iio_map sunxi_gpadc_hwmon_maps[] = {
+	{
+		.adc_channel_label = "temp_adc",
+		.consumer_dev_name = "iio_hwmon.0",
+	},
+	{},
+};
+
+static const struct iio_chan_spec sunxi_gpadc_channels[] = {
+	ADC_CHANNEL(0, "adc_chan0"),
+	ADC_CHANNEL(1, "adc_chan1"),
+	ADC_CHANNEL(2, "adc_chan2"),
+	ADC_CHANNEL(3, "adc_chan3"),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.datasheet_name = "temp_adc",
+	},
+};
+
+static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+	int val = 0;
+
+	mutex_lock(&indio_dev->mlock);
+
+	reinit_completion(&info->completion);
+	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
+					     SUN6I_TP_ADC_SELECT |
+					     SUN6I_ADC_CHAN_SELECT(channel));
+	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
+						 TP_FIFO_FLUSH);
+	enable_irq(info->fifo_data_irq);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 msecs_to_jiffies(100))) {
+		disable_irq(info->fifo_data_irq);
+		mutex_unlock(&indio_dev->mlock);
+		return -ETIMEDOUT;
+	}
+
+	val = info->adc_data;
+	disable_irq(info->fifo_data_irq);
+	mutex_unlock(&indio_dev->mlock);
+
+	return val;
+}
+
+static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+	int val = 0;
+
+	mutex_lock(&indio_dev->mlock);
+
+	reinit_completion(&info->completion);
+
+	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
+						 TP_FIFO_FLUSH);
+	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
+	enable_irq(info->temp_data_irq);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 msecs_to_jiffies(100))) {
+		disable_irq(info->temp_data_irq);
+		mutex_unlock(&indio_dev->mlock);
+		return -ETIMEDOUT;
+	}
+
+	if (info->flags & ARCH_SUN4I)
+		val = info->temp_data * 133 - 257000;
+	else if (info->flags & ARCH_SUN5I)
+		val = info->temp_data * 100 - 144700;
+	else if (info->flags & ARCH_SUN6I)
+		val = info->temp_data * 167 - 271000;
+
+	disable_irq(info->temp_data_irq);
+	mutex_unlock(&indio_dev->mlock);
+	return val;
+}
+
+static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		*val = sunxi_gpadc_temp_read(indio_dev);
+		if (*val < 0)
+			return *val;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_RAW:
+		*val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
+		if (*val < 0)
+			return *val;
+
+		return IIO_VAL_INT;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info sunxi_gpadc_iio_info = {
+	.read_raw = sunxi_gpadc_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
+{
+	struct sunxi_gpadc_dev *info = dev_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
+	if (ret == 0)
+		complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
+{
+	struct sunxi_gpadc_dev *info = dev_id;
+	int ret;
+
+	ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
+	if (ret == 0)
+		complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_gpadc_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_dev *info = NULL;
+	struct iio_dev *indio_dev = NULL;
+	int ret = 0;
+	unsigned int irq;
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
+
+	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev) {
+		dev_err(&pdev->dev, "failed to allocate iio device.\n");
+		return -ENOMEM;
+	}
+	info = iio_priv(indio_dev);
+
+	info->regmap = sunxi_gpadc_mfd_dev->regmap;
+	init_completion(&info->completion);
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &sunxi_gpadc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
+	indio_dev->channels = sunxi_gpadc_channels;
+
+	info->flags = platform_get_device_id(pdev)->driver_data;
+
+	regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
+					     FS_DIV(7) |
+					     T_ACQ(63));
+	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
+	regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
+	regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) | TEMP_PERIOD(1953));
+
+	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"no TEMP_DATA_PENDING interrupt registered\n");
+		return irq;
+	}
+
+	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
+	ret = devm_request_any_context_irq(&pdev->dev, irq,
+					   sunxi_gpadc_temp_data_irq_handler,
+					   0, "temp_data", info);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"could not request TEMP_DATA_PENDING interrupt: %d\n",
+			ret);
+		return ret;
+	}
+
+	info->temp_data_irq = irq;
+	disable_irq(irq);
+
+	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"no FIFO_DATA_PENDING interrupt registered\n");
+		return irq;
+	}
+
+	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
+	ret = devm_request_any_context_irq(&pdev->dev, irq,
+					   sunxi_gpadc_fifo_data_irq_handler,
+					   0, "fifo_data", info);
+	if (ret < 0) {
+		dev_err(&pdev->dev,
+			"could not request FIFO_DATA_PENDING interrupt: %d\n",
+			ret);
+		return ret;
+	}
+
+	info->fifo_data_irq = irq;
+	disable_irq(irq);
+
+	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register iio map array\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not register the device\n");
+		iio_map_array_unregister(indio_dev);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "successfully loaded\n");
+
+	return ret;
+}
+
+static int sunxi_gpadc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = NULL;
+	struct sunxi_gpadc_dev *info = NULL;
+
+	iio_device_unregister(indio_dev);
+	iio_map_array_unregister(indio_dev);
+	info = iio_priv(indio_dev);
+	regmap_write(info->regmap, TP_INT_FIFOC, 0);
+
+	return 0;
+}
+
+static const struct platform_device_id sunxi_gpadc_id[] = {
+	{ "sun4i-a10-gpadc-iio", ARCH_SUN4I },
+	{ "sun5i-a13-gpadc-iio", ARCH_SUN5I },
+	{ "sun6i-a31-gpadc-iio", ARCH_SUN6I },
+	{ /*sentinel*/ },
+};
+
+static struct platform_driver sunxi_gpadc_driver = {
+	.driver = {
+		.name = "sunxi-gpadc-iio",
+	},
+	.id_table = sunxi_gpadc_id,
+	.probe = sunxi_gpadc_probe,
+	.remove = sunxi_gpadc_remove,
+};
+
+module_platform_driver(sunxi_gpadc_driver);
+
+MODULE_DESCRIPTION("ADC driver for sunxi platforms");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.5.0

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Antoine Tenart]

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

Hello Quentin!

On Tue, Jun 28, 2016 at 10:18:16AM +0200, Quentin Schulz wrote:
>
> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
> new file mode 100644
> index 0000000..5840f43
> --- /dev/null
> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
> @@ -0,0 +1,371 @@

Same here, you can add a license statement :-)

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +#include <linux/regmap.h>

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 28/06/16 09:18, Quentin Schulz wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen
> controller and a thermal sensor. This patch adds the ADC driver which is
> based on the MFD for the same SoCs ADC.
> 
> This also registers the thermal adc channel in the iio map array so
> iio_hwmon could use it without modifying the Device Tree.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
Hi Quentin.

I'm a bit in two minds about some of this.  That temperature sensor is
so obviously meant for hwmon purposes, I'm tempted to suggest it might
actually make sense to put it directly in hwmon rather than using the
bridge.  That obviously makes it less flexible in some ways (i.e. for
use within the thermal subsystem at some point).

Guenter, what do you think?

I'm guessing detailed docs for this part aren't avaiable publicly? :(

So the rest of my comments are kind of predicated on me having roughtly
understood how this device works from the structure of the driver.

The temperature sensor is really effectively as separate ADC?
The main interest in this is for key detection?

Anyhow, if the data flow for the temperatures sensor is not synced with
the other ADC channels, adding buffered (pushed) output from the driver in
future will be fiddly and with a 250Hz device you'll probably want it.
Basically IIO buffered supports assumes each iio device will sample data
at a particular frequency. If channels are not synchronized in that fashion
then you have to register multiple devices or only pick a subset of channels
to export.

For the key detection you have already observed that IIO needs some
additions to be able to have consumers of what we term 'events' e.g. threshold
interrupts.

Looking at the lradc-keys driver in tree, it looks like we only really have
really simple threshold interrups - configured to detect a very low voltage?
+ only one per channel.

So not too nasty a case, but you are right some work is needed in IIO as
we simply don't have a means of passing these on as yet or configuring them
from in kernel consumers.
If we take the easy route and don't demux incoming events then it shouldn't
be too hard to add (demux can follow later).  Hence any client device can try
to enable events it wants, but may get events that other client devices wanted
as well.

Config interface should be much the same as the write support for channels.
Data flow marginally harder, but pretty much a list of callbacks within
iio_push_event.

Not trivial, but not too tricky either.

The events subsystem has a few 'limitations' we need to address long term
but as this is in kernel interface only, we can do this now and fix stuff
up in future without any ABI breakage. (limitations are things like only
one event of a given type and direction per channel - main challenge on
that is finding a way of doing it without abi breakage).

Anyhow, sounds fun - wish I had the time to do it myself!

Otherwise, your remove is never going to work as indio_dev is always NULL.

Jonathan

> ---
>  drivers/iio/adc/Kconfig           |  12 ++
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 371 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 384 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 82c718c..b7b566a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -328,6 +328,18 @@ config NAU7802
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nau7802.
>  
> +config SUNXI_ADC
> +	tristate "ADC driver for sunxi platforms"
> +	depends on IIO
> +	depends on MFD_SUNXI_ADC
> +	help
> +	  Say yes here to build support for Allwinner SoCs (A10, A13 and A31)
> +	  SoCs ADC. This ADC provides 4 channels which can be used as an ADC or
> +	  as a touchscreen input and one channel for thermal sensor.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called sunxi-gpadc-iio.
> +
>  config PALMAS_GPADC
>  	tristate "TI Palmas General Purpose ADC"
>  	depends on MFD_PALMAS
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 0cb7921..2996a5b 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>  obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>  obj-$(CONFIG_NAU7802) += nau7802.o
> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
> new file mode 100644
> index 0000000..5840f43
> --- /dev/null
> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
> @@ -0,0 +1,371 @@
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +#include <linux/regmap.h>
> +
I'm fussy about this:  Please prefix all defines as some of these
are generic enough they may well end up defined in an included header
sometime in the future.

> +#define TP_CTRL0			0x00
> +#define TP_CTRL1			0x04
> +#define TP_CTRL2			0x08
> +#define TP_CTRL3			0x0c
> +#define TP_TPR				0x18
> +#define TP_CDAT				0x1c
> +#define TEMP_DATA			0x20
> +#define TP_DATA				0x24
> +
> +/* TP_CTRL0 bits */
> +#define ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
> +#define ADC_FIRST_DLY_MODE		BIT(23)
> +#define ADC_CLK_SELECT			BIT(22)
> +#define ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
> +#define FS_DIV(x)			((x) << 16) /* 4 bits */
> +#define T_ACQ(x)			((x) << 0)  /* 16 bits*/
> +
> +/* TP_CTRL1 bits */
> +#define STYLUS_UP_DEBOUNCE(x)		((x) << 12) /* 8 bits */
> +#define STYLUS_UP_DEBOUNCE_EN		BIT(9)
> +#define TOUCH_PAN_CALI_EN		BIT(6)
> +#define TP_DUAL_EN			BIT(5)
> +#define TP_MODE_EN			BIT(4)
> +#define TP_ADC_SELECT			BIT(3)
> +#define ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
> +
> +/* TP_CTRL1 bits for sun6i SOCs */
> +#define SUN6I_TOUCH_PAN_CALI_EN		BIT(7)
> +#define SUN6I_TP_DUAL_EN		BIT(6)
> +#define SUN6I_TP_MODE_EN		BIT(5)
> +#define SUN6I_TP_ADC_SELECT		BIT(4)
> +#define SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */
> +
> +/* TP_CTRL2 bits */
> +#define TP_SENSITIVE_ADJUST(x)		((x) << 28) /* 4 bits */
> +#define TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
> +#define PRE_MEA_EN			BIT(24)
> +#define PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits*/
> +
> +/* TP_CTRL3 bits */
> +#define FILTER_EN			BIT(2)
> +#define FILTER_TYPE(x)			((x) << 0)  /* 2 bits */
> +
> +/* TP_INT_FIFOC irq and fifo mask / control bits */
> +#define TEMP_IRQ_EN			BIT(18)
> +#define TP_OVERRUN_IRQ_EN		BIT(17)
> +#define TP_DATA_IRQ_EN			BIT(16)
> +#define TP_DATA_XY_CHANGE		BIT(13)
> +#define TP_FIFO_TRIG_LEVEL(x)		((x) << 8)  /* 5 bits */
> +#define TP_DATA_DRQ_EN			BIT(7)
> +#define TP_FIFO_FLUSH			BIT(4)
> +#define TP_UP_IRQ_EN			BIT(1)
> +#define TP_DOWN_IRQ_EN			BIT(0)
> +
> +/* TP_INT_FIFOS irq and fifo status bits */
> +#define TEMP_DATA_PENDING		BIT(18)
> +#define FIFO_OVERRUN_PENDING		BIT(17)
> +#define FIFO_DATA_PENDING		BIT(16)
> +#define TP_IDLE_FLG			BIT(2)
> +#define TP_UP_PENDING			BIT(1)
> +#define TP_DOWN_PENDING			BIT(0)
> +
> +/* TP_TPR bits */
> +#define TEMP_ENABLE(x)			((x) << 16)
> +#define TEMP_PERIOD(x)			((x) << 0)  /*t = x * 256 * 16 / clkin*/
> +
> +#define ARCH_SUN4I			BIT(0)
> +#define ARCH_SUN5I			BIT(1)
> +#define ARCH_SUN6I			BIT(2)
> +
> +struct sunxi_gpadc_dev {
> +	void __iomem			*regs;
> +	struct completion		completion;
> +	int				temp_data;
> +	u32				adc_data;
> +	struct regmap			*regmap;
> +	unsigned int			fifo_data_irq;
> +	unsigned int			temp_data_irq;
> +	unsigned int			flags;
> +};
> +
> +#define ADC_CHANNEL(_channel, _name) {				\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.datasheet_name = _name,				\
> +}
> +
> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
> +	{
> +		.adc_channel_label = "temp_adc",
> +		.consumer_dev_name = "iio_hwmon.0",
> +	},
> +	{},
> +};
> +
> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
> +	ADC_CHANNEL(0, "adc_chan0"),
> +	ADC_CHANNEL(1, "adc_chan1"),
> +	ADC_CHANNEL(2, "adc_chan2"),
> +	ADC_CHANNEL(3, "adc_chan3"),
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.datasheet_name = "temp_adc",
> +	},
> +};
> +
> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int val = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	reinit_completion(&info->completion);
> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
> +					     SUN6I_TP_ADC_SELECT |
> +					     SUN6I_ADC_CHAN_SELECT(channel));
> +	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
> +						 TP_FIFO_FLUSH);
> +	enable_irq(info->fifo_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		disable_irq(info->fifo_data_irq);
> +		mutex_unlock(&indio_dev->mlock);
> +		return -ETIMEDOUT;
> +	}
> +
> +	val = info->adc_data;
> +	disable_irq(info->fifo_data_irq);
> +	mutex_unlock(&indio_dev->mlock);
> +
> +	return val;
> +}
> +
> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int val = 0;
> +
> +	mutex_lock(&indio_dev->mlock);
> +
> +	reinit_completion(&info->completion);
> +
> +	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
> +						 TP_FIFO_FLUSH);
> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
> +	enable_irq(info->temp_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		disable_irq(info->temp_data_irq);
> +		mutex_unlock(&indio_dev->mlock);
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (info->flags & ARCH_SUN4I)
> +		val = info->temp_data * 133 - 257000;
> +	else if (info->flags & ARCH_SUN5I)
> +		val = info->temp_data * 100 - 144700;
> +	else if (info->flags & ARCH_SUN6I)
> +		val = info->temp_data * 167 - 271000;
> +
> +	disable_irq(info->temp_data_irq);
> +	mutex_unlock(&indio_dev->mlock);
> +	return val;
> +}
> +
> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		*val = sunxi_gpadc_temp_read(indio_dev);
> +		if (*val < 0)
> +			return *val;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		*val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
> +		if (*val < 0)
> +			return *val;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info sunxi_gpadc_iio_info = {
> +	.read_raw = sunxi_gpadc_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
> +{
> +	struct sunxi_gpadc_dev *info = dev_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
> +	if (ret == 0)
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
Interesting that it's a separate data flow for the temperature sensor.
That means that if you add buffered (pushed data) support in future either
you'll need to split the device in two or not allow the temperature channel
to go through the buffer interface.
> +
> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
> +{
> +	struct sunxi_gpadc_dev *info = dev_id;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
> +	if (ret == 0)
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info = NULL;
> +	struct iio_dev *indio_dev = NULL;
> +	int ret = 0;
> +	unsigned int irq;
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "failed to allocate iio device.\n");
> +		return -ENOMEM;
> +	}
> +	info = iio_priv(indio_dev);
> +
> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
> +	init_completion(&info->completion);
> +	indio_dev->name = dev_name(&pdev->dev);
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->dev.of_node = pdev->dev.of_node;
> +	indio_dev->info = &sunxi_gpadc_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
> +	indio_dev->channels = sunxi_gpadc_channels;
> +
> +	info->flags = platform_get_device_id(pdev)->driver_data;
> +
> +	regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
> +					     FS_DIV(7) |
> +					     T_ACQ(63));
> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
> +	regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
> +	regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) | TEMP_PERIOD(1953));
> +
> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no TEMP_DATA_PENDING interrupt registered\n");
> +		return irq;
> +	}
> +
> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> +					   sunxi_gpadc_temp_data_irq_handler,
> +					   0, "temp_data", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	info->temp_data_irq = irq;
> +	disable_irq(irq);
> +
> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
hohum.  A fifo?  This part is getting more interesting ;) I'll have to
dig out the datasheet at some point (if public).
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no FIFO_DATA_PENDING interrupt registered\n");
> +		return irq;
> +	}
> +
> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
> +					   sunxi_gpadc_fifo_data_irq_handler,
> +					   0, "fifo_data", info);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev,
> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	info->fifo_data_irq = irq;
> +	disable_irq(irq);
> +
> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
As mentioned for previous patch I think this should be described externally.
Chances are that some of those other adc channels are also going to be
in reality used for hwmon anyway so doing it in the device tree will give
you more flexibility.

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register iio map array\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "could not register the device\n");
> +		iio_map_array_unregister(indio_dev);
> +		return ret;
> +	}
> +
This is kind of self evident when the device turns up so I'd not bother
cluttering up the logs with it as no additional information is given.
> +	dev_info(&pdev->dev, "successfully loaded\n");
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = NULL;
?  If it's null the below isn't going to work as intended.
Missing a platform_get_drvdata which I'd just roll into the above
local variable definition.

> +	struct sunxi_gpadc_dev *info = NULL;
> +
> +	iio_device_unregister(indio_dev);
> +	iio_map_array_unregister(indio_dev);
> +	info = iio_priv(indio_dev);
I'd roll this into the local variable declaration above
(it's only a trivial bit of pointer arithemetic after all.
struct sunxi_gpadc_dev *info = iio_priv(indio_dev);

> +	regmap_write(info->regmap, TP_INT_FIFOC, 0);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id sunxi_gpadc_id[] = {
> +	{ "sun4i-a10-gpadc-iio", ARCH_SUN4I },
> +	{ "sun5i-a13-gpadc-iio", ARCH_SUN5I },
> +	{ "sun6i-a31-gpadc-iio", ARCH_SUN6I },
> +	{ /*sentinel*/ },
> +};
> +
> +static struct platform_driver sunxi_gpadc_driver = {
> +	.driver = {
> +		.name = "sunxi-gpadc-iio",
> +	},
> +	.id_table = sunxi_gpadc_id,
> +	.probe = sunxi_gpadc_probe,
> +	.remove = sunxi_gpadc_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_driver);
> +
> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 03/07/16 12:54, Jonathan Cameron wrote:
> On 28/06/16 09:18, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> Hi Quentin.
> 
> I'm a bit in two minds about some of this.  That temperature sensor is
> so obviously meant for hwmon purposes, I'm tempted to suggest it might
> actually make sense to put it directly in hwmon rather than using the
> bridge.  That obviously makes it less flexible in some ways (i.e. for
> use within the thermal subsystem at some point).
> 
> Guenter, what do you think?
> 
> I'm guessing detailed docs for this part aren't avaiable publicly? :(
> 
> So the rest of my comments are kind of predicated on me having roughtly
> understood how this device works from the structure of the driver.
> 
> The temperature sensor is really effectively as separate ADC?
> The main interest in this is for key detection?
Ah, I'm talking garbage, wrong ADC for the chip...  I'd crossed the two
threads.  Ignore that stuff.
> 
> Anyhow, if the data flow for the temperatures sensor is not synced with
> the other ADC channels, adding buffered (pushed) output from the driver in
> future will be fiddly and with a 250Hz device you'll probably want it.
> Basically IIO buffered supports assumes each iio device will sample data
> at a particular frequency. If channels are not synchronized in that fashion
> then you have to register multiple devices or only pick a subset of channels
> to export.
> 
> For the key detection you have already observed that IIO needs some
> additions to be able to have consumers of what we term 'events' e.g. threshold
> interrupts.
> 
> Looking at the lradc-keys driver in tree, it looks like we only really have
> really simple threshold interrups - configured to detect a very low voltage?
> + only one per channel.
> 
> So not too nasty a case, but you are right some work is needed in IIO as
> we simply don't have a means of passing these on as yet or configuring them
> from in kernel consumers.
> If we take the easy route and don't demux incoming events then it shouldn't
> be too hard to add (demux can follow later).  Hence any client device can try
> to enable events it wants, but may get events that other client devices wanted
> as well.
> 
> Config interface should be much the same as the write support for channels.
> Data flow marginally harder, but pretty much a list of callbacks within
> iio_push_event.
> 
> Not trivial, but not too tricky either.
> 
> The events subsystem has a few 'limitations' we need to address long term
> but as this is in kernel interface only, we can do this now and fix stuff
> up in future without any ABI breakage. (limitations are things like only
> one event of a given type and direction per channel - main challenge on
> that is finding a way of doing it without abi breakage).
> 
> Anyhow, sounds fun - wish I had the time to do it myself!
> 
> Otherwise, your remove is never going to work as indio_dev is always NULL.
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/Kconfig           |  12 ++
>>  drivers/iio/adc/Makefile          |   1 +
>>  drivers/iio/adc/sunxi-gpadc-iio.c | 371 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 384 insertions(+)
>>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 82c718c..b7b566a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -328,6 +328,18 @@ config NAU7802
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called nau7802.
>>  
>> +config SUNXI_ADC
>> +	tristate "ADC driver for sunxi platforms"
>> +	depends on IIO
>> +	depends on MFD_SUNXI_ADC
>> +	help
>> +	  Say yes here to build support for Allwinner SoCs (A10, A13 and A31)
>> +	  SoCs ADC. This ADC provides 4 channels which can be used as an ADC or
>> +	  as a touchscreen input and one channel for thermal sensor.
>> +
>> +          To compile this driver as a module, choose M here: the
>> +          module will be called sunxi-gpadc-iio.
>> +
>>  config PALMAS_GPADC
>>  	tristate "TI Palmas General Purpose ADC"
>>  	depends on MFD_PALMAS
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 0cb7921..2996a5b 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>>  obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>>  obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>>  obj-$(CONFIG_NAU7802) += nau7802.o
>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>>  obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>>  obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>>  obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
>> new file mode 100644
>> index 0000000..5840f43
>> --- /dev/null
>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
>> @@ -0,0 +1,371 @@
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>> +#include <linux/regmap.h>
>> +
> I'm fussy about this:  Please prefix all defines as some of these
> are generic enough they may well end up defined in an included header
> sometime in the future.
> 
>> +#define TP_CTRL0			0x00
>> +#define TP_CTRL1			0x04
>> +#define TP_CTRL2			0x08
>> +#define TP_CTRL3			0x0c
>> +#define TP_TPR				0x18
>> +#define TP_CDAT				0x1c
>> +#define TEMP_DATA			0x20
>> +#define TP_DATA				0x24
>> +
>> +/* TP_CTRL0 bits */
>> +#define ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
>> +#define ADC_FIRST_DLY_MODE		BIT(23)
>> +#define ADC_CLK_SELECT			BIT(22)
>> +#define ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
>> +#define FS_DIV(x)			((x) << 16) /* 4 bits */
>> +#define T_ACQ(x)			((x) << 0)  /* 16 bits*/
>> +
>> +/* TP_CTRL1 bits */
>> +#define STYLUS_UP_DEBOUNCE(x)		((x) << 12) /* 8 bits */
>> +#define STYLUS_UP_DEBOUNCE_EN		BIT(9)
>> +#define TOUCH_PAN_CALI_EN		BIT(6)
>> +#define TP_DUAL_EN			BIT(5)
>> +#define TP_MODE_EN			BIT(4)
>> +#define TP_ADC_SELECT			BIT(3)
>> +#define ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
>> +
>> +/* TP_CTRL1 bits for sun6i SOCs */
>> +#define SUN6I_TOUCH_PAN_CALI_EN		BIT(7)
>> +#define SUN6I_TP_DUAL_EN		BIT(6)
>> +#define SUN6I_TP_MODE_EN		BIT(5)
>> +#define SUN6I_TP_ADC_SELECT		BIT(4)
>> +#define SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */
>> +
>> +/* TP_CTRL2 bits */
>> +#define TP_SENSITIVE_ADJUST(x)		((x) << 28) /* 4 bits */
>> +#define TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
>> +#define PRE_MEA_EN			BIT(24)
>> +#define PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits*/
>> +
>> +/* TP_CTRL3 bits */
>> +#define FILTER_EN			BIT(2)
>> +#define FILTER_TYPE(x)			((x) << 0)  /* 2 bits */
>> +
>> +/* TP_INT_FIFOC irq and fifo mask / control bits */
>> +#define TEMP_IRQ_EN			BIT(18)
>> +#define TP_OVERRUN_IRQ_EN		BIT(17)
>> +#define TP_DATA_IRQ_EN			BIT(16)
>> +#define TP_DATA_XY_CHANGE		BIT(13)
>> +#define TP_FIFO_TRIG_LEVEL(x)		((x) << 8)  /* 5 bits */
>> +#define TP_DATA_DRQ_EN			BIT(7)
>> +#define TP_FIFO_FLUSH			BIT(4)
>> +#define TP_UP_IRQ_EN			BIT(1)
>> +#define TP_DOWN_IRQ_EN			BIT(0)
>> +
>> +/* TP_INT_FIFOS irq and fifo status bits */
>> +#define TEMP_DATA_PENDING		BIT(18)
>> +#define FIFO_OVERRUN_PENDING		BIT(17)
>> +#define FIFO_DATA_PENDING		BIT(16)
>> +#define TP_IDLE_FLG			BIT(2)
>> +#define TP_UP_PENDING			BIT(1)
>> +#define TP_DOWN_PENDING			BIT(0)
>> +
>> +/* TP_TPR bits */
>> +#define TEMP_ENABLE(x)			((x) << 16)
>> +#define TEMP_PERIOD(x)			((x) << 0)  /*t = x * 256 * 16 / clkin*/
>> +
>> +#define ARCH_SUN4I			BIT(0)
>> +#define ARCH_SUN5I			BIT(1)
>> +#define ARCH_SUN6I			BIT(2)
>> +
>> +struct sunxi_gpadc_dev {
>> +	void __iomem			*regs;
>> +	struct completion		completion;
>> +	int				temp_data;
>> +	u32				adc_data;
>> +	struct regmap			*regmap;
>> +	unsigned int			fifo_data_irq;
>> +	unsigned int			temp_data_irq;
>> +	unsigned int			flags;
>> +};
>> +
>> +#define ADC_CHANNEL(_channel, _name) {				\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.channel = _channel,					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>> +	.datasheet_name = _name,				\
>> +}
>> +
>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
>> +	{
>> +		.adc_channel_label = "temp_adc",
>> +		.consumer_dev_name = "iio_hwmon.0",
>> +	},
>> +	{},
>> +};
>> +
>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>> +	ADC_CHANNEL(0, "adc_chan0"),
>> +	ADC_CHANNEL(1, "adc_chan1"),
>> +	ADC_CHANNEL(2, "adc_chan2"),
>> +	ADC_CHANNEL(3, "adc_chan3"),
>> +	{
>> +		.type = IIO_TEMP,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.datasheet_name = "temp_adc",
>> +	},
>> +};
>> +
>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int val = 0;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +
>> +	reinit_completion(&info->completion);
>> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
>> +					     SUN6I_TP_ADC_SELECT |
>> +					     SUN6I_ADC_CHAN_SELECT(channel));
>> +	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>> +						 TP_FIFO_FLUSH);
>> +	enable_irq(info->fifo_data_irq);
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		disable_irq(info->fifo_data_irq);
>> +		mutex_unlock(&indio_dev->mlock);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	val = info->adc_data;
>> +	disable_irq(info->fifo_data_irq);
>> +	mutex_unlock(&indio_dev->mlock);
>> +
>> +	return val;
>> +}
>> +
>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int val = 0;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +
>> +	reinit_completion(&info->completion);
>> +
>> +	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>> +						 TP_FIFO_FLUSH);
>> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>> +	enable_irq(info->temp_data_irq);
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		disable_irq(info->temp_data_irq);
>> +		mutex_unlock(&indio_dev->mlock);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	if (info->flags & ARCH_SUN4I)
>> +		val = info->temp_data * 133 - 257000;
>> +	else if (info->flags & ARCH_SUN5I)
>> +		val = info->temp_data * 100 - 144700;
>> +	else if (info->flags & ARCH_SUN6I)
>> +		val = info->temp_data * 167 - 271000;
>> +
>> +	disable_irq(info->temp_data_irq);
>> +	mutex_unlock(&indio_dev->mlock);
>> +	return val;
>> +}
>> +
>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
>> +				struct iio_chan_spec const *chan,
>> +				int *val, int *val2, long mask)
>> +{
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		*val = sunxi_gpadc_temp_read(indio_dev);
>> +		if (*val < 0)
>> +			return *val;
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_RAW:
>> +		*val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
>> +		if (*val < 0)
>> +			return *val;
>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info sunxi_gpadc_iio_info = {
>> +	.read_raw = sunxi_gpadc_read_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sunxi_gpadc_dev *info = dev_id;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
>> +	if (ret == 0)
>> +		complete(&info->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
> Interesting that it's a separate data flow for the temperature sensor.
> That means that if you add buffered (pushed data) support in future either
> you'll need to split the device in two or not allow the temperature channel
> to go through the buffer interface.
>> +
>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sunxi_gpadc_dev *info = dev_id;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
>> +	if (ret == 0)
>> +		complete(&info->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int sunxi_gpadc_probe(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_dev *info = NULL;
>> +	struct iio_dev *indio_dev = NULL;
>> +	int ret = 0;
>> +	unsigned int irq;
>> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
>> +
>> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>> +	if (!indio_dev) {
>> +		dev_err(&pdev->dev, "failed to allocate iio device.\n");
>> +		return -ENOMEM;
>> +	}
>> +	info = iio_priv(indio_dev);
>> +
>> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
>> +	init_completion(&info->completion);
>> +	indio_dev->name = dev_name(&pdev->dev);
>> +	indio_dev->dev.parent = &pdev->dev;
>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>> +	indio_dev->info = &sunxi_gpadc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
>> +	indio_dev->channels = sunxi_gpadc_channels;
>> +
>> +	info->flags = platform_get_device_id(pdev)->driver_data;
>> +
>> +	regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
>> +					     FS_DIV(7) |
>> +					     T_ACQ(63));
>> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>> +	regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
>> +	regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) | TEMP_PERIOD(1953));
>> +
>> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no TEMP_DATA_PENDING interrupt registered\n");
>> +		return irq;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_temp_data_irq_handler,
>> +					   0, "temp_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	info->temp_data_irq = irq;
>> +	disable_irq(irq);
>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> hohum.  A fifo?  This part is getting more interesting ;) I'll have to
> dig out the datasheet at some point (if public).
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		return irq;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_fifo_data_irq_handler,
>> +					   0, "fifo_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	info->fifo_data_irq = irq;
>> +	disable_irq(irq);
>> +
>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
> As mentioned for previous patch I think this should be described externally.
> Chances are that some of those other adc channels are also going to be
> in reality used for hwmon anyway so doing it in the device tree will give
> you more flexibility.
> 
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "could not register the device\n");
>> +		iio_map_array_unregister(indio_dev);
>> +		return ret;
>> +	}
>> +
> This is kind of self evident when the device turns up so I'd not bother
> cluttering up the logs with it as no additional information is given.
>> +	dev_info(&pdev->dev, "successfully loaded\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = NULL;
> ?  If it's null the below isn't going to work as intended.
> Missing a platform_get_drvdata which I'd just roll into the above
> local variable definition.
> 
>> +	struct sunxi_gpadc_dev *info = NULL;
>> +
>> +	iio_device_unregister(indio_dev);
>> +	iio_map_array_unregister(indio_dev);
>> +	info = iio_priv(indio_dev);
> I'd roll this into the local variable declaration above
> (it's only a trivial bit of pointer arithemetic after all.
> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> 
>> +	regmap_write(info->regmap, TP_INT_FIFOC, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>> +	{ "sun4i-a10-gpadc-iio", ARCH_SUN4I },
>> +	{ "sun5i-a13-gpadc-iio", ARCH_SUN5I },
>> +	{ "sun6i-a31-gpadc-iio", ARCH_SUN6I },
>> +	{ /*sentinel*/ },
>> +};
>> +
>> +static struct platform_driver sunxi_gpadc_driver = {
>> +	.driver = {
>> +		.name = "sunxi-gpadc-iio",
>> +	},
>> +	.id_table = sunxi_gpadc_id,
>> +	.probe = sunxi_gpadc_probe,
>> +	.remove = sunxi_gpadc_remove,
>> +};
>> +
>> +module_platform_driver(sunxi_gpadc_driver);
>> +
>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
>> +MODULE_LICENSE("GPL v2");
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Guenter Roeck]

On 07/03/2016 04:54 AM, Jonathan Cameron wrote:
> On 28/06/16 09:18, Quentin Schulz wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>> controller and a thermal sensor. This patch adds the ADC driver which is
>> based on the MFD for the same SoCs ADC.
>>
>> This also registers the thermal adc channel in the iio map array so
>> iio_hwmon could use it without modifying the Device Tree.
>>
>> This driver probes on three different platform_device_id to take into
>> account slight differences between Allwinner SoCs ADCs.
>>
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> Hi Quentin.
>
> I'm a bit in two minds about some of this.  That temperature sensor is
> so obviously meant for hwmon purposes, I'm tempted to suggest it might
> actually make sense to put it directly in hwmon rather than using the
> bridge.  That obviously makes it less flexible in some ways (i.e. for
> use within the thermal subsystem at some point).
>
> Guenter, what do you think?
>

With the upcoming new hwmon API, thermal registration is handled in the
hwmon core, so that should not be an issue. Besides, other hwmon sensors
already register with the thermal subsystem as well.

This is difficult to evaluate without datasheet; I am not sure if
the chip supports limits or trip points. If it supports trip points,
thermal may be a better target.

Overall it does look like the temperature sensor would warrant
a separate driver. Only question is thermal or hwmon.

Guenter

> I'm guessing detailed docs for this part aren't avaiable publicly? :(
>
> So the rest of my comments are kind of predicated on me having roughtly
> understood how this device works from the structure of the driver.
>
> The temperature sensor is really effectively as separate ADC?
> The main interest in this is for key detection?
>
> Anyhow, if the data flow for the temperatures sensor is not synced with
> the other ADC channels, adding buffered (pushed) output from the driver in
> future will be fiddly and with a 250Hz device you'll probably want it.
> Basically IIO buffered supports assumes each iio device will sample data
> at a particular frequency. If channels are not synchronized in that fashion
> then you have to register multiple devices or only pick a subset of channels
> to export.
>
> For the key detection you have already observed that IIO needs some
> additions to be able to have consumers of what we term 'events' e.g. threshold
> interrupts.
>
> Looking at the lradc-keys driver in tree, it looks like we only really have
> really simple threshold interrups - configured to detect a very low voltage?
> + only one per channel.
>
> So not too nasty a case, but you are right some work is needed in IIO as
> we simply don't have a means of passing these on as yet or configuring them
> from in kernel consumers.
> If we take the easy route and don't demux incoming events then it shouldn't
> be too hard to add (demux can follow later).  Hence any client device can try
> to enable events it wants, but may get events that other client devices wanted
> as well.
>
> Config interface should be much the same as the write support for channels.
> Data flow marginally harder, but pretty much a list of callbacks within
> iio_push_event.
>
> Not trivial, but not too tricky either.
>
> The events subsystem has a few 'limitations' we need to address long term
> but as this is in kernel interface only, we can do this now and fix stuff
> up in future without any ABI breakage. (limitations are things like only
> one event of a given type and direction per channel - main challenge on
> that is finding a way of doing it without abi breakage).
>
> Anyhow, sounds fun - wish I had the time to do it myself!
>
> Otherwise, your remove is never going to work as indio_dev is always NULL.
>
> Jonathan
>
>> ---
>>   drivers/iio/adc/Kconfig           |  12 ++
>>   drivers/iio/adc/Makefile          |   1 +
>>   drivers/iio/adc/sunxi-gpadc-iio.c | 371 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 384 insertions(+)
>>   create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 82c718c..b7b566a 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -328,6 +328,18 @@ config NAU7802
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called nau7802.
>>
>> +config SUNXI_ADC
>> +	tristate "ADC driver for sunxi platforms"
>> +	depends on IIO
>> +	depends on MFD_SUNXI_ADC
>> +	help
>> +	  Say yes here to build support for Allwinner SoCs (A10, A13 and A31)
>> +	  SoCs ADC. This ADC provides 4 channels which can be used as an ADC or
>> +	  as a touchscreen input and one channel for thermal sensor.
>> +
>> +          To compile this driver as a module, choose M here: the
>> +          module will be called sunxi-gpadc-iio.
>> +
>>   config PALMAS_GPADC
>>   	tristate "TI Palmas General Purpose ADC"
>>   	depends on MFD_PALMAS
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 0cb7921..2996a5b 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>>   obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>>   obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>>   obj-$(CONFIG_NAU7802) += nau7802.o
>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>>   obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>>   obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>>   obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c b/drivers/iio/adc/sunxi-gpadc-iio.c
>> new file mode 100644
>> index 0000000..5840f43
>> --- /dev/null
>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
>> @@ -0,0 +1,371 @@
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/completion.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/driver.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>> +#include <linux/regmap.h>
>> +
> I'm fussy about this:  Please prefix all defines as some of these
> are generic enough they may well end up defined in an included header
> sometime in the future.
>
>> +#define TP_CTRL0			0x00
>> +#define TP_CTRL1			0x04
>> +#define TP_CTRL2			0x08
>> +#define TP_CTRL3			0x0c
>> +#define TP_TPR				0x18
>> +#define TP_CDAT				0x1c
>> +#define TEMP_DATA			0x20
>> +#define TP_DATA				0x24
>> +
>> +/* TP_CTRL0 bits */
>> +#define ADC_FIRST_DLY(x)		((x) << 24) /* 8 bits */
>> +#define ADC_FIRST_DLY_MODE		BIT(23)
>> +#define ADC_CLK_SELECT			BIT(22)
>> +#define ADC_CLK_DIVIDER(x)		((x) << 20) /* 2 bits */
>> +#define FS_DIV(x)			((x) << 16) /* 4 bits */
>> +#define T_ACQ(x)			((x) << 0)  /* 16 bits*/
>> +
>> +/* TP_CTRL1 bits */
>> +#define STYLUS_UP_DEBOUNCE(x)		((x) << 12) /* 8 bits */
>> +#define STYLUS_UP_DEBOUNCE_EN		BIT(9)
>> +#define TOUCH_PAN_CALI_EN		BIT(6)
>> +#define TP_DUAL_EN			BIT(5)
>> +#define TP_MODE_EN			BIT(4)
>> +#define TP_ADC_SELECT			BIT(3)
>> +#define ADC_CHAN_SELECT(x)		((x) << 0)  /* 3 bits */
>> +
>> +/* TP_CTRL1 bits for sun6i SOCs */
>> +#define SUN6I_TOUCH_PAN_CALI_EN		BIT(7)
>> +#define SUN6I_TP_DUAL_EN		BIT(6)
>> +#define SUN6I_TP_MODE_EN		BIT(5)
>> +#define SUN6I_TP_ADC_SELECT		BIT(4)
>> +#define SUN6I_ADC_CHAN_SELECT(x)	BIT(x)  /* 4 bits */
>> +
>> +/* TP_CTRL2 bits */
>> +#define TP_SENSITIVE_ADJUST(x)		((x) << 28) /* 4 bits */
>> +#define TP_MODE_SELECT(x)		((x) << 26) /* 2 bits */
>> +#define PRE_MEA_EN			BIT(24)
>> +#define PRE_MEA_THRE_CNT(x)		((x) << 0)  /* 24 bits*/
>> +
>> +/* TP_CTRL3 bits */
>> +#define FILTER_EN			BIT(2)
>> +#define FILTER_TYPE(x)			((x) << 0)  /* 2 bits */
>> +
>> +/* TP_INT_FIFOC irq and fifo mask / control bits */
>> +#define TEMP_IRQ_EN			BIT(18)
>> +#define TP_OVERRUN_IRQ_EN		BIT(17)
>> +#define TP_DATA_IRQ_EN			BIT(16)
>> +#define TP_DATA_XY_CHANGE		BIT(13)
>> +#define TP_FIFO_TRIG_LEVEL(x)		((x) << 8)  /* 5 bits */
>> +#define TP_DATA_DRQ_EN			BIT(7)
>> +#define TP_FIFO_FLUSH			BIT(4)
>> +#define TP_UP_IRQ_EN			BIT(1)
>> +#define TP_DOWN_IRQ_EN			BIT(0)
>> +
>> +/* TP_INT_FIFOS irq and fifo status bits */
>> +#define TEMP_DATA_PENDING		BIT(18)
>> +#define FIFO_OVERRUN_PENDING		BIT(17)
>> +#define FIFO_DATA_PENDING		BIT(16)
>> +#define TP_IDLE_FLG			BIT(2)
>> +#define TP_UP_PENDING			BIT(1)
>> +#define TP_DOWN_PENDING			BIT(0)
>> +
>> +/* TP_TPR bits */
>> +#define TEMP_ENABLE(x)			((x) << 16)
>> +#define TEMP_PERIOD(x)			((x) << 0)  /*t = x * 256 * 16 / clkin*/
>> +
>> +#define ARCH_SUN4I			BIT(0)
>> +#define ARCH_SUN5I			BIT(1)
>> +#define ARCH_SUN6I			BIT(2)
>> +
>> +struct sunxi_gpadc_dev {
>> +	void __iomem			*regs;
>> +	struct completion		completion;
>> +	int				temp_data;
>> +	u32				adc_data;
>> +	struct regmap			*regmap;
>> +	unsigned int			fifo_data_irq;
>> +	unsigned int			temp_data_irq;
>> +	unsigned int			flags;
>> +};
>> +
>> +#define ADC_CHANNEL(_channel, _name) {				\
>> +	.type = IIO_VOLTAGE,					\
>> +	.indexed = 1,						\
>> +	.channel = _channel,					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
>> +	.datasheet_name = _name,				\
>> +}
>> +
>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
>> +	{
>> +		.adc_channel_label = "temp_adc",
>> +		.consumer_dev_name = "iio_hwmon.0",
>> +	},
>> +	{},
>> +};
>> +
>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>> +	ADC_CHANNEL(0, "adc_chan0"),
>> +	ADC_CHANNEL(1, "adc_chan1"),
>> +	ADC_CHANNEL(2, "adc_chan2"),
>> +	ADC_CHANNEL(3, "adc_chan3"),
>> +	{
>> +		.type = IIO_TEMP,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> +		.datasheet_name = "temp_adc",
>> +	},
>> +};
>> +
>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int val = 0;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +
>> +	reinit_completion(&info->completion);
>> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
>> +					     SUN6I_TP_ADC_SELECT |
>> +					     SUN6I_ADC_CHAN_SELECT(channel));
>> +	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>> +						 TP_FIFO_FLUSH);
>> +	enable_irq(info->fifo_data_irq);
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		disable_irq(info->fifo_data_irq);
>> +		mutex_unlock(&indio_dev->mlock);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	val = info->adc_data;
>> +	disable_irq(info->fifo_data_irq);
>> +	mutex_unlock(&indio_dev->mlock);
>> +
>> +	return val;
>> +}
>> +
>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int val = 0;
>> +
>> +	mutex_lock(&indio_dev->mlock);
>> +
>> +	reinit_completion(&info->completion);
>> +
>> +	regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>> +						 TP_FIFO_FLUSH);
>> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>> +	enable_irq(info->temp_data_irq);
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		disable_irq(info->temp_data_irq);
>> +		mutex_unlock(&indio_dev->mlock);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	if (info->flags & ARCH_SUN4I)
>> +		val = info->temp_data * 133 - 257000;
>> +	else if (info->flags & ARCH_SUN5I)
>> +		val = info->temp_data * 100 - 144700;
>> +	else if (info->flags & ARCH_SUN6I)
>> +		val = info->temp_data * 167 - 271000;
>> +
>> +	disable_irq(info->temp_data_irq);
>> +	mutex_unlock(&indio_dev->mlock);
>> +	return val;
>> +}
>> +
>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
>> +				struct iio_chan_spec const *chan,
>> +				int *val, int *val2, long mask)
>> +{
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		*val = sunxi_gpadc_temp_read(indio_dev);
>> +		if (*val < 0)
>> +			return *val;
>> +
>> +		return IIO_VAL_INT;
>> +	case IIO_CHAN_INFO_RAW:
>> +		*val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
>> +		if (*val < 0)
>> +			return *val;
>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static const struct iio_info sunxi_gpadc_iio_info = {
>> +	.read_raw = sunxi_gpadc_read_raw,
>> +	.driver_module = THIS_MODULE,
>> +};
>> +
>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sunxi_gpadc_dev *info = dev_id;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
>> +	if (ret == 0)
>> +		complete(&info->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
> Interesting that it's a separate data flow for the temperature sensor.
> That means that if you add buffered (pushed data) support in future either
> you'll need to split the device in two or not allow the temperature channel
> to go through the buffer interface.
>> +
>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
>> +{
>> +	struct sunxi_gpadc_dev *info = dev_id;
>> +	int ret;
>> +
>> +	ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
>> +	if (ret == 0)
>> +		complete(&info->completion);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int sunxi_gpadc_probe(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_dev *info = NULL;
>> +	struct iio_dev *indio_dev = NULL;
>> +	int ret = 0;
>> +	unsigned int irq;
>> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
>> +
>> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>> +	if (!indio_dev) {
>> +		dev_err(&pdev->dev, "failed to allocate iio device.\n");
>> +		return -ENOMEM;
>> +	}
>> +	info = iio_priv(indio_dev);
>> +
>> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
>> +	init_completion(&info->completion);
>> +	indio_dev->name = dev_name(&pdev->dev);
>> +	indio_dev->dev.parent = &pdev->dev;
>> +	indio_dev->dev.of_node = pdev->dev.of_node;
>> +	indio_dev->info = &sunxi_gpadc_iio_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
>> +	indio_dev->channels = sunxi_gpadc_channels;
>> +
>> +	info->flags = platform_get_device_id(pdev)->driver_data;
>> +
>> +	regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
>> +					     FS_DIV(7) |
>> +					     T_ACQ(63));
>> +	regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>> +	regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
>> +	regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) | TEMP_PERIOD(1953));
>> +
>> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no TEMP_DATA_PENDING interrupt registered\n");
>> +		return irq;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_temp_data_irq_handler,
>> +					   0, "temp_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request TEMP_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	info->temp_data_irq = irq;
>> +	disable_irq(irq);
>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> hohum.  A fifo?  This part is getting more interesting ;) I'll have to
> dig out the datasheet at some point (if public).
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		return irq;
>> +	}
>> +
>> +	irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>> +	ret = devm_request_any_context_irq(&pdev->dev, irq,
>> +					   sunxi_gpadc_fifo_data_irq_handler,
>> +					   0, "fifo_data", info);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev,
>> +			"could not request FIFO_DATA_PENDING interrupt: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	info->fifo_data_irq = irq;
>> +	disable_irq(irq);
>> +
>> +	ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
> As mentioned for previous patch I think this should be described externally.
> Chances are that some of those other adc channels are also going to be
> in reality used for hwmon anyway so doing it in the device tree will give
> you more flexibility.
>
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to register iio map array\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "could not register the device\n");
>> +		iio_map_array_unregister(indio_dev);
>> +		return ret;
>> +	}
>> +
> This is kind of self evident when the device turns up so I'd not bother
> cluttering up the logs with it as no additional information is given.
>> +	dev_info(&pdev->dev, "successfully loaded\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = NULL;
> ?  If it's null the below isn't going to work as intended.
> Missing a platform_get_drvdata which I'd just roll into the above
> local variable definition.
>
>> +	struct sunxi_gpadc_dev *info = NULL;
>> +
>> +	iio_device_unregister(indio_dev);
>> +	iio_map_array_unregister(indio_dev);
>> +	info = iio_priv(indio_dev);
> I'd roll this into the local variable declaration above
> (it's only a trivial bit of pointer arithemetic after all.
> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>
>> +	regmap_write(info->regmap, TP_INT_FIFOC, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>> +	{ "sun4i-a10-gpadc-iio", ARCH_SUN4I },
>> +	{ "sun5i-a13-gpadc-iio", ARCH_SUN5I },
>> +	{ "sun6i-a31-gpadc-iio", ARCH_SUN6I },
>> +	{ /*sentinel*/ },
>> +};
>> +
>> +static struct platform_driver sunxi_gpadc_driver = {
>> +	.driver = {
>> +		.name = "sunxi-gpadc-iio",
>> +	},
>> +	.id_table = sunxi_gpadc_id,
>> +	.probe = sunxi_gpadc_probe,
>> +	.remove = sunxi_gpadc_remove,
>> +};
>> +
>> +module_platform_driver(sunxi_gpadc_driver);
>> +
>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
>> +MODULE_LICENSE("GPL v2");
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 03/07/2016 17:43, Guenter Roeck wrote:
> On 07/03/2016 04:54 AM, Jonathan Cameron wrote:
>> On 28/06/16 09:18, Quentin Schulz wrote:
>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>> based on the MFD for the same SoCs ADC.
>>>
>>> This also registers the thermal adc channel in the iio map array so
>>> iio_hwmon could use it without modifying the Device Tree.
>>>
>>> This driver probes on three different platform_device_id to take into
>>> account slight differences between Allwinner SoCs ADCs.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> Hi Quentin.
>>
>> I'm a bit in two minds about some of this.  That temperature sensor is
>> so obviously meant for hwmon purposes, I'm tempted to suggest it might
>> actually make sense to put it directly in hwmon rather than using the
>> bridge.  That obviously makes it less flexible in some ways (i.e. for
>> use within the thermal subsystem at some point).
>>
>> Guenter, what do you think?
>>
> 
> With the upcoming new hwmon API, thermal registration is handled in the
> hwmon core, so that should not be an issue. Besides, other hwmon sensors
> already register with the thermal subsystem as well.
> 
> This is difficult to evaluate without datasheet; I am not sure if
> the chip supports limits or trip points. If it supports trip points,
> thermal may be a better target.
> 
> Overall it does look like the temperature sensor would warrant
> a separate driver. Only question is thermal or hwmon.
> 
> Guenter

Actually, the publicly available documentation for the temperature
sensor is almost inexistent. We only have the registers for it. For most
of the work on temperature sensor, it has been taken from the current
driver (sun4i-ts in input/touschreen) from Hans de Goede.

You can find the documentation here:
http://dl.linux-sunxi.org/A10/A10%20User%20manual%20V1.50.pdf
http://dl.linux-sunxi.org/A13/A13%20User%20Manual%20V1.30.pdf
http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf

The ADC is either called TP controller, Touch Panel controller or GPADC.
The temperature sensor's registers are only defined in the User Manual
of the A10 (first link).

Nothing to be found for limits or trip points I think.

Quentin

>> I'm guessing detailed docs for this part aren't avaiable publicly? :(
>>
>> So the rest of my comments are kind of predicated on me having roughtly
>> understood how this device works from the structure of the driver.
>>
>> The temperature sensor is really effectively as separate ADC?
>> The main interest in this is for key detection?
>>
>> Anyhow, if the data flow for the temperatures sensor is not synced with
>> the other ADC channels, adding buffered (pushed) output from the
>> driver in
>> future will be fiddly and with a 250Hz device you'll probably want it.
>> Basically IIO buffered supports assumes each iio device will sample data
>> at a particular frequency. If channels are not synchronized in that
>> fashion
>> then you have to register multiple devices or only pick a subset of
>> channels
>> to export.
>>
>> For the key detection you have already observed that IIO needs some
>> additions to be able to have consumers of what we term 'events' e.g.
>> threshold
>> interrupts.
>>
>> Looking at the lradc-keys driver in tree, it looks like we only really
>> have
>> really simple threshold interrups - configured to detect a very low
>> voltage?
>> + only one per channel.
>>
>> So not too nasty a case, but you are right some work is needed in IIO as
>> we simply don't have a means of passing these on as yet or configuring
>> them
>> from in kernel consumers.
>> If we take the easy route and don't demux incoming events then it
>> shouldn't
>> be too hard to add (demux can follow later).  Hence any client device
>> can try
>> to enable events it wants, but may get events that other client
>> devices wanted
>> as well.
>>
>> Config interface should be much the same as the write support for
>> channels.
>> Data flow marginally harder, but pretty much a list of callbacks within
>> iio_push_event.
>>
>> Not trivial, but not too tricky either.
>>
>> The events subsystem has a few 'limitations' we need to address long term
>> but as this is in kernel interface only, we can do this now and fix stuff
>> up in future without any ABI breakage. (limitations are things like only
>> one event of a given type and direction per channel - main challenge on
>> that is finding a way of doing it without abi breakage).
>>
>> Anyhow, sounds fun - wish I had the time to do it myself!
>>
>> Otherwise, your remove is never going to work as indio_dev is always
>> NULL.
>>
>> Jonathan
>>
>>> ---
>>>   drivers/iio/adc/Kconfig           |  12 ++
>>>   drivers/iio/adc/Makefile          |   1 +
>>>   drivers/iio/adc/sunxi-gpadc-iio.c | 371
>>> ++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 384 insertions(+)
>>>   create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>>
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 82c718c..b7b566a 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -328,6 +328,18 @@ config NAU7802
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called nau7802.
>>>
>>> +config SUNXI_ADC
>>> +    tristate "ADC driver for sunxi platforms"
>>> +    depends on IIO
>>> +    depends on MFD_SUNXI_ADC
>>> +    help
>>> +      Say yes here to build support for Allwinner SoCs (A10, A13 and
>>> A31)
>>> +      SoCs ADC. This ADC provides 4 channels which can be used as an
>>> ADC or
>>> +      as a touchscreen input and one channel for thermal sensor.
>>> +
>>> +          To compile this driver as a module, choose M here: the
>>> +          module will be called sunxi-gpadc-iio.
>>> +
>>>   config PALMAS_GPADC
>>>       tristate "TI Palmas General Purpose ADC"
>>>       depends on MFD_PALMAS
>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>> index 0cb7921..2996a5b 100644
>>> --- a/drivers/iio/adc/Makefile
>>> +++ b/drivers/iio/adc/Makefile
>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>>>   obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>>>   obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>>>   obj-$(CONFIG_NAU7802) += nau7802.o
>>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>>>   obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>>>   obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>>>   obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c
>>> b/drivers/iio/adc/sunxi-gpadc-iio.c
>>> new file mode 100644
>>> index 0000000..5840f43
>>> --- /dev/null
>>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
>>> @@ -0,0 +1,371 @@
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/completion.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/machine.h>
>>> +#include <linux/iio/driver.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>>> +#include <linux/regmap.h>
>>> +
>> I'm fussy about this:  Please prefix all defines as some of these
>> are generic enough they may well end up defined in an included header
>> sometime in the future.
>>
>>> +#define TP_CTRL0            0x00
>>> +#define TP_CTRL1            0x04
>>> +#define TP_CTRL2            0x08
>>> +#define TP_CTRL3            0x0c
>>> +#define TP_TPR                0x18
>>> +#define TP_CDAT                0x1c
>>> +#define TEMP_DATA            0x20
>>> +#define TP_DATA                0x24
>>> +
>>> +/* TP_CTRL0 bits */
>>> +#define ADC_FIRST_DLY(x)        ((x) << 24) /* 8 bits */
>>> +#define ADC_FIRST_DLY_MODE        BIT(23)
>>> +#define ADC_CLK_SELECT            BIT(22)
>>> +#define ADC_CLK_DIVIDER(x)        ((x) << 20) /* 2 bits */
>>> +#define FS_DIV(x)            ((x) << 16) /* 4 bits */
>>> +#define T_ACQ(x)            ((x) << 0)  /* 16 bits*/
>>> +
>>> +/* TP_CTRL1 bits */
>>> +#define STYLUS_UP_DEBOUNCE(x)        ((x) << 12) /* 8 bits */
>>> +#define STYLUS_UP_DEBOUNCE_EN        BIT(9)
>>> +#define TOUCH_PAN_CALI_EN        BIT(6)
>>> +#define TP_DUAL_EN            BIT(5)
>>> +#define TP_MODE_EN            BIT(4)
>>> +#define TP_ADC_SELECT            BIT(3)
>>> +#define ADC_CHAN_SELECT(x)        ((x) << 0)  /* 3 bits */
>>> +
>>> +/* TP_CTRL1 bits for sun6i SOCs */
>>> +#define SUN6I_TOUCH_PAN_CALI_EN        BIT(7)
>>> +#define SUN6I_TP_DUAL_EN        BIT(6)
>>> +#define SUN6I_TP_MODE_EN        BIT(5)
>>> +#define SUN6I_TP_ADC_SELECT        BIT(4)
>>> +#define SUN6I_ADC_CHAN_SELECT(x)    BIT(x)  /* 4 bits */
>>> +
>>> +/* TP_CTRL2 bits */
>>> +#define TP_SENSITIVE_ADJUST(x)        ((x) << 28) /* 4 bits */
>>> +#define TP_MODE_SELECT(x)        ((x) << 26) /* 2 bits */
>>> +#define PRE_MEA_EN            BIT(24)
>>> +#define PRE_MEA_THRE_CNT(x)        ((x) << 0)  /* 24 bits*/
>>> +
>>> +/* TP_CTRL3 bits */
>>> +#define FILTER_EN            BIT(2)
>>> +#define FILTER_TYPE(x)            ((x) << 0)  /* 2 bits */
>>> +
>>> +/* TP_INT_FIFOC irq and fifo mask / control bits */
>>> +#define TEMP_IRQ_EN            BIT(18)
>>> +#define TP_OVERRUN_IRQ_EN        BIT(17)
>>> +#define TP_DATA_IRQ_EN            BIT(16)
>>> +#define TP_DATA_XY_CHANGE        BIT(13)
>>> +#define TP_FIFO_TRIG_LEVEL(x)        ((x) << 8)  /* 5 bits */
>>> +#define TP_DATA_DRQ_EN            BIT(7)
>>> +#define TP_FIFO_FLUSH            BIT(4)
>>> +#define TP_UP_IRQ_EN            BIT(1)
>>> +#define TP_DOWN_IRQ_EN            BIT(0)
>>> +
>>> +/* TP_INT_FIFOS irq and fifo status bits */
>>> +#define TEMP_DATA_PENDING        BIT(18)
>>> +#define FIFO_OVERRUN_PENDING        BIT(17)
>>> +#define FIFO_DATA_PENDING        BIT(16)
>>> +#define TP_IDLE_FLG            BIT(2)
>>> +#define TP_UP_PENDING            BIT(1)
>>> +#define TP_DOWN_PENDING            BIT(0)
>>> +
>>> +/* TP_TPR bits */
>>> +#define TEMP_ENABLE(x)            ((x) << 16)
>>> +#define TEMP_PERIOD(x)            ((x) << 0)  /*t = x * 256 * 16 /
>>> clkin*/
>>> +
>>> +#define ARCH_SUN4I            BIT(0)
>>> +#define ARCH_SUN5I            BIT(1)
>>> +#define ARCH_SUN6I            BIT(2)
>>> +
>>> +struct sunxi_gpadc_dev {
>>> +    void __iomem            *regs;
>>> +    struct completion        completion;
>>> +    int                temp_data;
>>> +    u32                adc_data;
>>> +    struct regmap            *regmap;
>>> +    unsigned int            fifo_data_irq;
>>> +    unsigned int            temp_data_irq;
>>> +    unsigned int            flags;
>>> +};
>>> +
>>> +#define ADC_CHANNEL(_channel, _name) {                \
>>> +    .type = IIO_VOLTAGE,                    \
>>> +    .indexed = 1,                        \
>>> +    .channel = _channel,                    \
>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
>>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
>>> +    .datasheet_name = _name,                \
>>> +}
>>> +
>>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
>>> +    {
>>> +        .adc_channel_label = "temp_adc",
>>> +        .consumer_dev_name = "iio_hwmon.0",
>>> +    },
>>> +    {},
>>> +};
>>> +
>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>>> +    ADC_CHANNEL(0, "adc_chan0"),
>>> +    ADC_CHANNEL(1, "adc_chan1"),
>>> +    ADC_CHANNEL(2, "adc_chan2"),
>>> +    ADC_CHANNEL(3, "adc_chan3"),
>>> +    {
>>> +        .type = IIO_TEMP,
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>> +        .datasheet_name = "temp_adc",
>>> +    },
>>> +};
>>> +
>>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)
>>> +{
>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>> +    int val = 0;
>>> +
>>> +    mutex_lock(&indio_dev->mlock);
>>> +
>>> +    reinit_completion(&info->completion);
>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
>>> +                         SUN6I_TP_ADC_SELECT |
>>> +                         SUN6I_ADC_CHAN_SELECT(channel));
>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>>> +                         TP_FIFO_FLUSH);
>>> +    enable_irq(info->fifo_data_irq);
>>> +
>>> +    if (!wait_for_completion_timeout(&info->completion,
>>> +                     msecs_to_jiffies(100))) {
>>> +        disable_irq(info->fifo_data_irq);
>>> +        mutex_unlock(&indio_dev->mlock);
>>> +        return -ETIMEDOUT;
>>> +    }
>>> +
>>> +    val = info->adc_data;
>>> +    disable_irq(info->fifo_data_irq);
>>> +    mutex_unlock(&indio_dev->mlock);
>>> +
>>> +    return val;
>>> +}
>>> +
>>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
>>> +{
>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>> +    int val = 0;
>>> +
>>> +    mutex_lock(&indio_dev->mlock);
>>> +
>>> +    reinit_completion(&info->completion);
>>> +
>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>>> +                         TP_FIFO_FLUSH);
>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>>> +    enable_irq(info->temp_data_irq);
>>> +
>>> +    if (!wait_for_completion_timeout(&info->completion,
>>> +                     msecs_to_jiffies(100))) {
>>> +        disable_irq(info->temp_data_irq);
>>> +        mutex_unlock(&indio_dev->mlock);
>>> +        return -ETIMEDOUT;
>>> +    }
>>> +
>>> +    if (info->flags & ARCH_SUN4I)
>>> +        val = info->temp_data * 133 - 257000;
>>> +    else if (info->flags & ARCH_SUN5I)
>>> +        val = info->temp_data * 100 - 144700;
>>> +    else if (info->flags & ARCH_SUN6I)
>>> +        val = info->temp_data * 167 - 271000;
>>> +
>>> +    disable_irq(info->temp_data_irq);
>>> +    mutex_unlock(&indio_dev->mlock);
>>> +    return val;
>>> +}
>>> +
>>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int *val, int *val2, long mask)
>>> +{
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_PROCESSED:
>>> +        *val = sunxi_gpadc_temp_read(indio_dev);
>>> +        if (*val < 0)
>>> +            return *val;
>>> +
>>> +        return IIO_VAL_INT;
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        *val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
>>> +        if (*val < 0)
>>> +            return *val;
>>> +
>>> +        return IIO_VAL_INT;
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return -EINVAL;
>>> +}
>>> +
>>> +static const struct iio_info sunxi_gpadc_iio_info = {
>>> +    .read_raw = sunxi_gpadc_read_raw,
>>> +    .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void
>>> *dev_id)
>>> +{
>>> +    struct sunxi_gpadc_dev *info = dev_id;
>>> +    int ret;
>>> +
>>> +    ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
>>> +    if (ret == 0)
>>> +        complete(&info->completion);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>> Interesting that it's a separate data flow for the temperature sensor.
>> That means that if you add buffered (pushed data) support in future
>> either
>> you'll need to split the device in two or not allow the temperature
>> channel
>> to go through the buffer interface.
>>> +
>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void
>>> *dev_id)
>>> +{
>>> +    struct sunxi_gpadc_dev *info = dev_id;
>>> +    int ret;
>>> +
>>> +    ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
>>> +    if (ret == 0)
>>> +        complete(&info->completion);
>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int sunxi_gpadc_probe(struct platform_device *pdev)
>>> +{
>>> +    struct sunxi_gpadc_dev *info = NULL;
>>> +    struct iio_dev *indio_dev = NULL;
>>> +    int ret = 0;
>>> +    unsigned int irq;
>>> +    struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
>>> +
>>> +    sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>>> +    if (!indio_dev) {
>>> +        dev_err(&pdev->dev, "failed to allocate iio device.\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +    info = iio_priv(indio_dev);
>>> +
>>> +    info->regmap = sunxi_gpadc_mfd_dev->regmap;
>>> +    init_completion(&info->completion);
>>> +    indio_dev->name = dev_name(&pdev->dev);
>>> +    indio_dev->dev.parent = &pdev->dev;
>>> +    indio_dev->dev.of_node = pdev->dev.of_node;
>>> +    indio_dev->info = &sunxi_gpadc_iio_info;
>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>> +    indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
>>> +    indio_dev->channels = sunxi_gpadc_channels;
>>> +
>>> +    info->flags = platform_get_device_id(pdev)->driver_data;
>>> +
>>> +    regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
>>> +                         FS_DIV(7) |
>>> +                         T_ACQ(63));
>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>>> +    regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
>>> +    regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) |
>>> TEMP_PERIOD(1953));
>>> +
>>> +    irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>>> +    if (irq < 0) {
>>> +        dev_err(&pdev->dev,
>>> +            "no TEMP_DATA_PENDING interrupt registered\n");
>>> +        return irq;
>>> +    }
>>> +
>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,
>>> +                       sunxi_gpadc_temp_data_irq_handler,
>>> +                       0, "temp_data", info);
>>> +    if (ret < 0) {
>>> +        dev_err(&pdev->dev,
>>> +            "could not request TEMP_DATA_PENDING interrupt: %d\n",
>>> +            ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    info->temp_data_irq = irq;
>>> +    disable_irq(irq);
>>> +
>>> +    irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> hohum.  A fifo?  This part is getting more interesting ;) I'll have to
>> dig out the datasheet at some point (if public).
>>> +    if (irq < 0) {
>>> +        dev_err(&pdev->dev,
>>> +            "no FIFO_DATA_PENDING interrupt registered\n");
>>> +        return irq;
>>> +    }
>>> +
>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,
>>> +                       sunxi_gpadc_fifo_data_irq_handler,
>>> +                       0, "fifo_data", info);
>>> +    if (ret < 0) {
>>> +        dev_err(&pdev->dev,
>>> +            "could not request FIFO_DATA_PENDING interrupt: %d\n",
>>> +            ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    info->fifo_data_irq = irq;
>>> +    disable_irq(irq);
>>> +
>>> +    ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>> As mentioned for previous patch I think this should be described
>> externally.
>> Chances are that some of those other adc channels are also going to be
>> in reality used for hwmon anyway so doing it in the device tree will give
>> you more flexibility.
>>
>>> +    if (ret < 0) {
>>> +        dev_err(&pdev->dev, "failed to register iio map array\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    platform_set_drvdata(pdev, indio_dev);
>>> +
>>> +    ret = iio_device_register(indio_dev);
>>> +    if (ret < 0) {
>>> +        dev_err(&pdev->dev, "could not register the device\n");
>>> +        iio_map_array_unregister(indio_dev);
>>> +        return ret;
>>> +    }
>>> +
>> This is kind of self evident when the device turns up so I'd not bother
>> cluttering up the logs with it as no additional information is given.
>>> +    dev_info(&pdev->dev, "successfully loaded\n");
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>>> +{
>>> +    struct iio_dev *indio_dev = NULL;
>> ?  If it's null the below isn't going to work as intended.
>> Missing a platform_get_drvdata which I'd just roll into the above
>> local variable definition.
>>
>>> +    struct sunxi_gpadc_dev *info = NULL;
>>> +
>>> +    iio_device_unregister(indio_dev);
>>> +    iio_map_array_unregister(indio_dev);
>>> +    info = iio_priv(indio_dev);
>> I'd roll this into the local variable declaration above
>> (it's only a trivial bit of pointer arithemetic after all.
>> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>
>>> +    regmap_write(info->regmap, TP_INT_FIFOC, 0);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>>> +    { "sun4i-a10-gpadc-iio", ARCH_SUN4I },
>>> +    { "sun5i-a13-gpadc-iio", ARCH_SUN5I },
>>> +    { "sun6i-a31-gpadc-iio", ARCH_SUN6I },
>>> +    { /*sentinel*/ },
>>> +};
>>> +
>>> +static struct platform_driver sunxi_gpadc_driver = {
>>> +    .driver = {
>>> +        .name = "sunxi-gpadc-iio",
>>> +    },
>>> +    .id_table = sunxi_gpadc_id,
>>> +    .probe = sunxi_gpadc_probe,
>>> +    .remove = sunxi_gpadc_remove,
>>> +};
>>> +
>>> +module_platform_driver(sunxi_gpadc_driver);
>>> +
>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Guenter Roeck]

On 07/04/2016 12:26 AM, Quentin Schulz wrote:
> On 03/07/2016 17:43, Guenter Roeck wrote:
>> On 07/03/2016 04:54 AM, Jonathan Cameron wrote:
>>> On 28/06/16 09:18, Quentin Schulz wrote:
>>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>>> controller and a thermal sensor. This patch adds the ADC driver which is
>>>> based on the MFD for the same SoCs ADC.
>>>>
>>>> This also registers the thermal adc channel in the iio map array so
>>>> iio_hwmon could use it without modifying the Device Tree.
>>>>
>>>> This driver probes on three different platform_device_id to take into
>>>> account slight differences between Allwinner SoCs ADCs.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> Hi Quentin.
>>>
>>> I'm a bit in two minds about some of this.  That temperature sensor is
>>> so obviously meant for hwmon purposes, I'm tempted to suggest it might
>>> actually make sense to put it directly in hwmon rather than using the
>>> bridge.  That obviously makes it less flexible in some ways (i.e. for
>>> use within the thermal subsystem at some point).
>>>
>>> Guenter, what do you think?
>>>
>>
>> With the upcoming new hwmon API, thermal registration is handled in the
>> hwmon core, so that should not be an issue. Besides, other hwmon sensors
>> already register with the thermal subsystem as well.
>>
>> This is difficult to evaluate without datasheet; I am not sure if
>> the chip supports limits or trip points. If it supports trip points,
>> thermal may be a better target.
>>
>> Overall it does look like the temperature sensor would warrant
>> a separate driver. Only question is thermal or hwmon.
>>
>> Guenter
>
> Actually, the publicly available documentation for the temperature
> sensor is almost inexistent. We only have the registers for it. For most
> of the work on temperature sensor, it has been taken from the current
> driver (sun4i-ts in input/touschreen) from Hans de Goede.
>
Does this mean that the temperature data will be exported twice, once through
iio and once through the touchscreen driver ?

Guenter

> You can find the documentation here:
> http://dl.linux-sunxi.org/A10/A10%20User%20manual%20V1.50.pdf
> http://dl.linux-sunxi.org/A13/A13%20User%20Manual%20V1.30.pdf
> http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf
>
> The ADC is either called TP controller, Touch Panel controller or GPADC.
> The temperature sensor's registers are only defined in the User Manual
> of the A10 (first link).
>
> Nothing to be found for limits or trip points I think.
>
> Quentin
>
>>> I'm guessing detailed docs for this part aren't avaiable publicly? :(
>>>
>>> So the rest of my comments are kind of predicated on me having roughtly
>>> understood how this device works from the structure of the driver.
>>>
>>> The temperature sensor is really effectively as separate ADC?
>>> The main interest in this is for key detection?
>>>
>>> Anyhow, if the data flow for the temperatures sensor is not synced with
>>> the other ADC channels, adding buffered (pushed) output from the
>>> driver in
>>> future will be fiddly and with a 250Hz device you'll probably want it.
>>> Basically IIO buffered supports assumes each iio device will sample data
>>> at a particular frequency. If channels are not synchronized in that
>>> fashion
>>> then you have to register multiple devices or only pick a subset of
>>> channels
>>> to export.
>>>
>>> For the key detection you have already observed that IIO needs some
>>> additions to be able to have consumers of what we term 'events' e.g.
>>> threshold
>>> interrupts.
>>>
>>> Looking at the lradc-keys driver in tree, it looks like we only really
>>> have
>>> really simple threshold interrups - configured to detect a very low
>>> voltage?
>>> + only one per channel.
>>>
>>> So not too nasty a case, but you are right some work is needed in IIO as
>>> we simply don't have a means of passing these on as yet or configuring
>>> them
>>> from in kernel consumers.
>>> If we take the easy route and don't demux incoming events then it
>>> shouldn't
>>> be too hard to add (demux can follow later).  Hence any client device
>>> can try
>>> to enable events it wants, but may get events that other client
>>> devices wanted
>>> as well.
>>>
>>> Config interface should be much the same as the write support for
>>> channels.
>>> Data flow marginally harder, but pretty much a list of callbacks within
>>> iio_push_event.
>>>
>>> Not trivial, but not too tricky either.
>>>
>>> The events subsystem has a few 'limitations' we need to address long term
>>> but as this is in kernel interface only, we can do this now and fix stuff
>>> up in future without any ABI breakage. (limitations are things like only
>>> one event of a given type and direction per channel - main challenge on
>>> that is finding a way of doing it without abi breakage).
>>>
>>> Anyhow, sounds fun - wish I had the time to do it myself!
>>>
>>> Otherwise, your remove is never going to work as indio_dev is always
>>> NULL.
>>>
>>> Jonathan
>>>
>>>> ---
>>>>    drivers/iio/adc/Kconfig           |  12 ++
>>>>    drivers/iio/adc/Makefile          |   1 +
>>>>    drivers/iio/adc/sunxi-gpadc-iio.c | 371
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 384 insertions(+)
>>>>    create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>>>
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index 82c718c..b7b566a 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -328,6 +328,18 @@ config NAU7802
>>>>          To compile this driver as a module, choose M here: the
>>>>          module will be called nau7802.
>>>>
>>>> +config SUNXI_ADC
>>>> +    tristate "ADC driver for sunxi platforms"
>>>> +    depends on IIO
>>>> +    depends on MFD_SUNXI_ADC
>>>> +    help
>>>> +      Say yes here to build support for Allwinner SoCs (A10, A13 and
>>>> A31)
>>>> +      SoCs ADC. This ADC provides 4 channels which can be used as an
>>>> ADC or
>>>> +      as a touchscreen input and one channel for thermal sensor.
>>>> +
>>>> +          To compile this driver as a module, choose M here: the
>>>> +          module will be called sunxi-gpadc-iio.
>>>> +
>>>>    config PALMAS_GPADC
>>>>        tristate "TI Palmas General Purpose ADC"
>>>>        depends on MFD_PALMAS
>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>> index 0cb7921..2996a5b 100644
>>>> --- a/drivers/iio/adc/Makefile
>>>> +++ b/drivers/iio/adc/Makefile
>>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>>>>    obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>>>>    obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>>>>    obj-$(CONFIG_NAU7802) += nau7802.o
>>>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>>>>    obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>>>>    obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>>>>    obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>>>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c
>>>> b/drivers/iio/adc/sunxi-gpadc-iio.c
>>>> new file mode 100644
>>>> index 0000000..5840f43
>>>> --- /dev/null
>>>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
>>>> @@ -0,0 +1,371 @@
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/completion.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/machine.h>
>>>> +#include <linux/iio/driver.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>>>> +#include <linux/regmap.h>
>>>> +
>>> I'm fussy about this:  Please prefix all defines as some of these
>>> are generic enough they may well end up defined in an included header
>>> sometime in the future.
>>>
>>>> +#define TP_CTRL0            0x00
>>>> +#define TP_CTRL1            0x04
>>>> +#define TP_CTRL2            0x08
>>>> +#define TP_CTRL3            0x0c
>>>> +#define TP_TPR                0x18
>>>> +#define TP_CDAT                0x1c
>>>> +#define TEMP_DATA            0x20
>>>> +#define TP_DATA                0x24
>>>> +
>>>> +/* TP_CTRL0 bits */
>>>> +#define ADC_FIRST_DLY(x)        ((x) << 24) /* 8 bits */
>>>> +#define ADC_FIRST_DLY_MODE        BIT(23)
>>>> +#define ADC_CLK_SELECT            BIT(22)
>>>> +#define ADC_CLK_DIVIDER(x)        ((x) << 20) /* 2 bits */
>>>> +#define FS_DIV(x)            ((x) << 16) /* 4 bits */
>>>> +#define T_ACQ(x)            ((x) << 0)  /* 16 bits*/
>>>> +
>>>> +/* TP_CTRL1 bits */
>>>> +#define STYLUS_UP_DEBOUNCE(x)        ((x) << 12) /* 8 bits */
>>>> +#define STYLUS_UP_DEBOUNCE_EN        BIT(9)
>>>> +#define TOUCH_PAN_CALI_EN        BIT(6)
>>>> +#define TP_DUAL_EN            BIT(5)
>>>> +#define TP_MODE_EN            BIT(4)
>>>> +#define TP_ADC_SELECT            BIT(3)
>>>> +#define ADC_CHAN_SELECT(x)        ((x) << 0)  /* 3 bits */
>>>> +
>>>> +/* TP_CTRL1 bits for sun6i SOCs */
>>>> +#define SUN6I_TOUCH_PAN_CALI_EN        BIT(7)
>>>> +#define SUN6I_TP_DUAL_EN        BIT(6)
>>>> +#define SUN6I_TP_MODE_EN        BIT(5)
>>>> +#define SUN6I_TP_ADC_SELECT        BIT(4)
>>>> +#define SUN6I_ADC_CHAN_SELECT(x)    BIT(x)  /* 4 bits */
>>>> +
>>>> +/* TP_CTRL2 bits */
>>>> +#define TP_SENSITIVE_ADJUST(x)        ((x) << 28) /* 4 bits */
>>>> +#define TP_MODE_SELECT(x)        ((x) << 26) /* 2 bits */
>>>> +#define PRE_MEA_EN            BIT(24)
>>>> +#define PRE_MEA_THRE_CNT(x)        ((x) << 0)  /* 24 bits*/
>>>> +
>>>> +/* TP_CTRL3 bits */
>>>> +#define FILTER_EN            BIT(2)
>>>> +#define FILTER_TYPE(x)            ((x) << 0)  /* 2 bits */
>>>> +
>>>> +/* TP_INT_FIFOC irq and fifo mask / control bits */
>>>> +#define TEMP_IRQ_EN            BIT(18)
>>>> +#define TP_OVERRUN_IRQ_EN        BIT(17)
>>>> +#define TP_DATA_IRQ_EN            BIT(16)
>>>> +#define TP_DATA_XY_CHANGE        BIT(13)
>>>> +#define TP_FIFO_TRIG_LEVEL(x)        ((x) << 8)  /* 5 bits */
>>>> +#define TP_DATA_DRQ_EN            BIT(7)
>>>> +#define TP_FIFO_FLUSH            BIT(4)
>>>> +#define TP_UP_IRQ_EN            BIT(1)
>>>> +#define TP_DOWN_IRQ_EN            BIT(0)
>>>> +
>>>> +/* TP_INT_FIFOS irq and fifo status bits */
>>>> +#define TEMP_DATA_PENDING        BIT(18)
>>>> +#define FIFO_OVERRUN_PENDING        BIT(17)
>>>> +#define FIFO_DATA_PENDING        BIT(16)
>>>> +#define TP_IDLE_FLG            BIT(2)
>>>> +#define TP_UP_PENDING            BIT(1)
>>>> +#define TP_DOWN_PENDING            BIT(0)
>>>> +
>>>> +/* TP_TPR bits */
>>>> +#define TEMP_ENABLE(x)            ((x) << 16)
>>>> +#define TEMP_PERIOD(x)            ((x) << 0)  /*t = x * 256 * 16 /
>>>> clkin*/
>>>> +
>>>> +#define ARCH_SUN4I            BIT(0)
>>>> +#define ARCH_SUN5I            BIT(1)
>>>> +#define ARCH_SUN6I            BIT(2)
>>>> +
>>>> +struct sunxi_gpadc_dev {
>>>> +    void __iomem            *regs;
>>>> +    struct completion        completion;
>>>> +    int                temp_data;
>>>> +    u32                adc_data;
>>>> +    struct regmap            *regmap;
>>>> +    unsigned int            fifo_data_irq;
>>>> +    unsigned int            temp_data_irq;
>>>> +    unsigned int            flags;
>>>> +};
>>>> +
>>>> +#define ADC_CHANNEL(_channel, _name) {                \
>>>> +    .type = IIO_VOLTAGE,                    \
>>>> +    .indexed = 1,                        \
>>>> +    .channel = _channel,                    \
>>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
>>>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
>>>> +    .datasheet_name = _name,                \
>>>> +}
>>>> +
>>>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
>>>> +    {
>>>> +        .adc_channel_label = "temp_adc",
>>>> +        .consumer_dev_name = "iio_hwmon.0",
>>>> +    },
>>>> +    {},
>>>> +};
>>>> +
>>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>>>> +    ADC_CHANNEL(0, "adc_chan0"),
>>>> +    ADC_CHANNEL(1, "adc_chan1"),
>>>> +    ADC_CHANNEL(2, "adc_chan2"),
>>>> +    ADC_CHANNEL(3, "adc_chan3"),
>>>> +    {
>>>> +        .type = IIO_TEMP,
>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>> +        .datasheet_name = "temp_adc",
>>>> +    },
>>>> +};
>>>> +
>>>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel)
>>>> +{
>>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>>> +    int val = 0;
>>>> +
>>>> +    mutex_lock(&indio_dev->mlock);
>>>> +
>>>> +    reinit_completion(&info->completion);
>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
>>>> +                         SUN6I_TP_ADC_SELECT |
>>>> +                         SUN6I_ADC_CHAN_SELECT(channel));
>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>>>> +                         TP_FIFO_FLUSH);
>>>> +    enable_irq(info->fifo_data_irq);
>>>> +
>>>> +    if (!wait_for_completion_timeout(&info->completion,
>>>> +                     msecs_to_jiffies(100))) {
>>>> +        disable_irq(info->fifo_data_irq);
>>>> +        mutex_unlock(&indio_dev->mlock);
>>>> +        return -ETIMEDOUT;
>>>> +    }
>>>> +
>>>> +    val = info->adc_data;
>>>> +    disable_irq(info->fifo_data_irq);
>>>> +    mutex_unlock(&indio_dev->mlock);
>>>> +
>>>> +    return val;
>>>> +}
>>>> +
>>>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
>>>> +{
>>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>>> +    int val = 0;
>>>> +
>>>> +    mutex_lock(&indio_dev->mlock);
>>>> +
>>>> +    reinit_completion(&info->completion);
>>>> +
>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>>>> +                         TP_FIFO_FLUSH);
>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>>>> +    enable_irq(info->temp_data_irq);
>>>> +
>>>> +    if (!wait_for_completion_timeout(&info->completion,
>>>> +                     msecs_to_jiffies(100))) {
>>>> +        disable_irq(info->temp_data_irq);
>>>> +        mutex_unlock(&indio_dev->mlock);
>>>> +        return -ETIMEDOUT;
>>>> +    }
>>>> +
>>>> +    if (info->flags & ARCH_SUN4I)
>>>> +        val = info->temp_data * 133 - 257000;
>>>> +    else if (info->flags & ARCH_SUN5I)
>>>> +        val = info->temp_data * 100 - 144700;
>>>> +    else if (info->flags & ARCH_SUN6I)
>>>> +        val = info->temp_data * 167 - 271000;
>>>> +
>>>> +    disable_irq(info->temp_data_irq);
>>>> +    mutex_unlock(&indio_dev->mlock);
>>>> +    return val;
>>>> +}
>>>> +
>>>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
>>>> +                struct iio_chan_spec const *chan,
>>>> +                int *val, int *val2, long mask)
>>>> +{
>>>> +    switch (mask) {
>>>> +    case IIO_CHAN_INFO_PROCESSED:
>>>> +        *val = sunxi_gpadc_temp_read(indio_dev);
>>>> +        if (*val < 0)
>>>> +            return *val;
>>>> +
>>>> +        return IIO_VAL_INT;
>>>> +    case IIO_CHAN_INFO_RAW:
>>>> +        *val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
>>>> +        if (*val < 0)
>>>> +            return *val;
>>>> +
>>>> +        return IIO_VAL_INT;
>>>> +    default:
>>>> +        break;
>>>> +    }
>>>> +
>>>> +    return -EINVAL;
>>>> +}
>>>> +
>>>> +static const struct iio_info sunxi_gpadc_iio_info = {
>>>> +    .read_raw = sunxi_gpadc_read_raw,
>>>> +    .driver_module = THIS_MODULE,
>>>> +};
>>>> +
>>>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void
>>>> *dev_id)
>>>> +{
>>>> +    struct sunxi_gpadc_dev *info = dev_id;
>>>> +    int ret;
>>>> +
>>>> +    ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
>>>> +    if (ret == 0)
>>>> +        complete(&info->completion);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>> Interesting that it's a separate data flow for the temperature sensor.
>>> That means that if you add buffered (pushed data) support in future
>>> either
>>> you'll need to split the device in two or not allow the temperature
>>> channel
>>> to go through the buffer interface.
>>>> +
>>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void
>>>> *dev_id)
>>>> +{
>>>> +    struct sunxi_gpadc_dev *info = dev_id;
>>>> +    int ret;
>>>> +
>>>> +    ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
>>>> +    if (ret == 0)
>>>> +        complete(&info->completion);
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int sunxi_gpadc_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct sunxi_gpadc_dev *info = NULL;
>>>> +    struct iio_dev *indio_dev = NULL;
>>>> +    int ret = 0;
>>>> +    unsigned int irq;
>>>> +    struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
>>>> +
>>>> +    sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
>>>> +
>>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>>>> +    if (!indio_dev) {
>>>> +        dev_err(&pdev->dev, "failed to allocate iio device.\n");
>>>> +        return -ENOMEM;
>>>> +    }
>>>> +    info = iio_priv(indio_dev);
>>>> +
>>>> +    info->regmap = sunxi_gpadc_mfd_dev->regmap;
>>>> +    init_completion(&info->completion);
>>>> +    indio_dev->name = dev_name(&pdev->dev);
>>>> +    indio_dev->dev.parent = &pdev->dev;
>>>> +    indio_dev->dev.of_node = pdev->dev.of_node;
>>>> +    indio_dev->info = &sunxi_gpadc_iio_info;
>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +    indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
>>>> +    indio_dev->channels = sunxi_gpadc_channels;
>>>> +
>>>> +    info->flags = platform_get_device_id(pdev)->driver_data;
>>>> +
>>>> +    regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
>>>> +                         FS_DIV(7) |
>>>> +                         T_ACQ(63));
>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>>>> +    regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
>>>> +    regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) |
>>>> TEMP_PERIOD(1953));
>>>> +
>>>> +    irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>>>> +    if (irq < 0) {
>>>> +        dev_err(&pdev->dev,
>>>> +            "no TEMP_DATA_PENDING interrupt registered\n");
>>>> +        return irq;
>>>> +    }
>>>> +
>>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,
>>>> +                       sunxi_gpadc_temp_data_irq_handler,
>>>> +                       0, "temp_data", info);
>>>> +    if (ret < 0) {
>>>> +        dev_err(&pdev->dev,
>>>> +            "could not request TEMP_DATA_PENDING interrupt: %d\n",
>>>> +            ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    info->temp_data_irq = irq;
>>>> +    disable_irq(irq);
>>>> +
>>>> +    irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>>> hohum.  A fifo?  This part is getting more interesting ;) I'll have to
>>> dig out the datasheet at some point (if public).
>>>> +    if (irq < 0) {
>>>> +        dev_err(&pdev->dev,
>>>> +            "no FIFO_DATA_PENDING interrupt registered\n");
>>>> +        return irq;
>>>> +    }
>>>> +
>>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,
>>>> +                       sunxi_gpadc_fifo_data_irq_handler,
>>>> +                       0, "fifo_data", info);
>>>> +    if (ret < 0) {
>>>> +        dev_err(&pdev->dev,
>>>> +            "could not request FIFO_DATA_PENDING interrupt: %d\n",
>>>> +            ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    info->fifo_data_irq = irq;
>>>> +    disable_irq(irq);
>>>> +
>>>> +    ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>>> As mentioned for previous patch I think this should be described
>>> externally.
>>> Chances are that some of those other adc channels are also going to be
>>> in reality used for hwmon anyway so doing it in the device tree will give
>>> you more flexibility.
>>>
>>>> +    if (ret < 0) {
>>>> +        dev_err(&pdev->dev, "failed to register iio map array\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    platform_set_drvdata(pdev, indio_dev);
>>>> +
>>>> +    ret = iio_device_register(indio_dev);
>>>> +    if (ret < 0) {
>>>> +        dev_err(&pdev->dev, "could not register the device\n");
>>>> +        iio_map_array_unregister(indio_dev);
>>>> +        return ret;
>>>> +    }
>>>> +
>>> This is kind of self evident when the device turns up so I'd not bother
>>> cluttering up the logs with it as no additional information is given.
>>>> +    dev_info(&pdev->dev, "successfully loaded\n");
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct iio_dev *indio_dev = NULL;
>>> ?  If it's null the below isn't going to work as intended.
>>> Missing a platform_get_drvdata which I'd just roll into the above
>>> local variable definition.
>>>
>>>> +    struct sunxi_gpadc_dev *info = NULL;
>>>> +
>>>> +    iio_device_unregister(indio_dev);
>>>> +    iio_map_array_unregister(indio_dev);
>>>> +    info = iio_priv(indio_dev);
>>> I'd roll this into the local variable declaration above
>>> (it's only a trivial bit of pointer arithemetic after all.
>>> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>>
>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, 0);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>>>> +    { "sun4i-a10-gpadc-iio", ARCH_SUN4I },
>>>> +    { "sun5i-a13-gpadc-iio", ARCH_SUN5I },
>>>> +    { "sun6i-a31-gpadc-iio", ARCH_SUN6I },
>>>> +    { /*sentinel*/ },
>>>> +};
>>>> +
>>>> +static struct platform_driver sunxi_gpadc_driver = {
>>>> +    .driver = {
>>>> +        .name = "sunxi-gpadc-iio",
>>>> +    },
>>>> +    .id_table = sunxi_gpadc_id,
>>>> +    .probe = sunxi_gpadc_probe,
>>>> +    .remove = sunxi_gpadc_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(sunxi_gpadc_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>>>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
>

Re: [PATCH 2/3] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/07/2016 18:29, Guenter Roeck wrote:
> On 07/04/2016 12:26 AM, Quentin Schulz wrote:
>> On 03/07/2016 17:43, Guenter Roeck wrote:
>>> On 07/03/2016 04:54 AM, Jonathan Cameron wrote:
>>>> On 28/06/16 09:18, Quentin Schulz wrote:
>>>>> The Allwinner SoCs all have an ADC that can also act as a touchscreen
>>>>> controller and a thermal sensor. This patch adds the ADC driver
>>>>> which is
>>>>> based on the MFD for the same SoCs ADC.
>>>>>
>>>>> This also registers the thermal adc channel in the iio map array so
>>>>> iio_hwmon could use it without modifying the Device Tree.
>>>>>
>>>>> This driver probes on three different platform_device_id to take into
>>>>> account slight differences between Allwinner SoCs ADCs.
>>>>>
>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>> Hi Quentin.
>>>>
>>>> I'm a bit in two minds about some of this.  That temperature sensor is
>>>> so obviously meant for hwmon purposes, I'm tempted to suggest it might
>>>> actually make sense to put it directly in hwmon rather than using the
>>>> bridge.  That obviously makes it less flexible in some ways (i.e. for
>>>> use within the thermal subsystem at some point).
>>>>
>>>> Guenter, what do you think?
>>>>
>>>
>>> With the upcoming new hwmon API, thermal registration is handled in the
>>> hwmon core, so that should not be an issue. Besides, other hwmon sensors
>>> already register with the thermal subsystem as well.
>>>
>>> This is difficult to evaluate without datasheet; I am not sure if
>>> the chip supports limits or trip points. If it supports trip points,
>>> thermal may be a better target.
>>>
>>> Overall it does look like the temperature sensor would warrant
>>> a separate driver. Only question is thermal or hwmon.
>>>
>>> Guenter
>>
>> Actually, the publicly available documentation for the temperature
>> sensor is almost inexistent. We only have the registers for it. For most
>> of the work on temperature sensor, it has been taken from the current
>> driver (sun4i-ts in input/touschreen) from Hans de Goede.
>>
> Does this mean that the temperature data will be exported twice, once
> through
> iio and once through the touchscreen driver ?
> 
> Guenter

Currently, the temperature data is exported once in thermal framework
from the sun4i-ts input driver.

In the proposed patch, the temperature is exported twice: once in iio
framework from the IIO driver and once in hwmon framework from iio_hwmon
driver (which takes data from a channel of the IIO driver).

Quentin

>> You can find the documentation here:
>> http://dl.linux-sunxi.org/A10/A10%20User%20manual%20V1.50.pdf
>> http://dl.linux-sunxi.org/A13/A13%20User%20Manual%20V1.30.pdf
>> http://dl.linux-sunxi.org/A31/A31%20User%20Manual%20V1.20.pdf
>>
>> The ADC is either called TP controller, Touch Panel controller or GPADC.
>> The temperature sensor's registers are only defined in the User Manual
>> of the A10 (first link).
>>
>> Nothing to be found for limits or trip points I think.
>>
>> Quentin
>>
>>>> I'm guessing detailed docs for this part aren't avaiable publicly? :(
>>>>
>>>> So the rest of my comments are kind of predicated on me having roughtly
>>>> understood how this device works from the structure of the driver.
>>>>
>>>> The temperature sensor is really effectively as separate ADC?
>>>> The main interest in this is for key detection?
>>>>
>>>> Anyhow, if the data flow for the temperatures sensor is not synced with
>>>> the other ADC channels, adding buffered (pushed) output from the
>>>> driver in
>>>> future will be fiddly and with a 250Hz device you'll probably want it.
>>>> Basically IIO buffered supports assumes each iio device will sample
>>>> data
>>>> at a particular frequency. If channels are not synchronized in that
>>>> fashion
>>>> then you have to register multiple devices or only pick a subset of
>>>> channels
>>>> to export.
>>>>
>>>> For the key detection you have already observed that IIO needs some
>>>> additions to be able to have consumers of what we term 'events' e.g.
>>>> threshold
>>>> interrupts.
>>>>
>>>> Looking at the lradc-keys driver in tree, it looks like we only really
>>>> have
>>>> really simple threshold interrups - configured to detect a very low
>>>> voltage?
>>>> + only one per channel.
>>>>
>>>> So not too nasty a case, but you are right some work is needed in
>>>> IIO as
>>>> we simply don't have a means of passing these on as yet or configuring
>>>> them
>>>> from in kernel consumers.
>>>> If we take the easy route and don't demux incoming events then it
>>>> shouldn't
>>>> be too hard to add (demux can follow later).  Hence any client device
>>>> can try
>>>> to enable events it wants, but may get events that other client
>>>> devices wanted
>>>> as well.
>>>>
>>>> Config interface should be much the same as the write support for
>>>> channels.
>>>> Data flow marginally harder, but pretty much a list of callbacks within
>>>> iio_push_event.
>>>>
>>>> Not trivial, but not too tricky either.
>>>>
>>>> The events subsystem has a few 'limitations' we need to address long
>>>> term
>>>> but as this is in kernel interface only, we can do this now and fix
>>>> stuff
>>>> up in future without any ABI breakage. (limitations are things like
>>>> only
>>>> one event of a given type and direction per channel - main challenge on
>>>> that is finding a way of doing it without abi breakage).
>>>>
>>>> Anyhow, sounds fun - wish I had the time to do it myself!
>>>>
>>>> Otherwise, your remove is never going to work as indio_dev is always
>>>> NULL.
>>>>
>>>> Jonathan
>>>>
>>>>> ---
>>>>>    drivers/iio/adc/Kconfig           |  12 ++
>>>>>    drivers/iio/adc/Makefile          |   1 +
>>>>>    drivers/iio/adc/sunxi-gpadc-iio.c | 371
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>    3 files changed, 384 insertions(+)
>>>>>    create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>>>>>
>>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>>> index 82c718c..b7b566a 100644
>>>>> --- a/drivers/iio/adc/Kconfig
>>>>> +++ b/drivers/iio/adc/Kconfig
>>>>> @@ -328,6 +328,18 @@ config NAU7802
>>>>>          To compile this driver as a module, choose M here: the
>>>>>          module will be called nau7802.
>>>>>
>>>>> +config SUNXI_ADC
>>>>> +    tristate "ADC driver for sunxi platforms"
>>>>> +    depends on IIO
>>>>> +    depends on MFD_SUNXI_ADC
>>>>> +    help
>>>>> +      Say yes here to build support for Allwinner SoCs (A10, A13 and
>>>>> A31)
>>>>> +      SoCs ADC. This ADC provides 4 channels which can be used as an
>>>>> ADC or
>>>>> +      as a touchscreen input and one channel for thermal sensor.
>>>>> +
>>>>> +          To compile this driver as a module, choose M here: the
>>>>> +          module will be called sunxi-gpadc-iio.
>>>>> +
>>>>>    config PALMAS_GPADC
>>>>>        tristate "TI Palmas General Purpose ADC"
>>>>>        depends on MFD_PALMAS
>>>>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>>>>> index 0cb7921..2996a5b 100644
>>>>> --- a/drivers/iio/adc/Makefile
>>>>> +++ b/drivers/iio/adc/Makefile
>>>>> @@ -32,6 +32,7 @@ obj-$(CONFIG_MCP3422) += mcp3422.o
>>>>>    obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o
>>>>>    obj-$(CONFIG_MXS_LRADC) += mxs-lradc.o
>>>>>    obj-$(CONFIG_NAU7802) += nau7802.o
>>>>> +obj-$(CONFIG_SUNXI_ADC) += sunxi-gpadc-iio.o
>>>>>    obj-$(CONFIG_PALMAS_GPADC) += palmas_gpadc.o
>>>>>    obj-$(CONFIG_QCOM_SPMI_IADC) += qcom-spmi-iadc.o
>>>>>    obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o
>>>>> diff --git a/drivers/iio/adc/sunxi-gpadc-iio.c
>>>>> b/drivers/iio/adc/sunxi-gpadc-iio.c
>>>>> new file mode 100644
>>>>> index 0000000..5840f43
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
>>>>> @@ -0,0 +1,371 @@
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_device.h>
>>>>> +#include <linux/clk.h>
>>>>> +#include <linux/completion.h>
>>>>> +#include <linux/iio/iio.h>
>>>>> +#include <linux/iio/machine.h>
>>>>> +#include <linux/iio/driver.h>
>>>>> +#include <linux/interrupt.h>
>>>>> +#include <linux/mfd/sunxi-gpadc-mfd.h>
>>>>> +#include <linux/regmap.h>
>>>>> +
>>>> I'm fussy about this:  Please prefix all defines as some of these
>>>> are generic enough they may well end up defined in an included header
>>>> sometime in the future.
>>>>
>>>>> +#define TP_CTRL0            0x00
>>>>> +#define TP_CTRL1            0x04
>>>>> +#define TP_CTRL2            0x08
>>>>> +#define TP_CTRL3            0x0c
>>>>> +#define TP_TPR                0x18
>>>>> +#define TP_CDAT                0x1c
>>>>> +#define TEMP_DATA            0x20
>>>>> +#define TP_DATA                0x24
>>>>> +
>>>>> +/* TP_CTRL0 bits */
>>>>> +#define ADC_FIRST_DLY(x)        ((x) << 24) /* 8 bits */
>>>>> +#define ADC_FIRST_DLY_MODE        BIT(23)
>>>>> +#define ADC_CLK_SELECT            BIT(22)
>>>>> +#define ADC_CLK_DIVIDER(x)        ((x) << 20) /* 2 bits */
>>>>> +#define FS_DIV(x)            ((x) << 16) /* 4 bits */
>>>>> +#define T_ACQ(x)            ((x) << 0)  /* 16 bits*/
>>>>> +
>>>>> +/* TP_CTRL1 bits */
>>>>> +#define STYLUS_UP_DEBOUNCE(x)        ((x) << 12) /* 8 bits */
>>>>> +#define STYLUS_UP_DEBOUNCE_EN        BIT(9)
>>>>> +#define TOUCH_PAN_CALI_EN        BIT(6)
>>>>> +#define TP_DUAL_EN            BIT(5)
>>>>> +#define TP_MODE_EN            BIT(4)
>>>>> +#define TP_ADC_SELECT            BIT(3)
>>>>> +#define ADC_CHAN_SELECT(x)        ((x) << 0)  /* 3 bits */
>>>>> +
>>>>> +/* TP_CTRL1 bits for sun6i SOCs */
>>>>> +#define SUN6I_TOUCH_PAN_CALI_EN        BIT(7)
>>>>> +#define SUN6I_TP_DUAL_EN        BIT(6)
>>>>> +#define SUN6I_TP_MODE_EN        BIT(5)
>>>>> +#define SUN6I_TP_ADC_SELECT        BIT(4)
>>>>> +#define SUN6I_ADC_CHAN_SELECT(x)    BIT(x)  /* 4 bits */
>>>>> +
>>>>> +/* TP_CTRL2 bits */
>>>>> +#define TP_SENSITIVE_ADJUST(x)        ((x) << 28) /* 4 bits */
>>>>> +#define TP_MODE_SELECT(x)        ((x) << 26) /* 2 bits */
>>>>> +#define PRE_MEA_EN            BIT(24)
>>>>> +#define PRE_MEA_THRE_CNT(x)        ((x) << 0)  /* 24 bits*/
>>>>> +
>>>>> +/* TP_CTRL3 bits */
>>>>> +#define FILTER_EN            BIT(2)
>>>>> +#define FILTER_TYPE(x)            ((x) << 0)  /* 2 bits */
>>>>> +
>>>>> +/* TP_INT_FIFOC irq and fifo mask / control bits */
>>>>> +#define TEMP_IRQ_EN            BIT(18)
>>>>> +#define TP_OVERRUN_IRQ_EN        BIT(17)
>>>>> +#define TP_DATA_IRQ_EN            BIT(16)
>>>>> +#define TP_DATA_XY_CHANGE        BIT(13)
>>>>> +#define TP_FIFO_TRIG_LEVEL(x)        ((x) << 8)  /* 5 bits */
>>>>> +#define TP_DATA_DRQ_EN            BIT(7)
>>>>> +#define TP_FIFO_FLUSH            BIT(4)
>>>>> +#define TP_UP_IRQ_EN            BIT(1)
>>>>> +#define TP_DOWN_IRQ_EN            BIT(0)
>>>>> +
>>>>> +/* TP_INT_FIFOS irq and fifo status bits */
>>>>> +#define TEMP_DATA_PENDING        BIT(18)
>>>>> +#define FIFO_OVERRUN_PENDING        BIT(17)
>>>>> +#define FIFO_DATA_PENDING        BIT(16)
>>>>> +#define TP_IDLE_FLG            BIT(2)
>>>>> +#define TP_UP_PENDING            BIT(1)
>>>>> +#define TP_DOWN_PENDING            BIT(0)
>>>>> +
>>>>> +/* TP_TPR bits */
>>>>> +#define TEMP_ENABLE(x)            ((x) << 16)
>>>>> +#define TEMP_PERIOD(x)            ((x) << 0)  /*t = x * 256 * 16 /
>>>>> clkin*/
>>>>> +
>>>>> +#define ARCH_SUN4I            BIT(0)
>>>>> +#define ARCH_SUN5I            BIT(1)
>>>>> +#define ARCH_SUN6I            BIT(2)
>>>>> +
>>>>> +struct sunxi_gpadc_dev {
>>>>> +    void __iomem            *regs;
>>>>> +    struct completion        completion;
>>>>> +    int                temp_data;
>>>>> +    u32                adc_data;
>>>>> +    struct regmap            *regmap;
>>>>> +    unsigned int            fifo_data_irq;
>>>>> +    unsigned int            temp_data_irq;
>>>>> +    unsigned int            flags;
>>>>> +};
>>>>> +
>>>>> +#define ADC_CHANNEL(_channel, _name) {                \
>>>>> +    .type = IIO_VOLTAGE,                    \
>>>>> +    .indexed = 1,                        \
>>>>> +    .channel = _channel,                    \
>>>>> +    .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),        \
>>>>> +    .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),    \
>>>>> +    .datasheet_name = _name,                \
>>>>> +}
>>>>> +
>>>>> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
>>>>> +    {
>>>>> +        .adc_channel_label = "temp_adc",
>>>>> +        .consumer_dev_name = "iio_hwmon.0",
>>>>> +    },
>>>>> +    {},
>>>>> +};
>>>>> +
>>>>> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
>>>>> +    ADC_CHANNEL(0, "adc_chan0"),
>>>>> +    ADC_CHANNEL(1, "adc_chan1"),
>>>>> +    ADC_CHANNEL(2, "adc_chan2"),
>>>>> +    ADC_CHANNEL(3, "adc_chan3"),
>>>>> +    {
>>>>> +        .type = IIO_TEMP,
>>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>>> +        .datasheet_name = "temp_adc",
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int
>>>>> channel)
>>>>> +{
>>>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>>>> +    int val = 0;
>>>>> +
>>>>> +    mutex_lock(&indio_dev->mlock);
>>>>> +
>>>>> +    reinit_completion(&info->completion);
>>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN |
>>>>> +                         SUN6I_TP_ADC_SELECT |
>>>>> +                         SUN6I_ADC_CHAN_SELECT(channel));
>>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>>>>> +                         TP_FIFO_FLUSH);
>>>>> +    enable_irq(info->fifo_data_irq);
>>>>> +
>>>>> +    if (!wait_for_completion_timeout(&info->completion,
>>>>> +                     msecs_to_jiffies(100))) {
>>>>> +        disable_irq(info->fifo_data_irq);
>>>>> +        mutex_unlock(&indio_dev->mlock);
>>>>> +        return -ETIMEDOUT;
>>>>> +    }
>>>>> +
>>>>> +    val = info->adc_data;
>>>>> +    disable_irq(info->fifo_data_irq);
>>>>> +    mutex_unlock(&indio_dev->mlock);
>>>>> +
>>>>> +    return val;
>>>>> +}
>>>>> +
>>>>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev)
>>>>> +{
>>>>> +    struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>>>> +    int val = 0;
>>>>> +
>>>>> +    mutex_lock(&indio_dev->mlock);
>>>>> +
>>>>> +    reinit_completion(&info->completion);
>>>>> +
>>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, TP_FIFO_TRIG_LEVEL(1) |
>>>>> +                         TP_FIFO_FLUSH);
>>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>>>>> +    enable_irq(info->temp_data_irq);
>>>>> +
>>>>> +    if (!wait_for_completion_timeout(&info->completion,
>>>>> +                     msecs_to_jiffies(100))) {
>>>>> +        disable_irq(info->temp_data_irq);
>>>>> +        mutex_unlock(&indio_dev->mlock);
>>>>> +        return -ETIMEDOUT;
>>>>> +    }
>>>>> +
>>>>> +    if (info->flags & ARCH_SUN4I)
>>>>> +        val = info->temp_data * 133 - 257000;
>>>>> +    else if (info->flags & ARCH_SUN5I)
>>>>> +        val = info->temp_data * 100 - 144700;
>>>>> +    else if (info->flags & ARCH_SUN6I)
>>>>> +        val = info->temp_data * 167 - 271000;
>>>>> +
>>>>> +    disable_irq(info->temp_data_irq);
>>>>> +    mutex_unlock(&indio_dev->mlock);
>>>>> +    return val;
>>>>> +}
>>>>> +
>>>>> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
>>>>> +                struct iio_chan_spec const *chan,
>>>>> +                int *val, int *val2, long mask)
>>>>> +{
>>>>> +    switch (mask) {
>>>>> +    case IIO_CHAN_INFO_PROCESSED:
>>>>> +        *val = sunxi_gpadc_temp_read(indio_dev);
>>>>> +        if (*val < 0)
>>>>> +            return *val;
>>>>> +
>>>>> +        return IIO_VAL_INT;
>>>>> +    case IIO_CHAN_INFO_RAW:
>>>>> +        *val = sunxi_gpadc_adc_read(indio_dev, chan->channel);
>>>>> +        if (*val < 0)
>>>>> +            return *val;
>>>>> +
>>>>> +        return IIO_VAL_INT;
>>>>> +    default:
>>>>> +        break;
>>>>> +    }
>>>>> +
>>>>> +    return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static const struct iio_info sunxi_gpadc_iio_info = {
>>>>> +    .read_raw = sunxi_gpadc_read_raw,
>>>>> +    .driver_module = THIS_MODULE,
>>>>> +};
>>>>> +
>>>>> +static irqreturn_t sunxi_gpadc_temp_data_irq_handler(int irq, void
>>>>> *dev_id)
>>>>> +{
>>>>> +    struct sunxi_gpadc_dev *info = dev_id;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = regmap_read(info->regmap, TEMP_DATA, &info->temp_data);
>>>>> +    if (ret == 0)
>>>>> +        complete(&info->completion);
>>>>> +
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>> Interesting that it's a separate data flow for the temperature sensor.
>>>> That means that if you add buffered (pushed data) support in future
>>>> either
>>>> you'll need to split the device in two or not allow the temperature
>>>> channel
>>>> to go through the buffer interface.
>>>>> +
>>>>> +static irqreturn_t sunxi_gpadc_fifo_data_irq_handler(int irq, void
>>>>> *dev_id)
>>>>> +{
>>>>> +    struct sunxi_gpadc_dev *info = dev_id;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = regmap_read(info->regmap, TP_DATA, &info->adc_data);
>>>>> +    if (ret == 0)
>>>>> +        complete(&info->completion);
>>>>> +
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static int sunxi_gpadc_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct sunxi_gpadc_dev *info = NULL;
>>>>> +    struct iio_dev *indio_dev = NULL;
>>>>> +    int ret = 0;
>>>>> +    unsigned int irq;
>>>>> +    struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
>>>>> +
>>>>> +    sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
>>>>> +
>>>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
>>>>> +    if (!indio_dev) {
>>>>> +        dev_err(&pdev->dev, "failed to allocate iio device.\n");
>>>>> +        return -ENOMEM;
>>>>> +    }
>>>>> +    info = iio_priv(indio_dev);
>>>>> +
>>>>> +    info->regmap = sunxi_gpadc_mfd_dev->regmap;
>>>>> +    init_completion(&info->completion);
>>>>> +    indio_dev->name = dev_name(&pdev->dev);
>>>>> +    indio_dev->dev.parent = &pdev->dev;
>>>>> +    indio_dev->dev.of_node = pdev->dev.of_node;
>>>>> +    indio_dev->info = &sunxi_gpadc_iio_info;
>>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> +    indio_dev->num_channels = ARRAY_SIZE(sunxi_gpadc_channels);
>>>>> +    indio_dev->channels = sunxi_gpadc_channels;
>>>>> +
>>>>> +    info->flags = platform_get_device_id(pdev)->driver_data;
>>>>> +
>>>>> +    regmap_write(info->regmap, TP_CTRL0, ADC_CLK_DIVIDER(2) |
>>>>> +                         FS_DIV(7) |
>>>>> +                         T_ACQ(63));
>>>>> +    regmap_write(info->regmap, TP_CTRL1, SUN6I_TP_MODE_EN);
>>>>> +    regmap_write(info->regmap, TP_CTRL3, FILTER_EN | FILTER_TYPE(1));
>>>>> +    regmap_write(info->regmap, TP_TPR, TEMP_ENABLE(1) |
>>>>> TEMP_PERIOD(1953));
>>>>> +
>>>>> +    irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>>>>> +    if (irq < 0) {
>>>>> +        dev_err(&pdev->dev,
>>>>> +            "no TEMP_DATA_PENDING interrupt registered\n");
>>>>> +        return irq;
>>>>> +    }
>>>>> +
>>>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,
>>>>> +                       sunxi_gpadc_temp_data_irq_handler,
>>>>> +                       0, "temp_data", info);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&pdev->dev,
>>>>> +            "could not request TEMP_DATA_PENDING interrupt: %d\n",
>>>>> +            ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    info->temp_data_irq = irq;
>>>>> +    disable_irq(irq);
>>>>> +
>>>>> +    irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>>>> hohum.  A fifo?  This part is getting more interesting ;) I'll have to
>>>> dig out the datasheet at some point (if public).
>>>>> +    if (irq < 0) {
>>>>> +        dev_err(&pdev->dev,
>>>>> +            "no FIFO_DATA_PENDING interrupt registered\n");
>>>>> +        return irq;
>>>>> +    }
>>>>> +
>>>>> +    irq = regmap_irq_get_virq(sunxi_gpadc_mfd_dev->regmap_irqc, irq);
>>>>> +    ret = devm_request_any_context_irq(&pdev->dev, irq,
>>>>> +                       sunxi_gpadc_fifo_data_irq_handler,
>>>>> +                       0, "fifo_data", info);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&pdev->dev,
>>>>> +            "could not request FIFO_DATA_PENDING interrupt: %d\n",
>>>>> +            ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    info->fifo_data_irq = irq;
>>>>> +    disable_irq(irq);
>>>>> +
>>>>> +    ret = iio_map_array_register(indio_dev, sunxi_gpadc_hwmon_maps);
>>>> As mentioned for previous patch I think this should be described
>>>> externally.
>>>> Chances are that some of those other adc channels are also going to be
>>>> in reality used for hwmon anyway so doing it in the device tree will
>>>> give
>>>> you more flexibility.
>>>>
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&pdev->dev, "failed to register iio map array\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    platform_set_drvdata(pdev, indio_dev);
>>>>> +
>>>>> +    ret = iio_device_register(indio_dev);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(&pdev->dev, "could not register the device\n");
>>>>> +        iio_map_array_unregister(indio_dev);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>> This is kind of self evident when the device turns up so I'd not bother
>>>> cluttering up the logs with it as no additional information is given.
>>>>> +    dev_info(&pdev->dev, "successfully loaded\n");
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int sunxi_gpadc_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct iio_dev *indio_dev = NULL;
>>>> ?  If it's null the below isn't going to work as intended.
>>>> Missing a platform_get_drvdata which I'd just roll into the above
>>>> local variable definition.
>>>>
>>>>> +    struct sunxi_gpadc_dev *info = NULL;
>>>>> +
>>>>> +    iio_device_unregister(indio_dev);
>>>>> +    iio_map_array_unregister(indio_dev);
>>>>> +    info = iio_priv(indio_dev);
>>>> I'd roll this into the local variable declaration above
>>>> (it's only a trivial bit of pointer arithemetic after all.
>>>> struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>>>>
>>>>> +    regmap_write(info->regmap, TP_INT_FIFOC, 0);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>>>>> +    { "sun4i-a10-gpadc-iio", ARCH_SUN4I },
>>>>> +    { "sun5i-a13-gpadc-iio", ARCH_SUN5I },
>>>>> +    { "sun6i-a31-gpadc-iio", ARCH_SUN6I },
>>>>> +    { /*sentinel*/ },
>>>>> +};
>>>>> +
>>>>> +static struct platform_driver sunxi_gpadc_driver = {
>>>>> +    .driver = {
>>>>> +        .name = "sunxi-gpadc-iio",
>>>>> +    },
>>>>> +    .id_table = sunxi_gpadc_id,
>>>>> +    .probe = sunxi_gpadc_probe,
>>>>> +    .remove = sunxi_gpadc_remove,
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(sunxi_gpadc_driver);
>>>>> +
>>>>> +MODULE_DESCRIPTION("ADC driver for sunxi platforms");
>>>>> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>>> -- 
>>>> To unsubscribe from this list: send the line "unsubscribe
>>>> linux-hwmon" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>
> 

[PATCH 3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Quentin Schulz]

iio_channel_get_all returns -ENODEV when it cannot find either phandles and
properties in the Device Tree or channels whose consumer_dev_name matches
iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
which might be probed after iio_hwmon.

It is better to defer the probe of iio_hwmon if such error is returned by
iio_channel_get_all in order to let a chance to iio drivers to expose
channels in iio_map_list.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/hwmon/iio_hwmon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..c0da4d9 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		name = dev->of_node->name;
 
 	channels = iio_channel_get_all(dev);
-	if (IS_ERR(channels))
+	if (IS_ERR(channels)) {
+		if (PTR_ERR(channels) == -ENODEV)
+			return -EPROBE_DEFER;
 		return PTR_ERR(channels);
+	}
 
 	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
 	if (st == NULL) {
-- 
2.5.0

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Guenter Roeck]

On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> properties in the Device Tree or channels whose consumer_dev_name matches
> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> which might be probed after iio_hwmon.
> 
> It is better to defer the probe of iio_hwmon if such error is returned by
> iio_channel_get_all in order to let a chance to iio drivers to expose
> channels in iio_map_list.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>  drivers/hwmon/iio_hwmon.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index b550ba5..c0da4d9 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		name = dev->of_node->name;
>  
>  	channels = iio_channel_get_all(dev);
> -	if (IS_ERR(channels))
> +	if (IS_ERR(channels)) {
> +		if (PTR_ERR(channels) == -ENODEV)
> +			return -EPROBE_DEFER;

The problem, as I see it, is with iio, which should return -EPROBE_DEFER
in this situation.

We can not convert -ENODEV to -EPROBE_DEFER without risking that the
channels are _really_ not there, which would result in endless "deferred"
messages.

Guenter

>  		return PTR_ERR(channels);
> +	}
>  
>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>  	if (st == NULL) {

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Jonathan Cameron]

On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>> iio_channel_get_all returns -ENODEV when it cannot find either
>phandles and
>> properties in the Device Tree or channels whose consumer_dev_name
>matches
>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>drivers
>> which might be probed after iio_hwmon.
>> 
>> It is better to defer the probe of iio_hwmon if such error is
>returned by
>> iio_channel_get_all in order to let a chance to iio drivers to expose
>> channels in iio_map_list.
>> 
>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>> ---
>>  drivers/hwmon/iio_hwmon.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> index b550ba5..c0da4d9 100644
>> --- a/drivers/hwmon/iio_hwmon.c
>> +++ b/drivers/hwmon/iio_hwmon.c
>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>*pdev)
>>  		name = dev->of_node->name;
>>  
>>  	channels = iio_channel_get_all(dev);
>> -	if (IS_ERR(channels))
>> +	if (IS_ERR(channels)) {
>> +		if (PTR_ERR(channels) == -ENODEV)
>> +			return -EPROBE_DEFER;
>
>The problem, as I see it, is with iio, which should return
>-EPROBE_DEFER
>in this situation.
Agreed. New fangled stuff this deferred probing :) 
>
>We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>channels are _really_ not there, which would result in endless
>"deferred"
>messages.
Hmm not entirely sure how we prevent that happening wherever it is done..

>
>Guenter
>
>>  		return PTR_ERR(channels);
>> +	}
>>  
>>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>  	if (st == NULL) {
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Lars-Peter Clausen]

On 06/30/2016 03:59 PM, Jonathan Cameron wrote:
> 
> 
> On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>>> iio_channel_get_all returns -ENODEV when it cannot find either
>> phandles and
>>> properties in the Device Tree or channels whose consumer_dev_name
>> matches
>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>> drivers
>>> which might be probed after iio_hwmon.
>>>
>>> It is better to defer the probe of iio_hwmon if such error is
>> returned by
>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>> channels in iio_map_list.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> ---
>>>  drivers/hwmon/iio_hwmon.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>> index b550ba5..c0da4d9 100644
>>> --- a/drivers/hwmon/iio_hwmon.c
>>> +++ b/drivers/hwmon/iio_hwmon.c
>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>> *pdev)
>>>  		name = dev->of_node->name;
>>>  
>>>  	channels = iio_channel_get_all(dev);
>>> -	if (IS_ERR(channels))
>>> +	if (IS_ERR(channels)) {
>>> +		if (PTR_ERR(channels) == -ENODEV)
>>> +			return -EPROBE_DEFER;
>>
>> The problem, as I see it, is with iio, which should return
>> -EPROBE_DEFER
>> in this situation.
> Agreed. New fangled stuff this deferred probing :) 
>>
>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>> channels are _really_ not there, which would result in endless
>> "deferred"
>> messages.
> Hmm not entirely sure how we prevent that happening wherever it is done..

EPROBE_DEFER is supposed to be returned if you know that a provider was
specified, but the provider is not yet available. Whereas
ENODEV/ENOENT/ENXIO (choose your favorite) is supposed to be returned when
no provider is available.

In the case of the bridge the later case does not really makes sense though
since it wouldn't work without any channels.

This of course requires that all provider-consumer maps are available before
the first consumer is registered. Which is not the case with IIO where the
provider registers the consumer map. We should really do what GPIO does
which registers the map in the board file.

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Guenter Roeck]

On 06/30/2016 06:59 AM, Jonathan Cameron wrote:
>
>
> On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>>> iio_channel_get_all returns -ENODEV when it cannot find either
>> phandles and
>>> properties in the Device Tree or channels whose consumer_dev_name
>> matches
>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>> drivers
>>> which might be probed after iio_hwmon.
>>>
>>> It is better to defer the probe of iio_hwmon if such error is
>> returned by
>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>> channels in iio_map_list.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> ---
>>>   drivers/hwmon/iio_hwmon.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>> index b550ba5..c0da4d9 100644
>>> --- a/drivers/hwmon/iio_hwmon.c
>>> +++ b/drivers/hwmon/iio_hwmon.c
>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>> *pdev)
>>>   		name = dev->of_node->name;
>>>
>>>   	channels = iio_channel_get_all(dev);
>>> -	if (IS_ERR(channels))
>>> +	if (IS_ERR(channels)) {
>>> +		if (PTR_ERR(channels) == -ENODEV)
>>> +			return -EPROBE_DEFER;
>>
>> The problem, as I see it, is with iio, which should return
>> -EPROBE_DEFER
>> in this situation.
> Agreed. New fangled stuff this deferred probing :)
>>
>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>> channels are _really_ not there, which would result in endless
>> "deferred"
>> messages.
> Hmm not entirely sure how we prevent that happening wherever it is done..
>

Outch. Better at the source, though. I didn't look at the iio code recently,
but can you detect the defer situation at least with devicetree ?

For non-devicetree situations, the only option I can think of would be
to replace the module initcall with a later initcall. That should solve
the problem if both iio_hwmon and and underlying drivers are built
into the kernel. If iio_hwmon is modular, the only real option I can
see is to make sure that all drivers it needs are loaded first.

Does this make sense ?

Guenter

>>
>> Guenter
>>
>>>   		return PTR_ERR(channels);
>>> +	}
>>>
>>>   	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>   	if (st == NULL) {
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Jonathan Cameron]

On 30/06/16 15:51, Guenter Roeck wrote:
> On 06/30/2016 06:59 AM, Jonathan Cameron wrote:
>>
>>
>> On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>>>> iio_channel_get_all returns -ENODEV when it cannot find either
>>> phandles and
>>>> properties in the Device Tree or channels whose consumer_dev_name
>>> matches
>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>>> drivers
>>>> which might be probed after iio_hwmon.
>>>>
>>>> It is better to defer the probe of iio_hwmon if such error is
>>> returned by
>>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>>> channels in iio_map_list.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>> ---
>>>>   drivers/hwmon/iio_hwmon.c | 5 ++++-
>>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>>> index b550ba5..c0da4d9 100644
>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>>> *pdev)
>>>>           name = dev->of_node->name;
>>>>
>>>>       channels = iio_channel_get_all(dev);
>>>> -    if (IS_ERR(channels))
>>>> +    if (IS_ERR(channels)) {
>>>> +        if (PTR_ERR(channels) == -ENODEV)
>>>> +            return -EPROBE_DEFER;
>>>
>>> The problem, as I see it, is with iio, which should return
>>> -EPROBE_DEFER
>>> in this situation.
>> Agreed. New fangled stuff this deferred probing :)
>>>
>>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>>> channels are _really_ not there, which would result in endless
>>> "deferred"
>>> messages.
>> Hmm not entirely sure how we prevent that happening wherever it is done..
>>
> 
> Outch. Better at the source, though. I didn't look at the iio code recently,
> but can you detect the defer situation at least with devicetree ?
> 
> For non-devicetree situations, the only option I can think of would be
> to replace the module initcall with a later initcall. That should solve
> the problem if both iio_hwmon and and underlying drivers are built
> into the kernel. If iio_hwmon is modular, the only real option I can
> see is to make sure that all drivers it needs are loaded first.
> 
> Does this make sense ?
I think we need to look in a couple of directions.  Firstly, investigate doing
something similar to gpio and basically move the setup of maps much earlier.
This will replace drivers presenting their own maps

The other direction is to get userspace (i.e. configfs) setup of these maps
working so for cases where it's a bit less hardware defined (i.e. using an
accelerometer as an input device) we can do once we know all devices relevant
are present and instantiate new instances on the fly.

Anyhow, neither is trivial unfortunately.

J
> 
> Guenter
> 
>>>
>>> Guenter
>>>
>>>>           return PTR_ERR(channels);
>>>> +    }
>>>>
>>>>       st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>       if (st == NULL) {
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [3/3] hwmon: iio_hwmon: defer probe when no channel is found

[Guenter Roeck]

On 07/03/2016 03:47 AM, Jonathan Cameron wrote:
> On 30/06/16 15:51, Guenter Roeck wrote:
>> On 06/30/2016 06:59 AM, Jonathan Cameron wrote:
>>>
>>>
>>> On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, Quentin Schulz wrote:
>>>>> iio_channel_get_all returns -ENODEV when it cannot find either
>>>> phandles and
>>>>> properties in the Device Tree or channels whose consumer_dev_name
>>>> matches
>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio
>>>> drivers
>>>>> which might be probed after iio_hwmon.
>>>>>
>>>>> It is better to defer the probe of iio_hwmon if such error is
>>>> returned by
>>>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>>>> channels in iio_map_list.
>>>>>
>>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>>> ---
>>>>>    drivers/hwmon/iio_hwmon.c | 5 ++++-
>>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>>>> index b550ba5..c0da4d9 100644
>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device
>>>> *pdev)
>>>>>            name = dev->of_node->name;
>>>>>
>>>>>        channels = iio_channel_get_all(dev);
>>>>> -    if (IS_ERR(channels))
>>>>> +    if (IS_ERR(channels)) {
>>>>> +        if (PTR_ERR(channels) == -ENODEV)
>>>>> +            return -EPROBE_DEFER;
>>>>
>>>> The problem, as I see it, is with iio, which should return
>>>> -EPROBE_DEFER
>>>> in this situation.
>>> Agreed. New fangled stuff this deferred probing :)
>>>>
>>>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the
>>>> channels are _really_ not there, which would result in endless
>>>> "deferred"
>>>> messages.
>>> Hmm not entirely sure how we prevent that happening wherever it is done..
>>>
>>
>> Outch. Better at the source, though. I didn't look at the iio code recently,
>> but can you detect the defer situation at least with devicetree ?
>>
>> For non-devicetree situations, the only option I can think of would be
>> to replace the module initcall with a later initcall. That should solve
>> the problem if both iio_hwmon and and underlying drivers are built
>> into the kernel. If iio_hwmon is modular, the only real option I can
>> see is to make sure that all drivers it needs are loaded first.
>>
>> Does this make sense ?
> I think we need to look in a couple of directions.  Firstly, investigate doing
> something similar to gpio and basically move the setup of maps much earlier.
> This will replace drivers presenting their own maps
>
> The other direction is to get userspace (i.e. configfs) setup of these maps
> working so for cases where it's a bit less hardware defined (i.e. using an
> accelerometer as an input device) we can do once we know all devices relevant
> are present and instantiate new instances on the fly.
>
> Anyhow, neither is trivial unfortunately.
>

On a higher level, I think it would be better if the iio-hwmon bridge was tied
to chips (and thus to the chip driver), and not be an independent binding.
No idea if/how we can do that, though.

Guenter

Re: [PATCH 0/3] add support for Allwinner SoCs ADC

[Chen-Yu Tsai]

Hi,

On Tue, Jun 28, 2016 at 4:45 PM, Quentin Schulz
<quentin.schulz@free-electrons.com> wrote:
> The Allwinner SoCs all have an ADC that can also act as a touchscreen controller
> and a thermal sensor. The first four channels can be used either for the ADC or
> the touchscreen and the fifth channel is used for the thermal sensor. We
> currently have a driver for the two latter functions in
> drivers/input/touchscreen/sun4i-ts.c but we don't have access to the ADC feature
> at all.
>
> This adds initial support for Allwinner SoCs ADC with all features. Yet, the
> touchscreen is not implemented but will be added later. To switch between
> touchscreen and ADC modes, you need to poke few bits in registers and
> (de)activate an interrupt (pen-up).
> A MFD is provided to let the input driver activate the pen-up interrupt through
> virtual interrupt, poke few bits via regmap and read data from the ADC driver
> while both (and iio_hwmon) are probed by the MFD.

I take it that we are going to replace the original sun4i-ts driver
with this new mfd one, and various sub device drivers?

> There exists slight modifications between the different SoCs ADC like the
> address of some registers and the scale and offset to apply to thermal sensor
> raw values. These modifications are done by drivers on different
> platform_device_id passed by the MFD when probing subdrivers.
>
> This also modifies iio-hwmon to allow probe deferring when no iio channel is
> found. Currently when no iio channel is found, the probing of iio-hwmon fails.
> This is problematic when iio-hwmon probes before the iio driver could register
> iio channels to share.

One thing about iio-hwmon is that it doesn't support hwmon labels.
Any chance you could improve this, so we see the same names in userspace?

Thanks
ChenYu

>
> Quentin Schulz (3):
>   mfd: add support for Allwinner SoCs ADC
>   iio: adc: add support for Allwinner SoCs ADC
>   hwmon: iio_hwmon: defer probe when no channel is found
>
>  drivers/hwmon/iio_hwmon.c           |   5 +-
>  drivers/iio/adc/Kconfig             |  12 ++
>  drivers/iio/adc/Makefile            |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c   | 371 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/Kconfig                 |  14 ++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 188 ++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  14 ++
>  8 files changed, 606 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>
> --
> 2.5.0
>

Re: [PATCH 0/3] add support for Allwinner SoCs ADC

[Quentin Schulz]

On 29/06/2016 05:28, Chen-Yu Tsai wrote:
> Hi,
> 
> On Tue, Jun 28, 2016 at 4:45 PM, Quentin Schulz
> <quentin.schulz@free-electrons.com> wrote:
>> The Allwinner SoCs all have an ADC that can also act as a touchscreen controller
>> and a thermal sensor. The first four channels can be used either for the ADC or
>> the touchscreen and the fifth channel is used for the thermal sensor. We
>> currently have a driver for the two latter functions in
>> drivers/input/touchscreen/sun4i-ts.c but we don't have access to the ADC feature
>> at all.
>>
>> This adds initial support for Allwinner SoCs ADC with all features. Yet, the
>> touchscreen is not implemented but will be added later. To switch between
>> touchscreen and ADC modes, you need to poke few bits in registers and
>> (de)activate an interrupt (pen-up).
>> A MFD is provided to let the input driver activate the pen-up interrupt through
>> virtual interrupt, poke few bits via regmap and read data from the ADC driver
>> while both (and iio_hwmon) are probed by the MFD.
> 
> I take it that we are going to replace the original sun4i-ts driver
> with this new mfd one, and various sub device drivers?
> 

Yes, that's the spirit.

[...]

> One thing about iio-hwmon is that it doesn't support hwmon labels.
> Any chance you could improve this, so we see the same names in userspace?

Should be an easy patch: reading datasheet_name from iio_chan_spec of
the iio_channel in iio_hwmon_probe, if none then fall back to current
naming convention.

I don't think it is linked to this serie of patches so I prefer to look
at it later.

Thanks!