[PATCH] pwm: brcmstb: Don't set can_sleep flag

[PATCH] pwm: brcmstb: Don't set can_sleep flag

[Axel Lin]

The .can_sleep flag is used for some PWM controllers that can't be used
in atomic context. Not such case for this driver, so don't set the
can_sleep flag.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/pwm/pwm-brcmstb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
index 423ce08..f58039b 100644
--- a/drivers/pwm/pwm-brcmstb.c
+++ b/drivers/pwm/pwm-brcmstb.c
@@ -270,7 +270,6 @@ static int brcmstb_pwm_probe(struct platform_device *pdev)
 	p->chip.ops = &brcmstb_pwm_ops;
 	p->chip.base = -1;
 	p->chip.npwm = 2;
-	p->chip.can_sleep = true;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	p->base = devm_ioremap_resource(&pdev->dev, res);
-- 
2.1.4

Re: [PATCH] pwm: brcmstb: Don't set can_sleep flag

[Florian Fainelli]

On 03/11/15 02:08, Axel Lin wrote:
> The .can_sleep flag is used for some PWM controllers that can't be used
> in atomic context. Not such case for this driver, so don't set the
> can_sleep flag.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks!

> ---
>  drivers/pwm/pwm-brcmstb.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
> index 423ce08..f58039b 100644
> --- a/drivers/pwm/pwm-brcmstb.c
> +++ b/drivers/pwm/pwm-brcmstb.c
> @@ -270,7 +270,6 @@ static int brcmstb_pwm_probe(struct platform_device *pdev)
>  	p->chip.ops = &brcmstb_pwm_ops;
>  	p->chip.base = -1;
>  	p->chip.npwm = 2;
> -	p->chip.can_sleep = true;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	p->base = devm_ioremap_resource(&pdev->dev, res);
> 


-- 
Florian

Re: [PATCH] pwm: brcmstb: Don't set can_sleep flag

[Brian Norris]

On Tue, Nov 03, 2015 at 06:08:40PM +0800, Axel Lin wrote:
> The .can_sleep flag is used for some PWM controllers that can't be used
> in atomic context. Not such case for this driver, so don't set the
> can_sleep flag.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>

Looks sane to me, though I've never touched this driver.

Reviewed-by: Brian Norris <computersforpeace@gmail.com>

BTW just a comment from an outsider here, the "can sleep" terminology
seems a bit misleading here. Just IMO of course, but saying something
"can sleep" sounds like a permissive statement, when actually it's a
restrictive statement being made by the driver (which is in this case
unnecessarily restrictive). The "might sleep" terminology used in one
other place would (again IMO) better communicate what I think is
intended here.

Though this problem is most likely result of mindless copy-and-paste,
it's possible that the terminology made it easier to gloss over. Or I
could just be blowing smoke.

Regards,
Brian

> ---
>  drivers/pwm/pwm-brcmstb.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-brcmstb.c b/drivers/pwm/pwm-brcmstb.c
> index 423ce08..f58039b 100644
> --- a/drivers/pwm/pwm-brcmstb.c
> +++ b/drivers/pwm/pwm-brcmstb.c
> @@ -270,7 +270,6 @@ static int brcmstb_pwm_probe(struct platform_device *pdev)
>  	p->chip.ops = &brcmstb_pwm_ops;
>  	p->chip.base = -1;
>  	p->chip.npwm = 2;
> -	p->chip.can_sleep = true;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	p->base = devm_ioremap_resource(&pdev->dev, res);
> -- 
> 2.1.4
> 
> 
> 

Re: [PATCH] pwm: brcmstb: Don't set can_sleep flag

[Florian Fainelli]

On 03/11/15 16:31, Brian Norris wrote:
> On Tue, Nov 03, 2015 at 06:08:40PM +0800, Axel Lin wrote:
>> The .can_sleep flag is used for some PWM controllers that can't be used
>> in atomic context. Not such case for this driver, so don't set the
>> can_sleep flag.
>>
>> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> 
> Looks sane to me, though I've never touched this driver.
> 
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> 
> BTW just a comment from an outsider here, the "can sleep" terminology
> seems a bit misleading here. Just IMO of course, but saying something
> "can sleep" sounds like a permissive statement, when actually it's a
> restrictive statement being made by the driver (which is in this case
> unnecessarily restrictive). The "might sleep" terminology used in one
> other place would (again IMO) better communicate what I think is
> intended here.
> 
> Though this problem is most likely result of mindless copy-and-paste,
> it's possible that the terminology made it easier to gloss over. Or I
> could just be blowing smoke.

Well, in this particular case, I have to admit this was copy and paste
which resulted in setting can_sleep initially.

Now, I do agree that the terminology is a little confusing here. If you
look at the GPIO API there are gpio_*_cansleep() accessors, which in
their case, convey the correct meaning: the operation can/might sleep.

Should we prepare a coccinelle script which fixes that at least for the
PWM subsystem?
-- 
Florian

Re: [PATCH] pwm: brcmstb: Don't set can_sleep flag

[Brian Norris]

On Tue, Nov 03, 2015 at 04:49:52PM -0800, Florian Fainelli wrote:
> Now, I do agree that the terminology is a little confusing here. If you
> look at the GPIO API there are gpio_*_cansleep() accessors, which in
> their case, convey the correct meaning: the operation can/might sleep.

Perhaps I'm misreading, but that actually sounds exactly the same as the
pwm_can_sleep() API. It sounds OK from a consumer/API perspective, but I
was just commenting on the perspective of the driver writer.

Maybe it's not worth much change, then, if several subsystems have
similar naming.

Brian