| Message ID | 1459368249-13241-35-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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.
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
On Thu, Mar 31, 2016 at 08:57:35AM +0200, Boris Brezillon wrote: > 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. It's usually not possible to do really atomic updates with PWM hardware. The idea is merely that we should be able to submit one request and the framework (and drivers) will be responsible for making sure it is applied as a whole or not at all. With the legacy API it is possible for users to set the duty cycle and period, but then fail to enable/disable the PWM. pwm_apply_state() reporting success should indicate that the hardware state is now what software wanted it to be. That kind of implies that the application is serialized. This doesn't imply that hardware state won't change between a call to pwm_get_state() and pwm_apply_state(), though technically this is what will usually happen because PWM devices are exclusively used by a single user. Users are responsible for synchronizing accesses within their own code. > 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. Yes, it should be the PWM framework's job to check for changes in state and discard no-ops. Thierry
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;
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(-)