Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1367566 > unrolled thread

[PATCH v5 34/46] clk: pwm: switch to the atomic API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v5 34/46] clk: pwm: switch to the atomic API Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-03-30 22:20 +0200
    Re: [PATCH v5 34/46] clk: pwm: switch to the atomic API Stephen Boyd <sboyd@codeaurora.org> - 2016-03-31 00:10 +0200
      Re: [PATCH v5 34/46] clk: pwm: switch to the atomic API Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-03-31 09:00 +0200

#1367566 — [PATCH v5 34/46] clk: pwm: switch to the atomic API

pwm_config/enable/disable() have been deprecated and should be replaced
by pwm_apply_state().

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/clk/clk-pwm.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index ebcd738..49ec5b1 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -28,15 +28,29 @@ static inline struct clk_pwm *to_clk_pwm(struct clk_hw *hw)
 static int clk_pwm_prepare(struct clk_hw *hw)
 {
 	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
+	struct pwm_state pstate;
 
-	return pwm_enable(clk_pwm->pwm);
+	pwm_get_state(clk_pwm->pwm, &pstate);
+	if (pstate.enabled)
+		return 0;
+
+	pstate.enabled = true;
+
+	return pwm_apply_state(clk_pwm->pwm, &pstate);
 }
 
 static void clk_pwm_unprepare(struct clk_hw *hw)
 {
 	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
+	struct pwm_state pstate;
+
+	pwm_get_state(clk_pwm->pwm, &pstate);
+	if (!pstate.enabled)
+		return;
 
-	pwm_disable(clk_pwm->pwm);
+	pstate.enabled = false;
+
+	pwm_apply_state(clk_pwm->pwm, &pstate);
 }
 
 static unsigned long clk_pwm_recalc_rate(struct clk_hw *hw,
@@ -56,6 +70,7 @@ static const struct clk_ops clk_pwm_ops = {
 static int clk_pwm_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
+	struct pwm_state pstate;
 	struct pwm_args pargs = { };
 	struct clk_init_data init;
 	struct clk_pwm *clk_pwm;
@@ -88,7 +103,12 @@ static int clk_pwm_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	ret = pwm_config(pwm, (pargs.period + 1) >> 1, pargs.period);
+	pwm_get_state(pwm, &pstate);
+	pstate.period = pargs.period;
+	pstate.polarity = pargs.polarity;
+	pstate.duty_cycle = (pargs.period + 1) >> 1;
+
+	ret = pwm_apply_state(pwm, &pstate);
 	if (ret < 0)
 		return ret;
 
-- 
2.5.0

[toc] | [next] | [standalone]

#1367634

On 03/30, Boris Brezillon wrote:
> diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> index ebcd738..49ec5b1 100644
> --- a/drivers/clk/clk-pwm.c
> +++ b/drivers/clk/clk-pwm.c
> @@ -28,15 +28,29 @@ static inline struct clk_pwm *to_clk_pwm(struct clk_hw *hw)
>  static int clk_pwm_prepare(struct clk_hw *hw)
>  {
>  	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
> +	struct pwm_state pstate;
>  
> -	return pwm_enable(clk_pwm->pwm);
> +	pwm_get_state(clk_pwm->pwm, &pstate);
> +	if (pstate.enabled)
> +		return 0;
> +
> +	pstate.enabled = true;
> +
> +	return pwm_apply_state(clk_pwm->pwm, &pstate);

This doesn't seem atomic anymore if we're checking the state and
then not calling apply_state if it's already enabled. But I
assume this doesn't matter because we "own" the pwm here?
Otherwise I would think this would be unconditional apply state
and duplicates would be ignored in the pwm framework.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1367872

Hi Stephen,

On Wed, 30 Mar 2016 15:01:49 -0700
Stephen Boyd <sboyd@codeaurora.org> wrote:

> On 03/30, Boris Brezillon wrote:
> > diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
> > index ebcd738..49ec5b1 100644
> > --- a/drivers/clk/clk-pwm.c
> > +++ b/drivers/clk/clk-pwm.c
> > @@ -28,15 +28,29 @@ static inline struct clk_pwm *to_clk_pwm(struct clk_hw *hw)
> >  static int clk_pwm_prepare(struct clk_hw *hw)
> >  {
> >  	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
> > +	struct pwm_state pstate;
> >  
> > -	return pwm_enable(clk_pwm->pwm);
> > +	pwm_get_state(clk_pwm->pwm, &pstate);
> > +	if (pstate.enabled)
> > +		return 0;
> > +
> > +	pstate.enabled = true;
> > +
> > +	return pwm_apply_state(clk_pwm->pwm, &pstate);
> 
> This doesn't seem atomic anymore if we're checking the state and
> then not calling apply_state if it's already enabled. But I
> assume this doesn't matter because we "own" the pwm here?

Yep. Actually it's not atomic in term of concurrency (maybe the
'atomic' word is not appropriate here). Atomicity is here referring to
the fact that we're now providing all the PWM parameters in the same
request instead of splitting it in pwm_config() + pwm_enable/disable()
calls.

Concurrent accesses still have to be controlled by the PWM user (which
is already the case for this driver, thanks to the locking
infrastructure in the CCF).

> Otherwise I would think this would be unconditional apply state
> and duplicates would be ignored in the pwm framework.
> 

Yep, I'll remove the if (pstate.enabled) branch.

Thanks for your review.

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web