On Fri, Jan 30, 2015 at 10:24:12PM +0100, Arnd Bergmann wrote: > On Thursday 29 January 2015 18:21:51 Eduardo Valentin wrote: > > Hello Arnd and Viresh, > > > > On Thu, Jan 29, 2015 at 01:42:32PM +0100, Arnd Bergmann wrote: > > > On Thursday 29 January 2015 15:40:15 Viresh Kumar wrote: > > > > obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o > > > > obj-$(CONFIG_UX500_SOC_DB8500) += dbx500-cpufreq.o > > > > -obj-$(CONFIG_ARM_EXYNOS_CPUFREQ) += exynos-cpufreq.o > > > > -obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ) += exynos4210-cpufreq.o > > > > -obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ) += exynos4x12-cpufreq.o > > > > -obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ) += exynos5250-cpufreq.o > > > > +obj-$(CONFIG_ARM_EXYNOS4210_CPUFREQ) += exynos-cpufreq.o exynos4210-cpufreq.o > > > > +obj-$(CONFIG_ARM_EXYNOS4X12_CPUFREQ) += exynos-cpufreq.o exynos4x12-cpufreq.o > > > > +obj-$(CONFIG_ARM_EXYNOS5250_CPUFREQ) += exynos-cpufreq.o exynos5250-cpufreq.o > > > > > > > > > > I'd have to try it, but this might fail if one of the three drivers > > > is built-in and another one is a module. > > > > > > Arnd > > > > Let me make one step back here. The original issue is, now this exynos > > cpufreq driver depends on of thermal; but of thermal can be built as > > module, while this cpufreq driver cannot. Original proposal is to allow > > module build in the exynos cpufreq driver. > > > > On the original proposal, my concern is that the driver code does not > > have separated modules, but one single module platform driver, which uses functions from > > other c files. On top of that, the patch originally allows four > > (independent) modules builds. Although the children drivers still > > selects the core part, we would still need to change the original patch > > to add module dependency too. > > > > So, my proposal was to, in order to allow module builds on this cpufreq > > driver, we also need to properly construct the driver into a single > > module, instead of several modules. The issue with my patch was the fact > > that it was allowing platforms that do not use that driver, to select it > > by default. And eventually this may cause a unusable module being loaded > > into those systems. > > > > Well, trying harder here in the same approach. The diff bellow is based > > on Arnd's original patch and on Viresh's amendment, except that the core > > part is now dependent on all the supported platforms, instead of > > ARCH_EXYNOS. This way, it wont load in platforms that are not supposed > > to be loaded. The user will be able to build the support for all > > platforms, or select which platforms he/she wants (as originally), > > except that now it can be a module, instead. > > > > I believe now by default it will still keep the driver only on those > > configs that expect it to be on. And it won't compile/load on platforms > > that it is not supposed to. It brings closer a config that is dependent on this > > driver, so it looks better in the menuconfig. > > > > Let me know if I missed something (feel free to amend to your patch): > > Yes, I think your refined approach works and is better than my > original patch, thanks a lot for giving it more thought! > > One tiny problem: > > > @@ -90,6 +84,20 @@ config ARM_EXYNOS_CPU_FREQ_BOOST_SW > > > > If in doubt, say N. > > > > +config ARM_EXYNOS5440_CPUFREQ > > + bool "SAMSUNG EXYNOS5440" > > + depends on SOC_EXYNOS5440 > > + depends on HAVE_CLK && OF > > + select PM_OPP > > + default y > > + help > > + This adds the CPUFreq driver for Samsung EXYNOS5440 > > + SoC. The nature of exynos5440 clock controller is > > + different than previous exynos controllers so not using > > + the common exynos framework. > > + > > + If in doubt, say N. > > I believe this one also has to be tristate, for the same reason. > I agree with you that it is better if we make it tristate. So, on my side, I have no concerns changing it to tristate. However, the exynos5440 cpufreq driver does not depend on of thermal as of today, and therefore, I did not touch this driver for this matter. Meaning, if it is not causing troubles, no need to mess with it. But I can add this change. No issues, on my side. > > > > -#ifdef CONFIG_ARM_EXYNOS4210_CPUFREQ > > +#if IS_ENABLED(CONFIG_ARM_EXYNOS4210_CPUFREQ) > > extern int exynos4210_cpufreq_init(struct exynos_dvfs_info *); > > #else > > static inline int exynos4210_cpufreq_init(struct exynos_dvfs_info *info) > > @@ -61,7 +61,7 @@ static inline int exynos4210_cpufreq_init(struct exynos_dvfs_info *info) > > return -EOPNOTSUPP; > > } > > #endif > > -#ifdef CONFIG_ARM_EXYNOS4X12_CPUFREQ > > +#if IS_ENABLED(CONFIG_ARM_EXYNOS4X12_CPUFREQ) > > extern int exynos4x12_cpufreq_init(struct exynos_dvfs_info *); > > #else > > static inline int exynos4x12_cpufreq_init(struct exynos_dvfs_info *info) > > @@ -69,7 +69,7 @@ static inline int exynos4x12_cpufreq_init(struct exynos_dvfs_info *info) > > return -EOPNOTSUPP; > > } > > #endif > > -#ifdef CONFIG_ARM_EXYNOS5250_CPUFREQ > > +#if IS_ENABLED(CONFIG_ARM_EXYNOS5250_CPUFREQ) > > extern int exynos5250_cpufreq_init(struct exynos_dvfs_info *); > > #else > > static inline int exynos5250_cpufreq_init(struct exynos_dvfs_info *info) > > This change is ok, but not needed, because the three extra symbols are still > bool. I would leave that change out, but I also don't mind it. Indeed. > > With the change to make ARM_EXYNOS5440_CPUFREQ tristate: > > Acked-by: Arnd Bergmann <arnd@xxxxxxxx> I will repost this patch with the minor amendments. Now, we only need an ack from cpufreq folks. BR, Eduardo Valentin
Attachment:
signature.asc
Description: Digital signature
