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

From: Caesar Wang
Date: Mon Dec 12 2016 - 05:46:43 EST

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