On Mon, Oct 22, 2012 at 09:13:35PM +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 <li...@prisktech.co.nz> > --- > v2 changes: > Merged binding doc patch with main code patch > Fixes as requested by Thierry Reding. > > .../devicetree/bindings/pwm/vt8500-pwm.txt | 17 ++++ > drivers/pwm/pwm-vt8500.c | 87 > +++++++++++++++----- > 2 files changed, 82 insertions(+), 22 deletions(-) > create mode 100644 Documentation/devicetree/bindings/pwm/vt8500-pwm.txt
Looking better... just a few minor comments. > diff --git a/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt > b/Documentation/devicetree/bindings/pwm/vt8500-pwm.txt > new file mode 100644 > index 0000000..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@d8220000 { > + #pwm-cells = <2>; > + compatible = "via,vt8500-pwm"; > + reg = <0xd8220000 0x1000>; > + clocks = <&clkpwm>; > +}; > diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c > index ad14389..36fef69 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 <alch...@gmail.com> > + * Copyright (C) 2012 Tony Prisk <li...@prisktech.co.nz> > + * Copyright (C) 2010 Alexey Charkov <alch...@gmail.com> > * > * 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,25 @@ > #include <linux/io.h> > #include <linux/pwm.h> > #include <linux/delay.h> > +#include <linux/clk.h> > > #include <asm/div64.h> > > -#define VT8500_NR_PWMS 4 > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_address.h> > + > +/* > + * 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; > + int enable_cnt; You don't need to keep a reference count yourself. The clock framework does that for you. > @@ -87,6 +99,11 @@ static int vt8500_pwm_enable(struct pwm_chip *chip, struct > pwm_device *pwm) > { > struct vt8500_chip *vt8500 = to_vt8500_chip(chip); > > + if (vt8500->enable_cnt == 0) > + clk_enable(vt8500->clk); > + > + vt8500->enable_cnt++; > + Again, reference counting isn't needed. But you should be checking for the return value of clk_enable(). > pwm_busy_wait(vt8500->base + 0x40 + pwm->hwpwm, (1 << 0)); > writel(5, vt8500->base + (pwm->hwpwm << 4)); > return 0; > @@ -98,6 +115,11 @@ 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)); > + > + vt8500->enable_cnt--; > + > + if (vt8500->enable_cnt == 0) > + clk_disable(vt8500->clk); Just clk_disable() will be enough. > @@ -107,12 +129,25 @@ 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); I've more often seen this without a blank line between. But if you prefer this for readability or whatever reasons feel free to keep it as is. > + > +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 +158,13 @@ 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); > + chip->enable_cnt = 0; > + > + if (!chip->clk) { The proper way to check this is with IS_ERR_OR_NULL(clk) and return PTR_ERR(clk) in case of failure. > + dev_err(&pdev->dev, "clock source not specified\n"); > + return -EINVAL; > + } > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (r == NULL) { > @@ -131,18 +173,24 @@ 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) { > + dev_err(&pdev->dev, "memory resource not available\n"); devm_request_and_ioremap() already outputs an error message in case of failure, so no need to repeat it here. > return -EADDRNOTAVAIL; > + } > + > + clk_prepare(chip->clk); clk_prepare() can fail, so you should check for errors. > > ret = pwmchip_add(&chip->chip); > - if (ret < 0) > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to add pwmchip\n"); > return ret; > + } > > platform_set_drvdata(pdev, chip); > return ret; > } > > -static int __devexit pwm_remove(struct platform_device *pdev) > +static int vt8500_pwm_remove(struct platform_device *pdev) > { > struct vt8500_chip *chip; > > @@ -150,28 +198,23 @@ static int __devexit pwm_remove(struct platform_device > *pdev) > if (chip == NULL) > return -ENODEV; > > + clk_unprepare(chip->clk); > + > return pwmchip_remove(&chip->chip); > } > > -static struct platform_driver pwm_driver = { > +static struct platform_driver vt8500_pwm_driver = { > + .probe = vt8500_pwm_probe, > + .remove = vt8500_pwm_remove, > .driver = { > .name = "vt8500-pwm", > .owner = THIS_MODULE, > + .of_match_table = vt8500_pwm_dt_ids, > }, > - .probe = pwm_probe, > - .remove = __devexit_p(pwm_remove), > }; > > -static int __init pwm_init(void) > -{ > - return platform_driver_register(&pwm_driver); > -} > -arch_initcall(pwm_init); > - > -static void __exit pwm_exit(void) > -{ > - platform_driver_unregister(&pwm_driver); > -} > -module_exit(pwm_exit); > +module_platform_driver(vt8500_pwm_driver); Similar to my comment regarding the MODULE_DEVICE_TABLE() macro, I've seen this usually without a blank line. But again that's mostly bike- shedding and I'm find taking the patch with the blank line left in. Thierry
pgpu9TEwJrIJa.pgp
Description: PGP signature