* [PATCH] thermal: imx: fix for dependency on cpu-freq
@ 2018-10-24 6:39 Anson Huang
2018-11-20 7:04 ` Anson Huang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Anson Huang @ 2018-10-24 6:39 UTC (permalink / raw)
To: rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
The thermal driver is a standalone driver for monitoring SoC temperature
by enabling thermal sensor, so it can be enabled even when CONFIG_CPU_FREQ
is NOT set. So remove the dependency with CPU_THERMAL.
Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal
driver to make thermal driver probe successfully when CONFIG_CPU_FREQ
is NOT set.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/thermal/Kconfig | 2 +-
drivers/thermal/imx_thermal.c | 4 ++++
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 1775d44..c8a6352 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -212,7 +212,7 @@ config HISI_THERMAL
config IMX_THERMAL
tristate "Temperature sensor driver for Freescale i.MX SoCs"
- depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
+ depends on ARCH_MXC || COMPILE_TEST
depends on NVMEM || !NVMEM
depends on MFD_SYSCON
depends on OF
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 1566154..87386d1 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -648,6 +648,7 @@ static const struct of_device_id of_imx_thermal_match[] = {
};
MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
+#ifdef CONFIG_CPU_FREQ
/*
* Create cooling device in case no #cooling-cells property is available in
* CPU node
@@ -668,6 +669,7 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
return 0;
}
+#endif
static int imx_thermal_probe(struct platform_device *pdev)
{
@@ -743,6 +745,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
data->socdata->power_down_mask);
+#ifdef CONFIG_CPU_FREQ
data->policy = cpufreq_cpu_get(0);
if (!data->policy) {
pr_debug("%s: CPUFreq policy not found\n", __func__);
@@ -755,6 +758,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
"failed to register cpufreq cooling device: %d\n", ret);
return ret;
}
+#endif
data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(data->thermal_clk)) {
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* RE: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-10-24 6:39 [PATCH] thermal: imx: fix for dependency on cpu-freq Anson Huang
@ 2018-11-20 7:04 ` Anson Huang
2018-11-20 8:44 ` Daniel Lezcano
2018-11-20 10:48 ` Viresh Kumar
2 siblings, 0 replies; 10+ messages in thread
From: Anson Huang @ 2018-11-20 7:04 UTC (permalink / raw)
To: rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
Gentle Ping...
Best Regards!
Anson Huang
> -----Original Message-----
> From: Anson Huang
> Sent: 2018年10月24日 14:40
> To: rui.zhang@intel.com; edubezval@gmail.com; linux-pm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH] thermal: imx: fix for dependency on cpu-freq
>
> The thermal driver is a standalone driver for monitoring SoC temperature by
> enabling thermal sensor, so it can be enabled even when CONFIG_CPU_FREQ is
> NOT set. So remove the dependency with CPU_THERMAL.
>
> Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal driver
> to make thermal driver probe successfully when CONFIG_CPU_FREQ is NOT
> set.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/thermal/Kconfig | 2 +-
> drivers/thermal/imx_thermal.c | 4 ++++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> 1775d44..c8a6352 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -212,7 +212,7 @@ config HISI_THERMAL
>
> config IMX_THERMAL
> tristate "Temperature sensor driver for Freescale i.MX SoCs"
> - depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
> + depends on ARCH_MXC || COMPILE_TEST
> depends on NVMEM || !NVMEM
> depends on MFD_SYSCON
> depends on OF
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 1566154..87386d1 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -648,6 +648,7 @@ static const struct of_device_id
> of_imx_thermal_match[] = { }; MODULE_DEVICE_TABLE(of,
> of_imx_thermal_match);
>
> +#ifdef CONFIG_CPU_FREQ
> /*
> * Create cooling device in case no #cooling-cells property is available in
> * CPU node
> @@ -668,6 +669,7 @@ static int imx_thermal_register_legacy_cooling(struct
> imx_thermal_data *data)
>
> return 0;
> }
> +#endif
>
> static int imx_thermal_probe(struct platform_device *pdev) { @@ -743,6
> +745,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
> data->socdata->power_down_mask);
>
> +#ifdef CONFIG_CPU_FREQ
> data->policy = cpufreq_cpu_get(0);
> if (!data->policy) {
> pr_debug("%s: CPUFreq policy not found\n", __func__); @@ -755,6
> +758,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> "failed to register cpufreq cooling device: %d\n", ret);
> return ret;
> }
> +#endif
>
> data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(data->thermal_clk)) {
> --
> 2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-10-24 6:39 [PATCH] thermal: imx: fix for dependency on cpu-freq Anson Huang
2018-11-20 7:04 ` Anson Huang
@ 2018-11-20 8:44 ` Daniel Lezcano
2018-11-20 8:47 ` Anson Huang
2018-11-20 10:48 ` Viresh Kumar
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2018-11-20 8:44 UTC (permalink / raw)
To: Anson Huang, rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
On 24/10/2018 08:39, Anson Huang wrote:
> The thermal driver is a standalone driver for monitoring SoC temperature
> by enabling thermal sensor, so it can be enabled even when CONFIG_CPU_FREQ
> is NOT set. So remove the dependency with CPU_THERMAL.
>
> Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal
> driver to make thermal driver probe successfully when CONFIG_CPU_FREQ
> is NOT set.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
Why not simply kill this legacy code ?
> drivers/thermal/Kconfig | 2 +-
> drivers/thermal/imx_thermal.c | 4 ++++
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 1775d44..c8a6352 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -212,7 +212,7 @@ config HISI_THERMAL
>
> config IMX_THERMAL
> tristate "Temperature sensor driver for Freescale i.MX SoCs"
> - depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
> + depends on ARCH_MXC || COMPILE_TEST
> depends on NVMEM || !NVMEM
> depends on MFD_SYSCON
> depends on OF
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index 1566154..87386d1 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -648,6 +648,7 @@ static const struct of_device_id of_imx_thermal_match[] = {
> };
> MODULE_DEVICE_TABLE(of, of_imx_thermal_match);
>
> +#ifdef CONFIG_CPU_FREQ
> /*
> * Create cooling device in case no #cooling-cells property is available in
> * CPU node
> @@ -668,6 +669,7 @@ static int imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
>
> return 0;
> }
> +#endif
>
> static int imx_thermal_probe(struct platform_device *pdev)
> {
> @@ -743,6 +745,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
> data->socdata->power_down_mask);
>
> +#ifdef CONFIG_CPU_FREQ
> data->policy = cpufreq_cpu_get(0);
> if (!data->policy) {
> pr_debug("%s: CPUFreq policy not found\n", __func__);
> @@ -755,6 +758,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> "failed to register cpufreq cooling device: %d\n", ret);
> return ret;
> }
> +#endif
>
> data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(data->thermal_clk)) {
>
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-11-20 8:44 ` Daniel Lezcano
@ 2018-11-20 8:47 ` Anson Huang
2018-11-20 8:54 ` Daniel Lezcano
0 siblings, 1 reply; 10+ messages in thread
From: Anson Huang @ 2018-11-20 8:47 UTC (permalink / raw)
To: Daniel Lezcano, rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
Hi, Daniel
Best Regards!
Anson Huang
> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: 2018年11月20日 16:45
> To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
> edubezval@gmail.com; linux-pm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
>
> On 24/10/2018 08:39, Anson Huang wrote:
> > The thermal driver is a standalone driver for monitoring SoC
> > temperature by enabling thermal sensor, so it can be enabled even when
> > CONFIG_CPU_FREQ is NOT set. So remove the dependency with
> CPU_THERMAL.
> >
> > Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal
> > driver to make thermal driver probe successfully when CONFIG_CPU_FREQ
> > is NOT set.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
>
> Why not simply kill this legacy code ?
Because killing legacy code will have old dtb compatible issue, old dtb
will NOT have cpufreq cooling function.
Anson.
>
>
>
> > drivers/thermal/Kconfig | 2 +-
> > drivers/thermal/imx_thermal.c | 4 ++++
> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > 1775d44..c8a6352 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -212,7 +212,7 @@ config HISI_THERMAL
> >
> > config IMX_THERMAL
> > tristate "Temperature sensor driver for Freescale i.MX SoCs"
> > - depends on (ARCH_MXC && CPU_THERMAL) || COMPILE_TEST
> > + depends on ARCH_MXC || COMPILE_TEST
> > depends on NVMEM || !NVMEM
> > depends on MFD_SYSCON
> > depends on OF
> > diff --git a/drivers/thermal/imx_thermal.c
> > b/drivers/thermal/imx_thermal.c index 1566154..87386d1 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -648,6 +648,7 @@ static const struct of_device_id
> > of_imx_thermal_match[] = { }; MODULE_DEVICE_TABLE(of,
> > of_imx_thermal_match);
> >
> > +#ifdef CONFIG_CPU_FREQ
> > /*
> > * Create cooling device in case no #cooling-cells property is available in
> > * CPU node
> > @@ -668,6 +669,7 @@ static int
> > imx_thermal_register_legacy_cooling(struct imx_thermal_data *data)
> >
> > return 0;
> > }
> > +#endif
> >
> > static int imx_thermal_probe(struct platform_device *pdev) { @@
> > -743,6 +745,7 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
> > regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
> > data->socdata->power_down_mask);
> >
> > +#ifdef CONFIG_CPU_FREQ
> > data->policy = cpufreq_cpu_get(0);
> > if (!data->policy) {
> > pr_debug("%s: CPUFreq policy not found\n", __func__); @@ -755,6
> > +758,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> > "failed to register cpufreq cooling device: %d\n", ret);
> > return ret;
> > }
> > +#endif
> >
> > data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(data->thermal_clk)) {
> >
>
>
> --
>
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .linaro.org%2F&data=02%7C01%7Canson.huang%40nxp.com%7C6d1060
> ba33e24e37bf5608d64ec46651%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636783002767430588&sdata=EO0xxkmPjZTReBzJ%2F%2FJj
> x%2FblahtmYxHHogDxw5u%2Fbjo%3D&reserved=0> Linaro.org │ Open
> source software for ARM SoCs
>
> Follow Linaro:
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .facebook.com%2Fpages%2FLinaro&data=02%7C01%7Canson.huang%40
> nxp.com%7C6d1060ba33e24e37bf5608d64ec46651%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C636783002767430588&sdata=gp3BlLl
> SXe6PGnpp93K%2B0n03XInnRscXERGl4T2d0Sc%3D&reserved=0>
> Facebook |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
> er.com%2F%23!%2Flinaroorg&data=02%7C01%7Canson.huang%40nxp.c
> om%7C6d1060ba33e24e37bf5608d64ec46651%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C636783002767430588&sdata=oGXtzLBie1Zy
> 419jAh9Lc3mw7Vu2KsCgaWYd4s2tujY%3D&reserved=0> Twitter |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .linaro.org%2Flinaro-blog%2F&data=02%7C01%7Canson.huang%40nxp.c
> om%7C6d1060ba33e24e37bf5608d64ec46651%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C636783002767430588&sdata=AbA1RofuJWY
> un%2BKZvLsvp6Hnkbn%2F1VqjCfiEQsDW%2Bc0%3D&reserved=0> Blog
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-11-20 8:47 ` Anson Huang
@ 2018-11-20 8:54 ` Daniel Lezcano
2018-11-20 8:58 ` Anson Huang
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2018-11-20 8:54 UTC (permalink / raw)
To: Anson Huang, rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
On 20/11/2018 09:47, Anson Huang wrote:
> Hi, Daniel
>
> Best Regards!
> Anson Huang
>
>> -----Original Message-----
>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>> Sent: 2018年11月20日 16:45
>> To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
>> edubezval@gmail.com; linux-pm@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
>>
>> On 24/10/2018 08:39, Anson Huang wrote:
>>> The thermal driver is a standalone driver for monitoring SoC
>>> temperature by enabling thermal sensor, so it can be enabled even when
>>> CONFIG_CPU_FREQ is NOT set. So remove the dependency with
>> CPU_THERMAL.
>>>
>>> Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal
>>> driver to make thermal driver probe successfully when CONFIG_CPU_FREQ
>>> is NOT set.
>>>
>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>> ---
>>
>> Why not simply kill this legacy code ?
>
> Because killing legacy code will have old dtb compatible issue, old dtb
> will NOT have cpufreq cooling function.
Yeah, I imagine that is the reason why you want to keep the legacy code
but do you really care about old DTB based boards? Are they still
updated with newer *upstream vanilla* kernels?
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-11-20 8:54 ` Daniel Lezcano
@ 2018-11-20 8:58 ` Anson Huang
2018-11-20 9:29 ` Daniel Lezcano
0 siblings, 1 reply; 10+ messages in thread
From: Anson Huang @ 2018-11-20 8:58 UTC (permalink / raw)
To: Daniel Lezcano, rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
Hi, Daniel
Best Regards!
Anson Huang
> -----Original Message-----
> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> Sent: 2018年11月20日 16:54
> To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
> edubezval@gmail.com; linux-pm@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
>
> On 20/11/2018 09:47, Anson Huang wrote:
> > Hi, Daniel
> >
> > Best Regards!
> > Anson Huang
> >
> >> -----Original Message-----
> >> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> >> Sent: 2018年11月20日 16:45
> >> To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
> >> edubezval@gmail.com; linux-pm@vger.kernel.org;
> >> linux-kernel@vger.kernel.org
> >> Cc: dl-linux-imx <linux-imx@nxp.com>
> >> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
> >>
> >> On 24/10/2018 08:39, Anson Huang wrote:
> >>> The thermal driver is a standalone driver for monitoring SoC
> >>> temperature by enabling thermal sensor, so it can be enabled even
> >>> when CONFIG_CPU_FREQ is NOT set. So remove the dependency with
> >> CPU_THERMAL.
> >>>
> >>> Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal
> >>> driver to make thermal driver probe successfully when
> >>> CONFIG_CPU_FREQ is NOT set.
> >>>
> >>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >>> ---
> >>
> >> Why not simply kill this legacy code ?
> >
> > Because killing legacy code will have old dtb compatible issue, old
> > dtb will NOT have cpufreq cooling function.
>
> Yeah, I imagine that is the reason why you want to keep the legacy code but do
> you really care about old DTB based boards? Are they still updated with newer
> *upstream vanilla* kernels?
I am NOT sure if there is someone care about it, but I did receive many comments
about old dtb compatible when I sent out other patches, so is it a solid requirement
of old dtb compatible when doing upstream, or each sub-system or maintainer has
different requirement about it? Actually I am happy to just remove the legacy
code, because it makes the code more clean and easy reading. Who can make the
decision?
Anson.
>
>
> --
>
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .linaro.org%2F&data=02%7C01%7Canson.huang%40nxp.com%7Cbb8cdd
> 4155fb4c02dab308d64ec5c9cd%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636783008739373537&sdata=x6qHDTFYb3SNARICs15KkLL7
> %2Fpp7enYZkZJHRoJksXs%3D&reserved=0> Linaro.org │ Open source
> software for ARM SoCs
>
> Follow Linaro:
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .facebook.com%2Fpages%2FLinaro&data=02%7C01%7Canson.huang%40
> nxp.com%7Cbb8cdd4155fb4c02dab308d64ec5c9cd%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C636783008739373537&sdata=JJ4H9Z0
> RAY%2F1uLlcKXNQN36L0ApFIwM9%2FuPOU9UBUcI%3D&reserved=0>
> Facebook |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitt
> er.com%2F%23!%2Flinaroorg&data=02%7C01%7Canson.huang%40nxp.c
> om%7Cbb8cdd4155fb4c02dab308d64ec5c9cd%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C636783008739373537&sdata=o0o7h8GYALt
> o6NuuI%2BAxFzx6rcr3VFg6CWwh3feGggI%3D&reserved=0> Twitter |
> <https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww
> .linaro.org%2Flinaro-blog%2F&data=02%7C01%7Canson.huang%40nxp.c
> om%7Cbb8cdd4155fb4c02dab308d64ec5c9cd%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C636783008739373537&sdata=6r2baB5ZimC
> zJEkrKTvCBH98%2BBlnw0ZHiYOdG5JlimA%3D&reserved=0> Blog
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-11-20 8:58 ` Anson Huang
@ 2018-11-20 9:29 ` Daniel Lezcano
2018-11-20 10:12 ` Lucas Stach
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2018-11-20 9:29 UTC (permalink / raw)
To: Anson Huang, rui.zhang, edubezval, linux-pm, linux-kernel; +Cc: dl-linux-imx
On 20/11/2018 09:58, Anson Huang wrote:
> Hi, Daniel
>
> Best Regards!
> Anson Huang
>
>> -----Original Message-----
>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>> Sent: 2018年11月20日 16:54
>> To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
>> edubezval@gmail.com; linux-pm@vger.kernel.org;
>> linux-kernel@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
>>
>> On 20/11/2018 09:47, Anson Huang wrote:
>>> Hi, Daniel
>>>
>>> Best Regards!
>>> Anson Huang
>>>
>>>> -----Original Message-----
>>>> From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
>>>> Sent: 2018年11月20日 16:45
>>>> To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
>>>> edubezval@gmail.com; linux-pm@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org
>>>> Cc: dl-linux-imx <linux-imx@nxp.com>
>>>> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
>>>>
>>>> On 24/10/2018 08:39, Anson Huang wrote:
>>>>> The thermal driver is a standalone driver for monitoring SoC
>>>>> temperature by enabling thermal sensor, so it can be enabled even
>>>>> when CONFIG_CPU_FREQ is NOT set. So remove the dependency with
>>>> CPU_THERMAL.
>>>>>
>>>>> Add CONFIG_CPU_FREQ check for cpu-freq related operation in thermal
>>>>> driver to make thermal driver probe successfully when
>>>>> CONFIG_CPU_FREQ is NOT set.
>>>>>
>>>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>>>> ---
>>>>
>>>> Why not simply kill this legacy code ?
>>>
>>> Because killing legacy code will have old dtb compatible issue, old
>>> dtb will NOT have cpufreq cooling function.
>>
>> Yeah, I imagine that is the reason why you want to keep the legacy code but do
>> you really care about old DTB based boards? Are they still updated with newer
>> *upstream vanilla* kernels?
>
> I am NOT sure if there is someone care about it, but I did receive many comments
> about old dtb compatible when I sent out other patches, so is it a solid requirement
> of old dtb compatible when doing upstream, or each sub-system or maintainer has
> different requirement about it? Actually I am happy to just remove the legacy
> code, because it makes the code more clean and easy reading. Who can make the
> decision?
Yes, making sure to not break the compatibility makes the patch
submission easier. However, sometime it makes sense to put in question
if keeping old (and hackish) code really matters.
Old boards are rarely updated with newer kernels and when that happens,
usually the DT is updated also.
IMO, this decision is in the hands of the platform maintainers. I
suggest to send a patch removing the legacy code Cc'ing all of them.
--
<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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-11-20 9:29 ` Daniel Lezcano
@ 2018-11-20 10:12 ` Lucas Stach
0 siblings, 0 replies; 10+ messages in thread
From: Lucas Stach @ 2018-11-20 10:12 UTC (permalink / raw)
To: Daniel Lezcano, Anson Huang, rui.zhang, edubezval, linux-pm,
linux-kernel
Cc: dl-linux-imx
Am Dienstag, den 20.11.2018, 10:29 +0100 schrieb Daniel Lezcano:
> On 20/11/2018 09:58, Anson Huang wrote:
> > Hi, Daniel
> >
> > Best Regards!
> > Anson Huang
> >
> > > -----Original Message-----
> > > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> > > Sent: 2018年11月20日 16:54
> > > To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
> > > edubezval@gmail.com; linux-pm@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Cc: dl-linux-imx <linux-imx@nxp.com>
> > > Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
> > >
> > > On 20/11/2018 09:47, Anson Huang wrote:
> > > > Hi, Daniel
> > > >
> > > > Best Regards!
> > > > Anson Huang
> > > >
> > > > > -----Original Message-----
> > > > > From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org]
> > > > > Sent: 2018年11月20日 16:45
> > > > > To: Anson Huang <anson.huang@nxp.com>; rui.zhang@intel.com;
> > > > > edubezval@gmail.com; linux-pm@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org
> > > > > Cc: dl-linux-imx <linux-imx@nxp.com>
> > > > > Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-
> > > > > freq
> > > > >
> > > > > On 24/10/2018 08:39, Anson Huang wrote:
> > > > > > The thermal driver is a standalone driver for monitoring
> > > > > > SoC
> > > > > > temperature by enabling thermal sensor, so it can be
> > > > > > enabled even
> > > > > > when CONFIG_CPU_FREQ is NOT set. So remove the dependency
> > > > > > with
> > > > >
> > > > > CPU_THERMAL.
> > > > > >
> > > > > > Add CONFIG_CPU_FREQ check for cpu-freq related operation in
> > > > > > thermal
> > > > > > driver to make thermal driver probe successfully when
> > > > > > CONFIG_CPU_FREQ is NOT set.
> > > > > >
> > > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > > > ---
> > > > >
> > > > > Why not simply kill this legacy code ?
> > > >
> > > > Because killing legacy code will have old dtb compatible issue,
> > > > old
> > > > dtb will NOT have cpufreq cooling function.
> > >
> > > Yeah, I imagine that is the reason why you want to keep the
> > > legacy code but do
> > > you really care about old DTB based boards? Are they still
> > > updated with newer
> > > *upstream vanilla* kernels?
> >
> > I am NOT sure if there is someone care about it, but I did receive
> > many comments
> > about old dtb compatible when I sent out other patches, so is it a
> > solid requirement
> > of old dtb compatible when doing upstream, or each sub-system or
> > maintainer has
> > different requirement about it? Actually I am happy to just remove
> > the legacy
> > code, because it makes the code more clean and easy reading. Who
> > can make the
> > decision?
>
> Yes, making sure to not break the compatibility makes the patch
> submission easier. However, sometime it makes sense to put in
> question
> if keeping old (and hackish) code really matters.
>
> Old boards are rarely updated with newer kernels and when that
> happens,
> usually the DT is updated also.
>
> IMO, this decision is in the hands of the platform maintainers. I
> suggest to send a patch removing the legacy code Cc'ing all of them.
On i.MX we usually try to keep DT compatibility as much as possible.
There are cases where keeping the compatibility is just too much of a
burden on maintenance, but IMHO this is not the case for the piece of
code in question here.
Regards,
Lucas
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-10-24 6:39 [PATCH] thermal: imx: fix for dependency on cpu-freq Anson Huang
2018-11-20 7:04 ` Anson Huang
2018-11-20 8:44 ` Daniel Lezcano
@ 2018-11-20 10:48 ` Viresh Kumar
2018-11-21 1:58 ` Anson Huang
2 siblings, 1 reply; 10+ messages in thread
From: Viresh Kumar @ 2018-11-20 10:48 UTC (permalink / raw)
To: anson.huang
Cc: Zhang Rui, Eduardo Valentin, Linux PM list,
Linux Kernel Mailing List, linux-imx
While I am aligned with the fact that we need to carry this code for backward
compatibility, there are few things I would suggest to improve.
On Wed, Oct 24, 2018 at 12:10 PM Anson Huang <anson.huang@nxp.com> wrote:
> static int imx_thermal_probe(struct platform_device *pdev)
> {
> @@ -743,6 +745,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
> data->socdata->power_down_mask);
>
> +#ifdef CONFIG_CPU_FREQ
> data->policy = cpufreq_cpu_get(0);
> if (!data->policy) {
> pr_debug("%s: CPUFreq policy not found\n", __func__);
> @@ -755,6 +758,7 @@ static int imx_thermal_probe(struct platform_device *pdev)
> "failed to register cpufreq cooling device: %d\n", ret);
> return ret;
> }
> +#endif
>
> data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(data->thermal_clk)) {
You missed the error handling code which unregisters cooling/cpufreq stuff.
And it would be better to write things in a somewhat better way, like this:
#ifdef CONFIG_CPU_FREQ
static int imx_thermal_register_legacy_cooling(...)
{
... current function body
}
static void imx_thermal_unregister_legacy_cooling(...)
{
new routine body to unregister things
}
#else
static inline int imx_thermal_register_legacy_cooling(...)
{
return 0;
}
static void imx_thermal_unregister_legacy_cooling(...) { }
#endif
And then you can get rid of ifdef hackery in the middle of probe().
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [PATCH] thermal: imx: fix for dependency on cpu-freq
2018-11-20 10:48 ` Viresh Kumar
@ 2018-11-21 1:58 ` Anson Huang
0 siblings, 0 replies; 10+ messages in thread
From: Anson Huang @ 2018-11-21 1:58 UTC (permalink / raw)
To: Viresh Kumar
Cc: Zhang Rui, Eduardo Valentin, Linux PM list,
Linux Kernel Mailing List, dl-linux-imx
Hi, Viresh
Best Regards!
Anson Huang
> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: 2018年11月20日 18:49
> To: Anson Huang <anson.huang@nxp.com>
> Cc: Zhang Rui <rui.zhang@intel.com>; Eduardo Valentin
> <edubezval@gmail.com>; Linux PM list <linux-pm@vger.kernel.org>; Linux
> Kernel Mailing List <linux-kernel@vger.kernel.org>; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH] thermal: imx: fix for dependency on cpu-freq
>
> While I am aligned with the fact that we need to carry this code for backward
> compatibility, there are few things I would suggest to improve.
>
> On Wed, Oct 24, 2018 at 12:10 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > static int imx_thermal_probe(struct platform_device *pdev) { @@
> > -743,6 +745,7 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
> > regmap_write(map, data->socdata->sensor_ctrl + REG_SET,
> > data->socdata->power_down_mask);
> >
> > +#ifdef CONFIG_CPU_FREQ
> > data->policy = cpufreq_cpu_get(0);
> > if (!data->policy) {
> > pr_debug("%s: CPUFreq policy not found\n", __func__);
> > @@ -755,6 +758,7 @@ static int imx_thermal_probe(struct platform_device
> *pdev)
> > "failed to register cpufreq cooling device: %d\n",
> ret);
> > return ret;
> > }
> > +#endif
> >
> > data->thermal_clk = devm_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(data->thermal_clk)) {
>
> You missed the error handling code which unregisters cooling/cpufreq stuff.
>
> And it would be better to write things in a somewhat better way, like this:
>
> #ifdef CONFIG_CPU_FREQ
>
> static int imx_thermal_register_legacy_cooling(...)
> {
> ... current function body
> }
>
> static void imx_thermal_unregister_legacy_cooling(...)
> {
> new routine body to unregister things }
>
> #else
> static inline int imx_thermal_register_legacy_cooling(...)
> {
> return 0;
> }
>
> static void imx_thermal_unregister_legacy_cooling(...) { }
>
> #endif
>
>
> And then you can get rid of ifdef hackery in the middle of probe().
Thanks for good suggestion, please help review the V2 patch I just sent out.
Anson.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-11-21 1:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-24 6:39 [PATCH] thermal: imx: fix for dependency on cpu-freq Anson Huang
2018-11-20 7:04 ` Anson Huang
2018-11-20 8:44 ` Daniel Lezcano
2018-11-20 8:47 ` Anson Huang
2018-11-20 8:54 ` Daniel Lezcano
2018-11-20 8:58 ` Anson Huang
2018-11-20 9:29 ` Daniel Lezcano
2018-11-20 10:12 ` Lucas Stach
2018-11-20 10:48 ` Viresh Kumar
2018-11-21 1:58 ` Anson Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).