| Message ID | 1484833733-16082-2-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 19, 2017 at 02:48:41PM +0100, Marek Szyprowski wrote: > PMU is something like a SoC wide service, so add a helper function to get > PMU regmap. This will be used by other Exynos device drivers. This way it > can be avoided to model this dependency in device tree (as phandles to PMU > node) for almost every device in the SoC. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Reviewed-by: Tomasz Figa <tomasz.figa@gmail.com> > --- > drivers/soc/samsung/exynos-pmu.c | 11 +++++++++++ > include/linux/soc/samsung/exynos-pmu.h | 10 ++++++++++ > 2 files changed, 21 insertions(+) > Thanks, applied. I'll prepare a tag for pinctrl in a sec. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 19, 2017 at 2:48 PM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > PMU is something like a SoC wide service, so add a helper function to get > PMU regmap. This will be used by other Exynos device drivers. This way it > can be avoided to model this dependency in device tree (as phandles to PMU > node) for almost every device in the SoC. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Reviewed-by: Tomasz Figa <tomasz.figa@gmail.com> But why? I could understand it as a local header in drivers/soc/samsung/*.h for those files. For the global kernel scope we usually look up the syscon regmap we need using a phandle, DT node or even compatible string directly from the device tree. Just #include <linux/mfd/syscon.h> syscon_node_to_regmap() syscon_regmap_lookup_by_compatible() syscon_regmap_lookup_by_pdevname() syscon_regmap_lookup_by_phandle() what is wrong with just using one of these? Introducing a special header just creates compile time problems with global interfaces when merging code and just litter the kernel for no good reason. Can you explain why this necessarily different approach is needed? I guess I should have said this earlier, mea culpa. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On 2017-01-26 10:43, Linus Walleij wrote: > On Thu, Jan 19, 2017 at 2:48 PM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > >> PMU is something like a SoC wide service, so add a helper function to get >> PMU regmap. This will be used by other Exynos device drivers. This way it >> can be avoided to model this dependency in device tree (as phandles to PMU >> node) for almost every device in the SoC. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Reviewed-by: Tomasz Figa <tomasz.figa@gmail.com> > But why? I could understand it as a local header in > drivers/soc/samsung/*.h for those files. > > For the global kernel scope we usually look up the syscon > regmap we need using a phandle, DT node or even > compatible string directly from the device tree. > > Just > #include <linux/mfd/syscon.h> > > syscon_node_to_regmap() > syscon_regmap_lookup_by_compatible() > syscon_regmap_lookup_by_pdevname() > syscon_regmap_lookup_by_phandle() > > what is wrong with just using one of these? > > Introducing a special header just creates compile time problems > with global interfaces when merging code and just litter the kernel > for no good reason. > > Can you explain why this necessarily different approach is > needed? syscon_regmap_lookup_by_phandle() requires to have a phandle in the client node. This has been used in v0 of this patchset and rejected as breaking the old DT "compatibility". syscon_regmap_lookup_by_compatible() requires to have a compatible string hardcoded in the client driver. This might be used, but will require to have (and maintain) a duplicated list of PMU compatibles in pin control driver. There is a little problem with exynos4212 and exynos4412, which have same pinctrl compatible (samsung,exynos4x12-pinctrl), but separate PMU compatibles (samsung,exynos4212-pmu and samsung,exynos4412-pmu). Luckily support for Exynos4212 has been scheduled for removal, so we can ignore this issue for now. The problem with maintaining a duplicated list of compatibles remains. syscon_regmap_lookup_by_pdevname() requires the provider to register regmap first, then to hardcode the provider name in all client drivers. A bit similar approach was used in v1, but the drawback was runtime probe order dependency between PMU driver and pin control driver. I was requested to move the code for acquiring PMU regmap to PMU driver, what in turn ended in the solution presented in v2/v3 of this patchset, which requires least code for handling, has no maintenance overhead, and doesn't cause any probe dependency issues. It can be also later used for almost every driver found on the newer Exynos SoCs, because for most of them there is a need to poke some registers in the PMU not handled separately by any of the existing frameworks. Various other drivers in the drivers/soc/ provide EXPORT_SYMBOL() based API, so I don't think that this is a totally wrong approach. > I guess I should have said this earlier, mea culpa. Best regards
On Thu, Jan 26, 2017 at 11:43 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Jan 19, 2017 at 2:48 PM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > >> PMU is something like a SoC wide service, so add a helper function to get >> PMU regmap. This will be used by other Exynos device drivers. This way it >> can be avoided to model this dependency in device tree (as phandles to PMU >> node) for almost every device in the SoC. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Reviewed-by: Tomasz Figa <tomasz.figa@gmail.com> > > But why? I could understand it as a local header in > drivers/soc/samsung/*.h for those files. Marek gave quite detailed answer... so let me just add minor note. Apparently we cannot satisfy everyone. When break the DT ABI, DT people are not happy. When we try to avoid ABI break, we create such dependency. In fact, such compile and runtime dependency is not unusual. Our drivers are coupled because our hardware modules are coupled. When they were put in arch/arm/mach-exynos, no one cared because everything was contained in mach-exynos. We try to move the code out of there thus we need to model such dependencies in a new way... or break the DT ABI. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 26, 2017 at 11:22 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > syscon_regmap_lookup_by_compatible() requires to have a compatible string > hardcoded in the client driver. This might be used, but will require to have > (and maintain) a duplicated list of PMU compatibles in pin control driver. > There is a little problem with exynos4212 and exynos4412, which have same > pinctrl compatible (samsung,exynos4x12-pinctrl), but separate PMU > compatibles I actually used this approach with the ARM reference designs: drivers/mtd/maps/physmap_of_versatile.c drivers/video/fbdev/amba-clcd-versatile.c It made sense to me, because for each compatible I anyways needed the .data field to distinguish between the different system controllers, because they are of course all slightly idiomatic. I guess in your case you can distinguish how to use the syscon by the pinctrl compatible instead. Oh well. I guess I'm OK with this then. I'll try to pull it in and see what happens. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c index 0acdfd82e751..5c269bf23210 100644 --- a/drivers/soc/samsung/exynos-pmu.c +++ b/drivers/soc/samsung/exynos-pmu.c @@ -11,6 +11,7 @@ #include <linux/of.h> #include <linux/of_address.h> +#include <linux/mfd/syscon.h> #include <linux/platform_device.h> #include <linux/delay.h> @@ -92,6 +93,16 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode) { /*sentinel*/ }, }; +struct regmap *exynos_get_pmu_regmap(void) +{ + struct device_node *np = of_find_matching_node(NULL, + exynos_pmu_of_device_ids); + if (np) + return syscon_node_to_regmap(np); + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL_GPL(exynos_get_pmu_regmap); + static int exynos_pmu_probe(struct platform_device *pdev) { const struct of_device_id *match; diff --git a/include/linux/soc/samsung/exynos-pmu.h b/include/linux/soc/samsung/exynos-pmu.h index e2e9de1acc5b..e57eb4b6cc5a 100644 --- a/include/linux/soc/samsung/exynos-pmu.h +++ b/include/linux/soc/samsung/exynos-pmu.h @@ -12,6 +12,8 @@ #ifndef __LINUX_SOC_EXYNOS_PMU_H #define __LINUX_SOC_EXYNOS_PMU_H +struct regmap; + enum sys_powerdown { SYS_AFTR, SYS_LPA, @@ -20,5 +22,13 @@ enum sys_powerdown { }; extern void exynos_sys_powerdown_conf(enum sys_powerdown mode); +#ifdef CONFIG_EXYNOS_PMU +extern struct regmap *exynos_get_pmu_regmap(void); +#else +static inline struct regmap *exynos_get_pmu_regmap(void) +{ + return ERR_PTR(-ENODEV); +} +#endif #endif /* __LINUX_SOC_EXYNOS_PMU_H */