[PATCH v2 0/5] thermal: fixes the rockchip thermal

[PATCH v2 0/5] thermal: fixes the rockchip thermal

[Caesar Wang]

There are five patches posted for upstream.
89267b5 thermal: rockchip: improve conversion error messages
a0b5649 thermal: rockchip: don't pass table structs by value
bceed92 thermal: rockchip: fixes invalid temperature case
30be6d0 thermal: rockchip: optimize the conversion table
35636e9 thermal: rockchip: handle the set_trips without the trip points.
--

Brain posted the below patches for upstream.
89267b5 thermal: rockchip: improve conversion error messages
a0b5649 thermal: rockchip: don't pass table structs by value
That make sense to improve efficiency

Caesar post the below patches for upstream.
bceed92 thermal: rockchip: fixes invalid temperature case
30be6d0 thermal: rockchip: optimize the conversion table
35636e9 thermal: rockchip: handle the set_trips without the trip points.
That will fixes some issue in special cases.
--

Anyway, this series patches should can improve the rockchip thermal driver.


Changes in v2:
- As Brian commnets that restructure this to pass error codes back to the
  upper layers.
- Improve the commit message.
- improve the commit as Brian commnets on https://patchwork.kernel.org/patch/9440985
- Fixes something as Brian comments on
  https://patchwork.kernel.org/patch/9440989.

Changes in v1:
- The original Brian posted on https://patchwork.kernel.org/patch/9437686
  Note: it'd probably be even nicer to know which sensor this was, but we've
  kinda abstracted that one away by this point...
- The original Brian posted on https://patchwork.kernel.org/patch/9437687

Brian Norris (2):
  thermal: rockchip: improve conversion error messages
  thermal: rockchip: don't pass table structs by value

Caesar Wang (3):
  thermal: rockchip: fixes invalid temperature case
  thermal: rockchip: optimize the conversion table
  thermal: rockchip: handle set_trips without the trip points

 drivers/thermal/rockchip_thermal.c | 147 +++++++++++++++++++++++++------------
 1 file changed, 101 insertions(+), 46 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] thermal: rockchip: improve conversion error messages

[Caesar Wang]

From: Brian Norris <briannorris@chromium.org>

These error messages don't give much information about what went wrong.
It would be nice, for one, to see what invalid temperature was being
requested when conversion fails. It's also good to return an error when
we can't handle a conversion properly.

While we're at it, fix the grammar too.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>

---

Changes in v2: None
Changes in v1:
- The original Brian posted on https://patchwork.kernel.org/patch/9437686
  Note: it'd probably be even nicer to know which sensor this was, but we've
  kinda abstracted that one away by this point...

 drivers/thermal/rockchip_thermal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index b811b0f..26c247c 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -424,7 +424,8 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
 	}
 
 exit:
-	pr_err("Invalid the conversion, error=%d\n", error);
+	pr_err("%s: invalid temperature, temp=%d error=%d\n",
+	       __func__, temp, error);
 	return error;
 }
 
@@ -475,7 +476,9 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 		}
 		break;
 	default:
-		pr_err("Invalid the conversion table\n");
+		pr_err("%s: invalid conversion table, mode=%d\n",
+		       __func__, table.mode);
+		return -EINVAL;
 	}
 
 	/*
-- 
2.7.4

[PATCH v2 2/5] thermal: rockchip: don't pass table structs by value

[Caesar Wang]

From: Brian Norris <briannorris@chromium.org>

This driver passes struct chip_tsadc_table by value throughout; this is
inefficient, and AFAICT, there is no reason for it. Let's pass pointers
instead.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Caesar Wang <wxt@rock-chips.com>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

Changes in v2: None
Changes in v1:
- The original Brian posted on https://patchwork.kernel.org/patch/9437687

 drivers/thermal/rockchip_thermal.c | 80 +++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 26c247c..766486f 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -118,11 +118,11 @@ struct rockchip_tsadc_chip {
 	void (*control)(void __iomem *reg, bool on);
 
 	/* Per-sensor methods */
-	int (*get_temp)(struct chip_tsadc_table table,
+	int (*get_temp)(const struct chip_tsadc_table *table,
 			int chn, void __iomem *reg, int *temp);
-	void (*set_alarm_temp)(struct chip_tsadc_table table,
+	void (*set_alarm_temp)(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *reg, int temp);
-	void (*set_tshut_temp)(struct chip_tsadc_table table,
+	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *reg, int temp);
 	void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m);
 
@@ -397,26 +397,26 @@ static const struct tsadc_table rk3399_code_table[] = {
 	{TSADCV3_DATA_MASK, 125000},
 };
 
-static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
+static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
 				   int temp)
 {
 	int high, low, mid;
 	u32 error = 0;
 
 	low = 0;
-	high = table.length - 1;
+	high = table->length - 1;
 	mid = (high + low) / 2;
 
 	/* Return mask code data when the temp is over table range */
-	if (temp < table.id[low].temp || temp > table.id[high].temp) {
-		error = table.data_mask;
+	if (temp < table->id[low].temp || temp > table->id[high].temp) {
+		error = table->data_mask;
 		goto exit;
 	}
 
 	while (low <= high) {
-		if (temp == table.id[mid].temp)
-			return table.id[mid].code;
-		else if (temp < table.id[mid].temp)
+		if (temp == table->id[mid].temp)
+			return table->id[mid].code;
+		else if (temp < table->id[mid].temp)
 			high = mid - 1;
 		else
 			low = mid + 1;
@@ -429,28 +429,28 @@ static u32 rk_tsadcv2_temp_to_code(struct chip_tsadc_table table,
 	return error;
 }
 
-static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
-				   int *temp)
+static int rk_tsadcv2_code_to_temp(const struct chip_tsadc_table *table,
+				   u32 code, int *temp)
 {
 	unsigned int low = 1;
-	unsigned int high = table.length - 1;
+	unsigned int high = table->length - 1;
 	unsigned int mid = (low + high) / 2;
 	unsigned int num;
 	unsigned long denom;
 
-	WARN_ON(table.length < 2);
+	WARN_ON(table->length < 2);
 
-	switch (table.mode) {
+	switch (table->mode) {
 	case ADC_DECREMENT:
-		code &= table.data_mask;
-		if (code < table.id[high].code)
+		code &= table->data_mask;
+		if (code < table->id[high].code)
 			return -EAGAIN;		/* Incorrect reading */
 
 		while (low <= high) {
-			if (code >= table.id[mid].code &&
-			    code < table.id[mid - 1].code)
+			if (code >= table->id[mid].code &&
+			    code < table->id[mid - 1].code)
 				break;
-			else if (code < table.id[mid].code)
+			else if (code < table->id[mid].code)
 				low = mid + 1;
 			else
 				high = mid - 1;
@@ -459,15 +459,15 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 		}
 		break;
 	case ADC_INCREMENT:
-		code &= table.data_mask;
-		if (code < table.id[low].code)
+		code &= table->data_mask;
+		if (code < table->id[low].code)
 			return -EAGAIN;		/* Incorrect reading */
 
 		while (low <= high) {
-			if (code <= table.id[mid].code &&
-			    code > table.id[mid - 1].code)
+			if (code <= table->id[mid].code &&
+			    code > table->id[mid - 1].code)
 				break;
-			else if (code > table.id[mid].code)
+			else if (code > table->id[mid].code)
 				low = mid + 1;
 			else
 				high = mid - 1;
@@ -477,7 +477,7 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 		break;
 	default:
 		pr_err("%s: invalid conversion table, mode=%d\n",
-		       __func__, table.mode);
+		       __func__, table->mode);
 		return -EINVAL;
 	}
 
@@ -487,10 +487,10 @@ static int rk_tsadcv2_code_to_temp(struct chip_tsadc_table table, u32 code,
 	 * temperature between 2 table entries is linear and interpolate
 	 * to produce less granular result.
 	 */
-	num = table.id[mid].temp - table.id[mid - 1].temp;
-	num *= abs(table.id[mid - 1].code - code);
-	denom = abs(table.id[mid - 1].code - table.id[mid].code);
-	*temp = table.id[mid - 1].temp + (num / denom);
+	num = table->id[mid].temp - table->id[mid - 1].temp;
+	num *= abs(table->id[mid - 1].code - code);
+	denom = abs(table->id[mid - 1].code - table->id[mid].code);
+	*temp = table->id[mid - 1].temp + (num / denom);
 
 	return 0;
 }
@@ -641,7 +641,7 @@ static void rk_tsadcv3_control(void __iomem *regs, bool enable)
 	writel_relaxed(val, regs + TSADCV2_AUTO_CON);
 }
 
-static int rk_tsadcv2_get_temp(struct chip_tsadc_table table,
+static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *regs, int *temp)
 {
 	u32 val;
@@ -651,17 +651,17 @@ static int rk_tsadcv2_get_temp(struct chip_tsadc_table table,
 	return rk_tsadcv2_code_to_temp(table, val, temp);
 }
 
-static void rk_tsadcv2_alarm_temp(struct chip_tsadc_table table,
+static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
 	u32 alarm_value, int_en;
 
 	/* Make sure the value is valid */
 	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
-	if (alarm_value == table.data_mask)
+	if (alarm_value == table->data_mask)
 		return;
 
-	writel_relaxed(alarm_value & table.data_mask,
+	writel_relaxed(alarm_value & table->data_mask,
 		       regs + TSADCV2_COMP_INT(chn));
 
 	int_en = readl_relaxed(regs + TSADCV2_INT_EN);
@@ -669,14 +669,14 @@ static void rk_tsadcv2_alarm_temp(struct chip_tsadc_table table,
 	writel_relaxed(int_en, regs + TSADCV2_INT_EN);
 }
 
-static void rk_tsadcv2_tshut_temp(struct chip_tsadc_table table,
+static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
 	u32 tshut_value, val;
 
 	/* Make sure the value is valid */
 	tshut_value = rk_tsadcv2_temp_to_code(table, temp);
-	if (tshut_value == table.data_mask)
+	if (tshut_value == table->data_mask)
 		return;
 
 	writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn));
@@ -886,7 +886,7 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
 	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
 		__func__, sensor->id, low, high);
 
-	tsadc->set_alarm_temp(tsadc->table,
+	tsadc->set_alarm_temp(&tsadc->table,
 			      sensor->id, thermal->regs, high);
 
 	return 0;
@@ -899,7 +899,7 @@ static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
 	const struct rockchip_tsadc_chip *tsadc = sensor->thermal->chip;
 	int retval;
 
-	retval = tsadc->get_temp(tsadc->table,
+	retval = tsadc->get_temp(&tsadc->table,
 				 sensor->id, thermal->regs, out_temp);
 	dev_dbg(&thermal->pdev->dev, "sensor %d - temp: %d, retval: %d\n",
 		sensor->id, *out_temp, retval);
@@ -985,7 +985,7 @@ rockchip_thermal_register_sensor(struct platform_device *pdev,
 	int error;
 
 	tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode);
-	tsadc->set_tshut_temp(tsadc->table, id, thermal->regs,
+	tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
 			      thermal->tshut_temp);
 
 	sensor->thermal = thermal;
@@ -1199,7 +1199,7 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
 
 		thermal->chip->set_tshut_mode(id, thermal->regs,
 					      thermal->tshut_mode);
-		thermal->chip->set_tshut_temp(thermal->chip->table,
+		thermal->chip->set_tshut_temp(&thermal->chip->table,
 					      id, thermal->regs,
 					      thermal->tshut_temp);
 	}
-- 
2.7.4

[PATCH v2 3/5] thermal: rockchip: fixes invalid temperature case

[Caesar Wang]

The temp_to_code function will return 0 when we set the temperature to a
invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch
will prevent this case happening. That will return the max analog value to
indicate the temperature is invalid or over table temperature range.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

Changes in v2:
- As Brian commnets that restructure this to pass error codes back to the
  upper layers.
- Improve the commit message.

Changes in v1: None

 drivers/thermal/rockchip_thermal.c | 41 ++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 766486f..e243e40 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -120,9 +120,9 @@ struct rockchip_tsadc_chip {
 	/* Per-sensor methods */
 	int (*get_temp)(const struct chip_tsadc_table *table,
 			int chn, void __iomem *reg, int *temp);
-	void (*set_alarm_temp)(const struct chip_tsadc_table *table,
+	int (*set_alarm_temp)(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *reg, int temp);
-	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
+	int (*set_tshut_temp)(const struct chip_tsadc_table *table,
 			       int chn, void __iomem *reg, int temp);
 	void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m);
 
@@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
 				   int temp)
 {
 	int high, low, mid;
-	u32 error = 0;
+	u32 error = table->data_mask;
 
 	low = 0;
 	high = table->length - 1;
 	mid = (high + low) / 2;
 
 	/* Return mask code data when the temp is over table range */
-	if (temp < table->id[low].temp || temp > table->id[high].temp) {
-		error = table->data_mask;
+	if (temp < table->id[low].temp || temp > table->id[high].temp)
 		goto exit;
-	}
 
 	while (low <= high) {
 		if (temp == table->id[mid].temp)
@@ -651,7 +649,7 @@ static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
 	return rk_tsadcv2_code_to_temp(table, val, temp);
 }
 
-static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
+static int rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
 	u32 alarm_value, int_en;
@@ -659,7 +657,7 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
 	/* Make sure the value is valid */
 	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
 	if (alarm_value == table->data_mask)
-		return;
+		return -ERANGE;
 
 	writel_relaxed(alarm_value & table->data_mask,
 		       regs + TSADCV2_COMP_INT(chn));
@@ -667,9 +665,11 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
 	int_en = readl_relaxed(regs + TSADCV2_INT_EN);
 	int_en |= TSADCV2_INT_SRC_EN(chn);
 	writel_relaxed(int_en, regs + TSADCV2_INT_EN);
+
+	return 0;
 }
 
-static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
+static int rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
 	u32 tshut_value, val;
@@ -677,13 +677,15 @@ static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
 	/* Make sure the value is valid */
 	tshut_value = rk_tsadcv2_temp_to_code(table, temp);
 	if (tshut_value == table->data_mask)
-		return;
+		return -ERANGE;
 
 	writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn));
 
 	/* TSHUT will be valid */
 	val = readl_relaxed(regs + TSADCV2_AUTO_CON);
 	writel_relaxed(val | TSADCV2_AUTO_SRC_EN(chn), regs + TSADCV2_AUTO_CON);
+
+	return 0;
 }
 
 static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
@@ -882,12 +884,17 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
 	struct rockchip_thermal_sensor *sensor = _sensor;
 	struct rockchip_thermal_data *thermal = sensor->thermal;
 	const struct rockchip_tsadc_chip *tsadc = thermal->chip;
+	int ret;
 
 	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
 		__func__, sensor->id, low, high);
 
-	tsadc->set_alarm_temp(&tsadc->table,
+	ret = tsadc->set_alarm_temp(&tsadc->table,
 			      sensor->id, thermal->regs, high);
+	if (ret)
+		dev_err(&thermal->pdev->dev,
+			"%s: disable sensor[%d] alarm temp: %d, ret: %d\n",
+			__func__, sensor->id, high, ret);
 
 	return 0;
 }
@@ -985,8 +992,12 @@ rockchip_thermal_register_sensor(struct platform_device *pdev,
 	int error;
 
 	tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode);
-	tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
+
+	error = tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
 			      thermal->tshut_temp);
+	if (error)
+		dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n",
+			__func__, thermal->tshut_temp, error);
 
 	sensor->thermal = thermal;
 	sensor->id = id;
@@ -1199,9 +1210,13 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
 
 		thermal->chip->set_tshut_mode(id, thermal->regs,
 					      thermal->tshut_mode);
-		thermal->chip->set_tshut_temp(&thermal->chip->table,
+
+		error = thermal->chip->set_tshut_temp(&thermal->chip->table,
 					      id, thermal->regs,
 					      thermal->tshut_temp);
+		if (error)
+			dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n",
+				__func__, thermal->tshut_temp, error);
 	}
 
 	thermal->chip->control(thermal->regs, true);
-- 
2.7.4

Re: [PATCH v2 3/5] thermal: rockchip: fixes invalid temperature case

[Brian Norris]

Hi Caesar,

On Wed, Nov 23, 2016 at 10:29:32PM +0800, Caesar Wang wrote:
> The temp_to_code function will return 0 when we set the temperature to a
> invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch
> will prevent this case happening. That will return the max analog value to
> indicate the temperature is invalid or over table temperature range.
> 
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
> 
> Changes in v2:
> - As Brian commnets that restructure this to pass error codes back to the
>   upper layers.
> - Improve the commit message.
> 
> Changes in v1: None
> 
>  drivers/thermal/rockchip_thermal.c | 41 ++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 766486f..e243e40 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -120,9 +120,9 @@ struct rockchip_tsadc_chip {
>  	/* Per-sensor methods */
>  	int (*get_temp)(const struct chip_tsadc_table *table,
>  			int chn, void __iomem *reg, int *temp);
> -	void (*set_alarm_temp)(const struct chip_tsadc_table *table,
> +	int (*set_alarm_temp)(const struct chip_tsadc_table *table,
>  			       int chn, void __iomem *reg, int temp);
> -	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
> +	int (*set_tshut_temp)(const struct chip_tsadc_table *table,
>  			       int chn, void __iomem *reg, int temp);
>  	void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m);
>  
> @@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
>  				   int temp)
>  {
>  	int high, low, mid;
> -	u32 error = 0;
> +	u32 error = table->data_mask;
>  
>  	low = 0;
>  	high = table->length - 1;
>  	mid = (high + low) / 2;
>  
>  	/* Return mask code data when the temp is over table range */
> -	if (temp < table->id[low].temp || temp > table->id[high].temp) {
> -		error = table->data_mask;
> +	if (temp < table->id[low].temp || temp > table->id[high].temp)
>  		goto exit;
> -	}
>  
>  	while (low <= high) {
>  		if (temp == table->id[mid].temp)
> @@ -651,7 +649,7 @@ static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
>  	return rk_tsadcv2_code_to_temp(table, val, temp);
>  }
>  
> -static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
> +static int rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>  				  int chn, void __iomem *regs, int temp)
>  {
>  	u32 alarm_value, int_en;
> @@ -659,7 +657,7 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>  	/* Make sure the value is valid */
>  	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
>  	if (alarm_value == table->data_mask)
> -		return;
> +		return -ERANGE;
>  
>  	writel_relaxed(alarm_value & table->data_mask,
>  		       regs + TSADCV2_COMP_INT(chn));
> @@ -667,9 +665,11 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>  	int_en = readl_relaxed(regs + TSADCV2_INT_EN);
>  	int_en |= TSADCV2_INT_SRC_EN(chn);
>  	writel_relaxed(int_en, regs + TSADCV2_INT_EN);
> +
> +	return 0;
>  }
>  
> -static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
> +static int rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>  				  int chn, void __iomem *regs, int temp)
>  {
>  	u32 tshut_value, val;
> @@ -677,13 +677,15 @@ static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>  	/* Make sure the value is valid */
>  	tshut_value = rk_tsadcv2_temp_to_code(table, temp);
>  	if (tshut_value == table->data_mask)
> -		return;
> +		return -ERANGE;
>  
>  	writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn));
>  
>  	/* TSHUT will be valid */
>  	val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>  	writel_relaxed(val | TSADCV2_AUTO_SRC_EN(chn), regs + TSADCV2_AUTO_CON);
> +
> +	return 0;
>  }
>  
>  static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
> @@ -882,12 +884,17 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
>  	struct rockchip_thermal_sensor *sensor = _sensor;
>  	struct rockchip_thermal_data *thermal = sensor->thermal;
>  	const struct rockchip_tsadc_chip *tsadc = thermal->chip;
> +	int ret;
>  
>  	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>  		__func__, sensor->id, low, high);
>  
> -	tsadc->set_alarm_temp(&tsadc->table,
> +	ret = tsadc->set_alarm_temp(&tsadc->table,
>  			      sensor->id, thermal->regs, high);
> +	if (ret)
> +		dev_err(&thermal->pdev->dev,
> +			"%s: disable sensor[%d] alarm temp: %d, ret: %d\n",

You're not necessarily disabling the sensor; you're just not programming
it. If this is the second time setting the trip points (e.g., using
'echo > trip_X_temp') then this just means you've retained the old
value.

Also, I'm not sure you need to print anything here, as the thermal core
prints errors for you. Maybe make this dev_dbg() so it doesn't print by
default?

> +			__func__, sensor->id, high, ret);
>  
>  	return 0;

Oh, but you missed the whole point of returning errors; you need to
propagate 'ret' here! i.e.:

	return ret;

Brian

>  }
> @@ -985,8 +992,12 @@ rockchip_thermal_register_sensor(struct platform_device *pdev,
>  	int error;
>  
>  	tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode);
> -	tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
> +
> +	error = tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
>  			      thermal->tshut_temp);
> +	if (error)
> +		dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n",
> +			__func__, thermal->tshut_temp, error);
>  
>  	sensor->thermal = thermal;
>  	sensor->id = id;
> @@ -1199,9 +1210,13 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>  
>  		thermal->chip->set_tshut_mode(id, thermal->regs,
>  					      thermal->tshut_mode);
> -		thermal->chip->set_tshut_temp(&thermal->chip->table,
> +
> +		error = thermal->chip->set_tshut_temp(&thermal->chip->table,
>  					      id, thermal->regs,
>  					      thermal->tshut_temp);
> +		if (error)
> +			dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n",
> +				__func__, thermal->tshut_temp, error);
>  	}
>  
>  	thermal->chip->control(thermal->regs, true);
> -- 
> 2.7.4
> 

Re: [PATCH v2 3/5] thermal: rockchip: fixes invalid temperature case

[Caesar Wang]

在 2016年11月24日 02:35, Brian Norris 写道:
> Hi Caesar,
>
> On Wed, Nov 23, 2016 at 10:29:32PM +0800, Caesar Wang wrote:
>> The temp_to_code function will return 0 when we set the temperature to a
>> invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch
>> will prevent this case happening. That will return the max analog value to
>> indicate the temperature is invalid or over table temperature range.
>>
>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>> ---
>>
>> Changes in v2:
>> - As Brian commnets that restructure this to pass error codes back to the
>>    upper layers.
>> - Improve the commit message.
>>
>> Changes in v1: None
>>
>>   drivers/thermal/rockchip_thermal.c | 41 ++++++++++++++++++++++++++------------
>>   1 file changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>> index 766486f..e243e40 100644
>> --- a/drivers/thermal/rockchip_thermal.c
>> +++ b/drivers/thermal/rockchip_thermal.c
>> @@ -120,9 +120,9 @@ struct rockchip_tsadc_chip {
>>   	/* Per-sensor methods */
>>   	int (*get_temp)(const struct chip_tsadc_table *table,
>>   			int chn, void __iomem *reg, int *temp);
>> -	void (*set_alarm_temp)(const struct chip_tsadc_table *table,
>> +	int (*set_alarm_temp)(const struct chip_tsadc_table *table,
>>   			       int chn, void __iomem *reg, int temp);
>> -	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
>> +	int (*set_tshut_temp)(const struct chip_tsadc_table *table,
>>   			       int chn, void __iomem *reg, int temp);
>>   	void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m);
>>   
>> @@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
>>   				   int temp)
>>   {
>>   	int high, low, mid;
>> -	u32 error = 0;
>> +	u32 error = table->data_mask;
>>   
>>   	low = 0;
>>   	high = table->length - 1;
>>   	mid = (high + low) / 2;
>>   
>>   	/* Return mask code data when the temp is over table range */
>> -	if (temp < table->id[low].temp || temp > table->id[high].temp) {
>> -		error = table->data_mask;
>> +	if (temp < table->id[low].temp || temp > table->id[high].temp)
>>   		goto exit;
>> -	}
>>   
>>   	while (low <= high) {
>>   		if (temp == table->id[mid].temp)
>> @@ -651,7 +649,7 @@ static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
>>   	return rk_tsadcv2_code_to_temp(table, val, temp);
>>   }
>>   
>> -static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>> +static int rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>>   				  int chn, void __iomem *regs, int temp)
>>   {
>>   	u32 alarm_value, int_en;
>> @@ -659,7 +657,7 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>>   	/* Make sure the value is valid */
>>   	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
>>   	if (alarm_value == table->data_mask)
>> -		return;
>> +		return -ERANGE;
>>   
>>   	writel_relaxed(alarm_value & table->data_mask,
>>   		       regs + TSADCV2_COMP_INT(chn));
>> @@ -667,9 +665,11 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>>   	int_en = readl_relaxed(regs + TSADCV2_INT_EN);
>>   	int_en |= TSADCV2_INT_SRC_EN(chn);
>>   	writel_relaxed(int_en, regs + TSADCV2_INT_EN);
>> +
>> +	return 0;
>>   }
>>   
>> -static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>> +static int rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>>   				  int chn, void __iomem *regs, int temp)
>>   {
>>   	u32 tshut_value, val;
>> @@ -677,13 +677,15 @@ static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>>   	/* Make sure the value is valid */
>>   	tshut_value = rk_tsadcv2_temp_to_code(table, temp);
>>   	if (tshut_value == table->data_mask)
>> -		return;
>> +		return -ERANGE;
>>   
>>   	writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn));
>>   
>>   	/* TSHUT will be valid */
>>   	val = readl_relaxed(regs + TSADCV2_AUTO_CON);
>>   	writel_relaxed(val | TSADCV2_AUTO_SRC_EN(chn), regs + TSADCV2_AUTO_CON);
>> +
>> +	return 0;
>>   }
>>   
>>   static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
>> @@ -882,12 +884,17 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
>>   	struct rockchip_thermal_sensor *sensor = _sensor;
>>   	struct rockchip_thermal_data *thermal = sensor->thermal;
>>   	const struct rockchip_tsadc_chip *tsadc = thermal->chip;
>> +	int ret;
>>   
>>   	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>>   		__func__, sensor->id, low, high);
>>   
>> -	tsadc->set_alarm_temp(&tsadc->table,
>> +	ret = tsadc->set_alarm_temp(&tsadc->table,
>>   			      sensor->id, thermal->regs, high);
>> +	if (ret)
>> +		dev_err(&thermal->pdev->dev,
>> +			"%s: disable sensor[%d] alarm temp: %d, ret: %d\n",
> You're not necessarily disabling the sensor; you're just not programming
> it. If this is the second time setting the trip points (e.g., using
> 'echo > trip_X_temp') then this just means you've retained the old
> value.
>
> Also, I'm not sure you need to print anything here, as the thermal core
> prints errors for you. Maybe make this dev_dbg() so it doesn't print by
> default?

Yeah, the thermal core will print something, look like I just print 
"failed set the sensor*....
or return ret".
Anyway,  that's not ugly thing.

>> +			__func__, sensor->id, high, ret);
>>   
>>   	return 0;
> Oh, but you missed the whole point of returning errors; you need to
> propagate 'ret' here! i.e.:
>
> 	return ret;

Right, thanks for tracking into it.
I will wait someone feedback for this series patches before posting new 
patchset.

-Caesar
>
> Brian
>
>>   }
>> @@ -985,8 +992,12 @@ rockchip_thermal_register_sensor(struct platform_device *pdev,
>>   	int error;
>>   
>>   	tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode);
>> -	tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
>> +
>> +	error = tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs,
>>   			      thermal->tshut_temp);
>> +	if (error)
>> +		dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n",
>> +			__func__, thermal->tshut_temp, error);
>>   
>>   	sensor->thermal = thermal;
>>   	sensor->id = id;
>> @@ -1199,9 +1210,13 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>>   
>>   		thermal->chip->set_tshut_mode(id, thermal->regs,
>>   					      thermal->tshut_mode);
>> -		thermal->chip->set_tshut_temp(&thermal->chip->table,
>> +
>> +		error = thermal->chip->set_tshut_temp(&thermal->chip->table,
>>   					      id, thermal->regs,
>>   					      thermal->tshut_temp);
>> +		if (error)
>> +			dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n",
>> +				__func__, thermal->tshut_temp, error);
>>   	}
>>   
>>   	thermal->chip->control(thermal->regs, true);
>> -- 
>> 2.7.4
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v2 4/5] thermal: rockchip: optimize the conversion table

[Caesar Wang]

In order to support the valid temperature can conver to analog value.
The rockchip thermal driver has not supported the all valid temperature
to convert the analog value. (e.g.: 61C, 62C, 63C....)

For example:
In some cases, we need adjust the trip point.
$cd /sys/class/thermal/thermal_zone*
$echo 68000 > trip_point_0_temp
That will return the max analogic value indicates the invalid before
posting this patch.

So, this patch will optimize the conversion table to support the other
cases.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Reviewed-by: Brian Norris <briannorris@chromium.org>
---

Changes in v2:
- improve the commit as Brian commnets on https://patchwork.kernel.org/patch/9440985

Changes in v1: None

 drivers/thermal/rockchip_thermal.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index e243e40..0d50df7 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -401,6 +401,8 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
 				   int temp)
 {
 	int high, low, mid;
+	unsigned long num;
+	unsigned int denom;
 	u32 error = table->data_mask;
 
 	low = 0;
@@ -421,6 +423,27 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table,
 		mid = (low + high) / 2;
 	}
 
+	/*
+	 * The conversion code granularity provided by the table. Let's
+	 * assume that the relationship between temperature and
+	 * analog value between 2 table entries is linear and interpolate
+	 * to produce less granular result.
+	 */
+	num = abs(table->id[mid].code - table->id[mid + 1].code);
+	num *= temp - table->id[mid].temp;
+	denom = table->id[mid + 1].temp - table->id[mid].temp;
+
+	switch (table->mode) {
+	case ADC_DECREMENT:
+		return table->id[mid].code - (num / denom);
+	case ADC_INCREMENT:
+		return table->id[mid].code + (num / denom);
+	default:
+		pr_err("%s: invalid conversion table, mode=%d\n",
+		       __func__, table->mode);
+		return error;
+	}
+
 exit:
 	pr_err("%s: invalid temperature, temp=%d error=%d\n",
 	       __func__, temp, error);
-- 
2.7.4

[PATCH v2 5/5] thermal: rockchip: handle set_trips without the trip points

[Caesar Wang]

In some cases, some sensors didn't need the trip points, the
set_trips will return {-INT_MAX, INT_MAX} to trigger thermal alarm.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
---

Changes in v2:
- Fixes something as Brian comments on
  https://patchwork.kernel.org/patch/9440989.

Changes in v1: None

 drivers/thermal/rockchip_thermal.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 0d50df7..f873053 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -675,7 +675,21 @@ static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table,
 static int rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
 				  int chn, void __iomem *regs, int temp)
 {
-	u32 alarm_value, int_en;
+	u32 alarm_value;
+	u32 int_en, int_clr;
+
+	/*
+	 * In some cases, some sensors didn't need the trip points, the
+	 * set_trips will pass {-INT_MAX, INT_MAX} to trigger tsadc alarm
+	 * in the end, ignore this case and disable the high temperature
+	 * interrupt.
+	 */
+	if (temp == INT_MAX) {
+		int_clr = readl_relaxed(regs + TSADCV2_INT_EN);
+		int_clr &= ~TSADCV2_INT_SRC_EN(chn);
+		writel_relaxed(int_clr, regs + TSADCV2_INT_EN);
+		return 0;
+	}
 
 	/* Make sure the value is valid */
 	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
-- 
2.7.4

Re: [PATCH v2 5/5] thermal: rockchip: handle set_trips without the trip points

[Brian Norris]

On Wed, Nov 23, 2016 at 10:29:34PM +0800, Caesar Wang wrote:
> In some cases, some sensors didn't need the trip points, the
> set_trips will return {-INT_MAX, INT_MAX} to trigger thermal alarm.
> 
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
> 
> Changes in v2:
> - Fixes something as Brian comments on
>   https://patchwork.kernel.org/patch/9440989.
> 
> Changes in v1: None

I think this looks better.

Reviewed-by: Brian Norris <briannorris@chromium.org>