* [bug report] thermal: add brcmstb AVS TMON driver
@ 2017-09-06 8:22 Dan Carpenter
2017-09-06 17:00 ` Brian Norris
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2017-09-06 8:22 UTC (permalink / raw)
To: computersforpeace; +Cc: linux-pm
Hello Brian Norris,
The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
Aug 9, 2017, leads to the following static checker warning:
drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)'
drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)'
drivers/thermal/broadcom/brcmstb_thermal.c
274 static int brcmstb_set_trips(void *data, int low, int high)
275 {
276 struct brcmstb_thermal_priv *priv = data;
277
278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
279
280 if (low) {
281 if (low > INT_MAX)
^^^^^^^^^^^^^
Never true
282 low = INT_MAX;
283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
285 } else {
286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
287 }
288
289 if (high < ULONG_MAX) {
^^^^^^^^^^^^^^^^
Always true
290 if (high > INT_MAX)
^^^^^^^^^^^^^^
Never true
291 high = INT_MAX;
292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
294 } else {
295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
296 }
297
298 return 0;
299 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver
2017-09-06 8:22 [bug report] thermal: add brcmstb AVS TMON driver Dan Carpenter
@ 2017-09-06 17:00 ` Brian Norris
2017-09-06 18:08 ` Florian Fainelli
0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2017-09-06 17:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-pm, Doug Berger, Florian Fainelli
On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote:
> Hello Brian Norris,
Hi Dan! Or Dan's robots.
> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
I'll blame Doug or Florian :)
Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the
tree where I first wrote this driver (before 'set_trips' was even merged
upstream). We can probably simplify this now.
But I'll leave that to Doug.
Brian
> Aug 9, 2017, leads to the following static checker warning:
>
> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)'
>
> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)'
>
> drivers/thermal/broadcom/brcmstb_thermal.c
> 274 static int brcmstb_set_trips(void *data, int low, int high)
> 275 {
> 276 struct brcmstb_thermal_priv *priv = data;
> 277
> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
> 279
> 280 if (low) {
> 281 if (low > INT_MAX)
> ^^^^^^^^^^^^^
> Never true
>
> 282 low = INT_MAX;
> 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
> 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
> 285 } else {
> 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
> 287 }
> 288
> 289 if (high < ULONG_MAX) {
> ^^^^^^^^^^^^^^^^
> Always true
>
> 290 if (high > INT_MAX)
> ^^^^^^^^^^^^^^
> Never true
>
> 291 high = INT_MAX;
> 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
> 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
> 294 } else {
> 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
> 296 }
> 297
> 298 return 0;
> 299 }
>
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver
2017-09-06 17:00 ` Brian Norris
@ 2017-09-06 18:08 ` Florian Fainelli
2017-09-06 18:37 ` Brian Norris
0 siblings, 1 reply; 8+ messages in thread
From: Florian Fainelli @ 2017-09-06 18:08 UTC (permalink / raw)
To: Brian Norris, Dan Carpenter; +Cc: linux-pm, Doug Berger, colin.king
On 09/06/2017 10:00 AM, Brian Norris wrote:
> On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote:
>> Hello Brian Norris,
>
> Hi Dan! Or Dan's robots.
>
>> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
>
> I'll blame Doug or Florian :)
>
> Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the
> tree where I first wrote this driver (before 'set_trips' was even merged
> upstream). We can probably simplify this now.
That is exactly what happened yes :) and Colin already sent a fix for that:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html
>
> But I'll leave that to Doug.
>
> Brian
>
>> Aug 9, 2017, leads to the following static checker warning:
>>
>> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
>> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)'
>>
>> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
>> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)'
>>
>> drivers/thermal/broadcom/brcmstb_thermal.c
>> 274 static int brcmstb_set_trips(void *data, int low, int high)
>> 275 {
>> 276 struct brcmstb_thermal_priv *priv = data;
>> 277
>> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
>> 279
>> 280 if (low) {
>> 281 if (low > INT_MAX)
>> ^^^^^^^^^^^^^
>> Never true
>>
>> 282 low = INT_MAX;
>> 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
>> 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
>> 285 } else {
>> 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
>> 287 }
>> 288
>> 289 if (high < ULONG_MAX) {
>> ^^^^^^^^^^^^^^^^
>> Always true
>>
>> 290 if (high > INT_MAX)
>> ^^^^^^^^^^^^^^
>> Never true
>>
>> 291 high = INT_MAX;
>> 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
>> 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
>> 294 } else {
>> 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
>> 296 }
>> 297
>> 298 return 0;
>> 299 }
>>
>>
>> regards,
>> dan carpenter
--
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver
2017-09-06 18:08 ` Florian Fainelli
@ 2017-09-06 18:37 ` Brian Norris
2017-09-06 19:33 ` Doug Berger
0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2017-09-06 18:37 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Dan Carpenter, linux-pm, Doug Berger, colin.king
Hi,
On Wed, Sep 06, 2017 at 11:08:59AM -0700, Florian Fainelli wrote:
> On 09/06/2017 10:00 AM, Brian Norris wrote:
> > On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote:
> >> Hello Brian Norris,
> >
> > Hi Dan! Or Dan's robots.
> >
> >> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
> >
> > I'll blame Doug or Florian :)
> >
> > Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the
> > tree where I first wrote this driver (before 'set_trips' was even merged
> > upstream). We can probably simplify this now.
>
> That is exactly what happened yes :) and Colin already sent a fix for that:
>
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html
Colin didn't fix all the problems. See below:
> >
> > But I'll leave that to Doug.
> >
> > Brian
> >
> >> Aug 9, 2017, leads to the following static checker warning:
> >>
> >> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
> >> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max > s32max)'
> >>
> >> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
> >> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max > s32max)'
> >>
> >> drivers/thermal/broadcom/brcmstb_thermal.c
> >> 274 static int brcmstb_set_trips(void *data, int low, int high)
> >> 275 {
> >> 276 struct brcmstb_thermal_priv *priv = data;
> >> 277
> >> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
> >> 279
> >> 280 if (low) {
> >> 281 if (low > INT_MAX)
> >> ^^^^^^^^^^^^^
> >> Never true
> >>
> >> 282 low = INT_MAX;
> >> 283 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
> >> 284 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
> >> 285 } else {
> >> 286 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
> >> 287 }
> >> 288
> >> 289 if (high < ULONG_MAX) {
> >> ^^^^^^^^^^^^^^^^
> >> Always true
We didn't expect this one ^^^ and Colin didn't fix it. We expected a
"max" value to turn the trip point off (in the 'else' branch). But we
can never possibly reach the 'else'.
Maybe my expectations are off now (I'm not really that familiar with the
current state of the thermal framework), but at any rate, we shouldn't
be leaving dead code.
I'll make a quick comment on Colin's patch too, so this doesn't get
lost.
Brian
> >> 290 if (high > INT_MAX)
> >> ^^^^^^^^^^^^^^
> >> Never true
> >>
> >> 291 high = INT_MAX;
> >> 292 avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
> >> 293 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
> >> 294 } else {
> >> 295 avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
> >> 296 }
> >> 297
> >> 298 return 0;
> >> 299 }
> >>
> >>
> >> regards,
> >> dan carpenter
>
>
> --
> Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver
2017-09-06 18:37 ` Brian Norris
@ 2017-09-06 19:33 ` Doug Berger
2017-09-06 19:46 ` Markus Mayer
0 siblings, 1 reply; 8+ messages in thread
From: Doug Berger @ 2017-09-06 19:33 UTC (permalink / raw)
To: Brian Norris, Florian Fainelli, Markus Mayer
Cc: Dan Carpenter, linux-pm, colin.king
On 09/06/2017 11:37 AM, Brian Norris wrote:
> Hi,
>
> On Wed, Sep 06, 2017 at 11:08:59AM -0700, Florian Fainelli wrote:
>> On 09/06/2017 10:00 AM, Brian Norris wrote:
>>> On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote:
>>>> Hello Brian Norris,
>>>
>>> Hi Dan! Or Dan's robots.
>>>
>>>> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
>>>
>>> I'll blame Doug or Florian :)
I of course have to blame Markus ;)
>>>
>>> Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the
>>> tree where I first wrote this driver (before 'set_trips' was even merged
>>> upstream). We can probably simplify this now.
>>
>> That is exactly what happened yes :) and Colin already sent a fix for
that:
>>
>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html
>
> Colin didn't fix all the problems. See below:
>
>>>
>>> But I'll leave that to Doug.
>>>
>>> Brian
>>>
>>>> Aug 9, 2017, leads to the following static checker warning:
>>>>
>>>> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
>>>> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max
> s32max)'
>>>>
>>>> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
>>>> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max
> s32max)'
>>>>
>>>> drivers/thermal/broadcom/brcmstb_thermal.c
>>>> 274 static int brcmstb_set_trips(void *data, int low, int high)
>>>> 275 {
>>>> 276 struct brcmstb_thermal_priv *priv = data;
>>>> 277
>>>> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low,
high);
>>>> 279
>>>> 280 if (low) {
>>>> 281 if (low > INT_MAX)
>>>> ^^^^^^^^^^^^^
>>>> Never true
>>>>
>>>> 282 low = INT_MAX;
>>>> 283 avs_tmon_set_trip_temp(priv,
TMON_TRIP_TYPE_LOW, low);
>>>> 284 avs_tmon_trip_enable(priv,
TMON_TRIP_TYPE_LOW, 1);
>>>> 285 } else {
>>>> 286 avs_tmon_trip_enable(priv,
TMON_TRIP_TYPE_LOW, 0);
>>>> 287 }
>>>> 288
>>>> 289 if (high < ULONG_MAX) {
>>>> ^^^^^^^^^^^^^^^^
>>>> Always true
>
> We didn't expect this one ^^^ and Colin didn't fix it. We expected a
> "max" value to turn the trip point off (in the 'else' branch). But we
> can never possibly reach the 'else'.
This actually appears to be the heart of the matter.
As stated above, the set_trips patch API that this logic was developed
for was different from the version of the patch ultimately merged
upstream and the brcmstb_thermal driver logic was not corrected before
being submitted upstream.
The key change appears to be as follows:
Old API - unsigned long ints and "If there is no trip point above or
below the current temperature, the passed trip temperature will be
ULONG_MAX or 0 respectively.
New API - ints and "If there is no trip point above or below the current
temperature, the passed trip temperature will be -INT_MAX or INT_MAX
respectively."
So there are latent logic errors in addition to the "dead code" errors
flagged here.
>
> Maybe my expectations are off now (I'm not really that familiar with the
> current state of the thermal framework), but at any rate, we shouldn't
> be leaving dead code.
>
> I'll make a quick comment on Colin's patch too, so this doesn't get
> lost.
>
> Brian
>
>>>> 290 if (high > INT_MAX)
>>>> ^^^^^^^^^^^^^^
>>>> Never true
>>>>
>>>> 291 high = INT_MAX;
>>>> 292 avs_tmon_set_trip_temp(priv,
TMON_TRIP_TYPE_HIGH, high);
>>>> 293 avs_tmon_trip_enable(priv,
TMON_TRIP_TYPE_HIGH, 1);
>>>> 294 } else {
>>>> 295 avs_tmon_trip_enable(priv,
TMON_TRIP_TYPE_HIGH, 0);
>>>> 296 }
>>>> 297
>>>> 298 return 0;
>>>> 299 }
>>>>
>>>>
>>>> regards,
>>>> dan carpenter
>>
>>
>> --
>> Florian
I'll leave it to Markus to correct the logic, unless he wants to punt it
back to me :).
Thanks!
Doug
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] thermal: add brcmstb AVS TMON driver
2017-09-06 19:33 ` Doug Berger
@ 2017-09-06 19:46 ` Markus Mayer
2017-09-14 23:19 ` [PATCH] thermal: brcmstb: disable trip points properly when needed Markus Mayer
0 siblings, 1 reply; 8+ messages in thread
From: Markus Mayer @ 2017-09-06 19:46 UTC (permalink / raw)
To: Doug Berger
Cc: Brian Norris, Florian Fainelli, Markus Mayer, Dan Carpenter,
Power Management List, colin.king
On 6 September 2017 at 16:33, Doug Berger <opendmb@gmail.com> wrote:
> On 09/06/2017 11:37 AM, Brian Norris wrote:
>> Hi,
>>
>> On Wed, Sep 06, 2017 at 11:08:59AM -0700, Florian Fainelli wrote:
>>> On 09/06/2017 10:00 AM, Brian Norris wrote:
>>>> On Wed, Sep 06, 2017 at 11:22:42AM +0300, Dan Carpenter wrote:
>>>>> Hello Brian Norris,
>>>>
>>>> Hi Dan! Or Dan's robots.
>>>>
>>>>> The patch 1e21c74eda83: "thermal: add brcmstb AVS TMON driver" from
>>>>
>>>> I'll blame Doug or Florian :)
>
> I of course have to blame Markus ;)
Tried fleeing to the other side of the continent, but it caught up
with me anyway. ;-)
>>>>
>>>> Actually, I'm pretty sure 'low' and 'high' were 'unsigned long' in the
>>>> tree where I first wrote this driver (before 'set_trips' was even merged
>>>> upstream). We can probably simplify this now.
>>>
>>> That is exactly what happened yes :) and Colin already sent a fix for
> that:
>>>
>>> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1483884.html
>>
>> Colin didn't fix all the problems. See below:
>>
>>>>
>>>> But I'll leave that to Doug.
>>>>
>>>> Brian
>>>>
>>>>> Aug 9, 2017, leads to the following static checker warning:
>>>>>
>>>>> drivers/thermal/broadcom/brcmstb_thermal.c:281 brcmstb_set_trips()
>>>>> warn: impossible condition '(low > ((~0 >> 1))) => (s32min-s32max
>> s32max)'
>>>>>
>>>>> drivers/thermal/broadcom/brcmstb_thermal.c:290 brcmstb_set_trips()
>>>>> warn: impossible condition '(high > ((~0 >> 1))) => (s32min-s32max
>> s32max)'
>>>>>
>>>>> drivers/thermal/broadcom/brcmstb_thermal.c
>>>>> 274 static int brcmstb_set_trips(void *data, int low, int high)
>>>>> 275 {
>>>>> 276 struct brcmstb_thermal_priv *priv = data;
>>>>> 277
>>>>> 278 dev_dbg(priv->dev, "set trips %d <--> %d\n", low,
> high);
>>>>> 279
>>>>> 280 if (low) {
>>>>> 281 if (low > INT_MAX)
>>>>> ^^^^^^^^^^^^^
>>>>> Never true
>>>>>
>>>>> 282 low = INT_MAX;
>>>>> 283 avs_tmon_set_trip_temp(priv,
> TMON_TRIP_TYPE_LOW, low);
>>>>> 284 avs_tmon_trip_enable(priv,
> TMON_TRIP_TYPE_LOW, 1);
>>>>> 285 } else {
>>>>> 286 avs_tmon_trip_enable(priv,
> TMON_TRIP_TYPE_LOW, 0);
>>>>> 287 }
>>>>> 288
>>>>> 289 if (high < ULONG_MAX) {
>>>>> ^^^^^^^^^^^^^^^^
>>>>> Always true
>>
>> We didn't expect this one ^^^ and Colin didn't fix it. We expected a
>> "max" value to turn the trip point off (in the 'else' branch). But we
>> can never possibly reach the 'else'.
> This actually appears to be the heart of the matter.
>
> As stated above, the set_trips patch API that this logic was developed
> for was different from the version of the patch ultimately merged
> upstream and the brcmstb_thermal driver logic was not corrected before
> being submitted upstream.
>
> The key change appears to be as follows:
>
> Old API - unsigned long ints and "If there is no trip point above or
> below the current temperature, the passed trip temperature will be
> ULONG_MAX or 0 respectively.
>
> New API - ints and "If there is no trip point above or below the current
> temperature, the passed trip temperature will be -INT_MAX or INT_MAX
> respectively."
>
> So there are latent logic errors in addition to the "dead code" errors
> flagged here.
>
>>
>> Maybe my expectations are off now (I'm not really that familiar with the
>> current state of the thermal framework), but at any rate, we shouldn't
>> be leaving dead code.
>>
>> I'll make a quick comment on Colin's patch too, so this doesn't get
>> lost.
>>
>> Brian
>>
>>>>> 290 if (high > INT_MAX)
>>>>> ^^^^^^^^^^^^^^
>>>>> Never true
>>>>>
>>>>> 291 high = INT_MAX;
>>>>> 292 avs_tmon_set_trip_temp(priv,
> TMON_TRIP_TYPE_HIGH, high);
>>>>> 293 avs_tmon_trip_enable(priv,
> TMON_TRIP_TYPE_HIGH, 1);
>>>>> 294 } else {
>>>>> 295 avs_tmon_trip_enable(priv,
> TMON_TRIP_TYPE_HIGH, 0);
>>>>> 296 }
>>>>> 297
>>>>> 298 return 0;
>>>>> 299 }
>>>>>
>>>>>
>>>>> regards,
>>>>> dan carpenter
>>>
>>>
>>> --
>>> Florian
>
> I'll leave it to Markus to correct the logic, unless he wants to punt it
> back to me :).
I'll fix it up when I get back home next week.
Cheers,
-Markus
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] thermal: brcmstb: disable trip points properly when needed
2017-09-06 19:46 ` Markus Mayer
@ 2017-09-14 23:19 ` Markus Mayer
2017-09-14 23:25 ` Markus Mayer
0 siblings, 1 reply; 8+ messages in thread
From: Markus Mayer @ 2017-09-14 23:19 UTC (permalink / raw)
To: Zhang Rui, Eduardo Valentin, Doug Berger, Brian Norris,
Florian Fainelli, Dan Carpenter, Colin King
Cc: Markus Mayer, Power Management List, ARM Kernel List,
Linux Kernel Mailing List
From: Markus Mayer <mmayer@broadcom.com>
The code checking for low and high temperature points was still based
on an earlier implementation of the driver. It wasn't working properly
with the data types currently being used.
We fix this by disabling the high trip point if our high temperature is
INT_MAX and disabling the low trip point if our low temperature is -INT_MAX
(or lower).
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
Here is my patch for the issue described. Please let me know if I should be
squashing this into the driver patch and re-submit the entire series.
drivers/thermal/broadcom/brcmstb_thermal.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
index 87b8e7a..1919f91 100644
--- a/drivers/thermal/broadcom/brcmstb_thermal.c
+++ b/drivers/thermal/broadcom/brcmstb_thermal.c
@@ -277,22 +277,23 @@ static int brcmstb_set_trips(void *data, int low, int high)
dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
- if (low) {
- if (low > INT_MAX)
- low = INT_MAX;
+ /*
+ * Disable low-temp if "low" is too small. As per thermal framework
+ * API, we use -INT_MAX rather than INT_MIN.
+ */
+ if (low <= -INT_MAX) {
+ avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
+ } else {
avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
- } else {
- avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
}
- if (high < ULONG_MAX) {
- if (high > INT_MAX)
- high = INT_MAX;
+ /* Disable high-temp if "high" is too big. */
+ if (high == INT_MAX) {
+ avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
+ } else {
avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
- } else {
- avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
}
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: brcmstb: disable trip points properly when needed
2017-09-14 23:19 ` [PATCH] thermal: brcmstb: disable trip points properly when needed Markus Mayer
@ 2017-09-14 23:25 ` Markus Mayer
0 siblings, 0 replies; 8+ messages in thread
From: Markus Mayer @ 2017-09-14 23:25 UTC (permalink / raw)
To: Markus Mayer
Cc: Zhang Rui, Eduardo Valentin, Doug Berger, Brian Norris,
Florian Fainelli, Dan Carpenter, Colin King,
Power Management List, ARM Kernel List, Linux Kernel Mailing List
On 14 September 2017 at 16:19, Markus Mayer <code@mmayer.net> wrote:
> From: Markus Mayer <mmayer@broadcom.com>
>
> The code checking for low and high temperature points was still based
> on an earlier implementation of the driver. It wasn't working properly
> with the data types currently being used.
>
> We fix this by disabling the high trip point if our high temperature is
> INT_MAX and disabling the low trip point if our low temperature is -INT_MAX
> (or lower).
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>
> Here is my patch for the issue described. Please let me know if I should be
> squashing this into the driver patch and re-submit the entire series.
Looks like my attempt at adding this patch to the existing thread may
not have worked out so well.
Here's a link: https://marc.info/?l=linux-pm&m=150472721929919&w=2
> drivers/thermal/broadcom/brcmstb_thermal.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> index 87b8e7a..1919f91 100644
> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> @@ -277,22 +277,23 @@ static int brcmstb_set_trips(void *data, int low, int high)
>
> dev_dbg(priv->dev, "set trips %d <--> %d\n", low, high);
>
> - if (low) {
> - if (low > INT_MAX)
> - low = INT_MAX;
> + /*
> + * Disable low-temp if "low" is too small. As per thermal framework
> + * API, we use -INT_MAX rather than INT_MIN.
> + */
> + if (low <= -INT_MAX) {
> + avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
> + } else {
> avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_LOW, low);
> avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 1);
> - } else {
> - avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_LOW, 0);
> }
>
> - if (high < ULONG_MAX) {
> - if (high > INT_MAX)
> - high = INT_MAX;
> + /* Disable high-temp if "high" is too big. */
> + if (high == INT_MAX) {
> + avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
> + } else {
> avs_tmon_set_trip_temp(priv, TMON_TRIP_TYPE_HIGH, high);
> avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 1);
> - } else {
> - avs_tmon_trip_enable(priv, TMON_TRIP_TYPE_HIGH, 0);
> }
>
> return 0;
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-14 23:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-06 8:22 [bug report] thermal: add brcmstb AVS TMON driver Dan Carpenter
2017-09-06 17:00 ` Brian Norris
2017-09-06 18:08 ` Florian Fainelli
2017-09-06 18:37 ` Brian Norris
2017-09-06 19:33 ` Doug Berger
2017-09-06 19:46 ` Markus Mayer
2017-09-14 23:19 ` [PATCH] thermal: brcmstb: disable trip points properly when needed Markus Mayer
2017-09-14 23:25 ` Markus Mayer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox