[PATCH v3 0/4] add support for Allwinner SoCs ADC

[PATCH v3 0/4] 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. It is meant to replace the current driver by using MFD and
subdrivers.

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 a few bits in registers
and (de)activate an interrupt (pen-up).
An MFD is provided to let the input driver activate the pen-up interrupt
through a virtual interrupt, poke a few bits via regmap and read data from
the ADC driver while both (and iio_hwmon) are probed by the MFD.

There are slight variations between the different SoCs ADC like the address
of some registers and the scale and offset to apply to raw thermal sensor
values. These variations are handled by using different platform_device_id,
passed to the sub-drivers when they are probed by the MFD.

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. Thus, this modifies iio-hwmon to probe late
with late_initcall making sure all drivers which provides iio channels have
probed before iio_hwmon.

When an MFD cell has an of_compatible (meaning it is present in the Device
Tree), other nodes can reference it using a phandle.
However when the MFD cell is not declared in the Device Tree, the only way
other nodes can reference it are by using a phandle to the MFD. Then when
this MFD cell tries to register itself in one framework, the registration
is denied by the framework because it is not matching the of_node of the
node which is referenced by the phandle in one of the other nodes.
This reattaches the of_node of the MFD to the MFD cell device structure
when the MFD cell has no of_compatible.
We need this modification to register the thermal sensor in the thermal
framework.

Removal of proposed patch for iio_hwmon's iio channel's label in v3. The
patch induces irreversible ABI changes and will be handled as a separate
patch since I think it is not absolutely necessary to have labels yet in
iio_hwmon.

Quentin Schulz (4):
  hwmon: iio_hwmon: delay probing with late_initcall
  mfd: add support for Allwinner SoCs ADC
  mfd: mfd-core: reattach mfd of_node to cells without of_compatible
  iio: adc: add support for Allwinner SoCs ADC

 drivers/hwmon/iio_hwmon.c           |  16 +-
 drivers/iio/adc/Kconfig             |  12 +
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c   | 513 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig                 |  15 ++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/mfd-core.c              |  14 +-
 drivers/mfd/sunxi-gpadc-mfd.c       | 189 +++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  94 +++++++
 9 files changed, 850 insertions(+), 6 deletions(-)
 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 v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

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

This makes sure iio_hwmon is probed after all iio drivers which provides
channels to iio_hwmon are probed, be they present in the DT or using
iio_map_list.

This replaces module_platform_driver() by an explicit code variant which
calls late_initcall() install of module_init(), meaning it probes after
all the drivers using module_init() as their init.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v3:
 - use late_initcall instead of deferring probe,

 drivers/hwmon/iio_hwmon.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..0a00bfb 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -192,7 +192,21 @@ static struct platform_driver __refdata iio_hwmon_driver = {
 	.remove = iio_hwmon_remove,
 };
 
-module_platform_driver(iio_hwmon_driver);
+static struct platform_driver * const drivers[] = {
+	&iio_hwmon_driver,
+};
+
+static int __init iio_hwmon_late_init(void)
+{
+	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
+}
+late_initcall(iio_hwmon_late_init);
+
+static void __exit iio_hwmon_exit(void)
+{
+	platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_exit(iio_hwmon_exit);
 
 MODULE_AUTHOR("Jonathan Cameron <jic23@kernel.org>");
 MODULE_DESCRIPTION("IIO to hwmon driver");
-- 
2.5.0

[PATCH v3 2/4] 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>
---

v3:
 - use defines in regmap_irq instead of hard coded BITs,
 - use of_device_id data field to chose which MFD cells to add considering
   the compatible responsible of the MFD probe,
 - remove useless initializations,
 - disable all interrupts before adding them to regmap_irqchip,
 - add goto error label in probe,
 - correct wrapping in header license,
 - move defines from IIO driver to header,
 - use GENMASK to limit the size of the variable passed to a macro,
 - prefix register BIT defines with the name of the register,
 - reorder defines,

v2:
 - add license headers,
 - reorder alphabetically includes,
 - add SUNXI_GPADC_ prefixes for defines,

 drivers/mfd/Kconfig                 |  15 +++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 189 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  94 ++++++++++++++++++
 4 files changed, 300 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 1bcf601..6180c2d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -29,6 +29,21 @@ config MFD_ACT8945A
 	  linear regulators, along with a complete ActivePath battery
 	  charger.
 
+config MFD_SUN4I_GPADC
+	tristate "Allwinner sunxi platforms' GPADC MFD driver"
+	select MFD_CORE
+	select REGMAP_MMIO
+	depends on ARCH_SUNXI || COMPILE_TEST
+	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_AS3711
 	bool "AMS AS3711"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 42a66e1..9dfd033 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -205,3 +205,5 @@ 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
 obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
+
+obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sunxi-gpadc-mfd.o
diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
new file mode 100644
index 0000000..3d70af0
--- /dev/null
+++ b/drivers/mfd/sunxi-gpadc-mfd.c
@@ -0,0 +1,189 @@
+/* ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/regmap.h>
+
+#include <linux/mfd/sunxi-gpadc-mfd.h>
+
+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,
+		       SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_IRQ_EN),
+	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0,
+		       SUNXI_GPADC_TP_INT_FIFOC_TEMP_IRQ_EN),
+};
+
+static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
+	.name = "sunxi_gpadc_mfd_irq_chip",
+	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
+	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
+	.mask_base = SUNXI_GPADC_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 const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
+	{
+		.compatible = "allwinner,sun4i-a10-ts",
+		.data = &sun4i_gpadc_mfd_cells,
+	}, {
+		.compatible = "allwinner,sun5i-a13-ts",
+		.data = &sun5i_gpadc_mfd_cells,
+	}, {
+		.compatible = "allwinner,sun6i-a31-ts",
+		.data = &sun6i_gpadc_mfd_cells,
+	}, { /* sentinel */ }
+};
+
+static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *mfd_dev;
+	struct resource *mem;
+	const struct of_device_id *of_id;
+	const struct mfd_cell *mfd_cells;
+	unsigned int irq;
+	int ret;
+
+	of_id = of_match_node(sunxi_gpadc_mfd_of_match, pdev->dev.of_node);
+	if (!of_id)
+		return -EINVAL;
+
+	mfd_dev = devm_kzalloc(&pdev->dev, sizeof(*mfd_dev), GFP_KERNEL);
+	if (!mfd_dev)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(mfd_dev->regs))
+		return PTR_ERR(mfd_dev->regs);
+
+	mfd_dev->dev = &pdev->dev;
+	dev_set_drvdata(mfd_dev->dev, mfd_dev);
+
+	mfd_dev->regmap = devm_regmap_init_mmio(mfd_dev->dev, mfd_dev->regs,
+						&sunxi_gpadc_mfd_regmap_config);
+	if (IS_ERR(mfd_dev->regmap)) {
+		ret = PTR_ERR(mfd_dev->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
+
+	/* Disable all interrupts */
+	regmap_write(mfd_dev->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
+
+	irq = platform_get_irq(pdev, 0);
+	ret = regmap_add_irq_chip(mfd_dev->regmap, irq, IRQF_ONESHOT, 0,
+				  &sunxi_gpadc_mfd_regmap_irq_chip,
+				  &mfd_dev->regmap_irqc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
+		return ret;
+	}
+
+	mfd_cells = of_id->data;
+	ret = mfd_add_devices(mfd_dev->dev, 0, mfd_cells, 2, NULL, 0, NULL);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+err:
+	regmap_del_irq_chip(irq, mfd_dev->regmap_irqc);
+	return ret;
+}
+
+static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *mfd_dev;
+	unsigned int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	mfd_remove_devices(&pdev->dev);
+	mfd_dev = dev_get_drvdata(&pdev->dev);
+	regmap_del_irq_chip(irq, mfd_dev->regmap_irqc);
+
+	return 0;
+}
+
+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("Allwinner sunxi platforms' GPADC MFD core driver");
+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..c21abae
--- /dev/null
+++ b/include/linux/mfd/sunxi-gpadc-mfd.h
@@ -0,0 +1,94 @@
+/* Header of ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * This program is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License version 2 as published by the
+ * Free Software Foundation.
+ */
+
+#ifndef __SUNXI_GPADC_MFD__H__
+#define __SUNXI_GPADC_MFD__H__
+
+#define SUNXI_GPADC_TP_CTRL0				0x00
+
+#define SUNXI_GPADC_TP_CTRL0_ADC_FIRST_DLY(x)		((GENMASK(7, 0) & (x)) << 24)
+#define SUNXI_GPADC_TP_CTRL0_ADC_FIRST_DLY_MODE		BIT(23)
+#define SUNXI_GPADC_TP_CTRL0_ADC_CLK_SELECT		BIT(22)
+#define SUNXI_GPADC_TP_CTRL0_ADC_CLK_DIVIDER(x)		((GENMASK(1, 0) & (x)) << 20)
+#define SUNXI_GPADC_TP_CTRL0_FS_DIV(x)			((GENMASK(3, 0) & (x)) << 16)
+#define SUNXI_GPADC_TP_CTRL0_T_ACQ(x)			(GENMASK(15, 0) & (x))
+
+#define SUNXI_GPADC_TP_CTRL1				0x04
+
+#define SUNXI_GPADC_TP_CTRL1_STYLUS_UP_DEBOUNCE(x)	((GENMASK(7, 0) & (x)) << 12)
+#define SUNXI_GPADC_TP_CTRL1_STYLUS_UP_DEBOUNCE_EN	BIT(9)
+#define SUNXI_GPADC_TP_CTRL1_TOUCH_PAN_CALI_EN		BIT(6)
+#define SUNXI_GPADC_TP_CTRL1_TP_DUAL_EN			BIT(5)
+#define SUNXI_GPADC_TP_CTRL1_TP_MODE_EN			BIT(4)
+#define SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT		BIT(3)
+#define SUNXI_GPADC_TP_CTRL1_ADC_CHAN_SELECT(x)		(GENMASK(2, 0) & (x))
+
+/* TP_CTRL1 bits for sun6i SOCs */
+#define SUNXI_GPADC_TP_CTRL1_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
+#define SUNXI_GPADC_TP_CTRL1_SUN6I_TP_DUAL_EN		BIT(6)
+#define SUNXI_GPADC_TP_CTRL1_SUN6I_TP_MODE_EN		BIT(5)
+#define SUNXI_GPADC_TP_CTRL1_SUN6I_TP_ADC_SELECT	BIT(4)
+#define SUNXI_GPADC_TP_CTRL1_SUN6I_ADC_CHAN_SELECT(x)	(GENMASK(3, 0) & BIT(x))
+
+#define SUNXI_GPADC_TP_CTRL2				0x08
+
+#define SUNXI_GPADC_TP_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)
+#define SUNXI_GPADC_TP_CTRL2_TP_MODE_SELECT(x)		((GENMASK(1, 0) & (x)) << 26)
+#define SUNXI_GPADC_TP_CTRL2_PRE_MEA_EN			BIT(24)
+#define SUNXI_GPADC_TP_CTRL2_PRE_MEA_THRE_CNT(x)	(GENMASK(23, 0) & (x))
+
+#define SUNXI_GPADC_TP_CTRL3				0x0c
+
+#define SUNXI_GPADC_TP_CTRL3_FILTER_EN			BIT(2)
+#define SUNXI_GPADC_TP_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
+
+#define SUNXI_GPADC_TP_TPR				0x18
+
+#define SUNXI_GPADC_TP_TPR_TEMP_ENABLE			BIT(16)
+#define SUNXI_GPADC_TP_TPR_TEMP_PERIOD(x)		(GENMASK(15, 0) & (x))
+
+#define SUNXI_GPADC_TP_INT_FIFOC			0x10
+
+#define SUNXI_GPADC_TP_INT_FIFOC_TEMP_IRQ_EN		BIT(18)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_OVERRUN_IRQ_EN	BIT(17)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_IRQ_EN		BIT(16)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_XY_CHANGE	BIT(13)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(x)	((GENMASK(4, 0) & (x)) << 8)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_DRQ_EN		BIT(7)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH		BIT(4)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
+#define SUNXI_GPADC_TP_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
+
+#define SUNXI_GPADC_TP_INT_FIFOS			0x14
+
+#define SUNXI_GPADC_TP_INT_FIFOS_TEMP_DATA_PENDING	BIT(18)
+#define SUNXI_GPADC_TP_INT_FIFOS_FIFO_OVERRUN_PENDING	BIT(17)
+#define SUNXI_GPADC_TP_INT_FIFOS_FIFO_DATA_PENDING	BIT(16)
+#define SUNXI_GPADC_TP_INT_FIFOS_TP_IDLE_FLG		BIT(2)
+#define SUNXI_GPADC_TP_INT_FIFOS_TP_UP_PENDING		BIT(1)
+#define SUNXI_GPADC_TP_INT_FIFOS_TP_DOWN_PENDING	BIT(0)
+
+#define SUNXI_GPADC_TP_CDAT				0x1c
+#define SUNXI_GPADC_TEMP_DATA				0x20
+#define SUNXI_GPADC_TP_DATA				0x24
+
+#define SUNXI_IRQ_FIFO_DATA				0
+#define SUNXI_IRQ_TEMP_DATA				1
+
+/* 10s delay before suspending the IP */
+#define SUNXI_GPADC_AUTOSUSPEND_DELAY			10000
+
+struct sunxi_gpadc_mfd_dev {
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct regmap_irq_chip_data	*regmap_irqc;
+	void __iomem			*regs;
+};
+
+#endif
-- 
2.5.0

[PATCH v3 3/4] mfd: mfd-core: reattach mfd of_node to cells without of_compatible

[Quentin Schulz]

When an MFD cell has an of_compatible (meaning it is present in the Device
Tree), other nodes can reference it using a phandle.

However when the MFD cell is not declared in the Device Tree, the only way
other nodes can reference it are by using a phandle to the MFD. Then when
this MFD cell tries to register itself in one framework, the registration
is denied by the framework because it is not matching the of_node of the
node which is referenced by the phandle in one of the other nodes.

This reattaches the of_node of the MFD to the MFD cell device structure
when the MFD cell has no of_compatible.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

We need this modification to register the thermal sensor in the thermal
framework.

Added in v3.

 drivers/mfd/mfd-core.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 3ac486a..0b19663 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -175,12 +175,16 @@ static int mfd_add_device(struct device *parent, int id,
 	if (ret < 0)
 		goto fail_res;
 
-	if (parent->of_node && cell->of_compatible) {
-		for_each_child_of_node(parent->of_node, np) {
-			if (of_device_is_compatible(np, cell->of_compatible)) {
-				pdev->dev.of_node = np;
-				break;
+	if (parent->of_node) {
+		if (cell->of_compatible) {
+			for_each_child_of_node(parent->of_node, np) {
+				if (of_device_is_compatible(np, cell->of_compatible)) {
+					pdev->dev.of_node = np;
+					break;
+				}
 			}
+		} else {
+			pdev->dev.of_node = parent->of_node;
 		}
 	}
 
-- 
2.5.0

[PATCH v3 4/4] 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 registers
the driver in the thermal framework.

This driver probes on three different platform_device_id to take into
account slight differences (registers bit and temperature computation)
between Allwinner SoCs ADCs.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

I don't like how I get the temperature for the thermal framework
(sunxi_gpadc_get_temp). I am doing the exact same process as the iio_hwmon
but in this function. This is duplicated code. I could use
iio_read_channel_processed but it needs to have the iio_channel structure
of the thermal sensor which I can only get with iio_channel_get which
matches the device name with the consumer_dev_name in the iio_map array.
And frankly, I do not see myself declaring the driver both as the producer
and the consumer of IIO channels.

Moreover, since the thermal sensor is configured to throw an interrupt
every X seconds, only the first request within these X seconds will not
time out. This means that because the thermal framework regularly poll the
thermal sensor, it is really difficult to get a value from sysfs without
getting first a tonne of times out. I don't know if we can do something
about that?

v3:
 - correct wrapping,
 - add comment about thermal sensor inner working,
 - move defines in mfd header,
 - use structure to define SoC specific registers or behaviour,
 - attach this structure to the device according to of_device_id of the
   platform device,
 - use new mutex instead of iio_dev mutex,
 - use atomic flags to avoid race between request_irq and disable_irq in
   probe,
 - switch from processed value to raw, offset and scale values for
   temperature ADC channel,
 - remove faulty sentinel in iio_chan_spec array,
 - add pm_runtime support,
 - register thermal sensor in thermal framework (forgotten since the
   beginning whereas it is present in current sun4i-ts driver),
 - remove useless ret variables to store return value of regmap_reads,
 - move comments on thermal sensor acquisition period in code instead of
   header,
 - adding goto label to unregister iio_map_array when failing to register
   iio_dev,

v2:
 - add SUNXI_GPADC_ prefixes for defines,
 - correct typo in Kconfig,
 - reorder alphabetically includes, makefile,
 - add license header,
 - fix architecture variations not being handled in interrupt handlers or
   read raw functions,
 - fix unability to return negative values from thermal sensor,
 - add gotos to reduce code repetition,
 - fix irq variable being unsigned int instead of int,
 - remove useless dev_err and dev_info,
 - deactivate all interrupts if probe fails,
 - fix iio_device_register on NULL variable,
 - deactivate ADC in the IP when probe fails or when removing driver,

 drivers/iio/adc/Kconfig           |  12 +
 drivers/iio/adc/Makefile          |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c | 513 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 526 insertions(+)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 25378c5..429ef16 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -384,6 +384,18 @@ config ROCKCHIP_SARADC
 	  To compile this driver as a module, choose M here: the
 	  module will be called rockchip_saradc.
 
+config SUN4I_GPADC
+	tristate "Support for the Allwinner SoCs GPADC"
+	depends on IIO
+	depends on MFD_SUN4I_GPADC
+	help
+	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
+	  GPADC. 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 TI_ADC081C
 	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
 	depends on I2C
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 38638d4..14d1739 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -37,6 +37,7 @@ 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
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
+obj-$(CONFIG_SUN4I_GPADC) += sunxi-gpadc-iio.o
 obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.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..5647688
--- /dev/null
+++ b/drivers/iio/adc/sunxi-gpadc-iio.c
@@ -0,0 +1,513 @@
+/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * This program is free software; you can redistribute it and/or modify it under
+ * the terms of the GNU General Public License version 2 as published by the
+ * Free Software Foundation.
+ *
+ * The Allwinner SoCs all have an ADC that can also act as a touchscreen
+ * controller and a thermal sensor.
+ * The thermal sensor works only when the ADC acts as a touchscreen controller
+ * and is configured to throw an interrupt every fixed periods of time (let say
+ * every X seconds).
+ * One would be tempted to disable the IP on the hardware side rather than
+ * disabling interrupts to save some power but that reset the internal clock of
+ * the IP, resulting in having to wait X seconds every time we want to read the
+ * value of the thermal sensor.
+ * This is also the reason of using autosuspend in pm_runtime. If there were no
+ * autosuspend, the thermal sensor would need X seconds after every
+ * pm_runtime_get_sync to get a value from the ADC. The autosuspend allows the
+ * thermal sensor to be requested again in a certain time span before it gets
+ * shutdown for not being used.
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/thermal.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/driver.h>
+#include <linux/iio/machine.h>
+#include <linux/mfd/sunxi-gpadc-mfd.h>
+
+const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
+{
+	return SUNXI_GPADC_TP_CTRL1_ADC_CHAN_SELECT(chan);
+}
+
+const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
+{
+	return SUNXI_GPADC_TP_CTRL1_SUN6I_ADC_CHAN_SELECT(chan);
+}
+
+struct sunxi_gpadc_soc_specific {
+	const int		temp_offset;
+	const int		temp_scale;
+	const unsigned int	tp_mode_en;
+	const unsigned int	tp_adc_select;
+	const unsigned int	(*adc_chan_select)(unsigned int chan);
+};
+
+static const struct sunxi_gpadc_soc_specific sun4i_gpadc_soc_specific = {
+	.temp_offset = -1932,
+	.temp_scale = 133,
+	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_TP_MODE_EN,
+	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT,
+	.adc_chan_select = &sun4i_gpadc_chan_select,
+};
+
+static const struct sunxi_gpadc_soc_specific sun5i_gpadc_soc_specific = {
+	.temp_offset = -1447,
+	.temp_scale = 100,
+	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_TP_MODE_EN,
+	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT,
+	.adc_chan_select = &sun4i_gpadc_chan_select,
+};
+
+static const struct sunxi_gpadc_soc_specific sun6i_gpadc_soc_specific = {
+	.temp_offset = -1623,
+	.temp_scale = 167,
+	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_SUN6I_TP_MODE_EN,
+	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_SUN6I_TP_ADC_SELECT,
+	.adc_chan_select = &sun6i_gpadc_chan_select,
+};
+
+struct sunxi_gpadc_dev {
+	struct iio_dev				*indio_dev;
+	void __iomem				*regs;
+	struct completion			completion;
+	int					temp_data;
+	u32					adc_data;
+	struct regmap				*regmap;
+	unsigned int				fifo_data_irq;
+	atomic_t				ignore_fifo_data_irq;
+	unsigned int				temp_data_irq;
+	atomic_t				ignore_temp_data_irq;
+	const struct sunxi_gpadc_soc_specific	*soc_specific;
+	struct mutex				mutex;
+};
+
+#define SUNXI_GPADC_ADC_CHANNEL(_channel, _name) {		\
+	.type = IIO_VOLTAGE,					\
+	.indexed = 1,						\
+	.channel = _channel,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.datasheet_name = _name,				\
+}
+
+static struct iio_map sunxi_gpadc_hwmon_maps[] = {
+	{
+		.adc_channel_label = "temp_adc",
+		.consumer_dev_name = "iio_hwmon.0",
+	},
+	{ /* sentinel */ },
+};
+
+static const struct iio_chan_spec sunxi_gpadc_channels[] = {
+	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
+	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
+	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
+	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.datasheet_name = "temp_adc",
+	},
+};
+
+static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
+				int *val)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+	int ret = 0;
+
+	pm_runtime_get_sync(indio_dev->dev.parent);
+	mutex_lock(&info->mutex);
+
+	reinit_completion(&info->completion);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+		     info->soc_specific->tp_mode_en |
+		     info->soc_specific->tp_adc_select |
+		     info->soc_specific->adc_chan_select(channel));
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
+		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
+		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
+	enable_irq(info->fifo_data_irq);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 msecs_to_jiffies(100))) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	*val = info->adc_data;
+
+out:
+	disable_irq(info->fifo_data_irq);
+	mutex_unlock(&info->mutex);
+	pm_runtime_mark_last_busy(indio_dev->dev.parent);
+	pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+	return ret;
+}
+
+static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+	int ret = 0;
+
+	pm_runtime_get_sync(indio_dev->dev.parent);
+	mutex_lock(&info->mutex);
+
+	reinit_completion(&info->completion);
+
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
+		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
+		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
+	/*
+	 * The temperature sensor returns valid data only when the ADC operates
+	 * in touchscreen mode.
+	 */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+		     info->soc_specific->tp_mode_en);
+	enable_irq(info->temp_data_irq);
+
+	if (!wait_for_completion_timeout(&info->completion,
+					 msecs_to_jiffies(100))) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	*val = info->temp_data;
+
+out:
+	disable_irq(info->temp_data_irq);
+	mutex_unlock(&info->mutex);
+	pm_runtime_mark_last_busy(indio_dev->dev.parent);
+	pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+	return ret;
+}
+
+static int sunxi_gpadc_temp_offset(struct iio_dev *indio_dev, int *val)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+
+	*val = info->soc_specific->temp_offset;
+
+	return 0;
+}
+
+static int sunxi_gpadc_temp_scale(struct iio_dev *indio_dev, int *val)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
+
+	*val = info->soc_specific->temp_scale;
+
+	return 0;
+}
+
+static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan, int *val,
+				int *val2, long mask)
+{
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OFFSET:
+		ret = sunxi_gpadc_temp_offset(indio_dev, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type == IIO_VOLTAGE) {
+			ret = sunxi_gpadc_adc_read(indio_dev, chan->channel,
+						   val);
+			if (ret)
+				return ret;
+		} else {
+			ret = sunxi_gpadc_temp_read(indio_dev, val);
+			if (ret)
+				return ret;
+		}
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = sunxi_gpadc_temp_scale(indio_dev, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	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;
+
+	if (atomic_read(&info->ignore_temp_data_irq))
+		return IRQ_HANDLED;
+
+	if (!regmap_read(info->regmap, SUNXI_GPADC_TEMP_DATA, &info->temp_data))
+		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;
+
+	if (atomic_read(&info->ignore_fifo_data_irq))
+		return IRQ_HANDLED;
+
+	if (!regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data))
+		complete(&info->completion);
+
+	return IRQ_HANDLED;
+}
+
+static int sunxi_gpadc_runtime_suspend(struct device *dev)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
+
+	mutex_lock(&info->mutex);
+
+	/* Disable the ADC on IP */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
+	/* Disable temperature sensor on IP */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
+
+	mutex_unlock(&info->mutex);
+
+	return 0;
+}
+
+static int sunxi_gpadc_runtime_resume(struct device *dev)
+{
+	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
+
+	mutex_lock(&info->mutex);
+
+	/* clkin = 6MHz */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0,
+		     SUNXI_GPADC_TP_CTRL0_ADC_CLK_DIVIDER(2) |
+		     SUNXI_GPADC_TP_CTRL0_FS_DIV(7) |
+		     SUNXI_GPADC_TP_CTRL0_T_ACQ(63));
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
+		     info->soc_specific->tp_mode_en);
+	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3,
+		     SUNXI_GPADC_TP_CTRL3_FILTER_EN |
+		     SUNXI_GPADC_TP_CTRL3_FILTER_TYPE(1));
+	/* period = SUNXI_GPADC_TP_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~1.3s */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
+		     SUNXI_GPADC_TP_TPR_TEMP_ENABLE |
+		     SUNXI_GPADC_TP_TPR_TEMP_PERIOD(1953));
+
+	mutex_unlock(&info->mutex);
+
+	return 0;
+}
+
+static int sunxi_gpadc_get_temp(void *data, int *temp)
+{
+	struct sunxi_gpadc_dev *info = (struct sunxi_gpadc_dev *)data;
+	int val, scale, offset;
+
+	/* If reading temperature times out, take stored previous value. */
+	if (sunxi_gpadc_temp_read(info->indio_dev, &val))
+		val = info->temp_data;
+	sunxi_gpadc_temp_scale(info->indio_dev, &scale);
+	sunxi_gpadc_temp_offset(info->indio_dev, &offset);
+
+	*temp = (val + offset) * scale;
+
+	return 0;
+}
+
+static const struct thermal_zone_of_device_ops sunxi_ts_tz_ops = {
+	.get_temp = &sunxi_gpadc_get_temp,
+};
+
+static const struct dev_pm_ops sunxi_gpadc_pm_ops = {
+	.runtime_suspend = &sunxi_gpadc_runtime_suspend,
+	.runtime_resume = &sunxi_gpadc_runtime_resume,
+};
+
+static int sunxi_gpadc_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_dev *info;
+	struct iio_dev *indio_dev;
+	int ret, irq;
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
+	struct thermal_zone_device *tzd;
+
+	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+
+	mutex_init(&info->mutex);
+	info->regmap = sunxi_gpadc_mfd_dev->regmap;
+	info->indio_dev = indio_dev;
+	atomic_set(&info->ignore_fifo_data_irq, 1);
+	atomic_set(&info->ignore_temp_data_irq, 1);
+	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->soc_specific = (struct sunxi_gpadc_soc_specific *)platform_get_device_id(pdev)->driver_data;
+
+	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
+						   &sunxi_ts_tz_ops);
+	if (IS_ERR(tzd)) {
+		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
+			PTR_ERR(tzd));
+		return PTR_ERR(tzd);
+	}
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev,
+					 SUNXI_GPADC_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"no TEMP_DATA_PENDING interrupt registered\n");
+		ret = irq;
+		goto err;
+	}
+
+	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);
+		goto err;
+	}
+
+	disable_irq(irq);
+	info->temp_data_irq = irq;
+	atomic_set(&info->ignore_temp_data_irq, 0);
+
+	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"no FIFO_DATA_PENDING interrupt registered\n");
+		ret = irq;
+		goto err;
+	}
+
+	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);
+		goto err;
+	}
+
+	disable_irq(irq);
+	info->fifo_data_irq = irq;
+	atomic_set(&info->ignore_fifo_data_irq, 0);
+
+	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");
+		goto err;
+	}
+
+	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");
+		goto err_map;
+	}
+
+	return 0;
+
+err_map:
+	iio_map_array_unregister(indio_dev);
+
+err:
+	pm_runtime_put(&pdev->dev);
+	/* Disable all hardware interrupts */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
+
+	return ret;
+}
+
+static int sunxi_gpadc_remove(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_dev *info;
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+	info = iio_priv(indio_dev);
+	iio_device_unregister(indio_dev);
+	iio_map_array_unregister(indio_dev);
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	/* Disable all hardware interrupts */
+	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
+
+	return 0;
+}
+
+static const struct platform_device_id sunxi_gpadc_id[] = {
+	{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)&sun4i_gpadc_soc_specific },
+	{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)&sun5i_gpadc_soc_specific },
+	{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)&sun6i_gpadc_soc_specific },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver sunxi_gpadc_driver = {
+	.driver = {
+		.name = "sunxi-gpadc-iio",
+		.pm = &sunxi_gpadc_pm_ops,
+	},
+	.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 v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Thomas Petazzoni]

Hello,

On Tue, 26 Jul 2016 09:43:44 +0200, Quentin Schulz wrote:

> -module_platform_driver(iio_hwmon_driver);
> +static struct platform_driver * const drivers[] = {
> +	&iio_hwmon_driver,
> +};
> +
> +static int __init iio_hwmon_late_init(void)
> +{
> +	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));

Why not platform_register_driver() ?

This would avoid the need to declare an array with just one entry.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Quentin Schulz]

Hi,

On 26/07/2016 09:48, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 26 Jul 2016 09:43:44 +0200, Quentin Schulz wrote:
> 
>> -module_platform_driver(iio_hwmon_driver);
>> +static struct platform_driver * const drivers[] = {
>> +	&iio_hwmon_driver,
>> +};
>> +
>> +static int __init iio_hwmon_late_init(void)
>> +{
>> +	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
> 
> Why not platform_register_driver() ?
> 
> This would avoid the need to declare an array with just one entry.

Actually, it is named platform_driver_register() as you just showed me
and that's the reason I didn't find it under platform_register_driver().

Thanks!

Quentin

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 09:43:44, 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.

Would it work if iio_channel_get_all returning ENODEV is used for returning 
EPROBE_DEFER in iio_channel_get_all? Using late initcalls for driver/device 
dependencies seems not right for me at this place.

Best regards,
Alexander

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Quentin Schulz]

On 26/07/2016 10:21, Alexander Stein wrote:
> On Tuesday 26 July 2016 09:43:44, 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.
> 
> Would it work if iio_channel_get_all returning ENODEV is used for returning 
> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for driver/device 
> dependencies seems not right for me at this place.

Then what if the iio_channel_get_all is called outside of the probe of a
driver? We'll have to change the error code, things we are apparently
trying to avoid (see v2 patches' discussions).

Thanks,
Quentin

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> On 26/07/2016 10:21, Alexander Stein wrote:
> > On Tuesday 26 July 2016 09:43:44, 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.
> > 
> > Would it work if iio_channel_get_all returning ENODEV is used for
> > returning
> > EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> > driver/device
> > dependencies seems not right for me at this place.
> 
> Then what if the iio_channel_get_all is called outside of the probe of a
> driver? We'll have to change the error code, things we are apparently
> trying to avoid (see v2 patches' discussions).

Maybe I didn't express my idea enough. I don't want to change the behavior of  
iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
in iio_hwmon_probe. I have something link the patch below in mind.

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

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Quentin Schulz]

On 26/07/2016 11:05, Alexander Stein wrote:
> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>> On 26/07/2016 10:21, Alexander Stein wrote:
>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>
>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>> returning
>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>> driver/device
>>> dependencies seems not right for me at this place.
>>
>> Then what if the iio_channel_get_all is called outside of the probe of a
>> driver? We'll have to change the error code, things we are apparently
>> trying to avoid (see v2 patches' discussions).
> 
> Maybe I didn't express my idea enough. I don't want to change the behavior of  
> iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
> in iio_hwmon_probe. I have something link the patch below in mind.
> 
> Best regards,
> Alexander
> ---
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index b550ba5..e32d150 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>                 name = dev->of_node->name;
>  
>         channels = iio_channel_get_all(dev);
> -       if (IS_ERR(channels))
> -               return PTR_ERR(channels);
> +       if (IS_ERR(channels)) {
> +               if (PTR_ERR(channels) == -ENODEV)
> +                       return -EPROBE_DEFER;
> +               else
> +                       return PTR_ERR(channels);
> +       }
>  
>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>         if (st == NULL) {

Indeed, I misunderstood what you told me.

Actually, the patch you proposed is part of my v1
(https://lkml.org/lkml/2016/6/28/203) and v2
(https://lkml.org/lkml/2016/7/15/215).
Jonathan and Guenter didn't really like the idea of changing the -ENODEV
in -EPROBE_DEFER.

What I thought you were proposing was to change the -ENODEV return code
inside iio_channel_get_all. This cannot be an option since the function
might be called outside of a probe (it is not yet, but might be in the
future?).

Of what I understood, two possibilities are then possible (proposed
either by Guenter or Jonathan): either rework the iio framework to
register iio map array earlier or to use late_initcall instead of init
for the driver consuming the iio channels.

Thanks,
Quentin

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Alexander Stein]

On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
> > On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> >> On 26/07/2016 10:21, Alexander Stein wrote:
> >>> On Tuesday 26 July 2016 09:43:44, 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.
> >>> 
> >>> Would it work if iio_channel_get_all returning ENODEV is used for
> >>> returning
> >>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> >>> driver/device
> >>> dependencies seems not right for me at this place.
> >> 
> >> Then what if the iio_channel_get_all is called outside of the probe of a
> >> driver? We'll have to change the error code, things we are apparently
> >> trying to avoid (see v2 patches' discussions).
> > 
> > Maybe I didn't express my idea enough. I don't want to change the behavior
> > of iio_channel_get_all at all. Just the result evaluation of
> > iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> > below in mind.
> > 
> > Best regards,
> > Alexander
> > ---
> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > index b550ba5..e32d150 100644
> > --- a/drivers/hwmon/iio_hwmon.c
> > +++ b/drivers/hwmon/iio_hwmon.c
> > @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> > *pdev)
> > 
> >                 name = dev->of_node->name;
> >         
> >         channels = iio_channel_get_all(dev);
> > 
> > -       if (IS_ERR(channels))
> > -               return PTR_ERR(channels);
> > +       if (IS_ERR(channels)) {
> > +               if (PTR_ERR(channels) == -ENODEV)
> > +                       return -EPROBE_DEFER;
> > +               else
> > +                       return PTR_ERR(channels);
> > +       }
> > 
> >         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> >         if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.

Thanks for the links.

> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).

AFAICS this is a helper function not knowing about device probing itself. And 
it should stay at that.

> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.

Interestingly using this problem would not arise due to module dependencies. 
But using late_initcall would mean this needs to be done on any driver using 
iio channels? I would rather keep those consumers simple.

Best regards,
Alexander

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Quentin Schulz]

On 26/07/2016 12:00, Alexander Stein wrote:
> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>> On 26/07/2016 11:05, Alexander Stein wrote:
>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>>>
>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>>> returning
>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>> driver/device
>>>>> dependencies seems not right for me at this place.
>>>>
>>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>>> driver? We'll have to change the error code, things we are apparently
>>>> trying to avoid (see v2 patches' discussions).
>>>
>>> Maybe I didn't express my idea enough. I don't want to change the behavior
>>> of iio_channel_get_all at all. Just the result evaluation of
>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
>>> below in mind.
>>>
>>> Best regards,
>>> Alexander
>>> ---
>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>> index b550ba5..e32d150 100644
>>> --- a/drivers/hwmon/iio_hwmon.c
>>> +++ b/drivers/hwmon/iio_hwmon.c
>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
>>> *pdev)
>>>
>>>                 name = dev->of_node->name;
>>>         
>>>         channels = iio_channel_get_all(dev);
>>>
>>> -       if (IS_ERR(channels))
>>> -               return PTR_ERR(channels);
>>> +       if (IS_ERR(channels)) {
>>> +               if (PTR_ERR(channels) == -ENODEV)
>>> +                       return -EPROBE_DEFER;
>>> +               else
>>> +                       return PTR_ERR(channels);
>>> +       }
>>>
>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>         if (st == NULL) {
>>
>> Indeed, I misunderstood what you told me.
>>
>> Actually, the patch you proposed is part of my v1
>> (https://lkml.org/lkml/2016/6/28/203) and v2
>> (https://lkml.org/lkml/2016/7/15/215).
>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
>> in -EPROBE_DEFER.
> 
> Thanks for the links.
> 
>> What I thought you were proposing was to change the -ENODEV return code
>> inside iio_channel_get_all. This cannot be an option since the function
>> might be called outside of a probe (it is not yet, but might be in the
>> future?).
> 
> AFAICS this is a helper function not knowing about device probing itself. And 
> it should stay at that.
> 
>> Of what I understood, two possibilities are then possible (proposed
>> either by Guenter or Jonathan): either rework the iio framework to
>> register iio map array earlier or to use late_initcall instead of init
>> for the driver consuming the iio channels.
> 
> Interestingly using this problem would not arise due to module dependencies. 
> But using late_initcall would mean this needs to be done on any driver using 
> iio channels? I would rather keep those consumers simple.

This would mean this needs to be done in any driver *using iio_map
array* to get iio channels. The other way of getting iio channels is
using properties in the Device Tree so no need for late_initcall in that
case.

Quentin

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Guenter Roeck]

On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> > On 26/07/2016 11:05, Alexander Stein wrote:
> > > On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> > >> On 26/07/2016 10:21, Alexander Stein wrote:
> > >>> On Tuesday 26 July 2016 09:43:44, 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.
> > >>> 
> > >>> Would it work if iio_channel_get_all returning ENODEV is used for
> > >>> returning
> > >>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> > >>> driver/device
> > >>> dependencies seems not right for me at this place.
> > >> 
> > >> Then what if the iio_channel_get_all is called outside of the probe of a
> > >> driver? We'll have to change the error code, things we are apparently
> > >> trying to avoid (see v2 patches' discussions).
> > > 
> > > Maybe I didn't express my idea enough. I don't want to change the behavior
> > > of iio_channel_get_all at all. Just the result evaluation of
> > > iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> > > below in mind.
> > > 
> > > Best regards,
> > > Alexander
> > > ---
> > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > > index b550ba5..e32d150 100644
> > > --- a/drivers/hwmon/iio_hwmon.c
> > > +++ b/drivers/hwmon/iio_hwmon.c
> > > @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> > > *pdev)
> > > 
> > >                 name = dev->of_node->name;
> > >         
> > >         channels = iio_channel_get_all(dev);
> > > 
> > > -       if (IS_ERR(channels))
> > > -               return PTR_ERR(channels);
> > > +       if (IS_ERR(channels)) {
> > > +               if (PTR_ERR(channels) == -ENODEV)
> > > +                       return -EPROBE_DEFER;
> > > +               else
> > > +                       return PTR_ERR(channels);
> > > +       }
> > > 
> > >         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > >         if (st == NULL) {
> > 
> > Indeed, I misunderstood what you told me.
> > 
> > Actually, the patch you proposed is part of my v1
> > (https://lkml.org/lkml/2016/6/28/203) and v2
> > (https://lkml.org/lkml/2016/7/15/215).
> > Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> > in -EPROBE_DEFER.
> 
> Thanks for the links.
> 
> > What I thought you were proposing was to change the -ENODEV return code
> > inside iio_channel_get_all. This cannot be an option since the function
> > might be called outside of a probe (it is not yet, but might be in the
> > future?).
> 
> AFAICS this is a helper function not knowing about device probing itself. And 
> it should stay at that.
> 
> > Of what I understood, two possibilities are then possible (proposed
> > either by Guenter or Jonathan): either rework the iio framework to
> > register iio map array earlier or to use late_initcall instead of init
> > for the driver consuming the iio channels.
> 
> Interestingly using this problem would not arise due to module dependencies. 
> But using late_initcall would mean this needs to be done on any driver using 
> iio channels? I would rather keep those consumers simple.
> 
Me too, but that would imply a solution in iio. The change you propose above
isn't exactly simple either, and would also be needed in each consumer driver.

Just for the record, I dislike the late_initcall solution as well, but I prefer
it over blindly converting ENODEV to EPROBE_DEFER.

Guenter

Re: [PATCH v3 2/4] mfd: add support for Allwinner SoCs ADC

[Maxime Ripard]

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

Hi Quentin,

On Tue, Jul 26, 2016 at 09:43:45AM +0200, 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>
> ---
> 
> v3:
>  - use defines in regmap_irq instead of hard coded BITs,
>  - use of_device_id data field to chose which MFD cells to add considering
>    the compatible responsible of the MFD probe,
>  - remove useless initializations,
>  - disable all interrupts before adding them to regmap_irqchip,
>  - add goto error label in probe,
>  - correct wrapping in header license,
>  - move defines from IIO driver to header,
>  - use GENMASK to limit the size of the variable passed to a macro,
>  - prefix register BIT defines with the name of the register,
>  - reorder defines,
> 
> v2:
>  - add license headers,
>  - reorder alphabetically includes,
>  - add SUNXI_GPADC_ prefixes for defines,
> 
>  drivers/mfd/Kconfig                 |  15 +++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 189 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  94 ++++++++++++++++++

Renaming the driver would be nice too.

>  4 files changed, 300 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 1bcf601..6180c2d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -29,6 +29,21 @@ config MFD_ACT8945A
>  	  linear regulators, along with a complete ActivePath battery
>  	  charger.
>  
> +config MFD_SUN4I_GPADC
> +	tristate "Allwinner sunxi platforms' GPADC MFD driver"
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	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_AS3711
>  	bool "AMS AS3711"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..9dfd033 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -205,3 +205,5 @@ 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
>  obj-$(CONFIG_MFD_MT6397)	+= mt6397-core.o
> +
> +obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sunxi-gpadc-mfd.o
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..3d70af0
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,189 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +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,
> +		       SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_IRQ_EN),
> +	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0,
> +		       SUNXI_GPADC_TP_INT_FIFOC_TEMP_IRQ_EN),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> +	.name = "sunxi_gpadc_mfd_irq_chip",
> +	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.mask_base = SUNXI_GPADC_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 const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
> +	{
> +		.compatible = "allwinner,sun4i-a10-ts",
> +		.data = &sun4i_gpadc_mfd_cells,
> +	}, {
> +		.compatible = "allwinner,sun5i-a13-ts",
> +		.data = &sun5i_gpadc_mfd_cells,
> +	}, {
> +		.compatible = "allwinner,sun6i-a31-ts",
> +		.data = &sun6i_gpadc_mfd_cells,
> +	}, { /* sentinel */ }
> +};
> +
> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *mfd_dev;
> +	struct resource *mem;
> +	const struct of_device_id *of_id;
> +	const struct mfd_cell *mfd_cells;
> +	unsigned int irq;
> +	int ret;
> +
> +	of_id = of_match_node(sunxi_gpadc_mfd_of_match, pdev->dev.of_node);
> +	if (!of_id)
> +		return -EINVAL;
> +
> +	mfd_dev = devm_kzalloc(&pdev->dev, sizeof(*mfd_dev), GFP_KERNEL);
> +	if (!mfd_dev)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(mfd_dev->regs))
> +		return PTR_ERR(mfd_dev->regs);
> +
> +	mfd_dev->dev = &pdev->dev;
> +	dev_set_drvdata(mfd_dev->dev, mfd_dev);
> +
> +	mfd_dev->regmap = devm_regmap_init_mmio(mfd_dev->dev, mfd_dev->regs,
> +						&sunxi_gpadc_mfd_regmap_config);
> +	if (IS_ERR(mfd_dev->regmap)) {
> +		ret = PTR_ERR(mfd_dev->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Disable all interrupts */
> +	regmap_write(mfd_dev->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = regmap_add_irq_chip(mfd_dev->regmap, irq, IRQF_ONESHOT, 0,
> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
> +				  &mfd_dev->regmap_irqc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mfd_cells = of_id->data;
> +	ret = mfd_add_devices(mfd_dev->dev, 0, mfd_cells, 2, NULL, 0, NULL);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +err:
> +	regmap_del_irq_chip(irq, mfd_dev->regmap_irqc);
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *mfd_dev;
> +	unsigned int irq;
> +
> +	irq = platform_get_irq(pdev, 0);

You can store the irq number instead of looking it up twice.

> +	mfd_remove_devices(&pdev->dev);
> +	mfd_dev = dev_get_drvdata(&pdev->dev);
> +	regmap_del_irq_chip(irq, mfd_dev->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +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("Allwinner sunxi platforms' GPADC MFD core driver");
> +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..c21abae
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,94 @@
> +/* Header of ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation.
> + */
> +
> +#ifndef __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define SUNXI_GPADC_TP_CTRL0				0x00
> +
> +#define SUNXI_GPADC_TP_CTRL0_ADC_FIRST_DLY(x)		((GENMASK(7, 0) & (x)) << 24)
> +#define SUNXI_GPADC_TP_CTRL0_ADC_FIRST_DLY_MODE		BIT(23)
> +#define SUNXI_GPADC_TP_CTRL0_ADC_CLK_SELECT		BIT(22)
> +#define SUNXI_GPADC_TP_CTRL0_ADC_CLK_DIVIDER(x)		((GENMASK(1, 0) & (x)) << 20)
> +#define SUNXI_GPADC_TP_CTRL0_FS_DIV(x)			((GENMASK(3, 0) & (x)) << 16)
> +#define SUNXI_GPADC_TP_CTRL0_T_ACQ(x)			(GENMASK(15, 0) & (x))
> +
> +#define SUNXI_GPADC_TP_CTRL1				0x04

Isn't GPADC and TP redundant in your prefix, you seem to be always
using TP, even for non touchscreen related defines...

> +
> +#define SUNXI_GPADC_TP_CTRL1_STYLUS_UP_DEBOUNCE(x)	((GENMASK(7, 0) & (x)) << 12)
> +#define SUNXI_GPADC_TP_CTRL1_STYLUS_UP_DEBOUNCE_EN	BIT(9)
> +#define SUNXI_GPADC_TP_CTRL1_TOUCH_PAN_CALI_EN		BIT(6)
> +#define SUNXI_GPADC_TP_CTRL1_TP_DUAL_EN			BIT(5)
> +#define SUNXI_GPADC_TP_CTRL1_TP_MODE_EN			BIT(4)
> +#define SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT		BIT(3)

... sometimes twice ...

> +#define SUNXI_GPADC_TP_CTRL1_ADC_CHAN_SELECT(x)		(GENMASK(2, 0) & (x))
> +
> +/* TP_CTRL1 bits for sun6i SOCs */
> +#define SUNXI_GPADC_TP_CTRL1_SUN6I_TOUCH_PAN_CALI_EN	BIT(7)
> +#define SUNXI_GPADC_TP_CTRL1_SUN6I_TP_DUAL_EN		BIT(6)
> +#define SUNXI_GPADC_TP_CTRL1_SUN6I_TP_MODE_EN		BIT(5)
> +#define SUNXI_GPADC_TP_CTRL1_SUN6I_TP_ADC_SELECT	BIT(4)
> +#define SUNXI_GPADC_TP_CTRL1_SUN6I_ADC_CHAN_SELECT(x)	(GENMASK(3, 0) & BIT(x))
> +
> +#define SUNXI_GPADC_TP_CTRL2				0x08
> +
> +#define SUNXI_GPADC_TP_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)
> +#define SUNXI_GPADC_TP_CTRL2_TP_MODE_SELECT(x)		((GENMASK(1, 0) & (x)) << 26)
> +#define SUNXI_GPADC_TP_CTRL2_PRE_MEA_EN			BIT(24)
> +#define SUNXI_GPADC_TP_CTRL2_PRE_MEA_THRE_CNT(x)	(GENMASK(23, 0) & (x))
> +
> +#define SUNXI_GPADC_TP_CTRL3				0x0c
> +
> +#define SUNXI_GPADC_TP_CTRL3_FILTER_EN			BIT(2)
> +#define SUNXI_GPADC_TP_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> +
> +#define SUNXI_GPADC_TP_TPR				0x18
> +
> +#define SUNXI_GPADC_TP_TPR_TEMP_ENABLE			BIT(16)
> +#define SUNXI_GPADC_TP_TPR_TEMP_PERIOD(x)		(GENMASK(15, 0) & (x))
> +
> +#define SUNXI_GPADC_TP_INT_FIFOC			0x10
> +
> +#define SUNXI_GPADC_TP_INT_FIFOC_TEMP_IRQ_EN		BIT(18)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_OVERRUN_IRQ_EN	BIT(17)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_IRQ_EN		BIT(16)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_XY_CHANGE	BIT(13)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(x)	((GENMASK(4, 0) & (x)) << 8)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_DATA_DRQ_EN		BIT(7)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH		BIT(4)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
> +#define SUNXI_GPADC_TP_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
> +
> +#define SUNXI_GPADC_TP_INT_FIFOS			0x14
> +
> +#define SUNXI_GPADC_TP_INT_FIFOS_TEMP_DATA_PENDING	BIT(18)
> +#define SUNXI_GPADC_TP_INT_FIFOS_FIFO_OVERRUN_PENDING	BIT(17)
> +#define SUNXI_GPADC_TP_INT_FIFOS_FIFO_DATA_PENDING	BIT(16)
> +#define SUNXI_GPADC_TP_INT_FIFOS_TP_IDLE_FLG		BIT(2)
> +#define SUNXI_GPADC_TP_INT_FIFOS_TP_UP_PENDING		BIT(1)
> +#define SUNXI_GPADC_TP_INT_FIFOS_TP_DOWN_PENDING	BIT(0)
> +
> +#define SUNXI_GPADC_TP_CDAT				0x1c
> +#define SUNXI_GPADC_TEMP_DATA				0x20
> +#define SUNXI_GPADC_TP_DATA				0x24
> +
> +#define SUNXI_IRQ_FIFO_DATA				0
> +#define SUNXI_IRQ_TEMP_DATA				1

... and sometimes not at all.

> +
> +/* 10s delay before suspending the IP */
> +#define SUNXI_GPADC_AUTOSUSPEND_DELAY			10000
> +
> +struct sunxi_gpadc_mfd_dev {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +	void __iomem			*regs;
> +};
> +
> +#endif
> -- 
> 2.5.0
> 

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Maxime Ripard]

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

On Tue, Jul 26, 2016 at 09:43:47AM +0200, 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 registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> I don't like how I get the temperature for the thermal framework
> (sunxi_gpadc_get_temp). I am doing the exact same process as the iio_hwmon
> but in this function. This is duplicated code. I could use
> iio_read_channel_processed but it needs to have the iio_channel structure
> of the thermal sensor which I can only get with iio_channel_get which
> matches the device name with the consumer_dev_name in the iio_map array.
> And frankly, I do not see myself declaring the driver both as the producer
> and the consumer of IIO channels.
> 
> Moreover, since the thermal sensor is configured to throw an interrupt
> every X seconds, only the first request within these X seconds will not
> time out. This means that because the thermal framework regularly poll the
> thermal sensor, it is really difficult to get a value from sysfs without
> getting first a tonne of times out. I don't know if we can do something
> about that?
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>    platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>    probe,
>  - switch from processed value to raw, offset and scale values for
>    temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>    beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>    header,
>  - adding goto label to unregister iio_map_array when failing to register
>    iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>    read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig           |  12 +
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 513 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 526 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..429ef16 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,18 @@ config ROCKCHIP_SARADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> +	tristate "Support for the Allwinner SoCs GPADC"
> +	depends on IIO
> +	depends on MFD_SUN4I_GPADC
> +	help
> +	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +	  GPADC. 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 TI_ADC081C
>  	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..14d1739 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ 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
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_GPADC) += sunxi-gpadc-iio.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.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..5647688
> --- /dev/null
> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
> @@ -0,0 +1,513 @@
> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation.
> + *
> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen
> + * controller and a thermal sensor.
> + * The thermal sensor works only when the ADC acts as a touchscreen controller
> + * and is configured to throw an interrupt every fixed periods of time (let say
> + * every X seconds).
> + * One would be tempted to disable the IP on the hardware side rather than
> + * disabling interrupts to save some power but that reset the internal clock of
> + * the IP, resulting in having to wait X seconds every time we want to read the
> + * value of the thermal sensor.
> + * This is also the reason of using autosuspend in pm_runtime. If there were no
> + * autosuspend, the thermal sensor would need X seconds after every
> + * pm_runtime_get_sync to get a value from the ADC. The autosuspend allows the
> + * thermal sensor to be requested again in a certain time span before it gets
> + * shutdown for not being used.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> +{
> +	return SUNXI_GPADC_TP_CTRL1_ADC_CHAN_SELECT(chan);
> +}
> +
> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
> +{
> +	return SUNXI_GPADC_TP_CTRL1_SUN6I_ADC_CHAN_SELECT(chan);
> +}
> +
> +struct sunxi_gpadc_soc_specific {
> +	const int		temp_offset;
> +	const int		temp_scale;
> +	const unsigned int	tp_mode_en;
> +	const unsigned int	tp_adc_select;
> +	const unsigned int	(*adc_chan_select)(unsigned int chan);
> +};
> +
> +static const struct sunxi_gpadc_soc_specific sun4i_gpadc_soc_specific = {
> +	.temp_offset = -1932,
> +	.temp_scale = 133,
> +	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_TP_MODE_EN,
> +	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT,
> +	.adc_chan_select = &sun4i_gpadc_chan_select,
> +};
> +
> +static const struct sunxi_gpadc_soc_specific sun5i_gpadc_soc_specific = {
> +	.temp_offset = -1447,
> +	.temp_scale = 100,
> +	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_TP_MODE_EN,
> +	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT,
> +	.adc_chan_select = &sun4i_gpadc_chan_select,
> +};
> +
> +static const struct sunxi_gpadc_soc_specific sun6i_gpadc_soc_specific = {
> +	.temp_offset = -1623,
> +	.temp_scale = 167,
> +	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_SUN6I_TP_MODE_EN,
> +	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_SUN6I_TP_ADC_SELECT,
> +	.adc_chan_select = &sun6i_gpadc_chan_select,
> +};
> +
> +struct sunxi_gpadc_dev {
> +	struct iio_dev				*indio_dev;
> +	void __iomem				*regs;
> +	struct completion			completion;
> +	int					temp_data;
> +	u32					adc_data;
> +	struct regmap				*regmap;
> +	unsigned int				fifo_data_irq;
> +	atomic_t				ignore_fifo_data_irq;
> +	unsigned int				temp_data_irq;
> +	atomic_t				ignore_temp_data_irq;
> +	const struct sunxi_gpadc_soc_specific	*soc_specific;
> +	struct mutex				mutex;
> +};
> +
> +#define SUNXI_GPADC_ADC_CHANNEL(_channel, _name) {		\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.datasheet_name = _name,				\
> +}
> +
> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
> +	{
> +		.adc_channel_label = "temp_adc",
> +		.consumer_dev_name = "iio_hwmon.0",
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.datasheet_name = "temp_adc",
> +	},
> +};
> +
> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
> +				int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(indio_dev->dev.parent);
> +	mutex_lock(&info->mutex);
> +
> +	reinit_completion(&info->completion);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en |
> +		     info->soc_specific->tp_adc_select |
> +		     info->soc_specific->adc_chan_select(channel));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
> +	enable_irq(info->fifo_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->adc_data;
> +
> +out:
> +	disable_irq(info->fifo_data_irq);
> +	mutex_unlock(&info->mutex);
> +	pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +	pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(indio_dev->dev.parent);
> +	mutex_lock(&info->mutex);
> +
> +	reinit_completion(&info->completion);
> +
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
> +	/*
> +	 * The temperature sensor returns valid data only when the ADC operates
> +	 * in touchscreen mode.
> +	 */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en);
> +	enable_irq(info->temp_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->temp_data;
> +
> +out:
> +	disable_irq(info->temp_data_irq);
> +	mutex_unlock(&info->mutex);
> +	pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +	pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> +	return ret;
> +}

This looks like the exact same function than above, can't that be
factored (for example by passing the interrupt number as argument, and
giving it a way to know if it's going to be used for the ADC or
temperature as an argument?)

> +static int sunxi_gpadc_temp_offset(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +
> +	*val = info->soc_specific->temp_offset;
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_temp_scale(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +
> +	*val = info->soc_specific->temp_scale;
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan, int *val,
> +				int *val2, long mask)
> +{
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		ret = sunxi_gpadc_temp_offset(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->type == IIO_VOLTAGE) {
> +			ret = sunxi_gpadc_adc_read(indio_dev, chan->channel,
> +						   val);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = sunxi_gpadc_temp_read(indio_dev, val);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = sunxi_gpadc_temp_scale(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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;
> +
> +	if (atomic_read(&info->ignore_temp_data_irq))
> +		return IRQ_HANDLED;
> +
> +	if (!regmap_read(info->regmap, SUNXI_GPADC_TEMP_DATA, &info->temp_data))
> +		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;
> +
> +	if (atomic_read(&info->ignore_fifo_data_irq))
> +		return IRQ_HANDLED;
> +
> +	if (!regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data))
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_gpadc_runtime_suspend(struct device *dev)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
> +
> +	mutex_lock(&info->mutex);
> +
> +	/* Disable the ADC on IP */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
> +	/* Disable temperature sensor on IP */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> +
> +	mutex_unlock(&info->mutex);

Having some comments somewhere about what this mutex protects would be
great (just like checkpatch tells you about).

However, I'm not sure this is even possible. Isn't the point of the
runtime_pm precisely to not be called while you're using the device?

> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_runtime_resume(struct device *dev)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
> +
> +	mutex_lock(&info->mutex);
> +
> +	/* clkin = 6MHz */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0,
> +		     SUNXI_GPADC_TP_CTRL0_ADC_CLK_DIVIDER(2) |
> +		     SUNXI_GPADC_TP_CTRL0_FS_DIV(7) |
> +		     SUNXI_GPADC_TP_CTRL0_T_ACQ(63));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3,
> +		     SUNXI_GPADC_TP_CTRL3_FILTER_EN |
> +		     SUNXI_GPADC_TP_CTRL3_FILTER_TYPE(1));
> +	/* period = SUNXI_GPADC_TP_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~1.3s */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
> +		     SUNXI_GPADC_TP_TPR_TEMP_ENABLE |
> +		     SUNXI_GPADC_TP_TPR_TEMP_PERIOD(1953));
> +
> +	mutex_unlock(&info->mutex);
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_get_temp(void *data, int *temp)
> +{
> +	struct sunxi_gpadc_dev *info = (struct sunxi_gpadc_dev *)data;
> +	int val, scale, offset;
> +
> +	/* If reading temperature times out, take stored previous value. */
> +	if (sunxi_gpadc_temp_read(info->indio_dev, &val))
> +		val = info->temp_data;
> +	sunxi_gpadc_temp_scale(info->indio_dev, &scale);
> +	sunxi_gpadc_temp_offset(info->indio_dev, &offset);
> +
> +	*temp = (val + offset) * scale;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops sunxi_ts_tz_ops = {
> +	.get_temp = &sunxi_gpadc_get_temp,
> +};
> +
> +static const struct dev_pm_ops sunxi_gpadc_pm_ops = {
> +	.runtime_suspend = &sunxi_gpadc_runtime_suspend,
> +	.runtime_resume = &sunxi_gpadc_runtime_resume,
> +};
> +
> +static int sunxi_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev;
> +	int ret, irq;
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	struct thermal_zone_device *tzd;
> +
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	mutex_init(&info->mutex);
> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
> +	info->indio_dev = indio_dev;
> +	atomic_set(&info->ignore_fifo_data_irq, 1);
> +	atomic_set(&info->ignore_temp_data_irq, 1);
> +	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->soc_specific = (struct sunxi_gpadc_soc_specific *)platform_get_device_id(pdev)->driver_data;

I'm not sure that cast is necessary (and you can probably shorten that
structure name).

> +
> +	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
> +						   &sunxi_ts_tz_ops);
> +	if (IS_ERR(tzd)) {
> +		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
> +			PTR_ERR(tzd));
> +		return PTR_ERR(tzd);
> +	}
> +
> +	pm_runtime_set_autosuspend_delay(&pdev->dev,
> +					 SUNXI_GPADC_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no TEMP_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	disable_irq(irq);
> +	info->temp_data_irq = irq;
> +	atomic_set(&info->ignore_temp_data_irq, 0);
> +
> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no FIFO_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	disable_irq(irq);
> +	info->fifo_data_irq = irq;
> +	atomic_set(&info->ignore_fifo_data_irq, 0);

These two blocks to handle the IRQs look very similar, you porbably
want to factor them.

> +	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");
> +		goto err;
> +	}
> +
> +	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");
> +		goto err_map;
> +	}

> +
> +	return 0;
> +
> +err_map:
> +	iio_map_array_unregister(indio_dev);
> +
> +err:
> +	pm_runtime_put(&pdev->dev);

You're never disabling it?


> +	/* Disable all hardware interrupts */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);

This looks like the wrong place to do that. You'll disable the
interrupts of all the devices of the MFD, which is probbaly not what
you want to do (and if you do, you want to do it in the MFD driver).

> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	info = iio_priv(indio_dev);
> +	iio_device_unregister(indio_dev);
> +	iio_map_array_unregister(indio_dev);
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	/* Disable all hardware interrupts */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id sunxi_gpadc_id[] = {
> +	{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)&sun4i_gpadc_soc_specific },
> +	{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)&sun5i_gpadc_soc_specific },
> +	{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)&sun6i_gpadc_soc_specific },

kernel_ulong_t ? that's weird.


> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver sunxi_gpadc_driver = {
> +	.driver = {
> +		.name = "sunxi-gpadc-iio",
> +		.pm = &sunxi_gpadc_pm_ops,
> +	},
> +	.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");

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

[-- Attachment #1.1: Type: text/plain, Size: 7791 bytes --]

Hi Maxime,

On 29/07/2016 09:12, Maxime Ripard wrote:
> On Tue, Jul 26, 2016 at 09:43:47AM +0200, Quentin Schulz wrote:
[...]
>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
>> +				int *val)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_sync(indio_dev->dev.parent);
>> +	mutex_lock(&info->mutex);
>> +
>> +	reinit_completion(&info->completion);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +		     info->soc_specific->tp_mode_en |
>> +		     info->soc_specific->tp_adc_select |
>> +		     info->soc_specific->adc_chan_select(channel));
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
>> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
>> +	enable_irq(info->fifo_data_irq);
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		ret = -ETIMEDOUT;
>> +		goto out;
>> +	}
>> +
>> +	*val = info->adc_data;
>> +
>> +out:
>> +	disable_irq(info->fifo_data_irq);
>> +	mutex_unlock(&info->mutex);
>> +	pm_runtime_mark_last_busy(indio_dev->dev.parent);
>> +	pm_runtime_put_autosuspend(indio_dev->dev.parent);
>> +
>> +	return ret;
>> +}
>> +
>> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_sync(indio_dev->dev.parent);
>> +	mutex_lock(&info->mutex);
>> +
>> +	reinit_completion(&info->completion);
>> +
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
>> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
>> +	/*
>> +	 * The temperature sensor returns valid data only when the ADC operates
>> +	 * in touchscreen mode.
>> +	 */
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +		     info->soc_specific->tp_mode_en);
>> +	enable_irq(info->temp_data_irq);
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		ret = -ETIMEDOUT;
>> +		goto out;
>> +	}
>> +
>> +	*val = info->temp_data;
>> +
>> +out:
>> +	disable_irq(info->temp_data_irq);
>> +	mutex_unlock(&info->mutex);
>> +	pm_runtime_mark_last_busy(indio_dev->dev.parent);
>> +	pm_runtime_put_autosuspend(indio_dev->dev.parent);
>> +
>> +	return ret;
>> +}
> 
> This looks like the exact same function than above, can't that be
> factored (for example by passing the interrupt number as argument, and
> giving it a way to know if it's going to be used for the ADC or
> temperature as an argument?)

Yes it can. I could use the interrupt number to know in which mode to
operate since each interrupt is only activated in one mode (temperature
or ADC).

[...]
>> +static int sunxi_gpadc_runtime_suspend(struct device *dev)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
>> +
>> +	mutex_lock(&info->mutex);
>> +
>> +	/* Disable the ADC on IP */
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
>> +	/* Disable temperature sensor on IP */
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
>> +
>> +	mutex_unlock(&info->mutex);
> 
> Having some comments somewhere about what this mutex protects would be
> great (just like checkpatch tells you about).

ACK.

> However, I'm not sure this is even possible. Isn't the point of the
> runtime_pm precisely to not be called while you're using the device?

I agree on the principle but I am using runtime_pm functions (I am
mainly talking about the pm_runtime_put function) when probing or
removing the driver. Let's say we remove the mutex locks in runtime_pm
functions, what will happen if we are reading raw values from the ADC
when removing the ADC driver for example?

>> +
>> +	return 0;
>> +}
>> +
>> +static int sunxi_gpadc_runtime_resume(struct device *dev)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
>> +
>> +	mutex_lock(&info->mutex);
>> +
>> +	/* clkin = 6MHz */
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0,
>> +		     SUNXI_GPADC_TP_CTRL0_ADC_CLK_DIVIDER(2) |
>> +		     SUNXI_GPADC_TP_CTRL0_FS_DIV(7) |
>> +		     SUNXI_GPADC_TP_CTRL0_T_ACQ(63));
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +		     info->soc_specific->tp_mode_en);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3,
>> +		     SUNXI_GPADC_TP_CTRL3_FILTER_EN |
>> +		     SUNXI_GPADC_TP_CTRL3_FILTER_TYPE(1));
>> +	/* period = SUNXI_GPADC_TP_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~1.3s */
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
>> +		     SUNXI_GPADC_TP_TPR_TEMP_ENABLE |
>> +		     SUNXI_GPADC_TP_TPR_TEMP_PERIOD(1953));
>> +
>> +	mutex_unlock(&info->mutex);
>> +
>> +	return 0;
>> +}
[...]
>> +	info->soc_specific = (struct sunxi_gpadc_soc_specific *)platform_get_device_id(pdev)->driver_data;
> 
> I'm not sure that cast is necessary (and you can probably shorten that
> structure name).

GCC warns about implicit pointer to integer cast in that case. ACK for
structure name.

[...]
>> +
>> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no TEMP_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	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);
>> +		goto err;
>> +	}
>> +
>> +	disable_irq(irq);
>> +	info->temp_data_irq = irq;
>> +	atomic_set(&info->ignore_temp_data_irq, 0);
>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	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);
>> +		goto err;
>> +	}
>> +
>> +	disable_irq(irq);
>> +	info->fifo_data_irq = irq;
>> +	atomic_set(&info->ignore_fifo_data_irq, 0);
> 
> These two blocks to handle the IRQs look very similar, you porbably
> want to factor them.

ACK.

[...]
>> +
>> +err:
>> +	pm_runtime_put(&pdev->dev);
> 
> You're never disabling it?

ACK.

>> +	/* Disable all hardware interrupts */
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> 
> This looks like the wrong place to do that. You'll disable the
> interrupts of all the devices of the MFD, which is probbaly not what
> you want to do (and if you do, you want to do it in the MFD driver).

Yes but all subdrivers of the MFD are using IIO channels from the ADC
driver so anyway they would not work at all.

[...]
>> +static const struct platform_device_id sunxi_gpadc_id[] = {
>> +	{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)&sun4i_gpadc_soc_specific },
>> +	{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)&sun5i_gpadc_soc_specific },
>> +	{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)&sun6i_gpadc_soc_specific },
> 
> kernel_ulong_t ? that's weird.

That's the type of the data field of platform_device_id
(http://lxr.free-electrons.com/source/include/linux/mod_devicetable.h#L498).
Otherwise, GCC warns with implicit pointer to integer cast.

Quentin


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

Re: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Russell King - ARM Linux]

On Tue, Jul 26, 2016 at 09:43:47AM +0200, Quentin Schulz wrote:
> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
> +				int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(indio_dev->dev.parent);
> +	mutex_lock(&info->mutex);
> +
> +	reinit_completion(&info->completion);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en |
> +		     info->soc_specific->tp_adc_select |
> +		     info->soc_specific->adc_chan_select(channel));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
> +	enable_irq(info->fifo_data_irq);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->adc_data;
> +
> +out:
> +	disable_irq(info->fifo_data_irq);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I spotted this while skipping over the patch - and also noticed the
below.

...
> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no TEMP_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	disable_irq(irq);
^^^^^^^^^^^^^^^^^^^^^^^^^^

> +	info->temp_data_irq = irq;
> +	atomic_set(&info->ignore_temp_data_irq, 0);
> +
> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no FIFO_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	disable_irq(irq);
> +	info->fifo_data_irq = irq;

Firstly, claiming and then immediately disabling an interrupt handler
looks very strange.  If you're disabling the interrupt because you're
concerned that you may receive an unexpected interrupt, this is no
good - consider what happens if the interrupt happens between you
claiming and disabling it.

Secondly, interrupts asserted while disabled are recorded and replayed
when you enable the interrupt, no matter when they happened (eg, they
could occur immediately after you disabled the interrupt.)

I think you need to comment each of the sites in the driver, explaining
why it's necessary to disable and enable the interrupt at the IRQ
controller like this, or get rid of these enable/disable_irq() calls.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Quentin Schulz]

On 04/08/2016 11:56, Russell King - ARM Linux wrote:
> On Tue, Jul 26, 2016 at 09:43:47AM +0200, Quentin Schulz wrote:
>> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
>> +				int *val)
>> +{
>> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_sync(indio_dev->dev.parent);
>> +	mutex_lock(&info->mutex);
>> +
>> +	reinit_completion(&info->completion);
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
>> +		     info->soc_specific->tp_mode_en |
>> +		     info->soc_specific->tp_adc_select |
>> +		     info->soc_specific->adc_chan_select(channel));
>> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
>> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
>> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
>> +	enable_irq(info->fifo_data_irq);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>> +
>> +	if (!wait_for_completion_timeout(&info->completion,
>> +					 msecs_to_jiffies(100))) {
>> +		ret = -ETIMEDOUT;
>> +		goto out;
>> +	}
>> +
>> +	*val = info->adc_data;
>> +
>> +out:
>> +	disable_irq(info->fifo_data_irq);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I spotted this while skipping over the patch - and also noticed the
> below.
> 
> ...
>> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no TEMP_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	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);
>> +		goto err;
>> +	}
>> +
>> +	disable_irq(irq);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>> +	info->temp_data_irq = irq;
>> +	atomic_set(&info->ignore_temp_data_irq, 0);
>> +
>> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
>> +	if (irq < 0) {
>> +		dev_err(&pdev->dev,
>> +			"no FIFO_DATA_PENDING interrupt registered\n");
>> +		ret = irq;
>> +		goto err;
>> +	}
>> +
>> +	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);
>> +		goto err;
>> +	}
>> +
>> +	disable_irq(irq);
>> +	info->fifo_data_irq = irq;
> 
> Firstly, claiming and then immediately disabling an interrupt handler
> looks very strange.  If you're disabling the interrupt because you're
> concerned that you may receive an unexpected interrupt, this is no
> good - consider what happens if the interrupt happens between you
> claiming and disabling it.

Indeed. This has been detected in v2
(https://lkml.org/lkml/2016/7/19/246) but since I only set values in
structures by reading registers defined beforehand, it is not a race.
However, like anything in the kernel, the driver might evolve and use
undefined variables in the interrupt handler which will introduce a
race. This potential race will be handled in v4 with atomic flags around
interrupt initializations (before requesting and after disabling). If an
interrupt occurs between the two instructions, reading the flag will
state if we need to ignore the interrupt.

> Secondly, interrupts asserted while disabled are recorded and replayed
> when you enable the interrupt, no matter when they happened (eg, they
> could occur immediately after you disabled the interrupt.)
> 
> I think you need to comment each of the sites in the driver, explaining
> why it's necessary to disable and enable the interrupt at the IRQ
> controller like this, or get rid of these enable/disable_irq() calls.

Comments for this is planned in v4.

Thanks,

Quentin

Re: [PATCH v3 3/4] mfd: mfd-core: reattach mfd of_node to cells without of_compatible

[Lee Jones]

On Tue, 26 Jul 2016, Quentin Schulz wrote:

> When an MFD cell has an of_compatible (meaning it is present in the Device
> Tree), other nodes can reference it using a phandle.
> 
> However when the MFD cell is not declared in the Device Tree, the only way
> other nodes can reference it are by using a phandle to the MFD. Then when
> this MFD cell tries to register itself in one framework, the registration
> is denied by the framework because it is not matching the of_node of the
> node which is referenced by the phandle in one of the other nodes.
> 
> This reattaches the of_node of the MFD to the MFD cell device structure
> when the MFD cell has no of_compatible.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> We need this modification to register the thermal sensor in the thermal
> framework.
> 
> Added in v3.
> 
>  drivers/mfd/mfd-core.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)

Can you show me the DT code where this is used?

Is it used by a patch in this series?

> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index 3ac486a..0b19663 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -175,12 +175,16 @@ static int mfd_add_device(struct device *parent, int id,
>  	if (ret < 0)
>  		goto fail_res;
>  
> -	if (parent->of_node && cell->of_compatible) {
> -		for_each_child_of_node(parent->of_node, np) {
> -			if (of_device_is_compatible(np, cell->of_compatible)) {
> -				pdev->dev.of_node = np;
> -				break;
> +	if (parent->of_node) {
> +		if (cell->of_compatible) {
> +			for_each_child_of_node(parent->of_node, np) {
> +				if (of_device_is_compatible(np, cell->of_compatible)) {
> +					pdev->dev.of_node = np;
> +					break;
> +				}
>  			}
> +		} else {
> +			pdev->dev.of_node = parent->of_node;
>  		}
>  	}
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 26/07/16 10:33, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>>
>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>> returning
>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>> driver/device
>>>> dependencies seems not right for me at this place.
>>>
>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>> driver? We'll have to change the error code, things we are apparently
>>> trying to avoid (see v2 patches' discussions).
>>
>> Maybe I didn't express my idea enough. I don't want to change the behavior of  
>> iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
>> in iio_hwmon_probe. I have something link the patch below in mind.
>>
>> Best regards,
>> Alexander
>> ---
>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> index b550ba5..e32d150 100644
>> --- a/drivers/hwmon/iio_hwmon.c
>> +++ b/drivers/hwmon/iio_hwmon.c
>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>>                 name = dev->of_node->name;
>>  
>>         channels = iio_channel_get_all(dev);
>> -       if (IS_ERR(channels))
>> -               return PTR_ERR(channels);
>> +       if (IS_ERR(channels)) {
>> +               if (PTR_ERR(channels) == -ENODEV)
>> +                       return -EPROBE_DEFER;
>> +               else
>> +                       return PTR_ERR(channels);
>> +       }
>>  
>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>         if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.
> 
> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).
If that did happen I'd be tempted to introduce a new function to be
used outside of probe.  I definitely don't like rewriting in individual
drivers.

The defer question is still rather open in my mind. Will address it in the
other thread.

> 
> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.
Ultimately we probably need to rethink how the registration works
(it was written before deferred probing came along and that has rather
changed things for us!)

For now I'm not convinced that deferring is that painful even if we
do have to try reprobing every time a new module gets probed..

Jonathan
> 
> Thanks,
> Quentin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 26/07/16 17:04, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>>>>
>>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>>>> returning
>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>> driver/device
>>>>>> dependencies seems not right for me at this place.
>>>>>
>>>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>>>> driver? We'll have to change the error code, things we are apparently
>>>>> trying to avoid (see v2 patches' discussions).
>>>>
>>>> Maybe I didn't express my idea enough. I don't want to change the behavior
>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
>>>> below in mind.
>>>>
>>>> Best regards,
>>>> Alexander
>>>> ---
>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>>> index b550ba5..e32d150 100644
>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
>>>> *pdev)
>>>>
>>>>                 name = dev->of_node->name;
>>>>         
>>>>         channels = iio_channel_get_all(dev);
>>>>
>>>> -       if (IS_ERR(channels))
>>>> -               return PTR_ERR(channels);
>>>> +       if (IS_ERR(channels)) {
>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>> +                       return -EPROBE_DEFER;
>>>> +               else
>>>> +                       return PTR_ERR(channels);
>>>> +       }
>>>>
>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>         if (st == NULL) {
>>>
>>> Indeed, I misunderstood what you told me.
>>>
>>> Actually, the patch you proposed is part of my v1
>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>> (https://lkml.org/lkml/2016/7/15/215).
>>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
>>> in -EPROBE_DEFER.
>>
>> Thanks for the links.
>>
>>> What I thought you were proposing was to change the -ENODEV return code
>>> inside iio_channel_get_all. This cannot be an option since the function
>>> might be called outside of a probe (it is not yet, but might be in the
>>> future?).
>>
>> AFAICS this is a helper function not knowing about device probing itself. And 
>> it should stay at that.
>>
>>> Of what I understood, two possibilities are then possible (proposed
>>> either by Guenter or Jonathan): either rework the iio framework to
>>> register iio map array earlier or to use late_initcall instead of init
>>> for the driver consuming the iio channels.
>>
>> Interestingly using this problem would not arise due to module dependencies. 
>> But using late_initcall would mean this needs to be done on any driver using 
>> iio channels? I would rather keep those consumers simple.
>>
> Me too, but that would imply a solution in iio. The change you propose above
> isn't exactly simple either, and would also be needed in each consumer driver.
> 
> Just for the record, I dislike the late_initcall solution as well, but I prefer
> it over blindly converting ENODEV to EPROBE_DEFER.
I'm falling on the other side on this one right now. Though I'd be tempted
to renaming the function to something like iio_channel_get_all_or_defer
to make it explicit that it can result in deferred probing.

To my mind late_initcall tricks are rather hideous as well.  At least 
deferred probing should always work (with a fair cost associated with it of
course)

Lots of discussions ongoing on how to do dependency resolution that will
hopefully mean deferred probing only occurs in the pathological cases in
the future anyway. Interesting to see where that goes.

Jonathan
> 
> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Guenter Roeck]

On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
> On 26/07/16 17:04, Guenter Roeck wrote:
> > On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
> >> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> >>> On 26/07/2016 11:05, Alexander Stein wrote:
> >>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> >>>>> On 26/07/2016 10:21, Alexander Stein wrote:
> >>>>>> On Tuesday 26 July 2016 09:43:44, 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.
> >>>>>>
> >>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
> >>>>>> returning
> >>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> >>>>>> driver/device
> >>>>>> dependencies seems not right for me at this place.
> >>>>>
> >>>>> Then what if the iio_channel_get_all is called outside of the probe of a
> >>>>> driver? We'll have to change the error code, things we are apparently
> >>>>> trying to avoid (see v2 patches' discussions).
> >>>>
> >>>> Maybe I didn't express my idea enough. I don't want to change the behavior
> >>>> of iio_channel_get_all at all. Just the result evaluation of
> >>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> >>>> below in mind.
> >>>>
> >>>> Best regards,
> >>>> Alexander
> >>>> ---
> >>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> >>>> index b550ba5..e32d150 100644
> >>>> --- a/drivers/hwmon/iio_hwmon.c
> >>>> +++ b/drivers/hwmon/iio_hwmon.c
> >>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> >>>> *pdev)
> >>>>
> >>>>                 name = dev->of_node->name;
> >>>>         
> >>>>         channels = iio_channel_get_all(dev);
> >>>>
> >>>> -       if (IS_ERR(channels))
> >>>> -               return PTR_ERR(channels);
> >>>> +       if (IS_ERR(channels)) {
> >>>> +               if (PTR_ERR(channels) == -ENODEV)
> >>>> +                       return -EPROBE_DEFER;
> >>>> +               else
> >>>> +                       return PTR_ERR(channels);
> >>>> +       }
> >>>>
> >>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> >>>>         if (st == NULL) {
> >>>
> >>> Indeed, I misunderstood what you told me.
> >>>
> >>> Actually, the patch you proposed is part of my v1
> >>> (https://lkml.org/lkml/2016/6/28/203) and v2
> >>> (https://lkml.org/lkml/2016/7/15/215).
> >>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> >>> in -EPROBE_DEFER.
> >>
> >> Thanks for the links.
> >>
> >>> What I thought you were proposing was to change the -ENODEV return code
> >>> inside iio_channel_get_all. This cannot be an option since the function
> >>> might be called outside of a probe (it is not yet, but might be in the
> >>> future?).
> >>
> >> AFAICS this is a helper function not knowing about device probing itself. And 
> >> it should stay at that.
> >>
> >>> Of what I understood, two possibilities are then possible (proposed
> >>> either by Guenter or Jonathan): either rework the iio framework to
> >>> register iio map array earlier or to use late_initcall instead of init
> >>> for the driver consuming the iio channels.
> >>
> >> Interestingly using this problem would not arise due to module dependencies. 
> >> But using late_initcall would mean this needs to be done on any driver using 
> >> iio channels? I would rather keep those consumers simple.
> >>
> > Me too, but that would imply a solution in iio. The change you propose above
> > isn't exactly simple either, and would also be needed in each consumer driver.
> > 
> > Just for the record, I dislike the late_initcall solution as well, but I prefer
> > it over blindly converting ENODEV to EPROBE_DEFER.
> I'm falling on the other side on this one right now. Though I'd be tempted
> to renaming the function to something like iio_channel_get_all_or_defer
> to make it explicit that it can result in deferred probing.
> 
Would this new function return -EPROBE_DEFER instead of -ENODEV ?

Thanks,
Guenter

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>> On 26/07/16 17:04, Guenter Roeck wrote:
>> > On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> >> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>> >>> On 26/07/2016 11:05, Alexander Stein wrote:
>> >>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>> >>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>> >>>>>> On Tuesday 26 July 2016 09:43:44, 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.
>> >>>>>>
>> >>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>for
>> >>>>>> returning
>> >>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>> >>>>>> driver/device
>> >>>>>> dependencies seems not right for me at this place.
>> >>>>>
>> >>>>> Then what if the iio_channel_get_all is called outside of the
>probe of a
>> >>>>> driver? We'll have to change the error code, things we are
>apparently
>> >>>>> trying to avoid (see v2 patches' discussions).
>> >>>>
>> >>>> Maybe I didn't express my idea enough. I don't want to change
>the behavior
>> >>>> of iio_channel_get_all at all. Just the result evaluation of
>> >>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>the patch
>> >>>> below in mind.
>> >>>>
>> >>>> Best regards,
>> >>>> Alexander
>> >>>> ---
>> >>>> diff --git a/drivers/hwmon/iio_hwmon.c
>b/drivers/hwmon/iio_hwmon.c
>> >>>> index b550ba5..e32d150 100644
>> >>>> --- a/drivers/hwmon/iio_hwmon.c
>> >>>> +++ b/drivers/hwmon/iio_hwmon.c
>> >>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>platform_device
>> >>>> *pdev)
>> >>>>
>> >>>>                 name = dev->of_node->name;
>> >>>>         
>> >>>>         channels = iio_channel_get_all(dev);
>> >>>>
>> >>>> -       if (IS_ERR(channels))
>> >>>> -               return PTR_ERR(channels);
>> >>>> +       if (IS_ERR(channels)) {
>> >>>> +               if (PTR_ERR(channels) == -ENODEV)
>> >>>> +                       return -EPROBE_DEFER;
>> >>>> +               else
>> >>>> +                       return PTR_ERR(channels);
>> >>>> +       }
>> >>>>
>> >>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>> >>>>         if (st == NULL) {
>> >>>
>> >>> Indeed, I misunderstood what you told me.
>> >>>
>> >>> Actually, the patch you proposed is part of my v1
>> >>> (https://lkml.org/lkml/2016/6/28/203) and v2
>> >>> (https://lkml.org/lkml/2016/7/15/215).
>> >>> Jonathan and Guenter didn't really like the idea of changing the
>-ENODEV
>> >>> in -EPROBE_DEFER.
>> >>
>> >> Thanks for the links.
>> >>
>> >>> What I thought you were proposing was to change the -ENODEV
>return code
>> >>> inside iio_channel_get_all. This cannot be an option since the
>function
>> >>> might be called outside of a probe (it is not yet, but might be
>in the
>> >>> future?).
>> >>
>> >> AFAICS this is a helper function not knowing about device probing
>itself. And 
>> >> it should stay at that.
>> >>
>> >>> Of what I understood, two possibilities are then possible
>(proposed
>> >>> either by Guenter or Jonathan): either rework the iio framework
>to
>> >>> register iio map array earlier or to use late_initcall instead of
>init
>> >>> for the driver consuming the iio channels.
>> >>
>> >> Interestingly using this problem would not arise due to module
>dependencies. 
>> >> But using late_initcall would mean this needs to be done on any
>driver using 
>> >> iio channels? I would rather keep those consumers simple.
>> >>
>> > Me too, but that would imply a solution in iio. The change you
>propose above
>> > isn't exactly simple either, and would also be needed in each
>consumer driver.
>> > 
>> > Just for the record, I dislike the late_initcall solution as well,
>but I prefer
>> > it over blindly converting ENODEV to EPROBE_DEFER.
>> I'm falling on the other side on this one right now. Though I'd be
>tempted
>> to renaming the function to something like
>iio_channel_get_all_or_defer
>> to make it explicit that it can result in deferred probing.
>> 
>Would this new function return -EPROBE_DEFER instead of -ENODEV ?
Yes. Though whether it really adds much over doing that in drivers isn't clear.

Hmm. Needs more thought...
>
>Thanks,
>Guenter
>--
>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: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Jonathan Cameron]

On 26/07/16 08:43, 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 registers
> the driver in the thermal framework.
> 
> This driver probes on three different platform_device_id to take into
> account slight differences (registers bit and temperature computation)
> between Allwinner SoCs ADCs.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> I don't like how I get the temperature for the thermal framework
> (sunxi_gpadc_get_temp). I am doing the exact same process as the iio_hwmon
> but in this function. This is duplicated code. I could use
> iio_read_channel_processed but it needs to have the iio_channel structure
> of the thermal sensor which I can only get with iio_channel_get which
> matches the device name with the consumer_dev_name in the iio_map array.
> And frankly, I do not see myself declaring the driver both as the producer
> and the consumer of IIO channels.
I also can't see a clean way of reusing the value calculation code.  You
could construct and iio_channel by hand easily enough, but that would be
a pretty ugly bit of 'boundary crossing'.
> 
> Moreover, since the thermal sensor is configured to throw an interrupt
> every X seconds, only the first request within these X seconds will not
> time out. This means that because the thermal framework regularly poll the
> thermal sensor, it is really difficult to get a value from sysfs without
> getting first a tonne of times out. I don't know if we can do something
> about that?
Cache the value what ever read it? (thermal or sysfs) then return that
until a new one shows up...

Otherwise, I don't have anything to add to the other review comments.

Jonathan
> 
> v3:
>  - correct wrapping,
>  - add comment about thermal sensor inner working,
>  - move defines in mfd header,
>  - use structure to define SoC specific registers or behaviour,
>  - attach this structure to the device according to of_device_id of the
>    platform device,
>  - use new mutex instead of iio_dev mutex,
>  - use atomic flags to avoid race between request_irq and disable_irq in
>    probe,
>  - switch from processed value to raw, offset and scale values for
>    temperature ADC channel,
>  - remove faulty sentinel in iio_chan_spec array,
>  - add pm_runtime support,
>  - register thermal sensor in thermal framework (forgotten since the
>    beginning whereas it is present in current sun4i-ts driver),
>  - remove useless ret variables to store return value of regmap_reads,
>  - move comments on thermal sensor acquisition period in code instead of
>    header,
>  - adding goto label to unregister iio_map_array when failing to register
>    iio_dev,
> 
> v2:
>  - add SUNXI_GPADC_ prefixes for defines,
>  - correct typo in Kconfig,
>  - reorder alphabetically includes, makefile,
>  - add license header,
>  - fix architecture variations not being handled in interrupt handlers or
>    read raw functions,
>  - fix unability to return negative values from thermal sensor,
>  - add gotos to reduce code repetition,
>  - fix irq variable being unsigned int instead of int,
>  - remove useless dev_err and dev_info,
>  - deactivate all interrupts if probe fails,
>  - fix iio_device_register on NULL variable,
>  - deactivate ADC in the IP when probe fails or when removing driver,
> 
>  drivers/iio/adc/Kconfig           |  12 +
>  drivers/iio/adc/Makefile          |   1 +
>  drivers/iio/adc/sunxi-gpadc-iio.c | 513 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 526 insertions(+)
>  create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 25378c5..429ef16 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -384,6 +384,18 @@ config ROCKCHIP_SARADC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called rockchip_saradc.
>  
> +config SUN4I_GPADC
> +	tristate "Support for the Allwinner SoCs GPADC"
> +	depends on IIO
> +	depends on MFD_SUN4I_GPADC
> +	help
> +	  Say yes here to build support for Allwinner (A10, A13 and A31) SoCs
> +	  GPADC. 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 TI_ADC081C
>  	tristate "Texas Instruments ADC081C/ADC101C/ADC121C family"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 38638d4..14d1739 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -37,6 +37,7 @@ 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
>  obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
> +obj-$(CONFIG_SUN4I_GPADC) += sunxi-gpadc-iio.o
>  obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_ADC0832) += ti-adc0832.o
>  obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.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..5647688
> --- /dev/null
> +++ b/drivers/iio/adc/sunxi-gpadc-iio.c
> @@ -0,0 +1,513 @@
> +/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation.
> + *
> + * The Allwinner SoCs all have an ADC that can also act as a touchscreen
> + * controller and a thermal sensor.
> + * The thermal sensor works only when the ADC acts as a touchscreen controller
> + * and is configured to throw an interrupt every fixed periods of time (let say
> + * every X seconds).
> + * One would be tempted to disable the IP on the hardware side rather than
> + * disabling interrupts to save some power but that reset the internal clock of
> + * the IP, resulting in having to wait X seconds every time we want to read the
> + * value of the thermal sensor.
> + * This is also the reason of using autosuspend in pm_runtime. If there were no
> + * autosuspend, the thermal sensor would need X seconds after every
> + * pm_runtime_get_sync to get a value from the ADC. The autosuspend allows the
> + * thermal sensor to be requested again in a certain time span before it gets
> + * shutdown for not being used.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/driver.h>
> +#include <linux/iio/machine.h>
> +#include <linux/mfd/sunxi-gpadc-mfd.h>
> +
> +const unsigned int sun4i_gpadc_chan_select(unsigned int chan)
> +{
> +	return SUNXI_GPADC_TP_CTRL1_ADC_CHAN_SELECT(chan);
> +}
> +
> +const unsigned int sun6i_gpadc_chan_select(unsigned int chan)
> +{
> +	return SUNXI_GPADC_TP_CTRL1_SUN6I_ADC_CHAN_SELECT(chan);
> +}
> +
> +struct sunxi_gpadc_soc_specific {
> +	const int		temp_offset;
> +	const int		temp_scale;
> +	const unsigned int	tp_mode_en;
> +	const unsigned int	tp_adc_select;
> +	const unsigned int	(*adc_chan_select)(unsigned int chan);
> +};
> +
> +static const struct sunxi_gpadc_soc_specific sun4i_gpadc_soc_specific = {
> +	.temp_offset = -1932,
> +	.temp_scale = 133,
> +	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_TP_MODE_EN,
> +	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT,
> +	.adc_chan_select = &sun4i_gpadc_chan_select,
> +};
> +
> +static const struct sunxi_gpadc_soc_specific sun5i_gpadc_soc_specific = {
> +	.temp_offset = -1447,
> +	.temp_scale = 100,
> +	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_TP_MODE_EN,
> +	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_TP_ADC_SELECT,
> +	.adc_chan_select = &sun4i_gpadc_chan_select,
> +};
> +
> +static const struct sunxi_gpadc_soc_specific sun6i_gpadc_soc_specific = {
> +	.temp_offset = -1623,
> +	.temp_scale = 167,
> +	.tp_mode_en = SUNXI_GPADC_TP_CTRL1_SUN6I_TP_MODE_EN,
> +	.tp_adc_select = SUNXI_GPADC_TP_CTRL1_SUN6I_TP_ADC_SELECT,
> +	.adc_chan_select = &sun6i_gpadc_chan_select,
> +};
> +
> +struct sunxi_gpadc_dev {
> +	struct iio_dev				*indio_dev;
> +	void __iomem				*regs;
> +	struct completion			completion;
> +	int					temp_data;
> +	u32					adc_data;
> +	struct regmap				*regmap;
> +	unsigned int				fifo_data_irq;
> +	atomic_t				ignore_fifo_data_irq;
> +	unsigned int				temp_data_irq;
> +	atomic_t				ignore_temp_data_irq;
> +	const struct sunxi_gpadc_soc_specific	*soc_specific;
> +	struct mutex				mutex;
> +};
> +
> +#define SUNXI_GPADC_ADC_CHANNEL(_channel, _name) {		\
> +	.type = IIO_VOLTAGE,					\
> +	.indexed = 1,						\
> +	.channel = _channel,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.datasheet_name = _name,				\
> +}
> +
> +static struct iio_map sunxi_gpadc_hwmon_maps[] = {
> +	{
> +		.adc_channel_label = "temp_adc",
> +		.consumer_dev_name = "iio_hwmon.0",
> +	},
> +	{ /* sentinel */ },
> +};
> +
> +static const struct iio_chan_spec sunxi_gpadc_channels[] = {
> +	SUNXI_GPADC_ADC_CHANNEL(0, "adc_chan0"),
> +	SUNXI_GPADC_ADC_CHANNEL(1, "adc_chan1"),
> +	SUNXI_GPADC_ADC_CHANNEL(2, "adc_chan2"),
> +	SUNXI_GPADC_ADC_CHANNEL(3, "adc_chan3"),
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE) |
> +				      BIT(IIO_CHAN_INFO_OFFSET),
> +		.datasheet_name = "temp_adc",
> +	},
> +};
> +
> +static int sunxi_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
> +				int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(indio_dev->dev.parent);
> +	mutex_lock(&info->mutex);
> +
> +	reinit_completion(&info->completion);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en |
> +		     info->soc_specific->tp_adc_select |
> +		     info->soc_specific->adc_chan_select(channel));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
> +	enable_irq(info->fifo_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->adc_data;
> +
> +out:
> +	disable_irq(info->fifo_data_irq);
> +	mutex_unlock(&info->mutex);
> +	pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +	pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(indio_dev->dev.parent);
> +	mutex_lock(&info->mutex);
> +
> +	reinit_completion(&info->completion);
> +
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC,
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_TRIG_LEVEL(1) |
> +		     SUNXI_GPADC_TP_INT_FIFOC_TP_FIFO_FLUSH);
> +	/*
> +	 * The temperature sensor returns valid data only when the ADC operates
> +	 * in touchscreen mode.
> +	 */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en);
> +	enable_irq(info->temp_data_irq);
> +
> +	if (!wait_for_completion_timeout(&info->completion,
> +					 msecs_to_jiffies(100))) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*val = info->temp_data;
> +
> +out:
> +	disable_irq(info->temp_data_irq);
> +	mutex_unlock(&info->mutex);
> +	pm_runtime_mark_last_busy(indio_dev->dev.parent);
> +	pm_runtime_put_autosuspend(indio_dev->dev.parent);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_temp_offset(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +
> +	*val = info->soc_specific->temp_offset;
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_temp_scale(struct iio_dev *indio_dev, int *val)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(indio_dev);
> +
> +	*val = info->soc_specific->temp_scale;
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan, int *val,
> +				int *val2, long mask)
> +{
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_OFFSET:
> +		ret = sunxi_gpadc_temp_offset(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->type == IIO_VOLTAGE) {
> +			ret = sunxi_gpadc_adc_read(indio_dev, chan->channel,
> +						   val);
> +			if (ret)
> +				return ret;
> +		} else {
> +			ret = sunxi_gpadc_temp_read(indio_dev, val);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = sunxi_gpadc_temp_scale(indio_dev, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	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;
> +
> +	if (atomic_read(&info->ignore_temp_data_irq))
> +		return IRQ_HANDLED;
> +
> +	if (!regmap_read(info->regmap, SUNXI_GPADC_TEMP_DATA, &info->temp_data))
> +		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;
> +
> +	if (atomic_read(&info->ignore_fifo_data_irq))
> +		return IRQ_HANDLED;
> +
> +	if (!regmap_read(info->regmap, SUNXI_GPADC_TP_DATA, &info->adc_data))
> +		complete(&info->completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sunxi_gpadc_runtime_suspend(struct device *dev)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
> +
> +	mutex_lock(&info->mutex);
> +
> +	/* Disable the ADC on IP */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1, 0);
> +	/* Disable temperature sensor on IP */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR, 0);
> +
> +	mutex_unlock(&info->mutex);
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_runtime_resume(struct device *dev)
> +{
> +	struct sunxi_gpadc_dev *info = iio_priv(dev_get_drvdata(dev));
> +
> +	mutex_lock(&info->mutex);
> +
> +	/* clkin = 6MHz */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL0,
> +		     SUNXI_GPADC_TP_CTRL0_ADC_CLK_DIVIDER(2) |
> +		     SUNXI_GPADC_TP_CTRL0_FS_DIV(7) |
> +		     SUNXI_GPADC_TP_CTRL0_T_ACQ(63));
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL1,
> +		     info->soc_specific->tp_mode_en);
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_CTRL3,
> +		     SUNXI_GPADC_TP_CTRL3_FILTER_EN |
> +		     SUNXI_GPADC_TP_CTRL3_FILTER_TYPE(1));
> +	/* period = SUNXI_GPADC_TP_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~1.3s */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_TPR,
> +		     SUNXI_GPADC_TP_TPR_TEMP_ENABLE |
> +		     SUNXI_GPADC_TP_TPR_TEMP_PERIOD(1953));
> +
> +	mutex_unlock(&info->mutex);
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_get_temp(void *data, int *temp)
> +{
> +	struct sunxi_gpadc_dev *info = (struct sunxi_gpadc_dev *)data;
> +	int val, scale, offset;
> +
> +	/* If reading temperature times out, take stored previous value. */
> +	if (sunxi_gpadc_temp_read(info->indio_dev, &val))
> +		val = info->temp_data;
> +	sunxi_gpadc_temp_scale(info->indio_dev, &scale);
> +	sunxi_gpadc_temp_offset(info->indio_dev, &offset);
> +
> +	*temp = (val + offset) * scale;
> +
> +	return 0;
> +}
> +
> +static const struct thermal_zone_of_device_ops sunxi_ts_tz_ops = {
> +	.get_temp = &sunxi_gpadc_get_temp,
> +};
> +
> +static const struct dev_pm_ops sunxi_gpadc_pm_ops = {
> +	.runtime_suspend = &sunxi_gpadc_runtime_suspend,
> +	.runtime_resume = &sunxi_gpadc_runtime_resume,
> +};
> +
> +static int sunxi_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev;
> +	int ret, irq;
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	struct thermal_zone_device *tzd;
> +
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(pdev->dev.parent);
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	info = iio_priv(indio_dev);
> +
> +	mutex_init(&info->mutex);
> +	info->regmap = sunxi_gpadc_mfd_dev->regmap;
> +	info->indio_dev = indio_dev;
> +	atomic_set(&info->ignore_fifo_data_irq, 1);
> +	atomic_set(&info->ignore_temp_data_irq, 1);
> +	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->soc_specific = (struct sunxi_gpadc_soc_specific *)platform_get_device_id(pdev)->driver_data;
> +
> +	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
> +						   &sunxi_ts_tz_ops);
> +	if (IS_ERR(tzd)) {
> +		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
> +			PTR_ERR(tzd));
> +		return PTR_ERR(tzd);
> +	}
> +
> +	pm_runtime_set_autosuspend_delay(&pdev->dev,
> +					 SUNXI_GPADC_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	irq = platform_get_irq_byname(pdev, "TEMP_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no TEMP_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	disable_irq(irq);
> +	info->temp_data_irq = irq;
> +	atomic_set(&info->ignore_temp_data_irq, 0);
> +
> +	irq = platform_get_irq_byname(pdev, "FIFO_DATA_PENDING");
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"no FIFO_DATA_PENDING interrupt registered\n");
> +		ret = irq;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	disable_irq(irq);
> +	info->fifo_data_irq = irq;
> +	atomic_set(&info->ignore_fifo_data_irq, 0);
> +
> +	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");
> +		goto err;
> +	}
> +
> +	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");
> +		goto err_map;
> +	}
> +
> +	return 0;
> +
> +err_map:
> +	iio_map_array_unregister(indio_dev);
> +
> +err:
> +	pm_runtime_put(&pdev->dev);
> +	/* Disable all hardware interrupts */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> +
> +	return ret;
> +}
> +
> +static int sunxi_gpadc_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_dev *info;
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> +	info = iio_priv(indio_dev);
> +	iio_device_unregister(indio_dev);
> +	iio_map_array_unregister(indio_dev);
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	/* Disable all hardware interrupts */
> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id sunxi_gpadc_id[] = {
> +	{ "sun4i-a10-gpadc-iio", (kernel_ulong_t)&sun4i_gpadc_soc_specific },
> +	{ "sun5i-a13-gpadc-iio", (kernel_ulong_t)&sun5i_gpadc_soc_specific },
> +	{ "sun6i-a31-gpadc-iio", (kernel_ulong_t)&sun6i_gpadc_soc_specific },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver sunxi_gpadc_driver = {
> +	.driver = {
> +		.name = "sunxi-gpadc-iio",
> +		.pm = &sunxi_gpadc_pm_ops,
> +	},
> +	.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 v3 3/4] mfd: mfd-core: reattach mfd of_node to cells without of_compatible

[Maxime Ripard]

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

Hi Lee,

On Tue, Aug 09, 2016 at 02:48:47PM +0100, Lee Jones wrote:
> On Tue, 26 Jul 2016, Quentin Schulz wrote:
> 
> > When an MFD cell has an of_compatible (meaning it is present in the Device
> > Tree), other nodes can reference it using a phandle.
> > 
> > However when the MFD cell is not declared in the Device Tree, the only way
> > other nodes can reference it are by using a phandle to the MFD. Then when
> > this MFD cell tries to register itself in one framework, the registration
> > is denied by the framework because it is not matching the of_node of the
> > node which is referenced by the phandle in one of the other nodes.
> > 
> > This reattaches the of_node of the MFD to the MFD cell device structure
> > when the MFD cell has no of_compatible.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > ---
> > 
> > We need this modification to register the thermal sensor in the thermal
> > framework.
> > 
> > Added in v3.
> > 
> >  drivers/mfd/mfd-core.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> Can you show me the DT code where this is used?
> 
> Is it used by a patch in this series?

Yes and no.

This is not used directly by any code found in those patches.

However, those patches are actually a rewrite of an existing driver
that was there before (drivers/input/touchscreen/sun4i-ts.c), that
already had some DT bindings and was enabled in a few DT already [1].

The issue here is that for the new driver to be able to follow the
phandles as it used to (which is also already used [2]). Obviously, in
the usual mechanism (at least when you don't declare the mfd childs in
the DT), the childs won't have any of_node associated to it, and this
is fine in most cases.

This is where things get messy. The MFD childs will also register to
their framework without, and then the whole phandle lookup goes nuts,
because the phandles will point to the MFD's of_node, but no one will
actually be registered anywhere with that of_node, which means that we
broke all the links expressed by the phandles.

Maxime

1: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n1520
2: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n130

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v3 4/4] iio: adc: add support for Allwinner SoCs ADC

[Maxime Ripard]

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

On Thu, Aug 04, 2016 at 10:41:00AM +0200, Quentin Schulz wrote:
> > However, I'm not sure this is even possible. Isn't the point of the
> > runtime_pm precisely to not be called while you're using the device?
> 
> I agree on the principle but I am using runtime_pm functions (I am
> mainly talking about the pm_runtime_put function) when probing or
> removing the driver. Let's say we remove the mutex locks in runtime_pm
> functions, what will happen if we are reading raw values from the ADC
> when removing the ADC driver for example?

Most likely, the first thing you will be doing in your remove is to
unregister from the framework, so you won't be able to start any new
conversion. So that case shouldn't happen.

> >> +	/* Disable all hardware interrupts */
> >> +	regmap_write(info->regmap, SUNXI_GPADC_TP_INT_FIFOC, 0);
> > 
> > This looks like the wrong place to do that. You'll disable the
> > interrupts of all the devices of the MFD, which is probbaly not what
> > you want to do (and if you do, you want to do it in the MFD driver).
> 
> Yes but all subdrivers of the MFD are using IIO channels from the ADC
> driver so anyway they would not work at all.

I'm not sure what you mean by that.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

Re: [PATCH v3 3/4] mfd: mfd-core: reattach mfd of_node to cells without of_compatible

[Lee Jones]

On Wed, 24 Aug 2016, Maxime Ripard wrote:

> Hi Lee,
> 
> On Tue, Aug 09, 2016 at 02:48:47PM +0100, Lee Jones wrote:
> > On Tue, 26 Jul 2016, Quentin Schulz wrote:
> > 
> > > When an MFD cell has an of_compatible (meaning it is present in the Device
> > > Tree), other nodes can reference it using a phandle.
> > > 
> > > However when the MFD cell is not declared in the Device Tree, the only way
> > > other nodes can reference it are by using a phandle to the MFD. Then when
> > > this MFD cell tries to register itself in one framework, the registration
> > > is denied by the framework because it is not matching the of_node of the
> > > node which is referenced by the phandle in one of the other nodes.
> > > 
> > > This reattaches the of_node of the MFD to the MFD cell device structure
> > > when the MFD cell has no of_compatible.
> > > 
> > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > > ---
> > > 
> > > We need this modification to register the thermal sensor in the thermal
> > > framework.
> > > 
> > > Added in v3.
> > > 
> > >  drivers/mfd/mfd-core.c | 14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > Can you show me the DT code where this is used?
> > 
> > Is it used by a patch in this series?
> 
> Yes and no.
> 
> This is not used directly by any code found in those patches.
> 
> However, those patches are actually a rewrite of an existing driver
> that was there before (drivers/input/touchscreen/sun4i-ts.c), that
> already had some DT bindings and was enabled in a few DT already [1].
> 
> The issue here is that for the new driver to be able to follow the
> phandles as it used to (which is also already used [2]). Obviously, in
> the usual mechanism (at least when you don't declare the mfd childs in
> the DT), the childs won't have any of_node associated to it, and this
> is fine in most cases.
> 
> This is where things get messy. The MFD childs will also register to
> their framework without, and then the whole phandle lookup goes nuts,
> because the phandles will point to the MFD's of_node, but no one will
> actually be registered anywhere with that of_node, which means that we
> broke all the links expressed by the phandles.

I'm concerned that this change may have unintended side-effects for
existing drivers.  Can you point me to the C code where this is
causing an issue.  Perhaps we can solve the issue without changing
subsystem core code.  By doing so we reduced the chance of
destructive ramifications for others.

> 1: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n1520
> 2: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n130
> 



-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Quentin Schulz]

On 15/08/2016 23:35, Jonathan Cameron wrote:
> 
> 
> On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>>> On 26/07/16 17:04, Guenter Roeck wrote:
>>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>>>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>>>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>>>>>>>
>>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>> for
>>>>>>>>> returning
>>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>>>>> driver/device
>>>>>>>>> dependencies seems not right for me at this place.
>>>>>>>>
>>>>>>>> Then what if the iio_channel_get_all is called outside of the
>> probe of a
>>>>>>>> driver? We'll have to change the error code, things we are
>> apparently
>>>>>>>> trying to avoid (see v2 patches' discussions).
>>>>>>>
>>>>>>> Maybe I didn't express my idea enough. I don't want to change
>> the behavior
>>>>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>> the patch
>>>>>>> below in mind.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Alexander
>>>>>>> ---
>>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c
>> b/drivers/hwmon/iio_hwmon.c
>>>>>>> index b550ba5..e32d150 100644
>>>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>> platform_device
>>>>>>> *pdev)
>>>>>>>
>>>>>>>                 name = dev->of_node->name;
>>>>>>>         
>>>>>>>         channels = iio_channel_get_all(dev);
>>>>>>>
>>>>>>> -       if (IS_ERR(channels))
>>>>>>> -               return PTR_ERR(channels);
>>>>>>> +       if (IS_ERR(channels)) {
>>>>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>>>>> +                       return -EPROBE_DEFER;
>>>>>>> +               else
>>>>>>> +                       return PTR_ERR(channels);
>>>>>>> +       }
>>>>>>>
>>>>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>>>>         if (st == NULL) {
>>>>>>
>>>>>> Indeed, I misunderstood what you told me.
>>>>>>
>>>>>> Actually, the patch you proposed is part of my v1
>>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>>>>> (https://lkml.org/lkml/2016/7/15/215).
>>>>>> Jonathan and Guenter didn't really like the idea of changing the
>> -ENODEV
>>>>>> in -EPROBE_DEFER.
>>>>>
>>>>> Thanks for the links.
>>>>>
>>>>>> What I thought you were proposing was to change the -ENODEV
>> return code
>>>>>> inside iio_channel_get_all. This cannot be an option since the
>> function
>>>>>> might be called outside of a probe (it is not yet, but might be
>> in the
>>>>>> future?).
>>>>>
>>>>> AFAICS this is a helper function not knowing about device probing
>> itself. And 
>>>>> it should stay at that.
>>>>>
>>>>>> Of what I understood, two possibilities are then possible
>> (proposed
>>>>>> either by Guenter or Jonathan): either rework the iio framework
>> to
>>>>>> register iio map array earlier or to use late_initcall instead of
>> init
>>>>>> for the driver consuming the iio channels.
>>>>>
>>>>> Interestingly using this problem would not arise due to module
>> dependencies. 
>>>>> But using late_initcall would mean this needs to be done on any
>> driver using 
>>>>> iio channels? I would rather keep those consumers simple.
>>>>>
>>>> Me too, but that would imply a solution in iio. The change you
>> propose above
>>>> isn't exactly simple either, and would also be needed in each
>> consumer driver.
>>>>
>>>> Just for the record, I dislike the late_initcall solution as well,
>> but I prefer
>>>> it over blindly converting ENODEV to EPROBE_DEFER.
>>> I'm falling on the other side on this one right now. Though I'd be
>> tempted
>>> to renaming the function to something like
>> iio_channel_get_all_or_defer
>>> to make it explicit that it can result in deferred probing.
>>>
>> Would this new function return -EPROBE_DEFER instead of -ENODEV ?
> Yes. Though whether it really adds much over doing that in drivers isn't clear.
> 
> Hmm. Needs more thought...

Either we do the exact same "hack" as in the v2[1] in what you call
iio_channel_get_all_or_defer or we duplicate the code from
iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem
right to me but I really dislike the late_initcall method. With this
method we can only have one level of "channel dependency".

This means if we ever create a new driver which depends on channels from
the driver using late_initcall, we will also have to use late_initcall
and we can't be sure the new driver will always be probed after the
driver he depends on.

[1] https://lkml.org/lkml/2016/7/15/215

Quentin
>>
>> Thanks,
>> Guenter
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 3/4] mfd: mfd-core: reattach mfd of_node to cells without of_compatible

[Quentin Schulz]

On 31/08/2016 13:56, Lee Jones wrote:
> On Wed, 24 Aug 2016, Maxime Ripard wrote:
> 
>> Hi Lee,
>>
>> On Tue, Aug 09, 2016 at 02:48:47PM +0100, Lee Jones wrote:
>>> On Tue, 26 Jul 2016, Quentin Schulz wrote:
>>>
>>>> When an MFD cell has an of_compatible (meaning it is present in the Device
>>>> Tree), other nodes can reference it using a phandle.
>>>>
>>>> However when the MFD cell is not declared in the Device Tree, the only way
>>>> other nodes can reference it are by using a phandle to the MFD. Then when
>>>> this MFD cell tries to register itself in one framework, the registration
>>>> is denied by the framework because it is not matching the of_node of the
>>>> node which is referenced by the phandle in one of the other nodes.
>>>>
>>>> This reattaches the of_node of the MFD to the MFD cell device structure
>>>> when the MFD cell has no of_compatible.
>>>>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>>> ---
>>>>
>>>> We need this modification to register the thermal sensor in the thermal
>>>> framework.
>>>>
>>>> Added in v3.
>>>>
>>>>  drivers/mfd/mfd-core.c | 14 +++++++++-----
>>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> Can you show me the DT code where this is used?
>>>
>>> Is it used by a patch in this series?
>>
>> Yes and no.
>>
>> This is not used directly by any code found in those patches.
>>
>> However, those patches are actually a rewrite of an existing driver
>> that was there before (drivers/input/touchscreen/sun4i-ts.c), that
>> already had some DT bindings and was enabled in a few DT already [1].
>>
>> The issue here is that for the new driver to be able to follow the
>> phandles as it used to (which is also already used [2]). Obviously, in
>> the usual mechanism (at least when you don't declare the mfd childs in
>> the DT), the childs won't have any of_node associated to it, and this
>> is fine in most cases.
>>
>> This is where things get messy. The MFD childs will also register to
>> their framework without, and then the whole phandle lookup goes nuts,
>> because the phandles will point to the MFD's of_node, but no one will
>> actually be registered anywhere with that of_node, which means that we
>> broke all the links expressed by the phandles.
> 
> I'm concerned that this change may have unintended side-effects for
> existing drivers.  Can you point me to the C code where this is
> causing an issue.  Perhaps we can solve the issue without changing
> subsystem core code.  By doing so we reduced the chance of
> destructive ramifications for others.
> 

I have actually a problem with this driver as well: the MFD driver is
probed thrice (once for itself and once for each of its subdrivers). I
haven't found yet why. Anyway.

In our case (Allwinner SoCs), the thermal sensor of the SoC is exposed
via the ADC. As Maxime explained, this driver is meant to replace an
existing driver
(http://lxr.free-electrons.com/source/drivers/input/touchscreen/sun4i-ts.c)
which already has its node
(http://lxr.free-electrons.com/source/arch/arm/boot/dts/sun5i.dtsi#L653)
in the DT.

The thermal driver has a node
(http://lxr.free-electrons.com/source/arch/arm/boot/dts/sun5i-a13.dtsi#L70)
in the DT which registers the ADC driver's node as a thermal sensor via
a phandle.

To avoid modifying the DT, the MFD driver is using the same node as the
existing driver but the MFD children are not added as children of that
node. This means the new ADC driver (which is a child of the MFD driver
and "node-less") which registers in thermal framework will be registered
without any of_node and thus, won't actually be used because the phandle
registered by the thermal driver in the DT does not match the of_node
(which is null) of the ADC driver registering in the thermal framework.

The function used to register in the thermal framework is
devm_thermal_zone_of_sensor_register
(http://lxr.free-electrons.com/source/drivers/thermal/of-thermal.c#L602).
It takes a device with an of_node referenced as a phandle in the DT and
any structure to pass to the get_temp function of the thermal framework
(among other parameters)
AFAIK, the device is only used for node matching between the phandle in
the thermal driver's node and the device's of_node while the structure
is used to compute the temperature.


>From this, we found two solutions:
- reattach the MFD driver's node to all its children, (what we did)
- "impersonate" the MFD driver in the devm_thermal_zone_of_sensor_register:

instead of
tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
                                           &sun4i_ts_tz_ops);

we could have:
tzd = devm_thermal_zone_of_sensor_register(pdev->dev.parent, 0, info,

                                           &sun4i_ts_tz_ops);

I'm starting to think the second solution is more appropriate.

Let me know your thoughts on this!

Quentin

>> 1: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n1520
>> 2: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n130
>>
> 
> 
> 

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Quentin Schulz]

On 01/09/2016 09:15, Quentin Schulz wrote:
> On 15/08/2016 23:35, Jonathan Cameron wrote:
>>
>>
>> On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>>>> On 26/07/16 17:04, Guenter Roeck wrote:
>>>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>>>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>>>>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>>>>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>>>>>>>>
>>>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>>> for
>>>>>>>>>> returning
>>>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>>>>>> driver/device
>>>>>>>>>> dependencies seems not right for me at this place.
>>>>>>>>>
>>>>>>>>> Then what if the iio_channel_get_all is called outside of the
>>> probe of a
>>>>>>>>> driver? We'll have to change the error code, things we are
>>> apparently
>>>>>>>>> trying to avoid (see v2 patches' discussions).
>>>>>>>>
>>>>>>>> Maybe I didn't express my idea enough. I don't want to change
>>> the behavior
>>>>>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>>> the patch
>>>>>>>> below in mind.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Alexander
>>>>>>>> ---
>>>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c
>>> b/drivers/hwmon/iio_hwmon.c
>>>>>>>> index b550ba5..e32d150 100644
>>>>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>>> platform_device
>>>>>>>> *pdev)
>>>>>>>>
>>>>>>>>                 name = dev->of_node->name;
>>>>>>>>         
>>>>>>>>         channels = iio_channel_get_all(dev);
>>>>>>>>
>>>>>>>> -       if (IS_ERR(channels))
>>>>>>>> -               return PTR_ERR(channels);
>>>>>>>> +       if (IS_ERR(channels)) {
>>>>>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>>>>>> +                       return -EPROBE_DEFER;
>>>>>>>> +               else
>>>>>>>> +                       return PTR_ERR(channels);
>>>>>>>> +       }
>>>>>>>>
>>>>>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>>>>>         if (st == NULL) {
>>>>>>>
>>>>>>> Indeed, I misunderstood what you told me.
>>>>>>>
>>>>>>> Actually, the patch you proposed is part of my v1
>>>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>>>>>> (https://lkml.org/lkml/2016/7/15/215).
>>>>>>> Jonathan and Guenter didn't really like the idea of changing the
>>> -ENODEV
>>>>>>> in -EPROBE_DEFER.
>>>>>>
>>>>>> Thanks for the links.
>>>>>>
>>>>>>> What I thought you were proposing was to change the -ENODEV
>>> return code
>>>>>>> inside iio_channel_get_all. This cannot be an option since the
>>> function
>>>>>>> might be called outside of a probe (it is not yet, but might be
>>> in the
>>>>>>> future?).
>>>>>>
>>>>>> AFAICS this is a helper function not knowing about device probing
>>> itself. And 
>>>>>> it should stay at that.
>>>>>>
>>>>>>> Of what I understood, two possibilities are then possible
>>> (proposed
>>>>>>> either by Guenter or Jonathan): either rework the iio framework
>>> to
>>>>>>> register iio map array earlier or to use late_initcall instead of
>>> init
>>>>>>> for the driver consuming the iio channels.
>>>>>>
>>>>>> Interestingly using this problem would not arise due to module
>>> dependencies. 
>>>>>> But using late_initcall would mean this needs to be done on any
>>> driver using 
>>>>>> iio channels? I would rather keep those consumers simple.
>>>>>>
>>>>> Me too, but that would imply a solution in iio. The change you
>>> propose above
>>>>> isn't exactly simple either, and would also be needed in each
>>> consumer driver.
>>>>>
>>>>> Just for the record, I dislike the late_initcall solution as well,
>>> but I prefer
>>>>> it over blindly converting ENODEV to EPROBE_DEFER.
>>>> I'm falling on the other side on this one right now. Though I'd be
>>> tempted
>>>> to renaming the function to something like
>>> iio_channel_get_all_or_defer
>>>> to make it explicit that it can result in deferred probing.
>>>>
>>> Would this new function return -EPROBE_DEFER instead of -ENODEV ?
>> Yes. Though whether it really adds much over doing that in drivers isn't clear.
>>
>> Hmm. Needs more thought...
> 
> Either we do the exact same "hack" as in the v2[1] in what you call
> iio_channel_get_all_or_defer or we duplicate the code from
> iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem
> right to me but I really dislike the late_initcall method. With this
> method we can only have one level of "channel dependency".
> 
> This means if we ever create a new driver which depends on channels from
> the driver using late_initcall, we will also have to use late_initcall
> and we can't be sure the new driver will always be probed after the
> driver he depends on.
> 
> [1] https://lkml.org/lkml/2016/7/15/215
> 
> Quentin

Should I revert back to the hack introduced in v2 then?

>>>
>>> Thanks,
>>> Guenter
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

[Jonathan Cameron]

On 01/09/16 10:03, Quentin Schulz wrote:
> On 01/09/2016 09:15, Quentin Schulz wrote:
>> On 15/08/2016 23:35, Jonathan Cameron wrote:
>>>
>>>
>>> On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>>>>> On 26/07/16 17:04, Guenter Roeck wrote:
>>>>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>>>>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>>>>>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>>>>>>> On Tuesday 26 July 2016 09:43:44, 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.
>>>>>>>>>>>
>>>>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>>>> for
>>>>>>>>>>> returning
>>>>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>>>>>>> driver/device
>>>>>>>>>>> dependencies seems not right for me at this place.
>>>>>>>>>>
>>>>>>>>>> Then what if the iio_channel_get_all is called outside of the
>>>> probe of a
>>>>>>>>>> driver? We'll have to change the error code, things we are
>>>> apparently
>>>>>>>>>> trying to avoid (see v2 patches' discussions).
>>>>>>>>>
>>>>>>>>> Maybe I didn't express my idea enough. I don't want to change
>>>> the behavior
>>>>>>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>>>> the patch
>>>>>>>>> below in mind.
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Alexander
>>>>>>>>> ---
>>>>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c
>>>> b/drivers/hwmon/iio_hwmon.c
>>>>>>>>> index b550ba5..e32d150 100644
>>>>>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>>>> platform_device
>>>>>>>>> *pdev)
>>>>>>>>>
>>>>>>>>>                 name = dev->of_node->name;
>>>>>>>>>         
>>>>>>>>>         channels = iio_channel_get_all(dev);
>>>>>>>>>
>>>>>>>>> -       if (IS_ERR(channels))
>>>>>>>>> -               return PTR_ERR(channels);
>>>>>>>>> +       if (IS_ERR(channels)) {
>>>>>>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>>>>>>> +                       return -EPROBE_DEFER;
>>>>>>>>> +               else
>>>>>>>>> +                       return PTR_ERR(channels);
>>>>>>>>> +       }
>>>>>>>>>
>>>>>>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>>>>>>         if (st == NULL) {
>>>>>>>>
>>>>>>>> Indeed, I misunderstood what you told me.
>>>>>>>>
>>>>>>>> Actually, the patch you proposed is part of my v1
>>>>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>>>>>>> (https://lkml.org/lkml/2016/7/15/215).
>>>>>>>> Jonathan and Guenter didn't really like the idea of changing the
>>>> -ENODEV
>>>>>>>> in -EPROBE_DEFER.
>>>>>>>
>>>>>>> Thanks for the links.
>>>>>>>
>>>>>>>> What I thought you were proposing was to change the -ENODEV
>>>> return code
>>>>>>>> inside iio_channel_get_all. This cannot be an option since the
>>>> function
>>>>>>>> might be called outside of a probe (it is not yet, but might be
>>>> in the
>>>>>>>> future?).
>>>>>>>
>>>>>>> AFAICS this is a helper function not knowing about device probing
>>>> itself. And 
>>>>>>> it should stay at that.
>>>>>>>
>>>>>>>> Of what I understood, two possibilities are then possible
>>>> (proposed
>>>>>>>> either by Guenter or Jonathan): either rework the iio framework
>>>> to
>>>>>>>> register iio map array earlier or to use late_initcall instead of
>>>> init
>>>>>>>> for the driver consuming the iio channels.
>>>>>>>
>>>>>>> Interestingly using this problem would not arise due to module
>>>> dependencies. 
>>>>>>> But using late_initcall would mean this needs to be done on any
>>>> driver using 
>>>>>>> iio channels? I would rather keep those consumers simple.
>>>>>>>
>>>>>> Me too, but that would imply a solution in iio. The change you
>>>> propose above
>>>>>> isn't exactly simple either, and would also be needed in each
>>>> consumer driver.
>>>>>>
>>>>>> Just for the record, I dislike the late_initcall solution as well,
>>>> but I prefer
>>>>>> it over blindly converting ENODEV to EPROBE_DEFER.
>>>>> I'm falling on the other side on this one right now. Though I'd be
>>>> tempted
>>>>> to renaming the function to something like
>>>> iio_channel_get_all_or_defer
>>>>> to make it explicit that it can result in deferred probing.
>>>>>
>>>> Would this new function return -EPROBE_DEFER instead of -ENODEV ?
>>> Yes. Though whether it really adds much over doing that in drivers isn't clear.
>>>
>>> Hmm. Needs more thought...
>>
>> Either we do the exact same "hack" as in the v2[1] in what you call
>> iio_channel_get_all_or_defer or we duplicate the code from
>> iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem
>> right to me but I really dislike the late_initcall method. With this
>> method we can only have one level of "channel dependency".
>>
>> This means if we ever create a new driver which depends on channels from
>> the driver using late_initcall, we will also have to use late_initcall
>> and we can't be sure the new driver will always be probed after the
>> driver he depends on.
>>
>> [1] https://lkml.org/lkml/2016/7/15/215
>>
>> Quentin
> 
> Should I revert back to the hack introduced in v2 then?
I think so.  Sorry I didn't see this until after you'd sent v4.

That hack had it's disadvantages but in many ways it was a least clean.

Jonathan
> 
>>>>
>>>> Thanks,
>>>> Guenter
>>>> --
>>>> 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
>