Skip to contentSkip to conversation

pwm fixes for 3.8

PATCH Superseded Tony Prisk 3 participants 5 messages Nov 19, 2012

Subsystems (2)
PWM Arm/VT8500 Arm architecture

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Tony Prisk

author
#0 lore raw

v2 cover pwm fixes for 3.8

Hi Theirry,

As discussed, here is the patch to correct the issue created by enabling disabling the pwm clock during pwm_enable/_disable. A second patch is including which fixes a build error introduced in the same patch.

These should be applied on top of the previous patch.

v2: Add error checking for clk_enable()

Tony Prisk (2):
Fix build error in pwm-vt8500.c
pwm: vt8500: Ensure pwm clock is enabled during pwm_config

 drivers/pwm/pwm-vt8500.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH v2 1/2 Fix build error in pwm-vt8500.c

A missing '{' causes a build error in pwm-vt8500.c

Signed-off-by Tony Prisk <[email protected]>
---
 drivers/pwm/pwm-vt8500.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pwm/pwm-vt8500.c +1 -1
index 970b0c6..806f72c 100644
@@ -100,7 +100,7 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
 
 	err = clk_enable(vt8500->clk);
-	if (err < 0)
+	if (err < 0) {
 		dev_err(chip->dev, "failed to enable clock\n");
 		return err;
 	};
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH v2 1/2 Fix build error in pwm-vt8500.c

Hello.

Quoted 12 lines · Tony Prisk

A missing '{' causes a build error in pwm-vt8500.c

Signed-off-by Tony Prisk <[email protected]>
---
  drivers/pwm/pwm-vt8500.c |    2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pwm/pwm-vt8500.c +0 -0
index 970b0c6..806f72c 100644

drivers/pwm/pwm-vt8500.c static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) · line 106

Quoted 8 lines
drivers/pwm/pwm-vt8500.c +1 -1
@@ -100,7 +100,7 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
  	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
 
  	err = clk_enable(vt8500->clk);
-	if (err < 0)
+	if (err < 0) {
  		dev_err(chip->dev, "failed to enable clock\n");
  		return err;
  	};

Could as well remove the unneeded semicolon here.

WBR, Sergei

author
#3 lore raw

PATCH v2 2/2 pwm: vt8500: Ensure pwm clock is enabled during pwm_config

In reply to Tony Prisk

This patch corrects a bug reported by Peter Vasil.

When all PWMs are disabled, PWM module may be disabled during calls to pwm_config. This patch enables/disables the clock in pwm_config to ensure the module is active before register read/ writes.

Signed-off-by Tony Prisk <[email protected]>
Tested-by Peter Vasil <[email protected]>
---
 drivers/pwm/pwm-vt8500.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/pwm/pwm-vt8500.c +11 -1
index 806f72c..9fdb70d 100644
@@ -62,6 +62,13 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
+	int err;
+
+	err = clk_enable(vt8500->clk);
+	if (err < 0) {
+		dev_err(chip->dev, "failed to enable clock in pwm_config()\n");
+		return err;
+	};
 
 	c = clk_get_rate(vt8500->clk);
 	c = c * period_ns;

@@ -75,8 +82,10 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (pv > 4095)
 		pv = 4095;
 
-	if (prescale > 1023)
+	if (prescale > 1023) {
+		clk_disable(vt8500->clk);
 		return -EINVAL;
+	}
 
 	c = (unsigned long long)pv * duty_ns;
 	do_div(c, period_ns);

@@ -91,6 +100,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 3));
 	writel(dc, vt8500->base + 0xc + (pwm->hwpwm << 4));
 
+	clk_disable(vt8500->clk);
 	return 0;
 }
Signature · 1 line
1.7.9.5
#4 lore raw

PATCH v2 2/2 pwm: vt8500: Ensure pwm clock is enabled during pwm_config

Quoted 18 lines · Tony Prisk

This patch corrects a bug reported by Peter Vasil.

When all PWMs are disabled, PWM module may be disabled during calls to pwm_config. This patch enables/disables the clock in pwm_config to ensure the module is active before register read/ writes.

Signed-off-by Tony Prisk <[email protected]>
Tested-by Peter Vasil <[email protected]>
---
 drivers/pwm/pwm-vt8500.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/pwm/pwm-vt8500.c +0 -0
index 806f72c..9fdb70d 100644

drivers/pwm/pwm-vt8500.c static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, · line 71

Quoted 10 lines
drivers/pwm/pwm-vt8500.c +7 -0
@@ -62,6 +62,13 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
 	unsigned long long c;
 	unsigned long period_cycles, prescale, pv, dc;
+	int err;
+
+	err = clk_enable(vt8500->clk);
+	if (err < 0) {
+		dev_err(chip->dev, "failed to enable clock in pwm_config()\n");
+		return err;
+	};

I'm not entirely happy about this error message. What bugs me about it is that the clock doesn't fail to enable in pwm_config() but rather in vt8500_pwm_config(), so it might be slightly misleading. I'm not sure if the failure location is needed at all. "failed to enable clock" sounds useful enough. Furthermore I think that if it fails here it will likely also fail elsewhere.

If you really absolutely want to specify where clk_enable() failed, then I'd be slightly happier with "%s(): failed to enable clock", __func__.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121119/4bb0c7a6/attachment.sig>