Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531218 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] thermal: fixes the rockchip thermal Caesar Wang <wxt@rock-chips.com> - 2016-11-28 12:20 +0100
    [PATCH v3 4/5] thermal: rockchip: optimize the conversion table Caesar Wang <wxt@rock-chips.com> - 2016-11-28 12:20 +0100
      Re: [PATCH v3 4/5] thermal: rockchip: optimize the conversion table Eduardo Valentin <edubezval@gmail.com> - 2016-11-29 02:50 +0100
      Re: [PATCH v3 4/5] thermal: rockchip: optimize the conversion table Eduardo Valentin <edubezval@gmail.com> - 2016-11-30 07:30 +0100
    [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Caesar Wang <wxt@rock-chips.com> - 2016-11-28 12:20 +0100
      Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Brian Norris <briannorris@chromium.org> - 2016-11-28 19:50 +0100
      Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Eduardo Valentin <edubezval@gmail.com> - 2016-11-29 02:50 +0100
        Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Brian Norris <briannorris@chromium.org> - 2016-11-29 23:00 +0100
          Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Eduardo Valentin <edubezval@gmail.com> - 2016-11-30 06:10 +0100
            Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Brian Norris <briannorris@chromium.org> - 2016-11-30 07:00 +0100
              Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Eduardo Valentin <edubezval@gmail.com> - 2016-11-30 07:30 +0100

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

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

[toc] | [next] | [standalone]

#1531222 — [PATCH v3 4/5] thermal: rockchip: optimize the conversion table

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

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

#1531224 — [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case

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

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

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

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
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web