| Message ID | 1459368249-13241-23-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Boris & Heiko, 在 2016/3/31 4:03, Boris BREZILLON 写道: > + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ > + pwm_get_state(pc->chip.pwms, &pstate); > + if (!pstate.enabled) > + clk_disable(pc->clk); We found a issue recently, if the pwm0 is not enabled at uboot and pwm2 is enabled at uboot, the PWM clock will be disabled at pwm0's probe. It is true to close the pwm clock, and the pwm2 can't work during a while, until the pwm2 probe, because the pwm0 and pwm2 are the same clock for their work. In fact, the pwm0 can not know the pwm2's status. So we need to get all the PWMs state in a public place where it's early than the PWM probe, if that's the way it is. Then keep the PWM clk enabled if theis is one PWM appears to be up and running. The place maybe in the clock core init, like adding pwm clock as critial one. Another way is that we don't enable pwm clock firstly at PWM probe, because whether or not the PWM state has been enabled in the Uboot, like other modules, our chip default PWM clock registers are enabled at the beginning, read the PWM registers directly to know the PWM state. Then if the PWM state is enabled, call the enable_clk(pc->clk) to add the clock count=1. If the PWM state is disabled, we do nothing. After all the PWMs are probed and all modules are probed, the clock core will gate the PWM clock if the clock count is 0, and keep clk enabled if the clock count is not 0. How do you feel about it?
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index 6a1087c..5c7e79c 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -302,6 +302,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) { const struct of_device_id *id; struct rockchip_pwm_chip *pc; + struct pwm_state pstate; struct resource *r; int ret; @@ -322,7 +323,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) if (IS_ERR(pc->clk)) return PTR_ERR(pc->clk); - ret = clk_prepare(pc->clk); + ret = clk_prepare_enable(pc->clk); if (ret) return ret; @@ -345,12 +346,33 @@ static int rockchip_pwm_probe(struct platform_device *pdev) dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); } + /* Keep the PWM clk enabled if the PWM appears to be up and running. */ + pwm_get_state(pc->chip.pwms, &pstate); + if (!pstate.enabled) + clk_disable(pc->clk); + return ret; } static int rockchip_pwm_remove(struct platform_device *pdev) { struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev); + struct pwm_state pstate; + + /* + * Disable the PWM clk before unpreparing it if the PWM device is still + * running. This should only happen when the last PWM user left it + * enabled, or when nobody requested a PWM that was previously enabled + * by the bootloader. + * + * FIXME: Maybe the core should disable all PWM devices in + * pwmchip_remove(). In this case we'd only have to call + * clk_unprepare() after pwmchip_remove(). + * + */ + pwm_get_state(pc->chip.pwms, &pstate); + if (pstate.enabled) + clk_disable(pc->clk); clk_unprepare(pc->clk);
The current logic will disable the PWM clk even if the PWM was left enabled by the bootloader (because it's controlling a critical device like a regulator for example). Keep the PWM clk enabled if the PWM is enabled to avoid any glitches. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/pwm/pwm-rockchip.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)