| Message ID | 20191212061702.BFE2D6E85603@corona.crabdance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | [RESEND] thermal: rockchip: enable hwmon | expand |
On Thu, Dec 12, 2019 at 11:47 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > By default, of-based thermal drivers do not enable hwmon. > Explicitly enable hwmon for both, the soc and gpu temperature > sensor. Is there any reason you need to expose this in hwmon? > Signed-off-by: Stefan Schaeckeler <schaecsn@gmx.net> > > --- > drivers/thermal/rockchip_thermal.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c > index 343c2f5c5a25..e47c60010259 100644 > --- a/drivers/thermal/rockchip_thermal.c > +++ b/drivers/thermal/rockchip_thermal.c > @@ -19,6 +19,8 @@ > #include <linux/mfd/syscon.h> > #include <linux/pinctrl/consumer.h> > > +#include "thermal_hwmon.h" > + > /** > * If the temperature over a period of time High, > * the resulting TSHUT gave CRU module,let it reset the entire chip, > @@ -1321,8 +1323,15 @@ static int rockchip_thermal_probe(struct platform_device *pdev) > > thermal->chip->control(thermal->regs, true); > > - for (i = 0; i < thermal->chip->chn_num; i++) > + for (i = 0; i < thermal->chip->chn_num; i++) { > rockchip_thermal_toggle_sensor(&thermal->sensors[i], true); > + thermal->sensors[i].tzd->tzp->no_hwmon = false; > + error = thermal_add_hwmon_sysfs(thermal->sensors[i].tzd); > + if (error) > + dev_warn(&pdev->dev, > + "failed to register sensor %d with hwmon: %d\n", > + i, error); > + } > > platform_set_drvdata(pdev, thermal); > > @@ -1344,6 +1353,7 @@ static int rockchip_thermal_remove(struct platform_device *pdev) > for (i = 0; i < thermal->chip->chn_num; i++) { > struct rockchip_thermal_sensor *sensor = &thermal->sensors[i]; > > + thermal_remove_hwmon_sysfs(sensor->tzd); > rockchip_thermal_toggle_sensor(sensor, false); > } > > -- > 2.24.0 >
Hello Amit, > On Thu, Dec 12, 2019 at 11:47 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > > > By default, of-based thermal drivers do not enable hwmon. > > Explicitly enable hwmon for both, the soc and gpu temperature > > sensor. > > Is there any reason you need to expose this in hwmon? Why hwmon: The soc embedds temperature sensors and hwmon is the standard way to expose sensors. Sensors exposed by hwmon are automagically found by userland clients. Users want to run sensors(1) and expect them to show up. Why in rockchip_thermal.c: drivers/thermal/ provides a high-level hwmon api in thermal_hwmon.[hc] which is used by at least these thermal drivers: rcar_gen3_thermal.c, rcar_thermal.c, st/stm_thermal.c, and broadcom/bcm2835_thermal.c. I want to hook up rockchip_thermal.c exactly the same way. Apparently, other architectures hook up the cpu temperature sensors to hwmon elsewhere. Most seem to do this in hwmon/, e.g. hwmon/coretemp.c. These drivers are written from scratch. Utilizing thermal_hwmon.[ch] for chips which have already drivers in drivers/thermal/ seems to be more elegant. Stefan
Hi Stefan, On Fri, Dec 13, 2019 at 4:59 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > Hello Amit, > > > On Thu, Dec 12, 2019 at 11:47 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > > > > > By default, of-based thermal drivers do not enable hwmon. > > > Explicitly enable hwmon for both, the soc and gpu temperature > > > sensor. > > > > Is there any reason you need to expose this in hwmon? > > Why hwmon: > > The soc embedds temperature sensors and hwmon is the standard way to expose > sensors. Let me rephrase - is there something in the hwmon subsystem that is needed that isn't provided by the thermal subsystem inside /sys/class/thermal? > Sensors exposed by hwmon are automagically found by userland clients. Users > want to run sensors(1) and expect them to show up. > That is a good point. In which case, I wonder if we should just fix this in of-thermal.c instead of requiring individual drivers to do write boilerplate code. I'm thinking of a flag that the driver could set to enable the thermal_hwmon interface for of-thermal drivers. > Why in rockchip_thermal.c: > > drivers/thermal/ provides a high-level hwmon api in thermal_hwmon.[hc] which is > used by at least these thermal drivers: rcar_gen3_thermal.c, rcar_thermal.c, > st/stm_thermal.c, and broadcom/bcm2835_thermal.c. I want to hook up > rockchip_thermal.c exactly the same way. > > Apparently, other architectures hook up the cpu temperature sensors to hwmon > elsewhere. Most seem to do this in hwmon/, e.g. hwmon/coretemp.c. These drivers > are written from scratch. Utilizing thermal_hwmon.[ch] for chips which have > already drivers in drivers/thermal/ seems to be more elegant. > > Stefan
Fix Guenter's email. On Fri, Dec 13, 2019 at 10:08 AM Amit Kucheria <amit.kucheria@verdurent.com> wrote: > > Hi Stefan, > > On Fri, Dec 13, 2019 at 4:59 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > > > Hello Amit, > > > > > On Thu, Dec 12, 2019 at 11:47 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > > > > > > > By default, of-based thermal drivers do not enable hwmon. > > > > Explicitly enable hwmon for both, the soc and gpu temperature > > > > sensor. > > > > > > Is there any reason you need to expose this in hwmon? > > > > Why hwmon: > > > > The soc embedds temperature sensors and hwmon is the standard way to expose > > sensors. > > Let me rephrase - is there something in the hwmon subsystem that is > needed that isn't provided by the thermal subsystem inside > /sys/class/thermal? > > > Sensors exposed by hwmon are automagically found by userland clients. Users > > want to run sensors(1) and expect them to show up. > > > > That is a good point. In which case, I wonder if we should just fix > this in of-thermal.c instead of requiring individual drivers to do > write boilerplate code. I'm thinking of a flag that the driver could > set to enable the thermal_hwmon interface for of-thermal drivers. > > > Why in rockchip_thermal.c: > > > > drivers/thermal/ provides a high-level hwmon api in thermal_hwmon.[hc] which is > > used by at least these thermal drivers: rcar_gen3_thermal.c, rcar_thermal.c, > > st/stm_thermal.c, and broadcom/bcm2835_thermal.c. I want to hook up > > rockchip_thermal.c exactly the same way. > > > > Apparently, other architectures hook up the cpu temperature sensors to hwmon > > elsewhere. Most seem to do this in hwmon/, e.g. hwmon/coretemp.c. These drivers > > are written from scratch. Utilizing thermal_hwmon.[ch] for chips which have > > already drivers in drivers/thermal/ seems to be more elegant. > > > > Stefan
On Fri, Dec 13, 2019 at 10:09:49AM +0530, Amit Kucheria wrote: > Fix Guenter's email. > > On Fri, Dec 13, 2019 at 10:08 AM Amit Kucheria > <amit.kucheria@verdurent.com> wrote: > > > > Hi Stefan, > > > > On Fri, Dec 13, 2019 at 4:59 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > > > > > Hello Amit, > > > > > > > On Thu, Dec 12, 2019 at 11:47 AM Stefan Schaeckeler <schaecsn@gmx.net> wrote: > > > > > > > > > > By default, of-based thermal drivers do not enable hwmon. > > > > > Explicitly enable hwmon for both, the soc and gpu temperature > > > > > sensor. > > > > > > > > Is there any reason you need to expose this in hwmon? > > > > > > Why hwmon: > > > > > > The soc embedds temperature sensors and hwmon is the standard way to expose > > > sensors. > > > > Let me rephrase - is there something in the hwmon subsystem that is > > needed that isn't provided by the thermal subsystem inside > > /sys/class/thermal? > > Doesn't the sentence below answer that question ? > > > Sensors exposed by hwmon are automagically found by userland clients. Users > > > want to run sensors(1) and expect them to show up. > > > > > > > That is a good point. In which case, I wonder if we should just fix > > this in of-thermal.c instead of requiring individual drivers to do > > write boilerplate code. I'm thinking of a flag that the driver could > > set to enable the thermal_hwmon interface for of-thermal drivers. It seems to me that would be outside the scope of this patch. > > > > > Why in rockchip_thermal.c: > > > > > > drivers/thermal/ provides a high-level hwmon api in thermal_hwmon.[hc] which is > > > used by at least these thermal drivers: rcar_gen3_thermal.c, rcar_thermal.c, > > > st/stm_thermal.c, and broadcom/bcm2835_thermal.c. I want to hook up > > > rockchip_thermal.c exactly the same way. > > > > > > Apparently, other architectures hook up the cpu temperature sensors to hwmon > > > elsewhere. Most seem to do this in hwmon/, e.g. hwmon/coretemp.c. These drivers > > > are written from scratch. Utilizing thermal_hwmon.[ch] for chips which have > > > already drivers in drivers/thermal/ seems to be more elegant. > > > There should either be a hwmon driver with a bridge to thermal, or a thermal driver with a bridge to hwmon, but not both. A couple of existing drivers implement both, but that should really be fixed. Guenter
On 12/12/2019 07:17, Stefan Schaeckeler wrote: > By default, of-based thermal drivers do not enable hwmon. > Explicitly enable hwmon for both, the soc and gpu temperature > sensor. > > Signed-off-by: Stefan Schaeckeler <schaecsn@gmx.net> Applied, and took the opportunity to test it. (for patchwork) Tested-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > drivers/thermal/rockchip_thermal.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c > index 343c2f5c5a25..e47c60010259 100644 > --- a/drivers/thermal/rockchip_thermal.c > +++ b/drivers/thermal/rockchip_thermal.c > @@ -19,6 +19,8 @@ > #include <linux/mfd/syscon.h> > #include <linux/pinctrl/consumer.h> > > +#include "thermal_hwmon.h" > + > /** > * If the temperature over a period of time High, > * the resulting TSHUT gave CRU module,let it reset the entire chip, > @@ -1321,8 +1323,15 @@ static int rockchip_thermal_probe(struct platform_device *pdev) > > thermal->chip->control(thermal->regs, true); > > - for (i = 0; i < thermal->chip->chn_num; i++) > + for (i = 0; i < thermal->chip->chn_num; i++) { > rockchip_thermal_toggle_sensor(&thermal->sensors[i], true); > + thermal->sensors[i].tzd->tzp->no_hwmon = false; > + error = thermal_add_hwmon_sysfs(thermal->sensors[i].tzd); > + if (error) > + dev_warn(&pdev->dev, > + "failed to register sensor %d with hwmon: %d\n", > + i, error); > + } > > platform_set_drvdata(pdev, thermal); > > @@ -1344,6 +1353,7 @@ static int rockchip_thermal_remove(struct platform_device *pdev) > for (i = 0; i < thermal->chip->chn_num; i++) { > struct rockchip_thermal_sensor *sensor = &thermal->sensors[i]; > > + thermal_remove_hwmon_sysfs(sensor->tzd); > rockchip_thermal_toggle_sensor(sensor, false); > } > > -- > 2.24.0 >
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c index 343c2f5c5a25..e47c60010259 100644 --- a/drivers/thermal/rockchip_thermal.c +++ b/drivers/thermal/rockchip_thermal.c @@ -19,6 +19,8 @@ #include <linux/mfd/syscon.h> #include <linux/pinctrl/consumer.h> +#include "thermal_hwmon.h" + /** * If the temperature over a period of time High, * the resulting TSHUT gave CRU module,let it reset the entire chip, @@ -1321,8 +1323,15 @@ static int rockchip_thermal_probe(struct platform_device *pdev) thermal->chip->control(thermal->regs, true); - for (i = 0; i < thermal->chip->chn_num; i++) + for (i = 0; i < thermal->chip->chn_num; i++) { rockchip_thermal_toggle_sensor(&thermal->sensors[i], true); + thermal->sensors[i].tzd->tzp->no_hwmon = false; + error = thermal_add_hwmon_sysfs(thermal->sensors[i].tzd); + if (error) + dev_warn(&pdev->dev, + "failed to register sensor %d with hwmon: %d\n", + i, error); + } platform_set_drvdata(pdev, thermal); @@ -1344,6 +1353,7 @@ static int rockchip_thermal_remove(struct platform_device *pdev) for (i = 0; i < thermal->chip->chn_num; i++) { struct rockchip_thermal_sensor *sensor = &thermal->sensors[i]; + thermal_remove_hwmon_sysfs(sensor->tzd); rockchip_thermal_toggle_sensor(sensor, false); }
By default, of-based thermal drivers do not enable hwmon. Explicitly enable hwmon for both, the soc and gpu temperature sensor. Signed-off-by: Stefan Schaeckeler <schaecsn@gmx.net> --- drivers/thermal/rockchip_thermal.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) -- 2.24.0