diff mbox series

[v3,3/3] pwm: mtk_disp: implement .get_state()

Message ID 1617703062-4251-4-git-send-email-rex-bc.chen@mediatek.com (mailing list archive)
State New
Headers show
Series Convert the mtk_disp driver to aotmic API | expand

Commit Message

Rex-BC Chen April 6, 2021, 9:57 a.m. UTC
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>
---
 drivers/pwm/pwm-mtk-disp.c | 46 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Uwe Kleine-König April 6, 2021, 10:27 a.m. UTC | #1
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
Thierry Reding April 9, 2021, 12:24 p.m. UTC | #2
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
Uwe Kleine-König April 9, 2021, 9:52 p.m. UTC | #3
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 mbox series

Patch

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,
 };