| Message ID | 20181217155644.29278-4-marek.vasut@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | thermal: Align devm_thermal_zone_{device,of_sensor}_register | expand |
> + 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?
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 >
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 >
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.
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.
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 ?
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 ?
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
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.
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
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
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;