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

[PATCH v3 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.
--

History version:
V1:
https://lkml.org/lkml/2016/11/22/250
V2:
https://lkml.org/lkml/2016/11/23/348
---

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 issues in special cases.
--

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


Changes in v3:
- fix trivial thing for error message nd return value.
- change the commit.
- Fixes something as Brian comments on

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 | 154 ++++++++++++++++++++++++-------------
 1 file changed, 101 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH v3 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 v3: None
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 v3 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>
---

Changes in v3: None
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 v3 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 v3:
- fix trivial thing for error message nd return value.

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 | 48 ++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 766486f..ca1730e 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -120,10 +120,10 @@ 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 chn, void __iomem *reg, int temp);
-	void (*set_tshut_temp)(const struct chip_tsadc_table *table,
-			       int chn, void __iomem *reg, int temp);
+	int (*set_alarm_temp)(const struct chip_tsadc_table *table,
+			      int chn, void __iomem *reg, int temp);
+	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);
 
 	/* Per-table methods */
@@ -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,15 +649,15 @@ 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,
-				  int chn, void __iomem *regs, int temp)
+static int 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)
-		return;
+		return -ERANGE;
 
 	writel_relaxed(alarm_value & table->data_mask,
 		       regs + TSADCV2_COMP_INT(chn));
@@ -667,23 +665,27 @@ 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,
-				  int chn, void __iomem *regs, int temp)
+static int 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)
-		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,
@@ -886,10 +888,8 @@ 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,
-			      sensor->id, thermal->regs, high);
-
-	return 0;
+	return tsadc->set_alarm_temp(&tsadc->table,
+				     sensor->id, thermal->regs, high);
 }
 
 static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
@@ -985,8 +985,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 +1203,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

[PATCH v3 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 v3: None
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 ca1730e..660ed3b 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 v3 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 pass {-INT_MAX, INT_MAX} to trigger tsadc alarm in the end,
ignore this case and disable the high temperature interrupt.

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

Changes in v3:
- change the commit.
- Fixes something as Brian comments on

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 660ed3b..56c663c 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 v3 3/5] thermal: rockchip: fixes invalid temperature case

[Brian Norris]

On Mon, Nov 28, 2016 at 07:12:02PM +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 v3:
> - fix trivial thing for error message nd return value.
> 
> 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 | 48 ++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 20 deletions(-)

Looks better now.

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

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

[Eduardo Valentin]

Hey Caesar, Brian,

On Mon, Nov 28, 2016 at 07:12:02PM +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.

<cut>

>  
>  	/* Make sure the value is valid */
>  	alarm_value = rk_tsadcv2_temp_to_code(table, temp);

dummy question here, looking at your tables, if I did not miss
something, looks like we have an accuracy of 5C steps. Not only, that,
we also support only multiples of 5C temperatures. If that observation
is correct, would it make more sense to simply check for this property,
and min and max temperature check, instead of going through the binary
search to check for valid temperature? 

>  	if (alarm_value == table->data_mask)
> -		return;
> +		return -ERANGE;
>  
>  	writel_relaxed(alarm_value & table->data_mask,
>  		       regs + TSADCV2_COMP_INT(chn));
> @@ -667,23 +665,27 @@ 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,
> -				  int chn, void __iomem *regs, int temp)
> +static int 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)
> -		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,
> @@ -886,10 +888,8 @@ 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,
> -			      sensor->id, thermal->regs, high);
> -
> -	return 0;
> +	return tsadc->set_alarm_temp(&tsadc->table,
> +				     sensor->id, thermal->regs, high);
>  }
>  
>  static int rockchip_thermal_get_temp(void *_sensor, int *out_temp)
> @@ -985,8 +985,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 +1203,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 v3 4/5] thermal: rockchip: optimize the conversion table

[Eduardo Valentin]

On Mon, Nov 28, 2016 at 07:12:03PM +0800, Caesar Wang wrote:
> 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 v3: None
> 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 ca1730e..660ed3b 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",

Is this really an invalid conversion table, or an invalid conversion mode?

> +		       __func__, table->mode);
> +		return error;
> +	}
> +
>  exit:
>  	pr_err("%s: invalid temperature, temp=%d error=%d\n",
>  	       __func__, temp, error);
> -- 
> 2.7.4
> 

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

[Eduardo Valentin]

Hello,

On Mon, Nov 28, 2016 at 07:12:00PM +0800, Caesar Wang wrote:
> 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 v3: None
> 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);

Given that we are improving messages, would it be more informative to
say that you have an invalid table mode?

> +		return -EINVAL;
>  	}
>  
>  	/*
> -- 
> 2.7.4
> 

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

[Brian Norris]

Hi,

On Mon, Nov 28, 2016 at 05:51:55PM -0800, Eduardo Valentin wrote:
> On Mon, Nov 28, 2016 at 07:12:00PM +0800, Caesar Wang wrote:
> > 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 v3: None
> > 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);
> 
> Given that we are improving messages, would it be more informative to
> say that you have an invalid table mode?

I considered the mode and ID listing to go hand in hand, so it was the
whole table that is wrong. But it is just as well to say the "table
mode" is wrong.

Maybe even better: "%s: unknown table mode: %d\n".

And I guess same answer for patch 4, where you had the same question.

Brian

> > +		return -EINVAL;
> >  	}
> >  
> >  	/*
> > -- 
> > 2.7.4
> > 

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

[Brian Norris]

Hi Eduardo,

I'm not sure I completely understand what you're asking, but I'll see
what I can answer.

On Mon, Nov 28, 2016 at 05:45:54PM -0800, Eduardo Valentin wrote:
> On Mon, Nov 28, 2016 at 07:12:02PM +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.
> 
> <cut>
> 
> >  
> >  	/* Make sure the value is valid */
> >  	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
> 
> dummy question here, looking at your tables, if I did not miss
> something, looks like we have an accuracy of 5C steps. Not only, that,
> we also support only multiples of 5C temperatures. If that observation

Currently, that's true I think. But patch 4 actually supports doing the
linear interpolation that is claimed (but not fully implemented) in the
comments today. So with that patch, we roughly support temperatures in
between the 5C intervals.

> is correct, would it make more sense to simply check for this property,

I'm not quite sure what you mean by "this property." Do you mean to just
assume that there will be 5C intervals, and jump ahead in the table
accordingly? Seems a bit fragile; nothing really guarantees that a
future ADC supported by this driver won't have 1, 2, 6, or 7C accuracy
(and therefore a different set of steps).

> and min and max temperature check, instead of going through the binary
> search to check for valid temperature? 

I was thinking while reviewing that the binary search serves more to
complicate things than to help -- it's much harder to read (and validate
that the loop termination logic is correct). And searching through a few
dozen table entries doesn't really get much benefit from a O(n) ->
O(log(n)) speed improvement.

Anyway, I'm not sure if you were thinking along the same lines as me.

Brian

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

[Eduardo Valentin]

Hey Brian,

On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote:
> Hi Eduardo,
> 
> I'm not sure I completely understand what you're asking, but I'll see
> what I can answer.
> 
> On Mon, Nov 28, 2016 at 05:45:54PM -0800, Eduardo Valentin wrote:
> > On Mon, Nov 28, 2016 at 07:12:02PM +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.
> > 
> > <cut>
> > 
> > >  
> > >  	/* Make sure the value is valid */
> > >  	alarm_value = rk_tsadcv2_temp_to_code(table, temp);
> > 
> > dummy question here, looking at your tables, if I did not miss
> > something, looks like we have an accuracy of 5C steps. Not only, that,
> > we also support only multiples of 5C temperatures. If that observation
> 
> Currently, that's true I think. But patch 4 actually supports doing the
> linear interpolation that is claimed (but not fully implemented) in the
> comments today. So with that patch, we roughly support temperatures in
> between the 5C intervals.
> 
> > is correct, would it make more sense to simply check for this property,
> 
> I'm not quite sure what you mean by "this property." Do you mean to just
> assume that there will be 5C intervals, and jump ahead in the table
> accordingly? Seems a bit fragile; nothing really guarantees that a
> future ADC supported by this driver won't have 1, 2, 6, or 7C accuracy
> (and therefore a different set of steps).

I was thinking something even simpler. I just thought that you could
avoid going into the binary search on the temp to code function by simply
checking if the temperature in the temp parameter is a multiple of the
table step.

I agree that might be a bit of a strong assumption, but then again, one
should avoid over engineering for future hardware, unless you already
know that the coming ADC versions will have different steps, or even
worse, no step pattern at all.

> 
> > and min and max temperature check, instead of going through the binary
> > search to check for valid temperature? 
> 
> I was thinking while reviewing that the binary search serves more to
> complicate things than to help -- it's much harder to read (and validate
> that the loop termination logic is correct). And searching through a few
> dozen table entries doesn't really get much benefit from a O(n) ->
> O(log(n)) speed improvement.

true. but if in your code path you do several walks in the table just to
check if parameters are valid, given that you could simply decide if
they are valid or not with simpler if condition, then, still worth, no?
:-)

> 
> Anyway, I'm not sure if you were thinking along the same lines as me.
> 

Something like that, except I though of something even simpler:
+	if ((temp % table->step) != 0)
+		return -ERANGE;

If temp passes that check, then you go to the temp -> code conversion.

> Brian

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

[Eduardo Valentin]

Hey,

On Mon, Nov 28, 2016 at 09:47:29PM -0800, Brian Norris wrote:
> Hi,
> 

<cut>

> > > +		       __func__, table.mode);
> > 
> > Given that we are improving messages, would it be more informative to
> > say that you have an invalid table mode?
> 
> I considered the mode and ID listing to go hand in hand, so it was the
> whole table that is wrong. But it is just as well to say the "table
> mode" is wrong.
> 
> Maybe even better: "%s: unknown table mode: %d\n".
> 

Yup, that works for me, even better.

> And I guess same answer for patch 4, where you had the same question.

yes

> 
> Brian
> 
> > > +		return -EINVAL;

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

[Brian Norris]

On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote:
> On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote:
> > I was thinking while reviewing that the binary search serves more to
> > complicate things than to help -- it's much harder to read (and validate
> > that the loop termination logic is correct). And searching through a few
> > dozen table entries doesn't really get much benefit from a O(n) ->
> > O(log(n)) speed improvement.
> 
> true. but if in your code path you do several walks in the table just to
> check if parameters are valid, given that you could simply decide if
> they are valid or not with simpler if condition, then, still worth, no?
> :-)

Yes, your suggestions seems like they would have made the code both (a
little) more straightforward and efficient. But...

> > Anyway, I'm not sure if you were thinking along the same lines as me.
> > 
> 
> Something like that, except I though of something even simpler:
> +	if ((temp % table->step) != 0)
> +		return -ERANGE;
> 
> If temp passes that check, then you go to the temp -> code conversion.

...that check isn't valid as of patch 4, where Caesar adds handling for
intermediate steps. We really never should have been strictly snapping
to the 5C steps in the first place; intermediate values are OK.

So, we still need some kind of search to find the right step -- or
closest bracketing range, to compute the interpolated value. We should
only reject temperatures that are too high or too low for the ADC to
represent.


--- Side track ---

BTW, when we're considering rejecting temperatures here: shouldn't this
be fed back to the upper layers more nicely? We're improving the error
handling for this driver in this series, but it still leaves things
behaving a little odd. When I tested, I can do:

## set something obviously way too high
echo 700000 > trip_point_X_temp

and get a 0 (success) return code from the sysfs write() syscall, even
though the rockchip driver rejected it with -ERANGE. Is there really no
way to feed back thermal range limits of a sensor to the of-thermal
framework?

Brian

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

[Eduardo Valentin]

Hello,

On Tue, Nov 29, 2016 at 09:59:28PM -0800, Brian Norris wrote:
> On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote:
> > On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote:
> > > I was thinking while reviewing that the binary search serves more to
> > > complicate things than to help -- it's much harder to read (and validate
> > > that the loop termination logic is correct). And searching through a few
> > > dozen table entries doesn't really get much benefit from a O(n) ->
> > > O(log(n)) speed improvement.
> > 
> > true. but if in your code path you do several walks in the table just to
> > check if parameters are valid, given that you could simply decide if
> > they are valid or not with simpler if condition, then, still worth, no?
> > :-)
> 
> Yes, your suggestions seems like they would have made the code both (a
> little) more straightforward and efficient. But...
> 
> > > Anyway, I'm not sure if you were thinking along the same lines as me.
> > > 
> > 
> > Something like that, except I though of something even simpler:
> > +	if ((temp % table->step) != 0)
> > +		return -ERANGE;
> > 
> > If temp passes that check, then you go to the temp -> code conversion.
> 
> ...that check isn't valid as of patch 4, where Caesar adds handling for
> intermediate steps. We really never should have been strictly snapping
> to the 5C steps in the first place; intermediate values are OK.
> 
> So, we still need some kind of search to find the right step -- or
> closest bracketing range, to compute the interpolated value. We should
> only reject temperatures that are too high or too low for the ADC to
> represent.

Ok. got it. check small comment on patch 4 then.

> 
> 
> --- Side track ---
> 
> BTW, when we're considering rejecting temperatures here: shouldn't this
> be fed back to the upper layers more nicely? We're improving the error
> handling for this driver in this series, but it still leaves things
> behaving a little odd. When I tested, I can do:
> 
> ## set something obviously way too high
> echo 700000 > trip_point_X_temp
> 
> and get a 0 (success) return code from the sysfs write() syscall, even
> though the rockchip driver rejected it with -ERANGE. Is there really no
> way to feed back thermal range limits of a sensor to the of-thermal
> framework?
> 

well, that is a bit strange to me. Are you sure you are returning the
-ERANGE? Because, my assumption is that the following of-thermal code
path would return the error code back to core:
 328         if (data->ops->set_trip_temp) {
 329                 int ret;
 330 
 331                 ret = data->ops->set_trip_temp(data->sensor_data, trip, temp);
 332                 if (ret)
 333                         return ret;
 334         }

And this part of thermal core would return it back to sysfs layer:
 757         ret = tz->ops->set_trip_temp(tz, trip, temperature);
 758         if (ret)
 759                 return ret;

or am I missing something?

> Brian

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

[Eduardo Valentin]

Hey,

On Mon, Nov 28, 2016 at 07:12:03PM +0800, Caesar Wang wrote:

<cut>

> +	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;


isn't the above 'mid + 1' off-by-one when mid ends being == table.length - 1?

You would be accessing table->id[table.length], which is wrong memory
access, no?

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

[Caesar Wang]

Hi

在 2016年11月30日 14:26, Eduardo Valentin 写道:
> Hello,
>
> On Tue, Nov 29, 2016 at 09:59:28PM -0800, Brian Norris wrote:
>> On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote:
>>> On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote:
>>>> I was thinking while reviewing that the binary search serves more to
>>>> complicate things than to help -- it's much harder to read (and validate
>>>> that the loop termination logic is correct). And searching through a few
>>>> dozen table entries doesn't really get much benefit from a O(n) ->
>>>> O(log(n)) speed improvement.
>>> true. but if in your code path you do several walks in the table just to
>>> check if parameters are valid, given that you could simply decide if
>>> they are valid or not with simpler if condition, then, still worth, no?
>>> :-)
>> Yes, your suggestions seems like they would have made the code both (a
>> little) more straightforward and efficient. But...
>>
>>>> Anyway, I'm not sure if you were thinking along the same lines as me.
>>>>
>>> Something like that, except I though of something even simpler:
>>> +	if ((temp % table->step) != 0)
>>> +		return -ERANGE;
>>>
>>> If temp passes that check, then you go to the temp -> code conversion.
>> ...that check isn't valid as of patch 4, where Caesar adds handling for
>> intermediate steps. We really never should have been strictly snapping
>> to the 5C steps in the first place; intermediate values are OK.
>>
>> So, we still need some kind of search to find the right step -- or
>> closest bracketing range, to compute the interpolated value. We should
>> only reject temperatures that are too high or too low for the ADC to
>> represent.
> Ok. got it. check small comment on patch 4 then.
>
>>
>> --- Side track ---
>>
>> BTW, when we're considering rejecting temperatures here: shouldn't this
>> be fed back to the upper layers more nicely? We're improving the error
>> handling for this driver in this series, but it still leaves things
>> behaving a little odd. When I tested, I can do:
>>
>> ## set something obviously way too high
>> echo 700000 > trip_point_X_temp
>>
>> and get a 0 (success) return code from the sysfs write() syscall, even
>> though the rockchip driver rejected it with -ERANGE. Is there really no
>> way to feed back thermal range limits of a sensor to the of-thermal
>> framework?
>>
> well, that is a bit strange to me. Are you sure you are returning the
> -ERANGE? Because, my assumption is that the following of-thermal code
> path would return the error code back to core:
>   328         if (data->ops->set_trip_temp) {
>   329                 int ret;
>   330
>   331                 ret = data->ops->set_trip_temp(data->sensor_data, trip, temp);
>   332                 if (ret)
>   333                         return ret;
>   334         }
>
> And this part of thermal core would return it back to sysfs layer:
>   757         ret = tz->ops->set_trip_temp(tz, trip, temperature);
>   758         if (ret)
>   759                 return ret;
>
> or am I missing something?

that should be related to the many trips. The trips will search the next 
trips.
So in general, trip_ponit_0_temp <= trip_ponit_1_temp <=trip_ponit_1_temp.

I'm assume you set"echo 700000 > trip_point_0_temp", and you keep trip1 
and trip2....

  [   34.449718] trip_point_temp_store, temp=700000
[   34.454568] of_thermal_set_trip_temp:336,temp=700000
[   34.459612] thermal_sys: thermal_zone_set_trips:583, temp=45000, 
trip_temp=700000, hys=2000
[   34.468026] thermal_sys: thermal_zone_set_trips:583, temp=45000, 
trip_temp=85000, hys=2000
[   34.476336] thermal_sys: thermal_zone_set_trips:583, temp=45000, 
trip_temp=95000, hys=2000
[   34.484634] thermal thermal_zone0: new temperature boundaries: 
-2147483647 < x < 85000
[   34.492619] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 0: low: -2147483647, high 85000
===>　So rockchip thermal will return 0.


That should report error when you meet the needs of order.
order--> trip_ponit_0_temp <= trip_ponit_1_temp <=trip_ponit_1_temp.

Fox example:
[  100.898552] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=700000, hys=2000
[  100.906964] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=710000, hys=2000
[  100.916329] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=720000, hys=2000
[  100.924685] thermal thermal_zone0: new temperature boundaries: 
-2147483647 < x < 700000
[  100.932965] rockchip-thermal ff260000.tsadc: 
rockchip_thermal_set_trips: sensor 0: low: -2147483647, high 700000
[  100.943138] rk_tsadcv2_alarm_temp:682, temp=700000
[  100.948201] rk_tsadcv2_temp_to_code: invalid temperature, temp=700000 
error=1023
[  100.955598] thermal thermal_zone0: Failed to set trips: -34
===>　So rockchip thermal will return error.

-
Caesar
>
>> Brian
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Caesar Wang]

在 2016年11月30日 13:04, Eduardo Valentin 写道:
> Hey,
>
> On Mon, Nov 28, 2016 at 09:47:29PM -0800, Brian Norris wrote:
>> Hi,
>>
> <cut>
>
>>>> +		       __func__, table.mode);
>>> Given that we are improving messages, would it be more informative to
>>> say that you have an invalid table mode?
>> I considered the mode and ID listing to go hand in hand, so it was the
>> whole table that is wrong. But it is just as well to say the "table
>> mode" is wrong.
>>
>> Maybe even better: "%s: unknown table mode: %d\n".
>>
> Yup, that works for me, even better.

Done.  Sorry for delay.

>> And I guess same answer for patch 4, where you had the same question.
> yes
>
>> Brian
>>
>>>> +		return -EINVAL;
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Caesar Wang]

在 2016年11月30日 14:29, Eduardo Valentin 写道:
> Hey,
>
> On Mon, Nov 28, 2016 at 07:12:03PM +0800, Caesar Wang wrote:
>
> <cut>
>
>> +	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;
>
> isn't the above 'mid + 1' off-by-one when mid ends being == table.length - 1?
>
> You would be accessing table->id[table.length], which is wrong memory
> access, no?

Yup, that's indeed a real issue for me.
FIxes on next version.

Thanks.

-Caesar

>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip