diff mbox

[v2,1/5] thermal: change "hysteresis" as optional property

Message ID 1456458227-12950-2-git-send-email-leo.yan@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Leo Yan Feb. 26, 2016, 3:43 a.m. UTC
The property "hysteresis" is mandatory for trip points, so if without
it the thermal zone cannot register successfully. But "hysteresis" is
ignored in the thermal subsystem and only inquired by several thermal
sensor drivers.

So change "hysteresis" as optional properties.

Signed-off-by: Leo Yan <leo.yan@linaro.org>
---
 Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++----
 drivers/thermal/of-thermal.c                          | 9 ++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Javi Merino March 3, 2016, 10:45 a.m. UTC | #1
On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> The property "hysteresis" is mandatory for trip points, so if without
> it the thermal zone cannot register successfully. But "hysteresis" is
> ignored in the thermal subsystem and only inquired by several thermal
> sensor drivers.
> 
> So change "hysteresis" as optional properties.

I had forgotten that hysteresis is enforced in DT.  Thanks for fixing
this!
 
> Signed-off-by: Leo Yan <leo.yan@linaro.org>
> ---
>  Documentation/devicetree/bindings/thermal/thermal.txt | 9 +++++----
>  drivers/thermal/of-thermal.c                          | 9 ++++-----
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 41b817f..7d79e77 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -89,10 +89,6 @@ Required properties:
>    Type: signed		in millicelsius.
>    Size: one cell
>  
> -- hysteresis:		A low hysteresis value on temperature property (above).
> -  Type: unsigned	This is a relative value, in millicelsius.
> -  Size: one cell
> -
>  - type:			a string containing the trip type. Expected values are:
>  	"active":	A trip point to enable active cooling
>  	"passive":	A trip point to enable passive cooling
> @@ -100,6 +96,11 @@ Required properties:
>  	"critical":	Hardware not reliable.
>    Type: string
>  
> +Optional properties:
> +- hysteresis:		A low hysteresis value on temperature property (above).
> +  Type: unsigned	This is a relative value, in millicelsius.
> +  Size: one cell
> +
>  * Cooling device maps
>  
>  The cooling device maps node is a node to describe how cooling devices
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 9043f8f..ab05500 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -689,11 +689,10 @@ static int thermal_of_populate_trip(struct device_node *np,
>  	trip->temperature = prop;
>  
>  	ret = of_property_read_u32(np, "hysteresis", &prop);
> -	if (ret < 0) {
> -		pr_err("missing hysteresis property\n");
> -		return ret;
> -	}
> -	trip->hysteresis = prop;
> +	if (ret < 0)
> +		pr_warning("missing hysteresis property\n");

I'd remove the warning.  It is an optional parameter, so there is no
need to warn about something going wrong.  As you say in the commit
log, it is ignored in the thermal subsystem and only used by some
sensors so no need to warn about it missing.

Other than that, I'm happy to see this merged.

Acked-by: Javi Merino <javi.merino@arm.com>

> +	else
> +		trip->hysteresis = prop;
>  
>  	ret = thermal_of_get_trip_type(np, &trip->type);
>  	if (ret < 0) {
> -- 
> 1.9.1
>
Eduardo Valentin March 3, 2016, 4:29 p.m. UTC | #2
Hi Leo,

On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> The property "hysteresis" is mandatory for trip points, so if without
> it the thermal zone cannot register successfully. But "hysteresis" is
> ignored in the thermal subsystem and only inquired by several thermal
> sensor drivers.

I am not sure this a good direction to go. Remember that Linux
implementation not necessarily has to be the implication of the DT
binding. Hysteresis is a property that plays a significant role on
thermal control systems, which in many cases avoid overshooting cooling
actions. Having the DT writer to explicitly set it to 0 means that zone
does not suffer of overshooting and does not need hysteresis.

If the Linux thermal subsystem has a problem with handling hysteresis, I
would rather fix Linux code than relaxing the DT binding. Or if you
still believe hysteresis is really optional, I would prefer to see a
better justification than "Linux ignores it".

BR,

Eduardo
Leo Yan March 4, 2016, 3:03 a.m. UTC | #3
Hi Eduardo,

On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> Hi Leo,
> 
> On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > The property "hysteresis" is mandatory for trip points, so if without
> > it the thermal zone cannot register successfully. But "hysteresis" is
> > ignored in the thermal subsystem and only inquired by several thermal
> > sensor drivers.
> 
> I am not sure this a good direction to go. Remember that Linux
> implementation not necessarily has to be the implication of the DT
> binding. Hysteresis is a property that plays a significant role on
> thermal control systems, which in many cases avoid overshooting cooling
> actions. Having the DT writer to explicitly set it to 0 means that zone
> does not suffer of overshooting and does not need hysteresis.

After review current code, the "hysteresis" is used to calculate
temperature falling threshold with a more conservative value; so that
finally avoid overshooting issue.

Please confirm if is my understanding correct or not?

> If the Linux thermal subsystem has a problem with handling hysteresis, I
> would rather fix Linux code than relaxing the DT binding. Or if you
> still believe hysteresis is really optional, I would prefer to see a
> better justification than "Linux ignores it".

If we think about power allocator governor, PID's two parameters are
also used to dismiss overshooting issue: one is k_po (proportional
term), another is k_i (integral term). So that means after we apply
power allocator governor, we don't need parameter "hysteresis" due PID
algorithm can automatically dismiss potential errors.

Does this make sense?

Thanks,
Leo Yan
Javi Merino March 4, 2016, 11:57 a.m. UTC | #4
On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> Hi Eduardo,
> 
> On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > Hi Leo,
> > 
> > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > > The property "hysteresis" is mandatory for trip points, so if without
> > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > ignored in the thermal subsystem and only inquired by several thermal
> > > sensor drivers.
> > 
> > I am not sure this a good direction to go. Remember that Linux
> > implementation not necessarily has to be the implication of the DT
> > binding. Hysteresis is a property that plays a significant role on
> > thermal control systems, which in many cases avoid overshooting cooling
> > actions. Having the DT writer to explicitly set it to 0 means that zone
> > does not suffer of overshooting and does not need hysteresis.
> 
> After review current code, the "hysteresis" is used to calculate
> temperature falling threshold with a more conservative value; so that
> finally avoid overshooting issue.
> 
> Please confirm if is my understanding correct or not?
> 
> > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > would rather fix Linux code than relaxing the DT binding. Or if you
> > still believe hysteresis is really optional, I would prefer to see a
> > better justification than "Linux ignores it".

I see it the other way round, Is hysteresis a property that, without
it, the thermal code can't configure itself so it fails to create the
trip point?  The current code goes "There is no hysteresis for this
property, I don't know how to set up this trip point!".  I think we
can do better than this.

> If we think about power allocator governor, PID's two parameters are
> also used to dismiss overshooting issue: one is k_po (proportional
> term), another is k_i (integral term). So that means after we apply
> power allocator governor, we don't need parameter "hysteresis" due PID
> algorithm can automatically dismiss potential errors.

I disagree.  We shouldn't base DT decisions based on only one
governor in linux.  Having said that, AFAICS all governors currently
ignore hysteresis.

Cheers,
Javi
Leo Yan March 8, 2016, 1:57 p.m. UTC | #5
Hi Eduardo,

On Fri, Mar 04, 2016 at 11:57:53AM +0000, Javi Merino wrote:
> On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> > On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:

[...]

> > > > The property "hysteresis" is mandatory for trip points, so if without
> > > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > > ignored in the thermal subsystem and only inquired by several thermal
> > > > sensor drivers.
> > > 
> > > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > > would rather fix Linux code than relaxing the DT binding. Or if you
> > > still believe hysteresis is really optional, I would prefer to see a
> > > better justification than "Linux ignores it".
> 
> I see it the other way round, Is hysteresis a property that, without
> it, the thermal code can't configure itself so it fails to create the
> trip point?  The current code goes "There is no hysteresis for this
> property, I don't know how to set up this trip point!".  I think we
> can do better than this.

Do you agree with Javi's suggestion? If you think it's okay, I will
move on to send out a new version patch based on Javi's comments.

Thanks,
Leo Yan
Eduardo Valentin March 8, 2016, 8:55 p.m. UTC | #6
On Tue, Mar 08, 2016 at 09:57:43PM +0800, Leo Yan wrote:
> Hi Eduardo,
> 
> On Fri, Mar 04, 2016 at 11:57:53AM +0000, Javi Merino wrote:
> > On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> > > On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > > > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> 
> [...]
> 
> > > > > The property "hysteresis" is mandatory for trip points, so if without
> > > > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > > > ignored in the thermal subsystem and only inquired by several thermal
> > > > > sensor drivers.
> > > > 
> > > > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > > > would rather fix Linux code than relaxing the DT binding. Or if you
> > > > still believe hysteresis is really optional, I would prefer to see a
> > > > better justification than "Linux ignores it".
> > 
> > I see it the other way round, Is hysteresis a property that, without
> > it, the thermal code can't configure itself so it fails to create the
> > trip point?  The current code goes "There is no hysteresis for this
> > property, I don't know how to set up this trip point!".  I think we
> > can do better than this.
> 
> Do you agree with Javi's suggestion? If you think it's okay, I will
> move on to send out a new version patch based on Javi's comments.

No I don't. This discussion so far has been about Linux code. I still
havent seen an argument explaining why hysteresis has to be optional.

BR,

> 
> Thanks,
> Leo Yan
Javi Merino March 9, 2016, 11:10 a.m. UTC | #7
On Tue, Mar 08, 2016 at 12:55:59PM -0800, Eduardo Valentin wrote:
> On Tue, Mar 08, 2016 at 09:57:43PM +0800, Leo Yan wrote:
> > Hi Eduardo,
> > 
> > On Fri, Mar 04, 2016 at 11:57:53AM +0000, Javi Merino wrote:
> > > On Fri, Mar 04, 2016 at 11:03:49AM +0800, Leo Yan wrote:
> > > > On Thu, Mar 03, 2016 at 08:29:44AM -0800, Eduardo Valentin wrote:
> > > > > On Fri, Feb 26, 2016 at 11:43:43AM +0800, Leo Yan wrote:
> > 
> > [...]
> > 
> > > > > > The property "hysteresis" is mandatory for trip points, so if without
> > > > > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > > > > ignored in the thermal subsystem and only inquired by several thermal
> > > > > > sensor drivers.
> > > > > 
> > > > > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > > > > would rather fix Linux code than relaxing the DT binding. Or if you
> > > > > still believe hysteresis is really optional, I would prefer to see a
> > > > > better justification than "Linux ignores it".
> > > 
> > > I see it the other way round, Is hysteresis a property that, without
> > > it, the thermal code can't configure itself so it fails to create the
> > > trip point?  The current code goes "There is no hysteresis for this
> > > property, I don't know how to set up this trip point!".  I think we
> > > can do better than this.
> > 
> > Do you agree with Javi's suggestion? If you think it's okay, I will
> > move on to send out a new version patch based on Javi's comments.
> 
> No I don't. This discussion so far has been about Linux code. I still
> havent seen an argument explaining why hysteresis has to be optional.

Fair enough.  Looks like I'm holding this driver from being
upstreamed, so I'll back off.

Leo, sorry for misguiding you.  Please bring back the hysteresis
property you had in v1.
Leo Yan March 20, 2016, 3:40 p.m. UTC | #8
Hi Javi,

Sorry for late response.

On Wed, Mar 09, 2016 at 11:10:52AM +0000, Javi Merino wrote:
> On Tue, Mar 08, 2016 at 12:55:59PM -0800, Eduardo Valentin wrote:
> > On Tue, Mar 08, 2016 at 09:57:43PM +0800, Leo Yan wrote:

[...]

> > > > > > > The property "hysteresis" is mandatory for trip points, so if without
> > > > > > > it the thermal zone cannot register successfully. But "hysteresis" is
> > > > > > > ignored in the thermal subsystem and only inquired by several thermal
> > > > > > > sensor drivers.
> > > > > > 
> > > > > > If the Linux thermal subsystem has a problem with handling hysteresis, I
> > > > > > would rather fix Linux code than relaxing the DT binding. Or if you
> > > > > > still believe hysteresis is really optional, I would prefer to see a
> > > > > > better justification than "Linux ignores it".
> > > > 
> > > > I see it the other way round, Is hysteresis a property that, without
> > > > it, the thermal code can't configure itself so it fails to create the
> > > > trip point?  The current code goes "There is no hysteresis for this
> > > > property, I don't know how to set up this trip point!".  I think we
> > > > can do better than this.
> > > 
> > > Do you agree with Javi's suggestion? If you think it's okay, I will
> > > move on to send out a new version patch based on Javi's comments.
> > 
> > No I don't. This discussion so far has been about Linux code. I still
> > havent seen an argument explaining why hysteresis has to be optional.
> 
> Fair enough.  Looks like I'm holding this driver from being
> upstreamed, so I'll back off.
> 
> Leo, sorry for misguiding you.  Please bring back the hysteresis
> property you had in v1.

Not at all. I will add back hysteresis property and resend new patches.

Thanks,
Leo Yan
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
index 41b817f..7d79e77 100644
--- a/Documentation/devicetree/bindings/thermal/thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/thermal.txt
@@ -89,10 +89,6 @@  Required properties:
   Type: signed		in millicelsius.
   Size: one cell
 
-- hysteresis:		A low hysteresis value on temperature property (above).
-  Type: unsigned	This is a relative value, in millicelsius.
-  Size: one cell
-
 - type:			a string containing the trip type. Expected values are:
 	"active":	A trip point to enable active cooling
 	"passive":	A trip point to enable passive cooling
@@ -100,6 +96,11 @@  Required properties:
 	"critical":	Hardware not reliable.
   Type: string
 
+Optional properties:
+- hysteresis:		A low hysteresis value on temperature property (above).
+  Type: unsigned	This is a relative value, in millicelsius.
+  Size: one cell
+
 * Cooling device maps
 
 The cooling device maps node is a node to describe how cooling devices
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 9043f8f..ab05500 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -689,11 +689,10 @@  static int thermal_of_populate_trip(struct device_node *np,
 	trip->temperature = prop;
 
 	ret = of_property_read_u32(np, "hysteresis", &prop);
-	if (ret < 0) {
-		pr_err("missing hysteresis property\n");
-		return ret;
-	}
-	trip->hysteresis = prop;
+	if (ret < 0)
+		pr_warning("missing hysteresis property\n");
+	else
+		trip->hysteresis = prop;
 
 	ret = thermal_of_get_trip_type(np, &trip->type);
 	if (ret < 0) {