On Tue, Apr 06, 2021 at 05:57:42PM +0800, Rex-BC Chen wrote: > implement get_state function for pwm-mtk-disp > > Signed-off-by: Rex-BC Chen <rex-bc.c...@mediatek.com> > Signed-off-by: Jitao Shi <jitao....@mediatek.com>
Ideally you S-o-b line is the last one to show the order in which this
patch went from one person to another.
> ---
> drivers/pwm/pwm-mtk-disp.c | 46 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 502228adf718..166e0a8ca703 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -179,8 +179,54 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip,
> struct pwm_device *pwm,
> return mtk_disp_pwm_enable(chip, state);
> }
>
> +static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> + u32 clk_div, period, high_width, con0, con1;
> + u64 rate;
> + int err;
> +
> + 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 = 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);
As before: %pe please
> + return;
> + }
> +
> + rate = clk_get_rate(mdp->clk_main);
> +
> + con0 = readl(mdp->base + mdp->data->con0);
> + con1 = readl(mdp->base + mdp->data->con1);
> +
> + state->polarity = con0 & PWM_POLARITY ?
> + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> + state->enabled = !!(con0 & BIT(0));
> +
> + clk_div = (con0 & PWM_CLKDIV_MASK) >> PWM_CLKDIV_SHIFT;
> + period = con1 & PWM_PERIOD_MASK;
> + state->period = div_u64(period * (clk_div + 1) * NSEC_PER_SEC, rate);
> + high_width = (con1 & PWM_HIGH_WIDTH_MASK) >> PWM_HIGH_WIDTH_SHIFT;
> + state->duty_cycle = div_u64(high_width * (clk_div + 1) * NSEC_PER_SEC,
> + rate);
Please round up these divisions as in .apply() we're rounding down.
Otherwise .get_state isn't the inverse of .apply().
> +
> + if (!state->enabled) {
> + clk_disable_unprepare(mdp->clk_mm);
> + clk_disable_unprepare(mdp->clk_main);
That's wrong, you enabled unconditionally so you should also disable
unconditionally. If you want to prevent that clocks of a running PWM are
disabled by the unused clk initcall, this must be done in .probe() to be
sure it runs exactly once and early enough.
> + }
> +
> + mdp->enabled = state->enabled;
> +}
> +
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
