On Thursday, August 2, 2018 11:43:50 AM CEST Jonas Gorski wrote: > On 1 August 2018 at 22:44, Christian Lamparter <[email protected]> wrote: >> On Wednesday, August 1, 2018 3:21:00 PM CEST Jonas Gorski wrote: >>> On 30/07/18 22:33, Christian Lamparter wrote: >>>> >>>> This patch fixes the a compile issue that was triggered by >>>> apm821xx/sata when kmod-regmap was selected. >>>> >>>> The CONFIG_REGMAP is declared in drivers/base/regmap/Kconfig >>>> as type "bool" and not "tristate". Hence the symbol should >>>> never be set to module, as this confuses the #if CONFIG_REGMAP >>>> guards in include/linux/regmap.h: >>> This is actually the wrong fix and papers over an issue in one of our >>> local patches. >>> [...] >>> >>> We intentionally allow regmap to be built as a module, see >>> >>> https://github.com/openwrt/openwrt/blob/master/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >>> >>> So there are likely some additional EXPORT_SYMBOL_GPL()s required instead. >> Thanks for FYI, I missed this patch completely. >> >> I found that the issue lies in the upstream regulator core code. >> It is using these regmap's functions. And the CONFIG_REGULATOR symbol that >> pulls >> in the core's code is sadly a bool menuconfig option... so CONFIG_REGMAP >> pretty >> much becomes a requirement/select for the REGULATOR. > > This is one of the more trickier variants, it optionally supports > regmap thanks to the stubs provided if regmap is disabled - which > breaks if you compile regmap as a module. I expect this to happen > more in the future. >From what I can tell, this did already happend in the past as well.
https://github.com/openwrt/archive/commit/5e76bea8b4d45b87ea237bc19bcf8a2ac2ce9a42 Sadly, there's no explanation in the commit message. I guess it was a different time back then. Anyway, I can tell from my previous post, that the issue can fixed in the same way (i.e. adding CONFIG_REGMAP=y to the sata/config-default and a revert of the kmod-regmap patch). > This is a bit tricky to solve. We can't just generally stub out the > regmap stuff unless compiled in, as a lot of regulators depend on it. Right, it's becoming very tricky in OpenWrt patch context. In vanilla linux it's pretty straight forward, since it does not allow regmap to be a module. And I don't think upstream will be changing its stance on a regmap module. >From their POV "in upstream everything works as intended" and regmap is a critical part of many subsystems that "you want anyway (tm)". Which is sort of true too, but more to this later. > A proper fix is likely a bit more work (as usual). This begs the question if a proper fix for a hack makes sense or not. It's definitely easier to follow kirkwood target and just add the CONFIG_REGMAP=y to the config of apm821xx/sata and revert the kmod-regmap patch. >> diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> index 1c6e78df30..35803c3181 100644 >> --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch >> @@ -96,6 +97,15 @@ Signed-off-by: Felix Fietkau <[email protected]> >> postcore_initcall(regmap_initcall); >> + >> +MODULE_LICENSE("GPL"); >> +--- a/drivers/regulator/Kconfig >> ++++ b/drivers/regulator/Kconfig >> +@@ -1,5 +1,6 @@ >> + menuconfig REGULATOR >> + bool "Voltage and Current Regulator Support" >> ++ select REGMAP > > This will cause REGMAP to be included regardless if needed or not, but > would be acceptable (at least to me) as an interim build fix (most > targets using REGULATORs likely won't be part of the 4/32 gang > anyway). Yes exactly that is the idea. I did make that patch with ar71xx + tiny and ramips rt288x/rt305x in mind. The situation is a little bit different with ath79-tiny as its config-4.14 already sets: CONFIG_REGULATOR=y CONFIG_REGULATOR_FIXED_VOLTAGE=y And it inherits the CONFIG_REGMAP=y from the main ath79 config. If the REGMAP=y is only needed there because of the REGULATOR_FIXED_VOLTAGE (which does not need it), then it could make sense to extend the hack for this special case. In order to keep the kernel image as small as possible. (see below) > The correct fix is likely to > > a) add this REGMAP select to all REGULATOR drivers depending on/using > it (this can probably even be upstreamed) >From looking at the upstream Kconfigs, I think this is pretty much already the case. The drivers for the i2c jellybean regulator parts select REGMAP_I2C. The remaining regulators drivers are usually part of a SoC and have a indirect dependency/select to another symbol that selects REGMAP (these are mostly syscon/mfd symbols and CONFIG_SYSCON pulls in REGMAP_MMIO). So I guess this will only become a problem is regulator kmods are added. > b) (OpenWrt only) Change the usage in regulator/core.c and > regulator/helper.c to work like REGMAP is disabled if it is > build as a module. --- diff --git a/package/kernel/linux/modules/other.mk b/package/kernel/linux/modules/other.mk index dd037cf86c..7e18a21db3 100644 --- a/package/kernel/linux/modules/other.mk +++ b/package/kernel/linux/modules/other.mk @@ -718,7 +718,7 @@ define KernelPackage/regmap SUBMENU:=$(OTHER_MENU) TITLE:=Generic register map support DEPENDS:=+kmod-lib-lzo +kmod-i2c-core - KCONFIG:=CONFIG_REGMAP=y \ + KCONFIG:=CONFIG_REGMAP \ CONFIG_REGMAP_MMIO \ CONFIG_REGMAP_SPI \ CONFIG_REGMAP_I2C \ diff --git a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch index 1c6e78df30..458b7c35a1 100644 --- a/target/linux/generic/hack-4.14/259-regmap_dynamic.patch +++ b/target/linux/generic/hack-4.14/259-regmap_dynamic.patch @@ -103,7 +103,7 @@ Signed-off-by: Felix Fietkau <[email protected]> }) -#ifdef CONFIG_REGMAP -+#if IS_ENABLED(CONFIG_REGMAP) ++#if IS_REACHABLE(CONFIG_REGMAP) enum regmap_endian { /* Unspecified -> 0 -> Backwards compatible default */ --- I think this should do just that. To quote IS_REACHABLE help text:" IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled code can call a function defined in code compiled based on CONFIG_FOO. This is similar to IS_ENABLED(), but returns false when invoked from built-in code when CONFIG_FOO is set to 'm'." and it should combine the best of both worlds. (And maybe make it possible to disable the REGMAP=y from ath79-tiny. Any testers?) > b) is to allow building REGMAP as =m even when REGULATOR is enabled, > if no REGMAP requiring REGULATOR driver is enabled. I was more thinking about detangling just the REGULATOR_FIXED_VOLTAGE driver from the REGULATOR framework. So it can be built without the regulator core and regmap. I did play around with #ifdefing but nah, this turns out to become a much bigger fragile hack, that nobody is going to maintain. Regards, Christian Regards, Christian _______________________________________________ openwrt-devel mailing list [email protected] https://lists.openwrt.org/mailman/listinfo/openwrt-devel
