[PATCH 0/7] add CPU thermal throttling to Allwinner A33 SoC

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.

The Allwinner A33 only has a thermal sensor present in the GPADC. In
addition, there is not an existing DT binding for the GPADC. Thus, we do
not need the sun4i-gpadc MFD driver which was made to keep DT compatibility
and probe subdrivers without the need to add DT subnodes.

This series of patch adds the CPU thermal sensor for the A33 and CPU thermal
throttling. It also adds DT binding documentation for the IIO and MFD GPADC
drivers. Finally, it adds the cpu-supply property to the CPU node needed by
the Sinlinx SinA33 and Olinuxino A33 to adapt their CPU regulator voltage
depending on the currently used OPP. The other A33 boards all have their
cpu-supply property set.

This patch *HAS NOT* been tested on the Olinuxino A33.
 @Stefan (or anyone owning an Olinuxino A33), could you test this patch
 series on your board, test CPUfreq and tell us if it works in a stable
 manner? Thanks!

This series of patch is based on this[1] and this[2][3] series of patch.

[1] https://lkml.org/lkml/2016/12/13/298 : "[PATCH v9] add support for Allwinner
SoCs ADC"
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-December/473962.html
: "[PATCH] Allwinner A33 CPU frequency scaling support" without PATCH 4/6
[3] https://lkml.org/lkml/2016/12/19/72 : "[PATCH v2] ARM: dts: sun8i: add
opp-v2 table for A33"

Quentin Schulz (7):
  Documentation: DT: bindings: iio: adc: add documentation for Allwinner
    SoCs' GPADC driver
  Documentation: DT: bindings: mfd: add documentation for Allwinner
    SoCs' GPADC MFD driver
  iio: adc: sun4i-gpadc-iio: add support for A33 thermal sensor
  ARM: dts: sun8i-a33-sinlinx-sina33: add cpu-supply
  ARM: dts: sun8i-a33-olinuxino: add cpu-supply
  ARM: dtsi: sun8i-a33: add A33 thermal sensor
  ARM: dtsi: sun8i-a33: add CPU thermal throttling

 .../bindings/iio/adc/sun4i-gpadc-iio.txt           |  57 ++++++
 .../devicetree/bindings/mfd/sun4i-gpadc.txt        |  47 +++++
 arch/arm/boot/dts/sun8i-a33-olinuxino.dts          |   4 +
 arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts     |   4 +
 arch/arm/boot/dts/sun8i-a33.dtsi                   |  59 ++++++
 drivers/iio/adc/Kconfig                            |  21 ++-
 drivers/iio/adc/sun4i-gpadc-iio.c                  | 204 ++++++++++++++++-----
 include/linux/mfd/sun4i-gpadc.h                    |   4 +
 8 files changed, 343 insertions(+), 57 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt

-- 
2.9.3

The Allwinner SoCs all have an ADC that can also act as a thermal sensor
and sometimes as a touchscreen controller. If there is a touchscreen
controller, the first four channels can be used either for the ADC or
the touchscreen and the fifth channel is used for the thermal sensor.
If there is not a touchscreen controller, the one and only channel is
used for the thermal sensor.

This patch adds the documentation for the driver of the Allwinner SoCs'
GPADC.

Signed-off-by: Quentin Schulz <redacted>
---
 .../bindings/iio/adc/sun4i-gpadc-iio.txt           | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt b/Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt
new file mode 100644
index 0000000..aab768d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/sun4i-gpadc-iio.txt

@@ -0,0 +1,57 @@
+Allwinner SoCs' GPADC Device Tree bindings
+------------------------------------------
+
+The Allwinner SoCs all have an ADC that can also act as a thermal sensor and
+sometimes as a touchscreen controller. If there is a touchscreen controller, the
+first four channels can be used either for the ADC or the touchscreen and the
+fifth channel is used for the thermal sensor.
+If there is not a touchscreen controller, the one and only channel is used for
+the thermal sensor.
+
+Currently, the touchscreen controller does not have a driver using this ADC
+driver. The touchscreen controller is currently driven only by
+input/touchscreen/sun4i-ts.c which is absolutely incompatible with this driver.
+
+The Allwinner A10, A13 and A31 SoCs already have a DT binding for the
+aforementioned input driver, thus an MFD driver matches the existing DT binding
+(mfd/sun4i-gpadc.c) and replaces the input driver. No DT binding is required for
+these SoCs' ADC, everything is handled by the MFD which is matching the existing
+DT binding for input/touchscreen/sun4i-ts.c.
+
+The Allwinner A33 GPADC only have a thermal sensor and have a proper DT binding
+for this driver unlike the previously mentioned SoCs.
+
+Required properties:
+ - compatible: "allwinner,sun8i-a33-gpadc-iio"
+
+Optional properties:
+(for use with thermal framework for CPU thermal throttling for example, and/or
+ IIO consumers)
+ - #thermal-sensor-cells = <0>; (see
+Documentation/devicetree/bindings/thermal/thermal.txt)
+ - #io-channel-cells = <0>; (see
+Documentation/devicetree/bindings/iio/iio-bindings.txt)
+
+Example:
+
+thermal-zones {
+	cpu_thermal {
+		thermal-sensors = <&rtp>;
+		[...]
+	};
+};
+
+soc at 01c00000 {
+	[...]
+	rtp: rtp at 01c25000 {
+		compatible = "allwinner,sun8i-a33-gpadc-iio";
+		reg = <0x01c25000 0x100>;
+		#thermal-sensor-cells = <0>;
+		#io-channel-cells = <0>;
+	};
+}
+
+iio_hwmon {
+	compatible = "iio-hwmon";
+	io-channels = <&rtp>;
+};

-- 
2.9.3

The Allwinner SoCs all have an ADC that can also act as a thermal sensor
and sometimes as a touchscreen controller. If there is a touchscreen
controller, the first four channels can be used either for the ADC or
the touchscreen and the fifth channel is used for the thermal sensor.
If there is not a touchscreen controller, the one and only channel is
used for the thermal sensor.

The Allwinner SoCs already have an existing DT binding for the
touchscreen controller and thermal sensor for the sun4i-ts input driver
which does let the user use the ADC. To keep backward compatibility,
this MFD driver re-uses the same bindings as the sun4i-ts input driver
and will probe the required drivers to make the ADC and thermal sensor
work.

This patch adds the binding documentation for the MFD driver of the
Allwinner SoCs' GPADC.

Signed-off-by: Quentin Schulz <redacted>
---
 .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 47 ++++++++++++++++++++++
 1 file changed, 47 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt

diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
new file mode 100644
index 0000000..bc4b4f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt

@@ -0,0 +1,47 @@
+Allwinner SoCs' GPADC Device Tree bindings
+------------------------------------------
+
+The Allwinner SoCs all have an ADC that can also act as a thermal sensor and
+sometimes as a touchscreen controller. If there is a touchscreen controller, the
+first four channels can be used either for the ADC or the touchscreen and the
+fifth channel is used for the thermal sensor.
+If there is not a touchscreen controller, the one and only channel is used for
+the thermal sensor.
+
+Currently, the touchscreen controller does not have a driver using this ADC
+driver. The touchscreen controller is currently driven only by
+input/touchscreen/sun4i-ts.c which is absolutely incompatible with this driver.
+
+The Allwinner A10, A13 and A31 SoCs already have a DT binding for the
+aforementioned input driver, thus this MFD driver matches the existing DT
+binding (mfd/sun4i-gpadc.c).
+To keep DT binding compatibility, the MFD replaces the sun4i-ts input driver and
+probes required drivers (IIO GPADC driver (iio/adc/sun4i-gpadc-iio.c),
+iio-hwmon and soon the touchscreen driver) without the need for a DT binding for
+each driver.
+
+Required properties:
+ - compatible: one of:
+	- "allwinner,sun4i-a10-ts",
+	- "allwinner,sun5i-a13-ts",
+	- "allwinner,sun6i-a31-ts"
+ - #thermal-sensor-cells = <0>;
+
+Example:
+
+thermal-zones {
+	cpu_thermal {
+		thermal-sensors = <&rtp>;
+		[...]
+	};
+};
+
+soc at 01c00000 {
+	[...]
+	rtp: rtp at 01c25000 {
+		compatible = "allwinner,sun6i-a31-ts";
+		reg = <0x01c25000 0x100>;
+		interrupts = <GIC_SPI 28 IRQ_TYPE_LEVEL_HIGH>;
+		#thermal-sensor-cells = <0>;
+	};
+};

-- 
2.9.3

This adds the cpu-supply DT property to the cpu0 DT node needed by
the board to adapt the regulator voltage depending on the currently use
OPP.

Signed-off-by: Quentin Schulz <redacted>
---

This hasn't been tested on the board but it is what I understand from the
schematics[1] of the board.

Stefan (or anyone owning this board), could you test this series of patch on the
Olinuxino A33, test CPUfreq on it and tell us if it works? Thanks!

[1] https://github.com/OLIMEX/OLINUXINO/raw/master/HARDWARE/A33/A33-OLinuXino_Rev_B1.pdf

 arch/arm/boot/dts/sun8i-a33-olinuxino.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33-olinuxino.dts b/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
index 9ea637e..df55f54 100644
--- a/arch/arm/boot/dts/sun8i-a33-olinuxino.dts
+++ b/arch/arm/boot/dts/sun8i-a33-olinuxino.dts

@@ -72,6 +72,10 @@
 	};
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc3>;
+};
+
 &ehci0 {
 	status = "okay";
 };

-- 
2.9.3

This adds CPU thermal throttling for the Allwinner A33. It uses the
thermal sensor present in the SoC's GPADC.

Signed-off-by: Quentin Schulz <redacted>
---
 arch/arm/boot/dts/sun8i-a33.dtsi | 47 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index 1fcae81..735ebea 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi

@@ -43,6 +43,7 @@
  */
 
 #include "sun8i-a23-a33.dtsi"
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	cpu0_opp_table: opp_table0 {

@@ -79,6 +80,9 @@
 			clocks = <&ccu CLK_CPUX>;
 			clock-names = "cpu";
 			operating-points-v2 = <&cpu0_opp_table>;
+			cooling-min-level = <0>;
+			cooling-max-level = <3>;
+			#cooling-cells = <2>;
 		};
 
 		cpu at 2 {

@@ -100,6 +104,49 @@
 		status = "disabled";
 	};
 
+	thermal-zones {
+		cpu_thermal {
+			/* milliseconds */
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&rtp>;
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_alert0>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+				map1 {
+					trip = <&cpu_alert1>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+
+			trips {
+				cpu_alert0: cpu_alert0 {
+					/* milliCelsius */
+					temperature = <75000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_alert1: cpu_alert1 {
+					/* milliCelsius */
+					temperature = <90000>;
+					hysteresis = <2000>;
+					type = "hot";
+				};
+
+				cpu_crit: cpu_crit {
+					/* milliCelsius */
+					temperature = <110000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+	};
+
 	memory {
 		reg = <0x40000000 0x80000000>;
 	};

-- 
2.9.3

This adds the DT node for the thermal sensor present in the Allwinner
A33 GPADC.

Signed-off-by: Quentin Schulz <redacted>
---
 arch/arm/boot/dts/sun8i-a33.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi
index 2878a77..1fcae81 100644
--- a/arch/arm/boot/dts/sun8i-a33.dtsi
+++ b/arch/arm/boot/dts/sun8i-a33.dtsi

@@ -151,6 +151,13 @@
 			reset-names = "ahb";
 		};
 
+		rtp: rtp at 01c25000 {
+			compatible = "allwinner,sun8i-a33-gpadc-iio";
+			reg = <0x01c25000 0x100>;
+			#thermal-sensor-cells = <0>;
+			#io-channel-cells = <0>;
+		};
+
 		fe0: display-frontend at 01e00000 {
 			compatible = "allwinner,sun8i-a33-display-frontend";
 			reg = <0x01e00000 0x20000>;

@@ -261,6 +268,11 @@
 			};
 		};
 	};
+
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&rtp>;
+	};
 };
 
 &ccu {

-- 
2.9.3

This adds the cpu-supply DT property to the cpu0 DT node needed by
the board to adapt the regulator voltage depending on the currently used
OPP.

Signed-off-by: Quentin Schulz <redacted>
---
 arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
index fef6abc..0901c57 100644
--- a/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts
+++ b/arch/arm/boot/dts/sun8i-a33-sinlinx-sina33.dts

@@ -63,6 +63,10 @@
 	};
 };
 
+&cpu0 {
+	cpu-supply = <&reg_dcdc3>;
+};
+
 &ehci0 {
 	status = "okay";
 };

-- 
2.9.3

This adds support for the Allwinner A33 thermal sensor.

Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
which is dedicated to the thermal sensor. Moreover, its thermal sensor
does not generate interruptions, thus we only need to directly read the
register storing the temperature value.

The MFD used by the A10, A13 and A31, was created to avoid breaking the
DT binding, but since the nodes for the ADC weren't there for the A33,
it is not needed.

Signed-off-by: Quentin Schulz <redacted>
---
 drivers/iio/adc/Kconfig           |  21 ++--
 drivers/iio/adc/sun4i-gpadc-iio.c | 204 ++++++++++++++++++++++++++++----------
 include/linux/mfd/sun4i-gpadc.h   |   4 +
 3 files changed, 172 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6a6d369..06041ff 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig

@@ -437,17 +437,24 @@ config STX104
 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.
-
-	  The thermal sensor slows down ADC readings and can be disabled by
+# MFD_SUN4I_GPADC is needed for sun4i, sun5i and sun6i but not for sun8i
+	select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
+# THERMAL_OF can be disabled on sun4i, sun5i and sun6i to quicken ADC readings
+	depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I
+	depends on !TOUCHSCREEN_SUN4I
+	help
+	  Say yes here to build support for Allwinner (A10, A13, A31 and A33)
+	  SoCs GPADC.
+
+	  The ADC on A10, A13 and A31 provides 4 channels which can be used as
+	  an ADC or as a touchscreen input and one channel for thermal sensor.
+	  Their thermal sensor slows down ADC readings and can be disabled by
 	  disabling CONFIG_THERMAL_OF. However, the thermal sensor should be
 	  enabled by default since the SoC temperature is usually more critical
 	  than ADC readings.
 
+	  The ADC on A33 provides one channel for thermal sensor.
+
 	  To compile this driver as a module, choose M here: the module will be
 	  called sun4i-gpadc-iio.
 

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index a8e134f..8be694e 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c

@@ -1,4 +1,4 @@
-/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
+/* ADC driver for sunxi platforms' (A10, A13, A31 and A33) GPADC
  *
  * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com>
  *

@@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
 	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
 };
 
+static const struct gpadc_data sun8i_gpadc_data = {
+	.temp_offset = -1662,
+	.temp_scale = 162,
+	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
+};
+
 struct sun4i_gpadc_iio {
 	struct iio_dev			*indio_dev;
 	struct completion		completion;

@@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
 	unsigned int			temp_data_irq;
 	atomic_t			ignore_temp_data_irq;
 	const struct gpadc_data		*data;
+	bool				use_dt;
 	/* prevents concurrent reads of temperature and ADC */
 	struct mutex			mutex;
 };

@@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
 	SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
 };
 
+static const struct iio_chan_spec sun8i_gpadc_channels[] = {
+	{
+		.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 const struct regmap_config sun4i_gpadc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
 static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
 				 unsigned int irq)
 {

@@ -231,7 +255,6 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val,
 err:
 	pm_runtime_put_autosuspend(indio_dev->dev.parent);
 	mutex_unlock(&info->mutex);
-
 	return ret;
 }
 

@@ -246,6 +269,19 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
 static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	int ret;
+
+	if (info->use_dt) {
+		pm_runtime_get_sync(indio_dev->dev.parent);
+
+		ret = regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+		if (!ret)
+			pm_runtime_mark_last_busy(indio_dev->dev.parent);
+
+		pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+		return 0;
+	}
 
 	return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
 }

@@ -410,7 +446,7 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 			  unsigned int *irq, atomic_t *atomic)
 {
 	int ret;
-	struct sun4i_gpadc_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
+	struct sun4i_gpadc_dev *mfd_dev;
 	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(&pdev->dev));
 
 	/*

@@ -427,6 +463,8 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 	 */
 	atomic_set(atomic, 1);
 
+	mfd_dev = dev_get_drvdata(pdev->dev.parent);
+
 	ret = platform_get_irq_byname(pdev, name);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "no %s interrupt registered\n", name);

@@ -454,31 +492,68 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 	return 0;
 }
 
-static int sun4i_gpadc_probe(struct platform_device *pdev)
+static const struct of_device_id sun4i_gpadc_of_id[] = {
+	{
+		.compatible = "allwinner,sun8i-a33-gpadc-iio",
+		.data = &sun8i_gpadc_data,
+	},
+	{ /* sentinel */ }
+};
+
+static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
+				struct iio_dev *indio_dev)
 {
-	struct sun4i_gpadc_iio *info;
-	struct iio_dev *indio_dev;
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	const struct of_device_id *of_dev;
+	struct thermal_zone_device *tzd;
+	struct resource *mem;
+	void __iomem *base;
 	int ret;
-	struct sun4i_gpadc_dev *sun4i_gpadc_dev;
 
-	sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
+	of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
+	if (!of_dev)
+		return -ENODEV;
+
+	info->use_dt = true;
+	info->data = (struct gpadc_data *)of_dev->data;
+	indio_dev->num_channels = ARRAY_SIZE(sun8i_gpadc_channels);
+	indio_dev->channels = sun8i_gpadc_channels;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					     &sun4i_gpadc_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		ret = PTR_ERR(info->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
 
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
-	if (!indio_dev)
-		return -ENOMEM;
+	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
+						   &sun4i_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);
+	}
 
-	info = iio_priv(indio_dev);
-	platform_set_drvdata(pdev, indio_dev);
+	return 0;
+}
 
-	mutex_init(&info->mutex);
+static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
+				 struct iio_dev *indio_dev)
+{
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	struct sun4i_gpadc_dev *sun4i_gpadc_dev;
+	int ret;
+
+	info->use_dt = false;
+	sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
 	info->regmap = sun4i_gpadc_dev->regmap;
-	info->indio_dev = indio_dev;
-	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 = &sun4i_gpadc_iio_info;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+
 	indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
 	indio_dev->channels = sun4i_gpadc_channels;
 

@@ -494,7 +569,6 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 	 * register the sensor if that option is enabled, eventually leaving
 	 * that choice to the user.
 	 */
-
 	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
 		/*
 		 * This driver is a child of an MFD which has a node in the DT

@@ -519,8 +593,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"could not register thermal sensor: %ld\n",
 				PTR_ERR(tzd));
-			ret = PTR_ERR(tzd);
-			goto err;
+			return PTR_ERR(tzd);
 		}
 	} else {
 		indio_dev->num_channels =

@@ -528,49 +601,78 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 		indio_dev->channels = sun4i_gpadc_channels_no_temp;
 	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev,
-					 SUN4I_GPADC_AUTOSUSPEND_DELAY);
-	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_set_suspended(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
-
 	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
 		ret = sun4i_irq_init(pdev, "TEMP_DATA_PENDING",
 				     sun4i_gpadc_temp_data_irq_handler,
 				     "temp_data", &info->temp_data_irq,
 				     &info->ignore_temp_data_irq);
 		if (ret < 0)
-			goto err;
-	}
+			return ret;
 
-	ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
-			     sun4i_gpadc_fifo_data_irq_handler, "fifo_data",
-			     &info->fifo_data_irq, &info->ignore_fifo_data_irq);
-	if (ret < 0)
-		goto err;
+		ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
+				     sun4i_gpadc_fifo_data_irq_handler,
+				     "fifo_data", &info->fifo_data_irq,
+				     &info->ignore_fifo_data_irq);
+		if (ret < 0)
+			return ret;
+	}
 
-	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
-		ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
-		if (ret < 0) {
-			dev_err(&pdev->dev,
-				"failed to register iio map array\n");
-			goto err;
-		}
+	ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register iio map array\n");
+		return ret;
 	}
 
+	return 0;
+}
+
+static int sun4i_gpadc_probe(struct platform_device *pdev)
+{
+	struct sun4i_gpadc_iio *info;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+	platform_set_drvdata(pdev, indio_dev);
+
+	mutex_init(&info->mutex);
+	info->indio_dev = indio_dev;
+	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 = &sun4i_gpadc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	if (pdev->dev.of_node)
+		ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
+	else
+		ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
+
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev,
+					 SUN4I_GPADC_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not register the device\n");
-		goto err_map;
+		goto err;
 	}
 
 	return 0;
 
-err_map:
-	if (IS_ENABLED(CONFIG_THERMAL_OF))
-		iio_map_array_unregister(indio_dev);
-
 err:
+	if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
+		iio_map_array_unregister(indio_dev);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 

@@ -580,10 +682,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 static int sun4i_gpadc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
 
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	if (IS_ENABLED(CONFIG_THERMAL_OF))
+	if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
 		iio_map_array_unregister(indio_dev);
 
 	return 0;

@@ -599,6 +702,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
 static struct platform_driver sun4i_gpadc_driver = {
 	.driver = {
 		.name = "sun4i-gpadc-iio",
+		.of_match_table = sun4i_gpadc_of_id,
 		.pm = &sun4i_gpadc_pm_ops,
 	},
 	.id_table = sun4i_gpadc_id,

diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 509e736..139872c 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h

@@ -38,6 +38,10 @@
 #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x)		(GENMASK(3, 0) & BIT(x))
 #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK			GENMASK(3, 0)
 
+/* TP_CTRL1 bits for sun8i SoCs */
+#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN			BIT(8)
+#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN			BIT(7)
+
 #define SUN4I_GPADC_CTRL2				0x08
 
 #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)

-- 
2.9.3

Hi,

On Tue, Dec 20, 2016 at 11:27:03AM +0100, Quentin Schulz wrote:

The DT bindings should be agnostic from the OS. You can remove all
mention of the implementations details in Linux.

(and you should wrap at 72 characters).

But we already have a binding document for that controller, so you
shouldn't create a new one, reuse the old one that is already there.

+Required properties:
+ - compatible: "allwinner,sun8i-a33-gpadc-iio"

IIO is an implementation detail. The IP is called GPADC.
You're also missing reg.

I wouldn't list that as optional.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161220/34694221/attachment.sig>

On Tue, Dec 20, 2016 at 11:27:04AM +0100, Quentin Schulz wrote:

Same thing here, we already have such a document, please amend it if
needed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161220/e00bb9e3/attachment.sig>

On Tue, Dec 20, 2016 at 11:27:05AM +0100, Quentin Schulz wrote:

The indentation is wrong here, and I wouldn't list all the families
there, this is just going to be always amended, for no particular
reason.

+	select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I

Why did you change the depends on to a select?

And isn't that redundant with the comment just above?

+# THERMAL_OF can be disabled on sun4i, sun5i and sun6i to quicken ADC readings

I'm not sure this is worth adding either. Any option can be added with
any number of side effects, I'm not sure we want to list all of them.

+	depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I

So you can't disable THERMAL_OF on MACH_SUN8I?

+	depends on !TOUCHSCREEN_SUN4I

This should be a different patch.

Again, I'm not sure putting all those details in the Kconfig help
really helps. This is only going to grow with more and more cases, and
this isn't something really helpful anyway.

sun8i_a33. Other SoCs from that same family might have different
channels.

Why is runtime_pm linked to the DT support or not?

Why is that change necessary?

You should have two patches. One to split the MFD part from the probe,
and a second one to add the DT probing.

Spurious change?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161220/c8014a12/attachment-0001.sig>

Hi,

On 20/12/2016 15:44, Maxime Ripard wrote:

On Tue, Dec 20, 2016 at 11:27:05AM +0100, Quentin Schulz wrote:

quoted

This adds support for the Allwinner A33 thermal sensor.

Unlike the A10, A13 and A31, the Allwinner A33 only has one channel
which is dedicated to the thermal sensor. Moreover, its thermal sensor
does not generate interruptions, thus we only need to directly read the
register storing the temperature value.

The MFD used by the A10, A13 and A31, was created to avoid breaking the
DT binding, but since the nodes for the ADC weren't there for the A33,
it is not needed.

Signed-off-by: Quentin Schulz <redacted>
---
 drivers/iio/adc/Kconfig           |  21 ++--
 drivers/iio/adc/sun4i-gpadc-iio.c | 204 ++++++++++++++++++++++++++++----------
 include/linux/mfd/sun4i-gpadc.h   |   4 +
 3 files changed, 172 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6a6d369..06041ff 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig

@@ -437,17 +437,24 @@ config STX104
 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.
-
-	  The thermal sensor slows down ADC readings and can be disabled by
+# MFD_SUN4I_GPADC is needed for sun4i, sun5i and sun6i but not for sun8i

The indentation is wrong here, and I wouldn't list all the families
there, this is just going to be always amended, for no particular
reason.

ACK.

quoted

+	select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I

Why did you change the depends on to a select?

The "depends on" does not have an if condition but "select" has. See:
https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt

And isn't that redundant with the comment just above?

quoted

+# THERMAL_OF can be disabled on sun4i, sun5i and sun6i to quicken ADC readings

I'm not sure this is worth adding either. Any option can be added with
any number of side effects, I'm not sure we want to list all of them.

ACK.

quoted

+	depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I

So you can't disable THERMAL_OF on MACH_SUN8I?

Not in the current state. devm_thermal_zone_of_sensor_register from
sun4i_gpadc_probe_dt returns an error if THERMAL_OF is disabled and
thus, the probe fails. Maybe it should rather fail silently and let the
user choose whether (s)he wants the thermal framework to be able to read
data from this driver?

quoted

+	depends on !TOUCHSCREEN_SUN4I

This should be a different patch.

quoted

+	help
+	  Say yes here to build support for Allwinner (A10, A13, A31 and A33)
+	  SoCs GPADC.
+
+	  The ADC on A10, A13 and A31 provides 4 channels which can be used as
+	  an ADC or as a touchscreen input and one channel for thermal sensor.
+	  Their thermal sensor slows down ADC readings and can be disabled by

Again, I'm not sure putting all those details in the Kconfig help
really helps. This is only going to grow with more and more cases, and
this isn't something really helpful anyway.

Are you suggesting to remove completely the paragraph on why it is
possible to disable CONFIG_THERMAL_OF for the A10, A13 and A31 or only
to remove the mention to SoCs?

quoted

 	  disabling CONFIG_THERMAL_OF. However, the thermal sensor should be
 	  enabled by default since the SoC temperature is usually more critical
 	  than ADC readings.
 
+	  The ADC on A33 provides one channel for thermal sensor.
+
 	  To compile this driver as a module, choose M here: the module will be
 	  called sun4i-gpadc-iio.
 

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index a8e134f..8be694e 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c

@@ -1,4 +1,4 @@
-/* ADC driver for sunxi platforms' (A10, A13 and A31) GPADC
+/* ADC driver for sunxi platforms' (A10, A13, A31 and A33) GPADC
  *
  * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com>
  *

@@ -85,6 +85,12 @@ static const struct gpadc_data sun6i_gpadc_data = {
 	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
 };
 
+static const struct gpadc_data sun8i_gpadc_data = {
+	.temp_offset = -1662,
+	.temp_scale = 162,
+	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
+};
+
 struct sun4i_gpadc_iio {
 	struct iio_dev			*indio_dev;
 	struct completion		completion;

@@ -96,6 +102,7 @@ struct sun4i_gpadc_iio {
 	unsigned int			temp_data_irq;
 	atomic_t			ignore_temp_data_irq;
 	const struct gpadc_data		*data;
+	bool				use_dt;
 	/* prevents concurrent reads of temperature and ADC */
 	struct mutex			mutex;
 };

@@ -138,6 +145,23 @@ static const struct iio_chan_spec sun4i_gpadc_channels_no_temp[] = {
 	SUN4I_GPADC_ADC_CHANNEL(3, "adc_chan3"),
 };
 
+static const struct iio_chan_spec sun8i_gpadc_channels[] = {

sun8i_a33. Other SoCs from that same family might have different
channels.

ACK.

quoted

+	{
+		.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 const struct regmap_config sun4i_gpadc_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
 static int sun4i_prepare_for_irq(struct iio_dev *indio_dev, int channel,
 				 unsigned int irq)
 {

@@ -231,7 +255,6 @@ static int sun4i_gpadc_read(struct iio_dev *indio_dev, int channel, int *val,
 err:
 	pm_runtime_put_autosuspend(indio_dev->dev.parent);
 	mutex_unlock(&info->mutex);
-
 	return ret;
 }
 

@@ -246,6 +269,19 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
 static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	int ret;
+
+	if (info->use_dt) {
+		pm_runtime_get_sync(indio_dev->dev.parent);
+
+		ret = regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+		if (!ret)
+			pm_runtime_mark_last_busy(indio_dev->dev.parent);
+
+		pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+		return 0;
+	}

Why is runtime_pm linked to the DT support or not?

quoted

 
 	return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
 }

The same runtime_pm functions are called when the driver is not probed
from DT.

sun4i_gpadc_read will call sun4i_prepare_for_irq which does a
pm_runtime_get_sync and then at the end of sun4i_gpadc_read,
pm_runtime_mark_last_busy and pm_runtime_put_autosuspend are called.

I just noticed I forgot to add a comment on this one. The A33
documentation tells us there is an interrupt for the thermal sensor but
after struggling with it, it is just false. I validated my guess with
Allwinner Linux kernel which does not wait an interrupt to read the data
register of the thermal sensor.

sun4i_gpadc_read always wait for an interrupt before reading data regs,
so I'm just not calling it and doing the "logic" (reading the data reg
and interfacing with runtime_pm) directly here in sun4i_gpadc_temp.

I'll add a comment on this one. Maybe I should create a function just
for the "logic" of the A33 thermal sensor, so it is less weird than
currently.

quoted

@@ -410,7 +446,7 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 			  unsigned int *irq, atomic_t *atomic)
 {
 	int ret;
-	struct sun4i_gpadc_dev *mfd_dev = dev_get_drvdata(pdev->dev.parent);
+	struct sun4i_gpadc_dev *mfd_dev;
 	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(&pdev->dev));
 
 	/*

@@ -427,6 +463,8 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 	 */
 	atomic_set(atomic, 1);
 
+	mfd_dev = dev_get_drvdata(pdev->dev.parent);
+

Why is that change necessary?

quoted

 	ret = platform_get_irq_byname(pdev, name);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "no %s interrupt registered\n", name);

@@ -454,31 +492,68 @@ static int sun4i_irq_init(struct platform_device *pdev, const char *name,
 	return 0;
 }
 
-static int sun4i_gpadc_probe(struct platform_device *pdev)
+static const struct of_device_id sun4i_gpadc_of_id[] = {
+	{
+		.compatible = "allwinner,sun8i-a33-gpadc-iio",
+		.data = &sun8i_gpadc_data,
+	},
+	{ /* sentinel */ }
+};
+
+static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
+				struct iio_dev *indio_dev)
 {
-	struct sun4i_gpadc_iio *info;
-	struct iio_dev *indio_dev;
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	const struct of_device_id *of_dev;
+	struct thermal_zone_device *tzd;
+	struct resource *mem;
+	void __iomem *base;
 	int ret;
-	struct sun4i_gpadc_dev *sun4i_gpadc_dev;
 
-	sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
+	of_dev = of_match_device(sun4i_gpadc_of_id, &pdev->dev);
+	if (!of_dev)
+		return -ENODEV;
+
+	info->use_dt = true;
+	info->data = (struct gpadc_data *)of_dev->data;
+	indio_dev->num_channels = ARRAY_SIZE(sun8i_gpadc_channels);
+	indio_dev->channels = sun8i_gpadc_channels;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	info->regmap = devm_regmap_init_mmio(&pdev->dev, base,
+					     &sun4i_gpadc_regmap_config);
+	if (IS_ERR(info->regmap)) {
+		ret = PTR_ERR(info->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
 
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
-	if (!indio_dev)
-		return -ENOMEM;
+	tzd = devm_thermal_zone_of_sensor_register(&pdev->dev, 0, info,
+						   &sun4i_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);
+	}
 
-	info = iio_priv(indio_dev);
-	platform_set_drvdata(pdev, indio_dev);
+	return 0;
+}
 
-	mutex_init(&info->mutex);
+static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
+				 struct iio_dev *indio_dev)
+{
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	struct sun4i_gpadc_dev *sun4i_gpadc_dev;
+	int ret;
+
+	info->use_dt = false;
+	sun4i_gpadc_dev = dev_get_drvdata(pdev->dev.parent);
 	info->regmap = sun4i_gpadc_dev->regmap;
-	info->indio_dev = indio_dev;
-	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 = &sun4i_gpadc_iio_info;
-	indio_dev->modes = INDIO_DIRECT_MODE;
+

You should have two patches. One to split the MFD part from the probe,
and a second one to add the DT probing.

ACK.

quoted

 	indio_dev->num_channels = ARRAY_SIZE(sun4i_gpadc_channels);
 	indio_dev->channels = sun4i_gpadc_channels;
 

@@ -494,7 +569,6 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 	 * register the sensor if that option is enabled, eventually leaving
 	 * that choice to the user.
 	 */
-

Spurious change?

Yes, lots of. Sorry, I will be more careful on that.

quoted

 	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
 		/*
 		 * This driver is a child of an MFD which has a node in the DT

@@ -519,8 +593,7 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev,
 				"could not register thermal sensor: %ld\n",
 				PTR_ERR(tzd));
-			ret = PTR_ERR(tzd);
-			goto err;
+			return PTR_ERR(tzd);
 		}
 	} else {
 		indio_dev->num_channels =

@@ -528,49 +601,78 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 		indio_dev->channels = sun4i_gpadc_channels_no_temp;
 	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev,
-					 SUN4I_GPADC_AUTOSUSPEND_DELAY);
-	pm_runtime_use_autosuspend(&pdev->dev);
-	pm_runtime_set_suspended(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
-
 	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
 		ret = sun4i_irq_init(pdev, "TEMP_DATA_PENDING",
 				     sun4i_gpadc_temp_data_irq_handler,
 				     "temp_data", &info->temp_data_irq,
 				     &info->ignore_temp_data_irq);
 		if (ret < 0)
-			goto err;
-	}
+			return ret;
 
-	ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
-			     sun4i_gpadc_fifo_data_irq_handler, "fifo_data",
-			     &info->fifo_data_irq, &info->ignore_fifo_data_irq);
-	if (ret < 0)
-		goto err;
+		ret = sun4i_irq_init(pdev, "FIFO_DATA_PENDING",
+				     sun4i_gpadc_fifo_data_irq_handler,
+				     "fifo_data", &info->fifo_data_irq,
+				     &info->ignore_fifo_data_irq);
+		if (ret < 0)
+			return ret;
+	}
 
-	if (IS_ENABLED(CONFIG_THERMAL_OF)) {
-		ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
-		if (ret < 0) {
-			dev_err(&pdev->dev,
-				"failed to register iio map array\n");
-			goto err;
-		}
+	ret = iio_map_array_register(indio_dev, sun4i_gpadc_hwmon_maps);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register iio map array\n");
+		return ret;
 	}
 
+	return 0;
+}
+
+static int sun4i_gpadc_probe(struct platform_device *pdev)
+{
+	struct sun4i_gpadc_iio *info;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*info));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	info = iio_priv(indio_dev);
+	platform_set_drvdata(pdev, indio_dev);
+
+	mutex_init(&info->mutex);
+	info->indio_dev = indio_dev;
+	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 = &sun4i_gpadc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	if (pdev->dev.of_node)
+		ret = sun4i_gpadc_probe_dt(pdev, indio_dev);
+	else
+		ret = sun4i_gpadc_probe_mfd(pdev, indio_dev);
+
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(&pdev->dev,
+					 SUN4I_GPADC_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not register the device\n");
-		goto err_map;
+		goto err;
 	}
 
 	return 0;
 
-err_map:
-	if (IS_ENABLED(CONFIG_THERMAL_OF))
-		iio_map_array_unregister(indio_dev);
-
 err:
+	if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
+		iio_map_array_unregister(indio_dev);
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 

@@ -580,10 +682,11 @@ static int sun4i_gpadc_probe(struct platform_device *pdev)
 static int sun4i_gpadc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
 
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	if (IS_ENABLED(CONFIG_THERMAL_OF))
+	if (!info->use_dt && IS_ENABLED(CONFIG_THERMAL_OF))
 		iio_map_array_unregister(indio_dev);
 
 	return 0;

@@ -599,6 +702,7 @@ static const struct platform_device_id sun4i_gpadc_id[] = {
 static struct platform_driver sun4i_gpadc_driver = {
 	.driver = {
 		.name = "sun4i-gpadc-iio",
+		.of_match_table = sun4i_gpadc_of_id,
 		.pm = &sun4i_gpadc_pm_ops,
 	},
 	.id_table = sun4i_gpadc_id,

diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 509e736..139872c 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h

@@ -38,6 +38,10 @@
 #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x)		(GENMASK(3, 0) & BIT(x))
 #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK			GENMASK(3, 0)
 
+/* TP_CTRL1 bits for sun8i SoCs */
+#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN			BIT(8)
+#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN			BIT(7)
+
 #define SUN4I_GPADC_CTRL2				0x08
 
 #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)

-- 
2.9.3

Thanks,
Maxime

Thanks,
Quentin

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161220/6300bcb0/attachment.sig>

Hi,

On 20/12/2016 15:25, Maxime Ripard wrote:

Hi,

On Tue, Dec 20, 2016 at 11:27:03AM +0100, Quentin Schulz wrote:

[...]

quoted

+Currently, the touchscreen controller does not have a driver using this ADC
+driver. The touchscreen controller is currently driven only by
+input/touchscreen/sun4i-ts.c which is absolutely incompatible with this driver.
+
+The Allwinner A10, A13 and A31 SoCs already have a DT binding for the
+aforementioned input driver, thus an MFD driver matches the existing DT binding
+(mfd/sun4i-gpadc.c) and replaces the input driver. No DT binding is required for
+these SoCs' ADC, everything is handled by the MFD which is matching the existing
+DT binding for input/touchscreen/sun4i-ts.c.
+
+The Allwinner A33 GPADC only have a thermal sensor and have a proper DT binding
+for this driver unlike the previously mentioned SoCs.

The DT bindings should be agnostic from the OS. You can remove all
mention of the implementations details in Linux.

(and you should wrap at 72 characters).

But we already have a binding document for that controller, so you
shouldn't create a new one, reuse the old one that is already there.

ACK.

quoted

+Required properties:
+ - compatible: "allwinner,sun8i-a33-gpadc-iio"

IIO is an implementation detail. The IP is called GPADC.
You're also missing reg.

ACK.

quoted

+
+Optional properties:
+(for use with thermal framework for CPU thermal throttling for example, and/or
+ IIO consumers)
+ - #thermal-sensor-cells = <0>; (see
+Documentation/devicetree/bindings/thermal/thermal.txt)
+ - #io-channel-cells = <0>; (see
+Documentation/devicetree/bindings/iio/iio-bindings.txt)

I wouldn't list that as optional.

In what sense? Do you mean you wouldn't put them here at all or you
would require them?

Thanks,
Quentin

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

Hi,

On Tue, Dec 20, 2016 at 04:20:04PM +0100, Quentin Schulz wrote:

quoted

quoted

+	select MFD_SUN4I_GPADC if MACH_SUN4I || MACH_SUN5I || MACH_SUN6I

Why did you change the depends on to a select?

The "depends on" does not have an if condition but "select" has. See:
https://www.kernel.org/doc/Documentation/kbuild/kconfig-language.txt

I think something like that should work:

depends on (MFD_SUN4I_GPADC && MACH_SUN4I) || MACH_SUN8I

quoted

quoted

+	depends on THERMAL_OF || MACH_SUN4I || MACH_SUN5I || MACH_SUN6I

So you can't disable THERMAL_OF on MACH_SUN8I?

Not in the current state. devm_thermal_zone_of_sensor_register from
sun4i_gpadc_probe_dt returns an error if THERMAL_OF is disabled and
thus, the probe fails. Maybe it should rather fail silently and let the
user choose whether (s)he wants the thermal framework to be able to read
data from this driver?

I'm usually not a big fan of inconsistencies in the behaviour of
drivers, but let's wait for the second version to discuss that.

quoted

quoted

+	depends on !TOUCHSCREEN_SUN4I

This should be a different patch.

quoted

+	help
+	  Say yes here to build support for Allwinner (A10, A13, A31 and A33)
+	  SoCs GPADC.
+
+	  The ADC on A10, A13 and A31 provides 4 channels which can be used as
+	  an ADC or as a touchscreen input and one channel for thermal sensor.
+	  Their thermal sensor slows down ADC readings and can be disabled by

Again, I'm not sure putting all those details in the Kconfig help
really helps. This is only going to grow with more and more cases, and
this isn't something really helpful anyway.

Are you suggesting to remove completely the paragraph on why it is
possible to disable CONFIG_THERMAL_OF for the A10, A13 and A31 or only
to remove the mention to SoCs?

Both actually :)

quoted

quoted

@@ -246,6 +269,19 @@ static int sun4i_gpadc_adc_read(struct iio_dev *indio_dev, int channel,
 static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(indio_dev);
+	int ret;
+
+	if (info->use_dt) {
+		pm_runtime_get_sync(indio_dev->dev.parent);
+
+		ret = regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+		if (!ret)
+			pm_runtime_mark_last_busy(indio_dev->dev.parent);
+
+		pm_runtime_put_autosuspend(indio_dev->dev.parent);
+
+		return 0;
+	}

Why is runtime_pm linked to the DT support or not?

quoted

 
 	return sun4i_gpadc_read(indio_dev, 0, val, info->temp_data_irq);
 }

The same runtime_pm functions are called when the driver is not probed
from DT.

sun4i_gpadc_read will call sun4i_prepare_for_irq which does a
pm_runtime_get_sync and then at the end of sun4i_gpadc_read,
pm_runtime_mark_last_busy and pm_runtime_put_autosuspend are called.

I just noticed I forgot to add a comment on this one. The A33
documentation tells us there is an interrupt for the thermal sensor but
after struggling with it, it is just false. I validated my guess with
Allwinner Linux kernel which does not wait an interrupt to read the data
register of the thermal sensor.

sun4i_gpadc_read always wait for an interrupt before reading data regs,
so I'm just not calling it and doing the "logic" (reading the data reg
and interfacing with runtime_pm) directly here in sun4i_gpadc_temp.

I'll add a comment on this one. Maybe I should create a function just
for the "logic" of the A33 thermal sensor, so it is less weird than
currently.

Ok. So this is not really about using the DT or not, but rather
whether you're on an A33 or not. You could probably add a broken_irq
field to test against that.

You should probably share the pm_runtim calls too between the two
cases.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161221/bb0d5df4/attachment.sig>

Hi,

On Tue, Dec 20, 2016 at 04:43:58PM +0100, Quentin Schulz wrote:

quoted

quoted

+
+Optional properties:
+(for use with thermal framework for CPU thermal throttling for example, and/or
+ IIO consumers)
+ - #thermal-sensor-cells = <0>; (see
+Documentation/devicetree/bindings/thermal/thermal.txt)
+ - #io-channel-cells = <0>; (see
+Documentation/devicetree/bindings/iio/iio-bindings.txt)

I wouldn't list that as optional.

In what sense? Do you mean you wouldn't put them here at all or you
would require them?

I would require them.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161221/8e164a7c/attachment.sig>