[PATCH v2] pwm: rockchip: Keep enabled PWMs running while probing

[Trent Piepho]

On Saturday, September 19, 2020 12:33:06 PM PST Simon South wrote:
> Following commit cfc4c189bc70 ("pwm: Read initial hardware state at
> request time") the Rockchip PWM driver can no longer assume a device's
> pwm_state structure has been populated after a call to pwmchip_add().
> Consequently, the test in rockchip_pwm_probe() intended to prevent the

> 
> @@ -362,7 +363,9 @@ static int rockchip_pwm_probe(struct platform_device 
*pdev)
...
        ret = pwmchip_add(&pc->chip);
...
>  	}
>  
>  	/* Keep the PWM clk enabled if the PWM appears to be up and 
running. */
> -	if (!pwm_is_enabled(pc->chip.pwms))
> +	enable_conf = pc->data->enable_conf;
> +	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
> +	if ((ctrl & enable_conf) != enable_conf)
>  		clk_disable(pc->clk);
>  

I came across this while trying to get a PBP working better.  It seems like 
the issue is the driver was calling pwm_is_enabled() without first requesting 
the pwm with pwm_get().  Which wouldn't even be possible normally, how would 
one get the pwm_chip to call pwm_is_enabled on, but the driver already has the 
pointer.

Anyway, it seems like this solution has a race.  Isn't the pwm live and 
requestable as soon as pwmchip_add() returns?  Which would mean that disabling 
the clock here could race with other code requesting and enabling the pwm.

Seems like it would be safer to check the initial state and turn off the clock 
before calling pwmchip_add.