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

From: Caesar Wang
Date: Wed Sep 17 2014 - 03:29:02 EST

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@xxxxxxxxxxxxxx>
I forgot to ask, is zhaoyifeng the correct name or is it a nickname?

Signed-off-by: Caesar Wang <caesar.wang@xxxxxxxxxxxxxx>
---
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/