[PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC

[PATCH v4 0/6] IIO-based thermal sensor driver for Allwinner H3 SoC

[Icenowy Zheng]

Allwiner H3 SoC has a thermal sensor, which is a large refactored version of
the old Allwinner "GPADC" (although it have already only thermal part left
in A33).

This patch tried to add support for the sensor in H3 based on the A33 thermal
sensor driver by Quentin Schulz, which is already merged.

Icenowy Zheng (6):
  dt-bindings: update the Allwinner GPADC device tree binding for H3
  iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain
    A33
  iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS
    variants
  iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
  ARM: sun8i: h3: add support for the thermal sensor in H3
  ARM: sun8i: h3: add partial CPU thermal zone

 .../devicetree/bindings/mfd/sun4i-gpadc.txt        |  30 +++-
 arch/arm/boot/dts/sun8i-h3.dtsi                    |  26 ++++
 drivers/iio/adc/sun4i-gpadc-iio.c                  | 173 ++++++++++++++++++++-
 include/linux/mfd/sun4i-gpadc.h                    |  33 +++-
 4 files changed, 249 insertions(+), 13 deletions(-)

-- 
2.13.5

[PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Icenowy Zheng]

Allwinner H3 features a thermal sensor like the one in A33, but has its
register re-arranged, the clock divider moved to CCU (originally the
clock divider is in ADC) and added a pair of bus clock and reset.

Update the binding document to cover H3.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v4:
- Add nvmem calibration data (not yet used by the driver)
Changes in v3:
- Clock name changes.
- Example node name changes.
- Add interupts (not yet used by the driver).

 .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30 ++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
index badff3611a98..6c470d584bf9 100644
--- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
+++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
@@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also act as a thermal sensor
 and sometimes as a touchscreen controller.
 
 Required properties:
-  - compatible: "allwinner,sun8i-a33-ths",
+  - compatible: must contain one of the following compatibles:
+		- "allwinner,sun8i-a33-ths"
+		- "allwinner,sun8i-h3-ths"
   - reg: mmio address range of the chip,
   - #thermal-sensor-cells: shall be 0,
   - #io-channel-cells: shall be 0,
 
-Example:
+Optional properties:
+  - nvmem-cells: A phandle to the calibration data provided by a nvmem device.
+                 If unspecified default values shall be used.
+  - nvmem-cell-names: Should be "calibration-data"
+
+Required properties for the following compatibles:
+		- "allwinner,sun8i-h3-ths"
+  - clocks: the bus clock and the input clock of the ADC,
+  - clock-names: should be "bus" and "mod",
+  - resets: the bus reset of the ADC,
+  - interrupts: the sampling interrupt of the ADC,
+
+Example for A33:
 	ths: ths@01c25000 {
 		compatible = "allwinner,sun8i-a33-ths";
 		reg = <0x01c25000 0x100>;
@@ -17,6 +31,18 @@ Example:
 		#io-channel-cells = <0>;
 	};
 
+Example for H3:
+	ths: thermal-sensor@1c25000 {
+		compatible = "allwinner,sun8i-h3-ths";
+		reg = <0x01c25000 0x400>;
+		clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
+		clock-names = "bus", "mod";
+		resets = <&ccu RST_BUS_THS>;
+		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+		#thermal-sensor-cells = <0>;
+		#io-channel-cells = <0>;
+	};
+
 sun4i, sun5i and sun6i SoCs are also supported via the older binding:
 
 sun4i resistive touchscreen controller
-- 
2.13.5

[PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33

[Icenowy Zheng]

As the H3 SoC, which is also in sun8i line, has totally different
register map for the thermal sensor (a cut down version of GPADC), we
should rename A23/A33-specified registers to contain A33, in order to
prevent obfuscation with H3 registers. Currently these registers are
only prefixed "SUN8I", not "SUN8I_A33".

Add "_A33" after "SUN8I" on the register names.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v4:
- Change A23 to A33, as the driver never supports A23.

 drivers/iio/adc/sun4i-gpadc-iio.c | 2 +-
 include/linux/mfd/sun4i-gpadc.h   | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 137f577d9432..68926b986cd0 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -88,7 +88,7 @@ static const struct gpadc_data sun6i_gpadc_data = {
 static const struct gpadc_data sun8i_a33_gpadc_data = {
 	.temp_offset = -1662,
 	.temp_scale = 162,
-	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
+	.tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN,
 };
 
 struct sun4i_gpadc_iio {
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 139872c2e0fe..78d31984a222 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -38,9 +38,9 @@
 #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)
+/* TP_CTRL1 bits for A33 */
+#define SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN		BIT(8)
+#define SUN8I_A33_GPADC_CTRL1_GPADC_CALI_EN		BIT(7)
 
 #define SUN4I_GPADC_CTRL2				0x08
 
-- 
2.13.5

[PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants

[Icenowy Zheng]

The SoCs after H3 has newer thermal sensor ADCs, which have two clock
inputs (bus clock and sampling clock) and a reset. The registers are
also re-arranged.

This commit reworks the code, adds the process of the clocks and
resets, and allows the sampling start/end code and the position of value
readout register to be altered.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/iio/adc/sun4i-gpadc-iio.c | 123 +++++++++++++++++++++++++++++++++++---
 1 file changed, 116 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 68926b986cd0..97845982d050 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -22,6 +22,7 @@
  * shutdown for not being used.
  */
 
+#include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -31,6 +32,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
+#include <linux/reset.h>
 #include <linux/thermal.h>
 #include <linux/delay.h>
 
@@ -49,6 +51,15 @@ static unsigned int sun6i_gpadc_chan_select(unsigned int chan)
 	return SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(chan);
 }
 
+struct sun4i_gpadc_iio;
+
+/*
+ * Prototypes for these functions, which enable these functions to be
+ * referenced in gpadc_data structures.
+ */
+static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info);
+static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info);
+
 struct gpadc_data {
 	int		temp_offset;
 	int		temp_scale;
@@ -56,6 +67,12 @@ struct gpadc_data {
 	unsigned int	tp_adc_select;
 	unsigned int	(*adc_chan_select)(unsigned int chan);
 	unsigned int	adc_chan_mask;
+	unsigned int	temp_data;
+	int		(*sample_start)(struct sun4i_gpadc_iio *info);
+	int		(*sample_end)(struct sun4i_gpadc_iio *info);
+	bool		has_bus_clk;
+	bool		has_bus_rst;
+	bool		has_mod_clk;
 };
 
 static const struct gpadc_data sun4i_gpadc_data = {
@@ -65,6 +82,9 @@ static const struct gpadc_data sun4i_gpadc_data = {
 	.tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT,
 	.adc_chan_select = &sun4i_gpadc_chan_select,
 	.adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK,
+	.temp_data = SUN4I_GPADC_TEMP_DATA,
+	.sample_start = sun4i_gpadc_sample_start,
+	.sample_end = sun4i_gpadc_sample_end,
 };
 
 static const struct gpadc_data sun5i_gpadc_data = {
@@ -74,6 +94,9 @@ static const struct gpadc_data sun5i_gpadc_data = {
 	.tp_adc_select = SUN4I_GPADC_CTRL1_TP_ADC_SELECT,
 	.adc_chan_select = &sun4i_gpadc_chan_select,
 	.adc_chan_mask = SUN4I_GPADC_CTRL1_ADC_CHAN_MASK,
+	.temp_data = SUN4I_GPADC_TEMP_DATA,
+	.sample_start = sun4i_gpadc_sample_start,
+	.sample_end = sun4i_gpadc_sample_end,
 };
 
 static const struct gpadc_data sun6i_gpadc_data = {
@@ -83,12 +106,18 @@ static const struct gpadc_data sun6i_gpadc_data = {
 	.tp_adc_select = SUN6I_GPADC_CTRL1_TP_ADC_SELECT,
 	.adc_chan_select = &sun6i_gpadc_chan_select,
 	.adc_chan_mask = SUN6I_GPADC_CTRL1_ADC_CHAN_MASK,
+	.temp_data = SUN4I_GPADC_TEMP_DATA,
+	.sample_start = sun4i_gpadc_sample_start,
+	.sample_end = sun4i_gpadc_sample_end,
 };
 
 static const struct gpadc_data sun8i_a33_gpadc_data = {
 	.temp_offset = -1662,
 	.temp_scale = 162,
 	.tp_mode_en = SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN,
+	.temp_data = SUN4I_GPADC_TEMP_DATA,
+	.sample_start = sun4i_gpadc_sample_start,
+	.sample_end = sun4i_gpadc_sample_end,
 };
 
 struct sun4i_gpadc_iio {
@@ -103,6 +132,9 @@ struct sun4i_gpadc_iio {
 	atomic_t			ignore_temp_data_irq;
 	const struct gpadc_data		*data;
 	bool				no_irq;
+	struct clk			*bus_clk;
+	struct clk			*mod_clk;
+	struct reset_control		*reset;
 	/* prevents concurrent reads of temperature and ADC */
 	struct mutex			mutex;
 	struct thermal_zone_device	*tzd;
@@ -277,7 +309,7 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
 	if (info->no_irq) {
 		pm_runtime_get_sync(indio_dev->dev.parent);
 
-		regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
+		regmap_read(info->regmap, info->data->temp_data, val);
 
 		pm_runtime_mark_last_busy(indio_dev->dev.parent);
 		pm_runtime_put_autosuspend(indio_dev->dev.parent);
@@ -383,10 +415,8 @@ static irqreturn_t sun4i_gpadc_fifo_data_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-static int sun4i_gpadc_runtime_suspend(struct device *dev)
+static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info)
 {
-	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
-
 	/* Disable the ADC on IP */
 	regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
 	/* Disable temperature sensor on IP */
@@ -395,10 +425,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int sun4i_gpadc_runtime_resume(struct device *dev)
+static int sun4i_gpadc_runtime_suspend(struct device *dev)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
 
+	return info->data->sample_end(info);
+}
+
+static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info)
+{
 	/* clkin = 6MHz */
 	regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
 		     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
@@ -416,6 +451,13 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
 	return 0;
 }
 
+static int sun4i_gpadc_runtime_resume(struct device *dev)
+{
+	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
+
+	return info->data->sample_start(info);
+}
+
 static int sun4i_gpadc_get_temp(void *data, int *temp)
 {
 	struct sun4i_gpadc_iio *info = data;
@@ -529,17 +571,75 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
 		return ret;
 	}
 
+	if (info->data->has_bus_rst) {
+		info->reset = devm_reset_control_get(&pdev->dev, NULL);
+		if (IS_ERR(info->reset)) {
+			ret = PTR_ERR(info->reset);
+			return ret;
+		}
+
+		ret = reset_control_deassert(info->reset);
+		if (ret)
+			return ret;
+	}
+
+	if (info->data->has_bus_clk) {
+		info->bus_clk = devm_clk_get(&pdev->dev, "bus");
+		if (IS_ERR(info->bus_clk)) {
+			ret = PTR_ERR(info->bus_clk);
+			goto assert_reset;
+		}
+
+		ret = clk_prepare_enable(info->bus_clk);
+		if (ret)
+			goto assert_reset;
+	}
+
+	if (info->data->has_mod_clk) {
+		info->mod_clk = devm_clk_get(&pdev->dev, "mod");
+		if (IS_ERR(info->mod_clk)) {
+			ret = PTR_ERR(info->mod_clk);
+			goto disable_bus_clk;
+		}
+
+		/* Running at 6MHz */
+		ret = clk_set_rate(info->mod_clk, 6000000);
+		if (ret)
+			goto disable_bus_clk;
+
+		ret = clk_prepare_enable(info->mod_clk);
+		if (ret)
+			goto disable_bus_clk;
+	}
+
 	if (!IS_ENABLED(CONFIG_THERMAL_OF))
 		return 0;
 
 	info->sensor_device = &pdev->dev;
 	info->tzd = thermal_zone_of_sensor_register(info->sensor_device, 0,
 						    info, &sun4i_ts_tz_ops);
-	if (IS_ERR(info->tzd))
+	if (IS_ERR(info->tzd)) {
 		dev_err(&pdev->dev, "could not register thermal sensor: %ld\n",
 			PTR_ERR(info->tzd));
+		ret = PTR_ERR(info->tzd);
+		goto disable_mod_clk;
+	}
+
+	return 0;
+
+disable_mod_clk:
+	if (info->data->has_mod_clk)
+		clk_disable_unprepare(info->mod_clk);
+
+disable_bus_clk:
+	if (info->data->has_bus_clk)
+		clk_disable_unprepare(info->bus_clk);
 
-	return PTR_ERR_OR_ZERO(info->tzd);
+assert_reset:
+	if (info->data->has_bus_rst)
+		reset_control_assert(info->reset);
+
+	return ret;
 }
 
 static int sun4i_gpadc_probe_mfd(struct platform_device *pdev,
@@ -698,6 +798,15 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
 	if (!info->no_irq)
 		iio_map_array_unregister(indio_dev);
 
+	if (info->data->has_mod_clk)
+		clk_disable_unprepare(info->mod_clk);
+
+	if (info->data->has_bus_clk)
+		clk_disable_unprepare(info->bus_clk);
+
+	if (info->data->has_bus_rst)
+		reset_control_assert(info->reset);
+
 	return 0;
 }
 
-- 
2.13.5

[PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Icenowy Zheng]

This adds support for the Allwinner H3 thermal sensor.

Allwinner H3 has a thermal sensor like the one in A33, but have its
registers nearly all re-arranged, sample clock moved to CCU and a pair
of bus clock and reset added. It's also the base of newer SoCs' thermal
sensors.

The thermal sensors on A64 and H5 is like the one on H3, but with of
course different formula factors.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
Changes in v4:
- Splitted out some code refactors.
- Code sequence changed back. (The gpadc_data went back to the start of
  the source file)

 drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 97845982d050..f7e4df6bd17a 100644
--- a/drivers/iio/adc/sun4i-gpadc-iio.c
+++ b/drivers/iio/adc/sun4i-gpadc-iio.c
@@ -59,6 +59,8 @@ struct sun4i_gpadc_iio;
  */
 static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info);
 static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info);
+static int sun8i_h3_gpadc_sample_start(struct sun4i_gpadc_iio *info);
+static int sun8i_h3_gpadc_sample_end(struct sun4i_gpadc_iio *info);
 
 struct gpadc_data {
 	int		temp_offset;
@@ -120,6 +122,24 @@ static const struct gpadc_data sun8i_a33_gpadc_data = {
 	.sample_end = sun4i_gpadc_sample_end,
 };
 
+static const struct gpadc_data sun8i_h3_gpadc_data = {
+	/*
+	 * The original formula on the datasheet seems to be wrong.
+	 * These factors are calculated based on the formula in the BSP
+	 * kernel, which is originally Tem = 217 - (T / 8.253), in which Tem
+	 * is the temperature in Celsius degree and T is the raw value
+	 * from the sensor.
+	 */
+	.temp_offset = -1791,
+	.temp_scale = -121,
+	.temp_data = SUN8I_H3_GPADC_TEMP_DATA,
+	.sample_start = sun8i_h3_gpadc_sample_start,
+	.sample_end = sun8i_h3_gpadc_sample_end,
+	.has_bus_clk = true,
+	.has_bus_rst = true,
+	.has_mod_clk = true,
+};
+
 struct sun4i_gpadc_iio {
 	struct iio_dev			*indio_dev;
 	struct completion		completion;
@@ -425,6 +445,14 @@ static int sun4i_gpadc_sample_end(struct sun4i_gpadc_iio *info)
 	return 0;
 }
 
+static int sun8i_h3_gpadc_sample_end(struct sun4i_gpadc_iio *info)
+{
+	/* Disable temperature sensor */
+	regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
+
+	return 0;
+}
+
 static int sun4i_gpadc_runtime_suspend(struct device *dev)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
@@ -451,6 +479,22 @@ static int sun4i_gpadc_sample_start(struct sun4i_gpadc_iio *info)
 	return 0;
 }
 
+static int sun8i_h3_gpadc_sample_start(struct sun4i_gpadc_iio *info)
+{
+	regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2,
+		     SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN |
+		     SUN8I_H3_GPADC_CTRL2_T_ACQ1(31));
+	regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
+		     SUN4I_GPADC_CTRL0_T_ACQ(31));
+	regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL3,
+		     SUN4I_GPADC_CTRL3_FILTER_EN |
+		     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
+	regmap_write(info->regmap, SUN8I_H3_GPADC_INTC,
+		     SUN8I_H3_GPADC_INTC_TEMP_PERIOD(800));
+
+	return 0;
+}
+
 static int sun4i_gpadc_runtime_resume(struct device *dev)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
@@ -537,6 +581,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = {
 		.compatible = "allwinner,sun8i-a33-ths",
 		.data = &sun8i_a33_gpadc_data,
 	},
+	{
+		.compatible = "allwinner,sun8i-h3-ths",
+		.data = &sun8i_h3_gpadc_data,
+	},
 	{ /* sentinel */ }
 };
 
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 78d31984a222..5c2a12101052 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -42,6 +42,9 @@
 #define SUN8I_A33_GPADC_CTRL1_CHOP_TEMP_EN		BIT(8)
 #define SUN8I_A33_GPADC_CTRL1_GPADC_CALI_EN		BIT(7)
 
+/* TP_CTRL1 bits for SoCs after H3 */
+#define SUN8I_H3_GPADC_CTRL1_GPADC_CALI_EN		BIT(17)
+
 #define SUN4I_GPADC_CTRL2				0x08
 
 #define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x)	((GENMASK(3, 0) & (x)) << 28)
@@ -49,7 +52,17 @@
 #define SUN4I_GPADC_CTRL2_PRE_MEA_EN			BIT(24)
 #define SUN4I_GPADC_CTRL2_PRE_MEA_THRE_CNT(x)		(GENMASK(23, 0) & (x))
 
+#define SUN8I_H3_GPADC_CTRL2				0x40
+
+#define SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN		BIT(0)
+#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x)			((GENMASK(15, 0) * (x)) << 16)
+
 #define SUN4I_GPADC_CTRL3				0x0c
+/*
+ * This register is named "Average filter Control Register" in H3 Datasheet,
+ * but the register's definition is the same as the old CTRL3 register.
+ */
+#define SUN8I_H3_GPADC_CTRL3				0x70
 
 #define SUN4I_GPADC_CTRL3_FILTER_EN			BIT(2)
 #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
@@ -71,6 +84,13 @@
 #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
 #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
 
+#define SUN8I_H3_GPADC_INTC				0x44
+
+#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
+#define SUN8I_H3_GPADC_INTC_TEMP_DATA			BIT(8)
+#define SUN8I_H3_GPADC_INTC_TEMP_SHUT			BIT(4)
+#define SUN8I_H3_GPADC_INTC_TEMP_ALARM			BIT(0)
+
 #define SUN4I_GPADC_INT_FIFOS				0x14
 
 #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING		BIT(18)
@@ -80,9 +100,16 @@
 #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING		BIT(1)
 #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING		BIT(0)
 
+#define SUN8I_H3_GPADC_INTS				0x44
+
+#define SUN8I_H3_GPADC_INTS_TEMP_DATA			BIT(8)
+#define SUN8I_H3_GPADC_INTS_TEMP_SHUT			BIT(4)
+#define SUN8I_H3_GPADC_INTS_TEMP_ALARM			BIT(0)
+
 #define SUN4I_GPADC_CDAT				0x1c
 #define SUN4I_GPADC_TEMP_DATA				0x20
 #define SUN4I_GPADC_DATA				0x24
+#define SUN8I_H3_GPADC_TEMP_DATA			0x80
 
 #define SUN4I_GPADC_IRQ_FIFO_DATA			0
 #define SUN4I_GPADC_IRQ_TEMP_DATA			1
-- 
2.13.5

[PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3

[Icenowy Zheng]

As we have gained the support for the thermal sensor in H3, we can now
add its device nodes to the device tree.

Add them to the H3 device tree.

The calibration data of the thermal sensor is still not added, as
it's currently not used, and the SID node is not added yet.

The H5 thermal sensor has some differences, and will be added furtherly.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
Reviewed-by: Chen-Yu Tsai <wens@csie.org>
---
Changes in v4:
- Mention calibration data in commit message.
Changes in v3:
- Clock name changes.
- Splited out thermal zone addition.

 arch/arm/boot/dts/sun8i-h3.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index b36f9f423c39..3220da3ad790 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -72,6 +72,23 @@
 		};
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&ths>;
+	};
+
+	soc {
+		ths: thermal-sensor@1c25000 {
+			compatible = "allwinner,sun8i-h3-ths";
+			reg = <0x01c25000 0x100>;
+			clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
+			clock-names = "bus", "mod";
+			resets = <&ccu RST_BUS_THS>;
+			#thermal-sensor-cells = <0>;
+			#io-channel-cells = <0>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.13.5

[PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone

[Icenowy Zheng]

Because of the restriction of the OF thermal framework, the thermal
sensor will fail to probe if the thermal zone doesn't exist.

Add a partial thermal zone which claims the H3 THS as the thermal sensor.

The cooling device (CPU DVFS) is still not added as it's not ready, and
the trip points are also not added yet.

Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
---
 arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 3220da3ad790..687c6457d214 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -89,6 +89,15 @@
 		};
 	};
 
+	thermal-zones {
+		cpu-thermal {
+			/* milliseconds */
+			polling-delay-passive = <250>;
+			polling-delay = <1000>;
+			thermal-sensors = <&ths>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv7-timer";
 		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
-- 
2.13.5

Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Quentin Schulz]

Hi Icenowy,

On 14/09/2017 16:52, Icenowy Zheng wrote:
> This adds support for the Allwinner H3 thermal sensor.
> 
> Allwinner H3 has a thermal sensor like the one in A33, but have its
> registers nearly all re-arranged, sample clock moved to CCU and a pair
> of bus clock and reset added. It's also the base of newer SoCs' thermal
> sensors.
> 
> The thermal sensors on A64 and H5 is like the one on H3, but with of
> course different formula factors.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
> Changes in v4:
> - Splitted out some code refactors.
> - Code sequence changed back. (The gpadc_data went back to the start of
>   the source file)
> 
>  drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
[...]
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 78d31984a222..5c2a12101052 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
[...]
> +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x)			((GENMASK(15, 0) * (x)) << 16)
> +

You want to replace * by &.

((GENMASK(15, 0) & (x)) << 16)

Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
obvious?

>  #define SUN4I_GPADC_CTRL3				0x0c
> +/*
> + * This register is named "Average filter Control Register" in H3 Datasheet,
> + * but the register's definition is the same as the old CTRL3 register.
> + */
> +#define SUN8I_H3_GPADC_CTRL3				0x70
>  

I would name it as it is in the documentation:
SUN8I_H3_THS_FILTER

No need for comments then.

>  #define SUN4I_GPADC_CTRL3_FILTER_EN			BIT(2)
>  #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> @@ -71,6 +84,13 @@
>  #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
>  #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
>  
> +#define SUN8I_H3_GPADC_INTC				0x44
> +
> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA			BIT(8)
> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT			BIT(4)
> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM			BIT(0)
> +

Since it isn't an ADC anymore but rather just a THS, why don't you use
SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
datasheet.

>  #define SUN4I_GPADC_INT_FIFOS				0x14
>  
>  #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING		BIT(18)
> @@ -80,9 +100,16 @@
>  #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING		BIT(1)
>  #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING		BIT(0)
>  
> +#define SUN8I_H3_GPADC_INTS				0x44

0x48

[...]

1) You're not using irqs, why would you define registers that will never
be used?

2) Why aren't you using irqs? I remember we discussed on IRC that you
had some problems with the H3 when resuming or when probing the driver.
The register would have a zero in it until you have a first sample that
arrived (i.e. after the sample rate you set with T_ACQ) that would make
the thermal framework panic since the thermal sensor would return
something way too hot and shutdown your board?

The H3 apparently supports IRQs, why do you not support them for the
temperature? They might be broken as it is on A33 but then it might be a
good idea to write it down in a comment in the driver (and not adding
the unused registers in the header file) or at least in the commit log.

3) Now that you have support for clocks, wouldn't it be a good idea to
disable them during suspend?

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

Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone

[Quentin Schulz]

Hi Icenowy,

On 14/09/2017 16:52, Icenowy Zheng wrote:
> Because of the restriction of the OF thermal framework, the thermal
> sensor will fail to probe if the thermal zone doesn't exist.
> 

Oh no, that's not good.

We discussed about it on IRC and I even proposed a patch for it, telling
you I would post it on the mailing list soon after. Of course, I forgot
and you definitely should have yelled at me for not doing it :)

I won't be able to test the patch soon. I can send it to you so that you
can test it and integrate it in your patch series so it won't block you.
Otherwise, we'll have to wait for a week or two for me to test it.

Thanks and sorry for forgetting to post the patch you need,
Quentin

> Add a partial thermal zone which claims the H3 THS as the thermal sensor.
> 
> The cooling device (CPU DVFS) is still not added as it's not ready, and
> the trip points are also not added yet.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 3220da3ad790..687c6457d214 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -89,6 +89,15 @@
>  		};
>  	};
>  
> +	thermal-zones {
> +		cpu-thermal {
> +			/* milliseconds */
> +			polling-delay-passive = <250>;
> +			polling-delay = <1000>;
> +			thermal-sensors = <&ths>;
> +		};
> +	};
> +
>  	timer {
>  		compatible = "arm,armv7-timer";
>  		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> 

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

Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[icenowy]

在 2017-09-16 17:45，Quentin Schulz 写道：
> Hi Icenowy,
> 
> On 14/09/2017 16:52, Icenowy Zheng wrote:
>> This adds support for the Allwinner H3 thermal sensor.
>> 
>> Allwinner H3 has a thermal sensor like the one in A33, but have its
>> registers nearly all re-arranged, sample clock moved to CCU and a pair
>> of bus clock and reset added. It's also the base of newer SoCs' 
>> thermal
>> sensors.
>> 
>> The thermal sensors on A64 and H5 is like the one on H3, but with of
>> course different formula factors.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> ---
>> Changes in v4:
>> - Splitted out some code refactors.
>> - Code sequence changed back. (The gpadc_data went back to the start 
>> of
>>   the source file)
>> 
>>  drivers/iio/adc/sun4i-gpadc-iio.c | 48 
>> +++++++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
>>  2 files changed, 75 insertions(+)
> [...]
>> diff --git a/include/linux/mfd/sun4i-gpadc.h 
>> b/include/linux/mfd/sun4i-gpadc.h
>> index 78d31984a222..5c2a12101052 100644
>> --- a/include/linux/mfd/sun4i-gpadc.h
>> +++ b/include/linux/mfd/sun4i-gpadc.h
> [...]
>> +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x)			((GENMASK(15, 0) * (x)) << 
>> 16)
>> +
> 
> You want to replace * by &.
> 
> ((GENMASK(15, 0) & (x)) << 16)
> 
> Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
> obvious?
> 
>>  #define SUN4I_GPADC_CTRL3				0x0c
>> +/*
>> + * This register is named "Average filter Control Register" in H3 
>> Datasheet,
>> + * but the register's definition is the same as the old CTRL3 
>> register.
>> + */
>> +#define SUN8I_H3_GPADC_CTRL3				0x70
>> 
> 
> I would name it as it is in the documentation:
> SUN8I_H3_THS_FILTER

The definition of this register is the same as the CTRL3.

Maybe this name is better, but the similarity between them still needs
to be documented, as the SUN4I_GPADC_CTRL3_XXX macros will be used to
populate this register.

> 
> No need for comments then.
> 
>>  #define SUN4I_GPADC_CTRL3_FILTER_EN			BIT(2)
>>  #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
>> @@ -71,6 +84,13 @@
>>  #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
>>  #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
>> 
>> +#define SUN8I_H3_GPADC_INTC				0x44
>> +
>> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) 
>> << 12)
>> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA			BIT(8)
>> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT			BIT(4)
>> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM			BIT(0)
>> +
> 
> Since it isn't an ADC anymore but rather just a THS, why don't you use
> SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
> datasheet.
> 
>>  #define SUN4I_GPADC_INT_FIFOS				0x14
>> 
>>  #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING		BIT(18)
>> @@ -80,9 +100,16 @@
>>  #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING		BIT(1)
>>  #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING		BIT(0)
>> 
>> +#define SUN8I_H3_GPADC_INTS				0x44
> 
> 0x48
> 
> [...]
> 
> 1) You're not using irqs, why would you define registers that will 
> never
> be used?

I will then rework it to use IRQs, but not now.

Maybe I should add it when I use them?

> 
> 2) Why aren't you using irqs? I remember we discussed on IRC that you
> had some problems with the H3 when resuming or when probing the driver.
> The register would have a zero in it until you have a first sample that
> arrived (i.e. after the sample rate you set with T_ACQ) that would make
> the thermal framework panic since the thermal sensor would return
> something way too hot and shutdown your board?

Nope, it's another problem -- the runtime resume function is even not
called before the first sample, and the first sample will happen when
the THS is still suspended.

> 
> The H3 apparently supports IRQs, why do you not support them for the
> temperature? They might be broken as it is on A33 but then it might be 
> a
> good idea to write it down in a comment in the driver (and not adding
> the unused registers in the header file) or at least in the commit log.
> 
> 3) Now that you have support for clocks, wouldn't it be a good idea to
> disable them during suspend?

Interesting... It's meaningful to disable the mod clock during suspend.

> 
> Thanks,
> Quentin

Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Quentin Schulz]

Hi Icenowy,

On 16/09/2017 12:14, icenowy@aosc.io wrote:
> 在 2017-09-16 17:45，Quentin Schulz 写道：
>> Hi Icenowy,
>>
>> On 14/09/2017 16:52, Icenowy Zheng wrote:
>>> This adds support for the Allwinner H3 thermal sensor.
>>>
>>> Allwinner H3 has a thermal sensor like the one in A33, but have its
>>> registers nearly all re-arranged, sample clock moved to CCU and a pair
>>> of bus clock and reset added. It's also the base of newer SoCs' thermal
>>> sensors.
>>>
>>> The thermal sensors on A64 and H5 is like the one on H3, but with of
>>> course different formula factors.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>>> ---
>>> Changes in v4:
>>> - Splitted out some code refactors.
>>> - Code sequence changed back. (The gpadc_data went back to the start of
>>>   the source file)
>>>
>>>  drivers/iio/adc/sun4i-gpadc-iio.c | 48
>>> +++++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
>>>  2 files changed, 75 insertions(+)
[...]
>>>  #define SUN4I_GPADC_CTRL3                0x0c
>>> +/*
>>> + * This register is named "Average filter Control Register" in H3
>>> Datasheet,
>>> + * but the register's definition is the same as the old CTRL3 register.
>>> + */
>>> +#define SUN8I_H3_GPADC_CTRL3                0x70
>>>
>>
>> I would name it as it is in the documentation:
>> SUN8I_H3_THS_FILTER
> 
> The definition of this register is the same as the CTRL3.
> 
> Maybe this name is better, but the similarity between them still needs
> to be documented, as the SUN4I_GPADC_CTRL3_XXX macros will be used to
> populate this register.
> 
>>
>> No need for comments then.
>>
>>>  #define SUN4I_GPADC_CTRL3_FILTER_EN            BIT(2)
>>>  #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)        (GENMASK(1, 0) & (x))

They have _FILTER_ in their name, isn't it enough?

Just a matter of taste for me.

>>> @@ -71,6 +84,13 @@
>>>  #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN        BIT(1)
>>>  #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN        BIT(0)
>>>
>>> +#define SUN8I_H3_GPADC_INTC                0x44
>>> +
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)        ((GENMASK(19, 0) &
>>> (x)) << 12)
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_DATA            BIT(8)
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT            BIT(4)
>>> +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM            BIT(0)
>>> +
>>
>> Since it isn't an ADC anymore but rather just a THS, why don't you use
>> SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
>> datasheet.
>>
>>>  #define SUN4I_GPADC_INT_FIFOS                0x14
>>>
>>>  #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING        BIT(18)
>>> @@ -80,9 +100,16 @@
>>>  #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING        BIT(1)
>>>  #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING        BIT(0)
>>>
>>> +#define SUN8I_H3_GPADC_INTS                0x44
>>
>> 0x48
>>
>> [...]
>>
>> 1) You're not using irqs, why would you define registers that will never
>> be used?
> 
> I will then rework it to use IRQs, but not now.
> 
> Maybe I should add it when I use them?
> 

Why not make it work right away the way we want :)?

>>
>> 2) Why aren't you using irqs? I remember we discussed on IRC that you
>> had some problems with the H3 when resuming or when probing the driver.
>> The register would have a zero in it until you have a first sample that
>> arrived (i.e. after the sample rate you set with T_ACQ) that would make
>> the thermal framework panic since the thermal sensor would return
>> something way too hot and shutdown your board?
> 
> Nope, it's another problem -- the runtime resume function is even not
> called before the first sample, and the first sample will happen when
> the THS is still suspended.
> 

As discussed on IRC (a long time ago :) ), it's a combination of two
problems:
1) get_temp (used by thermal framework) uses pm_runtime function that
isn't ready yet <= I will send a patch for registering thermal framework
after pm_runtime to you, hopefully in the upcoming hour or tomorrow,

2) The A33 (and H3 in your implementation) does not wait for an
interrupt to read the TEMP_DATA register which resets to 0 when the
sensor is disabled (or until a first sample arrives) i.e. when probing
or when resuming. Using IRQs would get rid of 2). It isn't critical for
A33 as the formula for temp returns something really cold so thermal
does not care. But for the H3, it's critically hot and shuts down your
board. To make it work on the H3, we would have to use a delay in the
pm_resume function equal to the sensor sampling rate and I don't really
like that. If we could use IRQs, it'd be better IMHO (but they aren't
working on A33).

Thanks,
Quentin

>>
>> The H3 apparently supports IRQs, why do you not support them for the
>> temperature? They might be broken as it is on A33 but then it might be a
>> good idea to write it down in a comment in the driver (and not adding
>> the unused registers in the header file) or at least in the commit log.
>>
>> 3) Now that you have support for clocks, wouldn't it be a good idea to
>> disable them during suspend?
> 
> Interesting... It's meaningful to disable the mod clock during suspend.
> 
>>
>> Thanks,
>> Quentin
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Jonathan Cameron]

On Thu, 14 Sep 2017 22:52:46 +0800
Icenowy Zheng <icenowy@aosc.io> wrote:

> Allwinner H3 features a thermal sensor like the one in A33, but has its
> register re-arranged, the clock divider moved to CCU (originally the
> clock divider is in ADC) and added a pair of bus clock and reset.
> 
> Update the binding document to cover H3.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Add nvmem calibration data (not yet used by the driver)
> Changes in v3:
> - Clock name changes.
> - Example node name changes.
> - Add interupts (not yet used by the driver).
> 
>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30 ++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> index badff3611a98..6c470d584bf9 100644
> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also act as a thermal sensor
>  and sometimes as a touchscreen controller.
>  
>  Required properties:
> -  - compatible: "allwinner,sun8i-a33-ths",
> +  - compatible: must contain one of the following compatibles:
> +		- "allwinner,sun8i-a33-ths"
> +		- "allwinner,sun8i-h3-ths"
>    - reg: mmio address range of the chip,
>    - #thermal-sensor-cells: shall be 0,
>    - #io-channel-cells: shall be 0,
>  
> -Example:
> +Optional properties:
> +  - nvmem-cells: A phandle to the calibration data provided by a nvmem device.
> +                 If unspecified default values shall be used.
> +  - nvmem-cell-names: Should be "calibration-data"

I think a cross reference to the nvmem binding docs would be good here.
It wasn't something I could remember coming across before.  Obviously
grep gets you there quickly enough, but a cross reference would be even
better.

Also would it make sense to have an example with these in?

Jonathan
> +
> +Required properties for the following compatibles:
> +		- "allwinner,sun8i-h3-ths"
> +  - clocks: the bus clock and the input clock of the ADC,
> +  - clock-names: should be "bus" and "mod",
> +  - resets: the bus reset of the ADC,
> +  - interrupts: the sampling interrupt of the ADC,
> +
> +Example for A33:
>  	ths: ths@01c25000 {
>  		compatible = "allwinner,sun8i-a33-ths";
>  		reg = <0x01c25000 0x100>;
> @@ -17,6 +31,18 @@ Example:
>  		#io-channel-cells = <0>;
>  	};
>  
> +Example for H3:
> +	ths: thermal-sensor@1c25000 {
> +		compatible = "allwinner,sun8i-h3-ths";
> +		reg = <0x01c25000 0x400>;
> +		clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
> +		clock-names = "bus", "mod";
> +		resets = <&ccu RST_BUS_THS>;
> +		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> +		#thermal-sensor-cells = <0>;
> +		#io-channel-cells = <0>;
> +	};
> +
>  sun4i, sun5i and sun6i SoCs are also supported via the older binding:
>  
>  sun4i resistive touchscreen controller

Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Jonathan Cameron]

On Sat, 16 Sep 2017 11:45:47 +0200
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> Hi Icenowy,
> 
> On 14/09/2017 16:52, Icenowy Zheng wrote:
> > This adds support for the Allwinner H3 thermal sensor.
> > 
> > Allwinner H3 has a thermal sensor like the one in A33, but have its
> > registers nearly all re-arranged, sample clock moved to CCU and a pair
> > of bus clock and reset added. It's also the base of newer SoCs' thermal
> > sensors.
> > 
> > The thermal sensors on A64 and H5 is like the one on H3, but with of
> > course different formula factors.
> > 
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > ---
> > Changes in v4:
> > - Splitted out some code refactors.
> > - Code sequence changed back. (The gpadc_data went back to the start of
> >   the source file)
> > 
> >  drivers/iio/adc/sun4i-gpadc-iio.c | 48 +++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
> >  2 files changed, 75 insertions(+)  
> [...]
> > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> > index 78d31984a222..5c2a12101052 100644
> > --- a/include/linux/mfd/sun4i-gpadc.h
> > +++ b/include/linux/mfd/sun4i-gpadc.h  
> [...]
> > +#define SUN8I_H3_GPADC_CTRL2_T_ACQ1(x)			((GENMASK(15, 0) * (x)) << 16)
> > +  
> 
> You want to replace * by &.
> 
> ((GENMASK(15, 0) & (x)) << 16)
> 
> Would ((GENMASK(31, 16) & ((x) << 16)) make the bits you set even more
> obvious?

Agreed. Would act as better 'documentation'.

Jonathan
> 
> >  #define SUN4I_GPADC_CTRL3				0x0c
> > +/*
> > + * This register is named "Average filter Control Register" in H3 Datasheet,
> > + * but the register's definition is the same as the old CTRL3 register.
> > + */
> > +#define SUN8I_H3_GPADC_CTRL3				0x70
> >    
> 
> I would name it as it is in the documentation:
> SUN8I_H3_THS_FILTER
> 
> No need for comments then.
> 
> >  #define SUN4I_GPADC_CTRL3_FILTER_EN			BIT(2)
> >  #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)		(GENMASK(1, 0) & (x))
> > @@ -71,6 +84,13 @@
> >  #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN		BIT(1)
> >  #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN		BIT(0)
> >  
> > +#define SUN8I_H3_GPADC_INTC				0x44
> > +
> > +#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
> > +#define SUN8I_H3_GPADC_INTC_TEMP_DATA			BIT(8)
> > +#define SUN8I_H3_GPADC_INTC_TEMP_SHUT			BIT(4)
> > +#define SUN8I_H3_GPADC_INTC_TEMP_ALARM			BIT(0)
> > +  
> 
> Since it isn't an ADC anymore but rather just a THS, why don't you use
> SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
> datasheet.
> 
> >  #define SUN4I_GPADC_INT_FIFOS				0x14
> >  
> >  #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING		BIT(18)
> > @@ -80,9 +100,16 @@
> >  #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING		BIT(1)
> >  #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING		BIT(0)
> >  
> > +#define SUN8I_H3_GPADC_INTS				0x44  
> 
> 0x48
> 
> [...]
> 
> 1) You're not using irqs, why would you define registers that will never
> be used?
> 
> 2) Why aren't you using irqs? I remember we discussed on IRC that you
> had some problems with the H3 when resuming or when probing the driver.
> The register would have a zero in it until you have a first sample that
> arrived (i.e. after the sample rate you set with T_ACQ) that would make
> the thermal framework panic since the thermal sensor would return
> something way too hot and shutdown your board?
> 
> The H3 apparently supports IRQs, why do you not support them for the
> temperature? They might be broken as it is on A33 but then it might be a
> good idea to write it down in a comment in the driver (and not adding
> the unused registers in the header file) or at least in the commit log.
> 
> 3) Now that you have support for clocks, wouldn't it be a good idea to
> disable them during suspend?
> 
> Thanks,
> Quentin

Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone

[Jonathan Cameron]

On Sat, 16 Sep 2017 12:05:49 +0200
Quentin Schulz <quentin.schulz@free-electrons.com> wrote:

> Hi Icenowy,
> 
> On 14/09/2017 16:52, Icenowy Zheng wrote:
> > Because of the restriction of the OF thermal framework, the thermal
> > sensor will fail to probe if the thermal zone doesn't exist.
> >   
> 
> Oh no, that's not good.
> 
> We discussed about it on IRC and I even proposed a patch for it, telling
> you I would post it on the mailing list soon after. Of course, I forgot
> and you definitely should have yelled at me for not doing it :)
> 
> I won't be able to test the patch soon. I can send it to you so that you
> can test it and integrate it in your patch series so it won't block you.
> Otherwise, we'll have to wait for a week or two for me to test it.
> 
> Thanks and sorry for forgetting to post the patch you need,
> Quentin

Other this outstanding issue I'm happy with the series, so hopefully
with Quentin's patch added we should be good to merge this one.

Jonathan

> 
> > Add a partial thermal zone which claims the H3 THS as the thermal sensor.
> > 
> > The cooling device (CPU DVFS) is still not added as it's not ready, and
> > the trip points are also not added yet.
> > 
> > Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > ---
> >  arch/arm/boot/dts/sun8i-h3.dtsi | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> > index 3220da3ad790..687c6457d214 100644
> > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > @@ -89,6 +89,15 @@
> >  		};
> >  	};
> >  
> > +	thermal-zones {
> > +		cpu-thermal {
> > +			/* milliseconds */
> > +			polling-delay-passive = <250>;
> > +			polling-delay = <1000>;
> > +			thermal-sensors = <&ths>;
> > +		};
> > +	};
> > +
> >  	timer {
> >  		compatible = "arm,armv7-timer";
> >  		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
> >   
> 

Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Maxime Ripard]

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

On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> Allwinner H3 features a thermal sensor like the one in A33, but has its
> register re-arranged, the clock divider moved to CCU (originally the
> clock divider is in ADC) and added a pair of bus clock and reset.
> 
> Update the binding document to cover H3.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Add nvmem calibration data (not yet used by the driver)
> Changes in v3:
> - Clock name changes.
> - Example node name changes.
> - Add interupts (not yet used by the driver).
> 
>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30 ++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> index badff3611a98..6c470d584bf9 100644
> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also act as a thermal sensor
>  and sometimes as a touchscreen controller.
>  
>  Required properties:
> -  - compatible: "allwinner,sun8i-a33-ths",
> +  - compatible: must contain one of the following compatibles:
> +		- "allwinner,sun8i-a33-ths"
> +		- "allwinner,sun8i-h3-ths"
>    - reg: mmio address range of the chip,
>    - #thermal-sensor-cells: shall be 0,
>    - #io-channel-cells: shall be 0,
>  
> -Example:
> +Optional properties:
> +  - nvmem-cells: A phandle to the calibration data provided by a nvmem device.
> +                 If unspecified default values shall be used.
> +  - nvmem-cell-names: Should be "calibration-data"

I'd prefer to have which sensor it applies to here. It wouldn't change
anything for the H3, but it definitely does for example for the A83t
that has two sensors, one for each cluster, and one for the GPU, each
with calibration data.

What about cluster0-calibration?

> +
> +Required properties for the following compatibles:
> +		- "allwinner,sun8i-h3-ths"
> +  - clocks: the bus clock and the input clock of the ADC,
> +  - clock-names: should be "bus" and "mod",
> +  - resets: the bus reset of the ADC,
> +  - interrupts: the sampling interrupt of the ADC,

For resets and interrupts, you should list all of them. If there's
only one, then there's no point telling which one it is.


Thanks,
Maxime

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

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

Re: [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33

[Maxime Ripard]

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

On Thu, Sep 14, 2017 at 10:52:47PM +0800, Icenowy Zheng wrote:
> As the H3 SoC, which is also in sun8i line, has totally different
> register map for the thermal sensor (a cut down version of GPADC), we
> should rename A23/A33-specified registers to contain A33, in order to
> prevent obfuscation with H3 registers. Currently these registers are
> only prefixed "SUN8I", not "SUN8I_A33".
> 
> Add "_A33" after "SUN8I" on the register names.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Maxime

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

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

Re: [PATCH v4 3/6] iio: adc: sun4i-gpadc-iio: rework code for supporting newer THS variants

[Maxime Ripard]

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

On Thu, Sep 14, 2017 at 10:52:48PM +0800, Icenowy Zheng wrote:
> The SoCs after H3 has newer thermal sensor ADCs, which have two clock
> inputs (bus clock and sampling clock) and a reset. The registers are
> also re-arranged.
> 
> This commit reworks the code, adds the process of the clocks and
> resets, and allows the sampling start/end code and the position of value
> readout register to be altered.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>

Please split that into separate commits.

Maxime

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

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

Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Icenowy Zheng]

于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
>> Allwinner H3 features a thermal sensor like the one in A33, but has
>its
>> register re-arranged, the clock divider moved to CCU (originally the
>> clock divider is in ADC) and added a pair of bus clock and reset.
>> 
>> Update the binding document to cover H3.
>> 
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>> Changes in v4:
>> - Add nvmem calibration data (not yet used by the driver)
>> Changes in v3:
>> - Clock name changes.
>> - Example node name changes.
>> - Add interupts (not yet used by the driver).
>> 
>>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30
>++++++++++++++++++++--
>>  1 file changed, 28 insertions(+), 2 deletions(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> index badff3611a98..6c470d584bf9 100644
>> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
>act as a thermal sensor
>>  and sometimes as a touchscreen controller.
>>  
>>  Required properties:
>> -  - compatible: "allwinner,sun8i-a33-ths",
>> +  - compatible: must contain one of the following compatibles:
>> +		- "allwinner,sun8i-a33-ths"
>> +		- "allwinner,sun8i-h3-ths"
>>    - reg: mmio address range of the chip,
>>    - #thermal-sensor-cells: shall be 0,
>>    - #io-channel-cells: shall be 0,
>>  
>> -Example:
>> +Optional properties:
>> +  - nvmem-cells: A phandle to the calibration data provided by a
>nvmem device.
>> +                 If unspecified default values shall be used.
>> +  - nvmem-cell-names: Should be "calibration-data"
>
>I'd prefer to have which sensor it applies to here. It wouldn't change
>anything for the H3, but it definitely does for example for the A83t
>that has two sensors, one for each cluster, and one for the GPU, each
>with calibration data.
>
>What about cluster0-calibration?

The calibration data is in fact a 2 word (8 bytes) zone,
which is reserved for 4 sensors on all SoCs, even on H3.
It's half word per sensor.

I prefer to just assume a 2 word cell for every SoC.

>
>> +
>> +Required properties for the following compatibles:
>> +		- "allwinner,sun8i-h3-ths"
>> +  - clocks: the bus clock and the input clock of the ADC,
>> +  - clock-names: should be "bus" and "mod",
>> +  - resets: the bus reset of the ADC,
>> +  - interrupts: the sampling interrupt of the ADC,
>
>For resets and interrupts, you should list all of them. If there's
>only one, then there's no point telling which one it is.
>
>
>Thanks,
>Maxime

Re: [PATCH v4 4/6] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Maxime Ripard]

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

On Sat, Sep 16, 2017 at 06:14:08PM +0800, icenowy@aosc.io wrote:
> > The H3 apparently supports IRQs, why do you not support them for the
> > temperature? They might be broken as it is on A33 but then it might be a
> > good idea to write it down in a comment in the driver (and not adding
> > the unused registers in the header file) or at least in the commit log.
> > 
> > 3) Now that you have support for clocks, wouldn't it be a good idea to
> > disable them during suspend?
> 
> Interesting... It's meaningful to disable the mod clock during suspend.

All clocks, actually. And put the device back into reset.

Maxime

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

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

Re: [PATCH v4 5/6] ARM: sun8i: h3: add support for the thermal sensor in H3

[Maxime Ripard]

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

On Thu, Sep 14, 2017 at 10:52:50PM +0800, Icenowy Zheng wrote:
> As we have gained the support for the thermal sensor in H3, we can now
> add its device nodes to the device tree.
> 
> Add them to the H3 device tree.
> 
> The calibration data of the thermal sensor is still not added, as
> it's currently not used, and the SID node is not added yet.
> 
> The H5 thermal sensor has some differences, and will be added furtherly.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Mention calibration data in commit message.
> Changes in v3:
> - Clock name changes.
> - Splited out thermal zone addition.
> 
>  arch/arm/boot/dts/sun8i-h3.dtsi | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index b36f9f423c39..3220da3ad790 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -72,6 +72,23 @@
>  		};
>  	};
>  
> +	iio-hwmon {
> +		compatible = "iio-hwmon";
> +		io-channels = <&ths>;
> +	};
> +
> +	soc {
> +		ths: thermal-sensor@1c25000 {
> +			compatible = "allwinner,sun8i-h3-ths";
> +			reg = <0x01c25000 0x100>;
> +			clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
> +			clock-names = "bus", "mod";
> +			resets = <&ccu RST_BUS_THS>;
> +			#thermal-sensor-cells = <0>;
> +			#io-channel-cells = <0>;

You're missing your interrupt.

Maxime

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

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

Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone

[Maxime Ripard]

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

Hi Jonathan,

On Sat, Sep 16, 2017 at 03:17:34PM -0700, Jonathan Cameron wrote:
> On Sat, 16 Sep 2017 12:05:49 +0200
> Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> 
> > Hi Icenowy,
> > 
> > On 14/09/2017 16:52, Icenowy Zheng wrote:
> > > Because of the restriction of the OF thermal framework, the thermal
> > > sensor will fail to probe if the thermal zone doesn't exist.
> > >   
> > 
> > Oh no, that's not good.
> > 
> > We discussed about it on IRC and I even proposed a patch for it, telling
> > you I would post it on the mailing list soon after. Of course, I forgot
> > and you definitely should have yelled at me for not doing it :)
> > 
> > I won't be able to test the patch soon. I can send it to you so that you
> > can test it and integrate it in your patch series so it won't block you.
> > Otherwise, we'll have to wait for a week or two for me to test it.
> > 
> > Thanks and sorry for forgetting to post the patch you need,
> > Quentin
> 
> Other this outstanding issue I'm happy with the series, so hopefully
> with Quentin's patch added we should be good to merge this one.

We will at least need a v5.

Maxime

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

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

Re: [PATCH v4 2/6] iio: adc: sun4i-gpadc-iio: rename A33-specified registers to contain A33

[Lee Jones]

On Thu, 14 Sep 2017, Icenowy Zheng wrote:

> As the H3 SoC, which is also in sun8i line, has totally different
> register map for the thermal sensor (a cut down version of GPADC), we
> should rename A23/A33-specified registers to contain A33, in order to
> prevent obfuscation with H3 registers. Currently these registers are
> only prefixed "SUN8I", not "SUN8I_A33".
> 
> Add "_A33" after "SUN8I" on the register names.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
> Changes in v4:
> - Change A23 to A33, as the driver never supports A23.
> 
>  drivers/iio/adc/sun4i-gpadc-iio.c | 2 +-

>  include/linux/mfd/sun4i-gpadc.h   | 6 +++---

Acked-by: Lee Jones <lee.jones@linaro.org>

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

Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Maxime Ripard]

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

On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
> 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> >> Allwinner H3 features a thermal sensor like the one in A33, but has
> >its
> >> register re-arranged, the clock divider moved to CCU (originally the
> >> clock divider is in ADC) and added a pair of bus clock and reset.
> >> 
> >> Update the binding document to cover H3.
> >> 
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> >> ---
> >> Changes in v4:
> >> - Add nvmem calibration data (not yet used by the driver)
> >> Changes in v3:
> >> - Clock name changes.
> >> - Example node name changes.
> >> - Add interupts (not yet used by the driver).
> >> 
> >>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30
> >++++++++++++++++++++--
> >>  1 file changed, 28 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> index badff3611a98..6c470d584bf9 100644
> >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
> >act as a thermal sensor
> >>  and sometimes as a touchscreen controller.
> >>  
> >>  Required properties:
> >> -  - compatible: "allwinner,sun8i-a33-ths",
> >> +  - compatible: must contain one of the following compatibles:
> >> +		- "allwinner,sun8i-a33-ths"
> >> +		- "allwinner,sun8i-h3-ths"
> >>    - reg: mmio address range of the chip,
> >>    - #thermal-sensor-cells: shall be 0,
> >>    - #io-channel-cells: shall be 0,
> >>  
> >> -Example:
> >> +Optional properties:
> >> +  - nvmem-cells: A phandle to the calibration data provided by a
> >nvmem device.
> >> +                 If unspecified default values shall be used.
> >> +  - nvmem-cell-names: Should be "calibration-data"
> >
> >I'd prefer to have which sensor it applies to here. It wouldn't change
> >anything for the H3, but it definitely does for example for the A83t
> >that has two sensors, one for each cluster, and one for the GPU, each
> >with calibration data.
> >
> >What about cluster0-calibration?
> 
> The calibration data is in fact a 2 word (8 bytes) zone,
> which is reserved for 4 sensors on all SoCs, even on H3.
> It's half word per sensor.
> 
> I prefer to just assume a 2 word cell for every SoC.

You have three different data sources, it should be reprensented as
such.

Otherwise, the client has to get some knowledge of how the data are
stored in the provider, which is an abstraction violation.

Maxime

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

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

Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[icenowy]

在 2017-09-18 16:30，Maxime Ripard 写道：
> On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
>> 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard 
>> <maxime.ripard@free-electrons.com> 写到:
>> >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
>> >> Allwinner H3 features a thermal sensor like the one in A33, but has
>> >its
>> >> register re-arranged, the clock divider moved to CCU (originally the
>> >> clock divider is in ADC) and added a pair of bus clock and reset.
>> >>
>> >> Update the binding document to cover H3.
>> >>
>> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>> >> ---
>> >> Changes in v4:
>> >> - Add nvmem calibration data (not yet used by the driver)
>> >> Changes in v3:
>> >> - Clock name changes.
>> >> - Example node name changes.
>> >> - Add interupts (not yet used by the driver).
>> >>
>> >>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30
>> >++++++++++++++++++++--
>> >>  1 file changed, 28 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >> index badff3611a98..6c470d584bf9 100644
>> >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
>> >act as a thermal sensor
>> >>  and sometimes as a touchscreen controller.
>> >>
>> >>  Required properties:
>> >> -  - compatible: "allwinner,sun8i-a33-ths",
>> >> +  - compatible: must contain one of the following compatibles:
>> >> +		- "allwinner,sun8i-a33-ths"
>> >> +		- "allwinner,sun8i-h3-ths"
>> >>    - reg: mmio address range of the chip,
>> >>    - #thermal-sensor-cells: shall be 0,
>> >>    - #io-channel-cells: shall be 0,
>> >>
>> >> -Example:
>> >> +Optional properties:
>> >> +  - nvmem-cells: A phandle to the calibration data provided by a
>> >nvmem device.
>> >> +                 If unspecified default values shall be used.
>> >> +  - nvmem-cell-names: Should be "calibration-data"
>> >
>> >I'd prefer to have which sensor it applies to here. It wouldn't change
>> >anything for the H3, but it definitely does for example for the A83t
>> >that has two sensors, one for each cluster, and one for the GPU, each
>> >with calibration data.
>> >
>> >What about cluster0-calibration?

I prefer sensor0-calibration to sensor3-calibration now.
(Theortically the new generation THS can support up to 4 sensors)

>> 
>> The calibration data is in fact a 2 word (8 bytes) zone,
>> which is reserved for 4 sensors on all SoCs, even on H3.
>> It's half word per sensor.
>> 
>> I prefer to just assume a 2 word cell for every SoC.
> 
> You have three different data sources, it should be reprensented as
> such.
> 
> Otherwise, the client has to get some knowledge of how the data are
> stored in the provider, which is an abstraction violation.
> 
> Maxime
> 
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Maxime Ripard]

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

On Mon, Sep 18, 2017 at 03:47:25PM +0000, icenowy@aosc.io wrote:
> 在 2017-09-18 16:30，Maxime Ripard 写道：
> > On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
> > > 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
> > > <maxime.ripard@free-electrons.com> 写到:
> > > >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> > > >> Allwinner H3 features a thermal sensor like the one in A33, but has
> > > >its
> > > >> register re-arranged, the clock divider moved to CCU (originally the
> > > >> clock divider is in ADC) and added a pair of bus clock and reset.
> > > >>
> > > >> Update the binding document to cover H3.
> > > >>
> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> > > >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> > > >> ---
> > > >> Changes in v4:
> > > >> - Add nvmem calibration data (not yet used by the driver)
> > > >> Changes in v3:
> > > >> - Clock name changes.
> > > >> - Example node name changes.
> > > >> - Add interupts (not yet used by the driver).
> > > >>
> > > >>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30
> > > >++++++++++++++++++++--
> > > >>  1 file changed, 28 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >> index badff3611a98..6c470d584bf9 100644
> > > >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> > > >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can also
> > > >act as a thermal sensor
> > > >>  and sometimes as a touchscreen controller.
> > > >>
> > > >>  Required properties:
> > > >> -  - compatible: "allwinner,sun8i-a33-ths",
> > > >> +  - compatible: must contain one of the following compatibles:
> > > >> +		- "allwinner,sun8i-a33-ths"
> > > >> +		- "allwinner,sun8i-h3-ths"
> > > >>    - reg: mmio address range of the chip,
> > > >>    - #thermal-sensor-cells: shall be 0,
> > > >>    - #io-channel-cells: shall be 0,
> > > >>
> > > >> -Example:
> > > >> +Optional properties:
> > > >> +  - nvmem-cells: A phandle to the calibration data provided by a
> > > >nvmem device.
> > > >> +                 If unspecified default values shall be used.
> > > >> +  - nvmem-cell-names: Should be "calibration-data"
> > > >
> > > >I'd prefer to have which sensor it applies to here. It wouldn't change
> > > >anything for the H3, but it definitely does for example for the A83t
> > > >that has two sensors, one for each cluster, and one for the GPU, each
> > > >with calibration data.
> > > >
> > > >What about cluster0-calibration?
> 
> I prefer sensor0-calibration to sensor3-calibration now.
> (Theortically the new generation THS can support up to 4 sensors)

The mapping that explains what sensor0 means can change in the
future. It's better to be explicit here, and just say upfront what
it's about.

Maxime

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

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

Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Icenowy Zheng]

于 2017年9月20日 GMT+08:00 下午3:52:23, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
>On Mon, Sep 18, 2017 at 03:47:25PM +0000, icenowy@aosc.io wrote:
>> 在 2017-09-18 16:30，Maxime Ripard 写道：
>> > On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
>> > > 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
>> > > <maxime.ripard@free-electrons.com> 写到:
>> > > >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
>> > > >> Allwinner H3 features a thermal sensor like the one in A33,
>but has
>> > > >its
>> > > >> register re-arranged, the clock divider moved to CCU
>(originally the
>> > > >> clock divider is in ADC) and added a pair of bus clock and
>reset.
>> > > >>
>> > > >> Update the binding document to cover H3.
>> > > >>
>> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
>> > > >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>> > > >> ---
>> > > >> Changes in v4:
>> > > >> - Add nvmem calibration data (not yet used by the driver)
>> > > >> Changes in v3:
>> > > >> - Clock name changes.
>> > > >> - Example node name changes.
>> > > >> - Add interupts (not yet used by the driver).
>> > > >>
>> > > >>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30
>> > > >++++++++++++++++++++--
>> > > >>  1 file changed, 28 insertions(+), 2 deletions(-)
>> > > >>
>> > > >> diff --git
>a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >> index badff3611a98..6c470d584bf9 100644
>> > > >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
>> > > >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can
>also
>> > > >act as a thermal sensor
>> > > >>  and sometimes as a touchscreen controller.
>> > > >>
>> > > >>  Required properties:
>> > > >> -  - compatible: "allwinner,sun8i-a33-ths",
>> > > >> +  - compatible: must contain one of the following
>compatibles:
>> > > >> +		- "allwinner,sun8i-a33-ths"
>> > > >> +		- "allwinner,sun8i-h3-ths"
>> > > >>    - reg: mmio address range of the chip,
>> > > >>    - #thermal-sensor-cells: shall be 0,
>> > > >>    - #io-channel-cells: shall be 0,
>> > > >>
>> > > >> -Example:
>> > > >> +Optional properties:
>> > > >> +  - nvmem-cells: A phandle to the calibration data provided
>by a
>> > > >nvmem device.
>> > > >> +                 If unspecified default values shall be used.
>> > > >> +  - nvmem-cell-names: Should be "calibration-data"
>> > > >
>> > > >I'd prefer to have which sensor it applies to here. It wouldn't
>change
>> > > >anything for the H3, but it definitely does for example for the
>A83t
>> > > >that has two sensors, one for each cluster, and one for the GPU,
>each
>> > > >with calibration data.
>> > > >
>> > > >What about cluster0-calibration?
>> 
>> I prefer sensor0-calibration to sensor3-calibration now.
>> (Theortically the new generation THS can support up to 4 sensors)
>
>The mapping that explains what sensor0 means can change in the
>future. It's better to be explicit here, and just say upfront what
>it's about.

I think for some SoC (e.g. A64) there's no clear explain on
the functions of the sensors.

In addition, in the THS controller the sensors has a
explicit sequence, and when referencing it in the DT
the number is still needed (in thermal zones).

>
>Maxime

Re: [linux-sunxi] Re: [PATCH v4 1/6] dt-bindings: update the Allwinner GPADC device tree binding for H3

[Maxime Ripard]

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

On Wed, Sep 20, 2017 at 08:04:02AM +0000, Icenowy Zheng wrote:
> 于 2017年9月20日 GMT+08:00 下午3:52:23, Maxime Ripard <maxime.ripard@free-electrons.com> 写到:
> >On Mon, Sep 18, 2017 at 03:47:25PM +0000, icenowy@aosc.io wrote:
> >> 在 2017-09-18 16:30，Maxime Ripard 写道：
> >> > On Mon, Sep 18, 2017 at 03:36:43PM +0800, Icenowy Zheng wrote:
> >> > > 于 2017年9月18日 GMT+08:00 下午3:33:36, Maxime Ripard
> >> > > <maxime.ripard@free-electrons.com> 写到:
> >> > > >On Thu, Sep 14, 2017 at 10:52:46PM +0800, Icenowy Zheng wrote:
> >> > > >> Allwinner H3 features a thermal sensor like the one in A33,
> >but has
> >> > > >its
> >> > > >> register re-arranged, the clock divider moved to CCU
> >(originally the
> >> > > >> clock divider is in ADC) and added a pair of bus clock and
> >reset.
> >> > > >>
> >> > > >> Update the binding document to cover H3.
> >> > > >>
> >> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io>
> >> > > >> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> >> > > >> ---
> >> > > >> Changes in v4:
> >> > > >> - Add nvmem calibration data (not yet used by the driver)
> >> > > >> Changes in v3:
> >> > > >> - Clock name changes.
> >> > > >> - Example node name changes.
> >> > > >> - Add interupts (not yet used by the driver).
> >> > > >>
> >> > > >>  .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 30
> >> > > >++++++++++++++++++++--
> >> > > >>  1 file changed, 28 insertions(+), 2 deletions(-)
> >> > > >>
> >> > > >> diff --git
> >a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >> index badff3611a98..6c470d584bf9 100644
> >> > > >> --- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >> +++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
> >> > > >> @@ -4,12 +4,26 @@ The Allwinner SoCs all have an ADC that can
> >also
> >> > > >act as a thermal sensor
> >> > > >>  and sometimes as a touchscreen controller.
> >> > > >>
> >> > > >>  Required properties:
> >> > > >> -  - compatible: "allwinner,sun8i-a33-ths",
> >> > > >> +  - compatible: must contain one of the following
> >compatibles:
> >> > > >> +		- "allwinner,sun8i-a33-ths"
> >> > > >> +		- "allwinner,sun8i-h3-ths"
> >> > > >>    - reg: mmio address range of the chip,
> >> > > >>    - #thermal-sensor-cells: shall be 0,
> >> > > >>    - #io-channel-cells: shall be 0,
> >> > > >>
> >> > > >> -Example:
> >> > > >> +Optional properties:
> >> > > >> +  - nvmem-cells: A phandle to the calibration data provided
> >by a
> >> > > >nvmem device.
> >> > > >> +                 If unspecified default values shall be used.
> >> > > >> +  - nvmem-cell-names: Should be "calibration-data"
> >> > > >
> >> > > >I'd prefer to have which sensor it applies to here. It wouldn't
> >change
> >> > > >anything for the H3, but it definitely does for example for the
> >A83t
> >> > > >that has two sensors, one for each cluster, and one for the GPU,
> >each
> >> > > >with calibration data.
> >> > > >
> >> > > >What about cluster0-calibration?
> >> 
> >> I prefer sensor0-calibration to sensor3-calibration now.
> >> (Theortically the new generation THS can support up to 4 sensors)
> >
> >The mapping that explains what sensor0 means can change in the
> >future. It's better to be explicit here, and just say upfront what
> >it's about.
> 
> I think for some SoC (e.g. A64) there's no clear explain on
> the functions of the sensors.

It's documented in the user manual ("sensor0 located in the CPU, sensor1 and
sensor2 located in the GPU"

> In addition, in the THS controller the sensors has a explicit
> sequence, and when referencing it in the DT the number is still
> needed (in thermal zones).

Yes, but that's something that can be made easier through defines too.

Maxime

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

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

Re: [PATCH v4 6/6] ARM: sun8i: h3: add partial CPU thermal zone

[Jonathan Cameron]

On Mon, 18 Sep 2017 10:27:03 +0200
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Hi Jonathan,
> 
> On Sat, Sep 16, 2017 at 03:17:34PM -0700, Jonathan Cameron wrote:
> > On Sat, 16 Sep 2017 12:05:49 +0200
> > Quentin Schulz <quentin.schulz@free-electrons.com> wrote:
> >   
> > > Hi Icenowy,
> > > 
> > > On 14/09/2017 16:52, Icenowy Zheng wrote:  
> > > > Because of the restriction of the OF thermal framework, the thermal
> > > > sensor will fail to probe if the thermal zone doesn't exist.
> > > >     
> > > 
> > > Oh no, that's not good.
> > > 
> > > We discussed about it on IRC and I even proposed a patch for it, telling
> > > you I would post it on the mailing list soon after. Of course, I forgot
> > > and you definitely should have yelled at me for not doing it :)
> > > 
> > > I won't be able to test the patch soon. I can send it to you so that you
> > > can test it and integrate it in your patch series so it won't block you.
> > > Otherwise, we'll have to wait for a week or two for me to test it.
> > > 
> > > Thanks and sorry for forgetting to post the patch you need,
> > > Quentin  
> > 
> > Other this outstanding issue I'm happy with the series, so hopefully
> > with Quentin's patch added we should be good to merge this one.  
> 
> We will at least need a v5.
> 
> Maxime
> 
Sure - I can see other issues are coming out of the woodwork!

Thanks,

Jonathan