RE: [PATCH] thermal: imx: fix for dependency on cpu-freq

[Anson Huang]

Hi, Viresh

Best Regards!
Anson Huang

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@xxxxxxxxxx]
> Sent: 2018å11æ20æ 18:49
> To: Anson Huang <anson.huang@xxxxxxx>
> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>; Eduardo Valentin
> <edubezval@xxxxxxxxx>; Linux PM list <linux-pm@xxxxxxxxxxxxxxx>; Linux
> Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx>; dl-linux-imx
> <linux-imx@xxxxxxx>
> 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@xxxxxxx>
> 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.