[PATCH] backlight: add PWM dependencies

[PATCH] backlight: add PWM dependencies

[Linus Walleij]

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(+)

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

Re: [PATCH] backlight: add PWM dependencies

[Jingoo Han]

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

Re: [PATCH] backlight: add PWM dependencies

[Linus Walleij]

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

Re: [PATCH] backlight: add PWM dependencies

[Jingoo Han]

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

Re: [PATCH] backlight: add PWM dependencies

[Jingoo Han]

On Thursday, February 06, 2014 3:50 PM, Jingoo Han wrote:
> 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.

(+cc Arnd Bergmann)

Oh, sorry. There was my mistake.
I tested this with linux-next tree.

With linux 3.14-rc1, it makes the problem as below.

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'

> 
> >
> > > ./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? :-)

Thierry Reding,
Would you confirm this?

In the case of "CONFIG_HAVE_PWM=y && CONFIG_PWM=n", it makes
the problem.

The HAVE_PWM symbol is only for legacy platforms that provide
the PWM API without using the generic framework. PXA looks to
use the generic PWM framework. Then, how about removing
"select HAVE_PWM" from PXA as below?

--- a/arch/arm/mach-pxa/Kconfig
+++ b/arch/arm/mach-pxa/Kconfig
@@ -7,7 +7,6 @@ comment "Intel/Marvell Dev Platforms (sorted by hardware release time)"
 config MACH_PXA3XX_DT
        bool "Support PXA3xx platforms from device tree"
        select CPU_PXA300
-       select HAVE_PWM
        select POWER_SUPPLY
        select PXA3xx
        select USE_OF
@@ -23,12 +22,10 @@ config ARCH_LUBBOCK

 config MACH_MAINSTONE
        bool "Intel HCDDBBVA0 Development Platform (aka Mainstone)"
-       select HAVE_PWM
        select PXA27x

 config MACH_ZYLONITE
        bool
-       select HAVE_PWM
        select PXA3xx
.....


Best regards,
Jingoo Han

Re: [PATCH] backlight: add PWM dependencies

[Linus Walleij]

On Thu, Feb 6, 2014 at 8:23 AM, Jingoo Han <jg1.han@samsung.com> wrote:

> In the case of "CONFIG_HAVE_PWM=y && CONFIG_PWM=n", it makes
> the problem.
>
> The HAVE_PWM symbol is only for legacy platforms that provide
> the PWM API without using the generic framework. PXA looks to
> use the generic PWM framework. Then, how about removing
> "select HAVE_PWM" from PXA as below?
>
> --- a/arch/arm/mach-pxa/Kconfig
> +++ b/arch/arm/mach-pxa/Kconfig
> @@ -7,7 +7,6 @@ comment "Intel/Marvell Dev Platforms (sorted by hardware release time)"
>  config MACH_PXA3XX_DT
>         bool "Support PXA3xx platforms from device tree"
>         select CPU_PXA300
> -       select HAVE_PWM
>         select POWER_SUPPLY
>         select PXA3xx
>         select USE_OF
> @@ -23,12 +22,10 @@ config ARCH_LUBBOCK
>
>  config MACH_MAINSTONE
>         bool "Intel HCDDBBVA0 Development Platform (aka Mainstone)"
> -       select HAVE_PWM
>         select PXA27x
>
>  config MACH_ZYLONITE
>         bool
> -       select HAVE_PWM
>         select PXA3xx

Looks like the right solution to me.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH] backlight: add PWM dependencies

[Arnd Bergmann]

On Thursday 06 February 2014, Jingoo Han wrote:
> In the case of "CONFIG_HAVE_PWM=y && CONFIG_PWM=n", it makes
> the problem.
> 
> The HAVE_PWM symbol is only for legacy platforms that provide
> the PWM API without using the generic framework. PXA looks to
> use the generic PWM framework. Then, how about removing
> "select HAVE_PWM" from PXA as below?
> 

I think this is correct, but we may need additional patches. I notice
that INPUT_MAX8997_HAPTIC and INPUT_PWM_BEEPER have a dependency on
HAVE_PWM at the moment, so those two drivers become impossible
to select after your change.

There is also one use of HAVE_PWM outside of PXA, for ARCH_LPC32XX.
This one seems to have the same problem.

Finally, I have recently encountered a couple of drivers
(BACKLIGHT_LM3630A, BACKLIGHT_LP855X, BACKLIGHT_LP8788) that use
the PWM interfaces but are missing a 'depends on PWM'. This is
strictly speaking a different problem, but we could try to solve
it at the same time.

	Arnd

Re: [PATCH] backlight: add PWM dependencies

[Arnd Bergmann]

On Thursday 06 February 2014 17:08:05 Arnd Bergmann wrote:
> 
> Finally, I have recently encountered a couple of drivers
> (BACKLIGHT_LM3630A, BACKLIGHT_LP855X, BACKLIGHT_LP8788) that use
> the PWM interfaces but are missing a 'depends on PWM'. This is
> strictly speaking a different problem, but we could try to solve
> it at the same time.
> 

D'oh. I just realized this is the bug that started the thread
with Linus' patch. Apparently I found one more instance that
he didn't find though.

	Arnd

Re: [PATCH] backlight: add PWM dependencies

[Jingoo Han]

On Friday, February 07, 2014 1:08 AM, Arnd Bergmann wrote:
> On Thursday 06 February 2014, Jingoo Han wrote:
> > In the case of "CONFIG_HAVE_PWM=y && CONFIG_PWM=n", it makes
> > the problem.
> >
> > The HAVE_PWM symbol is only for legacy platforms that provide
> > the PWM API without using the generic framework. PXA looks to
> > use the generic PWM framework. Then, how about removing
> > "select HAVE_PWM" from PXA as below?
> >
> 
> I think this is correct, but we may need additional patches. I notice
> that INPUT_MAX8997_HAPTIC and INPUT_PWM_BEEPER have a dependency on
> HAVE_PWM at the moment, so those two drivers become impossible
> to select after your change.
> 
> There is also one use of HAVE_PWM outside of PXA, for ARCH_LPC32XX.
> This one seems to have the same problem.

I looked at all HAVE_PWMs in the latest mainline kernel 3.14-rc1.

1. ARM - PXA
  ./arch/arm/mach-pxa/Kconfig

2. ARM - NXP LPC32XX
  ./arc ARM - PXA h/arm/Kconfig
  config ARCH_LPC32XX
  	select HAVE_PWM

3. MIPS - Ingenic JZ4740 based machines
  ./arch/mips/Kconfig
  config MACH_JZ4740
  	select HAVE_PWM

However, the legacy PWM drivers for PXA, LPC32XX, and JZ474 were
already moved to the generic PWM framework.
  ./drivers/pwm/pwm-pxa.c
  ./drivers/pwm/pwm-lpc32xx.c
  ./drivers/pwm/pwm-jz4740.c

In conclusion, HAVE_PWM should be removed, because HAVE_PWM is
NOT required anymore.

How about the following?

  [PATCH 1/7] ARM: pxa: don't select HAVE_PWM
  [PATCH 2/7] ARM: lpc32xx: don't select HAVE_PWM
  [PATCH 3/7] ARM: remove HAVE_PWM config option
  [PATCH 4/7] MIPS: jz4740: don't select HAVE_PWM
  [PATCH 5/7] Input: max8997_haptic: remove HAVE_PWM dependencies
  [PATCH 6/7] Input: pwm-beepe: remove HAVE_PWM dependencies
  [PATCH 7/7] pwm: don't use IS_ENABLED(CONFIG_HAVE_PWM)

I would like to merge it through PWM tree.
After merging these patches, all HAVE_PWM will be removed from
the mainline kernel. Thank you. :-)

Best regards,
Jingoo Han

> 
> Finally, I have recently encountered a couple of drivers
> (BACKLIGHT_LM3630A, BACKLIGHT_LP855X, BACKLIGHT_LP8788) that use
> the PWM interfaces but are missing a 'depends on PWM'. This is
> strictly speaking a different problem, but we could try to solve
> it at the same time.
> 
> 	Arnd

Re: [PATCH] backlight: add PWM dependencies

[Arnd Bergmann]

On Friday 07 February 2014, Jingoo Han wrote:
> How about the following?
> 
>   [PATCH 1/7] ARM: pxa: don't select HAVE_PWM
>   [PATCH 2/7] ARM: lpc32xx: don't select HAVE_PWM
>   [PATCH 3/7] ARM: remove HAVE_PWM config option
>   [PATCH 4/7] MIPS: jz4740: don't select HAVE_PWM
>   [PATCH 5/7] Input: max8997_haptic: remove HAVE_PWM dependencies
>   [PATCH 6/7] Input: pwm-beepe: remove HAVE_PWM dependencies
>   [PATCH 7/7] pwm: don't use IS_ENABLED(CONFIG_HAVE_PWM)
> 
> I would like to merge it through PWM tree.
> After merging these patches, all HAVE_PWM will be removed from
> the mainline kernel. Thank you. :-)

Sounds godo to me, thanks a lot for taking care of this!
I don't see any inter-dependencies between the various patches,
so we could also take the first three through the arm-soc tree
to avoid conflicts with other changes (or possibly the third
one through rmk's ARM tree, if he prefers).

	Arnd

Re: [PATCH] backlight: add PWM dependencies

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] backlight: add PWM dependencies

[Linus Walleij]

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

Re: [PATCH] backlight: add PWM dependencies

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1112 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]