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 70 > 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 70 > trip_point_0_temp", and you keep trip1 
and trip2


 [   34.449718] trip_point_temp_store, temp=70
[   34.454568] of_thermal_set_trip_temp:336,temp=70
[   34.459612] thermal_sys: thermal_zone_set_trips:583, temp=45000, 
trip_temp=70, 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 ff26.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=70, hys=2000
[  100.906964] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=71, hys=2000
[  100.916329] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=72, hys=2000
[  100.924685] thermal thermal_zone0: new temperature boundaries: 
-2147483647 < x < 70
[  100.932965] rockchip-thermal ff26.tsadc: 
rockchip_thermal_set_trips: sensor 0: low: -2147483647, high 70

[  100.943138] rk_tsadcv2_alarm_temp:682, temp=70
[  100.948201] rk_tsadcv2_temp_to_code: invalid temperature, temp=70 
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-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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 70 > 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 70 > trip_point_0_temp", and you keep trip1 
and trip2


 [   34.449718] trip_point_temp_store, temp=70
[   34.454568] of_thermal_set_trip_temp:336,temp=70
[   34.459612] thermal_sys: thermal_zone_set_trips:583, temp=45000, 
trip_temp=70, 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 ff26.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=70, hys=2000
[  100.906964] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=71, hys=2000
[  100.916329] thermal_sys: thermal_zone_set_trips:583, temp=58333, 
trip_temp=72, hys=2000
[  100.924685] thermal thermal_zone0: new temperature boundaries: 
-2147483647 < x < 70
[  100.932965] rockchip-thermal ff26.tsadc: 
rockchip_thermal_set_trips: sensor 0: low: -2147483647, high 70

[  100.943138] rk_tsadcv2_alarm_temp:682, temp=70
[  100.948201] rk_tsadcv2_temp_to_code: invalid temperature, temp=70 
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-rockc...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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 70 > 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 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 70 > 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 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 70 > 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

[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 70 > 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]

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.
> > 
> > 
> > 
> > >  
> > >   /* 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 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.
> > 
> > 
> > 
> > >  
> > >   /* 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 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.
> 
> 
> 
> >  
> > /* 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

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



>  
>   /* 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(>pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>   __func__, sensor->id, low, high);
>  
> - tsadc->set_alarm_temp(>table,
> -   sensor->id, thermal->regs, high);
> -
> - return 0;
> + return tsadc->set_alarm_temp(>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(>table, id, thermal->regs,
> +
> + error = tsadc->set_tshut_temp(>table, id, thermal->regs,
> thermal->tshut_temp);
> + if (error)
> + dev_err(>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(>chip->table,
> +
> + error = thermal->chip->set_tshut_temp(>chip->table,
> id, thermal->regs,
> thermal->tshut_temp);
> + if (error)
> + dev_err(>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 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.



>  
>   /* 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(>pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>   __func__, sensor->id, low, high);
>  
> - tsadc->set_alarm_temp(>table,
> -   sensor->id, thermal->regs, high);
> -
> - return 0;
> + return tsadc->set_alarm_temp(>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(>table, id, thermal->regs,
> +
> + error = tsadc->set_tshut_temp(>table, id, thermal->regs,
> thermal->tshut_temp);
> + if (error)
> + dev_err(>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(>chip->table,
> +
> + error = thermal->chip->set_tshut_temp(>chip->table,
> id, thermal->regs,
> thermal->tshut_temp);
> + if (error)
> + dev_err(>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 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 
> ---
> 
> 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 

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