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

From: Caesar Wang
Date: Tue Nov 22 2016 - 21:07:07 EST


å 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@xxxxxxxxxxxxxx>
---

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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip