[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Shawn Guo]

The .config() calls clk_get_rate() which might sleep, so we need to set
pwm_chip can_sleep flag.  Otherwise, we see the following warning when
using pwm driven heartbeat led.

WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
DEBUG_LOCKS_WARN_ON(in_interrupt())
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
[<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
[<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
[<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
[<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
[<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
[<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
[<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
[<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
[<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
[<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
[<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
[<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
[<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
[<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
[<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
[<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
[<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
[<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
[<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)

Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
---
 drivers/pwm/pwm-mxs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index 9475bc7..4f1bb4e 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
 	mxs->chip.dev = &pdev->dev;
 	mxs->chip.ops = &mxs_pwm_ops;
 	mxs->chip.base = -1;
+	mxs->chip.can_sleep = true;
 	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
-- 
1.8.3.2

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Stefan Wahren]

Hi Shawn,

Am 08.04.2014 13:29, schrieb Shawn Guo:
> The .config() calls clk_get_rate() which might sleep, so we need to set
> pwm_chip can_sleep flag.  Otherwise, we see the following warning when
> using pwm driven heartbeat led.
>
> WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
> DEBUG_LOCKS_WARN_ON(in_interrupt())
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
> [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
> [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
> [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
> [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
> [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
> [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
> [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
> [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
> [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
> [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
> [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
> [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
> [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
> [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
> [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
> [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
> [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
> [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
>
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> ---
>  drivers/pwm/pwm-mxs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 9475bc7..4f1bb4e 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
>  	mxs->chip.dev = &pdev->dev;
>  	mxs->chip.ops = &mxs_pwm_ops;
>  	mxs->chip.base = -1;
> +	mxs->chip.can_sleep = true;
>  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);

thanks for the patch. I've tested it with our i.MX28 board and the
warning above never came. So it works.
Unfortunately the led still don't behave as expected. If i set the led
trigger to heartbeat the led goes on and stays in this state.

May be this has something to do with the following discussion

http://comments.gmane.org/gmane.linux.leds/208

Kind regards
Stefan Wahren

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Alexandre Belloni]

On 08/04/2014 at 15:46:59 +0200, Stefan Wahren wrote :
> Hi Shawn,
> 
> Am 08.04.2014 13:29, schrieb Shawn Guo:
> > The .config() calls clk_get_rate() which might sleep, so we need to set
> > pwm_chip can_sleep flag.  Otherwise, we see the following warning when
> > using pwm driven heartbeat led.
> >
> > WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
> > DEBUG_LOCKS_WARN_ON(in_interrupt())
> > Modules linked in:
> > CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> > [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
> > [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
> > [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
> > [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
> > [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
> > [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
> > [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
> > [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
> > [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
> > [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
> > [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
> > [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
> > [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
> > [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
> > [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
> > [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
> > [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
> > [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
> > [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
> >
> > Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  drivers/pwm/pwm-mxs.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> > index 9475bc7..4f1bb4e 100644
> > --- a/drivers/pwm/pwm-mxs.c
> > +++ b/drivers/pwm/pwm-mxs.c
> > @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
> >  	mxs->chip.dev = &pdev->dev;
> >  	mxs->chip.ops = &mxs_pwm_ops;
> >  	mxs->chip.base = -1;
> > +	mxs->chip.can_sleep = true;
> >  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> 
> thanks for the patch. I've tested it with our i.MX28 board and the
> warning above never came. So it works.
> Unfortunately the led still don't behave as expected. If i set the led
> trigger to heartbeat the led goes on and stays in this state.
> 
> May be this has something to do with the following discussion
> 
> http://comments.gmane.org/gmane.linux.leds/208
> 

This may be but the solution is not correct we want to keep disabling
the pwm when brightness is 0. Can you try the latest series from
Russell:

http://thread.gmane.org/gmane.linux.leds/579 and set active_low but keep
the pwm polarity normal.

Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Alexandre Belloni]

On 08/04/2014 at 19:29:57 +0800, Shawn Guo wrote :
> The .config() calls clk_get_rate() which might sleep, so we need to set
> pwm_chip can_sleep flag.  Otherwise, we see the following warning when
> using pwm driven heartbeat led.
> 
> WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
> DEBUG_LOCKS_WARN_ON(in_interrupt())
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
> [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
> [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
> [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
> [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
> [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
> [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
> [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
> [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
> [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
> [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
> [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
> [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
> [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
> [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
> [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
> [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
> [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
> [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
> 
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>

Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
>  drivers/pwm/pwm-mxs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 9475bc7..4f1bb4e 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
>  	mxs->chip.dev = &pdev->dev;
>  	mxs->chip.ops = &mxs_pwm_ops;
>  	mxs->chip.base = -1;
> +	mxs->chip.can_sleep = true;
>  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> -- 
> 1.8.3.2
> 
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Stefan Wahren]

Hi Alexandre,

Am 08.04.2014 16:18, schrieb Alexandre Belloni:
> On 08/04/2014 at 15:46:59 +0200, Stefan Wahren wrote :
>> Hi Shawn,
>>
>> Am 08.04.2014 13:29, schrieb Shawn Guo:
>>> The .config() calls clk_get_rate() which might sleep, so we need to set
>>> pwm_chip can_sleep flag.  Otherwise, we see the following warning when
>>> using pwm driven heartbeat led.
>>>
>>> WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
>>> DEBUG_LOCKS_WARN_ON(in_interrupt())
>>> Modules linked in:
>>> CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
>>> [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
>>> [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
>>> [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
>>> [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
>>> [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
>>> [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
>>> [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
>>> [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
>>> [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
>>> [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
>>> [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
>>> [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
>>> [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
>>> [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
>>> [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
>>> [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
>>> [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
>>> [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
>>> [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
>>>
>>> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
>>> ---
>>>  drivers/pwm/pwm-mxs.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
>>> index 9475bc7..4f1bb4e 100644
>>> --- a/drivers/pwm/pwm-mxs.c
>>> +++ b/drivers/pwm/pwm-mxs.c
>>> @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
>>>  	mxs->chip.dev = &pdev->dev;
>>>  	mxs->chip.ops = &mxs_pwm_ops;
>>>  	mxs->chip.base = -1;
>>> +	mxs->chip.can_sleep = true;
>>>  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
>>>  	if (ret < 0) {
>>>  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
>> thanks for the patch. I've tested it with our i.MX28 board and the
>> warning above never came. So it works.
>> Unfortunately the led still don't behave as expected. If i set the led
>> trigger to heartbeat the led goes on and stays in this state.
>>
>> May be this has something to do with the following discussion
>>
>> http://comments.gmane.org/gmane.linux.leds/208
>>
> This may be but the solution is not correct we want to keep disabling
> the pwm when brightness is 0. Can you try the latest series from
> Russell:
>
> http://thread.gmane.org/gmane.linux.leds/579 and set active_low but keep
> the pwm polarity normal.
>
> Regards,
>

i applied these 5 patches, but it doesn't fix the problem.

Here are my results for the pwm driver led on i.MX28:

version                  | trigger: none          | trigger: heartbeat
-----------------------------------------------------------------------------
3.14                     | OK                     | NOT OK (WARNING, LED
permanent on)
  + shawn's patch        | OK                     | NOT OK (LED
permanent on)
    + russell's patches  | OK                     | NOT OK (LED
permanent on)
      + active-low       | NOT OK (inverse logic) | NOT OK (LED
permanent on)

Russell's patches work as expected, but have not influence on the heartbeat.

Regards,
Stefan

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Stefan Wahren]

Am 08.04.2014 13:29, schrieb Shawn Guo:
> The .config() calls clk_get_rate() which might sleep, so we need to set
> pwm_chip can_sleep flag.  Otherwise, we see the following warning when
> using pwm driven heartbeat led.
>
> WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
> DEBUG_LOCKS_WARN_ON(in_interrupt())
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
> [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
> [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
> [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
> [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
> [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
> [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
> [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
> [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
> [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
> [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
> [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
> [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
> [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
> [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
> [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
> [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
> [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
> [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
>
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Shawn Guo <shawn.guo@freescale.com>

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> ---
>  drivers/pwm/pwm-mxs.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 9475bc7..4f1bb4e 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
>  	mxs->chip.dev = &pdev->dev;
>  	mxs->chip.ops = &mxs_pwm_ops;
>  	mxs->chip.base = -1;
> +	mxs->chip.can_sleep = true;
>  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Uwe Kleine-König]

Hello,

On Tue, Apr 08, 2014 at 03:46:59PM +0200, Stefan Wahren wrote:
> Am 08.04.2014 13:29, schrieb Shawn Guo:
> > The .config() calls clk_get_rate() which might sleep, so we need to set
> > pwm_chip can_sleep flag.  Otherwise, we see the following warning when
> > using pwm driven heartbeat led.
> >
> > WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
> > DEBUG_LOCKS_WARN_ON(in_interrupt())
> > Modules linked in:
> > CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> > [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
> > [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
> > [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
> > [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
> > [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
> > [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
> > [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
> > [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
> > [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
> > [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
> > [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
> > [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
> > [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
> > [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
> > [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
> > [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
> > [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
> > [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
> > [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
> >
> > Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > ---
> >  drivers/pwm/pwm-mxs.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> > index 9475bc7..4f1bb4e 100644
> > --- a/drivers/pwm/pwm-mxs.c
> > +++ b/drivers/pwm/pwm-mxs.c
> > @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
> >  	mxs->chip.dev = &pdev->dev;
> >  	mxs->chip.ops = &mxs_pwm_ops;
> >  	mxs->chip.base = -1;
> > +	mxs->chip.can_sleep = true;
> >  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
> >  	if (ret < 0) {
> >  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> 
> thanks for the patch. I've tested it with our i.MX28 board and the
> warning above never came. So it works.
> Unfortunately the led still don't behave as expected. If i set the led
> trigger to heartbeat the led goes on and stays in this state.
> 
> May be this has something to do with the following discussion
> 
> http://comments.gmane.org/gmane.linux.leds/208
I guess that's your problem, yes. Does patch 2 of my series (i.e.
http://thread.gmane.org/gmane.linux.ports.arm.kernel/282593/focus=282596)
help you?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Alexandre Belloni]

Hi,

On 08/04/2014 at 18:53:43 +0200, Stefan Wahren wrote :
> i applied these 5 patches, but it doesn't fix the problem.
> 
> Here are my results for the pwm driver led on i.MX28:
> 
> version                  | trigger: none          | trigger: heartbeat
> -----------------------------------------------------------------------------
> 3.14                     | OK                     | NOT OK (WARNING, LED
> permanent on)
>   + shawn's patch        | OK                     | NOT OK (LED
> permanent on)
>     + russell's patches  | OK                     | NOT OK (LED
> permanent on)
>       + active-low       | NOT OK (inverse logic) | NOT OK (LED
> permanent on)
> 
> Russell's patches work as expected, but have not influence on the heartbeat.
> 

Yeah, I got it wrong or explained badly.

So, if without Russell's patches, your pwm is not inversed, then you would
have to set active_low and inverse the pwm.
If it was inversed, then you'll have to set active_low and let the pwm
with the normal polarity. I know this is a bit contrived but this was
exactly my point why we shouldn't call it active_low.

Can you try to switch the polarity when setting active_low ?

I'm not home, else I'll try on my i.mx28 boards...

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Alexandre Belloni]

Hi,

On 08/04/2014 at 19:59:04 +0200, Uwe Kleine-K?nig wrote :
> Hello,
> 
> On Tue, Apr 08, 2014 at 03:46:59PM +0200, Stefan Wahren wrote:
> > Am 08.04.2014 13:29, schrieb Shawn Guo:
> > > The .config() calls clk_get_rate() which might sleep, so we need to set
> > > pwm_chip can_sleep flag.  Otherwise, we see the following warning when
> > > using pwm driven heartbeat led.
> > >
> > > WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:856 mutex_trylock+0x184/0x1a4()
> > > DEBUG_LOCKS_WARN_ON(in_interrupt())
> > > Modules linked in:
> > > CPU: 0 PID: 0 Comm: swapper Not tainted 3.14.0-rc5 #18
> > > [<c0015420>] (unwind_backtrace) from [<c0012cb0>] (show_stack+0x10/0x14)
> > > [<c0012cb0>] (show_stack) from [<c001daf8>] (warn_slowpath_common+0x6c/0x8c)
> > > [<c001daf8>] (warn_slowpath_common) from [<c001dbac>] (warn_slowpath_fmt+0x30/0x40)
> > > [<c001dbac>] (warn_slowpath_fmt) from [<c045df74>] (mutex_trylock+0x184/0x1a4)
> > > [<c045df74>] (mutex_trylock) from [<c0360950>] (clk_prepare_lock+0xc/0xec)
> > > [<c0360950>] (clk_prepare_lock) from [<c0362020>] (clk_get_rate+0xc/0x68)
> > > [<c0362020>] (clk_get_rate) from [<c028d07c>] (mxs_pwm_config+0x20/0x198)
> > > [<c028d07c>] (mxs_pwm_config) from [<c028bde8>] (pwm_config+0x60/0x70)
> > > [<c028bde8>] (pwm_config) from [<c034b61c>] (__led_pwm_set+0x1c/0x3c)
> > > [<c034b61c>] (__led_pwm_set) from [<c034bc3c>] (led_heartbeat_function+0x70/0x110)
> > > [<c034bc3c>] (led_heartbeat_function) from [<c00292f0>] (call_timer_fn+0x7c/0x164)
> > > [<c00292f0>] (call_timer_fn) from [<c00295c8>] (run_timer_softirq+0x1f0/0x260)
> > > [<c00295c8>] (run_timer_softirq) from [<c002255c>] (__do_softirq+0xc4/0x2f0)
> > > [<c002255c>] (__do_softirq) from [<c0022890>] (irq_exit+0xa4/0x10c)
> > > [<c0022890>] (irq_exit) from [<c0010240>] (handle_IRQ+0x34/0x84)
> > > [<c0010240>] (handle_IRQ) from [<c0013524>] (__irq_svc+0x44/0x54)
> > > [<c0013524>] (__irq_svc) from [<c00107f8>] (arch_cpu_idle+0x40/0x48)
> > > [<c00107f8>] (arch_cpu_idle) from [<c005deb8>] (cpu_startup_entry+0x70/0x198)
> > > [<c005deb8>] (cpu_startup_entry) from [<c060aac8>] (start_kernel+0x2a8/0x2f8)
> > >
> > > Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com>
> > > ---
> > >  drivers/pwm/pwm-mxs.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> > > index 9475bc7..4f1bb4e 100644
> > > --- a/drivers/pwm/pwm-mxs.c
> > > +++ b/drivers/pwm/pwm-mxs.c
> > > @@ -147,6 +147,7 @@ static int mxs_pwm_probe(struct platform_device *pdev)
> > >  	mxs->chip.dev = &pdev->dev;
> > >  	mxs->chip.ops = &mxs_pwm_ops;
> > >  	mxs->chip.base = -1;
> > > +	mxs->chip.can_sleep = true;
> > >  	ret = of_property_read_u32(np, "fsl,pwm-number", &mxs->chip.npwm);
> > >  	if (ret < 0) {
> > >  		dev_err(&pdev->dev, "failed to get pwm number: %d\n", ret);
> > 
> > thanks for the patch. I've tested it with our i.MX28 board and the
> > warning above never came. So it works.
> > Unfortunately the led still don't behave as expected. If i set the led
> > trigger to heartbeat the led goes on and stays in this state.
> > 
> > May be this has something to do with the following discussion
> > 
> > http://comments.gmane.org/gmane.linux.leds/208
> I guess that's your problem, yes. Does patch 2 of my series (i.e.
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/282593/focus=282596)
> help you?
> 

While I had exactly the same issue back in september and wrote the exact
same patch that got rejected. I would agree with Thierry here taht we
need to keep the pwm_disable() there as it potentially allows to save
power.

However, it seems to not work quite well with PWMs from Freescale (see
the thread from Russell). Or I would say with PWM with an undefined
state when disabled and I believe we are soon to find more.

We could either:
 - add a flag like can_sleep that would allow driver to know that the
   pwm always has to be enable to get sane results.
 - or introduce functions like prepare/unprepare to be called from probe
   and remove in the leds-pwm driver and that will enable/disable the
channel in pwm-mxs. pwm_enable/pwm_disable not doing anything.

That is waht is coming to mind but it might be too late for me to think
straight.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Russell King - ARM Linux]

On Wed, Apr 09, 2014 at 01:16:54AM +0200, Alexandre Belloni wrote:
> On 08/04/2014 at 18:53:43 +0200, Stefan Wahren wrote :
> > i applied these 5 patches, but it doesn't fix the problem.
> > 
> > Here are my results for the pwm driver led on i.MX28:
> > 
> > version                  | trigger: none          | trigger: heartbeat
> > -----------------------------------------------------------------------------
> > 3.14                     | OK                     | NOT OK (WARNING, LED
> > permanent on)
> >   + shawn's patch        | OK                     | NOT OK (LED
> > permanent on)
> >     + russell's patches  | OK                     | NOT OK (LED
> > permanent on)
> >       + active-low       | NOT OK (inverse logic) | NOT OK (LED
> > permanent on)
> > 
> > Russell's patches work as expected, but have not influence on the heartbeat.
> > 
> 
> Yeah, I got it wrong or explained badly.
> 
> So, if without Russell's patches, your pwm is not inversed, then you would
> have to set active_low and inverse the pwm.
> If it was inversed, then you'll have to set active_low and let the pwm
> with the normal polarity. I know this is a bit contrived but this was
> exactly my point why we shouldn't call it active_low.
> 
> Can you try to switch the polarity when setting active_low ?
> 
> I'm not home, else I'll try on my i.mx28 boards...

I just tried heartbeat here, on iMX6, and it works fine.  That's with this
in the DT:

        pwmleds {
                compatible = "pwm-leds";
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_cubox_i_pwm1>;

                front {
                        active-low;
                        label = "imx6:red:front";
                        pwms = <&pwm1 0 50000>;
                };
        };

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Shawn Guo]

On Wed, Apr 09, 2014 at 12:44:18AM +0100, Russell King - ARM Linux wrote:
> On Wed, Apr 09, 2014 at 01:16:54AM +0200, Alexandre Belloni wrote:
> > I'm not home, else I'll try on my i.mx28 boards...
> 
> I just tried heartbeat here, on iMX6, and it works fine.  That's with this
> in the DT:

FYI. i.MX28 is known as 'mxs' sub-architecture and uses pwm-mxs driver
rather than pwm-imx which is the one for i.MX6.

Shawn

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Uwe Kleine-König]

Hello,

On Wed, Apr 09, 2014 at 01:42:43AM +0200, Alexandre Belloni wrote:
> On 08/04/2014 at 19:59:04 +0200, Uwe Kleine-K?nig wrote :
> > On Tue, Apr 08, 2014 at 03:46:59PM +0200, Stefan Wahren wrote:
> > > Am 08.04.2014 13:29, schrieb Shawn Guo:
> > > Unfortunately the led still don't behave as expected. If i set the led
> > > trigger to heartbeat the led goes on and stays in this state.
> > > 
> > > May be this has something to do with the following discussion
> > > 
> > > http://comments.gmane.org/gmane.linux.leds/208
> > I guess that's your problem, yes. Does patch 2 of my series (i.e.
> > http://thread.gmane.org/gmane.linux.ports.arm.kernel/282593/focus=282596)
> > help you?
> > 
> 
> While I had exactly the same issue back in september and wrote the exact
> same patch that got rejected. I would agree with Thierry here taht we
> need to keep the pwm_disable() there as it potentially allows to save
> power.
>
> However, it seems to not work quite well with PWMs from Freescale (see
> the thread from Russell). Or I would say with PWM with an undefined
> state when disabled and I believe we are soon to find more.
> 
> We could either:
>  - add a flag like can_sleep that would allow driver to know that the
>    pwm always has to be enable to get sane results.
>
>  - or introduce functions like prepare/unprepare to be called from probe
>    and remove in the leds-pwm driver and that will enable/disable the
> channel in pwm-mxs. pwm_enable/pwm_disable not doing anything.
There are a few more options. I repeat your's to get a single list:

 a) Somehow tell the API users that pwm_disable might not result in a
    flat zero after set_duty(0).
 a') Anchor in the API that users must not expect anything from the pwm
    pin after pwm_disable.
 b) Make pwm_enable/pwm_disable noops on i.MX28
 c) Make set_duty only return when the new value reached the pin.
 d) Make pwm_disable wait to yield the expected result
 d') Make pwm_disable wait if the last programmed duty cycle is 0 or
    full period.
 e) Introduce a new callback that does the waiting, e.g. a .commit
    callback
 e') Introduce a new callback pwm_config_sync that does wait
    appropriatly, keep pwm_config as is.
 f) Make the i.MX28 driver switch the pin the gpio mode with the
    expected output level in pwm_disable.

a') is a special case of a); d') is a special case of d), ditto for e')
and e).

a) and e) would result in changes in the pwm API. For c) I'd like to
make the API docs more explicit, too. (i.e. demand that the new
configuration is already active after pwm_config returns)

Both e) and a) have the drawback that the API becomes more complicated
and users will get it wrong. a) and b) are bad from a power management
POV. Maybe c) is what most users expect, but d) and d') are more
effective as they result in less waiting and still are good enough. f)
is more complicated to implement, also it depends on a gpio being
available in hardware. So if we choose f) we still might need a fallback
implementation from the other options.

My series implemented a'), but Thierry didn't like it. I think my
favorite is e').

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Russell King - ARM Linux]

On Wed, Apr 09, 2014 at 11:41:17AM +0800, Shawn Guo wrote:
> On Wed, Apr 09, 2014 at 12:44:18AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Apr 09, 2014 at 01:16:54AM +0200, Alexandre Belloni wrote:
> > > I'm not home, else I'll try on my i.mx28 boards...
> > 
> > I just tried heartbeat here, on iMX6, and it works fine.  That's with this
> > in the DT:
> 
> FYI. i.MX28 is known as 'mxs' sub-architecture and uses pwm-mxs driver
> rather than pwm-imx which is the one for i.MX6.

My point being by doing the above test is that the pwm_disable() call
in leds-pwm works correctly with iMX6 and the "troublesome" active-low
flag set at the leds-pwm level.

It wouldn't work on iMX6 if you tried to use the broken inversion on the
PWM output - it would have exactly the effects being reported on iMX28
as setting a duty of 100% would pull the signal low and disabling the PWM
will also pull the signal low (which are the two states heartbeat uses.)

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Alexandre Belloni]

On 09/04/2014 at 10:23:41 +0200, Uwe Kleine-K?nig wrote :
> > While I had exactly the same issue back in september and wrote the exact
> > same patch that got rejected. I would agree with Thierry here taht we
> > need to keep the pwm_disable() there as it potentially allows to save
> > power.
> >
> > However, it seems to not work quite well with PWMs from Freescale (see
> > the thread from Russell). Or I would say with PWM with an undefined
> > state when disabled and I believe we are soon to find more.
> > 
> > We could either:
> >  - add a flag like can_sleep that would allow driver to know that the
> >    pwm always has to be enable to get sane results.
> >
> >  - or introduce functions like prepare/unprepare to be called from probe
> >    and remove in the leds-pwm driver and that will enable/disable the
> > channel in pwm-mxs. pwm_enable/pwm_disable not doing anything.
> There are a few more options. I repeat your's to get a single list:
> 
>  a) Somehow tell the API users that pwm_disable might not result in a
>     flat zero after set_duty(0).
>  a') Anchor in the API that users must not expect anything from the pwm
>     pin after pwm_disable.
>  b) Make pwm_enable/pwm_disable noops on i.MX28
>  c) Make set_duty only return when the new value reached the pin.
>  d) Make pwm_disable wait to yield the expected result
>  d') Make pwm_disable wait if the last programmed duty cycle is 0 or
>     full period.
>  e) Introduce a new callback that does the waiting, e.g. a .commit
>     callback
>  e') Introduce a new callback pwm_config_sync that does wait
>     appropriatly, keep pwm_config as is.
>  f) Make the i.MX28 driver switch the pin the gpio mode with the
>     expected output level in pwm_disable.
> 
> a') is a special case of a); d') is a special case of d), ditto for e')
> and e).
> 
> a) and e) would result in changes in the pwm API. For c) I'd like to
> make the API docs more explicit, too. (i.e. demand that the new
> configuration is already active after pwm_config returns)
> 
> Both e) and a) have the drawback that the API becomes more complicated
> and users will get it wrong. a) and b) are bad from a power management
> POV. Maybe c) is what most users expect, but d) and d') are more
> effective as they result in less waiting and still are good enough. f)
> is more complicated to implement, also it depends on a gpio being
> available in hardware. So if we choose f) we still might need a fallback
> implementation from the other options.
> 
> My series implemented a'), but Thierry didn't like it. I think my
> favorite is e').
> 

I think I would actually prefer d'. Doing the synchronisation in d' is
abstracting it for the user.
But e' is also fine as long as we manage to educate users of when to use
pwm_config() or pwm_config_sync(). I believe your idea for leds-pwm
would be to call pwm_config_sync() instead of pwm_config() before
pwm_disable().

I actually had to implement d' for pwm-atmel, to wait that the last
update got through before disabling the pwm, see
http://thread.gmane.org/gmane.linux.pwm/451 but doing that in
pwm_config_sync() would probably also work.


For a and a' I think that Thierry intended that a user could expect
pwm_disable() to not change the output of the PWM when the duty cycle is
0.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Thierry Reding]

On Wed, Apr 09, 2014 at 12:03:13PM +0200, Alexandre Belloni wrote:
> On 09/04/2014 at 10:23:41 +0200, Uwe Kleine-K?nig wrote :
> > > While I had exactly the same issue back in september and wrote the exact
> > > same patch that got rejected. I would agree with Thierry here taht we
> > > need to keep the pwm_disable() there as it potentially allows to save
> > > power.
> > >
> > > However, it seems to not work quite well with PWMs from Freescale (see
> > > the thread from Russell). Or I would say with PWM with an undefined
> > > state when disabled and I believe we are soon to find more.
> > > 
> > > We could either:
> > >  - add a flag like can_sleep that would allow driver to know that the
> > >    pwm always has to be enable to get sane results.
> > >
> > >  - or introduce functions like prepare/unprepare to be called from probe
> > >    and remove in the leds-pwm driver and that will enable/disable the
> > > channel in pwm-mxs. pwm_enable/pwm_disable not doing anything.
> > There are a few more options. I repeat your's to get a single list:
> > 
> >  a) Somehow tell the API users that pwm_disable might not result in a
> >     flat zero after set_duty(0).
> >  a') Anchor in the API that users must not expect anything from the pwm
> >     pin after pwm_disable.
> >  b) Make pwm_enable/pwm_disable noops on i.MX28
> >  c) Make set_duty only return when the new value reached the pin.
> >  d) Make pwm_disable wait to yield the expected result
> >  d') Make pwm_disable wait if the last programmed duty cycle is 0 or
> >     full period.
> >  e) Introduce a new callback that does the waiting, e.g. a .commit
> >     callback
> >  e') Introduce a new callback pwm_config_sync that does wait
> >     appropriatly, keep pwm_config as is.
> >  f) Make the i.MX28 driver switch the pin the gpio mode with the
> >     expected output level in pwm_disable.
> > 
> > a') is a special case of a); d') is a special case of d), ditto for e')
> > and e).
> > 
> > a) and e) would result in changes in the pwm API. For c) I'd like to
> > make the API docs more explicit, too. (i.e. demand that the new
> > configuration is already active after pwm_config returns)
> > 
> > Both e) and a) have the drawback that the API becomes more complicated
> > and users will get it wrong. a) and b) are bad from a power management
> > POV. Maybe c) is what most users expect, but d) and d') are more
> > effective as they result in less waiting and still are good enough. f)
> > is more complicated to implement, also it depends on a gpio being
> > available in hardware. So if we choose f) we still might need a fallback
> > implementation from the other options.
> > 
> > My series implemented a'), but Thierry didn't like it. I think my
> > favorite is e').
> > 
> 
> I think I would actually prefer d'. Doing the synchronisation in d' is
> abstracting it for the user.

My preference would be simply d). Mostly because I don't like special
cases and especially because I don't see an advantage in special casing
0 and full period duty cycles. That doesn't mean of course that drivers
can't special case if they really want or have to.

That said I've also been thinking about adding support for e), which
would allow atomically changing the duty cycle, period and polarity of a
channel. This might become necessary at some point.

The downside of course being that user drivers will need to change. But
I suspect that may not be necessary for existing users since the current
API works for them well enough (except for the case currently under
discussion, of course).

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140409/e0fb25bb/attachment.sig>

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Uwe Kleine-König]

Hello,

On Wed, Apr 09, 2014 at 12:46:53PM +0200, Thierry Reding wrote:
> On Wed, Apr 09, 2014 at 12:03:13PM +0200, Alexandre Belloni wrote:
> > On 09/04/2014 at 10:23:41 +0200, Uwe Kleine-K?nig wrote :
> > > > While I had exactly the same issue back in september and wrote the exact
> > > > same patch that got rejected. I would agree with Thierry here taht we
> > > > need to keep the pwm_disable() there as it potentially allows to save
> > > > power.
> > > >
> > > > However, it seems to not work quite well with PWMs from Freescale (see
> > > > the thread from Russell). Or I would say with PWM with an undefined
> > > > state when disabled and I believe we are soon to find more.
> > > > 
> > > > We could either:
> > > >  - add a flag like can_sleep that would allow driver to know that the
> > > >    pwm always has to be enable to get sane results.
> > > >
> > > >  - or introduce functions like prepare/unprepare to be called from probe
> > > >    and remove in the leds-pwm driver and that will enable/disable the
> > > > channel in pwm-mxs. pwm_enable/pwm_disable not doing anything.
> > > There are a few more options. I repeat your's to get a single list:
> > > 
> > >  a) Somehow tell the API users that pwm_disable might not result in a
> > >     flat zero after set_duty(0).
> > >  a') Anchor in the API that users must not expect anything from the pwm
> > >     pin after pwm_disable.
> > >  b) Make pwm_enable/pwm_disable noops on i.MX28
> > >  c) Make set_duty only return when the new value reached the pin.
> > >  d) Make pwm_disable wait to yield the expected result
> > >  d') Make pwm_disable wait if the last programmed duty cycle is 0 or
> > >     full period.
> > >  e) Introduce a new callback that does the waiting, e.g. a .commit
> > >     callback
> > >  e') Introduce a new callback pwm_config_sync that does wait
> > >     appropriatly, keep pwm_config as is.
> > >  f) Make the i.MX28 driver switch the pin the gpio mode with the
> > >     expected output level in pwm_disable.
> > > [...]
> > > My series implemented a'), but Thierry didn't like it. I think my
> > > favorite is e').
> > > 
> > 
> > I think I would actually prefer d'. Doing the synchronisation in d' is
> > abstracting it for the user.
> 
> My preference would be simply d). Mostly because I don't like special
> cases and especially because I don't see an advantage in special casing
> 0 and full period duty cycles. That doesn't mean of course that drivers
> can't special case if they really want or have to.
Well d' is just an optimisation, because if you have a duty cycle
between 0 and full (exclusive) and no other means of syncronisation
you cannot influence where pwm_disable stops the output, at low or high.

Do we have cases where pwm_disable makes the pin high-z? Or something
else which violates the assumption "pwm_config(pwm, 0, period);
pwm_disable(pwm); makes the pin 0"?
 
> That said I've also been thinking about adding support for e), which
> would allow atomically changing the duty cycle, period and polarity of a
> channel. This might become necessary at some point.
On i.MX28 you cannot change atomically, only program atomically. The
current period will end regularily anyhow as programmed before. But
maybe that's what you mean?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] pwm: mxs: set pwm_chip can_sleep flag

[Stefan Wahren]

Hi,

Am 09.04.2014 16:35, schrieb Uwe Kleine-K?nig:
> Hello,
>
> On Wed, Apr 09, 2014 at 12:46:53PM +0200, Thierry Reding wrote:
>>
>> My preference would be simply d). Mostly because I don't like special
>> cases and especially because I don't see an advantage in special casing
>> 0 and full period duty cycles. That doesn't mean of course that drivers
>> can't special case if they really want or have to.
> Well d' is just an optimisation, because if you have a duty cycle
> between 0 and full (exclusive) and no other means of syncronisation
> you cannot influence where pwm_disable stops the output, at low or high.
>
> Do we have cases where pwm_disable makes the pin high-z? Or something
> else which violates the assumption "pwm_config(pwm, 0, period);
> pwm_disable(pwm); makes the pin 0"?
>  
>> That said I've also been thinking about adding support for e), which
>> would allow atomically changing the duty cycle, period and polarity of a
>> channel. This might become necessary at some point.
> On i.MX28 you cannot change atomically, only program atomically. The
> current period will end regularily anyhow as programmed before. But
> maybe that's what you mean?!
>
> Best regards
> Uwe
>

i want to ask gently, if somebody working on this issue?

Regards,
Stefan Wahren