[PATCH v4 0/4] Rockchip soc thermal driver

[PATCH v4 0/4] Rockchip soc thermal driver

[Caesar Wang]

Changes in v4:
        * address comments from Jonathan Cameron,huangtao and zhaoyifeng:
        - this series thermal driver still be put in driver/thermal/
        - modify the thermal driver description.

Changes in v3:(add dts configure)
        * address comments from Dmitry Torokhov and Arnd Bergmann:
        - fix clock-names in rockchip-thermal.txt
        - remove rockchip_thermal_control() in rockchip_set_mode()
        - fix some code style.
        - add dts configure.

Changes in v2:
        * address comments from Heiko Stubner:
        - fix dt-bindings in rockchip-thermal.txt
        - remove Author mark
        - rename TSADC_XXX->TSADCV2_XXX,it eill ready to merge compatible other SoCs.
        - fix a identation
        - remove clk_set_rate(),it's no necessary.
        - fix the SIMPLE_DEV_PM_OPS() function  style.

Tested on rk3288 SDK board

Caesar Wang (4):
  thermal: rockchip: add driver for thermal
  dt-bindings: document Rockchip thermal
  ARM: dts: add main Thermal info to rk3288
  ARM: dts: enable Thermal on rk3288-evb board

 .../bindings/thermal/rockchip-thermal.txt          |  20 +
 arch/arm/boot/dts/rk3288-evb.dtsi                  |   7 +
 arch/arm/boot/dts/rk3288.dtsi                      |  18 +
 drivers/thermal/Kconfig                            |   9 +
 drivers/thermal/Makefile                           |   1 +
 drivers/thermal/rockchip_thermal.c                 | 669 +++++++++++++++++++++
 6 files changed, 724 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
 create mode 100644 drivers/thermal/rockchip_thermal.c

-- 
1.9.1

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Caesar Wang]

Thermal is TS-ADC Controller module supports
user-defined mode and automatic mode.

User-defined mode refers,TSADC all the control signals entirely by
software writing to register for direct control.

Automaic mode refers to the module automatically poll TSADC output,
and the results were checked.If you find that the temperature High
in a period of time,an interrupt is generated to the processor
down-measures taken;if the temperature over a period of time High,
the resulting TSHUT gave CRU module,let it reset the entire chip,
or via GPIO give PMIC.

Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
---
 drivers/thermal/Kconfig            |   9 +
 drivers/thermal/Makefile           |   1 +
 drivers/thermal/rockchip_thermal.c | 669 +++++++++++++++++++++++++++++++++++++
 3 files changed, 679 insertions(+)
 create mode 100644 drivers/thermal/rockchip_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index f9a1386..a00aa1e 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -133,6 +133,15 @@ config SPEAR_THERMAL
 	  Enable this to plug the SPEAr thermal sensor driver into the Linux
 	  thermal framework.
 
+config ROCKCHIP_THERMAL
+	tristate "Rockchip thermal driver"
+	depends on ARCH_ROCKCHIP
+	help
+	  Support for Temperature Sensor ADC (TS-ADC) found on Rockchip SoCs.
+	  It supports one critical trip point and one passive trip point.  The
+	  cpufreq is used as the cooling device to throttle CPUs when the
+	  passive trip is crossed.
+
 config RCAR_THERMAL
 	tristate "Renesas R-Car thermal driver"
 	depends on ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index de0636a..b48b817 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -19,6 +19,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
 
 # platform thermal drivers
 obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
+obj-$(CONFIG_ROCKCHIP_THERMAL) 	+= rockchip_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
 obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
 obj-y				+= samsung/
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
new file mode 100644
index 0000000..011f387
--- /dev/null
+++ b/drivers/thermal/rockchip_thermal.c
@@ -0,0 +1,669 @@
+/*
+ * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+*/
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/cpu_cooling.h>
+#include "thermal_core.h"
+
+enum rockchip_thermal_trip {
+	ROCKCHIP_TRIP_PASSIVE,
+	ROCKCHIP_TRIP_CRITICAL,
+	ROCKCHIP_TRIP_NUM,
+};
+
+struct rockchip_thermal_data {
+	const struct rockchip_tsadc_platform_data *pdata;
+	struct thermal_zone_device *tz;
+	struct thermal_cooling_device *cdev;
+	enum thermal_device_mode mode;
+	void __iomem *regs;
+
+	signed long temp_passive;
+	signed long temp_critical;
+	signed long temp_force_shut;
+	signed long alarm_temp;
+	signed long last_temp;
+	bool irq_enabled;
+	int irq;
+	struct clk *clk;
+	struct clk *pclk;
+};
+
+struct rockchip_tsadc_platform_data {
+	u8 irq_en;
+	signed long temp_passive;
+	signed long temp_critical;
+	signed long temp_force_shut;
+	int passive_delay;
+	int polling_delay;
+
+	int (*irq_handle)(void __iomem *reg);
+	int (*initialize)(void __iomem *reg, signed long temp_force_shut);
+	int (*control)(void __iomem *reg, bool on);
+	u32 (*code_to_temp)(int temp);
+	u32 (*temp_to_code)(int temp);
+	void (*set_alarm_temp)(void __iomem *regs, signed long temp);
+};
+
+/*TSADC V2 Sensor info define:*/
+#define TSADCV2_AUTO_CON			0x04
+#define TSADCV2_INT_EN				0x08
+#define TSADCV2_INT_PD				0x0c
+#define TSADCV2_DATA1				0x24
+#define TSADCV2_COMP1_INT			0x34
+#define TSADCV2_COMP1_SHUT			0x44
+#define TSADCV2_AUTO_PERIOD			0x68
+#define TSADCV2_AUTO_PERIOD_HT			0x6c
+
+#define TSADCV2_AUTO_SRC1_EN			(1 << 5)
+#define TSADCV2_AUTO_EN				(1 << 0)
+#define TSADCV2_AUTO_DISABLE			~(1 << 0)
+#define TSADCV2_AUTO_STAS_BUSY			(1 << 16)
+#define TSADCV2_AUTO_STAS_BUSY_MASK		(1 << 16)
+#define TSADCV2_SHUT_2GPIO_SRC1_EN		(1 << 5)
+#define TSADCV2_INT_SRC1_EN			(1 << 1)
+#define TSADCV2_SHUT_SRC1_STATUS		(1 << 5)
+#define TSADCV2_INT_SRC1_STATUS			(1 << 1)
+
+#define TSADCV2_DATA_MASK			0xfff
+#define TSADCV2_HIGHT_INT_DEBOUNCE		0x60
+#define TSADCV2_HIGHT_TSHUT_DEBOUNCE		0x64
+#define TSADCV2_HIGHT_INT_DEBOUNCE_TIME		0x0a
+#define TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME	0x0a
+#define TSADCV2_AUTO_PERIOD_TIME		0x03e8
+#define TSADCV2_AUTO_PERIOD_HT_TIME		0x64
+
+struct tsadc_table {
+	int code;
+	int temp;
+};
+
+static const struct tsadc_table v2_code_table[] = {
+	{TSADCV2_DATA_MASK, -40},
+	{3800, -40},
+	{3792, -35},
+	{3783, -30},
+	{3774, -25},
+	{3765, -20},
+	{3756, -15},
+	{3747, -10},
+	{3737, -5},
+	{3728, 0},
+	{3718, 5},
+	{3708, 10},
+	{3698, 15},
+	{3688, 20},
+	{3678, 25},
+	{3667, 30},
+	{3656, 35},
+	{3645, 40},
+	{3634, 45},
+	{3623, 50},
+	{3611, 55},
+	{3600, 60},
+	{3588, 65},
+	{3575, 70},
+	{3563, 75},
+	{3550, 80},
+	{3537, 85},
+	{3524, 90},
+	{3510, 95},
+	{3496, 100},
+	{3482, 105},
+	{3467, 110},
+	{3452, 115},
+	{3437, 120},
+	{3421, 125},
+	{0, 125},
+};
+
+static int rk_tsadcv2_irq_handle(void __iomem *regs)
+{
+	u32 val;
+
+	val = readl_relaxed(regs + TSADCV2_INT_PD);
+	writel_relaxed(val & ~(1 << 8), regs + TSADCV2_INT_PD);
+
+	return 0;
+}
+
+static u32 rk_tsadcv2_temp_to_code(int temp)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
+		if (temp <= v2_code_table[i].temp && temp >
+		    v2_code_table[i - 1].temp)
+			return v2_code_table[i].code;
+	}
+
+	return 0;
+}
+
+static u32 rk_tsadcv2_code_to_temp(int code)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
+		if (code <= v2_code_table[i].code && code >
+		    v2_code_table[i + 1].code){
+			return v2_code_table[i].temp;
+		}
+	}
+
+	return 0;
+}
+
+static int rk_tsadcv2_initialize(void __iomem *regs,
+				 signed long temp_force_shut)
+{
+	int shutdown_value;
+
+	shutdown_value = rk_tsadcv2_temp_to_code(temp_force_shut);
+	/* Enable measurements at ~ 10 Hz */
+	writel_relaxed(0, regs + TSADCV2_AUTO_CON);
+	writel_relaxed(TSADCV2_AUTO_PERIOD_TIME, regs + TSADCV2_AUTO_PERIOD);
+	writel_relaxed(TSADCV2_AUTO_PERIOD_HT_TIME, regs +
+		       TSADCV2_AUTO_PERIOD_HT);
+	writel_relaxed(shutdown_value, regs + TSADCV2_COMP1_SHUT);
+	writel_relaxed(TSADCV2_HIGHT_INT_DEBOUNCE_TIME, regs +
+		       TSADCV2_HIGHT_INT_DEBOUNCE);
+	writel_relaxed(TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME, regs +
+		       TSADCV2_HIGHT_TSHUT_DEBOUNCE);
+	writel_relaxed(TSADCV2_SHUT_2GPIO_SRC1_EN | TSADCV2_INT_SRC1_EN, regs +
+		       TSADCV2_INT_EN);
+	writel_relaxed(TSADCV2_AUTO_SRC1_EN | TSADCV2_AUTO_EN, regs +
+		       TSADCV2_AUTO_CON);
+
+	return 0;
+}
+
+static int rk_tsadcv2_control(void __iomem *regs, bool on)
+{
+	u32 val;
+
+	if (on) {
+		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
+		writel_relaxed(val | TSADCV2_AUTO_EN, regs + TSADCV2_AUTO_CON);
+	} else {
+		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
+		writel_relaxed(val & TSADCV2_AUTO_DISABLE,
+			       regs + TSADCV2_AUTO_CON);
+	}
+
+	return 0;
+}
+
+static void rk_tsadcv2_alarm_temp(void __iomem *regs, signed long alarm_temp)
+{
+	int alarm_value;
+
+	alarm_value = rk_tsadcv2_temp_to_code(alarm_temp);
+	writel_relaxed(alarm_value, regs + TSADCV2_COMP1_INT);
+}
+
+struct rockchip_tsadc_platform_data const rk3288_tsadc_data = {
+	.irq_en = 1,
+	.temp_passive = 85000,
+	.temp_critical = 100000,
+	.temp_force_shut = 120000,
+	.passive_delay = 2000,
+	.polling_delay = 1000,
+	.irq_handle = rk_tsadcv2_irq_handle,
+	.initialize = rk_tsadcv2_initialize,
+	.control = rk_tsadcv2_control,
+	.code_to_temp = rk_tsadcv2_code_to_temp,
+	.temp_to_code = rk_tsadcv2_temp_to_code,
+	.set_alarm_temp = rk_tsadcv2_alarm_temp,
+};
+
+static const struct of_device_id of_rockchip_thermal_match[] = {
+	{
+		.compatible = "rockchip,rk3288-tsadc",
+		.data = (void *)&rk3288_tsadc_data,
+	},
+	{ /* end */ },
+};
+MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
+
+static void rockchip_set_alarm_temp(struct rockchip_thermal_data *data,
+				    signed long alarm_temp)
+{
+	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
+
+	data->alarm_temp = alarm_temp;
+	if (p_tsadc_data->set_alarm_temp)
+		p_tsadc_data->set_alarm_temp(data->regs, alarm_temp);
+}
+
+static int rockchip_get_temp(struct thermal_zone_device *tz,
+			     unsigned long *temp)
+{
+	struct rockchip_thermal_data *data = tz->devdata;
+	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
+	u32 val;
+
+	val = readl_relaxed(data->regs + TSADCV2_DATA1);
+	*temp = p_tsadc_data->code_to_temp(val);
+
+	/* Update alarm value to next higher trip point */
+	if (data->alarm_temp == data->temp_passive && *temp >=
+	    data->temp_passive)
+		rockchip_set_alarm_temp(data, data->temp_critical);
+
+	if (data->alarm_temp == data->temp_critical && *temp <
+	    data->temp_passive) {
+		rockchip_set_alarm_temp(data, data->temp_passive);
+		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
+			data->alarm_temp / 1000);
+	}
+
+	if (*temp != data->last_temp) {
+		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
+		data->last_temp = *temp;
+	}
+
+	/* Reenable alarm IRQ if temperature below alarm temperature */
+	if (!data->irq_enabled && *temp < data->alarm_temp) {
+		data->irq_enabled = true;
+		enable_irq(data->irq);
+	}
+
+	return 0;
+}
+
+static int rockchip_thermal_initialize(struct rockchip_thermal_data *data)
+{
+	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
+
+	if (p_tsadc_data->initialize)
+		p_tsadc_data->initialize(data->regs, data->temp_force_shut);
+	rockchip_set_alarm_temp(data, data->temp_passive);
+
+	return 0;
+}
+
+static void rockchip_thermal_control(struct rockchip_thermal_data *data,
+				     bool on)
+{
+	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
+
+	if (p_tsadc_data->control)
+		p_tsadc_data->control(data->regs, on);
+
+	if (on) {
+		data->irq_enabled = true;
+		data->mode = THERMAL_DEVICE_ENABLED;
+	} else {
+		data->irq_enabled = false;
+		data->mode = THERMAL_DEVICE_DISABLED;
+	}
+}
+
+static int rockchip_get_mode(struct thermal_zone_device *tz,
+			     enum thermal_device_mode *mode)
+{
+	struct rockchip_thermal_data *data = tz->devdata;
+
+	*mode = data->mode;
+
+	return 0;
+}
+
+static int rockchip_set_mode(struct thermal_zone_device *tz,
+			     enum thermal_device_mode mode)
+{
+	struct rockchip_thermal_data *data = tz->devdata;
+	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
+
+	if (mode == THERMAL_DEVICE_ENABLED) {
+		tz->polling_delay = p_tsadc_data->polling_delay;
+		tz->passive_delay = p_tsadc_data->passive_delay;
+		if (!data->irq_enabled) {
+			data->irq_enabled = true;
+			enable_irq(data->irq);
+		}
+	} else {
+		tz->polling_delay = 0;
+		tz->passive_delay = 0;
+		if (data->irq_enabled) {
+			disable_irq(data->irq);
+			data->irq_enabled = false;
+		}
+	}
+
+	data->mode = mode;
+	thermal_zone_device_update(tz);
+
+	return 0;
+}
+
+static int rockchip_get_trip_type(struct thermal_zone_device *tz, int trip,
+				  enum thermal_trip_type *type)
+{
+	*type = (trip == ROCKCHIP_TRIP_PASSIVE) ? THERMAL_TRIP_PASSIVE :
+		 THERMAL_TRIP_CRITICAL;
+
+	return 0;
+}
+
+static int rockchip_get_crit_temp(struct thermal_zone_device *tz,
+				  unsigned long *temp)
+{
+	struct rockchip_thermal_data *data = tz->devdata;
+
+	*temp = data->temp_critical;
+
+	return 0;
+}
+
+static int rockchip_get_trip_temp(struct thermal_zone_device *tz, int trip,
+				  unsigned long *temp)
+{
+	struct rockchip_thermal_data *data = tz->devdata;
+
+	*temp = (trip == ROCKCHIP_TRIP_PASSIVE) ? data->temp_passive :
+		 data->temp_critical;
+
+	return 0;
+}
+
+static int rockchip_set_trip_temp(struct thermal_zone_device *tz, int trip,
+				  unsigned long temp)
+{
+	struct rockchip_thermal_data *data = tz->devdata;
+
+	if (trip == ROCKCHIP_TRIP_CRITICAL)
+		return -EPERM;
+
+	data->temp_passive = temp;
+	rockchip_set_alarm_temp(data, temp);
+
+	return 0;
+}
+
+static int rockchip_bind(struct thermal_zone_device *tz,
+			 struct thermal_cooling_device *cdev)
+{
+	int ret;
+
+	ret = thermal_zone_bind_cooling_device(tz, ROCKCHIP_TRIP_PASSIVE, cdev,
+					       THERMAL_NO_LIMIT,
+					       THERMAL_NO_LIMIT);
+	if (ret) {
+		dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
+			tz->type, cdev->type, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rockchip_unbind(struct thermal_zone_device *tz,
+			   struct thermal_cooling_device *cdev)
+{
+	int ret;
+
+	ret = thermal_zone_unbind_cooling_device(tz,
+						 ROCKCHIP_TRIP_PASSIVE, cdev);
+	if (ret) {
+		dev_err(&tz->device,
+			"unbinding zone %s with cdev %s failed:%d\n", tz->type,
+			cdev->type, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct thermal_zone_device_ops rockchip_tz_ops = {
+	.bind = rockchip_bind,
+	.unbind = rockchip_unbind,
+	.get_temp = rockchip_get_temp,
+	.get_mode = rockchip_get_mode,
+	.set_mode = rockchip_set_mode,
+	.get_trip_type = rockchip_get_trip_type,
+	.get_trip_temp = rockchip_get_trip_temp,
+	.get_crit_temp = rockchip_get_crit_temp,
+	.set_trip_temp = rockchip_set_trip_temp,
+};
+
+static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
+{
+	struct rockchip_thermal_data *data = data;
+	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
+
+	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
+		data->alarm_temp / 1000);
+
+	if (p_tsadc_data->irq_en && p_tsadc_data->irq_handle)
+		p_tsadc_data->irq_handle(data->regs);
+
+	thermal_zone_device_update(data->tz);
+
+	return IRQ_HANDLED;
+}
+
+static int rockchip_thermal_probe(struct platform_device *pdev)
+{
+	struct rockchip_thermal_data *data;
+	const struct rockchip_tsadc_platform_data *p_tsadc_data;
+	struct cpumask clip_cpus;
+	struct resource *res;
+	const struct of_device_id *match;
+	int ret, temp;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_thermal_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	data->regs = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(data->regs)) {
+		dev_err(&pdev->dev, "Could not get tsadc source, %p\n",
+			data->regs);
+		return PTR_ERR(data->regs);
+	}
+
+	match = of_match_node(of_rockchip_thermal_match, pdev->dev.of_node);
+	if (!match)
+		return -ENXIO;
+	data->pdata = (const struct rockchip_tsadc_platform_data *)match->data;
+	if (!data->pdata)
+		return -EINVAL;
+	p_tsadc_data = data->pdata;
+
+	data->clk = devm_clk_get(&pdev->dev, "tsadc");
+	if (IS_ERR(data->clk)) {
+		dev_err(&pdev->dev, "failed to get tsadc clock\n");
+		return PTR_ERR(data->clk);
+	}
+
+	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
+	if (IS_ERR(data->pclk)) {
+		dev_err(&pdev->dev, "failed to get tsadc pclk\n");
+		return PTR_ERR(data->pclk);
+	}
+
+	/*
+	* Use a default of 10KHz for the converter clock.
+	* This may become user-configurable in the future.
+	*/
+	ret = clk_set_rate(data->clk, 10000);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to set tsadc clk rate, %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable converter clock\n");
+		goto err_clk;
+	}
+
+	ret = clk_prepare_enable(data->pclk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to enable pclk\n");
+		goto err_pclk;
+	}
+
+	platform_set_drvdata(pdev, data);
+
+	if (of_property_read_u32(pdev->dev.of_node, "passive-temp", &temp)) {
+		dev_warn(&pdev->dev,
+			 "Missing default passive temp property in the DT.\n");
+		data->temp_passive = p_tsadc_data->temp_passive;
+	} else {
+		data->temp_passive = temp;
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node, "critical-temp", &temp)) {
+		dev_warn(&pdev->dev,
+			 "Missing default critical temp property in the DT.\n");
+		data->temp_critical = p_tsadc_data->temp_critical;
+	} else {
+		data->temp_critical = temp;
+	}
+
+	if (of_property_read_u32(pdev->dev.of_node, "force-shut-temp", &temp)) {
+		dev_warn(&pdev->dev,
+			 "Missing default force shut down temp property in the DT.\n");
+		data->temp_force_shut = p_tsadc_data->temp_force_shut;
+	} else {
+		data->temp_force_shut = temp;
+	}
+
+	cpumask_set_cpu(0, &clip_cpus);
+	data->cdev = of_cpufreq_cooling_register(pdev->dev.of_node, &clip_cpus);
+	if (IS_ERR(data->cdev)) {
+		dev_err(&pdev->dev, "failed to register cpufreq cooling device\n");
+		goto disable_clk;
+	}
+
+	data->tz = thermal_zone_device_register("rockchip_thermal",
+						ROCKCHIP_TRIP_NUM,
+						0, data,
+						&rockchip_tz_ops, NULL,
+						p_tsadc_data->passive_delay,
+						p_tsadc_data->polling_delay);
+
+	if (IS_ERR(data->tz)) {
+		dev_err(&pdev->dev, "failed to register thermal zone device\n");
+		goto fail_cpufreq_register;
+	}
+
+	if (p_tsadc_data->irq_en) {
+		data->irq = platform_get_irq(pdev, 0);
+		if (data->irq < 0) {
+			dev_err(&pdev->dev, "no irq resource?\n");
+			goto fail_irq;
+		}
+
+		ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
+				rockchip_thermal_alarm_irq_thread,
+				IRQF_ONESHOT, "rockchip_thermal", data);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"failed to request tsadc irq: %d\n", ret);
+			goto fail_thermal_unregister;
+		}
+	}
+
+	rockchip_thermal_initialize(data);
+	rockchip_thermal_control(data, true);
+
+	return 0;
+
+fail_thermal_unregister:
+	thermal_zone_device_unregister(data->tz);
+fail_irq:
+fail_cpufreq_register:
+	cpufreq_cooling_unregister(data->cdev);
+disable_clk:
+err_pclk:
+	clk_disable_unprepare(data->pclk);
+err_clk:
+	clk_disable_unprepare(data->clk);
+
+	return ret;
+}
+
+static int rockchip_thermal_remove(struct platform_device *pdev)
+{
+	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
+
+	rockchip_thermal_control(data, false);
+
+	thermal_zone_device_unregister(data->tz);
+	cpufreq_cooling_unregister(data->cdev);
+	cpufreq_cooling_unregister(data->cdev);
+
+	clk_disable_unprepare(data->clk);
+	clk_disable_unprepare(data->pclk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int rockchip_thermal_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
+
+	rockchip_thermal_control(data, false);
+
+	return 0;
+}
+
+static int rockchip_thermal_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
+
+	rockchip_thermal_initialize(data);
+	rockchip_thermal_control(data, true);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(rockchip_thermal_pm_ops,
+			 rockchip_thermal_suspend, rockchip_thermal_resume);
+
+static struct platform_driver rockchip_thermal_driver = {
+	.driver = {
+		   .name = "rockchip-thermal",
+		   .owner = THIS_MODULE,
+		   .pm = &rockchip_thermal_pm_ops,
+		   .of_match_table = of_rockchip_thermal_match,
+		   },
+	.probe = rockchip_thermal_probe,
+	.remove = rockchip_thermal_remove,
+};
+
+module_platform_driver(rockchip_thermal_driver);
+
+MODULE_DESCRIPTION("ROCKCHIP THERMAL Driver");
+MODULE_AUTHOR("Rockchip, Inc.");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:rockchip-thermal");
-- 
1.9.1

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Eduardo Valentin]

Hello Ceasar,

On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
> Thermal is TS-ADC Controller module supports
> user-defined mode and automatic mode.
> 
> User-defined mode refers,TSADC all the control signals entirely by
> software writing to register for direct control.
> 
> Automaic mode refers to the module automatically poll TSADC output,
> and the results were checked.If you find that the temperature High
> in a period of time,an interrupt is generated to the processor
> down-measures taken;if the temperature over a period of time High,
> the resulting TSHUT gave CRU module,let it reset the entire chip,
> or via GPIO give PMIC.
> 
> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> ---
>  drivers/thermal/Kconfig            |   9 +
>  drivers/thermal/Makefile           |   1 +
>  drivers/thermal/rockchip_thermal.c | 669 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 679 insertions(+)
>  create mode 100644 drivers/thermal/rockchip_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index f9a1386..a00aa1e 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -133,6 +133,15 @@ config SPEAR_THERMAL
>  	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>  	  thermal framework.
>  
> +config ROCKCHIP_THERMAL
> +	tristate "Rockchip thermal driver"
> +	depends on ARCH_ROCKCHIP
> +	help
> +	  Support for Temperature Sensor ADC (TS-ADC) found on Rockchip SoCs.
> +	  It supports one critical trip point and one passive trip point.  The
> +	  cpufreq is used as the cooling device to throttle CPUs when the
> +	  passive trip is crossed.
> +
>  config RCAR_THERMAL
>  	tristate "Renesas R-Car thermal driver"
>  	depends on ARCH_SHMOBILE || COMPILE_TEST
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index de0636a..b48b817 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -19,6 +19,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
>  
>  # platform thermal drivers
>  obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> +obj-$(CONFIG_ROCKCHIP_THERMAL) 	+= rockchip_thermal.o
>  obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>  obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>  obj-y				+= samsung/
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> new file mode 100644
> index 0000000..011f387
> --- /dev/null
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -0,0 +1,669 @@
> +/*
> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> +*/
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/cpu_cooling.h>
> +#include "thermal_core.h"

Why do you need thermal_core.h here? Wouldn't linux/thermal.h be
sufficient?

> +
> +enum rockchip_thermal_trip {
> +	ROCKCHIP_TRIP_PASSIVE,
> +	ROCKCHIP_TRIP_CRITICAL,
> +	ROCKCHIP_TRIP_NUM,
> +};
> +

Why do you need your own trip types?

> +struct rockchip_thermal_data {
> +	const struct rockchip_tsadc_platform_data *pdata;
> +	struct thermal_zone_device *tz;
> +	struct thermal_cooling_device *cdev;
> +	enum thermal_device_mode mode;
> +	void __iomem *regs;
> +
> +	signed long temp_passive;
> +	signed long temp_critical;
> +	signed long temp_force_shut;
> +	signed long alarm_temp;
> +	signed long last_temp;
> +	bool irq_enabled;
> +	int irq;
> +	struct clk *clk;
> +	struct clk *pclk;
> +};
> +
> +struct rockchip_tsadc_platform_data {
> +	u8 irq_en;
> +	signed long temp_passive;
> +	signed long temp_critical;
> +	signed long temp_force_shut;
> +	int passive_delay;
> +	int polling_delay;
> +
> +	int (*irq_handle)(void __iomem *reg);
> +	int (*initialize)(void __iomem *reg, signed long temp_force_shut);
> +	int (*control)(void __iomem *reg, bool on);
> +	u32 (*code_to_temp)(int temp);
> +	u32 (*temp_to_code)(int temp);
> +	void (*set_alarm_temp)(void __iomem *regs, signed long temp);
> +};
> +
> +/*TSADC V2 Sensor info define:*/
> +#define TSADCV2_AUTO_CON			0x04
> +#define TSADCV2_INT_EN				0x08
> +#define TSADCV2_INT_PD				0x0c
> +#define TSADCV2_DATA1				0x24
> +#define TSADCV2_COMP1_INT			0x34
> +#define TSADCV2_COMP1_SHUT			0x44
> +#define TSADCV2_AUTO_PERIOD			0x68
> +#define TSADCV2_AUTO_PERIOD_HT			0x6c
> +
> +#define TSADCV2_AUTO_SRC1_EN			(1 << 5)
> +#define TSADCV2_AUTO_EN				(1 << 0)
> +#define TSADCV2_AUTO_DISABLE			~(1 << 0)
> +#define TSADCV2_AUTO_STAS_BUSY			(1 << 16)
> +#define TSADCV2_AUTO_STAS_BUSY_MASK		(1 << 16)
> +#define TSADCV2_SHUT_2GPIO_SRC1_EN		(1 << 5)
> +#define TSADCV2_INT_SRC1_EN			(1 << 1)
> +#define TSADCV2_SHUT_SRC1_STATUS		(1 << 5)
> +#define TSADCV2_INT_SRC1_STATUS			(1 << 1)
> +

The above bits can be defined with BIT() macro, right? Something like:
+#define TSADCV2_INT_SRC1_STATUS			BIT(1)


> +#define TSADCV2_DATA_MASK			0xfff
> +#define TSADCV2_HIGHT_INT_DEBOUNCE		0x60
> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE		0x64
> +#define TSADCV2_HIGHT_INT_DEBOUNCE_TIME		0x0a
> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME	0x0a
> +#define TSADCV2_AUTO_PERIOD_TIME		0x03e8
> +#define TSADCV2_AUTO_PERIOD_HT_TIME		0x64
> +
> +struct tsadc_table {
> +	int code;
> +	int temp;
> +};
> +
> +static const struct tsadc_table v2_code_table[] = {
> +	{TSADCV2_DATA_MASK, -40},
> +	{3800, -40},
> +	{3792, -35},
> +	{3783, -30},
> +	{3774, -25},
> +	{3765, -20},
> +	{3756, -15},
> +	{3747, -10},
> +	{3737, -5},
> +	{3728, 0},
> +	{3718, 5},
> +	{3708, 10},
> +	{3698, 15},
> +	{3688, 20},
> +	{3678, 25},
> +	{3667, 30},
> +	{3656, 35},
> +	{3645, 40},
> +	{3634, 45},
> +	{3623, 50},
> +	{3611, 55},
> +	{3600, 60},
> +	{3588, 65},
> +	{3575, 70},
> +	{3563, 75},
> +	{3550, 80},
> +	{3537, 85},
> +	{3524, 90},
> +	{3510, 95},
> +	{3496, 100},
> +	{3482, 105},
> +	{3467, 110},
> +	{3452, 115},
> +	{3437, 120},
> +	{3421, 125},
> +	{0, 125},
> +};
> +
> +static int rk_tsadcv2_irq_handle(void __iomem *regs)
> +{
> +	u32 val;
> +
> +	val = readl_relaxed(regs + TSADCV2_INT_PD);
> +	writel_relaxed(val & ~(1 << 8), regs + TSADCV2_INT_PD);

Why do you need to clear bit 8? Why hardcoded?

> +
> +	return 0;
> +}
> +
> +static u32 rk_tsadcv2_temp_to_code(int temp)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
> +		if (temp <= v2_code_table[i].temp && temp >
> +		    v2_code_table[i - 1].temp)
> +			return v2_code_table[i].code;
> +	}
> +
> +	return 0;
> +}
> +
> +static u32 rk_tsadcv2_code_to_temp(int code)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
> +		if (code <= v2_code_table[i].code && code >
> +		    v2_code_table[i + 1].code){
> +			return v2_code_table[i].temp;
> +		}
> +	}
> +
> +	return 0;
> +}

I know the table is not something too big, but considering it is ordered
by either ADC value code or by temp, at least one of the above searching function
may be more efficient, right?

> +
> +static int rk_tsadcv2_initialize(void __iomem *regs,
> +				 signed long temp_force_shut)
> +{
> +	int shutdown_value;
> +
> +	shutdown_value = rk_tsadcv2_temp_to_code(temp_force_shut);
> +	/* Enable measurements at ~ 10 Hz */

Does it leave the sampling clock at 10Hz? Is this clock exposed via
clock framework?

> +	writel_relaxed(0, regs + TSADCV2_AUTO_CON);
> +	writel_relaxed(TSADCV2_AUTO_PERIOD_TIME, regs + TSADCV2_AUTO_PERIOD);
> +	writel_relaxed(TSADCV2_AUTO_PERIOD_HT_TIME, regs +
> +		       TSADCV2_AUTO_PERIOD_HT);
> +	writel_relaxed(shutdown_value, regs + TSADCV2_COMP1_SHUT);
> +	writel_relaxed(TSADCV2_HIGHT_INT_DEBOUNCE_TIME, regs +
> +		       TSADCV2_HIGHT_INT_DEBOUNCE);
> +	writel_relaxed(TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME, regs +
> +		       TSADCV2_HIGHT_TSHUT_DEBOUNCE);
> +	writel_relaxed(TSADCV2_SHUT_2GPIO_SRC1_EN | TSADCV2_INT_SRC1_EN, regs +
> +		       TSADCV2_INT_EN);
> +	writel_relaxed(TSADCV2_AUTO_SRC1_EN | TSADCV2_AUTO_EN, regs +
> +		       TSADCV2_AUTO_CON);
> +
> +	return 0;
> +}
> +
> +static int rk_tsadcv2_control(void __iomem *regs, bool on)
> +{
> +	u32 val;
> +
> +	if (on) {
> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
> +		writel_relaxed(val | TSADCV2_AUTO_EN, regs + TSADCV2_AUTO_CON);
> +	} else {
> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
> +		writel_relaxed(val & TSADCV2_AUTO_DISABLE,
> +			       regs + TSADCV2_AUTO_CON);
> +	}
> +
> +	return 0;
> +}
> +
> +static void rk_tsadcv2_alarm_temp(void __iomem *regs, signed long alarm_temp)
> +{
> +	int alarm_value;
> +
> +	alarm_value = rk_tsadcv2_temp_to_code(alarm_temp);
> +	writel_relaxed(alarm_value, regs + TSADCV2_COMP1_INT);
> +}
> +
> +struct rockchip_tsadc_platform_data const rk3288_tsadc_data = {
> +	.irq_en = 1,
> +	.temp_passive = 85000,
> +	.temp_critical = 100000,
> +	.temp_force_shut = 120000,
> +	.passive_delay = 2000,
> +	.polling_delay = 1000,
> +	.irq_handle = rk_tsadcv2_irq_handle,
> +	.initialize = rk_tsadcv2_initialize,
> +	.control = rk_tsadcv2_control,
> +	.code_to_temp = rk_tsadcv2_code_to_temp,
> +	.temp_to_code = rk_tsadcv2_temp_to_code,
> +	.set_alarm_temp = rk_tsadcv2_alarm_temp,
> +};

shall the above struct be also static?

> +
> +static const struct of_device_id of_rockchip_thermal_match[] = {
> +	{
> +		.compatible = "rockchip,rk3288-tsadc",
> +		.data = (void *)&rk3288_tsadc_data,
> +	},
> +	{ /* end */ },
> +};
> +MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
> +
> +static void rockchip_set_alarm_temp(struct rockchip_thermal_data *data,
> +				    signed long alarm_temp)
> +{
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> +
> +	data->alarm_temp = alarm_temp;
> +	if (p_tsadc_data->set_alarm_temp)
> +		p_tsadc_data->set_alarm_temp(data->regs, alarm_temp);
> +}
> +
> +static int rockchip_get_temp(struct thermal_zone_device *tz,
> +			     unsigned long *temp)
> +{
> +	struct rockchip_thermal_data *data = tz->devdata;
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> +	u32 val;
> +
> +	val = readl_relaxed(data->regs + TSADCV2_DATA1);
> +	*temp = p_tsadc_data->code_to_temp(val);
> +
> +	/* Update alarm value to next higher trip point */
> +	if (data->alarm_temp == data->temp_passive && *temp >=
> +	    data->temp_passive)
> +		rockchip_set_alarm_temp(data, data->temp_critical);
> +
> +	if (data->alarm_temp == data->temp_critical && *temp <
> +	    data->temp_passive) {
> +		rockchip_set_alarm_temp(data, data->temp_passive);
> +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
> +			data->alarm_temp / 1000);
> +	}
> +
> +	if (*temp != data->last_temp) {
> +		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
> +		data->last_temp = *temp;
> +	}
> +
> +	/* Reenable alarm IRQ if temperature below alarm temperature */
> +	if (!data->irq_enabled && *temp < data->alarm_temp) {
> +		data->irq_enabled = true;
> +		enable_irq(data->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rockchip_thermal_initialize(struct rockchip_thermal_data *data)
> +{
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> +
> +	if (p_tsadc_data->initialize)
> +		p_tsadc_data->initialize(data->regs, data->temp_force_shut);
> +	rockchip_set_alarm_temp(data, data->temp_passive);
> +
> +	return 0;
> +}
> +
> +static void rockchip_thermal_control(struct rockchip_thermal_data *data,
> +				     bool on)
> +{
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> +
> +	if (p_tsadc_data->control)
> +		p_tsadc_data->control(data->regs, on);
> +
> +	if (on) {
> +		data->irq_enabled = true;
> +		data->mode = THERMAL_DEVICE_ENABLED;
> +	} else {
> +		data->irq_enabled = false;
> +		data->mode = THERMAL_DEVICE_DISABLED;
> +	}
> +}
> +
> +static int rockchip_get_mode(struct thermal_zone_device *tz,
> +			     enum thermal_device_mode *mode)
> +{
> +	struct rockchip_thermal_data *data = tz->devdata;
> +
> +	*mode = data->mode;
> +
> +	return 0;
> +}
> +
> +static int rockchip_set_mode(struct thermal_zone_device *tz,
> +			     enum thermal_device_mode mode)
> +{
> +	struct rockchip_thermal_data *data = tz->devdata;
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> +
> +	if (mode == THERMAL_DEVICE_ENABLED) {
> +		tz->polling_delay = p_tsadc_data->polling_delay;
> +		tz->passive_delay = p_tsadc_data->passive_delay;
> +		if (!data->irq_enabled) {
> +			data->irq_enabled = true;
> +			enable_irq(data->irq);
> +		}
> +	} else {
> +		tz->polling_delay = 0;
> +		tz->passive_delay = 0;
> +		if (data->irq_enabled) {
> +			disable_irq(data->irq);
> +			data->irq_enabled = false;
> +		}
> +	}
> +
> +	data->mode = mode;
> +	thermal_zone_device_update(tz);
> +
> +	return 0;
> +}
> +
> +static int rockchip_get_trip_type(struct thermal_zone_device *tz, int trip,
> +				  enum thermal_trip_type *type)
> +{
> +	*type = (trip == ROCKCHIP_TRIP_PASSIVE) ? THERMAL_TRIP_PASSIVE :
> +		 THERMAL_TRIP_CRITICAL;
> +
> +	return 0;
> +}
> +
> +static int rockchip_get_crit_temp(struct thermal_zone_device *tz,
> +				  unsigned long *temp)
> +{
> +	struct rockchip_thermal_data *data = tz->devdata;
> +
> +	*temp = data->temp_critical;
> +
> +	return 0;
> +}
> +
> +static int rockchip_get_trip_temp(struct thermal_zone_device *tz, int trip,
> +				  unsigned long *temp)
> +{
> +	struct rockchip_thermal_data *data = tz->devdata;
> +
> +	*temp = (trip == ROCKCHIP_TRIP_PASSIVE) ? data->temp_passive :
> +		 data->temp_critical;
> +
> +	return 0;
> +}
> +
> +static int rockchip_set_trip_temp(struct thermal_zone_device *tz, int trip,
> +				  unsigned long temp)
> +{
> +	struct rockchip_thermal_data *data = tz->devdata;
> +
> +	if (trip == ROCKCHIP_TRIP_CRITICAL)
> +		return -EPERM;
> +
> +	data->temp_passive = temp;
> +	rockchip_set_alarm_temp(data, temp);
> +
> +	return 0;
> +}
> +
> +static int rockchip_bind(struct thermal_zone_device *tz,
> +			 struct thermal_cooling_device *cdev)
> +{
> +	int ret;
> +
> +	ret = thermal_zone_bind_cooling_device(tz, ROCKCHIP_TRIP_PASSIVE, cdev,
> +					       THERMAL_NO_LIMIT,
> +					       THERMAL_NO_LIMIT);
> +	if (ret) {
> +		dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
> +			tz->type, cdev->type, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rockchip_unbind(struct thermal_zone_device *tz,
> +			   struct thermal_cooling_device *cdev)
> +{
> +	int ret;
> +
> +	ret = thermal_zone_unbind_cooling_device(tz,
> +						 ROCKCHIP_TRIP_PASSIVE, cdev);
> +	if (ret) {
> +		dev_err(&tz->device,
> +			"unbinding zone %s with cdev %s failed:%d\n", tz->type,
> +			cdev->type, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

The method of binding and unbinding used above requires you to check if
you are binding to the right cdev. If in your system you register more
than one cdev, say someone loads the power supply core, which in turns
register a cooling device for charging, your thermal zone will bind it
to TRIP_PASSIVE. It will happen to any cooling device that eventually
gets registered to the thermal framework. Is that the desired outcome?

If not, you may want to compare the paramenter cdev to your data->cdev.

> +
> +static struct thermal_zone_device_ops rockchip_tz_ops = {
> +	.bind = rockchip_bind,
> +	.unbind = rockchip_unbind,
> +	.get_temp = rockchip_get_temp,
> +	.get_mode = rockchip_get_mode,
> +	.set_mode = rockchip_set_mode,
> +	.get_trip_type = rockchip_get_trip_type,
> +	.get_trip_temp = rockchip_get_trip_temp,
> +	.get_crit_temp = rockchip_get_crit_temp,
> +	.set_trip_temp = rockchip_set_trip_temp,
> +};
> +
> +static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
> +{
> +	struct rockchip_thermal_data *data = data;
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> +
> +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
> +		data->alarm_temp / 1000);
> +
> +	if (p_tsadc_data->irq_en && p_tsadc_data->irq_handle)
> +		p_tsadc_data->irq_handle(data->regs);
> +
> +	thermal_zone_device_update(data->tz);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int rockchip_thermal_probe(struct platform_device *pdev)
> +{
> +	struct rockchip_thermal_data *data;
> +	const struct rockchip_tsadc_platform_data *p_tsadc_data;
> +	struct cpumask clip_cpus;
> +	struct resource *res;
> +	const struct of_device_id *match;
> +	int ret, temp;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_thermal_data),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(data->regs)) {
> +		dev_err(&pdev->dev, "Could not get tsadc source, %p\n",
> +			data->regs);
> +		return PTR_ERR(data->regs);
> +	}
> +
> +	match = of_match_node(of_rockchip_thermal_match, pdev->dev.of_node);
> +	if (!match)
> +		return -ENXIO;
> +	data->pdata = (const struct rockchip_tsadc_platform_data *)match->data;
> +	if (!data->pdata)
> +		return -EINVAL;
> +	p_tsadc_data = data->pdata;
> +
> +	data->clk = devm_clk_get(&pdev->dev, "tsadc");
> +	if (IS_ERR(data->clk)) {
> +		dev_err(&pdev->dev, "failed to get tsadc clock\n");
> +		return PTR_ERR(data->clk);
> +	}
> +
> +	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
> +	if (IS_ERR(data->pclk)) {
> +		dev_err(&pdev->dev, "failed to get tsadc pclk\n");
> +		return PTR_ERR(data->pclk);
> +	}
> +
> +	/*
> +	* Use a default of 10KHz for the converter clock.
> +	* This may become user-configurable in the future.
> +	*/
> +	ret = clk_set_rate(data->clk, 10000);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to set tsadc clk rate, %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(data->clk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to enable converter clock\n");
> +		goto err_clk;
> +	}
> +
> +	ret = clk_prepare_enable(data->pclk);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to enable pclk\n");
> +		goto err_pclk;
> +	}
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "passive-temp", &temp)) {
> +		dev_warn(&pdev->dev,
> +			 "Missing default passive temp property in the DT.\n");
> +		data->temp_passive = p_tsadc_data->temp_passive;
> +	} else {
> +		data->temp_passive = temp;
> +	}
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "critical-temp", &temp)) {
> +		dev_warn(&pdev->dev,
> +			 "Missing default critical temp property in the DT.\n");
> +		data->temp_critical = p_tsadc_data->temp_critical;
> +	} else {
> +		data->temp_critical = temp;
> +	}
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "force-shut-temp", &temp)) {
> +		dev_warn(&pdev->dev,
> +			 "Missing default force shut down temp property in the DT.\n");
> +		data->temp_force_shut = p_tsadc_data->temp_force_shut;
> +	} else {
> +		data->temp_force_shut = temp;
> +	}
> +
> +	cpumask_set_cpu(0, &clip_cpus);
> +	data->cdev = of_cpufreq_cooling_register(pdev->dev.of_node, &clip_cpus);
> +	if (IS_ERR(data->cdev)) {
> +		dev_err(&pdev->dev, "failed to register cpufreq cooling device\n");
> +		goto disable_clk;
> +	}
> +
> +	data->tz = thermal_zone_device_register("rockchip_thermal",
> +						ROCKCHIP_TRIP_NUM,
> +						0, data,
> +						&rockchip_tz_ops, NULL,
> +						p_tsadc_data->passive_delay,
> +						p_tsadc_data->polling_delay);
> +
> +	if (IS_ERR(data->tz)) {
> +		dev_err(&pdev->dev, "failed to register thermal zone device\n");
> +		goto fail_cpufreq_register;
> +	}
> +
> +	if (p_tsadc_data->irq_en) {
> +		data->irq = platform_get_irq(pdev, 0);
> +		if (data->irq < 0) {
> +			dev_err(&pdev->dev, "no irq resource?\n");
> +			goto fail_irq;
> +		}
> +
> +		ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
> +				rockchip_thermal_alarm_irq_thread,
> +				IRQF_ONESHOT, "rockchip_thermal", data);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to request tsadc irq: %d\n", ret);
> +			goto fail_thermal_unregister;
> +		}
> +	}
> +
> +	rockchip_thermal_initialize(data);
> +	rockchip_thermal_control(data, true);
> +
> +	return 0;
> +
> +fail_thermal_unregister:
> +	thermal_zone_device_unregister(data->tz);
> +fail_irq:
> +fail_cpufreq_register:
> +	cpufreq_cooling_unregister(data->cdev);
> +disable_clk:
> +err_pclk:
> +	clk_disable_unprepare(data->pclk);
> +err_clk:
> +	clk_disable_unprepare(data->clk);
> +
> +	return ret;
> +}
> +
> +static int rockchip_thermal_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> +
> +	rockchip_thermal_control(data, false);
> +
> +	thermal_zone_device_unregister(data->tz);
> +	cpufreq_cooling_unregister(data->cdev);
> +	cpufreq_cooling_unregister(data->cdev);
> +

The above call is duplicated.

> +	clk_disable_unprepare(data->clk);
> +	clk_disable_unprepare(data->pclk);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int rockchip_thermal_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> +
> +	rockchip_thermal_control(data, false);
> +
> +	return 0;
> +}
> +
> +static int rockchip_thermal_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> +
> +	rockchip_thermal_initialize(data);
> +	rockchip_thermal_control(data, true);
> +
> +	return 0;
> +}

Would you need to manage your clocks too in the suspend resume path
(data->clk and data->pclk)?

> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(rockchip_thermal_pm_ops,
> +			 rockchip_thermal_suspend, rockchip_thermal_resume);
> +
> +static struct platform_driver rockchip_thermal_driver = {
> +	.driver = {
> +		   .name = "rockchip-thermal",
> +		   .owner = THIS_MODULE,
> +		   .pm = &rockchip_thermal_pm_ops,
> +		   .of_match_table = of_rockchip_thermal_match,
> +		   },
> +	.probe = rockchip_thermal_probe,
> +	.remove = rockchip_thermal_remove,
> +};
> +
> +module_platform_driver(rockchip_thermal_driver);
> +
> +MODULE_DESCRIPTION("ROCKCHIP THERMAL Driver");
> +MODULE_AUTHOR("Rockchip, Inc.");
> +MODULE_LICENSE("GPL");

Your file header states GPL version two. Thus I suppose you meant:
+MODULE_LICENSE("GPL v2");

right?

Check include/linux/module.h for further clarifications.

> +MODULE_ALIAS("platform:rockchip-thermal");
> -- 
> 1.9.1
> 
> 

BR,

Eduardo Valentin

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Caesar Wang]

Dear Eduardo,

I'm  sorry for it.
I just received this message.Maybe my mailbox has a problem.

Thank you for your comments.

? 2014?08?31? 04:09, Eduardo Valentin ??:
> Hello Ceasar,
>
> On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
>> Thermal is TS-ADC Controller module supports
>> user-defined mode and automatic mode.
>>
>> User-defined mode refers,TSADC all the control signals entirely by
>> software writing to register for direct control.
>>
>> Automaic mode refers to the module automatically poll TSADC output,
>> and the results were checked.If you find that the temperature High
>> in a period of time,an interrupt is generated to the processor
>> down-measures taken;if the temperature over a period of time High,
>> the resulting TSHUT gave CRU module,let it reset the entire chip,
>> or via GPIO give PMIC.
>>
>> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
>> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>> ---
>>   drivers/thermal/Kconfig            |   9 +
>>   drivers/thermal/Makefile           |   1 +
>>   drivers/thermal/rockchip_thermal.c | 669 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 679 insertions(+)
>>   create mode 100644 drivers/thermal/rockchip_thermal.c
>>
>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>> index f9a1386..a00aa1e 100644
>> --- a/drivers/thermal/Kconfig
>> +++ b/drivers/thermal/Kconfig
>> @@ -133,6 +133,15 @@ config SPEAR_THERMAL
>>   	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>>   	  thermal framework.
>>   
>> +config ROCKCHIP_THERMAL
>> +	tristate "Rockchip thermal driver"
>> +	depends on ARCH_ROCKCHIP
>> +	help
>> +	  Support for Temperature Sensor ADC (TS-ADC) found on Rockchip SoCs.
>> +	  It supports one critical trip point and one passive trip point.  The
>> +	  cpufreq is used as the cooling device to throttle CPUs when the
>> +	  passive trip is crossed.
>> +
>>   config RCAR_THERMAL
>>   	tristate "Renesas R-Car thermal driver"
>>   	depends on ARCH_SHMOBILE || COMPILE_TEST
>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>> index de0636a..b48b817 100644
>> --- a/drivers/thermal/Makefile
>> +++ b/drivers/thermal/Makefile
>> @@ -19,6 +19,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
>>   
>>   # platform thermal drivers
>>   obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>> +obj-$(CONFIG_ROCKCHIP_THERMAL) 	+= rockchip_thermal.o
>>   obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>>   obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>>   obj-y				+= samsung/
>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>> new file mode 100644
>> index 0000000..011f387
>> --- /dev/null
>> +++ b/drivers/thermal/rockchip_thermal.c
>> @@ -0,0 +1,669 @@
>> +/*
>> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> +*/
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/cpu_cooling.h>
>> +#include "thermal_core.h"
> Why do you need thermal_core.h here? Wouldn't linux/thermal.h be
> sufficient?
>

yes, I will use linux/thermal.h.
>> +
>> +enum rockchip_thermal_trip {
>> +	ROCKCHIP_TRIP_PASSIVE,
>> +	ROCKCHIP_TRIP_CRITICAL,
>> +	ROCKCHIP_TRIP_NUM,
>> +};
>> +
> Why do you need your own trip types?
>
>> +struct rockchip_thermal_data {
>> +	const struct rockchip_tsadc_platform_data *pdata;
>> +	struct thermal_zone_device *tz;
>> +	struct thermal_cooling_device *cdev;
>> +	enum thermal_device_mode mode;
>> +	void __iomem *regs;
>> +
>> +	signed long temp_passive;
>> +	signed long temp_critical;
>> +	signed long temp_force_shut;
>> +	signed long alarm_temp;
>> +	signed long last_temp;
>> +	bool irq_enabled;
>> +	int irq;
>> +	struct clk *clk;
>> +	struct clk *pclk;
>> +};
>> +
>> +struct rockchip_tsadc_platform_data {
>> +	u8 irq_en;
>> +	signed long temp_passive;
>> +	signed long temp_critical;
>> +	signed long temp_force_shut;
>> +	int passive_delay;
>> +	int polling_delay;
>> +
>> +	int (*irq_handle)(void __iomem *reg);
>> +	int (*initialize)(void __iomem *reg, signed long temp_force_shut);
>> +	int (*control)(void __iomem *reg, bool on);
>> +	u32 (*code_to_temp)(int temp);
>> +	u32 (*temp_to_code)(int temp);
>> +	void (*set_alarm_temp)(void __iomem *regs, signed long temp);
>> +};
>> +
>> +/*TSADC V2 Sensor info define:*/
>> +#define TSADCV2_AUTO_CON			0x04
>> +#define TSADCV2_INT_EN				0x08
>> +#define TSADCV2_INT_PD				0x0c
>> +#define TSADCV2_DATA1				0x24
>> +#define TSADCV2_COMP1_INT			0x34
>> +#define TSADCV2_COMP1_SHUT			0x44
>> +#define TSADCV2_AUTO_PERIOD			0x68
>> +#define TSADCV2_AUTO_PERIOD_HT			0x6c
>> +
>> +#define TSADCV2_AUTO_SRC1_EN			(1 << 5)
>> +#define TSADCV2_AUTO_EN				(1 << 0)
>> +#define TSADCV2_AUTO_DISABLE			~(1 << 0)
>> +#define TSADCV2_AUTO_STAS_BUSY			(1 << 16)
>> +#define TSADCV2_AUTO_STAS_BUSY_MASK		(1 << 16)
>> +#define TSADCV2_SHUT_2GPIO_SRC1_EN		(1 << 5)
>> +#define TSADCV2_INT_SRC1_EN			(1 << 1)
>> +#define TSADCV2_SHUT_SRC1_STATUS		(1 << 5)
>> +#define TSADCV2_INT_SRC1_STATUS			(1 << 1)
>> +
> The above bits can be defined with BIT() macro, right?

Yes. The Bit() will be more resonable..
>   Something like:
> +#define TSADCV2_INT_SRC1_STATUS			BIT(1)
>
>
>> +#define TSADCV2_DATA_MASK			0xfff
>> +#define TSADCV2_HIGHT_INT_DEBOUNCE		0x60
>> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE		0x64
>> +#define TSADCV2_HIGHT_INT_DEBOUNCE_TIME		0x0a
>> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME	0x0a
>> +#define TSADCV2_AUTO_PERIOD_TIME		0x03e8
>> +#define TSADCV2_AUTO_PERIOD_HT_TIME		0x64
>> +
>> +struct tsadc_table {
>> +	int code;
>> +	int temp;
>> +};
>> +
>> +static const struct tsadc_table v2_code_table[] = {
>> +	{TSADCV2_DATA_MASK, -40},
>> +	{3800, -40},
>> +	{3792, -35},
>> +	{3783, -30},
>> +	{3774, -25},
>> +	{3765, -20},
>> +	{3756, -15},
>> +	{3747, -10},
>> +	{3737, -5},
>> +	{3728, 0},
>> +	{3718, 5},
>> +	{3708, 10},
>> +	{3698, 15},
>> +	{3688, 20},
>> +	{3678, 25},
>> +	{3667, 30},
>> +	{3656, 35},
>> +	{3645, 40},
>> +	{3634, 45},
>> +	{3623, 50},
>> +	{3611, 55},
>> +	{3600, 60},
>> +	{3588, 65},
>> +	{3575, 70},
>> +	{3563, 75},
>> +	{3550, 80},
>> +	{3537, 85},
>> +	{3524, 90},
>> +	{3510, 95},
>> +	{3496, 100},
>> +	{3482, 105},
>> +	{3467, 110},
>> +	{3452, 115},
>> +	{3437, 120},
>> +	{3421, 125},
>> +	{0, 125},
>> +};
>> +
>> +static int rk_tsadcv2_irq_handle(void __iomem *regs)
>> +{
>> +	u32 val;
>> +
>> +	val = readl_relaxed(regs + TSADCV2_INT_PD);
>> +	writel_relaxed(val & ~(1 << 8), regs + TSADCV2_INT_PD);
> Why do you need to clear bit 8? Why hardcoded?

The bit 8 set 0 to clear the interrupt.

>> +
>> +	return 0;
>> +}
>> +
>> +static u32 rk_tsadcv2_temp_to_code(int temp)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
>> +		if (temp <= v2_code_table[i].temp && temp >
>> +		    v2_code_table[i - 1].temp)
>> +			return v2_code_table[i].code;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static u32 rk_tsadcv2_code_to_temp(int code)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
>> +		if (code <= v2_code_table[i].code && code >
>> +		    v2_code_table[i + 1].code){
>> +			return v2_code_table[i].temp;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> I know the table is not something too big, but considering it is ordered
> by either ADC value code or by temp, at least one of the above searching function
> may be more efficient, right?
Yes, use the point will be more efficient.
>> +
>> +static int rk_tsadcv2_initialize(void __iomem *regs,
>> +				 signed long temp_force_shut)
>> +{
>> +	int shutdown_value;
>> +
>> +	shutdown_value = rk_tsadcv2_temp_to_code(temp_force_shut);
>> +	/* Enable measurements at ~ 10 Hz */
> Does it leave the sampling clock at 10Hz?
yes.
> Is this clock exposed via
> clock framework?

The clock is divided by data->clk.
>
>> +	writel_relaxed(0, regs + TSADCV2_AUTO_CON);
>> +	writel_relaxed(TSADCV2_AUTO_PERIOD_TIME, regs + TSADCV2_AUTO_PERIOD);
>> +	writel_relaxed(TSADCV2_AUTO_PERIOD_HT_TIME, regs +
>> +		       TSADCV2_AUTO_PERIOD_HT);
>> +	writel_relaxed(shutdown_value, regs + TSADCV2_COMP1_SHUT);
>> +	writel_relaxed(TSADCV2_HIGHT_INT_DEBOUNCE_TIME, regs +
>> +		       TSADCV2_HIGHT_INT_DEBOUNCE);
>> +	writel_relaxed(TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME, regs +
>> +		       TSADCV2_HIGHT_TSHUT_DEBOUNCE);
>> +	writel_relaxed(TSADCV2_SHUT_2GPIO_SRC1_EN | TSADCV2_INT_SRC1_EN, regs +
>> +		       TSADCV2_INT_EN);
>> +	writel_relaxed(TSADCV2_AUTO_SRC1_EN | TSADCV2_AUTO_EN, regs +
>> +		       TSADCV2_AUTO_CON);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rk_tsadcv2_control(void __iomem *regs, bool on)
>> +{
>> +	u32 val;
>> +
>> +	if (on) {
>> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>> +		writel_relaxed(val | TSADCV2_AUTO_EN, regs + TSADCV2_AUTO_CON);
>> +	} else {
>> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>> +		writel_relaxed(val & TSADCV2_AUTO_DISABLE,
>> +			       regs + TSADCV2_AUTO_CON);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rk_tsadcv2_alarm_temp(void __iomem *regs, signed long alarm_temp)
>> +{
>> +	int alarm_value;
>> +
>> +	alarm_value = rk_tsadcv2_temp_to_code(alarm_temp);
>> +	writel_relaxed(alarm_value, regs + TSADCV2_COMP1_INT);
>> +}
>> +
>> +struct rockchip_tsadc_platform_data const rk3288_tsadc_data = {
>> +	.irq_en = 1,
>> +	.temp_passive = 85000,
>> +	.temp_critical = 100000,
>> +	.temp_force_shut = 120000,
>> +	.passive_delay = 2000,
>> +	.polling_delay = 1000,
>> +	.irq_handle = rk_tsadcv2_irq_handle,
>> +	.initialize = rk_tsadcv2_initialize,
>> +	.control = rk_tsadcv2_control,
>> +	.code_to_temp = rk_tsadcv2_code_to_temp,
>> +	.temp_to_code = rk_tsadcv2_temp_to_code,
>> +	.set_alarm_temp = rk_tsadcv2_alarm_temp,
>> +};
> shall the above struct be also static?
OK.
>> +
>> +static const struct of_device_id of_rockchip_thermal_match[] = {
>> +	{
>> +		.compatible = "rockchip,rk3288-tsadc",
>> +		.data = (void *)&rk3288_tsadc_data,
>> +	},
>> +	{ /* end */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
>> +
>> +static void rockchip_set_alarm_temp(struct rockchip_thermal_data *data,
>> +				    signed long alarm_temp)
>> +{
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>> +
>> +	data->alarm_temp = alarm_temp;
>> +	if (p_tsadc_data->set_alarm_temp)
>> +		p_tsadc_data->set_alarm_temp(data->regs, alarm_temp);
>> +}
>> +
>> +static int rockchip_get_temp(struct thermal_zone_device *tz,
>> +			     unsigned long *temp)
>> +{
>> +	struct rockchip_thermal_data *data = tz->devdata;
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>> +	u32 val;
>> +
>> +	val = readl_relaxed(data->regs + TSADCV2_DATA1);
>> +	*temp = p_tsadc_data->code_to_temp(val);
>> +
>> +	/* Update alarm value to next higher trip point */
>> +	if (data->alarm_temp == data->temp_passive && *temp >=
>> +	    data->temp_passive)
>> +		rockchip_set_alarm_temp(data, data->temp_critical);
>> +
>> +	if (data->alarm_temp == data->temp_critical && *temp <
>> +	    data->temp_passive) {
>> +		rockchip_set_alarm_temp(data, data->temp_passive);
>> +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
>> +			data->alarm_temp / 1000);
>> +	}
>> +
>> +	if (*temp != data->last_temp) {
>> +		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
>> +		data->last_temp = *temp;
>> +	}
>> +
>> +	/* Reenable alarm IRQ if temperature below alarm temperature */
>> +	if (!data->irq_enabled && *temp < data->alarm_temp) {
>> +		data->irq_enabled = true;
>> +		enable_irq(data->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_thermal_initialize(struct rockchip_thermal_data *data)
>> +{
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>> +
>> +	if (p_tsadc_data->initialize)
>> +		p_tsadc_data->initialize(data->regs, data->temp_force_shut);
>> +	rockchip_set_alarm_temp(data, data->temp_passive);
>> +
>> +	return 0;
>> +}
>> +
>> +static void rockchip_thermal_control(struct rockchip_thermal_data *data,
>> +				     bool on)
>> +{
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>> +
>> +	if (p_tsadc_data->control)
>> +		p_tsadc_data->control(data->regs, on);
>> +
>> +	if (on) {
>> +		data->irq_enabled = true;
>> +		data->mode = THERMAL_DEVICE_ENABLED;
>> +	} else {
>> +		data->irq_enabled = false;
>> +		data->mode = THERMAL_DEVICE_DISABLED;
>> +	}
>> +}
>> +
>> +static int rockchip_get_mode(struct thermal_zone_device *tz,
>> +			     enum thermal_device_mode *mode)
>> +{
>> +	struct rockchip_thermal_data *data = tz->devdata;
>> +
>> +	*mode = data->mode;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_set_mode(struct thermal_zone_device *tz,
>> +			     enum thermal_device_mode mode)
>> +{
>> +	struct rockchip_thermal_data *data = tz->devdata;
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>> +
>> +	if (mode == THERMAL_DEVICE_ENABLED) {
>> +		tz->polling_delay = p_tsadc_data->polling_delay;
>> +		tz->passive_delay = p_tsadc_data->passive_delay;
>> +		if (!data->irq_enabled) {
>> +			data->irq_enabled = true;
>> +			enable_irq(data->irq);
>> +		}
>> +	} else {
>> +		tz->polling_delay = 0;
>> +		tz->passive_delay = 0;
>> +		if (data->irq_enabled) {
>> +			disable_irq(data->irq);
>> +			data->irq_enabled = false;
>> +		}
>> +	}
>> +
>> +	data->mode = mode;
>> +	thermal_zone_device_update(tz);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_get_trip_type(struct thermal_zone_device *tz, int trip,
>> +				  enum thermal_trip_type *type)
>> +{
>> +	*type = (trip == ROCKCHIP_TRIP_PASSIVE) ? THERMAL_TRIP_PASSIVE :
>> +		 THERMAL_TRIP_CRITICAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_get_crit_temp(struct thermal_zone_device *tz,
>> +				  unsigned long *temp)
>> +{
>> +	struct rockchip_thermal_data *data = tz->devdata;
>> +
>> +	*temp = data->temp_critical;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_get_trip_temp(struct thermal_zone_device *tz, int trip,
>> +				  unsigned long *temp)
>> +{
>> +	struct rockchip_thermal_data *data = tz->devdata;
>> +
>> +	*temp = (trip == ROCKCHIP_TRIP_PASSIVE) ? data->temp_passive :
>> +		 data->temp_critical;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_set_trip_temp(struct thermal_zone_device *tz, int trip,
>> +				  unsigned long temp)
>> +{
>> +	struct rockchip_thermal_data *data = tz->devdata;
>> +
>> +	if (trip == ROCKCHIP_TRIP_CRITICAL)
>> +		return -EPERM;
>> +
>> +	data->temp_passive = temp;
>> +	rockchip_set_alarm_temp(data, temp);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_bind(struct thermal_zone_device *tz,
>> +			 struct thermal_cooling_device *cdev)
>> +{
>> +	int ret;
>> +
>> +	ret = thermal_zone_bind_cooling_device(tz, ROCKCHIP_TRIP_PASSIVE, cdev,
>> +					       THERMAL_NO_LIMIT,
>> +					       THERMAL_NO_LIMIT);
>> +	if (ret) {
>> +		dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
>> +			tz->type, cdev->type, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_unbind(struct thermal_zone_device *tz,
>> +			   struct thermal_cooling_device *cdev)
>> +{
>> +	int ret;
>> +
>> +	ret = thermal_zone_unbind_cooling_device(tz,
>> +						 ROCKCHIP_TRIP_PASSIVE, cdev);
>> +	if (ret) {
>> +		dev_err(&tz->device,
>> +			"unbinding zone %s with cdev %s failed:%d\n", tz->type,
>> +			cdev->type, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> The method of binding and unbinding used above requires you to check if
> you are binding to the right cdev. If in your system you register more
> than one cdev, say someone loads the power supply core, which in turns
> register a cooling device for charging, your thermal zone will bind it
> to TRIP_PASSIVE. It will happen to any cooling device that eventually
> gets registered to the thermal framework. Is that the desired outcome?

Yes,I will use the generic trip points in the dts and fix it in the 
thermal driver.
> If not, you may want to compare the paramenter cdev to your data->cdev.
>
>> +
>> +static struct thermal_zone_device_ops rockchip_tz_ops = {
>> +	.bind = rockchip_bind,
>> +	.unbind = rockchip_unbind,
>> +	.get_temp = rockchip_get_temp,
>> +	.get_mode = rockchip_get_mode,
>> +	.set_mode = rockchip_set_mode,
>> +	.get_trip_type = rockchip_get_trip_type,
>> +	.get_trip_temp = rockchip_get_trip_temp,
>> +	.get_crit_temp = rockchip_get_crit_temp,
>> +	.set_trip_temp = rockchip_set_trip_temp,
>> +};
>> +
>> +static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
>> +{
>> +	struct rockchip_thermal_data *data = data;
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>> +
>> +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
>> +		data->alarm_temp / 1000);
>> +
>> +	if (p_tsadc_data->irq_en && p_tsadc_data->irq_handle)
>> +		p_tsadc_data->irq_handle(data->regs);
>> +
>> +	thermal_zone_device_update(data->tz);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int rockchip_thermal_probe(struct platform_device *pdev)
>> +{
>> +	struct rockchip_thermal_data *data;
>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data;
>> +	struct cpumask clip_cpus;
>> +	struct resource *res;
>> +	const struct of_device_id *match;
>> +	int ret, temp;
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_thermal_data),
>> +			    GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(data->regs)) {
>> +		dev_err(&pdev->dev, "Could not get tsadc source, %p\n",
>> +			data->regs);
>> +		return PTR_ERR(data->regs);
>> +	}
>> +
>> +	match = of_match_node(of_rockchip_thermal_match, pdev->dev.of_node);
>> +	if (!match)
>> +		return -ENXIO;
>> +	data->pdata = (const struct rockchip_tsadc_platform_data *)match->data;
>> +	if (!data->pdata)
>> +		return -EINVAL;
>> +	p_tsadc_data = data->pdata;
>> +
>> +	data->clk = devm_clk_get(&pdev->dev, "tsadc");
>> +	if (IS_ERR(data->clk)) {
>> +		dev_err(&pdev->dev, "failed to get tsadc clock\n");
>> +		return PTR_ERR(data->clk);
>> +	}
>> +
>> +	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
>> +	if (IS_ERR(data->pclk)) {
>> +		dev_err(&pdev->dev, "failed to get tsadc pclk\n");
>> +		return PTR_ERR(data->pclk);
>> +	}
>> +
>> +	/*
>> +	* Use a default of 10KHz for the converter clock.
>> +	* This may become user-configurable in the future.
>> +	*/
>> +	ret = clk_set_rate(data->clk, 10000);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to set tsadc clk rate, %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(data->clk);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to enable converter clock\n");
>> +		goto err_clk;
>> +	}
>> +
>> +	ret = clk_prepare_enable(data->pclk);
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to enable pclk\n");
>> +		goto err_pclk;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, data);
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "passive-temp", &temp)) {
>> +		dev_warn(&pdev->dev,
>> +			 "Missing default passive temp property in the DT.\n");
>> +		data->temp_passive = p_tsadc_data->temp_passive;
>> +	} else {
>> +		data->temp_passive = temp;
>> +	}
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "critical-temp", &temp)) {
>> +		dev_warn(&pdev->dev,
>> +			 "Missing default critical temp property in the DT.\n");
>> +		data->temp_critical = p_tsadc_data->temp_critical;
>> +	} else {
>> +		data->temp_critical = temp;
>> +	}
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "force-shut-temp", &temp)) {
>> +		dev_warn(&pdev->dev,
>> +			 "Missing default force shut down temp property in the DT.\n");
>> +		data->temp_force_shut = p_tsadc_data->temp_force_shut;
>> +	} else {
>> +		data->temp_force_shut = temp;
>> +	}
>> +
>> +	cpumask_set_cpu(0, &clip_cpus);
>> +	data->cdev = of_cpufreq_cooling_register(pdev->dev.of_node, &clip_cpus);
>> +	if (IS_ERR(data->cdev)) {
>> +		dev_err(&pdev->dev, "failed to register cpufreq cooling device\n");
>> +		goto disable_clk;
>> +	}
>> +
>> +	data->tz = thermal_zone_device_register("rockchip_thermal",
>> +						ROCKCHIP_TRIP_NUM,
>> +						0, data,
>> +						&rockchip_tz_ops, NULL,
>> +						p_tsadc_data->passive_delay,
>> +						p_tsadc_data->polling_delay);
>> +
>> +	if (IS_ERR(data->tz)) {
>> +		dev_err(&pdev->dev, "failed to register thermal zone device\n");
>> +		goto fail_cpufreq_register;
>> +	}
>> +
>> +	if (p_tsadc_data->irq_en) {
>> +		data->irq = platform_get_irq(pdev, 0);
>> +		if (data->irq < 0) {
>> +			dev_err(&pdev->dev, "no irq resource?\n");
>> +			goto fail_irq;
>> +		}
>> +
>> +		ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
>> +				rockchip_thermal_alarm_irq_thread,
>> +				IRQF_ONESHOT, "rockchip_thermal", data);
>> +		if (ret < 0) {
>> +			dev_err(&pdev->dev,
>> +				"failed to request tsadc irq: %d\n", ret);
>> +			goto fail_thermal_unregister;
>> +		}
>> +	}
>> +
>> +	rockchip_thermal_initialize(data);
>> +	rockchip_thermal_control(data, true);
>> +
>> +	return 0;
>> +
>> +fail_thermal_unregister:
>> +	thermal_zone_device_unregister(data->tz);
>> +fail_irq:
>> +fail_cpufreq_register:
>> +	cpufreq_cooling_unregister(data->cdev);
>> +disable_clk:
>> +err_pclk:
>> +	clk_disable_unprepare(data->pclk);
>> +err_clk:
>> +	clk_disable_unprepare(data->clk);
>> +
>> +	return ret;
>> +}
>> +
>> +static int rockchip_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>> +
>> +	rockchip_thermal_control(data, false);
>> +
>> +	thermal_zone_device_unregister(data->tz);
>> +	cpufreq_cooling_unregister(data->cdev);
>> +	cpufreq_cooling_unregister(data->cdev);
>> +
> The above call is duplicated.

OK.
>> +	clk_disable_unprepare(data->clk);
>> +	clk_disable_unprepare(data->pclk);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int rockchip_thermal_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>> +
>> +	rockchip_thermal_control(data, false);
>> +
>> +	return 0;
>> +}
>> +
>> +static int rockchip_thermal_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>> +
>> +	rockchip_thermal_initialize(data);
>> +	rockchip_thermal_control(data, true);
>> +
>> +	return 0;
>> +}
> Would you need to manage your clocks too in the suspend resume path
> (data->clk and data->pclk)?
yes.
Usually,it's need disable to save power.
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(rockchip_thermal_pm_ops,
>> +			 rockchip_thermal_suspend, rockchip_thermal_resume);
>> +
>> +static struct platform_driver rockchip_thermal_driver = {
>> +	.driver = {
>> +		   .name = "rockchip-thermal",
>> +		   .owner = THIS_MODULE,
>> +		   .pm = &rockchip_thermal_pm_ops,
>> +		   .of_match_table = of_rockchip_thermal_match,
>> +		   },
>> +	.probe = rockchip_thermal_probe,
>> +	.remove = rockchip_thermal_remove,
>> +};
>> +
>> +module_platform_driver(rockchip_thermal_driver);
>> +
>> +MODULE_DESCRIPTION("ROCKCHIP THERMAL Driver");
>> +MODULE_AUTHOR("Rockchip, Inc.");
>> +MODULE_LICENSE("GPL");
> Your file header states GPL version two. Thus I suppose you meant:
> +MODULE_LICENSE("GPL v2");
>
> right?
>
> Check include/linux/module.h for further clarifications.

But I think the  "GPL"  is support for thr GNU Public License v2 or later.
I will fix GPL v2 if I get it wrong.

>> +MODULE_ALIAS("platform:rockchip-thermal");
>> -- 
>> 1.9.1
>>
>>
> BR,
>
> Eduardo Valentin
>
>
>

-- 
Best regards,
Caesar

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Eduardo Valentin]

Hello Caesar,

On Wed, Sep 10, 2014 at 12:39:07PM +0800, Caesar Wang wrote:
> Dear Eduardo,
> 
> I'm  sorry for it.
> I just received this message.Maybe my mailbox has a problem.


No problems. You can take your time.

> 
> Thank you for your comments.
> 
> ? 2014?08?31? 04:09, Eduardo Valentin ??:
> > Hello Ceasar,
> >
> > On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
> >> Thermal is TS-ADC Controller module supports
> >> user-defined mode and automatic mode.
> >>
> >> User-defined mode refers,TSADC all the control signals entirely by
> >> software writing to register for direct control.
> >>
> >> Automaic mode refers to the module automatically poll TSADC output,
> >> and the results were checked.If you find that the temperature High
> >> in a period of time,an interrupt is generated to the processor
> >> down-measures taken;if the temperature over a period of time High,
> >> the resulting TSHUT gave CRU module,let it reset the entire chip,
> >> or via GPIO give PMIC.
> >>
> >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>

I forgot to ask, is zhaoyifeng the correct name or is it a nickname? 

> >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> >> ---
> >>   drivers/thermal/Kconfig            |   9 +
> >>   drivers/thermal/Makefile           |   1 +
> >>   drivers/thermal/rockchip_thermal.c | 669 +++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 679 insertions(+)
> >>   create mode 100644 drivers/thermal/rockchip_thermal.c
> >>
> >> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> >> index f9a1386..a00aa1e 100644
> >> --- a/drivers/thermal/Kconfig
> >> +++ b/drivers/thermal/Kconfig
> >> @@ -133,6 +133,15 @@ config SPEAR_THERMAL
> >>   	  Enable this to plug the SPEAr thermal sensor driver into the Linux
> >>   	  thermal framework.
> >>   
> >> +config ROCKCHIP_THERMAL
> >> +	tristate "Rockchip thermal driver"
> >> +	depends on ARCH_ROCKCHIP
> >> +	help
> >> +	  Support for Temperature Sensor ADC (TS-ADC) found on Rockchip SoCs.
> >> +	  It supports one critical trip point and one passive trip point.  The
> >> +	  cpufreq is used as the cooling device to throttle CPUs when the
> >> +	  passive trip is crossed.
> >> +
> >>   config RCAR_THERMAL
> >>   	tristate "Renesas R-Car thermal driver"
> >>   	depends on ARCH_SHMOBILE || COMPILE_TEST
> >> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> >> index de0636a..b48b817 100644
> >> --- a/drivers/thermal/Makefile
> >> +++ b/drivers/thermal/Makefile
> >> @@ -19,6 +19,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
> >>   
> >>   # platform thermal drivers
> >>   obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
> >> +obj-$(CONFIG_ROCKCHIP_THERMAL) 	+= rockchip_thermal.o
> >>   obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
> >>   obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
> >>   obj-y				+= samsung/
> >> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> >> new file mode 100644
> >> index 0000000..011f387
> >> --- /dev/null
> >> +++ b/drivers/thermal/rockchip_thermal.c
> >> @@ -0,0 +1,669 @@
> >> +/*
> >> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> >> + * more details.
> >> + *
> >> +*/
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/io.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regulator/consumer.h>
> >> +#include <linux/cpu_cooling.h>
> >> +#include "thermal_core.h"
> > Why do you need thermal_core.h here? Wouldn't linux/thermal.h be
> > sufficient?
> >
> 
> yes, I will use linux/thermal.h.
> >> +
> >> +enum rockchip_thermal_trip {
> >> +	ROCKCHIP_TRIP_PASSIVE,
> >> +	ROCKCHIP_TRIP_CRITICAL,
> >> +	ROCKCHIP_TRIP_NUM,
> >> +};
> >> +
> > Why do you need your own trip types?
> >
> >> +struct rockchip_thermal_data {
> >> +	const struct rockchip_tsadc_platform_data *pdata;
> >> +	struct thermal_zone_device *tz;
> >> +	struct thermal_cooling_device *cdev;
> >> +	enum thermal_device_mode mode;
> >> +	void __iomem *regs;
> >> +
> >> +	signed long temp_passive;
> >> +	signed long temp_critical;
> >> +	signed long temp_force_shut;
> >> +	signed long alarm_temp;
> >> +	signed long last_temp;
> >> +	bool irq_enabled;
> >> +	int irq;
> >> +	struct clk *clk;
> >> +	struct clk *pclk;
> >> +};
> >> +
> >> +struct rockchip_tsadc_platform_data {
> >> +	u8 irq_en;
> >> +	signed long temp_passive;
> >> +	signed long temp_critical;
> >> +	signed long temp_force_shut;
> >> +	int passive_delay;
> >> +	int polling_delay;
> >> +
> >> +	int (*irq_handle)(void __iomem *reg);
> >> +	int (*initialize)(void __iomem *reg, signed long temp_force_shut);
> >> +	int (*control)(void __iomem *reg, bool on);
> >> +	u32 (*code_to_temp)(int temp);
> >> +	u32 (*temp_to_code)(int temp);
> >> +	void (*set_alarm_temp)(void __iomem *regs, signed long temp);
> >> +};
> >> +
> >> +/*TSADC V2 Sensor info define:*/
> >> +#define TSADCV2_AUTO_CON			0x04
> >> +#define TSADCV2_INT_EN				0x08
> >> +#define TSADCV2_INT_PD				0x0c
> >> +#define TSADCV2_DATA1				0x24
> >> +#define TSADCV2_COMP1_INT			0x34
> >> +#define TSADCV2_COMP1_SHUT			0x44
> >> +#define TSADCV2_AUTO_PERIOD			0x68
> >> +#define TSADCV2_AUTO_PERIOD_HT			0x6c
> >> +
> >> +#define TSADCV2_AUTO_SRC1_EN			(1 << 5)
> >> +#define TSADCV2_AUTO_EN				(1 << 0)
> >> +#define TSADCV2_AUTO_DISABLE			~(1 << 0)
> >> +#define TSADCV2_AUTO_STAS_BUSY			(1 << 16)
> >> +#define TSADCV2_AUTO_STAS_BUSY_MASK		(1 << 16)
> >> +#define TSADCV2_SHUT_2GPIO_SRC1_EN		(1 << 5)
> >> +#define TSADCV2_INT_SRC1_EN			(1 << 1)
> >> +#define TSADCV2_SHUT_SRC1_STATUS		(1 << 5)
> >> +#define TSADCV2_INT_SRC1_STATUS			(1 << 1)
> >> +
> > The above bits can be defined with BIT() macro, right?
> 
> Yes. The Bit() will be more resonable..
> >   Something like:
> > +#define TSADCV2_INT_SRC1_STATUS			BIT(1)
> >
> >
> >> +#define TSADCV2_DATA_MASK			0xfff
> >> +#define TSADCV2_HIGHT_INT_DEBOUNCE		0x60
> >> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE		0x64
> >> +#define TSADCV2_HIGHT_INT_DEBOUNCE_TIME		0x0a
> >> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME	0x0a
> >> +#define TSADCV2_AUTO_PERIOD_TIME		0x03e8
> >> +#define TSADCV2_AUTO_PERIOD_HT_TIME		0x64
> >> +
> >> +struct tsadc_table {
> >> +	int code;
> >> +	int temp;
> >> +};
> >> +
> >> +static const struct tsadc_table v2_code_table[] = {
> >> +	{TSADCV2_DATA_MASK, -40},
> >> +	{3800, -40},
> >> +	{3792, -35},
> >> +	{3783, -30},
> >> +	{3774, -25},
> >> +	{3765, -20},
> >> +	{3756, -15},
> >> +	{3747, -10},
> >> +	{3737, -5},
> >> +	{3728, 0},
> >> +	{3718, 5},
> >> +	{3708, 10},
> >> +	{3698, 15},
> >> +	{3688, 20},
> >> +	{3678, 25},
> >> +	{3667, 30},
> >> +	{3656, 35},
> >> +	{3645, 40},
> >> +	{3634, 45},
> >> +	{3623, 50},
> >> +	{3611, 55},
> >> +	{3600, 60},
> >> +	{3588, 65},
> >> +	{3575, 70},
> >> +	{3563, 75},
> >> +	{3550, 80},
> >> +	{3537, 85},
> >> +	{3524, 90},
> >> +	{3510, 95},
> >> +	{3496, 100},
> >> +	{3482, 105},
> >> +	{3467, 110},
> >> +	{3452, 115},
> >> +	{3437, 120},
> >> +	{3421, 125},
> >> +	{0, 125},
> >> +};
> >> +
> >> +static int rk_tsadcv2_irq_handle(void __iomem *regs)
> >> +{
> >> +	u32 val;
> >> +
> >> +	val = readl_relaxed(regs + TSADCV2_INT_PD);
> >> +	writel_relaxed(val & ~(1 << 8), regs + TSADCV2_INT_PD);
> > Why do you need to clear bit 8? Why hardcoded?
> 
> The bit 8 set 0 to clear the interrupt.
> 

Would it make sense to create a macro for this?

> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static u32 rk_tsadcv2_temp_to_code(int temp)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
> >> +		if (temp <= v2_code_table[i].temp && temp >
> >> +		    v2_code_table[i - 1].temp)
> >> +			return v2_code_table[i].code;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static u32 rk_tsadcv2_code_to_temp(int code)
> >> +{
> >> +	int i;
> >> +
> >> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
> >> +		if (code <= v2_code_table[i].code && code >
> >> +		    v2_code_table[i + 1].code){
> >> +			return v2_code_table[i].temp;
> >> +		}
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > I know the table is not something too big, but considering it is ordered
> > by either ADC value code or by temp, at least one of the above searching function
> > may be more efficient, right?
> Yes, use the point will be more efficient.
> >> +
> >> +static int rk_tsadcv2_initialize(void __iomem *regs,
> >> +				 signed long temp_force_shut)
> >> +{
> >> +	int shutdown_value;
> >> +
> >> +	shutdown_value = rk_tsadcv2_temp_to_code(temp_force_shut);
> >> +	/* Enable measurements at ~ 10 Hz */
> > Does it leave the sampling clock at 10Hz?
> yes.

ok


> > Is this clock exposed via
> > clock framework?
> 
> The clock is divided by data->clk.

ok

> >
> >> +	writel_relaxed(0, regs + TSADCV2_AUTO_CON);
> >> +	writel_relaxed(TSADCV2_AUTO_PERIOD_TIME, regs + TSADCV2_AUTO_PERIOD);
> >> +	writel_relaxed(TSADCV2_AUTO_PERIOD_HT_TIME, regs +
> >> +		       TSADCV2_AUTO_PERIOD_HT);
> >> +	writel_relaxed(shutdown_value, regs + TSADCV2_COMP1_SHUT);
> >> +	writel_relaxed(TSADCV2_HIGHT_INT_DEBOUNCE_TIME, regs +
> >> +		       TSADCV2_HIGHT_INT_DEBOUNCE);
> >> +	writel_relaxed(TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME, regs +
> >> +		       TSADCV2_HIGHT_TSHUT_DEBOUNCE);
> >> +	writel_relaxed(TSADCV2_SHUT_2GPIO_SRC1_EN | TSADCV2_INT_SRC1_EN, regs +
> >> +		       TSADCV2_INT_EN);
> >> +	writel_relaxed(TSADCV2_AUTO_SRC1_EN | TSADCV2_AUTO_EN, regs +
> >> +		       TSADCV2_AUTO_CON);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rk_tsadcv2_control(void __iomem *regs, bool on)
> >> +{
> >> +	u32 val;
> >> +
> >> +	if (on) {
> >> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
> >> +		writel_relaxed(val | TSADCV2_AUTO_EN, regs + TSADCV2_AUTO_CON);
> >> +	} else {
> >> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
> >> +		writel_relaxed(val & TSADCV2_AUTO_DISABLE,
> >> +			       regs + TSADCV2_AUTO_CON);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void rk_tsadcv2_alarm_temp(void __iomem *regs, signed long alarm_temp)
> >> +{
> >> +	int alarm_value;
> >> +
> >> +	alarm_value = rk_tsadcv2_temp_to_code(alarm_temp);
> >> +	writel_relaxed(alarm_value, regs + TSADCV2_COMP1_INT);
> >> +}
> >> +
> >> +struct rockchip_tsadc_platform_data const rk3288_tsadc_data = {
> >> +	.irq_en = 1,
> >> +	.temp_passive = 85000,
> >> +	.temp_critical = 100000,
> >> +	.temp_force_shut = 120000,
> >> +	.passive_delay = 2000,
> >> +	.polling_delay = 1000,
> >> +	.irq_handle = rk_tsadcv2_irq_handle,
> >> +	.initialize = rk_tsadcv2_initialize,
> >> +	.control = rk_tsadcv2_control,
> >> +	.code_to_temp = rk_tsadcv2_code_to_temp,
> >> +	.temp_to_code = rk_tsadcv2_temp_to_code,
> >> +	.set_alarm_temp = rk_tsadcv2_alarm_temp,
> >> +};
> > shall the above struct be also static?
> OK.
> >> +
> >> +static const struct of_device_id of_rockchip_thermal_match[] = {
> >> +	{
> >> +		.compatible = "rockchip,rk3288-tsadc",
> >> +		.data = (void *)&rk3288_tsadc_data,
> >> +	},
> >> +	{ /* end */ },
> >> +};
> >> +MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
> >> +
> >> +static void rockchip_set_alarm_temp(struct rockchip_thermal_data *data,
> >> +				    signed long alarm_temp)
> >> +{
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> >> +
> >> +	data->alarm_temp = alarm_temp;
> >> +	if (p_tsadc_data->set_alarm_temp)
> >> +		p_tsadc_data->set_alarm_temp(data->regs, alarm_temp);
> >> +}
> >> +
> >> +static int rockchip_get_temp(struct thermal_zone_device *tz,
> >> +			     unsigned long *temp)
> >> +{
> >> +	struct rockchip_thermal_data *data = tz->devdata;
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> >> +	u32 val;
> >> +
> >> +	val = readl_relaxed(data->regs + TSADCV2_DATA1);
> >> +	*temp = p_tsadc_data->code_to_temp(val);
> >> +
> >> +	/* Update alarm value to next higher trip point */
> >> +	if (data->alarm_temp == data->temp_passive && *temp >=
> >> +	    data->temp_passive)
> >> +		rockchip_set_alarm_temp(data, data->temp_critical);
> >> +
> >> +	if (data->alarm_temp == data->temp_critical && *temp <
> >> +	    data->temp_passive) {
> >> +		rockchip_set_alarm_temp(data, data->temp_passive);
> >> +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
> >> +			data->alarm_temp / 1000);
> >> +	}
> >> +
> >> +	if (*temp != data->last_temp) {
> >> +		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
> >> +		data->last_temp = *temp;
> >> +	}
> >> +
> >> +	/* Reenable alarm IRQ if temperature below alarm temperature */
> >> +	if (!data->irq_enabled && *temp < data->alarm_temp) {
> >> +		data->irq_enabled = true;
> >> +		enable_irq(data->irq);
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_thermal_initialize(struct rockchip_thermal_data *data)
> >> +{
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> >> +
> >> +	if (p_tsadc_data->initialize)
> >> +		p_tsadc_data->initialize(data->regs, data->temp_force_shut);
> >> +	rockchip_set_alarm_temp(data, data->temp_passive);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void rockchip_thermal_control(struct rockchip_thermal_data *data,
> >> +				     bool on)
> >> +{
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> >> +
> >> +	if (p_tsadc_data->control)
> >> +		p_tsadc_data->control(data->regs, on);
> >> +
> >> +	if (on) {
> >> +		data->irq_enabled = true;
> >> +		data->mode = THERMAL_DEVICE_ENABLED;
> >> +	} else {
> >> +		data->irq_enabled = false;
> >> +		data->mode = THERMAL_DEVICE_DISABLED;
> >> +	}
> >> +}
> >> +
> >> +static int rockchip_get_mode(struct thermal_zone_device *tz,
> >> +			     enum thermal_device_mode *mode)
> >> +{
> >> +	struct rockchip_thermal_data *data = tz->devdata;
> >> +
> >> +	*mode = data->mode;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_set_mode(struct thermal_zone_device *tz,
> >> +			     enum thermal_device_mode mode)
> >> +{
> >> +	struct rockchip_thermal_data *data = tz->devdata;
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> >> +
> >> +	if (mode == THERMAL_DEVICE_ENABLED) {
> >> +		tz->polling_delay = p_tsadc_data->polling_delay;
> >> +		tz->passive_delay = p_tsadc_data->passive_delay;
> >> +		if (!data->irq_enabled) {
> >> +			data->irq_enabled = true;
> >> +			enable_irq(data->irq);
> >> +		}
> >> +	} else {
> >> +		tz->polling_delay = 0;
> >> +		tz->passive_delay = 0;
> >> +		if (data->irq_enabled) {
> >> +			disable_irq(data->irq);
> >> +			data->irq_enabled = false;
> >> +		}
> >> +	}
> >> +
> >> +	data->mode = mode;
> >> +	thermal_zone_device_update(tz);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_get_trip_type(struct thermal_zone_device *tz, int trip,
> >> +				  enum thermal_trip_type *type)
> >> +{
> >> +	*type = (trip == ROCKCHIP_TRIP_PASSIVE) ? THERMAL_TRIP_PASSIVE :
> >> +		 THERMAL_TRIP_CRITICAL;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_get_crit_temp(struct thermal_zone_device *tz,
> >> +				  unsigned long *temp)
> >> +{
> >> +	struct rockchip_thermal_data *data = tz->devdata;
> >> +
> >> +	*temp = data->temp_critical;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_get_trip_temp(struct thermal_zone_device *tz, int trip,
> >> +				  unsigned long *temp)
> >> +{
> >> +	struct rockchip_thermal_data *data = tz->devdata;
> >> +
> >> +	*temp = (trip == ROCKCHIP_TRIP_PASSIVE) ? data->temp_passive :
> >> +		 data->temp_critical;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_set_trip_temp(struct thermal_zone_device *tz, int trip,
> >> +				  unsigned long temp)
> >> +{
> >> +	struct rockchip_thermal_data *data = tz->devdata;
> >> +
> >> +	if (trip == ROCKCHIP_TRIP_CRITICAL)
> >> +		return -EPERM;
> >> +
> >> +	data->temp_passive = temp;
> >> +	rockchip_set_alarm_temp(data, temp);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_bind(struct thermal_zone_device *tz,
> >> +			 struct thermal_cooling_device *cdev)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = thermal_zone_bind_cooling_device(tz, ROCKCHIP_TRIP_PASSIVE, cdev,
> >> +					       THERMAL_NO_LIMIT,
> >> +					       THERMAL_NO_LIMIT);
> >> +	if (ret) {
> >> +		dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
> >> +			tz->type, cdev->type, ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_unbind(struct thermal_zone_device *tz,
> >> +			   struct thermal_cooling_device *cdev)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = thermal_zone_unbind_cooling_device(tz,
> >> +						 ROCKCHIP_TRIP_PASSIVE, cdev);
> >> +	if (ret) {
> >> +		dev_err(&tz->device,
> >> +			"unbinding zone %s with cdev %s failed:%d\n", tz->type,
> >> +			cdev->type, ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> > The method of binding and unbinding used above requires you to check if
> > you are binding to the right cdev. If in your system you register more
> > than one cdev, say someone loads the power supply core, which in turns
> > register a cooling device for charging, your thermal zone will bind it
> > to TRIP_PASSIVE. It will happen to any cooling device that eventually
> > gets registered to the thermal framework. Is that the desired outcome?
> 
> Yes,I will use the generic trip points in the dts and fix it in the 
> thermal driver.

great

> > If not, you may want to compare the paramenter cdev to your data->cdev.
> >
> >> +
> >> +static struct thermal_zone_device_ops rockchip_tz_ops = {
> >> +	.bind = rockchip_bind,
> >> +	.unbind = rockchip_unbind,
> >> +	.get_temp = rockchip_get_temp,
> >> +	.get_mode = rockchip_get_mode,
> >> +	.set_mode = rockchip_set_mode,
> >> +	.get_trip_type = rockchip_get_trip_type,
> >> +	.get_trip_temp = rockchip_get_trip_temp,
> >> +	.get_crit_temp = rockchip_get_crit_temp,
> >> +	.set_trip_temp = rockchip_set_trip_temp,
> >> +};
> >> +
> >> +static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
> >> +{
> >> +	struct rockchip_thermal_data *data = data;
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
> >> +
> >> +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
> >> +		data->alarm_temp / 1000);
> >> +
> >> +	if (p_tsadc_data->irq_en && p_tsadc_data->irq_handle)
> >> +		p_tsadc_data->irq_handle(data->regs);
> >> +
> >> +	thermal_zone_device_update(data->tz);
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static int rockchip_thermal_probe(struct platform_device *pdev)
> >> +{
> >> +	struct rockchip_thermal_data *data;
> >> +	const struct rockchip_tsadc_platform_data *p_tsadc_data;
> >> +	struct cpumask clip_cpus;
> >> +	struct resource *res;
> >> +	const struct of_device_id *match;
> >> +	int ret, temp;
> >> +
> >> +	data = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_thermal_data),
> >> +			    GFP_KERNEL);
> >> +	if (!data)
> >> +		return -ENOMEM;
> >> +
> >> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
> >> +	if (IS_ERR(data->regs)) {
> >> +		dev_err(&pdev->dev, "Could not get tsadc source, %p\n",
> >> +			data->regs);
> >> +		return PTR_ERR(data->regs);
> >> +	}
> >> +
> >> +	match = of_match_node(of_rockchip_thermal_match, pdev->dev.of_node);
> >> +	if (!match)
> >> +		return -ENXIO;
> >> +	data->pdata = (const struct rockchip_tsadc_platform_data *)match->data;
> >> +	if (!data->pdata)
> >> +		return -EINVAL;
> >> +	p_tsadc_data = data->pdata;
> >> +
> >> +	data->clk = devm_clk_get(&pdev->dev, "tsadc");
> >> +	if (IS_ERR(data->clk)) {
> >> +		dev_err(&pdev->dev, "failed to get tsadc clock\n");
> >> +		return PTR_ERR(data->clk);
> >> +	}
> >> +
> >> +	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
> >> +	if (IS_ERR(data->pclk)) {
> >> +		dev_err(&pdev->dev, "failed to get tsadc pclk\n");
> >> +		return PTR_ERR(data->pclk);
> >> +	}
> >> +
> >> +	/*
> >> +	* Use a default of 10KHz for the converter clock.
> >> +	* This may become user-configurable in the future.
> >> +	*/
> >> +	ret = clk_set_rate(data->clk, 10000);
> >> +	if (ret < 0) {
> >> +		dev_err(&pdev->dev, "failed to set tsadc clk rate, %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	ret = clk_prepare_enable(data->clk);
> >> +	if (ret < 0) {
> >> +		dev_err(&pdev->dev, "failed to enable converter clock\n");
> >> +		goto err_clk;
> >> +	}
> >> +
> >> +	ret = clk_prepare_enable(data->pclk);
> >> +	if (ret < 0) {
> >> +		dev_err(&pdev->dev, "failed to enable pclk\n");
> >> +		goto err_pclk;
> >> +	}
> >> +
> >> +	platform_set_drvdata(pdev, data);
> >> +
> >> +	if (of_property_read_u32(pdev->dev.of_node, "passive-temp", &temp)) {
> >> +		dev_warn(&pdev->dev,
> >> +			 "Missing default passive temp property in the DT.\n");
> >> +		data->temp_passive = p_tsadc_data->temp_passive;
> >> +	} else {
> >> +		data->temp_passive = temp;
> >> +	}
> >> +
> >> +	if (of_property_read_u32(pdev->dev.of_node, "critical-temp", &temp)) {
> >> +		dev_warn(&pdev->dev,
> >> +			 "Missing default critical temp property in the DT.\n");
> >> +		data->temp_critical = p_tsadc_data->temp_critical;
> >> +	} else {
> >> +		data->temp_critical = temp;
> >> +	}
> >> +
> >> +	if (of_property_read_u32(pdev->dev.of_node, "force-shut-temp", &temp)) {
> >> +		dev_warn(&pdev->dev,
> >> +			 "Missing default force shut down temp property in the DT.\n");
> >> +		data->temp_force_shut = p_tsadc_data->temp_force_shut;
> >> +	} else {
> >> +		data->temp_force_shut = temp;
> >> +	}
> >> +
> >> +	cpumask_set_cpu(0, &clip_cpus);
> >> +	data->cdev = of_cpufreq_cooling_register(pdev->dev.of_node, &clip_cpus);
> >> +	if (IS_ERR(data->cdev)) {
> >> +		dev_err(&pdev->dev, "failed to register cpufreq cooling device\n");
> >> +		goto disable_clk;
> >> +	}
> >> +
> >> +	data->tz = thermal_zone_device_register("rockchip_thermal",
> >> +						ROCKCHIP_TRIP_NUM,
> >> +						0, data,
> >> +						&rockchip_tz_ops, NULL,
> >> +						p_tsadc_data->passive_delay,
> >> +						p_tsadc_data->polling_delay);
> >> +
> >> +	if (IS_ERR(data->tz)) {
> >> +		dev_err(&pdev->dev, "failed to register thermal zone device\n");
> >> +		goto fail_cpufreq_register;
> >> +	}
> >> +
> >> +	if (p_tsadc_data->irq_en) {
> >> +		data->irq = platform_get_irq(pdev, 0);
> >> +		if (data->irq < 0) {
> >> +			dev_err(&pdev->dev, "no irq resource?\n");
> >> +			goto fail_irq;
> >> +		}
> >> +
> >> +		ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
> >> +				rockchip_thermal_alarm_irq_thread,
> >> +				IRQF_ONESHOT, "rockchip_thermal", data);
> >> +		if (ret < 0) {
> >> +			dev_err(&pdev->dev,
> >> +				"failed to request tsadc irq: %d\n", ret);
> >> +			goto fail_thermal_unregister;
> >> +		}
> >> +	}
> >> +
> >> +	rockchip_thermal_initialize(data);
> >> +	rockchip_thermal_control(data, true);
> >> +
> >> +	return 0;
> >> +
> >> +fail_thermal_unregister:
> >> +	thermal_zone_device_unregister(data->tz);
> >> +fail_irq:
> >> +fail_cpufreq_register:
> >> +	cpufreq_cooling_unregister(data->cdev);
> >> +disable_clk:
> >> +err_pclk:
> >> +	clk_disable_unprepare(data->pclk);
> >> +err_clk:
> >> +	clk_disable_unprepare(data->clk);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int rockchip_thermal_remove(struct platform_device *pdev)
> >> +{
> >> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> >> +
> >> +	rockchip_thermal_control(data, false);
> >> +
> >> +	thermal_zone_device_unregister(data->tz);
> >> +	cpufreq_cooling_unregister(data->cdev);
> >> +	cpufreq_cooling_unregister(data->cdev);
> >> +
> > The above call is duplicated.
> 
> OK.
> >> +	clk_disable_unprepare(data->clk);
> >> +	clk_disable_unprepare(data->pclk);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +#ifdef CONFIG_PM_SLEEP
> >> +static int rockchip_thermal_suspend(struct device *dev)
> >> +{
> >> +	struct platform_device *pdev = to_platform_device(dev);
> >> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> >> +
> >> +	rockchip_thermal_control(data, false);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int rockchip_thermal_resume(struct device *dev)
> >> +{
> >> +	struct platform_device *pdev = to_platform_device(dev);
> >> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> >> +
> >> +	rockchip_thermal_initialize(data);
> >> +	rockchip_thermal_control(data, true);
> >> +
> >> +	return 0;
> >> +}
> > Would you need to manage your clocks too in the suspend resume path
> > (data->clk and data->pclk)?
> yes.
> Usually,it's need disable to save power.

Yes, that's my point. Please consider handling clocks to improve power
savings.

> >> +#endif
> >> +
> >> +static SIMPLE_DEV_PM_OPS(rockchip_thermal_pm_ops,
> >> +			 rockchip_thermal_suspend, rockchip_thermal_resume);
> >> +
> >> +static struct platform_driver rockchip_thermal_driver = {
> >> +	.driver = {
> >> +		   .name = "rockchip-thermal",
> >> +		   .owner = THIS_MODULE,
> >> +		   .pm = &rockchip_thermal_pm_ops,
> >> +		   .of_match_table = of_rockchip_thermal_match,
> >> +		   },
> >> +	.probe = rockchip_thermal_probe,
> >> +	.remove = rockchip_thermal_remove,
> >> +};
> >> +
> >> +module_platform_driver(rockchip_thermal_driver);
> >> +
> >> +MODULE_DESCRIPTION("ROCKCHIP THERMAL Driver");
> >> +MODULE_AUTHOR("Rockchip, Inc.");
> >> +MODULE_LICENSE("GPL");
> > Your file header states GPL version two. Thus I suppose you meant:
> > +MODULE_LICENSE("GPL v2");
> >
> > right?
> >
> > Check include/linux/module.h for further clarifications.
> 
> But I think the  "GPL"  is support for thr GNU Public License v2 or later.

Exactly, if you intend to support v3, then it is fine. But your code
header states only V2.

> I will fix GPL v2 if I get it wrong.

If your license is GPLv2 only as mentioned in your header, please use 'GPL
v2' tag.

Cheers,

> 
> >> +MODULE_ALIAS("platform:rockchip-thermal");
> >> -- 
> >> 1.9.1
> >>
> >>
> > BR,
> >
> > Eduardo Valentin
> >
> >
> >
> 
> -- 
> Best regards,
> Caesar
> 
> 

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Caesar Wang]

Dear Eduardo,

? 2014/9/10 20:46, Eduardo Valentin ??:
> Hello Caesar,
>
> On Wed, Sep 10, 2014 at 12:39:07PM +0800, Caesar Wang wrote:
>> Dear Eduardo,
>>
>> I'm  sorry for it.
>> I just received this message.Maybe my mailbox has a problem.
>
> No problems. You can take your time.
>
>> Thank you for your comments.
>>
>> ? 2014?08?31? 04:09, Eduardo Valentin ??:
>>> Hello Ceasar,
>>>
>>> On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
>>>> Thermal is TS-ADC Controller module supports
>>>> user-defined mode and automatic mode.
>>>>
>>>> User-defined mode refers,TSADC all the control signals entirely by
>>>> software writing to register for direct control.
>>>>
>>>> Automaic mode refers to the module automatically poll TSADC output,
>>>> and the results were checked.If you find that the temperature High
>>>> in a period of time,an interrupt is generated to the processor
>>>> down-measures taken;if the temperature over a period of time High,
>>>> the resulting TSHUT gave CRU module,let it reset the entire chip,
>>>> or via GPIO give PMIC.
>>>>
>>>> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> I forgot to ask, is zhaoyifeng the correct name or is it a nickname?
:zhaoyifeng is chinese pinyin,Maybe I can correct name :Zhao Yi Feng or 
give him a English name:-D .

But his name is zhaoyifeng, so....
>>>> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>>>> ---
>>>>    drivers/thermal/Kconfig            |   9 +
>>>>    drivers/thermal/Makefile           |   1 +
>>>>    drivers/thermal/rockchip_thermal.c | 669 +++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 679 insertions(+)
>>>>    create mode 100644 drivers/thermal/rockchip_thermal.c
>>>>
>>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>>> index f9a1386..a00aa1e 100644
>>>> --- a/drivers/thermal/Kconfig
>>>> +++ b/drivers/thermal/Kconfig
>>>> @@ -133,6 +133,15 @@ config SPEAR_THERMAL
>>>>    	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>>>>    	  thermal framework.
>>>>    
>>>> +config ROCKCHIP_THERMAL
>>>> +	tristate "Rockchip thermal driver"
>>>> +	depends on ARCH_ROCKCHIP
>>>> +	help
>>>> +	  Support for Temperature Sensor ADC (TS-ADC) found on Rockchip SoCs.
>>>> +	  It supports one critical trip point and one passive trip point.  The
>>>> +	  cpufreq is used as the cooling device to throttle CPUs when the
>>>> +	  passive trip is crossed.
>>>> +
>>>>    config RCAR_THERMAL
>>>>    	tristate "Renesas R-Car thermal driver"
>>>>    	depends on ARCH_SHMOBILE || COMPILE_TEST
>>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>>> index de0636a..b48b817 100644
>>>> --- a/drivers/thermal/Makefile
>>>> +++ b/drivers/thermal/Makefile
>>>> @@ -19,6 +19,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
>>>>    
>>>>    # platform thermal drivers
>>>>    obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>>>> +obj-$(CONFIG_ROCKCHIP_THERMAL) 	+= rockchip_thermal.o
>>>>    obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>>>>    obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>>>>    obj-y				+= samsung/
>>>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>>>> new file mode 100644
>>>> index 0000000..011f387
>>>> --- /dev/null
>>>> +++ b/drivers/thermal/rockchip_thermal.c
>>>> @@ -0,0 +1,669 @@
>>>> +/*
>>>> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> +*/
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/cpu_cooling.h>
>>>> +#include "thermal_core.h"
>>> Why do you need thermal_core.h here? Wouldn't linux/thermal.h be
>>> sufficient?
>>>
>> yes, I will use linux/thermal.h.
>>>> +
>>>> +enum rockchip_thermal_trip {
>>>> +	ROCKCHIP_TRIP_PASSIVE,
>>>> +	ROCKCHIP_TRIP_CRITICAL,
>>>> +	ROCKCHIP_TRIP_NUM,
>>>> +};
>>>> +
>>> Why do you need your own trip types?
>>>
>>>> +struct rockchip_thermal_data {
>>>> +	const struct rockchip_tsadc_platform_data *pdata;
>>>> +	struct thermal_zone_device *tz;
>>>> +	struct thermal_cooling_device *cdev;
>>>> +	enum thermal_device_mode mode;
>>>> +	void __iomem *regs;
>>>> +
>>>> +	signed long temp_passive;
>>>> +	signed long temp_critical;
>>>> +	signed long temp_force_shut;
>>>> +	signed long alarm_temp;
>>>> +	signed long last_temp;
>>>> +	bool irq_enabled;
>>>> +	int irq;
>>>> +	struct clk *clk;
>>>> +	struct clk *pclk;
>>>> +};
>>>> +
>>>> +struct rockchip_tsadc_platform_data {
>>>> +	u8 irq_en;
>>>> +	signed long temp_passive;
>>>> +	signed long temp_critical;
>>>> +	signed long temp_force_shut;
>>>> +	int passive_delay;
>>>> +	int polling_delay;
>>>> +
>>>> +	int (*irq_handle)(void __iomem *reg);
>>>> +	int (*initialize)(void __iomem *reg, signed long temp_force_shut);
>>>> +	int (*control)(void __iomem *reg, bool on);
>>>> +	u32 (*code_to_temp)(int temp);
>>>> +	u32 (*temp_to_code)(int temp);
>>>> +	void (*set_alarm_temp)(void __iomem *regs, signed long temp);
>>>> +};
>>>> +
>>>> +/*TSADC V2 Sensor info define:*/
>>>> +#define TSADCV2_AUTO_CON			0x04
>>>> +#define TSADCV2_INT_EN				0x08
>>>> +#define TSADCV2_INT_PD				0x0c
>>>> +#define TSADCV2_DATA1				0x24
>>>> +#define TSADCV2_COMP1_INT			0x34
>>>> +#define TSADCV2_COMP1_SHUT			0x44
>>>> +#define TSADCV2_AUTO_PERIOD			0x68
>>>> +#define TSADCV2_AUTO_PERIOD_HT			0x6c
>>>> +
>>>> +#define TSADCV2_AUTO_SRC1_EN			(1 << 5)
>>>> +#define TSADCV2_AUTO_EN				(1 << 0)
>>>> +#define TSADCV2_AUTO_DISABLE			~(1 << 0)
>>>> +#define TSADCV2_AUTO_STAS_BUSY			(1 << 16)
>>>> +#define TSADCV2_AUTO_STAS_BUSY_MASK		(1 << 16)
>>>> +#define TSADCV2_SHUT_2GPIO_SRC1_EN		(1 << 5)
>>>> +#define TSADCV2_INT_SRC1_EN			(1 << 1)
>>>> +#define TSADCV2_SHUT_SRC1_STATUS		(1 << 5)
>>>> +#define TSADCV2_INT_SRC1_STATUS			(1 << 1)
>>>> +
>>> The above bits can be defined with BIT() macro, right?
>> Yes. The Bit() will be more resonable..
>>>    Something like:
>>> +#define TSADCV2_INT_SRC1_STATUS			BIT(1)
>>>
>>>
>>>> +#define TSADCV2_DATA_MASK			0xfff
>>>> +#define TSADCV2_HIGHT_INT_DEBOUNCE		0x60
>>>> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE		0x64
>>>> +#define TSADCV2_HIGHT_INT_DEBOUNCE_TIME		0x0a
>>>> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME	0x0a
>>>> +#define TSADCV2_AUTO_PERIOD_TIME		0x03e8
>>>> +#define TSADCV2_AUTO_PERIOD_HT_TIME		0x64
>>>> +
>>>> +struct tsadc_table {
>>>> +	int code;
>>>> +	int temp;
>>>> +};
>>>> +
>>>> +static const struct tsadc_table v2_code_table[] = {
>>>> +	{TSADCV2_DATA_MASK, -40},
>>>> +	{3800, -40},
>>>> +	{3792, -35},
>>>> +	{3783, -30},
>>>> +	{3774, -25},
>>>> +	{3765, -20},
>>>> +	{3756, -15},
>>>> +	{3747, -10},
>>>> +	{3737, -5},
>>>> +	{3728, 0},
>>>> +	{3718, 5},
>>>> +	{3708, 10},
>>>> +	{3698, 15},
>>>> +	{3688, 20},
>>>> +	{3678, 25},
>>>> +	{3667, 30},
>>>> +	{3656, 35},
>>>> +	{3645, 40},
>>>> +	{3634, 45},
>>>> +	{3623, 50},
>>>> +	{3611, 55},
>>>> +	{3600, 60},
>>>> +	{3588, 65},
>>>> +	{3575, 70},
>>>> +	{3563, 75},
>>>> +	{3550, 80},
>>>> +	{3537, 85},
>>>> +	{3524, 90},
>>>> +	{3510, 95},
>>>> +	{3496, 100},
>>>> +	{3482, 105},
>>>> +	{3467, 110},
>>>> +	{3452, 115},
>>>> +	{3437, 120},
>>>> +	{3421, 125},
>>>> +	{0, 125},
>>>> +};
>>>> +
>>>> +static int rk_tsadcv2_irq_handle(void __iomem *regs)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	val = readl_relaxed(regs + TSADCV2_INT_PD);
>>>> +	writel_relaxed(val & ~(1 << 8), regs + TSADCV2_INT_PD);
>>> Why do you need to clear bit 8? Why hardcoded?
>> The bit 8 set 0 to clear the interrupt.
>>
> Would it make sense to create a macro for this?

OK .
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static u32 rk_tsadcv2_temp_to_code(int temp)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
>>>> +		if (temp <= v2_code_table[i].temp && temp >
>>>> +		    v2_code_table[i - 1].temp)
>>>> +			return v2_code_table[i].code;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static u32 rk_tsadcv2_code_to_temp(int code)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
>>>> +		if (code <= v2_code_table[i].code && code >
>>>> +		    v2_code_table[i + 1].code){
>>>> +			return v2_code_table[i].temp;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>> I know the table is not something too big, but considering it is ordered
>>> by either ADC value code or by temp, at least one of the above searching function
>>> may be more efficient, right?
>> Yes, use the point will be more efficient.
>>>> +
>>>> +static int rk_tsadcv2_initialize(void __iomem *regs,
>>>> +				 signed long temp_force_shut)
>>>> +{
>>>> +	int shutdown_value;
>>>> +
>>>> +	shutdown_value = rk_tsadcv2_temp_to_code(temp_force_shut);
>>>> +	/* Enable measurements at ~ 10 Hz */
>>> Does it leave the sampling clock at 10Hz?
>> yes.
> ok
>
>
>>> Is this clock exposed via
>>> clock framework?
>> The clock is divided by data->clk.
> ok
>
>>>> +	writel_relaxed(0, regs + TSADCV2_AUTO_CON);
>>>> +	writel_relaxed(TSADCV2_AUTO_PERIOD_TIME, regs + TSADCV2_AUTO_PERIOD);
>>>> +	writel_relaxed(TSADCV2_AUTO_PERIOD_HT_TIME, regs +
>>>> +		       TSADCV2_AUTO_PERIOD_HT);
>>>> +	writel_relaxed(shutdown_value, regs + TSADCV2_COMP1_SHUT);
>>>> +	writel_relaxed(TSADCV2_HIGHT_INT_DEBOUNCE_TIME, regs +
>>>> +		       TSADCV2_HIGHT_INT_DEBOUNCE);
>>>> +	writel_relaxed(TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME, regs +
>>>> +		       TSADCV2_HIGHT_TSHUT_DEBOUNCE);
>>>> +	writel_relaxed(TSADCV2_SHUT_2GPIO_SRC1_EN | TSADCV2_INT_SRC1_EN, regs +
>>>> +		       TSADCV2_INT_EN);
>>>> +	writel_relaxed(TSADCV2_AUTO_SRC1_EN | TSADCV2_AUTO_EN, regs +
>>>> +		       TSADCV2_AUTO_CON);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rk_tsadcv2_control(void __iomem *regs, bool on)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	if (on) {
>>>> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>>>> +		writel_relaxed(val | TSADCV2_AUTO_EN, regs + TSADCV2_AUTO_CON);
>>>> +	} else {
>>>> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>>>> +		writel_relaxed(val & TSADCV2_AUTO_DISABLE,
>>>> +			       regs + TSADCV2_AUTO_CON);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void rk_tsadcv2_alarm_temp(void __iomem *regs, signed long alarm_temp)
>>>> +{
>>>> +	int alarm_value;
>>>> +
>>>> +	alarm_value = rk_tsadcv2_temp_to_code(alarm_temp);
>>>> +	writel_relaxed(alarm_value, regs + TSADCV2_COMP1_INT);
>>>> +}
>>>> +
>>>> +struct rockchip_tsadc_platform_data const rk3288_tsadc_data = {
>>>> +	.irq_en = 1,
>>>> +	.temp_passive = 85000,
>>>> +	.temp_critical = 100000,
>>>> +	.temp_force_shut = 120000,
>>>> +	.passive_delay = 2000,
>>>> +	.polling_delay = 1000,
>>>> +	.irq_handle = rk_tsadcv2_irq_handle,
>>>> +	.initialize = rk_tsadcv2_initialize,
>>>> +	.control = rk_tsadcv2_control,
>>>> +	.code_to_temp = rk_tsadcv2_code_to_temp,
>>>> +	.temp_to_code = rk_tsadcv2_temp_to_code,
>>>> +	.set_alarm_temp = rk_tsadcv2_alarm_temp,
>>>> +};
>>> shall the above struct be also static?
>> OK.
>>>> +
>>>> +static const struct of_device_id of_rockchip_thermal_match[] = {
>>>> +	{
>>>> +		.compatible = "rockchip,rk3288-tsadc",
>>>> +		.data = (void *)&rk3288_tsadc_data,
>>>> +	},
>>>> +	{ /* end */ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
>>>> +
>>>> +static void rockchip_set_alarm_temp(struct rockchip_thermal_data *data,
>>>> +				    signed long alarm_temp)
>>>> +{
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	data->alarm_temp = alarm_temp;
>>>> +	if (p_tsadc_data->set_alarm_temp)
>>>> +		p_tsadc_data->set_alarm_temp(data->regs, alarm_temp);
>>>> +}
>>>> +
>>>> +static int rockchip_get_temp(struct thermal_zone_device *tz,
>>>> +			     unsigned long *temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +	u32 val;
>>>> +
>>>> +	val = readl_relaxed(data->regs + TSADCV2_DATA1);
>>>> +	*temp = p_tsadc_data->code_to_temp(val);
>>>> +
>>>> +	/* Update alarm value to next higher trip point */
>>>> +	if (data->alarm_temp == data->temp_passive && *temp >=
>>>> +	    data->temp_passive)
>>>> +		rockchip_set_alarm_temp(data, data->temp_critical);
>>>> +
>>>> +	if (data->alarm_temp == data->temp_critical && *temp <
>>>> +	    data->temp_passive) {
>>>> +		rockchip_set_alarm_temp(data, data->temp_passive);
>>>> +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
>>>> +			data->alarm_temp / 1000);
>>>> +	}
>>>> +
>>>> +	if (*temp != data->last_temp) {
>>>> +		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
>>>> +		data->last_temp = *temp;
>>>> +	}
>>>> +
>>>> +	/* Reenable alarm IRQ if temperature below alarm temperature */
>>>> +	if (!data->irq_enabled && *temp < data->alarm_temp) {
>>>> +		data->irq_enabled = true;
>>>> +		enable_irq(data->irq);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_initialize(struct rockchip_thermal_data *data)
>>>> +{
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	if (p_tsadc_data->initialize)
>>>> +		p_tsadc_data->initialize(data->regs, data->temp_force_shut);
>>>> +	rockchip_set_alarm_temp(data, data->temp_passive);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void rockchip_thermal_control(struct rockchip_thermal_data *data,
>>>> +				     bool on)
>>>> +{
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	if (p_tsadc_data->control)
>>>> +		p_tsadc_data->control(data->regs, on);
>>>> +
>>>> +	if (on) {
>>>> +		data->irq_enabled = true;
>>>> +		data->mode = THERMAL_DEVICE_ENABLED;
>>>> +	} else {
>>>> +		data->irq_enabled = false;
>>>> +		data->mode = THERMAL_DEVICE_DISABLED;
>>>> +	}
>>>> +}
>>>> +
>>>> +static int rockchip_get_mode(struct thermal_zone_device *tz,
>>>> +			     enum thermal_device_mode *mode)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	*mode = data->mode;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_set_mode(struct thermal_zone_device *tz,
>>>> +			     enum thermal_device_mode mode)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	if (mode == THERMAL_DEVICE_ENABLED) {
>>>> +		tz->polling_delay = p_tsadc_data->polling_delay;
>>>> +		tz->passive_delay = p_tsadc_data->passive_delay;
>>>> +		if (!data->irq_enabled) {
>>>> +			data->irq_enabled = true;
>>>> +			enable_irq(data->irq);
>>>> +		}
>>>> +	} else {
>>>> +		tz->polling_delay = 0;
>>>> +		tz->passive_delay = 0;
>>>> +		if (data->irq_enabled) {
>>>> +			disable_irq(data->irq);
>>>> +			data->irq_enabled = false;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	data->mode = mode;
>>>> +	thermal_zone_device_update(tz);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_get_trip_type(struct thermal_zone_device *tz, int trip,
>>>> +				  enum thermal_trip_type *type)
>>>> +{
>>>> +	*type = (trip == ROCKCHIP_TRIP_PASSIVE) ? THERMAL_TRIP_PASSIVE :
>>>> +		 THERMAL_TRIP_CRITICAL;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_get_crit_temp(struct thermal_zone_device *tz,
>>>> +				  unsigned long *temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	*temp = data->temp_critical;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_get_trip_temp(struct thermal_zone_device *tz, int trip,
>>>> +				  unsigned long *temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	*temp = (trip == ROCKCHIP_TRIP_PASSIVE) ? data->temp_passive :
>>>> +		 data->temp_critical;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>>> +				  unsigned long temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	if (trip == ROCKCHIP_TRIP_CRITICAL)
>>>> +		return -EPERM;
>>>> +
>>>> +	data->temp_passive = temp;
>>>> +	rockchip_set_alarm_temp(data, temp);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_bind(struct thermal_zone_device *tz,
>>>> +			 struct thermal_cooling_device *cdev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = thermal_zone_bind_cooling_device(tz, ROCKCHIP_TRIP_PASSIVE, cdev,
>>>> +					       THERMAL_NO_LIMIT,
>>>> +					       THERMAL_NO_LIMIT);
>>>> +	if (ret) {
>>>> +		dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
>>>> +			tz->type, cdev->type, ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_unbind(struct thermal_zone_device *tz,
>>>> +			   struct thermal_cooling_device *cdev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = thermal_zone_unbind_cooling_device(tz,
>>>> +						 ROCKCHIP_TRIP_PASSIVE, cdev);
>>>> +	if (ret) {
>>>> +		dev_err(&tz->device,
>>>> +			"unbinding zone %s with cdev %s failed:%d\n", tz->type,
>>>> +			cdev->type, ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>> The method of binding and unbinding used above requires you to check if
>>> you are binding to the right cdev. If in your system you register more
>>> than one cdev, say someone loads the power supply core, which in turns
>>> register a cooling device for charging, your thermal zone will bind it
>>> to TRIP_PASSIVE. It will happen to any cooling device that eventually
>>> gets registered to the thermal framework. Is that the desired outcome?
>> Yes,I will use the generic trip points in the dts and fix it in the
>> thermal driver.
> great
>
>>> If not, you may want to compare the paramenter cdev to your data->cdev.
>>>
>>>> +
>>>> +static struct thermal_zone_device_ops rockchip_tz_ops = {
>>>> +	.bind = rockchip_bind,
>>>> +	.unbind = rockchip_unbind,
>>>> +	.get_temp = rockchip_get_temp,
>>>> +	.get_mode = rockchip_get_mode,
>>>> +	.set_mode = rockchip_set_mode,
>>>> +	.get_trip_type = rockchip_get_trip_type,
>>>> +	.get_trip_temp = rockchip_get_trip_temp,
>>>> +	.get_crit_temp = rockchip_get_crit_temp,
>>>> +	.set_trip_temp = rockchip_set_trip_temp,
>>>> +};
>>>> +
>>>> +static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = data;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
>>>> +		data->alarm_temp / 1000);
>>>> +
>>>> +	if (p_tsadc_data->irq_en && p_tsadc_data->irq_handle)
>>>> +		p_tsadc_data->irq_handle(data->regs);
>>>> +
>>>> +	thermal_zone_device_update(data->tz);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct rockchip_thermal_data *data;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data;
>>>> +	struct cpumask clip_cpus;
>>>> +	struct resource *res;
>>>> +	const struct of_device_id *match;
>>>> +	int ret, temp;
>>>> +
>>>> +	data = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_thermal_data),
>>>> +			    GFP_KERNEL);
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>>> +	if (IS_ERR(data->regs)) {
>>>> +		dev_err(&pdev->dev, "Could not get tsadc source, %p\n",
>>>> +			data->regs);
>>>> +		return PTR_ERR(data->regs);
>>>> +	}
>>>> +
>>>> +	match = of_match_node(of_rockchip_thermal_match, pdev->dev.of_node);
>>>> +	if (!match)
>>>> +		return -ENXIO;
>>>> +	data->pdata = (const struct rockchip_tsadc_platform_data *)match->data;
>>>> +	if (!data->pdata)
>>>> +		return -EINVAL;
>>>> +	p_tsadc_data = data->pdata;
>>>> +
>>>> +	data->clk = devm_clk_get(&pdev->dev, "tsadc");
>>>> +	if (IS_ERR(data->clk)) {
>>>> +		dev_err(&pdev->dev, "failed to get tsadc clock\n");
>>>> +		return PTR_ERR(data->clk);
>>>> +	}
>>>> +
>>>> +	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
>>>> +	if (IS_ERR(data->pclk)) {
>>>> +		dev_err(&pdev->dev, "failed to get tsadc pclk\n");
>>>> +		return PTR_ERR(data->pclk);
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	* Use a default of 10KHz for the converter clock.
>>>> +	* This may become user-configurable in the future.
>>>> +	*/
>>>> +	ret = clk_set_rate(data->clk, 10000);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "failed to set tsadc clk rate, %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(data->clk);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "failed to enable converter clock\n");
>>>> +		goto err_clk;
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(data->pclk);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "failed to enable pclk\n");
>>>> +		goto err_pclk;
>>>> +	}
>>>> +
>>>> +	platform_set_drvdata(pdev, data);
>>>> +
>>>> +	if (of_property_read_u32(pdev->dev.of_node, "passive-temp", &temp)) {
>>>> +		dev_warn(&pdev->dev,
>>>> +			 "Missing default passive temp property in the DT.\n");
>>>> +		data->temp_passive = p_tsadc_data->temp_passive;
>>>> +	} else {
>>>> +		data->temp_passive = temp;
>>>> +	}
>>>> +
>>>> +	if (of_property_read_u32(pdev->dev.of_node, "critical-temp", &temp)) {
>>>> +		dev_warn(&pdev->dev,
>>>> +			 "Missing default critical temp property in the DT.\n");
>>>> +		data->temp_critical = p_tsadc_data->temp_critical;
>>>> +	} else {
>>>> +		data->temp_critical = temp;
>>>> +	}
>>>> +
>>>> +	if (of_property_read_u32(pdev->dev.of_node, "force-shut-temp", &temp)) {
>>>> +		dev_warn(&pdev->dev,
>>>> +			 "Missing default force shut down temp property in the DT.\n");
>>>> +		data->temp_force_shut = p_tsadc_data->temp_force_shut;
>>>> +	} else {
>>>> +		data->temp_force_shut = temp;
>>>> +	}
>>>> +
>>>> +	cpumask_set_cpu(0, &clip_cpus);
>>>> +	data->cdev = of_cpufreq_cooling_register(pdev->dev.of_node, &clip_cpus);
>>>> +	if (IS_ERR(data->cdev)) {
>>>> +		dev_err(&pdev->dev, "failed to register cpufreq cooling device\n");
>>>> +		goto disable_clk;
>>>> +	}
>>>> +
>>>> +	data->tz = thermal_zone_device_register("rockchip_thermal",
>>>> +						ROCKCHIP_TRIP_NUM,
>>>> +						0, data,
>>>> +						&rockchip_tz_ops, NULL,
>>>> +						p_tsadc_data->passive_delay,
>>>> +						p_tsadc_data->polling_delay);
>>>> +
>>>> +	if (IS_ERR(data->tz)) {
>>>> +		dev_err(&pdev->dev, "failed to register thermal zone device\n");
>>>> +		goto fail_cpufreq_register;
>>>> +	}
>>>> +
>>>> +	if (p_tsadc_data->irq_en) {
>>>> +		data->irq = platform_get_irq(pdev, 0);
>>>> +		if (data->irq < 0) {
>>>> +			dev_err(&pdev->dev, "no irq resource?\n");
>>>> +			goto fail_irq;
>>>> +		}
>>>> +
>>>> +		ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
>>>> +				rockchip_thermal_alarm_irq_thread,
>>>> +				IRQF_ONESHOT, "rockchip_thermal", data);
>>>> +		if (ret < 0) {
>>>> +			dev_err(&pdev->dev,
>>>> +				"failed to request tsadc irq: %d\n", ret);
>>>> +			goto fail_thermal_unregister;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	rockchip_thermal_initialize(data);
>>>> +	rockchip_thermal_control(data, true);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +fail_thermal_unregister:
>>>> +	thermal_zone_device_unregister(data->tz);
>>>> +fail_irq:
>>>> +fail_cpufreq_register:
>>>> +	cpufreq_cooling_unregister(data->cdev);
>>>> +disable_clk:
>>>> +err_pclk:
>>>> +	clk_disable_unprepare(data->pclk);
>>>> +err_clk:
>>>> +	clk_disable_unprepare(data->clk);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>>>> +
>>>> +	rockchip_thermal_control(data, false);
>>>> +
>>>> +	thermal_zone_device_unregister(data->tz);
>>>> +	cpufreq_cooling_unregister(data->cdev);
>>>> +	cpufreq_cooling_unregister(data->cdev);
>>>> +
>>> The above call is duplicated.
>> OK.
>>>> +	clk_disable_unprepare(data->clk);
>>>> +	clk_disable_unprepare(data->pclk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int rockchip_thermal_suspend(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>>>> +
>>>> +	rockchip_thermal_control(data, false);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_resume(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>>>> +
>>>> +	rockchip_thermal_initialize(data);
>>>> +	rockchip_thermal_control(data, true);
>>>> +
>>>> +	return 0;
>>>> +}
>>> Would you need to manage your clocks too in the suspend resume path
>>> (data->clk and data->pclk)?
>> yes.
>> Usually,it's need disable to save power.
> Yes, that's my point. Please consider handling clocks to improve power
> savings.
>
>>>> +#endif
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(rockchip_thermal_pm_ops,
>>>> +			 rockchip_thermal_suspend, rockchip_thermal_resume);
>>>> +
>>>> +static struct platform_driver rockchip_thermal_driver = {
>>>> +	.driver = {
>>>> +		   .name = "rockchip-thermal",
>>>> +		   .owner = THIS_MODULE,
>>>> +		   .pm = &rockchip_thermal_pm_ops,
>>>> +		   .of_match_table = of_rockchip_thermal_match,
>>>> +		   },
>>>> +	.probe = rockchip_thermal_probe,
>>>> +	.remove = rockchip_thermal_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(rockchip_thermal_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("ROCKCHIP THERMAL Driver");
>>>> +MODULE_AUTHOR("Rockchip, Inc.");
>>>> +MODULE_LICENSE("GPL");
>>> Your file header states GPL version two. Thus I suppose you meant:
>>> +MODULE_LICENSE("GPL v2");
>>>
>>> right?
>>>
>>> Check include/linux/module.h for further clarifications.
>> But I think the  "GPL"  is support for thr GNU Public License v2 or later.
> Exactly, if you intend to support v3, then it is fine. But your code
> header states only V2.
>
>> I will fix GPL v2 if I get it wrong.
> If your license is GPLv2 only as mentioned in your header, please use 'GPL
> v2' tag.
>
> Cheers,
>
>>>> +MODULE_ALIAS("platform:rockchip-thermal");
>>>> -- 
>>>> 1.9.1
>>>>
>>>>
>>> BR,
>>>
>>> Eduardo Valentin
>>>
>>>
>>>
>> -- 
>> Best regards,
>> Caesar
>>
>>
>
>

-- 
Best regards,
Caesar

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Caesar Wang]

Eduardo,

? 2014/9/10 20:46, Eduardo Valentin ??:
> Hello Caesar,
>
> On Wed, Sep 10, 2014 at 12:39:07PM +0800, Caesar Wang wrote:
>> Dear Eduardo,
>>
>> I'm  sorry for it.
>> I just received this message.Maybe my mailbox has a problem.
>
> No problems. You can take your time.
>
>> Thank you for your comments.
>>
>> ? 2014?08?31? 04:09, Eduardo Valentin ??:
>>> Hello Ceasar,
>>>
>>> On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
>>>> Thermal is TS-ADC Controller module supports
>>>> user-defined mode and automatic mode.
>>>>
>>>> User-defined mode refers,TSADC all the control signals entirely by
>>>> software writing to register for direct control.
>>>>
>>>> Automaic mode refers to the module automatically poll TSADC output,
>>>> and the results were checked.If you find that the temperature High
>>>> in a period of time,an interrupt is generated to the processor
>>>> down-measures taken;if the temperature over a period of time High,
>>>> the resulting TSHUT gave CRU module,let it reset the entire chip,
>>>> or via GPIO give PMIC.
>>>>
>>>> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> I forgot to ask, is zhaoyifeng the correct name or is it a nickname?
>
>>>> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>>>> ---
>>>>    drivers/thermal/Kconfig            |   9 +
>>>>    drivers/thermal/Makefile           |   1 +
>>>>    drivers/thermal/rockchip_thermal.c | 669 +++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 679 insertions(+)
>>>>    create mode 100644 drivers/thermal/rockchip_thermal.c
>>>>
>>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
>>>> index f9a1386..a00aa1e 100644
>>>> --- a/drivers/thermal/Kconfig
>>>> +++ b/drivers/thermal/Kconfig
>>>> @@ -133,6 +133,15 @@ config SPEAR_THERMAL
>>>>    	  Enable this to plug the SPEAr thermal sensor driver into the Linux
>>>>    	  thermal framework.
>>>>    
>>>> +config ROCKCHIP_THERMAL
>>>> +	tristate "Rockchip thermal driver"
>>>> +	depends on ARCH_ROCKCHIP
>>>> +	help
>>>> +	  Support for Temperature Sensor ADC (TS-ADC) found on Rockchip SoCs.
>>>> +	  It supports one critical trip point and one passive trip point.  The
>>>> +	  cpufreq is used as the cooling device to throttle CPUs when the
>>>> +	  passive trip is crossed.
>>>> +
>>>>    config RCAR_THERMAL
>>>>    	tristate "Renesas R-Car thermal driver"
>>>>    	depends on ARCH_SHMOBILE || COMPILE_TEST
>>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
>>>> index de0636a..b48b817 100644
>>>> --- a/drivers/thermal/Makefile
>>>> +++ b/drivers/thermal/Makefile
>>>> @@ -19,6 +19,7 @@ thermal_sys-$(CONFIG_CPU_THERMAL)	+= cpu_cooling.o
>>>>    
>>>>    # platform thermal drivers
>>>>    obj-$(CONFIG_SPEAR_THERMAL)	+= spear_thermal.o
>>>> +obj-$(CONFIG_ROCKCHIP_THERMAL) 	+= rockchip_thermal.o
>>>>    obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
>>>>    obj-$(CONFIG_KIRKWOOD_THERMAL)  += kirkwood_thermal.o
>>>>    obj-y				+= samsung/
>>>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>>>> new file mode 100644
>>>> index 0000000..011f387
>>>> --- /dev/null
>>>> +++ b/drivers/thermal/rockchip_thermal.c
>>>> @@ -0,0 +1,669 @@
>>>> +/*
>>>> + * Copyright (c) 2014, Fuzhou Rockchip Electronics Co., Ltd
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>>> + * more details.
>>>> + *
>>>> +*/
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <linux/cpu_cooling.h>
>>>> +#include "thermal_core.h"
>>> Why do you need thermal_core.h here? Wouldn't linux/thermal.h be
>>> sufficient?
>>>
>> yes, I will use linux/thermal.h.
>>>> +
>>>> +enum rockchip_thermal_trip {
>>>> +	ROCKCHIP_TRIP_PASSIVE,
>>>> +	ROCKCHIP_TRIP_CRITICAL,
>>>> +	ROCKCHIP_TRIP_NUM,
>>>> +};
>>>> +
>>> Why do you need your own trip types?
>>>
>>>> +struct rockchip_thermal_data {
>>>> +	const struct rockchip_tsadc_platform_data *pdata;
>>>> +	struct thermal_zone_device *tz;
>>>> +	struct thermal_cooling_device *cdev;
>>>> +	enum thermal_device_mode mode;
>>>> +	void __iomem *regs;
>>>> +
>>>> +	signed long temp_passive;
>>>> +	signed long temp_critical;
>>>> +	signed long temp_force_shut;
>>>> +	signed long alarm_temp;
>>>> +	signed long last_temp;
>>>> +	bool irq_enabled;
>>>> +	int irq;
>>>> +	struct clk *clk;
>>>> +	struct clk *pclk;
>>>> +};
>>>> +
>>>> +struct rockchip_tsadc_platform_data {
>>>> +	u8 irq_en;
>>>> +	signed long temp_passive;
>>>> +	signed long temp_critical;
>>>> +	signed long temp_force_shut;
>>>> +	int passive_delay;
>>>> +	int polling_delay;
>>>> +
>>>> +	int (*irq_handle)(void __iomem *reg);
>>>> +	int (*initialize)(void __iomem *reg, signed long temp_force_shut);
>>>> +	int (*control)(void __iomem *reg, bool on);
>>>> +	u32 (*code_to_temp)(int temp);
>>>> +	u32 (*temp_to_code)(int temp);
>>>> +	void (*set_alarm_temp)(void __iomem *regs, signed long temp);
>>>> +};
>>>> +
>>>> +/*TSADC V2 Sensor info define:*/
>>>> +#define TSADCV2_AUTO_CON			0x04
>>>> +#define TSADCV2_INT_EN				0x08
>>>> +#define TSADCV2_INT_PD				0x0c
>>>> +#define TSADCV2_DATA1				0x24
>>>> +#define TSADCV2_COMP1_INT			0x34
>>>> +#define TSADCV2_COMP1_SHUT			0x44
>>>> +#define TSADCV2_AUTO_PERIOD			0x68
>>>> +#define TSADCV2_AUTO_PERIOD_HT			0x6c
>>>> +
>>>> +#define TSADCV2_AUTO_SRC1_EN			(1 << 5)
>>>> +#define TSADCV2_AUTO_EN				(1 << 0)
>>>> +#define TSADCV2_AUTO_DISABLE			~(1 << 0)
>>>> +#define TSADCV2_AUTO_STAS_BUSY			(1 << 16)
>>>> +#define TSADCV2_AUTO_STAS_BUSY_MASK		(1 << 16)
>>>> +#define TSADCV2_SHUT_2GPIO_SRC1_EN		(1 << 5)
>>>> +#define TSADCV2_INT_SRC1_EN			(1 << 1)
>>>> +#define TSADCV2_SHUT_SRC1_STATUS		(1 << 5)
>>>> +#define TSADCV2_INT_SRC1_STATUS			(1 << 1)
>>>> +
>>> The above bits can be defined with BIT() macro, right?
>> Yes. The Bit() will be more resonable..
>>>    Something like:
>>> +#define TSADCV2_INT_SRC1_STATUS			BIT(1)
>>>
>>>
>>>> +#define TSADCV2_DATA_MASK			0xfff
>>>> +#define TSADCV2_HIGHT_INT_DEBOUNCE		0x60
>>>> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE		0x64
>>>> +#define TSADCV2_HIGHT_INT_DEBOUNCE_TIME		0x0a
>>>> +#define TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME	0x0a
>>>> +#define TSADCV2_AUTO_PERIOD_TIME		0x03e8
>>>> +#define TSADCV2_AUTO_PERIOD_HT_TIME		0x64
>>>> +
>>>> +struct tsadc_table {
>>>> +	int code;
>>>> +	int temp;
>>>> +};
>>>> +
>>>> +static const struct tsadc_table v2_code_table[] = {
>>>> +	{TSADCV2_DATA_MASK, -40},
>>>> +	{3800, -40},
>>>> +	{3792, -35},
>>>> +	{3783, -30},
>>>> +	{3774, -25},
>>>> +	{3765, -20},
>>>> +	{3756, -15},
>>>> +	{3747, -10},
>>>> +	{3737, -5},
>>>> +	{3728, 0},
>>>> +	{3718, 5},
>>>> +	{3708, 10},
>>>> +	{3698, 15},
>>>> +	{3688, 20},
>>>> +	{3678, 25},
>>>> +	{3667, 30},
>>>> +	{3656, 35},
>>>> +	{3645, 40},
>>>> +	{3634, 45},
>>>> +	{3623, 50},
>>>> +	{3611, 55},
>>>> +	{3600, 60},
>>>> +	{3588, 65},
>>>> +	{3575, 70},
>>>> +	{3563, 75},
>>>> +	{3550, 80},
>>>> +	{3537, 85},
>>>> +	{3524, 90},
>>>> +	{3510, 95},
>>>> +	{3496, 100},
>>>> +	{3482, 105},
>>>> +	{3467, 110},
>>>> +	{3452, 115},
>>>> +	{3437, 120},
>>>> +	{3421, 125},
>>>> +	{0, 125},
>>>> +};
>>>> +
>>>> +static int rk_tsadcv2_irq_handle(void __iomem *regs)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	val = readl_relaxed(regs + TSADCV2_INT_PD);
>>>> +	writel_relaxed(val & ~(1 << 8), regs + TSADCV2_INT_PD);
>>> Why do you need to clear bit 8? Why hardcoded?
>> The bit 8 set 0 to clear the interrupt.
>>
> Would it make sense to create a macro for this?
>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static u32 rk_tsadcv2_temp_to_code(int temp)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
>>>> +		if (temp <= v2_code_table[i].temp && temp >
>>>> +		    v2_code_table[i - 1].temp)
>>>> +			return v2_code_table[i].code;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static u32 rk_tsadcv2_code_to_temp(int code)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
>>>> +		if (code <= v2_code_table[i].code && code >
>>>> +		    v2_code_table[i + 1].code){
>>>> +			return v2_code_table[i].temp;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>> I know the table is not something too big, but considering it is ordered
>>> by either ADC value code or by temp, at least one of the above searching function
>>> may be more efficient, right?

In general,I should use dichotomy,Say, as the follows:

static u32 rk_tsadcv2_temp_to_code(int temp)
{
   int low = 0, high, mid;
   high =  ARRAY_SIZE(v2_code_table) - 1;
   ......

     while(high >= low) {
             mid = (high + low)/2;
             if (v2_code_table[mid].temp == temp)
                 return v2_code_table[mid].code;
             if (v2_code_table[mid].temp > temp)
                 high = mid - 1;
             if (v2_code_table[mid].temp < temp)
                 low = mid + 1;
     }
}

but, I better like to use the follows than the about.

static u32 rk_tsadcv2_temp_to_code(int temp)
{
     int i;

     for (i = 0; i < ARRAY_SIZE(v2_code_table) - 1; i++) {
         if (temp <= v2_code_table[i].temp)
             return v2_code_table[i].code;
     }

     return 0;
}

Do you agree?

I sent Patch v5,you can review again,if you are available.
>> Yes, use the point will be more efficient.
>>>> +
>>>> +static int rk_tsadcv2_initialize(void __iomem *regs,
>>>> +				 signed long temp_force_shut)
>>>> +{
>>>> +	int shutdown_value;
>>>> +
>>>> +	shutdown_value = rk_tsadcv2_temp_to_code(temp_force_shut);
>>>> +	/* Enable measurements at ~ 10 Hz */
>>> Does it leave the sampling clock at 10Hz?
>> yes.
> ok
>
>
>>> Is this clock exposed via
>>> clock framework?
>> The clock is divided by data->clk.
> ok
>
>>>> +	writel_relaxed(0, regs + TSADCV2_AUTO_CON);
>>>> +	writel_relaxed(TSADCV2_AUTO_PERIOD_TIME, regs + TSADCV2_AUTO_PERIOD);
>>>> +	writel_relaxed(TSADCV2_AUTO_PERIOD_HT_TIME, regs +
>>>> +		       TSADCV2_AUTO_PERIOD_HT);
>>>> +	writel_relaxed(shutdown_value, regs + TSADCV2_COMP1_SHUT);
>>>> +	writel_relaxed(TSADCV2_HIGHT_INT_DEBOUNCE_TIME, regs +
>>>> +		       TSADCV2_HIGHT_INT_DEBOUNCE);
>>>> +	writel_relaxed(TSADCV2_HIGHT_TSHUT_DEBOUNCE_TIME, regs +
>>>> +		       TSADCV2_HIGHT_TSHUT_DEBOUNCE);
>>>> +	writel_relaxed(TSADCV2_SHUT_2GPIO_SRC1_EN | TSADCV2_INT_SRC1_EN, regs +
>>>> +		       TSADCV2_INT_EN);
>>>> +	writel_relaxed(TSADCV2_AUTO_SRC1_EN | TSADCV2_AUTO_EN, regs +
>>>> +		       TSADCV2_AUTO_CON);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rk_tsadcv2_control(void __iomem *regs, bool on)
>>>> +{
>>>> +	u32 val;
>>>> +
>>>> +	if (on) {
>>>> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>>>> +		writel_relaxed(val | TSADCV2_AUTO_EN, regs + TSADCV2_AUTO_CON);
>>>> +	} else {
>>>> +		val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>>>> +		writel_relaxed(val & TSADCV2_AUTO_DISABLE,
>>>> +			       regs + TSADCV2_AUTO_CON);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void rk_tsadcv2_alarm_temp(void __iomem *regs, signed long alarm_temp)
>>>> +{
>>>> +	int alarm_value;
>>>> +
>>>> +	alarm_value = rk_tsadcv2_temp_to_code(alarm_temp);
>>>> +	writel_relaxed(alarm_value, regs + TSADCV2_COMP1_INT);
>>>> +}
>>>> +
>>>> +struct rockchip_tsadc_platform_data const rk3288_tsadc_data = {
>>>> +	.irq_en = 1,
>>>> +	.temp_passive = 85000,
>>>> +	.temp_critical = 100000,
>>>> +	.temp_force_shut = 120000,
>>>> +	.passive_delay = 2000,
>>>> +	.polling_delay = 1000,
>>>> +	.irq_handle = rk_tsadcv2_irq_handle,
>>>> +	.initialize = rk_tsadcv2_initialize,
>>>> +	.control = rk_tsadcv2_control,
>>>> +	.code_to_temp = rk_tsadcv2_code_to_temp,
>>>> +	.temp_to_code = rk_tsadcv2_temp_to_code,
>>>> +	.set_alarm_temp = rk_tsadcv2_alarm_temp,
>>>> +};
>>> shall the above struct be also static?
>> OK.
>>>> +
>>>> +static const struct of_device_id of_rockchip_thermal_match[] = {
>>>> +	{
>>>> +		.compatible = "rockchip,rk3288-tsadc",
>>>> +		.data = (void *)&rk3288_tsadc_data,
>>>> +	},
>>>> +	{ /* end */ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, of_rockchip_thermal_match);
>>>> +
>>>> +static void rockchip_set_alarm_temp(struct rockchip_thermal_data *data,
>>>> +				    signed long alarm_temp)
>>>> +{
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	data->alarm_temp = alarm_temp;
>>>> +	if (p_tsadc_data->set_alarm_temp)
>>>> +		p_tsadc_data->set_alarm_temp(data->regs, alarm_temp);
>>>> +}
>>>> +
>>>> +static int rockchip_get_temp(struct thermal_zone_device *tz,
>>>> +			     unsigned long *temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +	u32 val;
>>>> +
>>>> +	val = readl_relaxed(data->regs + TSADCV2_DATA1);
>>>> +	*temp = p_tsadc_data->code_to_temp(val);
>>>> +
>>>> +	/* Update alarm value to next higher trip point */
>>>> +	if (data->alarm_temp == data->temp_passive && *temp >=
>>>> +	    data->temp_passive)
>>>> +		rockchip_set_alarm_temp(data, data->temp_critical);
>>>> +
>>>> +	if (data->alarm_temp == data->temp_critical && *temp <
>>>> +	    data->temp_passive) {
>>>> +		rockchip_set_alarm_temp(data, data->temp_passive);
>>>> +		dev_dbg(&tz->device, "thermal alarm off: T < %lu\n",
>>>> +			data->alarm_temp / 1000);
>>>> +	}
>>>> +
>>>> +	if (*temp != data->last_temp) {
>>>> +		dev_dbg(&tz->device, "millicelsius: %ld\n", *temp);
>>>> +		data->last_temp = *temp;
>>>> +	}
>>>> +
>>>> +	/* Reenable alarm IRQ if temperature below alarm temperature */
>>>> +	if (!data->irq_enabled && *temp < data->alarm_temp) {
>>>> +		data->irq_enabled = true;
>>>> +		enable_irq(data->irq);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_initialize(struct rockchip_thermal_data *data)
>>>> +{
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	if (p_tsadc_data->initialize)
>>>> +		p_tsadc_data->initialize(data->regs, data->temp_force_shut);
>>>> +	rockchip_set_alarm_temp(data, data->temp_passive);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void rockchip_thermal_control(struct rockchip_thermal_data *data,
>>>> +				     bool on)
>>>> +{
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	if (p_tsadc_data->control)
>>>> +		p_tsadc_data->control(data->regs, on);
>>>> +
>>>> +	if (on) {
>>>> +		data->irq_enabled = true;
>>>> +		data->mode = THERMAL_DEVICE_ENABLED;
>>>> +	} else {
>>>> +		data->irq_enabled = false;
>>>> +		data->mode = THERMAL_DEVICE_DISABLED;
>>>> +	}
>>>> +}
>>>> +
>>>> +static int rockchip_get_mode(struct thermal_zone_device *tz,
>>>> +			     enum thermal_device_mode *mode)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	*mode = data->mode;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_set_mode(struct thermal_zone_device *tz,
>>>> +			     enum thermal_device_mode mode)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	if (mode == THERMAL_DEVICE_ENABLED) {
>>>> +		tz->polling_delay = p_tsadc_data->polling_delay;
>>>> +		tz->passive_delay = p_tsadc_data->passive_delay;
>>>> +		if (!data->irq_enabled) {
>>>> +			data->irq_enabled = true;
>>>> +			enable_irq(data->irq);
>>>> +		}
>>>> +	} else {
>>>> +		tz->polling_delay = 0;
>>>> +		tz->passive_delay = 0;
>>>> +		if (data->irq_enabled) {
>>>> +			disable_irq(data->irq);
>>>> +			data->irq_enabled = false;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	data->mode = mode;
>>>> +	thermal_zone_device_update(tz);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_get_trip_type(struct thermal_zone_device *tz, int trip,
>>>> +				  enum thermal_trip_type *type)
>>>> +{
>>>> +	*type = (trip == ROCKCHIP_TRIP_PASSIVE) ? THERMAL_TRIP_PASSIVE :
>>>> +		 THERMAL_TRIP_CRITICAL;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_get_crit_temp(struct thermal_zone_device *tz,
>>>> +				  unsigned long *temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	*temp = data->temp_critical;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_get_trip_temp(struct thermal_zone_device *tz, int trip,
>>>> +				  unsigned long *temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	*temp = (trip == ROCKCHIP_TRIP_PASSIVE) ? data->temp_passive :
>>>> +		 data->temp_critical;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_set_trip_temp(struct thermal_zone_device *tz, int trip,
>>>> +				  unsigned long temp)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = tz->devdata;
>>>> +
>>>> +	if (trip == ROCKCHIP_TRIP_CRITICAL)
>>>> +		return -EPERM;
>>>> +
>>>> +	data->temp_passive = temp;
>>>> +	rockchip_set_alarm_temp(data, temp);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_bind(struct thermal_zone_device *tz,
>>>> +			 struct thermal_cooling_device *cdev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = thermal_zone_bind_cooling_device(tz, ROCKCHIP_TRIP_PASSIVE, cdev,
>>>> +					       THERMAL_NO_LIMIT,
>>>> +					       THERMAL_NO_LIMIT);
>>>> +	if (ret) {
>>>> +		dev_err(&tz->device, "binding zone %s with cdev %s failed:%d\n",
>>>> +			tz->type, cdev->type, ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_unbind(struct thermal_zone_device *tz,
>>>> +			   struct thermal_cooling_device *cdev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = thermal_zone_unbind_cooling_device(tz,
>>>> +						 ROCKCHIP_TRIP_PASSIVE, cdev);
>>>> +	if (ret) {
>>>> +		dev_err(&tz->device,
>>>> +			"unbinding zone %s with cdev %s failed:%d\n", tz->type,
>>>> +			cdev->type, ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>> The method of binding and unbinding used above requires you to check if
>>> you are binding to the right cdev. If in your system you register more
>>> than one cdev, say someone loads the power supply core, which in turns
>>> register a cooling device for charging, your thermal zone will bind it
>>> to TRIP_PASSIVE. It will happen to any cooling device that eventually
>>> gets registered to the thermal framework. Is that the desired outcome?
>> Yes,I will use the generic trip points in the dts and fix it in the
>> thermal driver.
> great
>
>>> If not, you may want to compare the paramenter cdev to your data->cdev.
>>>
>>>> +
>>>> +static struct thermal_zone_device_ops rockchip_tz_ops = {
>>>> +	.bind = rockchip_bind,
>>>> +	.unbind = rockchip_unbind,
>>>> +	.get_temp = rockchip_get_temp,
>>>> +	.get_mode = rockchip_get_mode,
>>>> +	.set_mode = rockchip_set_mode,
>>>> +	.get_trip_type = rockchip_get_trip_type,
>>>> +	.get_trip_temp = rockchip_get_trip_temp,
>>>> +	.get_crit_temp = rockchip_get_crit_temp,
>>>> +	.set_trip_temp = rockchip_set_trip_temp,
>>>> +};
>>>> +
>>>> +static irqreturn_t rockchip_thermal_alarm_irq_thread(int irq, void *dev)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = data;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data = data->pdata;
>>>> +
>>>> +	dev_dbg(&data->tz->device, "THERMAL ALARM: T > %lu\n",
>>>> +		data->alarm_temp / 1000);
>>>> +
>>>> +	if (p_tsadc_data->irq_en && p_tsadc_data->irq_handle)
>>>> +		p_tsadc_data->irq_handle(data->regs);
>>>> +
>>>> +	thermal_zone_device_update(data->tz);
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct rockchip_thermal_data *data;
>>>> +	const struct rockchip_tsadc_platform_data *p_tsadc_data;
>>>> +	struct cpumask clip_cpus;
>>>> +	struct resource *res;
>>>> +	const struct of_device_id *match;
>>>> +	int ret, temp;
>>>> +
>>>> +	data = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_thermal_data),
>>>> +			    GFP_KERNEL);
>>>> +	if (!data)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	data->regs = devm_ioremap_resource(&pdev->dev, res);
>>>> +	if (IS_ERR(data->regs)) {
>>>> +		dev_err(&pdev->dev, "Could not get tsadc source, %p\n",
>>>> +			data->regs);
>>>> +		return PTR_ERR(data->regs);
>>>> +	}
>>>> +
>>>> +	match = of_match_node(of_rockchip_thermal_match, pdev->dev.of_node);
>>>> +	if (!match)
>>>> +		return -ENXIO;
>>>> +	data->pdata = (const struct rockchip_tsadc_platform_data *)match->data;
>>>> +	if (!data->pdata)
>>>> +		return -EINVAL;
>>>> +	p_tsadc_data = data->pdata;
>>>> +
>>>> +	data->clk = devm_clk_get(&pdev->dev, "tsadc");
>>>> +	if (IS_ERR(data->clk)) {
>>>> +		dev_err(&pdev->dev, "failed to get tsadc clock\n");
>>>> +		return PTR_ERR(data->clk);
>>>> +	}
>>>> +
>>>> +	data->pclk = devm_clk_get(&pdev->dev, "apb_pclk");
>>>> +	if (IS_ERR(data->pclk)) {
>>>> +		dev_err(&pdev->dev, "failed to get tsadc pclk\n");
>>>> +		return PTR_ERR(data->pclk);
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	* Use a default of 10KHz for the converter clock.
>>>> +	* This may become user-configurable in the future.
>>>> +	*/
>>>> +	ret = clk_set_rate(data->clk, 10000);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "failed to set tsadc clk rate, %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(data->clk);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "failed to enable converter clock\n");
>>>> +		goto err_clk;
>>>> +	}
>>>> +
>>>> +	ret = clk_prepare_enable(data->pclk);
>>>> +	if (ret < 0) {
>>>> +		dev_err(&pdev->dev, "failed to enable pclk\n");
>>>> +		goto err_pclk;
>>>> +	}
>>>> +
>>>> +	platform_set_drvdata(pdev, data);
>>>> +
>>>> +	if (of_property_read_u32(pdev->dev.of_node, "passive-temp", &temp)) {
>>>> +		dev_warn(&pdev->dev,
>>>> +			 "Missing default passive temp property in the DT.\n");
>>>> +		data->temp_passive = p_tsadc_data->temp_passive;
>>>> +	} else {
>>>> +		data->temp_passive = temp;
>>>> +	}
>>>> +
>>>> +	if (of_property_read_u32(pdev->dev.of_node, "critical-temp", &temp)) {
>>>> +		dev_warn(&pdev->dev,
>>>> +			 "Missing default critical temp property in the DT.\n");
>>>> +		data->temp_critical = p_tsadc_data->temp_critical;
>>>> +	} else {
>>>> +		data->temp_critical = temp;
>>>> +	}
>>>> +
>>>> +	if (of_property_read_u32(pdev->dev.of_node, "force-shut-temp", &temp)) {
>>>> +		dev_warn(&pdev->dev,
>>>> +			 "Missing default force shut down temp property in the DT.\n");
>>>> +		data->temp_force_shut = p_tsadc_data->temp_force_shut;
>>>> +	} else {
>>>> +		data->temp_force_shut = temp;
>>>> +	}
>>>> +
>>>> +	cpumask_set_cpu(0, &clip_cpus);
>>>> +	data->cdev = of_cpufreq_cooling_register(pdev->dev.of_node, &clip_cpus);
>>>> +	if (IS_ERR(data->cdev)) {
>>>> +		dev_err(&pdev->dev, "failed to register cpufreq cooling device\n");
>>>> +		goto disable_clk;
>>>> +	}
>>>> +
>>>> +	data->tz = thermal_zone_device_register("rockchip_thermal",
>>>> +						ROCKCHIP_TRIP_NUM,
>>>> +						0, data,
>>>> +						&rockchip_tz_ops, NULL,
>>>> +						p_tsadc_data->passive_delay,
>>>> +						p_tsadc_data->polling_delay);
>>>> +
>>>> +	if (IS_ERR(data->tz)) {
>>>> +		dev_err(&pdev->dev, "failed to register thermal zone device\n");
>>>> +		goto fail_cpufreq_register;
>>>> +	}
>>>> +
>>>> +	if (p_tsadc_data->irq_en) {
>>>> +		data->irq = platform_get_irq(pdev, 0);
>>>> +		if (data->irq < 0) {
>>>> +			dev_err(&pdev->dev, "no irq resource?\n");
>>>> +			goto fail_irq;
>>>> +		}
>>>> +
>>>> +		ret = devm_request_threaded_irq(&pdev->dev, data->irq, NULL,
>>>> +				rockchip_thermal_alarm_irq_thread,
>>>> +				IRQF_ONESHOT, "rockchip_thermal", data);
>>>> +		if (ret < 0) {
>>>> +			dev_err(&pdev->dev,
>>>> +				"failed to request tsadc irq: %d\n", ret);
>>>> +			goto fail_thermal_unregister;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	rockchip_thermal_initialize(data);
>>>> +	rockchip_thermal_control(data, true);
>>>> +
>>>> +	return 0;
>>>> +
>>>> +fail_thermal_unregister:
>>>> +	thermal_zone_device_unregister(data->tz);
>>>> +fail_irq:
>>>> +fail_cpufreq_register:
>>>> +	cpufreq_cooling_unregister(data->cdev);
>>>> +disable_clk:
>>>> +err_pclk:
>>>> +	clk_disable_unprepare(data->pclk);
>>>> +err_clk:
>>>> +	clk_disable_unprepare(data->clk);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>>>> +
>>>> +	rockchip_thermal_control(data, false);
>>>> +
>>>> +	thermal_zone_device_unregister(data->tz);
>>>> +	cpufreq_cooling_unregister(data->cdev);
>>>> +	cpufreq_cooling_unregister(data->cdev);
>>>> +
>>> The above call is duplicated.
>> OK.
>>>> +	clk_disable_unprepare(data->clk);
>>>> +	clk_disable_unprepare(data->pclk);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int rockchip_thermal_suspend(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>>>> +
>>>> +	rockchip_thermal_control(data, false);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int rockchip_thermal_resume(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>>>> +
>>>> +	rockchip_thermal_initialize(data);
>>>> +	rockchip_thermal_control(data, true);
>>>> +
>>>> +	return 0;
>>>> +}
>>> Would you need to manage your clocks too in the suspend resume path
>>> (data->clk and data->pclk)?
>> yes.
>> Usually,it's need disable to save power.
> Yes, that's my point. Please consider handling clocks to improve power
> savings.
>
>>>> +#endif
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(rockchip_thermal_pm_ops,
>>>> +			 rockchip_thermal_suspend, rockchip_thermal_resume);
>>>> +
>>>> +static struct platform_driver rockchip_thermal_driver = {
>>>> +	.driver = {
>>>> +		   .name = "rockchip-thermal",
>>>> +		   .owner = THIS_MODULE,
>>>> +		   .pm = &rockchip_thermal_pm_ops,
>>>> +		   .of_match_table = of_rockchip_thermal_match,
>>>> +		   },
>>>> +	.probe = rockchip_thermal_probe,
>>>> +	.remove = rockchip_thermal_remove,
>>>> +};
>>>> +
>>>> +module_platform_driver(rockchip_thermal_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("ROCKCHIP THERMAL Driver");
>>>> +MODULE_AUTHOR("Rockchip, Inc.");
>>>> +MODULE_LICENSE("GPL");
>>> Your file header states GPL version two. Thus I suppose you meant:
>>> +MODULE_LICENSE("GPL v2");
>>>
>>> right?
>>>
>>> Check include/linux/module.h for further clarifications.
>> But I think the  "GPL"  is support for thr GNU Public License v2 or later.
> Exactly, if you intend to support v3, then it is fine. But your code
> header states only V2.
>
>> I will fix GPL v2 if I get it wrong.
> If your license is GPLv2 only as mentioned in your header, please use 'GPL
> v2' tag.
>
> Cheers,
>
>>>> +MODULE_ALIAS("platform:rockchip-thermal");
>>>> -- 
>>>> 1.9.1
>>>>
>>>>
>>> BR,
>>>
>>> Eduardo Valentin
>>>
>>>
>>>
>> -- 
>> Best regards,
>> Caesar
>>
>>
>
>

-- 
Best regards,
Caesar

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Dmitry Torokhov]

Hi Caesar,

On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
> +static int rockchip_thermal_remove(struct platform_device *pdev)
> +{
> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
> +
> +	rockchip_thermal_control(data, false);
> +
> +	thermal_zone_device_unregister(data->tz);
> +	cpufreq_cooling_unregister(data->cdev);
> +	cpufreq_cooling_unregister(data->cdev);

It looks like we are unregistering the same cooling device one too many times.

Thanks.

-- 
Dmitry

[PATCH v4 1/4] thermal: rockchip: add driver for thermal

[Caesar Wang]

? 2014?09?05? 01:06, Dmitry Torokhov ??:
> Hi Caesar,
>
> On Wed, Sep 03, 2014 at 10:10:36AM +0800, Caesar Wang wrote:
>> +static int rockchip_thermal_remove(struct platform_device *pdev)
>> +{
>> +	struct rockchip_thermal_data *data = platform_get_drvdata(pdev);
>> +
>> +	rockchip_thermal_control(data, false);
>> +
>> +	thermal_zone_device_unregister(data->tz);
>> +	cpufreq_cooling_unregister(data->cdev);
>> +	cpufreq_cooling_unregister(data->cdev);
> It looks like we are unregistering the same cooling device one too many times.
>
> Thanks.
>
yes,I made a stupid mistake.  Thanks~

-- 
Best regards,
Caesar

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Caesar Wang]

This add the necessary binding documentation for the thermal
found on Rockchip SoCs

Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
---
 .../devicetree/bindings/thermal/rockchip-thermal.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/rockchip-thermal.txt

diff --git a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
new file mode 100644
index 0000000..1ed4d4c
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
@@ -0,0 +1,20 @@
+* Temperature Sensor ADC (TSADC) on rockchip SoCs
+
+Required properties:
+- compatible: "rockchip,rk3288-tsadc"
+- reg: physical base address of the controller and length of memory mapped
+       region.
+- interrupts: The interrupt number to the cpu. The interrupt specifier format
+	      depends on the interrupt controller.
+- clocks: Must contain an entry for each entry in clock-names.
+- clock-names: Shall be "tsadc" for the converter-clock, and "apb_pclk" for
+	       the peripheral clock.
+
+Example:
+tsadc: tsadc at ff280000 {
+	compatible = "rockchip,rk3288-tsadc";
+	reg = <0xff280000 0x100>;
+	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
+	clock-names = "tsadc", "apb_pclk";
+};
-- 
1.9.1

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Heiko Stübner]

Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> This add the necessary binding documentation for the thermal
> found on Rockchip SoCs
> 
> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> ---
>  .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> 
> diff --git a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new file
> mode 100644
> index 0000000..1ed4d4c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> @@ -0,0 +1,20 @@
> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> +
> +Required properties:
> +- compatible: "rockchip,rk3288-tsadc"
> +- reg: physical base address of the controller and length of memory mapped
> +       region.
> +- interrupts: The interrupt number to the cpu. The interrupt specifier
> format +	      depends on the interrupt controller.
> +- clocks: Must contain an entry for each entry in clock-names.
> +- clock-names: Shall be "tsadc" for the converter-clock, and "apb_pclk" for
> +	       the peripheral clock.

You're using the passive-temp, critical-temp and force-shut-temp properties in 
your driver without declaring them here.

But more importantly, please use the generic trip-points for this. I guess it 
shouldn't be a problem to introduce a "forced-shutdown" trippoint [0] for the 
additional trip-point you have - thermal maintainers, please shout if I'm 
wrong :-)


Heiko


[0] in a separate patch, changing
- thermal_trip_type enum in include/linux/thermal.h
- trip_types mapping in drivers/thermal/of-thermal.c
- Documentation/devicetree/bindings/thermal/thermal.txt

> +
> +Example:
> +tsadc: tsadc at ff280000 {
> +	compatible = "rockchip,rk3288-tsadc";
> +	reg = <0xff280000 0x100>;
> +	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
> +	clock-names = "tsadc", "apb_pclk";
> +};

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Caesar Wang]

? 2014?09?03? 16:07, Heiko St?bner ??:
> Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
>> This add the necessary binding documentation for the thermal
>> found on Rockchip SoCs
>>
>> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
>> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>> ---
>>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
>> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new file
>> mode 100644
>> index 0000000..1ed4d4c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>> @@ -0,0 +1,20 @@
>> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
>> +
>> +Required properties:
>> +- compatible: "rockchip,rk3288-tsadc"
>> +- reg: physical base address of the controller and length of memory mapped
>> +       region.
>> +- interrupts: The interrupt number to the cpu. The interrupt specifier
>> format +	      depends on the interrupt controller.
>> +- clocks: Must contain an entry for each entry in clock-names.
>> +- clock-names: Shall be "tsadc" for the converter-clock, and "apb_pclk" for
>> +	       the peripheral clock.
> You're using the passive-temp, critical-temp and force-shut-temp properties in
> your driver without declaring them here.

frankly,the about are need be declared. but  there are 4 types[0] for 
trip in thermal framework,
there is no force-shut for me. So I want to change it three additional 
properties in [PATCH V4 4/4],


[0]
{
     THERMAL_TRIP_CRITICAL,
     THERMAL_TRIP_HOT,
     THERMAL_TRIP_PASSIVE,
     THERMAL_TRIP_ACTIVE,
}

> But more importantly, please use the generic trip-points for this. I guess it
> shouldn't be a problem to introduce a "forced-shutdown" trippoint [0] for the
> additional trip-point you have - thermal maintainers, please shout if I'm
> wrong :-)

It's a good option.
I can send a patch,but I don't know whether the thermal maintainers will 
accept it.

Maybe,they have a better way to suggest it.:-)


PS:I will sent a new patch If I still have no received their suggestions 
in two days.

>
> Heiko
>
>
> [0] in a separate patch, changing
> - thermal_trip_type enum in include/linux/thermal.h
> - trip_types mapping in drivers/thermal/of-thermal.c
> - Documentation/devicetree/bindings/thermal/thermal.txt
>
>> +
>> +Example:
>> +tsadc: tsadc at ff280000 {
>> +	compatible = "rockchip,rk3288-tsadc";
>> +	reg = <0xff280000 0x100>;
>> +	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
>> +	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
>> +	clock-names = "tsadc", "apb_pclk";
>> +};
>
>
>

-- 
Best regards,
Caesar

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Zhang Rui]

On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> ? 2014?09?03? 16:07, Heiko St?bner ??:
> > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> >> This add the necessary binding documentation for the thermal
> >> found on Rockchip SoCs
> >>
> >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> >> ---
> >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> >>   create mode 100644
> >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new file
> >> mode 100644
> >> index 0000000..1ed4d4c
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >> @@ -0,0 +1,20 @@
> >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> >> +
> >> +Required properties:
> >> +- compatible: "rockchip,rk3288-tsadc"
> >> +- reg: physical base address of the controller and length of memory mapped
> >> +       region.
> >> +- interrupts: The interrupt number to the cpu. The interrupt specifier
> >> format +	      depends on the interrupt controller.
> >> +- clocks: Must contain an entry for each entry in clock-names.
> >> +- clock-names: Shall be "tsadc" for the converter-clock, and "apb_pclk" for
> >> +	       the peripheral clock.
> > You're using the passive-temp, critical-temp and force-shut-temp properties in
> > your driver without declaring them here.
> 
> frankly,the about are need be declared. but  there are 4 types[0] for 
> trip in thermal framework,
> there is no force-shut for me. So I want to change it three additional 
> properties in [PATCH V4 4/4],
> 
> 
> [0]
> {
>      THERMAL_TRIP_CRITICAL,
>      THERMAL_TRIP_HOT,
>      THERMAL_TRIP_PASSIVE,
>      THERMAL_TRIP_ACTIVE,
> }
> 
this sounds reasonable to me.

> > But more importantly, please use the generic trip-points for this. I guess it
> > shouldn't be a problem to introduce a "forced-shutdown" trippoint [0] for the
> > additional trip-point you have - thermal maintainers, please shout if I'm
> > wrong :-)
> 
what is the difference between a critical trip point and a
"forced-shutdown" trip point?
Thermal core will do a shutdown in case the critical trip point is
triggered.

thanks,
rui
> It's a good option.
> I can send a patch,but I don't know whether the thermal maintainers will 
> accept it.
> 
> Maybe,they have a better way to suggest it.:-)
> 
> 
> PS:I will sent a new patch If I still have no received their suggestions 
> in two days.
> 
> >
> > Heiko
> >
> >
> > [0] in a separate patch, changing
> > - thermal_trip_type enum in include/linux/thermal.h
> > - trip_types mapping in drivers/thermal/of-thermal.c
> > - Documentation/devicetree/bindings/thermal/thermal.txt
> >
> >> +
> >> +Example:
> >> +tsadc: tsadc at ff280000 {
> >> +	compatible = "rockchip,rk3288-tsadc";
> >> +	reg = <0xff280000 0x100>;
> >> +	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> >> +	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
> >> +	clock-names = "tsadc", "apb_pclk";
> >> +};
> >
> >
> >
> 

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Heiko Stübner]

Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
> On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> > ? 2014?09?03? 16:07, Heiko St?bner ??:
> > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> > >> This add the necessary binding documentation for the thermal
> > >> found on Rockchip SoCs
> > >> 
> > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> > >> ---
> > >> 
> > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> > >> 
> > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> > >> 
> > >>   create mode 100644
> > >> 
> > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > >> 
> > >> diff --git
> > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new
> > >> file
> > >> mode 100644
> > >> index 0000000..1ed4d4c
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > >> @@ -0,0 +1,20 @@
> > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> > >> +
> > >> +Required properties:
> > >> +- compatible: "rockchip,rk3288-tsadc"
> > >> +- reg: physical base address of the controller and length of memory
> > >> mapped
> > >> +       region.
> > >> +- interrupts: The interrupt number to the cpu. The interrupt specifier
> > >> format +	      depends on the interrupt controller.
> > >> +- clocks: Must contain an entry for each entry in clock-names.
> > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
> > >> "apb_pclk" for +	       the peripheral clock.
> > > 
> > > You're using the passive-temp, critical-temp and force-shut-temp
> > > properties in your driver without declaring them here.
> > 
> > frankly,the about are need be declared. but  there are 4 types[0] for
> > trip in thermal framework,
> > there is no force-shut for me. So I want to change it three additional
> > properties in [PATCH V4 4/4],
> > 
> > 
> > [0]
> > {
> > 
> >      THERMAL_TRIP_CRITICAL,
> >      THERMAL_TRIP_HOT,
> >      THERMAL_TRIP_PASSIVE,
> >      THERMAL_TRIP_ACTIVE,
> > 
> > }
> 
> this sounds reasonable to me.
> 
> > > But more importantly, please use the generic trip-points for this. I
> > > guess it shouldn't be a problem to introduce a "forced-shutdown"
> > > trippoint [0] for the additional trip-point you have - thermal
> > > maintainers, please shout if I'm wrong :-)
> 
> what is the difference between a critical trip point and a
> "forced-shutdown" trip point?
> Thermal core will do a shutdown in case the critical trip point is
> triggered.

The forced-shutdown is where the thermal controller is supposed to also do a 
shutdown in hardware. As you said the thermal core will also shutdown at the 
critical trip point, I guess we could map Caesar's value like

trip-point		tsadc
critical		forced-shutdown (the 120 degrees in patch 4)
hot			critical (the 100 degrees)
...


> 
> thanks,
> rui
> 
> > It's a good option.
> > I can send a patch,but I don't know whether the thermal maintainers will
> > accept it.
> > 
> > Maybe,they have a better way to suggest it.:-)
> > 
> > 
> > PS:I will sent a new patch If I still have no received their suggestions
> > in two days.
> > 
> > > Heiko
> > > 
> > > 
> > > [0] in a separate patch, changing
> > > - thermal_trip_type enum in include/linux/thermal.h
> > > - trip_types mapping in drivers/thermal/of-thermal.c
> > > - Documentation/devicetree/bindings/thermal/thermal.txt
> > > 
> > >> +
> > >> +Example:
> > >> +tsadc: tsadc at ff280000 {
> > >> +	compatible = "rockchip,rk3288-tsadc";
> > >> +	reg = <0xff280000 0x100>;
> > >> +	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> > >> +	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
> > >> +	clock-names = "tsadc", "apb_pclk";
> > >> +};

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Eduardo Valentin]

Hello

On Tue, Sep 09, 2014 at 01:35:31PM +0200, Heiko St?bner wrote:
> Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
> > On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> > > ? 2014?09?03? 16:07, Heiko St?bner ??:
> > > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> > > >> This add the necessary binding documentation for the thermal
> > > >> found on Rockchip SoCs
> > > >> 
> > > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> > > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> > > >> ---
> > > >> 
> > > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> > > >> 
> > > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> > > >> 
> > > >>   create mode 100644
> > > >> 
> > > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > >> 
> > > >> diff --git
> > > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new
> > > >> file
> > > >> mode 100644
> > > >> index 0000000..1ed4d4c
> > > >> --- /dev/null
> > > >> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > >> @@ -0,0 +1,20 @@
> > > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> > > >> +
> > > >> +Required properties:
> > > >> +- compatible: "rockchip,rk3288-tsadc"
> > > >> +- reg: physical base address of the controller and length of memory
> > > >> mapped
> > > >> +       region.
> > > >> +- interrupts: The interrupt number to the cpu. The interrupt specifier
> > > >> format +	      depends on the interrupt controller.
> > > >> +- clocks: Must contain an entry for each entry in clock-names.
> > > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
> > > >> "apb_pclk" for +	       the peripheral clock.
> > > > 
> > > > You're using the passive-temp, critical-temp and force-shut-temp
> > > > properties in your driver without declaring them here.
> > > 
> > > frankly,the about are need be declared. but  there are 4 types[0] for
> > > trip in thermal framework,
> > > there is no force-shut for me. So I want to change it three additional
> > > properties in [PATCH V4 4/4],
> > > 
> > > 
> > > [0]
> > > {
> > > 
> > >      THERMAL_TRIP_CRITICAL,
> > >      THERMAL_TRIP_HOT,
> > >      THERMAL_TRIP_PASSIVE,
> > >      THERMAL_TRIP_ACTIVE,
> > > 
> > > }
> > 
> > this sounds reasonable to me.
> > 
> > > > But more importantly, please use the generic trip-points for this. I
> > > > guess it shouldn't be a problem to introduce a "forced-shutdown"
> > > > trippoint [0] for the additional trip-point you have - thermal
> > > > maintainers, please shout if I'm wrong :-)
> > 
> > what is the difference between a critical trip point and a
> > "forced-shutdown" trip point?
> > Thermal core will do a shutdown in case the critical trip point is
> > triggered.
> 
> The forced-shutdown is where the thermal controller is supposed to also do a 

Currently, there is no discrimination between hardware configured /
triggered thermal shutdown and software detected / triggered thermal shutdown.
One way to implement it though is to reuse the critical trip type. Even
if you use more than one trip type it is doable, it will depend on the
priorities you give to software triggered and hardware triggered.

> shutdown in hardware. As you said the thermal core will also shutdown at the 
> critical trip point, I guess we could map Caesar's value like
> 
> trip-point		tsadc
> critical		forced-shutdown (the 120 degrees in patch 4)

> hot			critical (the 100 degrees)
> ...
> 
> 

In the case we are planing to expand the trip type range, adding one
specific to hardware configurable shutdown makes sense to me too.
Alhtough, as I mention, I believe with the current generic trip types,
this situation can be covered already. Besides, I believe
'forced-shutdown' does not sound a descriptive enough though. I would
suggest something more specific, say 'hardware-shutdown'. 

> > 
> > thanks,
> > rui
> > 
> > > It's a good option.
> > > I can send a patch,but I don't know whether the thermal maintainers will
> > > accept it.
> > > 
> > > Maybe,they have a better way to suggest it.:-)
> > > 
> > > 
> > > PS:I will sent a new patch If I still have no received their suggestions
> > > in two days.
> > > 
> > > > Heiko
> > > > 
> > > > 
> > > > [0] in a separate patch, changing
> > > > - thermal_trip_type enum in include/linux/thermal.h
> > > > - trip_types mapping in drivers/thermal/of-thermal.c
> > > > - Documentation/devicetree/bindings/thermal/thermal.txt
> > > > 
> > > >> +
> > > >> +Example:
> > > >> +tsadc: tsadc at ff280000 {
> > > >> +	compatible = "rockchip,rk3288-tsadc";
> > > >> +	reg = <0xff280000 0x100>;
> > > >> +	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> > > >> +	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
> > > >> +	clock-names = "tsadc", "apb_pclk";
> > > >> +};
> 

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Zhang Rui]

On Tue, 2014-09-09 at 11:09 -0400, Eduardo Valentin wrote:
> Hello
> 
> On Tue, Sep 09, 2014 at 01:35:31PM +0200, Heiko St?bner wrote:
> > Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
> > > On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> > > > ? 2014?09?03? 16:07, Heiko St?bner ??:
> > > > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> > > > >> This add the necessary binding documentation for the thermal
> > > > >> found on Rockchip SoCs
> > > > >> 
> > > > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> > > > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> > > > >> ---
> > > > >> 
> > > > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> > > > >> 
> > > > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> > > > >> 
> > > > >>   create mode 100644
> > > > >> 
> > > > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > > >> 
> > > > >> diff --git
> > > > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new
> > > > >> file
> > > > >> mode 100644
> > > > >> index 0000000..1ed4d4c
> > > > >> --- /dev/null
> > > > >> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > > >> @@ -0,0 +1,20 @@
> > > > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> > > > >> +
> > > > >> +Required properties:
> > > > >> +- compatible: "rockchip,rk3288-tsadc"
> > > > >> +- reg: physical base address of the controller and length of memory
> > > > >> mapped
> > > > >> +       region.
> > > > >> +- interrupts: The interrupt number to the cpu. The interrupt specifier
> > > > >> format +	      depends on the interrupt controller.
> > > > >> +- clocks: Must contain an entry for each entry in clock-names.
> > > > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
> > > > >> "apb_pclk" for +	       the peripheral clock.
> > > > > 
> > > > > You're using the passive-temp, critical-temp and force-shut-temp
> > > > > properties in your driver without declaring them here.
> > > > 
> > > > frankly,the about are need be declared. but  there are 4 types[0] for
> > > > trip in thermal framework,
> > > > there is no force-shut for me. So I want to change it three additional
> > > > properties in [PATCH V4 4/4],
> > > > 
> > > > 
> > > > [0]
> > > > {
> > > > 
> > > >      THERMAL_TRIP_CRITICAL,
> > > >      THERMAL_TRIP_HOT,
> > > >      THERMAL_TRIP_PASSIVE,
> > > >      THERMAL_TRIP_ACTIVE,
> > > > 
> > > > }
> > > 
> > > this sounds reasonable to me.
> > > 
> > > > > But more importantly, please use the generic trip-points for this. I
> > > > > guess it shouldn't be a problem to introduce a "forced-shutdown"
> > > > > trippoint [0] for the additional trip-point you have - thermal
> > > > > maintainers, please shout if I'm wrong :-)
> > > 
> > > what is the difference between a critical trip point and a
> > > "forced-shutdown" trip point?
> > > Thermal core will do a shutdown in case the critical trip point is
> > > triggered.
> > 
> > The forced-shutdown is where the thermal controller is supposed to also do a 
> 
> Currently, there is no discrimination between hardware configured /
> triggered thermal shutdown and software detected / triggered thermal shutdown.
> One way to implement it though is to reuse the critical trip type. Even
> if you use more than one trip type it is doable, it will depend on the
> priorities you give to software triggered and hardware triggered.
> 
> > shutdown in hardware. As you said the thermal core will also shutdown at the 
> > critical trip point, I guess we could map Caesar's value like
> > 
> > trip-point		tsadc
> > critical		forced-shutdown (the 120 degrees in patch 4)
> 
> > hot			critical (the 100 degrees)
> > ...
> > 
> > 
> 
> In the case we are planing to expand the trip type range, adding one
> specific to hardware configurable shutdown makes sense to me too.
hmmm, why? you don't want an orderly shutdown? I still do not understand
why we need a hardware shutdown trip point.
Say, if we expect the system to be shutdown at 100C, I don't think we
have a chance to trigger the hardware shutdown trip point.
Further more, if my understanding is right, thermal core won't do
anything for the hardware shutdown trip point because the system will be
shutdown automatically, right? If this is true, why bother introducing
this to thermal core?

thanks,
rui
> Alhtough, as I mention, I believe with the current generic trip types,
> this situation can be covered already.
>  Besides, I believe
> 'forced-shutdown' does not sound a descriptive enough though. I would
> suggest something more specific, say 'hardware-shutdown'. 
> 
> > > 
> > > thanks,
> > > rui
> > > 
> > > > It's a good option.
> > > > I can send a patch,but I don't know whether the thermal maintainers will
> > > > accept it.
> > > > 
> > > > Maybe,they have a better way to suggest it.:-)
> > > > 
> > > > 
> > > > PS:I will sent a new patch If I still have no received their suggestions
> > > > in two days.
> > > > 
> > > > > Heiko
> > > > > 
> > > > > 
> > > > > [0] in a separate patch, changing
> > > > > - thermal_trip_type enum in include/linux/thermal.h
> > > > > - trip_types mapping in drivers/thermal/of-thermal.c
> > > > > - Documentation/devicetree/bindings/thermal/thermal.txt
> > > > > 
> > > > >> +
> > > > >> +Example:
> > > > >> +tsadc: tsadc at ff280000 {
> > > > >> +	compatible = "rockchip,rk3288-tsadc";
> > > > >> +	reg = <0xff280000 0x100>;
> > > > >> +	interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> > > > >> +	clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
> > > > >> +	clock-names = "tsadc", "apb_pclk";
> > > > >> +};
> > 
> 

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[edubezval at gmail.com]

Hello,

On Tue, Sep 9, 2014 at 9:02 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> On Tue, 2014-09-09 at 11:09 -0400, Eduardo Valentin wrote:
>> Hello
>>
>> On Tue, Sep 09, 2014 at 01:35:31PM +0200, Heiko St?bner wrote:
>> > Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
>> > > On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
>> > > > ? 2014?09?03? 16:07, Heiko St?bner ??:
>> > > > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
>> > > > >> This add the necessary binding documentation for the thermal
>> > > > >> found on Rockchip SoCs
>> > > > >>
>> > > > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
>> > > > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>> > > > >> ---
>> > > > >>
>> > > > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
>> > > > >>
>> > > > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
>> > > > >>
>> > > > >>   create mode 100644
>> > > > >>
>> > > > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>> > > > >>
>> > > > >> diff --git
>> > > > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt new
>> > > > >> file
>> > > > >> mode 100644
>> > > > >> index 0000000..1ed4d4c
>> > > > >> --- /dev/null
>> > > > >> +++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>> > > > >> @@ -0,0 +1,20 @@
>> > > > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
>> > > > >> +
>> > > > >> +Required properties:
>> > > > >> +- compatible: "rockchip,rk3288-tsadc"
>> > > > >> +- reg: physical base address of the controller and length of memory
>> > > > >> mapped
>> > > > >> +       region.
>> > > > >> +- interrupts: The interrupt number to the cpu. The interrupt specifier
>> > > > >> format +           depends on the interrupt controller.
>> > > > >> +- clocks: Must contain an entry for each entry in clock-names.
>> > > > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
>> > > > >> "apb_pclk" for +            the peripheral clock.
>> > > > >
>> > > > > You're using the passive-temp, critical-temp and force-shut-temp
>> > > > > properties in your driver without declaring them here.
>> > > >
>> > > > frankly,the about are need be declared. but  there are 4 types[0] for
>> > > > trip in thermal framework,
>> > > > there is no force-shut for me. So I want to change it three additional
>> > > > properties in [PATCH V4 4/4],
>> > > >
>> > > >
>> > > > [0]
>> > > > {
>> > > >
>> > > >      THERMAL_TRIP_CRITICAL,
>> > > >      THERMAL_TRIP_HOT,
>> > > >      THERMAL_TRIP_PASSIVE,
>> > > >      THERMAL_TRIP_ACTIVE,
>> > > >
>> > > > }
>> > >
>> > > this sounds reasonable to me.
>> > >
>> > > > > But more importantly, please use the generic trip-points for this. I
>> > > > > guess it shouldn't be a problem to introduce a "forced-shutdown"
>> > > > > trippoint [0] for the additional trip-point you have - thermal
>> > > > > maintainers, please shout if I'm wrong :-)
>> > >
>> > > what is the difference between a critical trip point and a
>> > > "forced-shutdown" trip point?
>> > > Thermal core will do a shutdown in case the critical trip point is
>> > > triggered.
>> >
>> > The forced-shutdown is where the thermal controller is supposed to also do a
>>
>> Currently, there is no discrimination between hardware configured /
>> triggered thermal shutdown and software detected / triggered thermal shutdown.
>> One way to implement it though is to reuse the critical trip type. Even
>> if you use more than one trip type it is doable, it will depend on the
>> priorities you give to software triggered and hardware triggered.
>>
>> > shutdown in hardware. As you said the thermal core will also shutdown at the
>> > critical trip point, I guess we could map Caesar's value like
>> >
>> > trip-point          tsadc
>> > critical            forced-shutdown (the 120 degrees in patch 4)
>>
>> > hot                 critical (the 100 degrees)
>> > ...
>> >
>> >
>>
>> In the case we are planing to expand the trip type range, adding one
>> specific to hardware configurable shutdown makes sense to me too.
> hmmm, why? you don't want an orderly shutdown? I still do not understand
> why we need a hardware shutdown trip point.
> Say, if we expect the system to be shutdown at 100C, I don't think we
> have a chance to trigger the hardware shutdown trip point.
> Further more, if my understanding is right, thermal core won't do
> anything for the hardware shutdown trip point because the system will be
> shutdown automatically, right? If this is true, why bother introducing
> this to thermal core?
>

Some ICs allow configuring the temperature when the shutdown will
happen. That is, you setup in registers the thermal shutdown
threshold, and one of the output pin of the IC is wired to, say, the
processor reset pin. Some other ICs have the threshold hardwired, and
cannot be configured.

Those options are a last chance to avoid processors to burn, in case
software really gets stuck at high temperatures.

The only thing that the thermal driver would need to worry is the
configuration step, that is, writing the value to the registers. In
the case the thermal core would have a specific trip type for such
case, the core itself would not do anything, except allowing designing
a thermal zone with hardware shutdown trips. And thus the thermal
driver would do the configuration.


Currently, the way I see to implement this is to interpret critical
trips as the threshold to be configured at the IC registers. That is,
reusing critical trips as orderly power down and as the hardware
shutdown threshold.

> thanks,
> rui
>> Alhtough, as I mention, I believe with the current generic trip types,
>> this situation can be covered already.
>>  Besides, I believe
>> 'forced-shutdown' does not sound a descriptive enough though. I would
>> suggest something more specific, say 'hardware-shutdown'.
>>
>> > >
>> > > thanks,
>> > > rui
>> > >
>> > > > It's a good option.
>> > > > I can send a patch,but I don't know whether the thermal maintainers will
>> > > > accept it.
>> > > >
>> > > > Maybe,they have a better way to suggest it.:-)
>> > > >
>> > > >
>> > > > PS:I will sent a new patch If I still have no received their suggestions
>> > > > in two days.
>> > > >
>> > > > > Heiko
>> > > > >
>> > > > >
>> > > > > [0] in a separate patch, changing
>> > > > > - thermal_trip_type enum in include/linux/thermal.h
>> > > > > - trip_types mapping in drivers/thermal/of-thermal.c
>> > > > > - Documentation/devicetree/bindings/thermal/thermal.txt
>> > > > >
>> > > > >> +
>> > > > >> +Example:
>> > > > >> +tsadc: tsadc at ff280000 {
>> > > > >> +    compatible = "rockchip,rk3288-tsadc";
>> > > > >> +    reg = <0xff280000 0x100>;
>> > > > >> +    interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
>> > > > >> +    clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
>> > > > >> +    clock-names = "tsadc", "apb_pclk";
>> > > > >> +};
>> >
>>
>
>



-- 
Eduardo Bezerra Valentin

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Heiko Stübner]

Am Dienstag, 9. September 2014, 21:14:18 schrieb edubezval at gmail.com:
> Hello,
> 
> On Tue, Sep 9, 2014 at 9:02 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> > On Tue, 2014-09-09 at 11:09 -0400, Eduardo Valentin wrote:
> >> Hello
> >> 
> >> On Tue, Sep 09, 2014 at 01:35:31PM +0200, Heiko St?bner wrote:
> >> > Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
> >> > > On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> >> > > > ? 2014?09?03? 16:07, Heiko St?bner ??:
> >> > > > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> >> > > > >> This add the necessary binding documentation for the thermal
> >> > > > >> found on Rockchip SoCs
> >> > > > >> 
> >> > > > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> >> > > > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> >> > > > >> ---
> >> > > > >> 
> >> > > > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> >> > > > >> 
> >> > > > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> >> > > > >> 
> >> > > > >>   create mode 100644
> >> > > > >> 
> >> > > > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >> > > > >> 
> >> > > > >> diff --git
> >> > > > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >> > > > >> new
> >> > > > >> file
> >> > > > >> mode 100644
> >> > > > >> index 0000000..1ed4d4c
> >> > > > >> --- /dev/null
> >> > > > >> +++
> >> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.tx
> >> > > > >> t
> >> > > > >> @@ -0,0 +1,20 @@
> >> > > > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> >> > > > >> +
> >> > > > >> +Required properties:
> >> > > > >> +- compatible: "rockchip,rk3288-tsadc"
> >> > > > >> +- reg: physical base address of the controller and length of
> >> > > > >> memory
> >> > > > >> mapped
> >> > > > >> +       region.
> >> > > > >> +- interrupts: The interrupt number to the cpu. The interrupt
> >> > > > >> specifier
> >> > > > >> format +           depends on the interrupt controller.
> >> > > > >> +- clocks: Must contain an entry for each entry in clock-names.
> >> > > > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
> >> > > > >> "apb_pclk" for +            the peripheral clock.
> >> > > > > 
> >> > > > > You're using the passive-temp, critical-temp and force-shut-temp
> >> > > > > properties in your driver without declaring them here.
> >> > > > 
> >> > > > frankly,the about are need be declared. but  there are 4 types[0]
> >> > > > for
> >> > > > trip in thermal framework,
> >> > > > there is no force-shut for me. So I want to change it three
> >> > > > additional
> >> > > > properties in [PATCH V4 4/4],
> >> > > > 
> >> > > > 
> >> > > > [0]
> >> > > > {
> >> > > > 
> >> > > >      THERMAL_TRIP_CRITICAL,
> >> > > >      THERMAL_TRIP_HOT,
> >> > > >      THERMAL_TRIP_PASSIVE,
> >> > > >      THERMAL_TRIP_ACTIVE,
> >> > > > 
> >> > > > }
> >> > > 
> >> > > this sounds reasonable to me.
> >> > > 
> >> > > > > But more importantly, please use the generic trip-points for
> >> > > > > this. I
> >> > > > > guess it shouldn't be a problem to introduce a "forced-shutdown"
> >> > > > > trippoint [0] for the additional trip-point you have - thermal
> >> > > > > maintainers, please shout if I'm wrong :-)
> >> > > 
> >> > > what is the difference between a critical trip point and a
> >> > > "forced-shutdown" trip point?
> >> > > Thermal core will do a shutdown in case the critical trip point is
> >> > > triggered.
> >> > 
> >> > The forced-shutdown is where the thermal controller is supposed to also
> >> > do a>> 
> >> Currently, there is no discrimination between hardware configured /
> >> triggered thermal shutdown and software detected / triggered thermal
> >> shutdown. One way to implement it though is to reuse the critical trip
> >> type. Even if you use more than one trip type it is doable, it will
> >> depend on the priorities you give to software triggered and hardware
> >> triggered.
> >> 
> >> > shutdown in hardware. As you said the thermal core will also shutdown
> >> > at the critical trip point, I guess we could map Caesar's value like
> >> > 
> >> > trip-point          tsadc
> >> > critical            forced-shutdown (the 120 degrees in patch 4)
> >> > 
> >> > hot                 critical (the 100 degrees)
> >> > ...
> >> 
> >> In the case we are planing to expand the trip type range, adding one
> >> specific to hardware configurable shutdown makes sense to me too.
> > 
> > hmmm, why? you don't want an orderly shutdown? I still do not understand
> > why we need a hardware shutdown trip point.
> > Say, if we expect the system to be shutdown at 100C, I don't think we
> > have a chance to trigger the hardware shutdown trip point.
> > Further more, if my understanding is right, thermal core won't do
> > anything for the hardware shutdown trip point because the system will be
> > shutdown automatically, right? If this is true, why bother introducing
> > this to thermal core?
> 
> Some ICs allow configuring the temperature when the shutdown will
> happen. That is, you setup in registers the thermal shutdown
> threshold, and one of the output pin of the IC is wired to, say, the
> processor reset pin. Some other ICs have the threshold hardwired, and
> cannot be configured.
> 
> Those options are a last chance to avoid processors to burn, in case
> software really gets stuck at high temperatures.
> 
> The only thing that the thermal driver would need to worry is the
> configuration step, that is, writing the value to the registers. In
> the case the thermal core would have a specific trip type for such
> case, the core itself would not do anything, except allowing designing
> a thermal zone with hardware shutdown trips. And thus the thermal
> driver would do the configuration.
> 
> 
> Currently, the way I see to implement this is to interpret critical
> trips as the threshold to be configured at the IC registers. That is,
> reusing critical trips as orderly power down and as the hardware
> shutdown threshold.

which was what I also meant to express above [but seemingly failed to do 
properly :-) ].

Critical is specified as "Hardware not reliable", so I'd think it wouldn't 
matter how the hw is shut down (orderly/unorderly) as long as its done.

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Zhang Rui]

On Wed, 2014-09-10 at 09:24 +0200, Heiko St?bner wrote:
> Am Dienstag, 9. September 2014, 21:14:18 schrieb edubezval at gmail.com:
> > Hello,
> > 
> > On Tue, Sep 9, 2014 at 9:02 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> > > On Tue, 2014-09-09 at 11:09 -0400, Eduardo Valentin wrote:
> > >> Hello
> > >> 
> > >> On Tue, Sep 09, 2014 at 01:35:31PM +0200, Heiko St?bner wrote:
> > >> > Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
> > >> > > On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> > >> > > > ? 2014?09?03? 16:07, Heiko St?bner ??:
> > >> > > > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> > >> > > > >> This add the necessary binding documentation for the thermal
> > >> > > > >> found on Rockchip SoCs
> > >> > > > >> 
> > >> > > > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> > >> > > > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> > >> > > > >> ---
> > >> > > > >> 
> > >> > > > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> > >> > > > >> 
> > >> > > > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> > >> > > > >> 
> > >> > > > >>   create mode 100644
> > >> > > > >> 
> > >> > > > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > >> > > > >> 
> > >> > > > >> diff --git
> > >> > > > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > >> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > >> > > > >> new
> > >> > > > >> file
> > >> > > > >> mode 100644
> > >> > > > >> index 0000000..1ed4d4c
> > >> > > > >> --- /dev/null
> > >> > > > >> +++
> > >> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.tx
> > >> > > > >> t
> > >> > > > >> @@ -0,0 +1,20 @@
> > >> > > > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> > >> > > > >> +
> > >> > > > >> +Required properties:
> > >> > > > >> +- compatible: "rockchip,rk3288-tsadc"
> > >> > > > >> +- reg: physical base address of the controller and length of
> > >> > > > >> memory
> > >> > > > >> mapped
> > >> > > > >> +       region.
> > >> > > > >> +- interrupts: The interrupt number to the cpu. The interrupt
> > >> > > > >> specifier
> > >> > > > >> format +           depends on the interrupt controller.
> > >> > > > >> +- clocks: Must contain an entry for each entry in clock-names.
> > >> > > > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
> > >> > > > >> "apb_pclk" for +            the peripheral clock.
> > >> > > > > 
> > >> > > > > You're using the passive-temp, critical-temp and force-shut-temp
> > >> > > > > properties in your driver without declaring them here.
> > >> > > > 
> > >> > > > frankly,the about are need be declared. but  there are 4 types[0]
> > >> > > > for
> > >> > > > trip in thermal framework,
> > >> > > > there is no force-shut for me. So I want to change it three
> > >> > > > additional
> > >> > > > properties in [PATCH V4 4/4],
> > >> > > > 
> > >> > > > 
> > >> > > > [0]
> > >> > > > {
> > >> > > > 
> > >> > > >      THERMAL_TRIP_CRITICAL,
> > >> > > >      THERMAL_TRIP_HOT,
> > >> > > >      THERMAL_TRIP_PASSIVE,
> > >> > > >      THERMAL_TRIP_ACTIVE,
> > >> > > > 
> > >> > > > }
> > >> > > 
> > >> > > this sounds reasonable to me.
> > >> > > 
> > >> > > > > But more importantly, please use the generic trip-points for
> > >> > > > > this. I
> > >> > > > > guess it shouldn't be a problem to introduce a "forced-shutdown"
> > >> > > > > trippoint [0] for the additional trip-point you have - thermal
> > >> > > > > maintainers, please shout if I'm wrong :-)
> > >> > > 
> > >> > > what is the difference between a critical trip point and a
> > >> > > "forced-shutdown" trip point?
> > >> > > Thermal core will do a shutdown in case the critical trip point is
> > >> > > triggered.
> > >> > 
> > >> > The forced-shutdown is where the thermal controller is supposed to also
> > >> > do a>> 
> > >> Currently, there is no discrimination between hardware configured /
> > >> triggered thermal shutdown and software detected / triggered thermal
> > >> shutdown. One way to implement it though is to reuse the critical trip
> > >> type. Even if you use more than one trip type it is doable, it will
> > >> depend on the priorities you give to software triggered and hardware
> > >> triggered.
> > >> 
> > >> > shutdown in hardware. As you said the thermal core will also shutdown
> > >> > at the critical trip point, I guess we could map Caesar's value like
> > >> > 
> > >> > trip-point          tsadc
> > >> > critical            forced-shutdown (the 120 degrees in patch 4)
> > >> > 
> > >> > hot                 critical (the 100 degrees)
> > >> > ...
> > >> 
> > >> In the case we are planing to expand the trip type range, adding one
> > >> specific to hardware configurable shutdown makes sense to me too.
> > > 
> > > hmmm, why? you don't want an orderly shutdown? I still do not understand
> > > why we need a hardware shutdown trip point.
> > > Say, if we expect the system to be shutdown at 100C, I don't think we
> > > have a chance to trigger the hardware shutdown trip point.
> > > Further more, if my understanding is right, thermal core won't do
> > > anything for the hardware shutdown trip point because the system will be
> > > shutdown automatically, right? If this is true, why bother introducing
> > > this to thermal core?
> > 
> > Some ICs allow configuring the temperature when the shutdown will
> > happen. That is, you setup in registers the thermal shutdown
> > threshold, and one of the output pin of the IC is wired to, say, the
> > processor reset pin. Some other ICs have the threshold hardwired, and
> > cannot be configured.
> > 
> > Those options are a last chance to avoid processors to burn, in case
> > software really gets stuck at high temperatures.
> > 
> > The only thing that the thermal driver would need to worry is the
> > configuration step, that is, writing the value to the registers. In
> > the case the thermal core would have a specific trip type for such
> > case, the core itself would not do anything, except allowing designing
> > a thermal zone with hardware shutdown trips. And thus the thermal
> > driver would do the configuration.
> > 
> > 
> > Currently, the way I see to implement this is to interpret critical
> > trips as the threshold to be configured at the IC registers. That is,
> > reusing critical trips as orderly power down and as the hardware
> > shutdown threshold.
> 
> which was what I also meant to express above [but seemingly failed to do 
> properly :-) ].
> 
> Critical is specified as "Hardware not reliable", so I'd think it wouldn't 
> matter how the hw is shut down (orderly/unorderly) as long as its done.

Hmmm,

As what we want is to make thermal driver have a chance to configure the
hardware shutdown registers, I'm thinking if we can do this without
representing the hardware shutdown value as a trip point.
Say,
1. parse DT, and get the hardware shutdown temperature value, and store
it somewhere, e.g. struct __thermal_zone.
2. introduce a new parameter, int (*set_hardware_trip)(void *, long *),
in thermal_zone_of_sensor_register().
3. invoke set_hard_trip(tz, hardware_shutdown_temperature_value) in
thermal_zone_of_sensor_register().

thanks,
rui

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Eduardo Valentin]

Hello Rui,

On Thu, Sep 11, 2014 at 10:36:52AM +0800, Zhang Rui wrote:
> On Wed, 2014-09-10 at 09:24 +0200, Heiko St?bner wrote:
> > Am Dienstag, 9. September 2014, 21:14:18 schrieb edubezval at gmail.com:
> > > Hello,
> > > 
> > > On Tue, Sep 9, 2014 at 9:02 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> > > > On Tue, 2014-09-09 at 11:09 -0400, Eduardo Valentin wrote:
> > > >> Hello
> > > >> 
> > > >> On Tue, Sep 09, 2014 at 01:35:31PM +0200, Heiko St?bner wrote:
> > > >> > Am Dienstag, 9. September 2014, 10:27:17 schrieb Zhang Rui:
> > > >> > > On Thu, 2014-09-04 at 09:02 +0800, Caesar Wang wrote:
> > > >> > > > ? 2014?09?03? 16:07, Heiko St?bner ??:
> > > >> > > > > Am Mittwoch, 3. September 2014, 10:10:37 schrieb Caesar Wang:
> > > >> > > > >> This add the necessary binding documentation for the thermal
> > > >> > > > >> found on Rockchip SoCs
> > > >> > > > >> 
> > > >> > > > >> Signed-off-by: zhaoyifeng <zyf@rock-chips.com>
> > > >> > > > >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> > > >> > > > >> ---
> > > >> > > > >> 
> > > >> > > > >>   .../devicetree/bindings/thermal/rockchip-thermal.txt | 20
> > > >> > > > >> 
> > > >> > > > >> ++++++++++++++++++++ 1 file changed, 20 insertions(+)
> > > >> > > > >> 
> > > >> > > > >>   create mode 100644
> > > >> > > > >> 
> > > >> > > > >> Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > >> > > > >> 
> > > >> > > > >> diff --git
> > > >> > > > >> a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > >> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> > > >> > > > >> new
> > > >> > > > >> file
> > > >> > > > >> mode 100644
> > > >> > > > >> index 0000000..1ed4d4c
> > > >> > > > >> --- /dev/null
> > > >> > > > >> +++
> > > >> > > > >> b/Documentation/devicetree/bindings/thermal/rockchip-thermal.tx
> > > >> > > > >> t
> > > >> > > > >> @@ -0,0 +1,20 @@
> > > >> > > > >> +* Temperature Sensor ADC (TSADC) on rockchip SoCs
> > > >> > > > >> +
> > > >> > > > >> +Required properties:
> > > >> > > > >> +- compatible: "rockchip,rk3288-tsadc"
> > > >> > > > >> +- reg: physical base address of the controller and length of
> > > >> > > > >> memory
> > > >> > > > >> mapped
> > > >> > > > >> +       region.
> > > >> > > > >> +- interrupts: The interrupt number to the cpu. The interrupt
> > > >> > > > >> specifier
> > > >> > > > >> format +           depends on the interrupt controller.
> > > >> > > > >> +- clocks: Must contain an entry for each entry in clock-names.
> > > >> > > > >> +- clock-names: Shall be "tsadc" for the converter-clock, and
> > > >> > > > >> "apb_pclk" for +            the peripheral clock.
> > > >> > > > > 
> > > >> > > > > You're using the passive-temp, critical-temp and force-shut-temp
> > > >> > > > > properties in your driver without declaring them here.
> > > >> > > > 
> > > >> > > > frankly,the about are need be declared. but  there are 4 types[0]
> > > >> > > > for
> > > >> > > > trip in thermal framework,
> > > >> > > > there is no force-shut for me. So I want to change it three
> > > >> > > > additional
> > > >> > > > properties in [PATCH V4 4/4],
> > > >> > > > 
> > > >> > > > 
> > > >> > > > [0]
> > > >> > > > {
> > > >> > > > 
> > > >> > > >      THERMAL_TRIP_CRITICAL,
> > > >> > > >      THERMAL_TRIP_HOT,
> > > >> > > >      THERMAL_TRIP_PASSIVE,
> > > >> > > >      THERMAL_TRIP_ACTIVE,
> > > >> > > > 
> > > >> > > > }
> > > >> > > 
> > > >> > > this sounds reasonable to me.
> > > >> > > 
> > > >> > > > > But more importantly, please use the generic trip-points for
> > > >> > > > > this. I
> > > >> > > > > guess it shouldn't be a problem to introduce a "forced-shutdown"
> > > >> > > > > trippoint [0] for the additional trip-point you have - thermal
> > > >> > > > > maintainers, please shout if I'm wrong :-)
> > > >> > > 
> > > >> > > what is the difference between a critical trip point and a
> > > >> > > "forced-shutdown" trip point?
> > > >> > > Thermal core will do a shutdown in case the critical trip point is
> > > >> > > triggered.
> > > >> > 
> > > >> > The forced-shutdown is where the thermal controller is supposed to also
> > > >> > do a>> 
> > > >> Currently, there is no discrimination between hardware configured /
> > > >> triggered thermal shutdown and software detected / triggered thermal
> > > >> shutdown. One way to implement it though is to reuse the critical trip
> > > >> type. Even if you use more than one trip type it is doable, it will
> > > >> depend on the priorities you give to software triggered and hardware
> > > >> triggered.
> > > >> 
> > > >> > shutdown in hardware. As you said the thermal core will also shutdown
> > > >> > at the critical trip point, I guess we could map Caesar's value like
> > > >> > 
> > > >> > trip-point          tsadc
> > > >> > critical            forced-shutdown (the 120 degrees in patch 4)
> > > >> > 
> > > >> > hot                 critical (the 100 degrees)
> > > >> > ...
> > > >> 
> > > >> In the case we are planing to expand the trip type range, adding one
> > > >> specific to hardware configurable shutdown makes sense to me too.
> > > > 
> > > > hmmm, why? you don't want an orderly shutdown? I still do not understand
> > > > why we need a hardware shutdown trip point.
> > > > Say, if we expect the system to be shutdown at 100C, I don't think we
> > > > have a chance to trigger the hardware shutdown trip point.
> > > > Further more, if my understanding is right, thermal core won't do
> > > > anything for the hardware shutdown trip point because the system will be
> > > > shutdown automatically, right? If this is true, why bother introducing
> > > > this to thermal core?
> > > 
> > > Some ICs allow configuring the temperature when the shutdown will
> > > happen. That is, you setup in registers the thermal shutdown
> > > threshold, and one of the output pin of the IC is wired to, say, the
> > > processor reset pin. Some other ICs have the threshold hardwired, and
> > > cannot be configured.
> > > 
> > > Those options are a last chance to avoid processors to burn, in case
> > > software really gets stuck at high temperatures.
> > > 
> > > The only thing that the thermal driver would need to worry is the
> > > configuration step, that is, writing the value to the registers. In
> > > the case the thermal core would have a specific trip type for such
> > > case, the core itself would not do anything, except allowing designing
> > > a thermal zone with hardware shutdown trips. And thus the thermal
> > > driver would do the configuration.
> > > 
> > > 
> > > Currently, the way I see to implement this is to interpret critical
> > > trips as the threshold to be configured at the IC registers. That is,
> > > reusing critical trips as orderly power down and as the hardware
> > > shutdown threshold.
> > 
> > which was what I also meant to express above [but seemingly failed to do 
> > properly :-) ].
> > 
> > Critical is specified as "Hardware not reliable", so I'd think it wouldn't 
> > matter how the hw is shut down (orderly/unorderly) as long as its done.
> 
> Hmmm,
> 
> As what we want is to make thermal driver have a chance to configure the
> hardware shutdown registers, I'm thinking if we can do this without
> representing the hardware shutdown value as a trip point.
> Say,
> 1. parse DT, and get the hardware shutdown temperature value, and store
> it somewhere, e.g. struct __thermal_zone.
> 2. introduce a new parameter, int (*set_hardware_trip)(void *, long *),
> in thermal_zone_of_sensor_register().
> 3. invoke set_hard_trip(tz, hardware_shutdown_temperature_value) in
> thermal_zone_of_sensor_register().


The only issue I have with the above proposal is that not all platforms
use DT. Some still boot with boardfiles, for instance. Thus, the
parameter to configure hardware thermal shutdown needs to be common on
thermal core, not specific to of-thermal. Do you agree?


> 
> thanks,
> rui
> 

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Arnd Bergmann]

On Thursday 11 September 2014 08:18:43 Eduardo Valentin wrote:
> > As what we want is to make thermal driver have a chance to configure the
> > hardware shutdown registers, I'm thinking if we can do this without
> > representing the hardware shutdown value as a trip point.
> > Say,
> > 1. parse DT, and get the hardware shutdown temperature value, and store
> > it somewhere, e.g. struct __thermal_zone.
> > 2. introduce a new parameter, int (*set_hardware_trip)(void *, long *),
> > in thermal_zone_of_sensor_register().
> > 3. invoke set_hard_trip(tz, hardware_shutdown_temperature_value) in
> > thermal_zone_of_sensor_register().
> 
> The only issue I have with the above proposal is that not all platforms
> use DT. Some still boot with boardfiles, for instance. Thus, the
> parameter to configure hardware thermal shutdown needs to be common on
> thermal core, not specific to of-thermal. Do you agree?

Do you know of a machine that can't yet be converted to DT and that
needs this driver? In case of rockchips that is certainly not the
case, and we don't care about anybody trying to use board files out
of tree, they can just hack the thermal support as well.

	Arnd

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Eduardo Valentin]

Arnd,

On Thu, Sep 11, 2014 at 02:32:08PM +0200, Arnd Bergmann wrote:
> On Thursday 11 September 2014 08:18:43 Eduardo Valentin wrote:
> > > As what we want is to make thermal driver have a chance to configure the
> > > hardware shutdown registers, I'm thinking if we can do this without
> > > representing the hardware shutdown value as a trip point.
> > > Say,
> > > 1. parse DT, and get the hardware shutdown temperature value, and store
> > > it somewhere, e.g. struct __thermal_zone.
> > > 2. introduce a new parameter, int (*set_hardware_trip)(void *, long *),
> > > in thermal_zone_of_sensor_register().
> > > 3. invoke set_hard_trip(tz, hardware_shutdown_temperature_value) in
> > > thermal_zone_of_sensor_register().
> > 
> > The only issue I have with the above proposal is that not all platforms
> > use DT. Some still boot with boardfiles, for instance. Thus, the
> > parameter to configure hardware thermal shutdown needs to be common on
> > thermal core, not specific to of-thermal. Do you agree?
> 
> Do you know of a machine that can't yet be converted to DT and that
> needs this driver? In case of rockchips that is certainly not the
> case, and we don't care about anybody trying to use board files out
> of tree, they can just hack the thermal support as well.

I see. Again, the only concern I have is to produce thermal framework APIs
that would be only in the of-thermal. My point is not specific to this
patch, or this platform, but with a detail in the above proposal. 

While I agree to have a trip specific to configurable hardware triggered
thermal shutdown, I just don't see why it needs to be a feature
implemented only via of-thermal. It has to be properly defined in
thermal core.

The proposal of of-thermal is not to become a separate/competing thermal
framework.

> 
> 	Arnd

Cheers,

Eduardo

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Zhang Rui]

On Thu, 2014-09-11 at 11:53 -0400, Eduardo Valentin wrote:
> Arnd,
> 
> On Thu, Sep 11, 2014 at 02:32:08PM +0200, Arnd Bergmann wrote:
> > On Thursday 11 September 2014 08:18:43 Eduardo Valentin wrote:
> > > > As what we want is to make thermal driver have a chance to configure the
> > > > hardware shutdown registers, I'm thinking if we can do this without
> > > > representing the hardware shutdown value as a trip point.
> > > > Say,
> > > > 1. parse DT, and get the hardware shutdown temperature value, and store
> > > > it somewhere, e.g. struct __thermal_zone.
> > > > 2. introduce a new parameter, int (*set_hardware_trip)(void *, long *),
> > > > in thermal_zone_of_sensor_register().
> > > > 3. invoke set_hard_trip(tz, hardware_shutdown_temperature_value) in
> > > > thermal_zone_of_sensor_register().
> > > 
> > > The only issue I have with the above proposal is that not all platforms
> > > use DT. Some still boot with boardfiles, for instance. Thus, the
> > > parameter to configure hardware thermal shutdown needs to be common on
> > > thermal core, not specific to of-thermal. Do you agree?
> > 
> > Do you know of a machine that can't yet be converted to DT and that
> > needs this driver? In case of rockchips that is certainly not the
> > case, and we don't care about anybody trying to use board files out
> > of tree, they can just hack the thermal support as well.
> 
> I see. Again, the only concern I have is to produce thermal framework APIs
> that would be only in the of-thermal. My point is not specific to this
> patch, or this platform, but with a detail in the above proposal. 
> 
> While I agree to have a trip specific to configurable hardware triggered
> thermal shutdown, I just don't see why it needs to be a feature
> implemented only via of-thermal. It has to be properly defined in
> thermal core.
> 
> The proposal of of-thermal is not to become a separate/competing thermal
> framework.
> 
Agreed.
And I think we can have such feature in thermal core.
But again I don't think we should represent it as an trip point. 

thanks,
rui
> > 
> > 	Arnd
> 
> Cheers,
> 
> Eduardo

[PATCH v4 2/4] dt-bindings: document Rockchip thermal

[Zhang Rui]

On Thu, 2014-09-11 at 11:53 -0400, Eduardo Valentin wrote:
> Arnd,
> 
> On Thu, Sep 11, 2014 at 02:32:08PM +0200, Arnd Bergmann wrote:
> > On Thursday 11 September 2014 08:18:43 Eduardo Valentin wrote:
> > > > As what we want is to make thermal driver have a chance to configure the
> > > > hardware shutdown registers, I'm thinking if we can do this without
> > > > representing the hardware shutdown value as a trip point.
> > > > Say,
> > > > 1. parse DT, and get the hardware shutdown temperature value, and store
> > > > it somewhere, e.g. struct __thermal_zone.
> > > > 2. introduce a new parameter, int (*set_hardware_trip)(void *, long *),
> > > > in thermal_zone_of_sensor_register().
> > > > 3. invoke set_hard_trip(tz, hardware_shutdown_temperature_value) in
> > > > thermal_zone_of_sensor_register().
> > > 
> > > The only issue I have with the above proposal is that not all platforms
> > > use DT. Some still boot with boardfiles, for instance. Thus, the
> > > parameter to configure hardware thermal shutdown needs to be common on
> > > thermal core, not specific to of-thermal. Do you agree?
> > 
> > Do you know of a machine that can't yet be converted to DT and that
> > needs this driver? In case of rockchips that is certainly not the
> > case, and we don't care about anybody trying to use board files out
> > of tree, they can just hack the thermal support as well.
> 
> I see. Again, the only concern I have is to produce thermal framework APIs
> that would be only in the of-thermal. My point is not specific to this
> patch, or this platform, but with a detail in the above proposal. 
> 
> While I agree to have a trip specific to configurable hardware triggered
> thermal shutdown, I just don't see why it needs to be a feature
> implemented only via of-thermal. It has to be properly defined in
> thermal core.
> 
> The proposal of of-thermal is not to become a separate/competing thermal
> framework.
> 
Agreed.
And I think we can have such feature in thermal core.
But again I don't think we should represent it as an trip point.
Instead, we can have a separate parameter for

thanks,
rui
> > 
> > 	Arnd
> 
> Cheers,
> 
> Eduardo

[PATCH v4 3/4] ARM: dts: add main Thermal info to rk3288

[Caesar Wang]

Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
---
 arch/arm/boot/dts/rk3288.dtsi | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 36be7bb..3d672e3 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -224,6 +224,17 @@
 		status = "disabled";
 	};
 
+	tsadc: tsadc at ff280000 {
+		compatible = "rockchip,rk3288-tsadc";
+		reg = <0xff280000 0x100>;
+		interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
+		clock-names = "tsadc", "apb_pclk";
+		pinctrl-names = "default";
+		pinctrl-1 = <&tsadc_int>;
+		status = "disabled";
+	};
+
 	/* NOTE: ohci at ff520000 doesn't actually work on hardware */
 
 	usb_hsic: usb at ff5c0000 {
@@ -611,5 +622,12 @@
 				rockchip,pins = <5 15 3 &pcfg_pull_none>;
 			};
 		};
+
+		tsadc {
+			tsadc_int: tsadc-int {
+				rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_up>;
+			};
+		};
+
 	};
 };
-- 
1.9.1

[PATCH v4 3/4] ARM: dts: add main Thermal info to rk3288

[Heiko Stübner]

Am Mittwoch, 3. September 2014, 10:10:38 schrieb Caesar Wang:
> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index 36be7bb..3d672e3 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -224,6 +224,17 @@
>  		status = "disabled";
>  	};
> 
> +	tsadc: tsadc at ff280000 {

please keep the list of nodes sorted by register address. So when looking at 
my v3.18-next/dts branch, it should be after uart4: serial at ff1c0000 and before 
usb_host0_ehci: usb at ff500000 .


> +		compatible = "rockchip,rk3288-tsadc";
> +		reg = <0xff280000 0x100>;
> +		interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
> +		clock-names = "tsadc", "apb_pclk";
> +		pinctrl-names = "default";
> +		pinctrl-1 = <&tsadc_int>;
> +		status = "disabled";
> +	};
> +
>  	/* NOTE: ohci at ff520000 doesn't actually work on hardware */
> 
>  	usb_hsic: usb at ff5c0000 {
> @@ -611,5 +622,12 @@
>  				rockchip,pins = <5 15 3 &pcfg_pull_none>;
>  			};
>  		};
> +
> +		tsadc {
> +			tsadc_int: tsadc-int {
> +				rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_up>;
> +			};
> +		};

After looking this up in the schematics I see that this is the 
overtemperature-protection output pin ... labeled OTP_OUT.
So I'd think the pinconfig should reflect this pin-name, especially as tsadc-int 
suggests, that this would be an interrupt leading into the tsadc, while in 
fact it is an output to a separate circuit.


Heiko

[PATCH v4 3/4] ARM: dts: add main Thermal info to rk3288

[Caesar Wang]

Hi Heiko,

? 2014?09?09? 19:37, Heiko St?bner ??:
> Am Mittwoch, 3. September 2014, 10:10:38 schrieb Caesar Wang:
>> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
>> ---
>>   arch/arm/boot/dts/rk3288.dtsi | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
>> index 36be7bb..3d672e3 100644
>> --- a/arch/arm/boot/dts/rk3288.dtsi
>> +++ b/arch/arm/boot/dts/rk3288.dtsi
>> @@ -224,6 +224,17 @@
>>   		status = "disabled";
>>   	};
>>
>> +	tsadc: tsadc at ff280000 {
> please keep the list of nodes sorted by register address. So when looking at
> my v3.18-next/dts branch, it should be after uart4: serial at ff1c0000 and before
> usb_host0_ehci: usb at ff500000 .
>

oh...,I will fix it.
>> +		compatible = "rockchip,rk3288-tsadc";
>> +		reg = <0xff280000 0x100>;
>> +		interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
>> +		clocks = <&cru SCLK_TSADC>, <&cru PCLK_TSADC>;
>> +		clock-names = "tsadc", "apb_pclk";
>> +		pinctrl-names = "default";
>> +		pinctrl-1 = <&tsadc_int>;
>> +		status = "disabled";
>> +	};
>> +
>>   	/* NOTE: ohci at ff520000 doesn't actually work on hardware */
>>
>>   	usb_hsic: usb at ff5c0000 {
>> @@ -611,5 +622,12 @@
>>   				rockchip,pins = <5 15 3 &pcfg_pull_none>;
>>   			};
>>   		};
>> +
>> +		tsadc {
>> +			tsadc_int: tsadc-int {
>> +				rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_up>;
>> +			};
>> +		};
> After looking this up in the schematics I see that this is the
> overtemperature-protection output pin ... labeled OTP_OUT.
> So I'd think the pinconfig should reflect this pin-name, especially as tsadc-int
> suggests, that this would be an interrupt leading into the tsadc, while in
> fact it is an output to a separate circuit.
>
Maybe,I guess  your mean as the follows .
Please correct it and  Laugh out if I got it wrong.

tsadc {
             otp_out: otp-out {
                 rockchip,pins = <0 10 RK_FUNC_GPIO &pcfg_pull_none>;
             };

             tsadc_int: tsadc-int {
                 rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_none>;
             };
         };
Then,I deal with the otp_out in thermal driver.
I believe The "otp_out" will restart board if it's a high active.

tsadc_int: tsadc-int {
+				rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_up>;
+			};

But,I think the about will be implemented in thermal driver.
> Heiko
>
>
>

-- 
Best regards,
Caesar

[PATCH v4 3/4] ARM: dts: add main Thermal info to rk3288

[Heiko Stübner]

Hi Caesar,

Am Mittwoch, 10. September 2014, 10:49:05 schrieb Caesar Wang:
> Hi Heiko,
> 
> ? 2014?09?09? 19:37, Heiko St?bner ??:
> > Am Mittwoch, 3. September 2014, 10:10:38 schrieb Caesar Wang:
> >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
> >> ---
> >> 
> >>   arch/arm/boot/dts/rk3288.dtsi | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >> 
> >> diff --git a/arch/arm/boot/dts/rk3288.dtsi
> >> b/arch/arm/boot/dts/rk3288.dtsi
> >> index 36be7bb..3d672e3 100644
> >> --- a/arch/arm/boot/dts/rk3288.dtsi
> >> +++ b/arch/arm/boot/dts/rk3288.dtsi

[...]

> >> @@ -611,5 +622,12 @@
> >> 
> >>   				rockchip,pins = <5 15 3 &pcfg_pull_none>;
> >>   			
> >>   			};
> >>   		
> >>   		};
> >> 
> >> +
> >> +		tsadc {
> >> +			tsadc_int: tsadc-int {
> >> +				rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_up>;
> >> +			};
> >> +		};
> > 
> > After looking this up in the schematics I see that this is the
> > overtemperature-protection output pin ... labeled OTP_OUT.
> > So I'd think the pinconfig should reflect this pin-name, especially as
> > tsadc-int suggests, that this would be an interrupt leading into the
> > tsadc, while in fact it is an output to a separate circuit.
> 
> Maybe,I guess  your mean as the follows .
> Please correct it and  Laugh out if I got it wrong.

I'll never laugh ... just try get the best solution / trying to understand 
backgrounds.

> 
> tsadc {
>              otp_out: otp-out {
>                  rockchip,pins = <0 10 RK_FUNC_GPIO &pcfg_pull_none>;
>              };
> 
>              tsadc_int: tsadc-int {
>                  rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_none>;
>              };
>          };
> Then,I deal with the otp_out in thermal driver.
> I believe The "otp_out" will restart board if it's a high active.
> 
> tsadc_int: tsadc-int {
> +				rockchip,pins = <0 10 RK_FUNC_1 &pcfg_pull_up>;
> +			};
> 
> But,I think the about will be implemented in thermal driver.

I only meant the pin name (tsadc-int -> otp-out). In the schmatics the pin 
(controlled by the tsadc) is labeled otp_out and leads to the overtemperature 
protection circuit as expected. And as it's controlled by the tsadc, it should 
of course stay as the desired func_1.


Heiko

[PATCH v4 4/4] ARM: dts: enable Thermal on rk3288-evb board

[Caesar Wang]

Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com>
---
 arch/arm/boot/dts/rk3288-evb.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 2964370..fe85dde 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -101,6 +101,13 @@
 	status = "okay";
 };
 
+&tsadc {
+	passive-temp = <80>;
+	critical-temp = <100>;
+	force-shut-temp = <120>;
+	status = "okay";
+};
+
 &pinctrl {
 	buttons {
 		pwrbtn: pwrbtn {
-- 
1.9.1