Messages in this thread | | | Date | Tue, 6 Apr 2021 12:19:36 +0200 | From | Uwe Kleine-König <> | Subject | Re: [v3,PATCH 2/3] pwm: mtk_disp: convert the driver to atomic API |
| |
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/ | [unhandled content-type:application/pgp-signature] | |