linux-kernel - Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 3 Mar 2016 10:45:23 +0000
From:	Javi Merino <javi.merino@....com>
To:	Leo Yan <leo.yan@...aro.org>
Cc:	Wei Xu <xuwei5@...ilicon.com>, Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Zhang Rui <rui.zhang@...el.com>,
	Eduardo Valentin <edubezval@...il.com>,
	kongxinwei <kong.kongxinwei@...ilicon.com>,
	Punit Agrawal <Punit.Agrawal@....com>,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 1/5] thermal: change "hysteresis" as optional property

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@...aro.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@....com>

> +	else
> +		trip->hysteresis = prop;
>  
>  	ret = thermal_of_get_trip_type(np, &trip->type);
>  	if (ret < 0) {
> -- 
> 1.9.1
>