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

[Jitao Shi <jitao.shi@mediatek.com>]

[-- Attachment #1: Type: text/plain, Size: 4247 bytes --]

Hi,

Thanks for your review.

On Sun, 2021-06-06 at 23:11 +0200, Uwe Kleine-König wrote:

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

I'll fix them next verison.


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

I'll fix them next verison.


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


I'll fix them next verison.


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


I'll fix them next verison.


> >  	return 0;
> >  }
> 
> Best regards
> Uwe
> 



[-- Attachment #2: Type: text/html, Size: 7532 bytes --]