[PATCH v2] of: thermal: Introduce "linux,hwmon" optional property

[PATCH v2] of: thermal: Introduce "linux,hwmon" optional property

[Michael Tatarinov]

Introduce an optional property called "linux,hwmon", which enable
registration in hwmon subsystems.

Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Signed-off-by: Michael Tatarinov <kukabu@gmail.com>
---
 Documentation/devicetree/bindings/thermal/thermal.txt | 6 ++++++
 drivers/thermal/of-thermal.c                          | 7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
index 88b6ea1ad290..0dad55e77c7e 100644
--- a/Documentation/devicetree/bindings/thermal/thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/thermal.txt
@@ -175,6 +175,10 @@ Optional property:
 			2000mW, while on a 10'' tablet is around
 			4500mW.
 
+- linux,hwmon:		Register the thermal zone in hwmon subsystems
+  Type: boolean 	(requires CONFIG_THERMAL_HWMON).
+  Size: one cell
+
 Note: The delay properties are bound to the maximum dT/dt (temperature
 derivative over time) in two situations for a thermal zone:
 (i)  - when passive cooling is activated (polling-delay-passive); and
@@ -556,6 +560,8 @@ thermal-zones {
 
 		sustainable-power = <2500>;
 
+		linux,hwmon;
+
 		trips {
 			/* Trips are based on resulting linear equation */
 			cpu_trip: cpu-trip {
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index d04ec3b9e5ff..037ef6bb9420 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -994,8 +994,11 @@ int __init of_parse_thermal_zones(void)
 			goto exit_free;
 		}
 
-		/* No hwmon because there might be hwmon drivers registering */
-		tzp->no_hwmon = true;
+		/**
+		 * No hwmon by default because there might be hwmon drivers
+		 * registering
+		 */
+		tzp->no_hwmon = !of_property_read_bool(child, "linux,hwmon");
 
 		if (!of_property_read_u32(child, "sustainable-power", &prop))
 			tzp->sustainable_power = prop;
-- 
2.13.6

Re: [PATCH v2] of: thermal: Introduce "linux,hwmon" optional property

[Daniel Lezcano]

On 05/10/2017 10:42, Michael Tatarinov wrote:
> Introduce an optional property called "linux,hwmon", which enable
> registration in hwmon subsystems.
> 
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Signed-off-by: Michael Tatarinov <kukabu@gmail.com>
> ---
>  Documentation/devicetree/bindings/thermal/thermal.txt | 6 ++++++
>  drivers/thermal/of-thermal.c                          | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/thermal.txt b/Documentation/devicetree/bindings/thermal/thermal.txt
> index 88b6ea1ad290..0dad55e77c7e 100644
> --- a/Documentation/devicetree/bindings/thermal/thermal.txt
> +++ b/Documentation/devicetree/bindings/thermal/thermal.txt
> @@ -175,6 +175,10 @@ Optional property:
>  			2000mW, while on a 10'' tablet is around
>  			4500mW.
>  
> +- linux,hwmon:		Register the thermal zone in hwmon subsystems
> +  Type: boolean 	(requires CONFIG_THERMAL_HWMON).
> +  Size: one cell
> +
>  Note: The delay properties are bound to the maximum dT/dt (temperature
>  derivative over time) in two situations for a thermal zone:
>  (i)  - when passive cooling is activated (polling-delay-passive); and
> @@ -556,6 +560,8 @@ thermal-zones {
>  
>  		sustainable-power = <2500>;
>  
> +		linux,hwmon;
> +
>  		trips {
>  			/* Trips are based on resulting linear equation */
>  			cpu_trip: cpu-trip {
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index d04ec3b9e5ff..037ef6bb9420 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -994,8 +994,11 @@ int __init of_parse_thermal_zones(void)
>  			goto exit_free;
>  		}
>  
> -		/* No hwmon because there might be hwmon drivers registering */
> -		tzp->no_hwmon = true;
> +		/**

Nit: this is kerneldoc comment format "/**"

> +		 * No hwmon by default because there might be hwmon drivers
> +		 * registering
> +		 */
> +		tzp->no_hwmon = !of_property_read_bool(child, "linux,hwmon");
>  
>  		if (!of_property_read_u32(child, "sustainable-power", &prop))
>  			tzp->sustainable_power = prop;
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2] of: thermal: Introduce "linux,hwmon" optional property

[Sudeep Holla]

Please add devicetree bindings mailing list when you are add such
generic and linux specific bindings. For now, I disagree on the approach
unless you convince that's the only way on devicetree list and get
ACK from DT binding maintainers.

On 05/10/17 09:42, Michael Tatarinov wrote:
> Introduce an optional property called "linux,hwmon", which enable
> registration in hwmon subsystems.
> 

Both the commit and the property binding seem to conflict with the
comment in Documentation/thermal/sysfs-api.txt

It states "when no_hwmon == false, a hwmon sysfs interface will be
created" while you seem to indicate that this is required to register
to hwmon subsystems.

If it's just to enable this sysfs access, then binding seems a bit of
misuse.

-- 
Regards,
Sudeep

Re: [PATCH v2] of: thermal: Introduce "linux,hwmon" optional property

[Michael Tatarinov]

Hello

2017-10-05 18:57 GMT+04:00, Sudeep Holla <sudeep.holla@arm.com>:
> Please add devicetree bindings mailing list when you are add such
> generic and linux specific bindings. For now, I disagree on the approach
> unless you convince that's the only way on devicetree list and get
> ACK from DT binding maintainers.

OK, I will do it after deciding your next issue

> On 05/10/17 09:42, Michael Tatarinov wrote:
>> Introduce an optional property called "linux,hwmon", which enable
>> registration in hwmon subsystems.
>>
>
> Both the commit and the property binding seem to conflict with the
> comment in Documentation/thermal/sysfs-api.txt
>
> It states "when no_hwmon == false, a hwmon sysfs interface will be
> created" while you seem to indicate that this is required to register
> to hwmon subsystems.
>
> If it's just to enable this sysfs access, then binding seems a bit of
> misuse.

You're right but if the property is "linux,no_hwmon" we've enabled
this for all drivers by default.
I do not want to change the default behavior.

>
> --
> Regards,
> Sudeep
>

Re: [PATCH v2] of: thermal: Introduce "linux,hwmon" optional property

[Sudeep Holla]

On 06/10/17 03:44, Michael Tatarinov wrote:
> Hello
> 
> 2017-10-05 18:57 GMT+04:00, Sudeep Holla <sudeep.holla@arm.com>:
>> Please add devicetree bindings mailing list when you are add such
>> generic and linux specific bindings. For now, I disagree on the approach
>> unless you convince that's the only way on devicetree list and get
>> ACK from DT binding maintainers.
> 
> OK, I will do it after deciding your next issue
> 
>> On 05/10/17 09:42, Michael Tatarinov wrote:
>>> Introduce an optional property called "linux,hwmon", which enable
>>> registration in hwmon subsystems.
>>>
>>
>> Both the commit and the property binding seem to conflict with the
>> comment in Documentation/thermal/sysfs-api.txt
>>
>> It states "when no_hwmon == false, a hwmon sysfs interface will be
>> created" while you seem to indicate that this is required to register
>> to hwmon subsystems.
>>
>> If it's just to enable this sysfs access, then binding seems a bit of
>> misuse.
> 
> You're right but if the property is "linux,no_hwmon" we've enabled
> this for all drivers by default.
> I do not want to change the default behavior.
> 

But you can't just create Linux specific bindings to influence the way
you want drivers to behave. In certain cases, we do need linux specific
bindings for some configurations. But I am not convinced this one falls
in that category. You can always check what DT maintainers think
especially when you creating some linux specific bindings which looks so
generic.

-- 
Regards,
Sudeep