Sorry for taking so long to reply to this, I had completely forgotten. On Mon, Apr 28, 2014 at 02:54:46PM +0200, Bart Tanghe wrote: > Add some better error handling and Device table support > Added Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt > > Signed-off-by: Bart Tanghe <bart.tan...@thomasmore.be>
There should be a description about this driver in the commit message. The above reads like a changelog since earlier versions of this patch, in which case it should go below a --- separator line, like so: Commit message goes here... ... Signed-off-by: Bart Tanghe <bart.tan...@thomasmore.be> --- Changes in v3: - add some better error handling - add device tree support I assume that "device table" was meant to be "device tree"? Also it might be useful to mention Raspberry Pi in the commit message. > diff --git a/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt > b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt > new file mode 100644 > index 0000000..44c0b95 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt > @@ -0,0 +1,13 @@ > +bcm2835 PWM controller I think this ought to be "BCM2835". Again, maybe this should mention the Raspberry Pi so that when people search for it they get a match on this document. > +Required properties: > +- compatible: should be "bcrm,pwm-bcm2835" According to Documentation/devicetree/bindings/vendor-prefixes.txt this should be "brcm,...". Also I'd suggest putting the SoC first, followed by the hardware block name: "brcm,bcm2835-pwm" > +- reg: physical base address and length of the controller's registers > + > +Examples: > + > +pwm@0x2020C000 { > + compatible = "bcrm,pwm-bcm2835"; > + reg = <0x2020C000 0x28>; I think other BCM2835 device tree bindings use lower-case for addresses, so you might want to follow that for consistency. Also unit-addresses are always in hexadecimal and shouldn't take a 0x prefix, so the above would become: pwm@2020c000 { ... }; > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 22f2f28..20341a3 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -62,6 +62,18 @@ config PWM_ATMEL_TCB > To compile this driver as a module, choose M here: the module > will be called pwm-atmel-tcb. > > +config PWM_BCM2835 > + tristate "BCM2835 PWM support" > + depends on MACH_BCM2835 || MACH_BCM2708 > + help > + PWM framework driver for BCM2835 controller (raspberry pi) I think the correct capitalization would be "Raspberry Pi". > + Only 1 channel is implemented. How many can it take? Why haven't all been implemented? > + The pwm core is tested with a pwm basic frequency of 1Mhz. > + Use period above 1000ns s/pwm/PWM/ in prose. Why this restriction? Doesn't it work with higher frequencies? Perhaps this shouldn't be a comment in the Kconfig entry but rather a runtime check (and error message)? > diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c > new file mode 100644 > index 0000000..ec9829b > --- /dev/null > +++ b/drivers/pwm/pwm-bcm2835.c > @@ -0,0 +1,198 @@ > +/* > + * pwm-bcm2835 driver > + * Standard raspberry pi (gpio18 - pwm0) Just drop these two lines, they don't provide very useful information. > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +/*mmio regiser mapping*/ s/mmio/MMIO/, s/regiser/register/. Also spaces after /* and before */. > + > +#define DUTY 0x14 > +#define PERIOD 0x10 > +#define CHANNEL 0x10 CHANNEL doesn't seem to be used. > + > +#define PWM_ENABLE 0x00000001 > +#define PWM_POLARITY 0x00000010 > + > +#define MASK_CTL_PWM 0x000000FF I prefer lowercase for hexadecimals. And there's no need to repeat all the leading zeroes. PWM_ENABLE and PWM_POLARITY seem to be bits, so I'd prefer: #define PWM_ENABLE (1 << 0) #define PWM_POLARITY (1 << 4) > +#define CTL_PWM 0x00000081 What does this value mean? Also even if this register is at offset 0 you should still add a symbolic name for it. > +#define DRIVER_AUTHOR "Bart Tanghe <bart.tan...@thomasmore.be>" > +#define DRIVER_DESC "A bcm2835 pwm driver - raspberry pi development > platform" These are only used once, so you don't have to #define them. Use them in the MODULE_*() macros directly. Also, perhaps a better and more generic description would be: "Broadcom BCM2835 (Raspberry Pi) PWM driver" And perhaps even drop "(Raspberry Pi)" since presumably the driver will work on any BCM2835-based board. > +struct bcm2835_pwm_chip { > + struct pwm_chip chip; > + struct device *dev; > + int channel; This field seems to be unused. > + int scaler; Perhaps this should be unsigned long instead of int? > + void __iomem *mmio_base; Calling this something like "base" or "regs" will save a lot of characters when accessing registers. > +static inline struct bcm2835_pwm_chip *to_bcm2835_pwm_chip( > + struct pwm_chip *chip){ > + > + return container_of(chip, struct bcm2835_pwm_chip, chip); > +} The preferred way to write the above is: static inline struct bcm2835_pwm_chip * to_bcm2835_pwm_chip(struct pwm_chip *chip) { ... } Perhaps if you make names a little shorter you can even get away without wrapping it: static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip) { ... } > +static int bcm2835_pwm_config(struct pwm_chip *chip, > + struct pwm_device *pwm, > + int duty_ns, int period_ns){ The pwm argument still fits on the first line. Also the opening brace ({) should go on a separate line. > + > + struct bcm2835_pwm_chip *pc; > + > + pc = container_of(chip, struct bcm2835_pwm_chip, chip); This should use the to_bcm2835_pwm() function defined earlier. > + > + iowrite32(duty_ns/pc->scaler, pc->mmio_base + DUTY); > + iowrite32(period_ns/pc->scaler, pc->mmio_base + PERIOD); These should use writel() instead of iowrite32() and there need to be spaces around '/'. > +static int bcm2835_pwm_enable(struct pwm_chip *chip, > + struct pwm_device *pwm){ Same as above. > + > + struct bcm2835_pwm_chip *pc; > + > + pc = container_of(chip, struct bcm2835_pwm_chip, chip); Should use to_bcm2835_pwm() and can go on a single line: struct bcm2835_pwm *pc = to_bcm2835_pwm(chip); > + > + iowrite32(ioread32(pc->mmio_base) | PWM_ENABLE, pc->mmio_base); Please break this up into: u32 value; ... value = readl(pc->mmio_base + ...); value |= PWM_ENABLE; writel(value, pc->mmio_base + ...); > +static void bcm2835_pwm_disable(struct pwm_chip *chip, > + struct pwm_device *pwm) > +{ > + struct bcm2835_pwm_chip *pc; > + > + pc = to_bcm2835_pwm_chip(chip); The above can go on a single line. > + > + iowrite32(ioread32(pc->mmio_base) & ~PWM_ENABLE, pc->mmio_base); > +} Same as above and below. > +static int bcm2835_pwm_probe(struct platform_device *pdev) > +{ > + struct bcm2835_pwm_chip *pwm; > + Gratuitous blank line. > + int ret; > + struct resource *r; > + u32 start, end; start and end don't seem to be used. > + struct clk *clk; > + > + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); > + if (!pwm) { > + dev_err(&pdev->dev, "failed to allocate memory\n"); No need for this error message. > + return -ENOMEM; > + } > + > + pwm->dev = &pdev->dev; > + > + clk = clk_get(&pdev->dev, NULL); devm_clk_get()? Also, don't you want to keep a reference to the clock in struct bcm2835_pwm so that you can disable the clock on driver removal? > + if (IS_ERR(clk)) { > + dev_err(&pdev->dev, "could not find clk: %ld\n", PTR_ERR(clk)); In text I prefer to use "clock" rather than "clk". > + devm_kfree(&pdev->dev, pwm); No need for this. The point of the devm_*() functions is that you don't have to manually clean up the resources that they manage. > + return PTR_ERR(clk); > + } > + > + pwm->scaler = (int)1000000000/clk_get_rate(clk); There's NSEC_PER_SEC and you shouldn't cast this if you change the type of scaler to unsigned long. So this becomes: pwm->scaler = NSEC_PER_SEC / clk_get_rate(clk); > + > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + pwm->mmio_base = devm_ioremap_resource(&pdev->dev, r); > + if (IS_ERR(pwm->mmio_base)) { > + devm_kfree(&pdev->dev, pwm); Again, no need to free the memory here. > + return PTR_ERR(pwm->mmio_base); > + } > + > + start = r->start; > + end = r->end; > + > + pwm->chip.dev = &pdev->dev; > + pwm->chip.ops = &bcm2835_pwm_ops; > + pwm->chip.npwm = 2; The Kconfig entry says that only a single PWM is implemented, so this should be 1, shouldn't it? > + > + ret = pwmchip_add(&pwm->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); > + devm_kfree(&pdev->dev, pwm); Drop this. > + return -1; This needs to propagate ret. > + } > + > + /*set the pwm0 configuration*/ Spaces after /* and before */. > + iowrite32((ioread32(pwm->mmio_base) & ~MASK_CTL_PWM) > + | CTL_PWM, pwm->mmio_base); Should this perhaps be delayed until the PWM is requested? What are the consequences of configuring the PWM? Also split this up into an explicit read/modify/write sequence please. > + > + platform_set_drvdata(pdev, pwm); > + > + return 0; > +} I notice that you never prepare or enable the clock here. Perhaps this isn't required because it's always on, but I think you should still call clk_prepare_enable() here (and clk_disable_unprepare() in .remove()) to make sure the driver is more portable. > + > +static int bcm2835_pwm_remove(struct platform_device *pdev) > +{ > + Gratuitous blank line. > + struct bcm2835_pwm_chip *pc; > + pc = platform_get_drvdata(pdev); The above can go on a single line. > + > + if (WARN_ON(!pc)) > + return -ENODEV; There's no need for this check. > + > + return pwmchip_remove(&pc->chip); > +} > + > +static const struct of_device_id bcm2835_pwm_of_match[] = { > + { .compatible = "bcrm,pwm-bcm2835", }, s/bcrm/brcm/ > + { /* sentinel */ } > +}; > + Gratuitous blank line. > +MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match); > + > +static struct platform_driver bcm2835_pwm_driver = { > + .driver = { > + .name = "pwm-bcm2835", > + .owner = THIS_MODULE, No need to initialize .owner here. module_platform_driver() will do that for you. > + .of_match_table = bcm2835_pwm_of_match, > + }, > + .probe = bcm2835_pwm_probe, > + .remove = bcm2835_pwm_remove, > +}; > +module_platform_driver(bcm2835_pwm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR(DRIVER_AUTHOR); > +MODULE_DESCRIPTION(DRIVER_DESC); A more natural ordering would be: MODULE_AUTHOR(...); MODULE_DESCRIPTION(...); MODULE_LICENSE(...); Thierry
pgpJ3ousmXgLR.pgp
Description: PGP signature