| Message ID | 1617703062-4251-4-git-send-email-rex-bc.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert the mtk_disp driver to aotmic API | expand |
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
On Tue, Apr 06, 2021 at 12:27:56PM +0200, Uwe Kleine-König wrote: > 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 According to the documentation %pe only works on pointers for which IS_ERR() is true, so I'm not sure it can be used with plain integer error codes. Looks like there's a bunch of drivers that will do %pe and then use ERR_PTR(err) to make this work, but to be honest, that seems like jumping through hoops. But I don't feel strongly either way, so choose whichever you want. Thierry
On Fri, Apr 09, 2021 at 02:24:43PM +0200, Thierry Reding wrote: > On Tue, Apr 06, 2021 at 12:27:56PM +0200, Uwe Kleine-König wrote: > > 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 > > According to the documentation %pe only works on pointers for which > IS_ERR() is true, so I'm not sure it can be used with plain integer > error codes. It cannot. > Looks like there's a bunch of drivers that will do %pe and then use > ERR_PTR(err) to make this work, but to be honest, that seems like > jumping through hoops. When I suggested to implement %dE to print error codes this was shot down by the printk guys who's position is that %pe has to be good enough for everybody. And yes, you'd need to pass ERR_PTR(err) then. Best regards Uwe
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); + 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); + + if (!state->enabled) { + clk_disable_unprepare(mdp->clk_mm); + clk_disable_unprepare(mdp->clk_main); + } + + mdp->enabled = state->enabled; +} + static const struct pwm_ops mtk_disp_pwm_ops = { .apply = mtk_disp_pwm_apply, + .get_state = mtk_disp_pwm_get_state, .owner = THIS_MODULE, };