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