Re: [PATCH v4 1/3] pwm: mtk-disp: adjust the clocks to avoid them mismatch
From: Uwe Kleine-König <hidden>
Date: 2021-06-06 21:12:06
Also in:
linux-arm-kernel, linux-mediatek, lkml
Hello, you missed to address Lee and me for your patch series. I just happend to stumble over this series by a fluke. On Thu, Jun 03, 2021 at 06:05:29PM +0800, Jitao Shi wrote:
The clk_main and clk_mm clocks are still on when system enter suspend. That will casue the power consumption. The clocks call the clk_prepare() in probe(), but the clk_unprepare() is called in remove(), it isn't called when system suspend.
The English could be improved here. Something like: The clks "main" and "mm" are prepared in .probe() (and unprepared in .remove()). This results in the clocks being on during suspend which results in unnecessarily increased power consumption.
Remove the clcok opterations from probe() and remove.
s/clcok/clock/, s/e\./e()/
quoted hunk ↗ jump to hunk
Add the clk_prepare_enable() in config(). Add the clk_disable_unprepare() in disable(). Signed-off-by: Jitao Shi <redacted> --- drivers/pwm/pwm-mtk-disp.c | 81 ++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 48 deletions(-)diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c index 9b3ba401a3db..b5771e2c54b8 100644 --- a/drivers/pwm/pwm-mtk-disp.c +++ b/drivers/pwm/pwm-mtk-disp.c@@ -47,6 +47,7 @@ struct mtk_disp_pwm { struct clk *clk_main; struct clk *clk_mm; void __iomem *base; + bool enabled; }; static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)@@ -74,6 +75,22 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, u64 div, rate; int err; + if (!mdp->enabled) { + err = clk_prepare_enable(mdp->clk_main); + if (err < 0) { + dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n", + err); + return err;
Please use %pe to get symbolic error names which are better understandable.
quoted hunk ↗ jump to hunk
+ } + err = clk_prepare_enable(mdp->clk_mm); + if (err < 0) { + dev_err(chip->dev, "Can't enable mdp->clk_mm: %d\n", + err); + clk_disable_unprepare(mdp->clk_main); + return err; + } + } + /* * Find period, high_width and clk_div to suit duty_ns and period_ns. * Calculate proper div value to keep period value in the bound.@@ -87,9 +104,15 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, rate = clk_get_rate(mdp->clk_main); clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >> PWM_PERIOD_BIT_WIDTH; - if (clk_div > PWM_CLKDIV_MAX) + if (clk_div > PWM_CLKDIV_MAX) { + dev_err(chip->dev, "clock rate is too high: rate = %d Hz\n", + rate);
Adding an error message here is orthogonal to this patch. Either drop it or mention it in the commit log please.
quoted hunk ↗ jump to hunk
+ if (!mdp->enabled) { + clk_disable_unprepare(mdp->clk_mm); + clk_disable_unprepare(mdp->clk_main); + } return -EINVAL; - + } div = NSEC_PER_SEC * (clk_div + 1); period = div64_u64(rate * period_ns, div); if (period > 0) [...]@@ -135,18 +145,9 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip); int err; - err = clk_enable(mdp->clk_main); - if (err < 0) - return err; - - err = clk_enable(mdp->clk_mm); - if (err < 0) { - clk_disable(mdp->clk_main); - return err; - } - mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask, mdp->data->enable_mask); + mdp->enabled = true;
The modification to .enable() looks wrong. After the PWM was disabled the clocks are off, so .enable() must reenable them, doesn't it?
return 0; }
Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachments
- signature.asc [application/pgp-signature] 488 bytes