[V3,3/6] thermal: Register hwmon in thermal_zone_of_sensor_register_param()
diff mbox series

Message ID 20181217155644.29278-4-marek.vasut@gmail.com
State Under Review
Delegated to: Geert Uytterhoeven
Headers show
Series
  • thermal: Align devm_thermal_zone_{device,of_sensor}_register
Related show

Commit Message

Marek Vasut Dec. 17, 2018, 3:56 p.m. UTC
From: Marek Vasut <marek.vasut@gmail.com>

Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
in case thermal_zone_params->no_hwmon is set to false. This behavior is
the same as thermal_zone_device_register().

From: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: linux-renesas-soc@vger.kernel.org
To: linux-pm@vger.kernel.org
Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
---
V2: No change
V3: - Work around the From line and SoB line checkpatch warning
    - Reorder the SoB line at the end
---
 drivers/thermal/of-thermal.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Wolfram Sang Dec. 17, 2018, 6:12 p.m. UTC | #1
> +			if (!IS_ERR(tzd)) {
> +				tzd->tzp = tzp;

I tried to read the thermal core but couldn't really figure it. We are
not leaking/overwriting the tzp pointer acquired in of-thermal.c here?
Simon Horman Dec. 18, 2018, 11:04 a.m. UTC | #2
On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut@gmail.com>
> 
> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
> in case thermal_zone_params->no_hwmon is set to false. This behavior is
> the same as thermal_zone_device_register().

As per v2:

If it is the same then is there any value in a helper function?

Regardless, this patch looks good to me.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-pm@vger.kernel.org
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> ---
> V2: No change
> V3: - Work around the From line and SoB line checkpatch warning
>     - Reorder the SoB line at the end
> ---
>  drivers/thermal/of-thermal.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index e1a303a5698c..5ccff7b678de 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -15,6 +15,7 @@
>  #include <linux/string.h>
>  
>  #include "thermal_core.h"
> +#include "thermal_hwmon.h"
>  
>  /***   Private data structures to represent thermal device tree data ***/
>  
> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>  							 data, ops);
> -			if (!IS_ERR(tzd))
> +			if (!IS_ERR(tzd)) {
> +				tzd->tzp = tzp;
>  				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
> +				if (!tzp || !tzp->no_hwmon) {
> +					ret = thermal_add_hwmon_sysfs(tzd);
> +					if (ret)
> +						tzd = ERR_PTR(ret);
> +				}
> +			}
>  
>  			of_node_put(sensor_specs.np);
>  			of_node_put(child);
> @@ -605,6 +613,8 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  
>  	tz = tzd->devdata;
>  
> +	thermal_remove_hwmon_sysfs(tzd);
> +
>  	/* no __thermal_zone, nothing to be done */
>  	if (!tz)
>  		return;
> -- 
> 2.18.0
>
Eduardo Valentin Dec. 18, 2018, 9:44 p.m. UTC | #3
On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
> From: Marek Vasut <marek.vasut@gmail.com>
> 
> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
> in case thermal_zone_params->no_hwmon is set to false. This behavior is
> the same as thermal_zone_device_register().
> 
> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: Eduardo Valentin <edubezval@gmail.com>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: linux-renesas-soc@vger.kernel.org
> To: linux-pm@vger.kernel.org
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> ---
> V2: No change
> V3: - Work around the From line and SoB line checkpatch warning
>     - Reorder the SoB line at the end
> ---
>  drivers/thermal/of-thermal.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index e1a303a5698c..5ccff7b678de 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -15,6 +15,7 @@
>  #include <linux/string.h>
>  
>  #include "thermal_core.h"
> +#include "thermal_hwmon.h"
>  
>  /***   Private data structures to represent thermal device tree data ***/
>  
> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>  							 data, ops);
> -			if (!IS_ERR(tzd))
> +			if (!IS_ERR(tzd)) {
> +				tzd->tzp = tzp;

So, here you will overwrite what was done in of_parse_thermal_zones().
That means, after this point, property like sustainable power, slope and
offset are gone.

>  				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
> +				if (!tzp || !tzp->no_hwmon) {
> +					ret = thermal_add_hwmon_sysfs(tzd);
> +					if (ret)
> +						tzd = ERR_PTR(ret);
> +				}
> +			}
>  
>  			of_node_put(sensor_specs.np);
>  			of_node_put(child);
> @@ -605,6 +613,8 @@ void thermal_zone_of_sensor_unregister(struct device *dev,
>  
>  	tz = tzd->devdata;
>  
> +	thermal_remove_hwmon_sysfs(tzd);
> +
>  	/* no __thermal_zone, nothing to be done */
>  	if (!tz)
>  		return;
> -- 
> 2.18.0
>
Marek Vasut Dec. 22, 2018, 2:16 a.m. UTC | #4
On 12/17/2018 07:12 PM, Wolfram Sang wrote:
> 
>> +			if (!IS_ERR(tzd)) {
>> +				tzd->tzp = tzp;
> 
> I tried to read the thermal core but couldn't really figure it. We are
> not leaking/overwriting the tzp pointer acquired in of-thermal.c here?

I think so, and Eduardo seem to be confirming it, albeit it's rather
inobvious.
Marek Vasut Dec. 22, 2018, 2:19 a.m. UTC | #5
On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
>> From: Marek Vasut <marek.vasut@gmail.com>
>>
>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
>> the same as thermal_zone_device_register().
>>
>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: Eduardo Valentin <edubezval@gmail.com>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: Zhang Rui <rui.zhang@intel.com>
>> Cc: linux-renesas-soc@vger.kernel.org
>> To: linux-pm@vger.kernel.org
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> ---
>> V2: No change
>> V3: - Work around the From line and SoB line checkpatch warning
>>     - Reorder the SoB line at the end
>> ---
>>  drivers/thermal/of-thermal.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>> index e1a303a5698c..5ccff7b678de 100644
>> --- a/drivers/thermal/of-thermal.c
>> +++ b/drivers/thermal/of-thermal.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/string.h>
>>  
>>  #include "thermal_core.h"
>> +#include "thermal_hwmon.h"
>>  
>>  /***   Private data structures to represent thermal device tree data ***/
>>  
>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>  							 data, ops);
>> -			if (!IS_ERR(tzd))
>> +			if (!IS_ERR(tzd)) {
>> +				tzd->tzp = tzp;
> 
> So, here you will overwrite what was done in of_parse_thermal_zones().
> That means, after this point, property like sustainable power, slope and
> offset are gone.

Hmmmmm, that was rather inobvious, indeed.

Do you have some suggestion how to pass in the no_hwmon = false then ?
Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
to clean that up.
Marek Vasut Jan. 15, 2019, 12:35 a.m. UTC | #6
On 12/22/18 3:19 AM, Marek Vasut wrote:
> On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
>> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
>>> From: Marek Vasut <marek.vasut@gmail.com>
>>>
>>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
>>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
>>> the same as thermal_zone_device_register().
>>>
>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: Eduardo Valentin <edubezval@gmail.com>
>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>> Cc: linux-renesas-soc@vger.kernel.org
>>> To: linux-pm@vger.kernel.org
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> ---
>>> V2: No change
>>> V3: - Work around the From line and SoB line checkpatch warning
>>>     - Reorder the SoB line at the end
>>> ---
>>>  drivers/thermal/of-thermal.c | 12 +++++++++++-
>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>> index e1a303a5698c..5ccff7b678de 100644
>>> --- a/drivers/thermal/of-thermal.c
>>> +++ b/drivers/thermal/of-thermal.c
>>> @@ -15,6 +15,7 @@
>>>  #include <linux/string.h>
>>>  
>>>  #include "thermal_core.h"
>>> +#include "thermal_hwmon.h"
>>>  
>>>  /***   Private data structures to represent thermal device tree data ***/
>>>  
>>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>>  							 data, ops);
>>> -			if (!IS_ERR(tzd))
>>> +			if (!IS_ERR(tzd)) {
>>> +				tzd->tzp = tzp;
>>
>> So, here you will overwrite what was done in of_parse_thermal_zones().
>> That means, after this point, property like sustainable power, slope and
>> offset are gone.
> 
> Hmmmmm, that was rather inobvious, indeed.
> 
> Do you have some suggestion how to pass in the no_hwmon = false then ?
> Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
> three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
> to clean that up.

Bump ?
Marek Vasut Jan. 28, 2019, 12:10 p.m. UTC | #7
On 1/15/19 1:35 AM, Marek Vasut wrote:
> On 12/22/18 3:19 AM, Marek Vasut wrote:
>> On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
>>> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
>>>> From: Marek Vasut <marek.vasut@gmail.com>
>>>>
>>>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
>>>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
>>>> the same as thermal_zone_device_register().
>>>>
>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>> Cc: Eduardo Valentin <edubezval@gmail.com>
>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>> To: linux-pm@vger.kernel.org
>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>> ---
>>>> V2: No change
>>>> V3: - Work around the From line and SoB line checkpatch warning
>>>>     - Reorder the SoB line at the end
>>>> ---
>>>>  drivers/thermal/of-thermal.c | 12 +++++++++++-
>>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>> index e1a303a5698c..5ccff7b678de 100644
>>>> --- a/drivers/thermal/of-thermal.c
>>>> +++ b/drivers/thermal/of-thermal.c
>>>> @@ -15,6 +15,7 @@
>>>>  #include <linux/string.h>
>>>>  
>>>>  #include "thermal_core.h"
>>>> +#include "thermal_hwmon.h"
>>>>  
>>>>  /***   Private data structures to represent thermal device tree data ***/
>>>>  
>>>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>>>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>>>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>>>  							 data, ops);
>>>> -			if (!IS_ERR(tzd))
>>>> +			if (!IS_ERR(tzd)) {
>>>> +				tzd->tzp = tzp;
>>>
>>> So, here you will overwrite what was done in of_parse_thermal_zones().
>>> That means, after this point, property like sustainable power, slope and
>>> offset are gone.
>>
>> Hmmmmm, that was rather inobvious, indeed.
>>
>> Do you have some suggestion how to pass in the no_hwmon = false then ?
>> Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
>> three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
>> to clean that up.
> 
> Bump ?

Bump again, any suggestions ?
Eduardo Valentin Feb. 5, 2019, 11:24 p.m. UTC | #8
On Mon, Jan 28, 2019 at 01:10:11PM +0100, Marek Vasut wrote:
> On 1/15/19 1:35 AM, Marek Vasut wrote:
> > On 12/22/18 3:19 AM, Marek Vasut wrote:
> >> On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
> >>> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
> >>>> From: Marek Vasut <marek.vasut@gmail.com>
> >>>>
> >>>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
> >>>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
> >>>> the same as thermal_zone_device_register().
> >>>>
> >>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>> Cc: Eduardo Valentin <edubezval@gmail.com>
> >>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>> Cc: Zhang Rui <rui.zhang@intel.com>
> >>>> Cc: linux-renesas-soc@vger.kernel.org
> >>>> To: linux-pm@vger.kernel.org
> >>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>> ---
> >>>> V2: No change
> >>>> V3: - Work around the From line and SoB line checkpatch warning
> >>>>     - Reorder the SoB line at the end
> >>>> ---
> >>>>  drivers/thermal/of-thermal.c | 12 +++++++++++-
> >>>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >>>> index e1a303a5698c..5ccff7b678de 100644
> >>>> --- a/drivers/thermal/of-thermal.c
> >>>> +++ b/drivers/thermal/of-thermal.c
> >>>> @@ -15,6 +15,7 @@
> >>>>  #include <linux/string.h>
> >>>>  
> >>>>  #include "thermal_core.h"
> >>>> +#include "thermal_hwmon.h"
> >>>>  
> >>>>  /***   Private data structures to represent thermal device tree data ***/
> >>>>  
> >>>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
> >>>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
> >>>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
> >>>>  							 data, ops);
> >>>> -			if (!IS_ERR(tzd))
> >>>> +			if (!IS_ERR(tzd)) {
> >>>> +				tzd->tzp = tzp;
> >>>
> >>> So, here you will overwrite what was done in of_parse_thermal_zones().
> >>> That means, after this point, property like sustainable power, slope and
> >>> offset are gone.
> >>
> >> Hmmmmm, that was rather inobvious, indeed.
> >>
> >> Do you have some suggestion how to pass in the no_hwmon = false then ?
> >> Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
> >> three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
> >> to clean that up.
> > 

Yeah, that is an issue.

> > Bump ?
> 
> Bump again, any suggestions ?

Yeah, a couple of ideas have been proposed for this issue.

First most tempting one is to have a DT property per thermal zone.
Making it linux specific, something prefixed by linux,<property>. I
recall Amit Kutcheria trying something similar to this, but dont
remember where that went. Frankly, this is a Linux thing, I am not
convinced DT is really the right place to fix this.

Another hack that could be written is a module parameter for of-thermal
that would reflect the no_hwmon value, globally. The down side here is
you have to make sure all drivers match that no_hwmon value, right?
> 
> -- 
> Best regards,
> Marek Vasut
Marek Vasut Feb. 11, 2019, 7:29 p.m. UTC | #9
On 2/6/19 12:24 AM, Eduardo Valentin wrote:
> On Mon, Jan 28, 2019 at 01:10:11PM +0100, Marek Vasut wrote:
>> On 1/15/19 1:35 AM, Marek Vasut wrote:
>>> On 12/22/18 3:19 AM, Marek Vasut wrote:
>>>> On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
>>>>> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
>>>>>> From: Marek Vasut <marek.vasut@gmail.com>
>>>>>>
>>>>>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
>>>>>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
>>>>>> the same as thermal_zone_device_register().
>>>>>>
>>>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>>> Cc: Eduardo Valentin <edubezval@gmail.com>
>>>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>> To: linux-pm@vger.kernel.org
>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>> ---
>>>>>> V2: No change
>>>>>> V3: - Work around the From line and SoB line checkpatch warning
>>>>>>     - Reorder the SoB line at the end
>>>>>> ---
>>>>>>  drivers/thermal/of-thermal.c | 12 +++++++++++-
>>>>>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>>>> index e1a303a5698c..5ccff7b678de 100644
>>>>>> --- a/drivers/thermal/of-thermal.c
>>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>  #include <linux/string.h>
>>>>>>  
>>>>>>  #include "thermal_core.h"
>>>>>> +#include "thermal_hwmon.h"
>>>>>>  
>>>>>>  /***   Private data structures to represent thermal device tree data ***/
>>>>>>  
>>>>>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>>>>>>  		if (sensor_specs.np == sensor_np && id == sensor_id) {
>>>>>>  			tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>>>>>  							 data, ops);
>>>>>> -			if (!IS_ERR(tzd))
>>>>>> +			if (!IS_ERR(tzd)) {
>>>>>> +				tzd->tzp = tzp;
>>>>>
>>>>> So, here you will overwrite what was done in of_parse_thermal_zones().
>>>>> That means, after this point, property like sustainable power, slope and
>>>>> offset are gone.
>>>>
>>>> Hmmmmm, that was rather inobvious, indeed.
>>>>
>>>> Do you have some suggestion how to pass in the no_hwmon = false then ?
>>>> Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
>>>> three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
>>>> to clean that up.
>>>
> 
> Yeah, that is an issue.
> 
>>> Bump ?
>>
>> Bump again, any suggestions ?
> 
> Yeah, a couple of ideas have been proposed for this issue.
> 
> First most tempting one is to have a DT property per thermal zone.
> Making it linux specific, something prefixed by linux,<property>. I
> recall Amit Kutcheria trying something similar to this, but dont
> remember where that went. Frankly, this is a Linux thing, I am not
> convinced DT is really the right place to fix this.

DT is not the right place for this, DT describes hardware and this is
policy, so it shouldn't be in DT.

> Another hack that could be written is a module parameter for of-thermal
> that would reflect the no_hwmon value, globally. The down side here is
> you have to make sure all drivers match that no_hwmon value, right?

Indeed, it should be the driver which decides whether or not to register
a HWMON interface for the thermal zone.

I guess for now, I'll just discard this entire series and hack the data
structure like other drivers do, since I don't see any reasonable solution.
Geert Uytterhoeven Feb. 12, 2019, 8:52 a.m. UTC | #10
Hi all,

On Mon, Feb 11, 2019 at 9:43 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> On 2/6/19 12:24 AM, Eduardo Valentin wrote:
> > On Mon, Jan 28, 2019 at 01:10:11PM +0100, Marek Vasut wrote:
> >> On 1/15/19 1:35 AM, Marek Vasut wrote:
> >>> On 12/22/18 3:19 AM, Marek Vasut wrote:
> >>>> On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
> >>>>> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
> >>>>>> From: Marek Vasut <marek.vasut@gmail.com>
> >>>>>>
> >>>>>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
> >>>>>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
> >>>>>> the same as thermal_zone_device_register().
> >>>>>>
> >>>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>>>> Cc: Eduardo Valentin <edubezval@gmail.com>
> >>>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>>>> Cc: Zhang Rui <rui.zhang@intel.com>
> >>>>>> Cc: linux-renesas-soc@vger.kernel.org
> >>>>>> To: linux-pm@vger.kernel.org
> >>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>>> ---
> >>>>>> V2: No change
> >>>>>> V3: - Work around the From line and SoB line checkpatch warning
> >>>>>>     - Reorder the SoB line at the end
> >>>>>> ---
> >>>>>>  drivers/thermal/of-thermal.c | 12 +++++++++++-
> >>>>>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> >>>>>> index e1a303a5698c..5ccff7b678de 100644
> >>>>>> --- a/drivers/thermal/of-thermal.c
> >>>>>> +++ b/drivers/thermal/of-thermal.c
> >>>>>> @@ -15,6 +15,7 @@
> >>>>>>  #include <linux/string.h>
> >>>>>>
> >>>>>>  #include "thermal_core.h"
> >>>>>> +#include "thermal_hwmon.h"
> >>>>>>
> >>>>>>  /***   Private data structures to represent thermal device tree data ***/
> >>>>>>
> >>>>>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
> >>>>>>                  if (sensor_specs.np == sensor_np && id == sensor_id) {
> >>>>>>                          tzd = thermal_zone_of_add_sensor(child, sensor_np,
> >>>>>>                                                           data, ops);
> >>>>>> -                        if (!IS_ERR(tzd))
> >>>>>> +                        if (!IS_ERR(tzd)) {
> >>>>>> +                                tzd->tzp = tzp;
> >>>>>
> >>>>> So, here you will overwrite what was done in of_parse_thermal_zones().
> >>>>> That means, after this point, property like sustainable power, slope and
> >>>>> offset are gone.
> >>>>
> >>>> Hmmmmm, that was rather inobvious, indeed.
> >>>>
> >>>> Do you have some suggestion how to pass in the no_hwmon = false then ?
> >>>> Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
> >>>> three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
> >>>> to clean that up.
> >>>
> >
> > Yeah, that is an issue.
> >
> >>> Bump ?
> >>
> >> Bump again, any suggestions ?
> >
> > Yeah, a couple of ideas have been proposed for this issue.
> >
> > First most tempting one is to have a DT property per thermal zone.
> > Making it linux specific, something prefixed by linux,<property>. I
> > recall Amit Kutcheria trying something similar to this, but dont
> > remember where that went. Frankly, this is a Linux thing, I am not
> > convinced DT is really the right place to fix this.
>
> DT is not the right place for this, DT describes hardware and this is
> policy, so it shouldn't be in DT.

Indeed.

> > Another hack that could be written is a module parameter for of-thermal
> > that would reflect the no_hwmon value, globally. The down side here is
> > you have to make sure all drivers match that no_hwmon value, right?
>
> Indeed, it should be the driver which decides whether or not to register
> a HWMON interface for the thermal zone.
>
> I guess for now, I'll just discard this entire series and hack the data
> structure like other drivers do, since I don't see any reasonable solution.

Pardon my ignorance, but when would a thermal driver (not) want to register
a hwmon interface for the thermal zone?

Gr{oetje,eeting}s,

                        Geert
Guenter Roeck Feb. 12, 2019, 4:12 p.m. UTC | #11
On 2/12/19 12:52 AM, Geert Uytterhoeven wrote:
> Hi all,
> 
> On Mon, Feb 11, 2019 at 9:43 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>> On 2/6/19 12:24 AM, Eduardo Valentin wrote:
>>> On Mon, Jan 28, 2019 at 01:10:11PM +0100, Marek Vasut wrote:
>>>> On 1/15/19 1:35 AM, Marek Vasut wrote:
>>>>> On 12/22/18 3:19 AM, Marek Vasut wrote:
>>>>>> On 12/18/2018 10:44 PM, Eduardo Valentin wrote:
>>>>>>> On Mon, Dec 17, 2018 at 04:56:41PM +0100, marek.vasut@gmail.com wrote:
>>>>>>>> From: Marek Vasut <marek.vasut@gmail.com>
>>>>>>>>
>>>>>>>> Register hwmon sysfs interface in thermal_zone_of_sensor_register_param()
>>>>>>>> in case thermal_zone_params->no_hwmon is set to false. This behavior is
>>>>>>>> the same as thermal_zone_device_register().
>>>>>>>>
>>>>>>>> From: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>>>>> Cc: Eduardo Valentin <edubezval@gmail.com>
>>>>>>>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>>>>>> Cc: Zhang Rui <rui.zhang@intel.com>
>>>>>>>> Cc: linux-renesas-soc@vger.kernel.org
>>>>>>>> To: linux-pm@vger.kernel.org
>>>>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>>>>> ---
>>>>>>>> V2: No change
>>>>>>>> V3: - Work around the From line and SoB line checkpatch warning
>>>>>>>>      - Reorder the SoB line at the end
>>>>>>>> ---
>>>>>>>>   drivers/thermal/of-thermal.c | 12 +++++++++++-
>>>>>>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
>>>>>>>> index e1a303a5698c..5ccff7b678de 100644
>>>>>>>> --- a/drivers/thermal/of-thermal.c
>>>>>>>> +++ b/drivers/thermal/of-thermal.c
>>>>>>>> @@ -15,6 +15,7 @@
>>>>>>>>   #include <linux/string.h>
>>>>>>>>
>>>>>>>>   #include "thermal_core.h"
>>>>>>>> +#include "thermal_hwmon.h"
>>>>>>>>
>>>>>>>>   /***   Private data structures to represent thermal device tree data ***/
>>>>>>>>
>>>>>>>> @@ -521,8 +522,15 @@ thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
>>>>>>>>                   if (sensor_specs.np == sensor_np && id == sensor_id) {
>>>>>>>>                           tzd = thermal_zone_of_add_sensor(child, sensor_np,
>>>>>>>>                                                            data, ops);
>>>>>>>> -                        if (!IS_ERR(tzd))
>>>>>>>> +                        if (!IS_ERR(tzd)) {
>>>>>>>> +                                tzd->tzp = tzp;
>>>>>>>
>>>>>>> So, here you will overwrite what was done in of_parse_thermal_zones().
>>>>>>> That means, after this point, property like sustainable power, slope and
>>>>>>> offset are gone.
>>>>>>
>>>>>> Hmmmmm, that was rather inobvious, indeed.
>>>>>>
>>>>>> Do you have some suggestion how to pass in the no_hwmon = false then ?
>>>>>> Since tzp->no_hwmon is set to true in of_parse_thermal_zones(), the
>>>>>> three drivers (stm32, rcar, rcar_gen3) seem to hack around it. I'd like
>>>>>> to clean that up.
>>>>>
>>>
>>> Yeah, that is an issue.
>>>
>>>>> Bump ?
>>>>
>>>> Bump again, any suggestions ?
>>>
>>> Yeah, a couple of ideas have been proposed for this issue.
>>>
>>> First most tempting one is to have a DT property per thermal zone.
>>> Making it linux specific, something prefixed by linux,<property>. I
>>> recall Amit Kutcheria trying something similar to this, but dont
>>> remember where that went. Frankly, this is a Linux thing, I am not
>>> convinced DT is really the right place to fix this.
>>
>> DT is not the right place for this, DT describes hardware and this is
>> policy, so it shouldn't be in DT.
> 
> Indeed.
> 
>>> Another hack that could be written is a module parameter for of-thermal
>>> that would reflect the no_hwmon value, globally. The down side here is
>>> you have to make sure all drivers match that no_hwmon value, right?
>>
>> Indeed, it should be the driver which decides whether or not to register
>> a HWMON interface for the thermal zone.
>>
>> I guess for now, I'll just discard this entire series and hack the data
>> structure like other drivers do, since I don't see any reasonable solution.
> 
> Pardon my ignorance, but when would a thermal driver (not) want to register
> a hwmon interface for the thermal zone?
> 

Thermal zones can also be registered from hwmon drivers, so we have a
chicken-and-egg problem.

Guenter

Patch
diff mbox series

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index e1a303a5698c..5ccff7b678de 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -15,6 +15,7 @@ 
 #include <linux/string.h>
 
 #include "thermal_core.h"
+#include "thermal_hwmon.h"
 
 /***   Private data structures to represent thermal device tree data ***/
 
@@ -521,8 +522,15 @@  thermal_zone_of_sensor_register_params(struct device *dev, int sensor_id,
 		if (sensor_specs.np == sensor_np && id == sensor_id) {
 			tzd = thermal_zone_of_add_sensor(child, sensor_np,
 							 data, ops);
-			if (!IS_ERR(tzd))
+			if (!IS_ERR(tzd)) {
+				tzd->tzp = tzp;
 				tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED);
+				if (!tzp || !tzp->no_hwmon) {
+					ret = thermal_add_hwmon_sysfs(tzd);
+					if (ret)
+						tzd = ERR_PTR(ret);
+				}
+			}
 
 			of_node_put(sensor_specs.np);
 			of_node_put(child);
@@ -605,6 +613,8 @@  void thermal_zone_of_sensor_unregister(struct device *dev,
 
 	tz = tzd->devdata;
 
+	thermal_remove_hwmon_sysfs(tzd);
+
 	/* no __thermal_zone, nothing to be done */
 	if (!tz)
 		return;