[RFC PATCH 0/3] IIO-based thermal sensor driver for Allwinner H3 SoC

[RFC PATCH 0/3] 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 patchset by Quentin Schulz at [1].

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/495505.html

Icenowy Zheng (3):
  dt-bindings: update the Allwinner GPADC device tree binding for H3
  iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
  ARM: sun8i: h3: add support for the thermal sensor in H3

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

-- 
2.12.0

[RFC PATCH 1/3] 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>
---
 .../devicetree/bindings/mfd/sun4i-gpadc.txt        | 23 ++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
index badff3611a98..7753133ca0ff 100644
--- a/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
+++ b/Documentation/devicetree/bindings/mfd/sun4i-gpadc.txt
@@ -4,12 +4,20 @@ 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:
+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 "ths",
+  - resets: the bus reset of the ADC,
+
+Example for A33:
 	ths: ths at 01c25000 {
 		compatible = "allwinner,sun8i-a33-ths";
 		reg = <0x01c25000 0x100>;
@@ -17,6 +25,17 @@ Example:
 		#io-channel-cells = <0>;
 	};
 
+Example for H3:
+	ths: ths at 01c25000 {
+		compatible = "allwinner,sun8i-h3-ths";
+		reg = <0x01c25000 0x100>;
+		clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
+		clock-names = "bus", "ths";
+		resets = <&ccu RST_BUS_THS>;
+		#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.12.0

[RFC PATCH 2/3] 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.

An option is added to gpadc_data struct, to indicate whether this device
is a new-generation Allwinner thermal sensor.

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>
---
 drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------
 include/linux/mfd/sun4i-gpadc.h   |  33 +++++++++-
 2 files changed, 141 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
index 74705aa37982..7512b1cae877 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>
 
@@ -56,6 +58,7 @@ struct gpadc_data {
 	unsigned int	tp_adc_select;
 	unsigned int	(*adc_chan_select)(unsigned int chan);
 	unsigned int	adc_chan_mask;
+	bool		gen2_ths;
 };
 
 static const struct gpadc_data sun4i_gpadc_data = {
@@ -88,7 +91,20 @@ 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_A23_GPADC_CTRL1_CHOP_TEMP_EN,
+};
+
+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,
+	.gen2_ths = true,
 };
 
 struct sun4i_gpadc_iio {
@@ -103,6 +119,9 @@ struct sun4i_gpadc_iio {
 	atomic_t			ignore_temp_data_irq;
 	const struct gpadc_data		*data;
 	bool				no_irq;
+	struct clk			*ths_bus_clk;
+	struct clk			*ths_clk;
+	struct reset_control		*reset;
 	/* prevents concurrent reads of temperature and ADC */
 	struct mutex			mutex;
 };
@@ -274,7 +293,11 @@ 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);
+		if (info->data->gen2_ths)
+			regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA,
+				    val);
+		else
+			regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
 
 		pm_runtime_mark_last_busy(indio_dev->dev.parent);
 		pm_runtime_put_autosuspend(indio_dev->dev.parent);
@@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
 {
 	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 */
-	regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
+	if (info->data->gen2_ths) {
+		/* Disable temperature sensor */
+		regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
+	} else {
+		/* Disable the ADC on IP */
+		regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
+		/* Disable temperature sensor on IP */
+		regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
+	}
 
 	return 0;
 }
@@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
 {
 	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
 
-	/* clkin = 6MHz */
-	regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
-		     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
-		     SUN4I_GPADC_CTRL0_FS_DIV(7) |
-		     SUN4I_GPADC_CTRL0_T_ACQ(63));
-	regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en);
-	regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
-		     SUN4I_GPADC_CTRL3_FILTER_EN |
-		     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
-	/* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */
-	regmap_write(info->regmap, SUN4I_GPADC_TPR,
-		     SUN4I_GPADC_TPR_TEMP_ENABLE |
-		     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
+	if (info->data->gen2_ths) {
+		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));
+	} else {
+		/* clkin = 6MHz */
+		regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
+			     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
+			     SUN4I_GPADC_CTRL0_FS_DIV(7) |
+			     SUN4I_GPADC_CTRL0_T_ACQ(63));
+		regmap_write(info->regmap, SUN4I_GPADC_CTRL1,
+			     info->data->tp_mode_en);
+		regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
+			     SUN4I_GPADC_CTRL3_FILTER_EN |
+			     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
+		/*
+		 * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin;
+		 * ~0.6s
+		 */
+		regmap_write(info->regmap, SUN4I_GPADC_TPR,
+			     SUN4I_GPADC_TPR_TEMP_ENABLE |
+			     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
+	}
 
 	return 0;
 }
@@ -494,6 +539,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 */ }
 };
 
@@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
 		return ret;
 	}
 
+	if (info->data->gen2_ths) {
+		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;
+
+		info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus");
+		if (IS_ERR(info->ths_bus_clk)) {
+			ret = PTR_ERR(info->ths_bus_clk);
+			return ret;
+		}
+
+		ret = clk_prepare_enable(info->ths_bus_clk);
+		if (ret)
+			return ret;
+
+		info->ths_clk = devm_clk_get(&pdev->dev, "ths");
+		if (IS_ERR(info->ths_clk)) {
+			ret = PTR_ERR(info->ths_clk);
+			return ret;
+		}
+
+		/* Running at 6MHz */
+		ret = clk_set_rate(info->ths_clk, 6000000);
+		if (ret)
+			return ret;
+
+		ret = clk_prepare_enable(info->ths_clk);
+		if (ret)
+			return ret;
+	}
+
 	if (!IS_ENABLED(CONFIG_THERMAL_OF))
 		return 0;
 
@@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
 	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
 		iio_map_array_unregister(indio_dev);
 
+	if (info->data->gen2_ths) {
+		clk_disable_unprepare(info->ths_clk);
+		clk_disable_unprepare(info->ths_bus_clk);
+		reset_control_deassert(info->reset);
+	}
+
 	return 0;
 }
 
diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
index 139872c2e0fe..f794a2988a93 100644
--- a/include/linux/mfd/sun4i-gpadc.h
+++ b/include/linux/mfd/sun4i-gpadc.h
@@ -38,9 +38,12 @@
 #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 sun8i A23/A33 SoCs */
+#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN		BIT(8)
+#define SUN8I_A23_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
 
@@ -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.12.0

[RFC PATCH 3/3] 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 H5 thermal sensor has some differences, and will be added furtherly.

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

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index b36f9f423c39..552217bb9266 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -72,6 +72,32 @@
 		};
 	};
 
+	iio-hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&ths>;
+	};
+
+	soc {
+		ths: ths at 01c25000 {
+			compatible = "allwinner,sun8i-h3-ths";
+			reg = <0x01c25000 0x100>;
+			clocks = <&ccu CLK_BUS_THS>, <&ccu CLK_THS>;
+			clock-names = "bus", "ths";
+			resets = <&ccu RST_BUS_THS>;
+			#thermal-sensor-cells = <0>;
+			#io-channel-cells = <0>;
+		};
+	};
+
+	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.12.0

[RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Quentin Schulz]

Hi,

On 28/03/2017 19:30, 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.
> 
> An option is added to gpadc_data struct, to indicate whether this device
> is a new-generation Allwinner thermal sensor.
> 
> 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>
> ---
>  drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------
>  include/linux/mfd/sun4i-gpadc.h   |  33 +++++++++-
>  2 files changed, 141 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
> index 74705aa37982..7512b1cae877 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>
>  
> @@ -56,6 +58,7 @@ struct gpadc_data {
>  	unsigned int	tp_adc_select;
>  	unsigned int	(*adc_chan_select)(unsigned int chan);
>  	unsigned int	adc_chan_mask;
> +	bool		gen2_ths;
>  };
>  

Instead of a boolean, give the TEMP_DATA register address.

>  static const struct gpadc_data sun4i_gpadc_data = {
> @@ -88,7 +91,20 @@ 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_A23_GPADC_CTRL1_CHOP_TEMP_EN,
> +};

Separate patch for this?

> +
> +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,
> +	.gen2_ths = true,
>  };
>  
>  struct sun4i_gpadc_iio {
> @@ -103,6 +119,9 @@ struct sun4i_gpadc_iio {
>  	atomic_t			ignore_temp_data_irq;
>  	const struct gpadc_data		*data;
>  	bool				no_irq;
> +	struct clk			*ths_bus_clk;
> +	struct clk			*ths_clk;
> +	struct reset_control		*reset;
>  	/* prevents concurrent reads of temperature and ADC */
>  	struct mutex			mutex;
>  };
> @@ -274,7 +293,11 @@ 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);
> +		if (info->data->gen2_ths)
> +			regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA,
> +				    val);
> +		else
> +			regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>  

Instead of gen2_ths, use the TEMP_DATA register address.

>  		pm_runtime_mark_last_busy(indio_dev->dev.parent);
>  		pm_runtime_put_autosuspend(indio_dev->dev.parent);
> @@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
>  {
>  	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 */
> -	regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
> +	if (info->data->gen2_ths) {
> +		/* Disable temperature sensor */
> +		regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
> +	} else {
> +		/* Disable the ADC on IP */
> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
> +		/* Disable temperature sensor on IP */
> +		regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
> +	}
>  

Either use another register address or add a suspend function to struct
gpadc_data which will be different for each version of the IP.

>  	return 0;
>  }
> @@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
>  {
>  	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
>  
> -	/* clkin = 6MHz */
> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
> -		     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
> -		     SUN4I_GPADC_CTRL0_FS_DIV(7) |
> -		     SUN4I_GPADC_CTRL0_T_ACQ(63));
> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en);
> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
> -		     SUN4I_GPADC_CTRL3_FILTER_EN |
> -		     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
> -	/* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */
> -	regmap_write(info->regmap, SUN4I_GPADC_TPR,
> -		     SUN4I_GPADC_TPR_TEMP_ENABLE |
> -		     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
> +	if (info->data->gen2_ths) {
> +		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));
> +	} else {
> +		/* clkin = 6MHz */
> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
> +			     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
> +			     SUN4I_GPADC_CTRL0_FS_DIV(7) |
> +			     SUN4I_GPADC_CTRL0_T_ACQ(63));
> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL1,
> +			     info->data->tp_mode_en);
> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
> +			     SUN4I_GPADC_CTRL3_FILTER_EN |
> +			     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
> +		/*
> +		 * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin;
> +		 * ~0.6s
> +		 */
> +		regmap_write(info->regmap, SUN4I_GPADC_TPR,
> +			     SUN4I_GPADC_TPR_TEMP_ENABLE |
> +			     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
> +	}
>  

Same here as suspend function?

>  	return 0;
>  }
> @@ -494,6 +539,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 */ }
>  };
>  
> @@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>  		return ret;
>  	}
>  
> +	if (info->data->gen2_ths) {
> +		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;
> +
> +		info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus");
> +		if (IS_ERR(info->ths_bus_clk)) {
> +			ret = PTR_ERR(info->ths_bus_clk);
> +			return ret;
> +		}
> +
> +		ret = clk_prepare_enable(info->ths_bus_clk);
> +		if (ret)
> +			return ret;
> +
> +		info->ths_clk = devm_clk_get(&pdev->dev, "ths");
> +		if (IS_ERR(info->ths_clk)) {
> +			ret = PTR_ERR(info->ths_clk);
> +			return ret;
> +		}
> +
> +		/* Running at 6MHz */
> +		ret = clk_set_rate(info->ths_clk, 6000000);
> +		if (ret)
> +			return ret;
> +
> +		ret = clk_prepare_enable(info->ths_clk);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (!IS_ENABLED(CONFIG_THERMAL_OF))
>  		return 0;
>  
> @@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
>  	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>  		iio_map_array_unregister(indio_dev);
>  
> +	if (info->data->gen2_ths) {
> +		clk_disable_unprepare(info->ths_clk);
> +		clk_disable_unprepare(info->ths_bus_clk);
> +		reset_control_deassert(info->reset);
> +	}
> +

I'm not really fond of using this boolean as I don't see it being
possibly reused for any other SoCs that has a GPADC or THS.

Here, you could make use of a list/array of clk which then can be reused
for other SoCs just by changing the list. Add a default rate to the
gpadc_data structure and you're go to go.

>  	return 0;
>  }
>  
> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
> index 139872c2e0fe..f794a2988a93 100644
> --- a/include/linux/mfd/sun4i-gpadc.h
> +++ b/include/linux/mfd/sun4i-gpadc.h
> @@ -38,9 +38,12 @@
>  #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 sun8i A23/A33 SoCs */
> +#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN		BIT(8)
> +#define SUN8I_A23_GPADC_CTRL1_GPADC_CALI_EN		BIT(7)
> +

Different patch for these?

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

[RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Jonathan Cameron]

On 29/03/17 07:54, Quentin Schulz wrote:
> Hi,
> 
> On 28/03/2017 19:30, 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.
>>
>> An option is added to gpadc_data struct, to indicate whether this device
>> is a new-generation Allwinner thermal sensor.
>>
>> 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>
>> ---
>>  drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------
>>  include/linux/mfd/sun4i-gpadc.h   |  33 +++++++++-
>>  2 files changed, 141 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
>> index 74705aa37982..7512b1cae877 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>
>>  
>> @@ -56,6 +58,7 @@ struct gpadc_data {
>>  	unsigned int	tp_adc_select;
>>  	unsigned int	(*adc_chan_select)(unsigned int chan);
>>  	unsigned int	adc_chan_mask;
>> +	bool		gen2_ths;
>>  };
>>  
> 
> Instead of a boolean, give the TEMP_DATA register address.
> 
>>  static const struct gpadc_data sun4i_gpadc_data = {
>> @@ -88,7 +91,20 @@ 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_A23_GPADC_CTRL1_CHOP_TEMP_EN,
>> +};
> 
> Separate patch for this?
Indeed, ideal to rework original first (really easy to review)
then add support for new part.
> 
>> +
>> +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,
>> +	.gen2_ths = true,
>>  };
>>  
>>  struct sun4i_gpadc_iio {
>> @@ -103,6 +119,9 @@ struct sun4i_gpadc_iio {
>>  	atomic_t			ignore_temp_data_irq;
>>  	const struct gpadc_data		*data;
>>  	bool				no_irq;
>> +	struct clk			*ths_bus_clk;
>> +	struct clk			*ths_clk;
>> +	struct reset_control		*reset;
>>  	/* prevents concurrent reads of temperature and ADC */
>>  	struct mutex			mutex;
>>  };
>> @@ -274,7 +293,11 @@ 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);
>> +		if (info->data->gen2_ths)
>> +			regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA,
>> +				    val);
>> +		else
>> +			regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>>  
> 
> Instead of gen2_ths, use the TEMP_DATA register address.
Agreed. Put a set of register addresses into the gpadc_data structure.
Both more flexible and easier to read.
> 
>>  		pm_runtime_mark_last_busy(indio_dev->dev.parent);
>>  		pm_runtime_put_autosuspend(indio_dev->dev.parent);
>> @@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
>>  {
>>  	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 */
>> -	regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
>> +	if (info->data->gen2_ths) {
>> +		/* Disable temperature sensor */
>> +		regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
>> +	} else {
>> +		/* Disable the ADC on IP */
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
>> +		/* Disable temperature sensor on IP */
>> +		regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
>> +	}
>>  
> 
> Either use another register address or add a suspend function to struct
> gpadc_data which will be different for each version of the IP.
> 
>>  	return 0;
>>  }
>> @@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
>>  {
>>  	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
>>  
>> -	/* clkin = 6MHz */
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
>> -		     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
>> -		     SUN4I_GPADC_CTRL0_FS_DIV(7) |
>> -		     SUN4I_GPADC_CTRL0_T_ACQ(63));
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en);
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
>> -		     SUN4I_GPADC_CTRL3_FILTER_EN |
>> -		     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
>> -	/* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */
>> -	regmap_write(info->regmap, SUN4I_GPADC_TPR,
>> -		     SUN4I_GPADC_TPR_TEMP_ENABLE |
>> -		     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
>> +	if (info->data->gen2_ths) {
>> +		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));
>> +	} else {
>> +		/* clkin = 6MHz */
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
>> +			     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
>> +			     SUN4I_GPADC_CTRL0_FS_DIV(7) |
>> +			     SUN4I_GPADC_CTRL0_T_ACQ(63));
Looks like you'll need something like a function pointer in your
_data structure as well to cover cases where they are totally
different like here.  or you could use regmap fields if they map
reasonably well... (doesn't look like they do though!)
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL1,
>> +			     info->data->tp_mode_en);
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
>> +			     SUN4I_GPADC_CTRL3_FILTER_EN |
>> +			     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
>> +		/*
>> +		 * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin;
>> +		 * ~0.6s
>> +		 */
>> +		regmap_write(info->regmap, SUN4I_GPADC_TPR,
>> +			     SUN4I_GPADC_TPR_TEMP_ENABLE |
>> +			     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
>> +	}
>>  
> 
> Same here as suspend function?
> 
>>  	return 0;
>>  }
>> @@ -494,6 +539,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 */ }
>>  };
>>  
>> @@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>>  		return ret;
>>  	}
>>  
>> +	if (info->data->gen2_ths) {
>> +		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;
>> +
>> +		info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus");
>> +		if (IS_ERR(info->ths_bus_clk)) {
>> +			ret = PTR_ERR(info->ths_bus_clk);
>> +			return ret;
>> +		}
>> +
>> +		ret = clk_prepare_enable(info->ths_bus_clk);
>> +		if (ret)
>> +			return ret;
>> +
>> +		info->ths_clk = devm_clk_get(&pdev->dev, "ths");
>> +		if (IS_ERR(info->ths_clk)) {
>> +			ret = PTR_ERR(info->ths_clk);
>> +			return ret;
>> +		}
>> +
>> +		/* Running at 6MHz */
>> +		ret = clk_set_rate(info->ths_clk, 6000000);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = clk_prepare_enable(info->ths_clk);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	if (!IS_ENABLED(CONFIG_THERMAL_OF))
>>  		return 0;
>>  
>> @@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
>>  	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>  		iio_map_array_unregister(indio_dev);
>>  
>> +	if (info->data->gen2_ths) {
>> +		clk_disable_unprepare(info->ths_clk);
>> +		clk_disable_unprepare(info->ths_bus_clk);
>> +		reset_control_deassert(info->reset);
>> +	}
>> +
> 
> I'm not really fond of using this boolean as I don't see it being
> possibly reused for any other SoCs that has a GPADC or THS.
> 
> Here, you could make use of a list/array of clk which then can be reused
> for other SoCs just by changing the list. Add a default rate to the
> gpadc_data structure and you're go to go.
> 
>>  	return 0;
>>  }
>>  
>> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
>> index 139872c2e0fe..f794a2988a93 100644
>> --- a/include/linux/mfd/sun4i-gpadc.h
>> +++ b/include/linux/mfd/sun4i-gpadc.h
>> @@ -38,9 +38,12 @@
>>  #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 sun8i A23/A33 SoCs */
>> +#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN		BIT(8)
>> +#define SUN8I_A23_GPADC_CTRL1_GPADC_CALI_EN		BIT(7)
>> +
> 
> Different patch for these?
Looks to me like you want a precursor that reworks the driver to
be ready to support the new part, followed by a patch that actually
introduces the new part.

Jonathan
> 
> Thanks,
> Quentin
> 

[RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Maxime Ripard]

On Wed, Mar 29, 2017 at 02:57:02PM +0800, Icenowy Zheng wrote:
> > > @@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) 
> > >? if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF)) 
> > >? iio_map_array_unregister(indio_dev); 
> > >? 
> > > + if (info->data->gen2_ths) { 
> > > + clk_disable_unprepare(info->ths_clk); 
> > > + clk_disable_unprepare(info->ths_bus_clk); 
> > > + reset_control_deassert(info->reset); 
> > > + } 
> > > + 
> >
> > I'm not really fond of using this boolean as I don't see it being 
> > possibly reused for any other SoCs that has a GPADC or THS. 
> 
> Because you didn't care new SoCs :-)
> 
> All SoCs after H3 (A64, H5, R40) uses the same THS architecture with
> H3.

That's not really Quentin's point. His point is that having things
like flags and/or variables to identify various behaviours that might
differ from one SoC to the other usually works better when you want to
support several of them.

For example, replacing the gen2_ths by one variable with the number of
clocks, one with the number of channels, a bool to say it has a reset,
etc. definitely works better for us when Allwinner does some mix and
match between each SoC. And this happen most of the time.

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/20170329/253ec5b8/attachment.sig>

[RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor

[Lee Jones]

On Wed, 29 Mar 2017, Icenowy Zheng wrote:

> 
> 2017?3?29? 14:54? Quentin Schulz <quentin.schulz@free-electrons.com>???
> >
> > Hi, 
> >
> > On 28/03/2017 19:30, 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. 
> > > 
> > > An option is added to gpadc_data struct, to indicate whether this device 
> > > is a new-generation Allwinner thermal sensor. 
> > > 
> > > 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> 
> > > --- 
> > >? drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------ 
> > >? include/linux/mfd/sun4i-gpadc.h?? |? 33 +++++++++- 
> > >? 2 files changed, 141 insertions(+), 22 deletions(-) 

How have you managed to reply un-threaded?

Please ensure you mailer attaches your reply to the rest of the
thread, or things will become very confusing, very quickly.

> > > diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c 
> > > index 74705aa37982..7512b1cae877 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> 
> > >? 
> > > @@ -56,6 +58,7 @@ struct gpadc_data { 
> > >? unsigned int tp_adc_select; 
> > >? unsigned int (*adc_chan_select)(unsigned int chan); 
> > >? unsigned int adc_chan_mask; 
> > > + bool gen2_ths; 
> > >? }; 
> > >? 
> >
> > Instead of a boolean, give the TEMP_DATA register address. 
> >
> > >? static const struct gpadc_data sun4i_gpadc_data = { 
> > > @@ -88,7 +91,20 @@ 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_A23_GPADC_CTRL1_CHOP_TEMP_EN, 
> > > +}; 
> >
> > Separate patch for this? 
> >
> > > + 
> > > +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, 
> > > + .gen2_ths = true, 
> > >? }; 
> > >? 
> > >? struct sun4i_gpadc_iio { 
> > > @@ -103,6 +119,9 @@ struct sun4i_gpadc_iio { 
> > >? atomic_t ignore_temp_data_irq; 
> > >? const struct gpadc_data *data; 
> > >? bool no_irq; 
> > > + struct clk *ths_bus_clk; 
> > > + struct clk *ths_clk; 
> > > + struct reset_control *reset; 
> > >? /* prevents concurrent reads of temperature and ADC */ 
> > >? struct mutex mutex; 
> > >? }; 
> > > @@ -274,7 +293,11 @@ 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); 
> > > + if (info->data->gen2_ths) 
> > > + regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA, 
> > > + ??? val); 
> > > + else 
> > > + regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val); 
> > >? 
> >
> > Instead of gen2_ths, use the TEMP_DATA register address. 
> >
> > >? pm_runtime_mark_last_busy(indio_dev->dev.parent); 
> > >? pm_runtime_put_autosuspend(indio_dev->dev.parent); 
> > > @@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev) 
> > >? { 
> > >? 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 */ 
> > > - regmap_write(info->regmap, SUN4I_GPADC_TPR, 0); 
> > > + if (info->data->gen2_ths) { 
> > > + /* Disable temperature sensor */ 
> > > + regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0); 
> > > + } else { 
> > > + /* Disable the ADC on IP */ 
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0); 
> > > + /* Disable temperature sensor on IP */ 
> > > + regmap_write(info->regmap, SUN4I_GPADC_TPR, 0); 
> > > + } 
> > >? 
> >
> > Either use another register address or add a suspend function to struct 
> > gpadc_data which will be different for each version of the IP. 
> >
> > >? return 0; 
> > >? } 
> > > @@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev) 
> > >? { 
> > >? struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev)); 
> > >? 
> > > - /* clkin = 6MHz */ 
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL0, 
> > > - ???? SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) | 
> > > - ???? SUN4I_GPADC_CTRL0_FS_DIV(7) | 
> > > - ???? SUN4I_GPADC_CTRL0_T_ACQ(63)); 
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en); 
> > > - regmap_write(info->regmap, SUN4I_GPADC_CTRL3, 
> > > - ???? SUN4I_GPADC_CTRL3_FILTER_EN | 
> > > - ???? SUN4I_GPADC_CTRL3_FILTER_TYPE(1)); 
> > > - /* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */ 
> > > - regmap_write(info->regmap, SUN4I_GPADC_TPR, 
> > > - ???? SUN4I_GPADC_TPR_TEMP_ENABLE | 
> > > - ???? SUN4I_GPADC_TPR_TEMP_PERIOD(800)); 
> > > + if (info->data->gen2_ths) { 
> > > + 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)); 
> > > + } else { 
> > > + /* clkin = 6MHz */ 
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL0, 
> > > + ???? SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) | 
> > > + ???? SUN4I_GPADC_CTRL0_FS_DIV(7) | 
> > > + ???? SUN4I_GPADC_CTRL0_T_ACQ(63)); 
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 
> > > + ???? info->data->tp_mode_en); 
> > > + regmap_write(info->regmap, SUN4I_GPADC_CTRL3, 
> > > + ???? SUN4I_GPADC_CTRL3_FILTER_EN | 
> > > + ???? SUN4I_GPADC_CTRL3_FILTER_TYPE(1)); 
> > > + /* 
> > > + * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; 
> > > + * ~0.6s 
> > > + */ 
> > > + regmap_write(info->regmap, SUN4I_GPADC_TPR, 
> > > + ???? SUN4I_GPADC_TPR_TEMP_ENABLE | 
> > > + ???? SUN4I_GPADC_TPR_TEMP_PERIOD(800)); 
> > > + } 
> > >? 
> >
> > Same here as suspend function? 
> >
> > >? return 0; 
> > >? } 
> > > @@ -494,6 +539,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 */ } 
> > >? }; 
> > >? 
> > > @@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev, 
> > >? return ret; 
> > >? } 
> > >? 
> > > + if (info->data->gen2_ths) { 
> > > + 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; 
> > > + 
> > > + info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus"); 
> > > + if (IS_ERR(info->ths_bus_clk)) { 
> > > + ret = PTR_ERR(info->ths_bus_clk); 
> > > + return ret; 
> > > + } 
> > > + 
> > > + ret = clk_prepare_enable(info->ths_bus_clk); 
> > > + if (ret) 
> > > + return ret; 
> > > + 
> > > + info->ths_clk = devm_clk_get(&pdev->dev, "ths"); 
> > > + if (IS_ERR(info->ths_clk)) { 
> > > + ret = PTR_ERR(info->ths_clk); 
> > > + return ret; 
> > > + } 
> > > + 
> > > + /* Running at 6MHz */ 
> > > + ret = clk_set_rate(info->ths_clk, 6000000); 
> > > + if (ret) 
> > > + return ret; 
> > > + 
> > > + ret = clk_prepare_enable(info->ths_clk); 
> > > + if (ret) 
> > > + return ret; 
> > > + } 
> > > + 
> > >? if (!IS_ENABLED(CONFIG_THERMAL_OF)) 
> > >? return 0; 
> > >? 
> > > @@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev) 
> > >? if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF)) 
> > >? iio_map_array_unregister(indio_dev); 
> > >? 
> > > + if (info->data->gen2_ths) { 
> > > + clk_disable_unprepare(info->ths_clk); 
> > > + clk_disable_unprepare(info->ths_bus_clk); 
> > > + reset_control_deassert(info->reset); 
> > > + } 
> > > + 
> >
> > I'm not really fond of using this boolean as I don't see it being 
> > possibly reused for any other SoCs that has a GPADC or THS. 
> 
> Because you didn't care new SoCs :-)
> 
> All SoCs after H3 (A64, H5, R40) uses the same THS architecture with H3.
> 
> >
> > Here, you could make use of a list/array of clk which then can be reused 
> > for other SoCs just by changing the list. Add a default rate to the 
> > gpadc_data structure and you're go to go. 
> >
> > >? return 0; 
> > >? } 
> > >? 
> > > diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h 
> > > index 139872c2e0fe..f794a2988a93 100644 
> > > --- a/include/linux/mfd/sun4i-gpadc.h 
> > > +++ b/include/linux/mfd/sun4i-gpadc.h 
> > > @@ -38,9 +38,12 @@ 
> > >? #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 sun8i A23/A33 SoCs */ 
> > > +#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN BIT(8) 
> > > +#define SUN8I_A23_GPADC_CTRL1_GPADC_CALI_EN BIT(7) 
> > > + 
> >
> > Different patch for these? 
> >
> > Thanks, 
> > Quentin 
> >
> > _______________________________________________ 
> > linux-arm-kernel mailing list 
> > linux-arm-kernel at lists.infradead.org 
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel 

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