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

Groups > linux.kernel > #1470307 > unrolled thread

[PATCHv2 0/2] pwm: sunxi: give the pwm IP block more time

Back to article view | Back to linux.kernel

Contents

  [PATCHv2 0/2] pwm: sunxi: give the pwm IP block more time Olliver Schinagl <oliver@schinagl.nl> - 2016-08-25 20:00 +0200
    [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable Olliver Schinagl <oliver@schinagl.nl> - 2016-08-25 20:00 +0200
      Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before  disable Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-08-27 00:20 +0200
        Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse  before disable Olliver Schinagl <oliver@schinagl.nl> - 2016-09-06 09:20 +0200
          Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before  disable Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-09-06 22:00 +0200
            Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse  before disable Olliver Schinagl <oliver@schinagl.nl> - 2016-09-09 11:10 +0200
    [PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to become ready Olliver Schinagl <oliver@schinagl.nl> - 2016-08-25 20:00 +0200
      Re: [PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to  become ready Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-08-27 00:30 +0200

#1470307 — [PATCHv2 0/2] pwm: sunxi: give the pwm IP block more time

Hi all,

last year I've sent out this patch series and there was little technical
discussion, so hopefully we can rekindle it.

This patch series fixes 2 issues. For one, we add a delay between disabling
the PWM hardware block and removing the clock, this to yield the PWM hardware
time to actually finish it's PWM and finish in the proper state. If we do not
do this, the output may stay high and may overload speakers for example.

The other issue might need a little more discussion. It also yields the PWM
hardware some extra time. When changing the config, we enable the clock
together with the parameters for the PWM. Sometimes however, the PWM IP core
has not had the time yet to actually set the ready bit. By first enabling
the gate, and then change things, the IP has time to do its thing. The PWM
block has probably done its thing even after the writel has returned. So an
extra delay (of one bus clock cycle?) is overkill.

Changes since v1:
	- Split patch series into several smaller patch series
	- Added driver author

Olliver Schinagl (2):
  pwm: sunxi: allow the pwm to finish its pulse before disable
  pwm: sunxi: Yield some time to the pwm-block to become ready

 drivers/pwm/pwm-sun4i.c | 54 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 18 deletions(-)

-- 
2.8.1

[toc] | [next] | [standalone]

#1470312 — [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

When we inform the PWM block to stop toggeling the output, we may end up
in a state where the output is not what we would expect (e.g. not the
low-pulse) but whatever the output was at when the clock got disabled.

To counter this we have to wait for maximally the time of one whole
period to ensure the pwm hardware was able to finish. Since we already
told the PWM hardware to disable it self, it will not continue toggling
but merly finish its current pulse.

If a whole period is considered to much, it may be contemplated to use a
half period + a little bit to ensure we get passed the transition.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/pwm-sun4i.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 03a99a5..5e97c8a 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -8,6 +8,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
+
+	/* Allow for the PWM hardware to finish its last toggle. The pulse
+	 * may have just started and thus we should wait a full period.
+	 */
+	ndelay(pwm_get_period(pwm));
+
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 	spin_unlock(&sun4i_pwm->ctrl_lock);
-- 
2.8.1

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

#1471016 — Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Multipart message — attachments visible in raw view] — view raw

On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> When we inform the PWM block to stop toggeling the output, we may end up
> in a state where the output is not what we would expect (e.g. not the
> low-pulse) but whatever the output was at when the clock got disabled.
> 
> To counter this we have to wait for maximally the time of one whole
> period to ensure the pwm hardware was able to finish. Since we already
> told the PWM hardware to disable it self, it will not continue toggling
> but merly finish its current pulse.
> 
> If a whole period is considered to much, it may be contemplated to use a
> half period + a little bit to ensure we get passed the transition.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 03a99a5..5e97c8a 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	spin_lock(&sun4i_pwm->ctrl_lock);
>  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +	spin_unlock(&sun4i_pwm->ctrl_lock);
> +
> +	/* Allow for the PWM hardware to finish its last toggle. The pulse
> +	 * may have just started and thus we should wait a full period.
> +	 */
> +	ndelay(pwm_get_period(pwm));

Can't that use the ready bit as well?

Maxime

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

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

#1477112 — Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Multipart message — attachments visible in raw view] — view raw

Hi Maxime!,

On za, 2016-08-27 at 00:19 +0200, Maxime Ripard wrote:
> On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > 
> > When we inform the PWM block to stop toggeling the output, we may
> > end up
> > in a state where the output is not what we would expect (e.g. not
> > the
> > low-pulse) but whatever the output was at when the clock got
> > disabled.
> > 
> > To counter this we have to wait for maximally the time of one whole
> > period to ensure the pwm hardware was able to finish. Since we
> > already
> > told the PWM hardware to disable it self, it will not continue
> > toggling
> > but merly finish its current pulse.
> > 
> > If a whole period is considered to much, it may be contemplated to
> > use a
> > half period + a little bit to ensure we get passed the transition.
> > 
> > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> > ---
> >  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 03a99a5..5e97c8a 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -8,6 +8,7 @@
> >  
> >  #include <linux/bitops.h>
> >  #include <linux/clk.h>
> > +#include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip
> > *chip, struct pwm_device *pwm)
> >  	spin_lock(&sun4i_pwm->ctrl_lock);
> >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > +
> > +	/* Allow for the PWM hardware to finish its last toggle.
> > The pulse
> > +	 * may have just started and thus we should wait a full
> > period.
> > +	 */
> > +	ndelay(pwm_get_period(pwm));
> 
> Can't that use the ready bit as well?
It depends whatever is cheaper. If we disable the pwm, we have to
commit that request to hardware first. Then we have to read back the
has ready and in the strange situation it is not, wait for it to become
ready?

Also, that would mean we would loop in a spin lock, or keep
setting/clearing an additional spinlock to read the ready bit.

If that is cheaper then an ndelay, I can rewrite it of course, and
assuming the 'ready' bit gets set from disabeling the PWM. It needs to
be investigated if disabeling the PWM, the ready bit is used.q

> 
> Maxime
> 

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

#1477791 — Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 06, 2016 at 09:12:56AM +0200, Olliver Schinagl wrote:
> Hi Maxime!,
> 
> On za, 2016-08-27 at 00:19 +0200, Maxime Ripard wrote:
> > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > 
> > > When we inform the PWM block to stop toggeling the output, we may
> > > end up
> > > in a state where the output is not what we would expect (e.g. not
> > > the
> > > low-pulse) but whatever the output was at when the clock got
> > > disabled.
> > > 
> > > To counter this we have to wait for maximally the time of one whole
> > > period to ensure the pwm hardware was able to finish. Since we
> > > already
> > > told the PWM hardware to disable it self, it will not continue
> > > toggling
> > > but merly finish its current pulse.
> > > 
> > > If a whole period is considered to much, it may be contemplated to
> > > use a
> > > half period + a little bit to ensure we get passed the transition.
> > > 
> > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> > > ---
> > >  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 03a99a5..5e97c8a 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -8,6 +8,7 @@
> > >  
> > >  #include <linux/bitops.h>
> > >  #include <linux/clk.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/err.h>
> > >  #include <linux/io.h>
> > >  #include <linux/module.h>
> > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct pwm_chip
> > > *chip, struct pwm_device *pwm)
> > >  	spin_lock(&sun4i_pwm->ctrl_lock);
> > >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > +
> > > +	/* Allow for the PWM hardware to finish its last toggle.
> > > The pulse
> > > +	 * may have just started and thus we should wait a full
> > > period.
> > > +	 */
> > > +	ndelay(pwm_get_period(pwm));
> > 
> > Can't that use the ready bit as well?
> It depends whatever is cheaper. If we disable the pwm, we have to
> commit that request to hardware first. Then we have to read back the
> has ready and in the strange situation it is not, wait for it to become
> ready?

If it works like you were suggesting, yes.

> Also, that would mean we would loop in a spin lock, or keep
> setting/clearing an additional spinlock to read the ready bit.

You're using a spin_lock, so it's not that bad, but I was just
suggesting replacing the ndelay.

Maxime

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

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

#1479766 — Re: [PATCH 1/2] pwm: sunxi: allow the pwm to finish its pulse before disable

[Multipart message — attachments visible in raw view] — view raw

On di, 2016-09-06 at 21:51 +0200, Maxime Ripard wrote:
> On Tue, Sep 06, 2016 at 09:12:56AM +0200, Olliver Schinagl wrote:
> > 
> > Hi Maxime!,
> > 
> > On za, 2016-08-27 at 00:19 +0200, Maxime Ripard wrote:
> > > 
> > > On Thu, Aug 25, 2016 at 07:50:10PM +0200, Olliver Schinagl wrote:
> > > > 
> > > > 
> > > > When we inform the PWM block to stop toggeling the output, we
> > > > may
> > > > end up
> > > > in a state where the output is not what we would expect (e.g.
> > > > not
> > > > the
> > > > low-pulse) but whatever the output was at when the clock got
> > > > disabled.
> > > > 
> > > > To counter this we have to wait for maximally the time of one
> > > > whole
> > > > period to ensure the pwm hardware was able to finish. Since we
> > > > already
> > > > told the PWM hardware to disable it self, it will not continue
> > > > toggling
> > > > but merly finish its current pulse.
> > > > 
> > > > If a whole period is considered to much, it may be contemplated
> > > > to
> > > > use a
> > > > half period + a little bit to ensure we get passed the
> > > > transition.
> > > > 
> > > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> > > > ---
> > > >  drivers/pwm/pwm-sun4i.c | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index 03a99a5..5e97c8a 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -8,6 +8,7 @@
> > > >  
> > > >  #include <linux/bitops.h>
> > > >  #include <linux/clk.h>
> > > > +#include <linux/delay.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/module.h>
> > > > @@ -245,6 +246,16 @@ static void sun4i_pwm_disable(struct
> > > > pwm_chip
> > > > *chip, struct pwm_device *pwm)
> > > >  	spin_lock(&sun4i_pwm->ctrl_lock);
> > > >  	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > >  	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
> > > > +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> > > > +	spin_unlock(&sun4i_pwm->ctrl_lock);
> > > > +
> > > > +	/* Allow for the PWM hardware to finish its last
> > > > toggle.
> > > > The pulse
> > > > +	 * may have just started and thus we should wait a
> > > > full
> > > > period.
> > > > +	 */
> > > > +	ndelay(pwm_get_period(pwm));
> > > 
> > > Can't that use the ready bit as well?
> > It depends whatever is cheaper. If we disable the pwm, we have to
> > commit that request to hardware first. Then we have to read back
> > the
> > has ready and in the strange situation it is not, wait for it to
> > become
> > ready?
> 
> If it works like you were suggesting, yes.
> 
> > 
> > Also, that would mean we would loop in a spin lock, or keep
> > setting/clearing an additional spinlock to read the ready bit.
> 
> You're using a spin_lock, so it's not that bad, but I was just
> suggesting replacing the ndelay.
If you say the spin_lock + wait for the ready is just as expensive as
the ndelay, or the ndelay is less preferred, then I gladly make the
change; but I think we need the ndelay for the else where we do not
have the ready flag (A10 or A13 iirc?)

Olliver

> 
> Maxime
> 

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

#1470313 — [PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to become ready

From: Olliver Schinagl <o.schinagl@ultimaker.com>

The pwm-block of some of the sunxi chips feature a 'ready' flag to
indicate the software that it is ready for new commands.

Right now, when we call pwm_config and set the period, we write the
values to the registers, and turn off the clock to the IP. Because of
this, the hardware does not have time to configure the hardware and set
the 'ready' flag.

By running the clock just before making new changes and before checking
if the hardware is ready, the hardware has time to reconfigure itself
and set the clear the flag appropriately.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
---
 drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5e97c8a..dd198c3 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -105,6 +105,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
 	int err;
+	int ret = 0;
+
+	/* Let the PWM hardware run before making any changes. We do this to
+	 * allow the hardware to have some time to clear the 'ready' flag.
+	 */
+	err = clk_prepare_enable(sun4i_pwm->clk);
+	if (err) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return err;
+	}
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_rate = clk_get_rate(sun4i_pwm->clk);
 
@@ -137,7 +153,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		if (div - 1 > PWM_PRD_MASK) {
 			dev_err(chip->dev, "period exceeds the maximum value\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			spin_lock(&sun4i_pwm->ctrl_lock);
+			goto out;
 		}
 	}
 
@@ -146,26 +164,14 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(div, period_ns);
 	dty = div;
 
-	err = clk_prepare_enable(sun4i_pwm->clk);
-	if (err) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return err;
-	}
-
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-
 	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
-		spin_unlock(&sun4i_pwm->ctrl_lock);
-		clk_disable_unprepare(sun4i_pwm->clk);
-		return -EBUSY;
-	}
-
-	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	if (clk_gate) {
-		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		ret = -EBUSY;
+		goto out;
 	}
+	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
@@ -175,6 +181,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
 
+out:
 	if (clk_gate) {
 		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 		val |= clk_gate;
@@ -184,7 +191,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
 
-	return 0;
+	return ret;
 }
 
 static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.8.1

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

#1471022 — Re: [PATCH 2/2] pwm: sunxi: Yield some time to the pwm-block to become ready

[Multipart message — attachments visible in raw view] — view raw

Hi,

On Thu, Aug 25, 2016 at 07:50:11PM +0200, Olliver Schinagl wrote:
> From: Olliver Schinagl <o.schinagl@ultimaker.com>
> 
> The pwm-block of some of the sunxi chips feature a 'ready' flag to
> indicate the software that it is ready for new commands.
> 
> Right now, when we call pwm_config and set the period, we write the
> values to the registers, and turn off the clock to the IP. Because of
> this, the hardware does not have time to configure the hardware and set
> the 'ready' flag.
> 
> By running the clock just before making new changes and before checking
> if the hardware is ready, the hardware has time to reconfigure itself
> and set the clear the flag appropriately.
> 
> Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 5e97c8a..dd198c3 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -105,6 +105,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	u64 clk_rate, div = 0;
>  	unsigned int prescaler = 0;
>  	int err;
> +	int ret = 0;
> +
> +	/* Let the PWM hardware run before making any changes. We do this to
> +	 * allow the hardware to have some time to clear the 'ready' flag.
> +	 */

This is not the proper comment style.

> +	err = clk_prepare_enable(sun4i_pwm->clk);
> +	if (err) {
> +		dev_err(chip->dev, "failed to enable PWM clock\n");
> +		return err;
> +	}

New line please.

> +	spin_lock(&sun4i_pwm->ctrl_lock);
> +	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> +	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);

What are you doing here? You clear a bit, and then put the same one
back in?

> +	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +	spin_unlock(&sun4i_pwm->ctrl_lock);
>  
>  	clk_rate = clk_get_rate(sun4i_pwm->clk);
>  
> @@ -137,7 +153,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  		if (div - 1 > PWM_PRD_MASK) {
>  			dev_err(chip->dev, "period exceeds the maximum value\n");
> -			return -EINVAL;
> +			ret = -EINVAL;
> +			spin_lock(&sun4i_pwm->ctrl_lock);

Uh? That's really suspicious. And even if right, please don't do that,
this is just really bad for the comprehension of the workflow.

Maxime

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web