lkml.org 
[lkml]   [2021]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state()
    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.chen@mediatek.com>
    > Signed-off-by: Jitao Shi <jitao.shi@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/ |
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2021-04-06 12:29    [W:2.480 / U:0.332 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site