[PATCH 0/5] thermal: rockchip: optimization to improve the driver

[PATCH 0/5] thermal: rockchip: optimization to improve the driver

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

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 the set_trips without the trip points.

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

-- 
2.7.4

[PATCH 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:
- The original Brian posted on https://patchwork.kernel.org/patch/9437685
  Brain said: "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 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>
Tested-by: Caesar Wang <wxt@rock-chips.com>
---

Changes:
- 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 3/5] thermal: rockchip: fixes invalid temperature case

[Caesar Wang]

The temp_to_code function will return 0 when we set the trip points value
or valid temperature.
This patch will prevent this case happening.

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

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

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 766486f..535f1fa 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -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)
-- 
2.7.4

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

[Brian Norris]

On Tue, Nov 22, 2016 at 08:34:46PM +0800, Caesar Wang wrote:
> The temp_to_code function will return 0 when we set the trip points value
> or valid temperature.

I'm not quite sure what you mean by "when we set the trip points value
or valid temperature." Do you mean "when we set the trip point's value
to an invalid temperature"?

Assuming that's what you meant...

> This patch will prevent this case happening.

This is good to change, but IMO, it's better to actually pick a close
value, instead of the max. e.g., if you support temperatures at degree
intervals of 80, 85, 90, ..., 125, but someone lists 82 in the device
tree, we should pick either 80 or 85, not 125.

Brian

> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
> 
>  drivers/thermal/rockchip_thermal.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 766486f..535f1fa 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -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)
> -- 
> 2.7.4
> 

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

[Brian Norris]

On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote:
> On Tue, Nov 22, 2016 at 08:34:46PM +0800, Caesar Wang wrote:
> > The temp_to_code function will return 0 when we set the trip points value
> > or valid temperature.
> 
> I'm not quite sure what you mean by "when we set the trip points value
> or valid temperature." Do you mean "when we set the trip point's value
> to an invalid temperature"?
> 
> Assuming that's what you meant...
> 
> > This patch will prevent this case happening.
> 
> This is good to change, but IMO, it's better to actually pick a close
> value, instead of the max. e.g., if you support temperatures at degree
> intervals of 80, 85, 90, ..., 125, but someone lists 82 in the device
> tree, we should pick either 80 or 85, not 125.

I see that's what you're doing in a patch later in this series. Good.

> > Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> > ---
> > 
> >  drivers/thermal/rockchip_thermal.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> > index 766486f..535f1fa 100644
> > --- a/drivers/thermal/rockchip_thermal.c
> > +++ b/drivers/thermal/rockchip_thermal.c
> > @@ -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;

I was revisiting the logic here though, and I don't understand your
error case. You're treating "too low" and "too high" the same, and in
either case, you're choosing a value of ->data_mask. That doesn't make
sense to me, especially for ADC_DECREMENT cases like rk3288. In that
case, you're programming the trip to the lowest possible temperature.

It seems like either you should make this conditional, so that "too low"
and "too high" make sane alternative choices (like MAX or MIN temp), or
else restructure this to pass error codes back to the upper layers.

Brian

> > -	}
> >  
> >  	while (low <= high) {
> >  		if (temp == table->id[mid].temp)
> > -- 
> > 2.7.4
> > 

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

[Caesar Wang]

在 2016年11月23日 05:52, Brian Norris 写道:
> On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote:
>> On Tue, Nov 22, 2016 at 08:34:46PM +0800, Caesar Wang wrote:
>>> The temp_to_code function will return 0 when we set the trip points value
>>> or valid temperature.
>> I'm not quite sure what you mean by "when we set the trip points value
>> or valid temperature." Do you mean "when we set the trip point's value
>> to an invalid temperature"?
>>
>> Assuming that's what you meant...

Almost, I will improve the commit.

>>
>>> This patch will prevent this case happening.
>> This is good to change, but IMO, it's better to actually pick a close
>> value, instead of the max. e.g., if you support temperatures at degree
>> intervals of 80, 85, 90, ..., 125, but someone lists 82 in the device
>> tree, we should pick either 80 or 85, not 125.
> I see that's what you're doing in a patch later in this series. Good.

Yeah, find another issue during writing this patch, as the patch[4/5].

>
>>> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
>>> ---
>>>
>>>   drivers/thermal/rockchip_thermal.c | 6 ++----
>>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>>> index 766486f..535f1fa 100644
>>> --- a/drivers/thermal/rockchip_thermal.c
>>> +++ b/drivers/thermal/rockchip_thermal.c
>>> @@ -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;
> I was revisiting the logic here though, and I don't understand your
> error case. You're treating "too low" and "too high" the same, and in
> either case, you're choosing a value of ->data_mask. That doesn't make
> sense to me, especially for ADC_DECREMENT cases like rk3288. In that
> case, you're programming the trip to the lowest possible temperature.

I admit that's not perfect, but that should conform to reality.

Whichever is the adc value, 12it or 10bit.
#define TSADCV2_DATA_MASK            0xfff
#define TSADCV3_DATA_MASK            0x3ff

The "too low" and "too high" are same, that should indicate that temperature is
invalid or over table range.

The currect code will return the max analog value to warn it.
---

The temperature {-40C, 125C} is for rockchip SoCs, that should be 
similar with real world's temperature {-INT_MAX, INT_MAX}.

-Caesar
>
> It seems like either you should make this conditional, so that "too low"
> and "too high" make sane alternative choices (like MAX or MIN temp), or
> else restructure this to pass error codes back to the upper layers.
>
> Brian
>
>>> -	}
>>>   
>>>   	while (low <= high) {
>>>   		if (temp == table->id[mid].temp)
>>> -- 
>>> 2.7.4
>>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip



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

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

[Brian Norris]

On Wed, Nov 23, 2016 at 10:06:15AM +0800, Caesar Wang wrote:
> 在 2016年11月23日 05:52, Brian Norris 写道:
> >On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote:

> >>>+	if (temp < table->id[low].temp || temp > table->id[high].temp)
> >>>  		goto exit;
> >I was revisiting the logic here though, and I don't understand your
> >error case. You're treating "too low" and "too high" the same, and in
> >either case, you're choosing a value of ->data_mask. That doesn't make
> >sense to me, especially for ADC_DECREMENT cases like rk3288. In that
> >case, you're programming the trip to the lowest possible temperature.
> 
> I admit that's not perfect, but that should conform to reality.
> 
> Whichever is the adc value, 12it or 10bit.
> #define TSADCV2_DATA_MASK            0xfff
> #define TSADCV3_DATA_MASK            0x3ff
> 
> The "too low" and "too high" are same, that should indicate that temperature is
> invalid or over table range.
> 
> The currect code will return the max analog value to warn it.
> ---
> 
> The temperature {-40C, 125C} is for rockchip SoCs, that should be
> similar with real world's temperature {-INT_MAX, INT_MAX}.

IIUC, "too high" should not be interpreted as TSADCV2_DATA_MASK on
rk3288, should it? That corresponds to -40C, which means you'll be
triggering the alarm temperature at a very *low* temperature, not a very
high one, no?

Brian

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

[Caesar Wang]

在 2016年11月23日 10:33, Brian Norris 写道:
> On Wed, Nov 23, 2016 at 10:06:15AM +0800, Caesar Wang wrote:
>> 在 2016年11月23日 05:52, Brian Norris 写道:
>>> On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote:
>>>>> +	if (temp < table->id[low].temp || temp > table->id[high].temp)
>>>>>   		goto exit;
>>> I was revisiting the logic here though, and I don't understand your
>>> error case. You're treating "too low" and "too high" the same, and in
>>> either case, you're choosing a value of ->data_mask. That doesn't make
>>> sense to me, especially for ADC_DECREMENT cases like rk3288. In that
>>> case, you're programming the trip to the lowest possible temperature.
>> I admit that's not perfect, but that should conform to reality.
>>
>> Whichever is the adc value, 12it or 10bit.
>> #define TSADCV2_DATA_MASK            0xfff
>> #define TSADCV3_DATA_MASK            0x3ff
>>
>> The "too low" and "too high" are same, that should indicate that temperature is
>> invalid or over table range.
>>
>> The currect code will return the max analog value to warn it.
>> ---
>>
>> The temperature {-40C, 125C} is for rockchip SoCs, that should be
>> similar with real world's temperature {-INT_MAX, INT_MAX}.
> IIUC, "too high" should not be interpreted as TSADCV2_DATA_MASK on
> rk3288, should it? That corresponds to -40C, which means you'll be
> triggering the alarm temperature at a very *low* temperature, not a very
> high one, no?

The "too high" will correspond to -40C on rk3288, but shouldn't trigger 
the alarm temperature.

Due to the alarm or tshut function will handle it.

e.g.:
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)
         return;
....
}
or
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)
         return;
...
}

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

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

[Brian Norris]

On Wed, Nov 23, 2016 at 11:03:33AM +0800, Caesar Wang wrote:
> 在 2016年11月23日 10:33, Brian Norris 写道:
> >IIUC, "too high" should not be interpreted as TSADCV2_DATA_MASK on
> >rk3288, should it? That corresponds to -40C, which means you'll be
> >triggering the alarm temperature at a very *low* temperature, not a very
> >high one, no?
> 
> The "too high" will correspond to -40C on rk3288, but shouldn't
> trigger the alarm temperature.
> 
> Due to the alarm or tshut function will handle it.
> 
> e.g.:
> 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)
>         return;

Ah, right. I keep forgetting about this odd error handling.

That's still the wrong error handling though; the right response is
never to avoid doing anything (and therefore returning "success" to the
thermal core). You need to either program a high (or low) trip value, or
else report an error (i.e., allow rk_tsadcv2_alarm_temp() to return an
error code back to the calling function). Otherwise, this:

echo -45000 > trip_0_temp

will succeed without error, and:

cat trip_0_temp
-45000

will return the cached temperature from of-thermal, even though the trip
point is programmed to something else entirely.

Brian

[PATCH 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 has not supported the all valid temperature.

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 report the error message before posting this patch.

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

 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 535f1fa..f4d4be9 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

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

[Brian Norris]

On Tue, Nov 22, 2016 at 08:34:47PM +0800, Caesar Wang wrote:
> In order to support the valid temperature can conver to analog value.
> The rockchip thermal has not supported the all valid temperature.
> 
> 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 report the error message before posting this patch.

Notably, this only printed an error in the kernel log. It didn't return
an error to the actual API, and instead it just programmed a 0 trip
point (or MASK, as of your previous patch). So you were lying to the
thermal core, which would still tell you that you programmed 68000 to
the trip point when you check:

  cat trip_point_0_temp

It's important to describe what you're fixing, so please reword the
commit log a bit.

> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> ---
> 
>  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 535f1fa..f4d4be9 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;
>  	}

If you manage to exit the above loop, you're assuming that:

	table->id[mid].temp < temp

I believe that's the case due to the round-down nature of the

  mid = (low + high) / 2
  
computation, but it's still a bit hard to reason about your binary
search loop.

Anyway, I think this is correct, so if you improve the commit message:

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

>  
> +	/*
> +	 * 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 5/5] thermal: rockchip: handle the 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>
---

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

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index f4d4be9..5b9c346 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -200,6 +200,7 @@ struct rockchip_thermal_data {
 #define TSADCV3_AUTO_Q_SEL_EN			BIT(1)
 
 #define TSADCV2_INT_SRC_EN(chn)			BIT(chn)
+#define TSADCV2_INT_SRC_SHIFT(chn)		chn
 #define TSADCV2_SHUT_2GPIO_SRC_EN(chn)		BIT(4 + (chn))
 #define TSADCV2_SHUT_2CRU_SRC_EN(chn)		BIT(8 + (chn))
 
@@ -903,10 +904,22 @@ 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;
+	u32 int_clr;
 
 	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
 		__func__, sensor->id, low, high);
 
+	/*
+	 * In some cases, some sensors didn't need the trip points, the
+	 * set_trips will return {-INT_MAX, INT_MAX} to trigger thermal alarm.
+	 */
+	if (high == INT_MAX) {
+		int_clr = readl_relaxed(thermal->regs + TSADCV2_INT_EN);
+		int_clr |= 0 << TSADCV2_INT_SRC_SHIFT(sensor->id);
+		writel_relaxed(int_clr, thermal->regs + TSADCV2_INT_EN);
+		return 0;
+	}
+
 	tsadc->set_alarm_temp(&tsadc->table,
 			      sensor->id, thermal->regs, high);
 
-- 
2.7.4

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

[Brian Norris]

On Tue, Nov 22, 2016 at 08:34:48PM +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>
> ---
> 
>  drivers/thermal/rockchip_thermal.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index f4d4be9..5b9c346 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -200,6 +200,7 @@ struct rockchip_thermal_data {
>  #define TSADCV3_AUTO_Q_SEL_EN			BIT(1)
>  
>  #define TSADCV2_INT_SRC_EN(chn)			BIT(chn)
> +#define TSADCV2_INT_SRC_SHIFT(chn)		chn
>  #define TSADCV2_SHUT_2GPIO_SRC_EN(chn)		BIT(4 + (chn))
>  #define TSADCV2_SHUT_2CRU_SRC_EN(chn)		BIT(8 + (chn))
>  
> @@ -903,10 +904,22 @@ 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;
> +	u32 int_clr;
>  
>  	dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>  		__func__, sensor->id, low, high);
>  
> +	/*
> +	 * In some cases, some sensors didn't need the trip points, the
> +	 * set_trips will return {-INT_MAX, INT_MAX} to trigger thermal alarm.

This language is a little unclear. AFAICT, set_trips is not "returning"
those values, it's "passing" them as arguments.

> +	 */
> +	if (high == INT_MAX) {
> +		int_clr = readl_relaxed(thermal->regs + TSADCV2_INT_EN);
> +		int_clr |= 0 << TSADCV2_INT_SRC_SHIFT(sensor->id);

Uhh, really? This line is a no-op; you're just OR'ing a value of zero...

It also seems like this should use the existing shift macro,
TSADCV2_INT_SRC_EN(). So:

		int_clr &= ~TSADCV2_INT_SRC_EN(sensor->id);

> +		writel_relaxed(int_clr, thermal->regs + TSADCV2_INT_EN);
> +		return 0;
> +	}
> +
>  	tsadc->set_alarm_temp(&tsadc->table,
>  			      sensor->id, thermal->regs, high);

Given that you have a level of indirection here for ->set_alarm_temp(),
it seems like you should not be doing the above register programming
directly in this function; it should either go in the ->set_alarm_temp()
callback, or in a new callback like ->disable_alarm().

Brian

>  
> -- 
> 2.7.4
>