Hello,

On Tue, Apr 06, 2021 at 05:57:41PM +0800, Rex-BC Chen wrote:
> @@ -84,33 +86,47 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
>  	 * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
>  	 */
> +	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);

Please use %pe, as this yields better readable error messages.

Also it might be sensible to first use the fact that (without patch 1
from this series) the clocks are always on and then rework the clk usage
in a separate patch.

> +			return err;
> +		}
> +		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;
> +		}
> +	}
>  	rate = clk_get_rate(mdp->clk_main);
> -	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> +	clk_div = div_u64(rate * state->period, NSEC_PER_SEC) >>
>  			  PWM_PERIOD_BIT_WIDTH;

rate * state->period might overflow, it would be great if this could be
catched. (But I don't consider this a stopper for this series.)

> -	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);

rate is an u64, %d isn't the right format for it. Doesn't this result in
a compiler warning?

> +		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);
> +	period = div64_u64(rate * state->period, div);
>  	if (period > 0)
>  		period--;
>  
> -	high_width = div64_u64(rate * duty_ns, div);
> +	high_width = div64_u64(rate * state->duty_cycle, div);
>  	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> -
> -	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;
> -	}
> +	polarity = 0;
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		polarity = PWM_POLARITY;

I'm unsure if support for polarity should be added en passant in this
patch. Maybe it would be clearer to add is separately.

>  	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
>  				 PWM_CLKDIV_MASK,
>  				 clk_div << PWM_CLKDIV_SHIFT);
> +	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> +				 PWM_POLARITY, polarity);
>  	mtk_disp_pwm_update_bits(mdp, mdp->data->con1,
>  				 PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK,
>  				 value);

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |