| Message ID | 1391518634-6472-1-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | Deferred |
| Headers | show |
On Tuesday, February 04, 2014 9:57 PM, Linus Walleij wrote: > > In some compilations the LM3630A and LP855X backlight drivers > fail like this: > > drivers/built-in.o: In function `lm3630a_pwm_ctrl': > drivers/video/backlight/lm3630a_bl.c:168: undefined reference to `pwm_config' > drivers/video/backlight/lm3630a_bl.c:172: undefined reference to `pwm_disable' > drivers/video/backlight/lm3630a_bl.c:170: undefined reference to `pwm_enable' > drivers/built-in.o: In function `lp855x_pwm_ctrl': > drivers/video/backlight/lp855x_bl.c:249: undefined reference to `pwm_config' > drivers/video/backlight/lp855x_bl.c:253: undefined reference to `pwm_disable' > drivers/video/backlight/lp855x_bl.c:251: undefined reference to `pwm_enable' > > This is because both drivers depend on the PWM framework, so > add this dependency to their Kconfig entries. However, even though, when CONFIG_PWM is not enabled, the problem should not happen. pwm_config(),pwm_disable(), and pwm_enable() are already defined for CONFIG_PWM=n case as below. ./include/linux/pwm.h #if IS_ENABLED(CONFIG_PWM) || IS_ENABLED(CONFIG_HAVE_PWM) ..... #else static inline struct pwm_device *pwm_request(int pwm_id, const char *label) { return ERR_PTR(-ENODEV); } static inline void pwm_free(struct pwm_device *pwm) { } static inline int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) { return -EINVAL; } static inline int pwm_enable(struct pwm_device *pwm) { return -EINVAL; } static inline void pwm_disable(struct pwm_device *pwm) { } #endif Is there 'drivers/pwm/core.o'? I reproduced this problem by commenting core.o with CONFIG_PWM=y. .config CONFIG_PWM=y ./drivers/pwm/Makefile @@ -1,4 +1,4 @@ -obj-$(CONFIG_PWM) += core.o +#obj-$(CONFIG_PWM) += core.o In this case, the following build errors happen. Even though, CONFIG_PWM is enabled, the same errors happen. Would you give me the more detailed information? Ex. how to reproduce the problem. drivers/built-in.o: In function `lm3630a_pwm_ctrl': drivers/video/backlight/lm3630a_bl.c:168: undefined reference to `pwm_config' drivers/video/backlight/lm3630a_bl.c:172: undefined reference to `pwm_disable' drivers/video/backlight/lm3630a_bl.c:170: undefined reference to `pwm_enable' drivers/built-in.o: In function `lm3630a_probe': drivers/video/backlight/lm3630a_bl.c:422: undefined reference to `devm_pwm_get' drivers/built-in.o: In function `lp855x_pwm_ctrl': drivers/video/backlight/lp855x_bl.c:249: undefined reference to `pwm_config' drivers/video/backlight/lp855x_bl.c:253: undefined reference to `pwm_disable' drivers/video/backlight/lp855x_bl.c:251: undefined reference to `pwm_enable' drivers/video/backlight/lp855x_bl.c:242: undefined reference to `devm_pwm_get' Thank you. Best regards, Jingoo Han > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/video/backlight/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig > index 5a3eb2ecb525..0604c3348761 100644 > --- a/drivers/video/backlight/Kconfig > +++ b/drivers/video/backlight/Kconfig > @@ -371,6 +371,7 @@ config BACKLIGHT_AAT2870 > config BACKLIGHT_LM3630A > tristate "Backlight Driver for LM3630A" > depends on BACKLIGHT_CLASS_DEVICE && I2C > + depends on PWM > select REGMAP_I2C > help > This supports TI LM3630A Backlight Driver > @@ -387,6 +388,7 @@ config BACKLIGHT_LM3639 > config BACKLIGHT_LP855X > tristate "Backlight driver for TI LP855X" > depends on BACKLIGHT_CLASS_DEVICE && I2C > + depends on PWM > help > This supports TI LP8550, LP8551, LP8552, LP8553, LP8555, LP8556 and > LP8557 backlight driver. > -- > 1.8.5.3 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 5, 2014 at 6:01 AM, Jingoo Han <jg1.han@samsung.com> wrote: > On Tuesday, February 04, 2014 9:57 PM, Linus Walleij wrote: >> >> In some compilations the LM3630A and LP855X backlight drivers >> fail like this: >> >> drivers/built-in.o: In function `lm3630a_pwm_ctrl': >> drivers/video/backlight/lm3630a_bl.c:168: undefined reference to `pwm_config' >> drivers/video/backlight/lm3630a_bl.c:172: undefined reference to `pwm_disable' >> drivers/video/backlight/lm3630a_bl.c:170: undefined reference to `pwm_enable' >> drivers/built-in.o: In function `lp855x_pwm_ctrl': >> drivers/video/backlight/lp855x_bl.c:249: undefined reference to `pwm_config' >> drivers/video/backlight/lp855x_bl.c:253: undefined reference to `pwm_disable' >> drivers/video/backlight/lp855x_bl.c:251: undefined reference to `pwm_enable' >> >> This is because both drivers depend on the PWM framework, so >> add this dependency to their Kconfig entries. > > However, even though, when CONFIG_PWM is not enabled, the problem > should not happen. pwm_config(),pwm_disable(), and pwm_enable() > are already defined for CONFIG_PWM=n case as below. So you may think but it does happen :-) I reproduced this with the defconfig for ARM pxa255-idp and enabling all boards for that platform, then enabling all available backlight drivers as compiled-in objects (y). > ./include/linux/pwm.h > #if IS_ENABLED(CONFIG_PWM) || IS_ENABLED(CONFIG_HAVE_PWM) > ..... > #else Hm PXA that I am using defines CONFIG_HAVE_PWM, but doesn't provide the required signatures (pwm_config/pwm_disable/pwm_enable). One of two things is wrong: - Either the PXA platform is breaking the CONFIG_HAVE_PWM contract by not providing pwm_config/pwm_disable/pwm_enable functions. Then HAVE_PWM should be removed from the PXA Kconfig selects. Or: - There is no such contract that these functions must exist if CONFIG_HAVE_PWM is defined, and the #if IS_ENABLED(CONFIG_HAVE_PWM) should be removed from <linux/pwm.h> Does anyone know which one it is? PWM subsystem maintainer? :-) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 05, 2014 5:58 PM, Linus Walleij wrote: > On Wed, Feb 5, 2014 at 6:01 AM, Jingoo Han <jg1.han@samsung.com> wrote: > > On Tuesday, February 04, 2014 9:57 PM, Linus Walleij wrote: > >> > >> In some compilations the LM3630A and LP855X backlight drivers > >> fail like this: > >> > >> drivers/built-in.o: In function `lm3630a_pwm_ctrl': > >> drivers/video/backlight/lm3630a_bl.c:168: undefined reference to `pwm_config' > >> drivers/video/backlight/lm3630a_bl.c:172: undefined reference to `pwm_disable' > >> drivers/video/backlight/lm3630a_bl.c:170: undefined reference to `pwm_enable' > >> drivers/built-in.o: In function `lp855x_pwm_ctrl': > >> drivers/video/backlight/lp855x_bl.c:249: undefined reference to `pwm_config' > >> drivers/video/backlight/lp855x_bl.c:253: undefined reference to `pwm_disable' > >> drivers/video/backlight/lp855x_bl.c:251: undefined reference to `pwm_enable' > >> > >> This is because both drivers depend on the PWM framework, so > >> add this dependency to their Kconfig entries. > > > > However, even though, when CONFIG_PWM is not enabled, the problem > > should not happen. pwm_config(),pwm_disable(), and pwm_enable() > > are already defined for CONFIG_PWM=n case as below. > > So you may think but it does happen :-) > > I reproduced this with the defconfig for ARM pxa255-idp and enabling > all boards for that platform, then enabling all available backlight drivers > as compiled-in objects (y). However, I cannot reproduce it with mainline kernel 3.14-rc1. 1. make pxa255-idp_defconfig 2. Enabling all boards (System Type -> Intel PXA2xx/PXA3xx Implementations -> ...) 3. Enabling all available backlight drivers as compiled-in objects (y) In this case, the LM3630A and LP855X backlight drivers are compiled properly as below: drivers/video/backlight/lm3630a_bl.o drivers/video/backlight/lp855x_bl.o Would you check it with mainline kernel 3.14-rc1? If the errors happen, please attach the .config file. Best regards, Jingoo Han > > > ./include/linux/pwm.h > > #if IS_ENABLED(CONFIG_PWM) || IS_ENABLED(CONFIG_HAVE_PWM) > > ..... > > #else > > Hm PXA that I am using defines CONFIG_HAVE_PWM, but doesn't > provide the required signatures (pwm_config/pwm_disable/pwm_enable). > > One of two things is wrong: > > - Either the PXA platform is breaking the CONFIG_HAVE_PWM > contract by not providing pwm_config/pwm_disable/pwm_enable > functions. Then HAVE_PWM should be removed from the PXA > Kconfig selects. > > Or: > > - There is no such contract that these functions must exist if > CONFIG_HAVE_PWM is defined, and the > #if IS_ENABLED(CONFIG_HAVE_PWM) > should be removed from <linux/pwm.h> > > Does anyone know which one it is? > > PWM subsystem maintainer? :-) > > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 04, 2014 at 01:57:14PM +0100, Linus Walleij wrote: > In some compilations the LM3630A and LP855X backlight drivers > fail like this: > > drivers/built-in.o: In function `lm3630a_pwm_ctrl': > drivers/video/backlight/lm3630a_bl.c:168: undefined reference to `pwm_config' > drivers/video/backlight/lm3630a_bl.c:172: undefined reference to `pwm_disable' > drivers/video/backlight/lm3630a_bl.c:170: undefined reference to `pwm_enable' > drivers/built-in.o: In function `lp855x_pwm_ctrl': > drivers/video/backlight/lp855x_bl.c:249: undefined reference to `pwm_config' > drivers/video/backlight/lp855x_bl.c:253: undefined reference to `pwm_disable' > drivers/video/backlight/lp855x_bl.c:251: undefined reference to `pwm_enable' > > This is because both drivers depend on the PWM framework, so > add this dependency to their Kconfig entries. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/video/backlight/Kconfig | 2 ++ > 1 file changed, 2 insertions(+) Hi Linus, it seems like at least BACKLIGHT_LP8788 is missing a corresponding dependency as well. I have applied Sascha's patch to remove the obsolete HAVE_PWM symbol, and this will fix at least the build issues. However it will also cause the driver to fail at runtime because the pwm_*() functions won't work. So I wonder if we should still apply this patch to make it clear that PWM support is necessary to make the driver work. I guess the point is somewhat moot because even if we had PWM enabled it could still happen that no PWM driver is enabled to provide a PWM device... I guess it's equally justifiable to leave that up to the defconfig. Should we just drop this patch? Cc'ing Arnd who's commented on Jingoo's alternate proposal. Thierry
On Mon, Feb 10, 2014 at 11:40 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > it seems like at least BACKLIGHT_LP8788 is missing a corresponding > dependency as well. > > I have applied Sascha's patch to remove the obsolete HAVE_PWM symbol, > and this will fix at least the build issues. However it will also cause > the driver to fail at runtime because the pwm_*() functions won't work. So it definately needs that API, not just stubs. But isn't it proper for Kconfig to allow you to break things like that by configuring out stuff and have stubs come in? I'm a bit torn here. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 10, 2014 at 12:09:29PM +0100, Linus Walleij wrote: > On Mon, Feb 10, 2014 at 11:40 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > > it seems like at least BACKLIGHT_LP8788 is missing a corresponding > > dependency as well. > > > > I have applied Sascha's patch to remove the obsolete HAVE_PWM symbol, > > and this will fix at least the build issues. However it will also cause > > the driver to fail at runtime because the pwm_*() functions won't work. > > So it definately needs that API, not just stubs. > > But isn't it proper for Kconfig to allow you to break things > like that by configuring out stuff and have stubs come in? > > I'm a bit torn here. After thinking about this some more, I've come to the conclusion that the drivers should have the dependency. Kconfig should make sure that the resulting drivers work. If somebody wanted to knowingly build this driver with a configuration that won't work at runtime, then they should be using COMPILE_TEST instead. So I'm leaning towards applying this patch if there are no objections. Thierry
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig index 5a3eb2ecb525..0604c3348761 100644 --- a/drivers/video/backlight/Kconfig +++ b/drivers/video/backlight/Kconfig @@ -371,6 +371,7 @@ config BACKLIGHT_AAT2870 config BACKLIGHT_LM3630A tristate "Backlight Driver for LM3630A" depends on BACKLIGHT_CLASS_DEVICE && I2C + depends on PWM select REGMAP_I2C help This supports TI LM3630A Backlight Driver @@ -387,6 +388,7 @@ config BACKLIGHT_LM3639 config BACKLIGHT_LP855X tristate "Backlight driver for TI LP855X" depends on BACKLIGHT_CLASS_DEVICE && I2C + depends on PWM help This supports TI LP8550, LP8551, LP8552, LP8553, LP8555, LP8556 and LP8557 backlight driver.
In some compilations the LM3630A and LP855X backlight drivers fail like this: drivers/built-in.o: In function `lm3630a_pwm_ctrl': drivers/video/backlight/lm3630a_bl.c:168: undefined reference to `pwm_config' drivers/video/backlight/lm3630a_bl.c:172: undefined reference to `pwm_disable' drivers/video/backlight/lm3630a_bl.c:170: undefined reference to `pwm_enable' drivers/built-in.o: In function `lp855x_pwm_ctrl': drivers/video/backlight/lp855x_bl.c:249: undefined reference to `pwm_config' drivers/video/backlight/lp855x_bl.c:253: undefined reference to `pwm_disable' drivers/video/backlight/lp855x_bl.c:251: undefined reference to `pwm_enable' This is because both drivers depend on the PWM framework, so add this dependency to their Kconfig entries. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/video/backlight/Kconfig | 2 ++ 1 file changed, 2 insertions(+)