Re: [PATCH v3] pwm: vt8500: Update vt8500 PWM driver support
On Wed, 2012-10-24 at 07:41 +0200, Thierry Reding wrote: > On Wed, Oct 24, 2012 at 04:46:58PM +1300, Tony Prisk wrote: > > This patch updates pwm-vt8500.c to support devicetree probing and > > make use of the common clock subsystem. > > > > A binding document describing the PWM controller found on > > arch-vt8500 is also included. > > > > Signed-off-by: Tony Prisk > > --- > > v2/v3: > > Fix errors/coding style as pointed out by Thierry Reding. > > > > .../devicetree/bindings/pwm/vt8500-pwm.txt | 17 > > drivers/pwm/pwm-vt8500.c | 86 > > ++-- > > 2 files changed, 80 insertions(+), 23 deletions(-) > > create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt > > Looking real good now. One last comment though and I think I'm ready to > take this... > > > + err = clk_enable(vt8500->clk); > > + if (err < 0) > > + dev_err(chip->dev, "failed to enable clock\n"); > > + return -EBUSY; > > + }; > > Why do you return EBUSY instead of err? Because I didn't notice this when I 'fixed' it - I changed the other one to return err/ret, missed this one. Will fix. > > Thierry > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3] pwm: vt8500: Update vt8500 PWM driver support
On Wed, Oct 24, 2012 at 04:46:58PM +1300, Tony Prisk wrote: > This patch updates pwm-vt8500.c to support devicetree probing and > make use of the common clock subsystem. > > A binding document describing the PWM controller found on > arch-vt8500 is also included. > > Signed-off-by: Tony Prisk > --- > v2/v3: > Fix errors/coding style as pointed out by Thierry Reding. > > .../devicetree/bindings/pwm/vt8500-pwm.txt | 17 > drivers/pwm/pwm-vt8500.c | 86 > ++-- > 2 files changed, 80 insertions(+), 23 deletions(-) > create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt Looking real good now. One last comment though and I think I'm ready to take this... > + err = clk_enable(vt8500->clk); > + if (err < 0) > + dev_err(chip->dev, "failed to enable clock\n"); > + return -EBUSY; > + }; Why do you return EBUSY instead of err? Thierry pgp6P4aMnBFoJ.pgp Description: PGP signature
Re: [PATCH v3] pwm: vt8500: Update vt8500 PWM driver support
On Wed, 2012-10-24 at 00:14 +0200, Thierry Reding wrote:
> On Tue, Oct 23, 2012 at 07:10:24AM +1300, Tony Prisk wrote:
> [...]
> > @@ -87,6 +98,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip,
> > struct pwm_device *pwm)
> > {
> > struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
> >
> > + if (!clk_enable(vt8500->clk)) {
> > + dev_err(chip->dev, "failed to enable clock\n");
> > + return -EBUSY;
> > + };
> > +
>
> I don't think that works. The clock API returns 0 on success and a
> negative error code on failure. So this should rather be something like:
>
> err = clk_enable(vt8500->clk);
> if (err < 0) {
> dev_err(chip->dev, "failed to enable clock: %d\n", err);
> return err;
> }
>
> > @@ -123,6 +153,12 @@ static int __devinit pwm_probe(struct platform_device
> > *pdev)
> > chip->chip.ops = &vt8500_pwm_ops;
> > chip->chip.base = -1;
> > chip->chip.npwm = VT8500_NR_PWMS;
> > + chip->clk = devm_clk_get(&pdev->dev, NULL);
> > +
>
> The blank line should go above the call to devm_clk_get().
>
> > + if (IS_ERR_OR_NULL(chip->clk)) {
> > + dev_err(&pdev->dev, "clock source not specified\n");
> > + return PTR_ERR(chip->clk);
> > + }
> [...]
> > + if (!clk_prepare(chip->clk)) {
> > + dev_err(&pdev->dev, "failed to prepare clock\n");
> > + return -EBUSY;
> > + }
> > +
>
> Same comment here. I wonder how this code can work, since if the clock
> is properly prepared, then it will return 0, and the above will return
> -EBUSY.
>
> > ret = pwmchip_add(&chip->chip);
> > - if (ret < 0)
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "failed to add pwmchip\n");
>
> Error messages can be considered prose, so this should be: "failed to
> add PWM chip".
>
> Thierry
I don't know why none of this caused a failure when boot tested. The
clock should have been disabled 'automagically' at bootup, and never
reenabled. *shrug* Fixed in new patch v3 (didn't notice there was
already a v3).
Regards
Tony P
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] pwm: vt8500: Update vt8500 PWM driver support
This patch updates pwm-vt8500.c to support devicetree probing and
make use of the common clock subsystem.
A binding document describing the PWM controller found on
arch-vt8500 is also included.
Signed-off-by: Tony Prisk
---
v2/v3:
Fix errors/coding style as pointed out by Thierry Reding.
.../devicetree/bindings/pwm/vt8500-pwm.txt | 17
drivers/pwm/pwm-vt8500.c | 86 ++--
2 files changed, 80 insertions(+), 23 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
new file mode 100644
index 000..bcc6367
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
@@ -0,0 +1,17 @@
+VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
+
+Required properties:
+- compatible: should be "via,vt8500-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: should be 2. The first cell specifies the per-chip index
+ of the PWM to use and the second cell is the period in nanoseconds.
+- clocks: phandle to the PWM source clock
+
+Example:
+
+pwm1: pwm@d822 {
+ #pwm-cells = <2>;
+ compatible = "via,vt8500-pwm";
+ reg = <0xd822 0x1000>;
+ clocks = <&clkpwm>;
+};
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index ad14389..2ecd70f 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -1,7 +1,8 @@
/*
* drivers/pwm/pwm-vt8500.c
*
- * Copyright (C) 2010 Alexey Charkov
+ * Copyright (C) 2012 Tony Prisk
+ * Copyright (C) 2010 Alexey Charkov
*
* This software is licensed under the terms of the GNU General Public
* License version 2, as published by the Free Software Foundation, and
@@ -21,14 +22,24 @@
#include
#include
#include
+#include
#include
-#define VT8500_NR_PWMS 4
+#include
+#include
+#include
+
+/*
+ * SoC architecture allocates register space for 4 PWMs but only
+ * 2 are currently implemented.
+ */
+#define VT8500_NR_PWMS 2
struct vt8500_chip {
struct pwm_chip chip;
void __iomem *base;
+ struct clk *clk;
};
#define to_vt8500_chip(chip) container_of(chip, struct vt8500_chip, chip)
@@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct
pwm_device *pwm,
unsigned long long c;
unsigned long period_cycles, prescale, pv, dc;
- c = 2500/2; /* wild guess --- need to implement clocks */
+ c = clk_get_rate(vt8500->clk);
c = c * period_ns;
do_div(c, 10);
period_cycles = c;
@@ -85,8 +96,15 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct
pwm_device *pwm,
static int vt8500_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
{
+ int err;
struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
+ err = clk_enable(vt8500->clk);
+ if (err < 0)
+ dev_err(chip->dev, "failed to enable clock\n");
+ return -EBUSY;
+ };
+
pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
writel(5, vt8500->base + (pwm->hwpwm << 4));
return 0;
@@ -98,6 +116,8 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct
pwm_device *pwm)
pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
writel(0, vt8500->base + (pwm->hwpwm << 4));
+
+ clk_disable(vt8500->clk);
}
static struct pwm_ops vt8500_pwm_ops = {
@@ -107,12 +127,24 @@ static struct pwm_ops vt8500_pwm_ops = {
.owner = THIS_MODULE,
};
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = "via,vt8500-pwm", },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
+
+static int vt8500_pwm_probe(struct platform_device *pdev)
{
struct vt8500_chip *chip;
struct resource *r;
+ struct device_node *np = pdev->dev.of_node;
int ret;
+ if (!np) {
+ dev_err(&pdev->dev, "invalid devicetree node\n");
+ return -EINVAL;
+ }
+
chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -124,6 +156,12 @@ static int __devinit pwm_probe(struct platform_device
*pdev)
chip->chip.base = -1;
chip->chip.npwm = VT8500_NR_PWMS;
+ chip->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR_OR_NULL(chip->clk)) {
+ dev_err(&pdev->dev, "clock source not specified\n");
+ return PTR_ERR(chip->clk);
+ }
+
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (r == NULL) {
dev_err(&pdev->dev, "no memory resource defined\n");
@@ -131,18 +169,26 @@ static int __devinit pwm_probe(struct platform_device
*pdev)
}
Re: [PATCH v3] pwm: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 07:10:24AM +1300, Tony Prisk wrote:
[...]
> @@ -87,6 +98,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct
> pwm_device *pwm)
> {
> struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
>
> + if (!clk_enable(vt8500->clk)) {
> + dev_err(chip->dev, "failed to enable clock\n");
> + return -EBUSY;
> + };
> +
I don't think that works. The clock API returns 0 on success and a
negative error code on failure. So this should rather be something like:
err = clk_enable(vt8500->clk);
if (err < 0) {
dev_err(chip->dev, "failed to enable clock: %d\n", err);
return err;
}
> @@ -123,6 +153,12 @@ static int __devinit pwm_probe(struct platform_device
> *pdev)
> chip->chip.ops = &vt8500_pwm_ops;
> chip->chip.base = -1;
> chip->chip.npwm = VT8500_NR_PWMS;
> + chip->clk = devm_clk_get(&pdev->dev, NULL);
> +
The blank line should go above the call to devm_clk_get().
> + if (IS_ERR_OR_NULL(chip->clk)) {
> + dev_err(&pdev->dev, "clock source not specified\n");
> + return PTR_ERR(chip->clk);
> + }
[...]
> + if (!clk_prepare(chip->clk)) {
> + dev_err(&pdev->dev, "failed to prepare clock\n");
> + return -EBUSY;
> + }
> +
Same comment here. I wonder how this code can work, since if the clock
is properly prepared, then it will return 0, and the above will return
-EBUSY.
> ret = pwmchip_add(&chip->chip);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to add pwmchip\n");
Error messages can be considered prose, so this should be: "failed to
add PWM chip".
Thierry
pgpnYee5NEgWa.pgp
Description: PGP signature
[PATCH v3] pwm: vt8500: Update vt8500 PWM driver support
This patch updates pwm-vt8500.c to support devicetree probing and
make use of the common clock subsystem.
A binding document describing the PWM controller found on
arch-vt8500 is also included.
Signed-off-by: Tony Prisk
---
.../devicetree/bindings/pwm/vt8500-pwm.txt | 17
drivers/pwm/pwm-vt8500.c | 83 ++--
2 files changed, 77 insertions(+), 23 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
new file mode 100644
index 000..bcc6367
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
@@ -0,0 +1,17 @@
+VIA/Wondermedia VT8500/WM8xxx series SoC PWM controller
+
+Required properties:
+- compatible: should be "via,vt8500-pwm"
+- reg: physical base address and length of the controller's registers
+- #pwm-cells: should be 2. The first cell specifies the per-chip index
+ of the PWM to use and the second cell is the period in nanoseconds.
+- clocks: phandle to the PWM source clock
+
+Example:
+
+pwm1: pwm@d822 {
+ #pwm-cells = <2>;
+ compatible = "via,vt8500-pwm";
+ reg = <0xd822 0x1000>;
+ clocks = <&clkpwm>;
+};
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index ad14389..ebbc41c 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -1,7 +1,8 @@
/*
* drivers/pwm/pwm-vt8500.c
*
- * Copyright (C) 2010 Alexey Charkov
+ * Copyright (C) 2012 Tony Prisk
+ * Copyright (C) 2010 Alexey Charkov
*
* This software is licensed under the terms of the GNU General Public
* License version 2, as published by the Free Software Foundation, and
@@ -21,14 +22,24 @@
#include
#include
#include
+#include
#include
-#define VT8500_NR_PWMS 4
+#include
+#include
+#include
+
+/*
+ * SoC architecture allocates register space for 4 PWMs but only
+ * 2 are currently implemented.
+ */
+#define VT8500_NR_PWMS 2
struct vt8500_chip {
struct pwm_chip chip;
void __iomem *base;
+ struct clk *clk;
};
#define to_vt8500_chip(chip) container_of(chip, struct vt8500_chip, chip)
@@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct
pwm_device *pwm,
unsigned long long c;
unsigned long period_cycles, prescale, pv, dc;
- c = 2500/2; /* wild guess --- need to implement clocks */
+ c = clk_get_rate(vt8500->clk);
c = c * period_ns;
do_div(c, 10);
period_cycles = c;
@@ -87,6 +98,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct
pwm_device *pwm)
{
struct vt8500_chip *vt8500 = to_vt8500_chip(chip);
+ if (!clk_enable(vt8500->clk)) {
+ dev_err(chip->dev, "failed to enable clock\n");
+ return -EBUSY;
+ };
+
pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
writel(5, vt8500->base + (pwm->hwpwm << 4));
return 0;
@@ -98,6 +114,8 @@ static void vt8500_pwm_disable(struct pwm_chip *chip, struct
pwm_device *pwm)
pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0));
writel(0, vt8500->base + (pwm->hwpwm << 4));
+
+ clk_disable(vt8500->clk);
}
static struct pwm_ops vt8500_pwm_ops = {
@@ -107,12 +125,24 @@ static struct pwm_ops vt8500_pwm_ops = {
.owner = THIS_MODULE,
};
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = "via,vt8500-pwm", },
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
+
+static int vt8500_pwm_probe(struct platform_device *pdev)
{
struct vt8500_chip *chip;
struct resource *r;
+ struct device_node *np = pdev->dev.of_node;
int ret;
+ if (!np) {
+ dev_err(&pdev->dev, "invalid devicetree node\n");
+ return -EINVAL;
+ }
+
chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(&pdev->dev, "failed to allocate memory\n");
@@ -123,6 +153,12 @@ static int __devinit pwm_probe(struct platform_device
*pdev)
chip->chip.ops = &vt8500_pwm_ops;
chip->chip.base = -1;
chip->chip.npwm = VT8500_NR_PWMS;
+ chip->clk = devm_clk_get(&pdev->dev, NULL);
+
+ if (IS_ERR_OR_NULL(chip->clk)) {
+ dev_err(&pdev->dev, "clock source not specified\n");
+ return PTR_ERR(chip->clk);
+ }
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (r == NULL) {
@@ -131,18 +167,25 @@ static int __devinit pwm_probe(struct platform_device
*pdev)
}
chip->base = devm_request_and_ioremap(&pdev->dev, r);
- if (chip->base == NULL)
+ if (!chip->base)
return -EADDRNOTAVAIL;
+ if (!clk_prepare(chip->clk)) {
+
