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. > > -#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. With the change to make ARM_EXYNOS5440_CPUFREQ tristate: Acked-by: Arnd Bergmann <arnd@xxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |