diff mbox

backlight: add PWM dependencies

Message ID 000001cf230c$60ec1ca0$22c455e0$%han@samsung.com
State Superseded
Headers show

Commit Message

Jingoo Han Feb. 6, 2014, 7:23 a.m. UTC 
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?


Best regards,
Jingoo Han

--
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

Comments

Linus Walleij Feb. 6, 2014, 8:32 a.m. UTC | #1 
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
--
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
Arnd Bergmann Feb. 6, 2014, 4:08 p.m. UTC | #2 
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
--
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
Arnd Bergmann Feb. 6, 2014, 4:35 p.m. UTC | #3 
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
--
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. 7, 2014, 3:05 a.m. UTC | #4 
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

--
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
Arnd Bergmann Feb. 7, 2014, 9:40 a.m. UTC | #5 
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
--
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
diff mbox

Patch

--- 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
.....