Patchwork backlight: add PWM dependencies

login
register
mail settings
Submitter Linus Walleij
Date Feb. 4, 2014, 12:57 p.m.
Message ID <1391518634-6472-1-git-send-email-linus.walleij@linaro.org>
Download mbox | patch
Permalink /patch/316585/
State New
Headers show

Comments

Linus Walleij - Feb. 4, 2014, 12:57 p.m.
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(+)
Jingoo Han - Feb. 5, 2014, 5:01 a.m.
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
Linus Walleij - Feb. 5, 2014, 8:57 a.m.
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
Jingoo Han - Feb. 6, 2014, 6:49 a.m.
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
Thierry Reding - Feb. 10, 2014, 10:40 a.m.
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
Linus Walleij - Feb. 10, 2014, 11:09 a.m.
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
Thierry Reding - Feb. 26, 2014, 1:25 p.m.
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

Patch

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.