Re: [PATCH v4 1/3] pwm: mtk-disp: adjust the clocks to avoid them mismatch

From: Uwe Kleine-König
Date: Sun Jun 06 2021 - 17:16:19 EST


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()/

> Add the clk_prepare_enable() in config().
> Add the clk_disable_unprepare() in disable().
>
> Signed-off-by: Jitao Shi <jitao.shi@xxxxxxxxxxxx>
> ---
> 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.

> + }
> + 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.

> + 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/ |

Attachment: signature.asc
Description: PGP signature