| Message ID | cb5a28d5fde04c09e9aace6ce619e445f895e0a8.1608735481.git.simon@simonsouth.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: rockchip: Eliminate potential race condition when probing | expand |
On Wed, Dec 23, 2020 at 11:01:08AM -0500, Simon South wrote: > Currently rockchip_pwm_probe() enables the PWM clock of every device it > matches, then disables the clock unless the device itself appears to have > been enabled (by a bootloader, presumably) before the kernel started. > > Simplify this by enabling (rather, keeping enabled) the PWM clock of only > devices that are already running, as enabling the clock for any other > device during probing is unnecessary. > > Signed-off-by: Simon South <simon@simonsouth.net> > --- > drivers/pwm/pwm-rockchip.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index 80f5e69d9b8a..02da7370db70 100644 > --- a/drivers/pwm/pwm-rockchip.c > +++ b/drivers/pwm/pwm-rockchip.c > @@ -327,12 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > return ret; > } > > - ret = clk_prepare_enable(pc->clk); > - if (ret) { > - dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret); > - return ret; > - } > - This undoes stuff that you introduced in patch 1. Don't you reintroduce the problem that was fixed by this patch? > ret = clk_prepare_enable(pc->pclk); > if (ret) { > dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); > @@ -357,8 +351,19 @@ static int rockchip_pwm_probe(struct platform_device *pdev) > enable_conf = pc->data->enable_conf; > ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl); > enabled = ((ctrl & enable_conf) == enable_conf); > - if (!enabled) > - clk_disable(pc->clk); > + if (enabled) { > + ret = clk_prepare_enable(pc->clk); > + if (ret) { > + dev_err(&pdev->dev, "Can't enable PWM clk: %d\n", ret); > + return ret; > + } > + } else { > + ret = clk_prepare(pc->clk); > + if (ret) { > + dev_err(&pdev->dev, "Can't prepare PWM clk: %d\n", ret); > + return ret; > + } > + } I don't see the advantage of this patch. In my eyes the code complication doesn't justify the gain (i.e. prevent the PWM clock being enabled for a few instructions). Best regards Uwe
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes: > I don't see the advantage of this patch. In my eyes the code > complication doesn't justify the gain (i.e. prevent the PWM clock > being enabled for a few instructions). I was starting to feel the same way after I sent out this series, so let's just drop this change. Thanks for the feedback, Uwe. I'll get a v4 out shortly.
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index 80f5e69d9b8a..02da7370db70 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -327,12 +327,6 @@ static int rockchip_pwm_probe(struct platform_device *pdev) return ret; } - ret = clk_prepare_enable(pc->clk); - if (ret) { - dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret); - return ret; - } - ret = clk_prepare_enable(pc->pclk); if (ret) { dev_err(&pdev->dev, "Can't enable APB clk: %d\n", ret); @@ -357,8 +351,19 @@ static int rockchip_pwm_probe(struct platform_device *pdev) enable_conf = pc->data->enable_conf; ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl); enabled = ((ctrl & enable_conf) == enable_conf); - if (!enabled) - clk_disable(pc->clk); + if (enabled) { + ret = clk_prepare_enable(pc->clk); + if (ret) { + dev_err(&pdev->dev, "Can't enable PWM clk: %d\n", ret); + return ret; + } + } else { + ret = clk_prepare(pc->clk); + if (ret) { + dev_err(&pdev->dev, "Can't prepare PWM clk: %d\n", ret); + return ret; + } + } clk_disable(pc->pclk);
Currently rockchip_pwm_probe() enables the PWM clock of every device it matches, then disables the clock unless the device itself appears to have been enabled (by a bootloader, presumably) before the kernel started. Simplify this by enabling (rather, keeping enabled) the PWM clock of only devices that are already running, as enabling the clock for any other device during probing is unnecessary. Signed-off-by: Simon South <simon@simonsouth.net> --- drivers/pwm/pwm-rockchip.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)