On 10-01-19, 05:30, Amit Kucheria wrote: > All cpufreq drivers do similar things to register as a cooling device. > Provide a generic call back so we can get rid of duplicated code in the > drivers. > > Signed-off-by: Amit Kucheria <amit.kucheria@xxxxxxxxxx> > Suggested-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > --- > drivers/thermal/cpu_cooling.c | 18 ++++++++++++++++++ > include/linux/cpu_cooling.h | 9 +++++++++ > 2 files changed, 27 insertions(+) We may be doing this differently, but I found some other issues with the patch. > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index dfd23245f778..5154ffc12332 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -694,6 +694,7 @@ __cpufreq_cooling_register(struct device_node *np, > > cpufreq_cdev->clipped_freq = cpufreq_cdev->freq_table[0].frequency; > cpufreq_cdev->cdev = cdev; > + policy->cooldev = cdev; Why was this required ? The below routine was already doing it... > > mutex_lock(&cooling_list_lock); > /* Register the notifier for first cpufreq cooling device */ > @@ -785,6 +786,23 @@ of_cpufreq_cooling_register(struct cpufreq_policy *policy) > } > EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register); > > +/** > + * generic_cpufreq_ready - generic driver callback to register a thermal_cooling_device > + * @policy: cpufreq policy > + */ > +void generic_cpufreq_ready(struct cpufreq_policy *policy) > +{ > + struct thermal_cooling_device **cdev = &policy->cooldev; > + > + *cdev = of_cpufreq_cooling_register(policy); ... here > + if (IS_ERR(*cdev)) { We never get an error here, only NULL. > + pr_err("Failed to register cpufreq cooling device for CPU%d: %ld\n", > + policy->cpu, PTR_ERR(cdev)); The of_cpufreq_cooling_register() routine already prints enough error messages on failures. > + cdev = NULL; This is a meaningless statement, perhaps you wanted to do *cdev = NULL :) -- viresh