| Message ID | 1398689686-30317-1-git-send-email-bart.tanghe@thomasmore.be |
|---|---|
| State | Rejected |
| Headers | show |
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.tanghe@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.tanghe@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.tanghe@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
No problem. Thanks for the feedback. I've got some question below. On 2014-08-25 15:19, Thierry Reding wrote: > 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.tanghe@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.tanghe@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? BCM2835 can take 2 pwm channels. I can implement 2 channels but can't physically test the second channel. Is that a problem? > >> + 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)? The frequency of the pwm hardware core is 19.7 MHZ. I'll implement a runtime check. > >> 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.tanghe@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. The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by a clock driver, as mentioned in a previous comment by Stephen Warren. Any clock programming should be performed by a clock driver. We don't have one of those upstream yet, mainly because it would rely on talking to the firmware (running on the VideoCore) to manipulate the clocks, and we don't have a firmware protocol driver either. Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver. The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental. If anyone has some suggestions? > >> + >> +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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/04/2014 04:05 AM, Bart Tanghe wrote: > No problem. Thanks for the feedback. > I've got some question below. > > On 2014-08-25 15:19, Thierry Reding wrote: >> 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.tanghe@thomasmore.be> >>> 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? > > BCM2835 can take 2 pwm channels. > I can implement 2 channels but can't physically test the second channel. Is that a problem? I don't think that's a problem; I would expect the channels to be identical, so testing 1 should be fine. >> 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. > The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by > a clock driver, as mentioned in a previous comment by Stephen Warren. > > Any clock programming should be performed by a clock driver. We don't > have one of those upstream yet, mainly because it would rely on talking > to the firmware (running on the VideoCore) to manipulate the clocks, and > we don't have a firmware protocol driver either. > > Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver. > The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental. > If anyone has some suggestions? Oh dear. It sounds like we need at least some form of clock driver for the platform then. I still don't think there's complete documentation for the HW, even though a lot of register docs were published which presumably cover the clock HW? Equally, given that the VC firmware assumes it owns most of the HW, it seems best to manipulate the clocks through the firmware interface rather than directly touching the HW. Unfortunately, I don't believe there's any ABI guarantee on the firmware interface. Perhaps we can get one? >>> +static const struct of_device_id bcm2835_pwm_of_match[] = { >>> + { .compatible = "bcrm,pwm-bcm2835", }, >> >> s/bcrm/brcm/ Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent with all the other HW on this SoC. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Sep 04, 2014 at 09:06:48AM -0600, Stephen Warren wrote: > On 09/04/2014 04:05 AM, Bart Tanghe wrote: > >No problem. Thanks for the feedback. > >I've got some question below. > > > >On 2014-08-25 15:19, Thierry Reding wrote: > >>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.tanghe@thomasmore.be> > > >>>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? > > > >BCM2835 can take 2 pwm channels. > >I can implement 2 channels but can't physically test the second channel. Is that a problem? > > I don't think that's a problem; I would expect the channels to be identical, > so testing 1 should be fine. Agreed. If it turns out not to work it can always be fixed. > >>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. > >The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by > >a clock driver, as mentioned in a previous comment by Stephen Warren. > > > >Any clock programming should be performed by a clock driver. We don't > >have one of those upstream yet, mainly because it would rely on talking > >to the firmware (running on the VideoCore) to manipulate the clocks, and > >we don't have a firmware protocol driver either. > > > >Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver. > >The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental. > >If anyone has some suggestions? > > Oh dear. It sounds like we need at least some form of clock driver for the > platform then. I still don't think there's complete documentation for the > HW, even though a lot of register docs were published which presumably cover > the clock HW? Equally, given that the VC firmware assumes it owns most of > the HW, it seems best to manipulate the clocks through the firmware > interface rather than directly touching the HW. Unfortunately, I don't > believe there's any ABI guarantee on the firmware interface. Perhaps we can > get one? Urgs... this VC firmware seems to be more of a headache that I thought it was. How is this handled in other drivers? Surely PWM isn't the first one that needs clocks? > >>>+static const struct of_device_id bcm2835_pwm_of_match[] = { > >>>+ { .compatible = "bcrm,pwm-bcm2835", }, > >> > >>s/bcrm/brcm/ > > Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent > with all the other HW on this SoC. Yes, please. Thierry
On 2014-09-26 09:11, Thierry Reding wrote: > On Thu, Sep 04, 2014 at 09:06:48AM -0600, Stephen Warren wrote: >> On 09/04/2014 04:05 AM, Bart Tanghe wrote: >>> No problem. Thanks for the feedback. >>> I've got some question below. >>> >>> On 2014-08-25 15:19, Thierry Reding wrote: >>>> 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.tanghe@thomasmore.be> >> >>>>> 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? >>> >>> BCM2835 can take 2 pwm channels. >>> I can implement 2 channels but can't physically test the second channel. Is that a problem? >> >> I don't think that's a problem; I would expect the channels to be identical, >> so testing 1 should be fine. > > Agreed. If it turns out not to work it can always be fixed. > >>>> 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. >>> The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by >>> a clock driver, as mentioned in a previous comment by Stephen Warren. >>> >>> Any clock programming should be performed by a clock driver. We don't >>> have one of those upstream yet, mainly because it would rely on talking >>> to the firmware (running on the VideoCore) to manipulate the clocks, and >>> we don't have a firmware protocol driver either. >>> >>> Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver. >>> The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental. >>> If anyone has some suggestions? >> >> Oh dear. It sounds like we need at least some form of clock driver for the >> platform then. I still don't think there's complete documentation for the >> HW, even though a lot of register docs were published which presumably cover >> the clock HW? Equally, given that the VC firmware assumes it owns most of >> the HW, it seems best to manipulate the clocks through the firmware >> interface rather than directly touching the HW. Unfortunately, I don't >> believe there's any ABI guarantee on the firmware interface. Perhaps we can >> get one? > > Urgs... this VC firmware seems to be more of a headache that I thought > it was. How is this handled in other drivers? Surely PWM isn't the first > one that needs clocks? > I've looked at the i2c, spi and uart clocks. They seems default enabled, or enabled at boot by the firmware. The clock dividers are accessible in the address range of the block in contrast to the pwm clock. In the firmware documentation, I can't find any reference to the pwm clock. >>>>> +static const struct of_device_id bcm2835_pwm_of_match[] = { >>>>> + { .compatible = "bcrm,pwm-bcm2835", }, >>>> >>>> s/bcrm/brcm/ >> >> Probably swap the order, so "brcm,bcm2835-pwm". That would be consistent >> with all the other HW on this SoC. > > Yes, please. > > Thierry > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/26/2014 01:11 AM, Thierry Reding wrote: > On Thu, Sep 04, 2014 at 09:06:48AM -0600, Stephen Warren wrote: >> On 09/04/2014 04:05 AM, Bart Tanghe wrote: >>> No problem. Thanks for the feedback. >>> I've got some question below. >>> >>> On 2014-08-25 15:19, Thierry Reding wrote: >>>> 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.tanghe@thomasmore.be> >> >>>>> 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? >>> >>> BCM2835 can take 2 pwm channels. >>> I can implement 2 channels but can't physically test the second channel. Is that a problem? >> >> I don't think that's a problem; I would expect the channels to be identical, >> so testing 1 should be fine. > > Agreed. If it turns out not to work it can always be fixed. > >>>> 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. >>> The frequency can be minimized by a clock_divider ( the pwm clock is default disabled). this has to be done by >>> a clock driver, as mentioned in a previous comment by Stephen Warren. >>> >>> Any clock programming should be performed by a clock driver. We don't >>> have one of those upstream yet, mainly because it would rely on talking >>> to the firmware (running on the VideoCore) to manipulate the clocks, and >>> we don't have a firmware protocol driver either. >>> >>> Nowadays, I'm using a userspace program to change the clock_divider, but would like to implement this in a clock driver. >>> The clock hardware description isn't implemented in the datasheet. I can convert the userspace prog to a clock driver but this is very experimental. >>> If anyone has some suggestions? >> >> Oh dear. It sounds like we need at least some form of clock driver for the >> platform then. I still don't think there's complete documentation for the >> HW, even though a lot of register docs were published which presumably cover >> the clock HW? Equally, given that the VC firmware assumes it owns most of >> the HW, it seems best to manipulate the clocks through the firmware >> interface rather than directly touching the HW. Unfortunately, I don't >> believe there's any ABI guarantee on the firmware interface. Perhaps we can >> get one? > > Urgs... this VC firmware seems to be more of a headache that I thought > it was. How is this handled in other drivers? Surely PWM isn't the first > one that needs clocks? For the other clocks, I've set up dummy fixed-rate clocks in the DT and/or "clock driver" code to satisfy references by phandle or clock name respectively. Since the other drivers don't actually manipulate the clock rates etc., this is enough for the drivers. I always hoped that enough HW information would either be reverse engineered or released to somehow disable the VC firmware, and implement all the drivers within Linux talking to HW directly. -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 26, 2014 at 08:45:57AM -0600, Stephen Warren wrote: > On 09/26/2014 01:11 AM, Thierry Reding wrote: > >On Thu, Sep 04, 2014 at 09:06:48AM -0600, Stephen Warren wrote: [...] > >>Oh dear. It sounds like we need at least some form of clock driver for the > >>platform then. I still don't think there's complete documentation for the > >>HW, even though a lot of register docs were published which presumably cover > >>the clock HW? Equally, given that the VC firmware assumes it owns most of > >>the HW, it seems best to manipulate the clocks through the firmware > >>interface rather than directly touching the HW. Unfortunately, I don't > >>believe there's any ABI guarantee on the firmware interface. Perhaps we can > >>get one? > > > >Urgs... this VC firmware seems to be more of a headache that I thought > >it was. How is this handled in other drivers? Surely PWM isn't the first > >one that needs clocks? > > For the other clocks, I've set up dummy fixed-rate clocks in the DT and/or > "clock driver" code to satisfy references by phandle or clock name > respectively. Since the other drivers don't actually manipulate the clock > rates etc., this is enough for the drivers. Given that this driver only queries the clock frequency adding a fixed- rate clock to the device tree should work as well. Then the calls to clk_prepare_enable() and clk_disable_unprepare() can still be added as appropriate so that the driver doesn't need to change if a proper clock driver ever gets added. Or am I missing anything? Perhaps the issue is that the default clock rate for the PWM clock isn't usable? That would still not prevent the driver from being merged. Thierry
On 2014-09-29 07:33, Thierry Reding wrote: > On Fri, Sep 26, 2014 at 08:45:57AM -0600, Stephen Warren wrote: >> On 09/26/2014 01:11 AM, Thierry Reding wrote: >>> On Thu, Sep 04, 2014 at 09:06:48AM -0600, Stephen Warren wrote: > [...] >>>> Oh dear. It sounds like we need at least some form of clock driver for the >>>> platform then. I still don't think there's complete documentation for the >>>> HW, even though a lot of register docs were published which presumably cover >>>> the clock HW? Equally, given that the VC firmware assumes it owns most of >>>> the HW, it seems best to manipulate the clocks through the firmware >>>> interface rather than directly touching the HW. Unfortunately, I don't >>>> believe there's any ABI guarantee on the firmware interface. Perhaps we can >>>> get one? >>> >>> Urgs... this VC firmware seems to be more of a headache that I thought >>> it was. How is this handled in other drivers? Surely PWM isn't the first >>> one that needs clocks? >> >> For the other clocks, I've set up dummy fixed-rate clocks in the DT and/or >> "clock driver" code to satisfy references by phandle or clock name >> respectively. Since the other drivers don't actually manipulate the clock >> rates etc., this is enough for the drivers. > > Given that this driver only queries the clock frequency adding a fixed- > rate clock to the device tree should work as well. Then the calls to > clk_prepare_enable() and clk_disable_unprepare() can still be added as > appropriate so that the driver doesn't need to change if a proper clock > driver ever gets added. > > Or am I missing anything? Perhaps the issue is that the default clock > rate for the PWM clock isn't usable? That would still not prevent the > driver from being merged. > > Thierry > The pwm clock is default unusable. To let the pwm clock run, It's necessary to change some clock registers. I've added the clk_prepare_enable and the clk_disable_unprepare functions. So the driver is able to work with a proper clock driver in the future. Bart -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 29, 2014 at 03:37:50PM +0200, Bart Tanghe wrote: > On 2014-09-29 07:33, Thierry Reding wrote: > > On Fri, Sep 26, 2014 at 08:45:57AM -0600, Stephen Warren wrote: > >> On 09/26/2014 01:11 AM, Thierry Reding wrote: > >>> On Thu, Sep 04, 2014 at 09:06:48AM -0600, Stephen Warren wrote: > > [...] > >>>> Oh dear. It sounds like we need at least some form of clock driver for the > >>>> platform then. I still don't think there's complete documentation for the > >>>> HW, even though a lot of register docs were published which presumably cover > >>>> the clock HW? Equally, given that the VC firmware assumes it owns most of > >>>> the HW, it seems best to manipulate the clocks through the firmware > >>>> interface rather than directly touching the HW. Unfortunately, I don't > >>>> believe there's any ABI guarantee on the firmware interface. Perhaps we can > >>>> get one? > >>> > >>> Urgs... this VC firmware seems to be more of a headache that I thought > >>> it was. How is this handled in other drivers? Surely PWM isn't the first > >>> one that needs clocks? > >> > >> For the other clocks, I've set up dummy fixed-rate clocks in the DT and/or > >> "clock driver" code to satisfy references by phandle or clock name > >> respectively. Since the other drivers don't actually manipulate the clock > >> rates etc., this is enough for the drivers. > > > > Given that this driver only queries the clock frequency adding a fixed- > > rate clock to the device tree should work as well. Then the calls to > > clk_prepare_enable() and clk_disable_unprepare() can still be added as > > appropriate so that the driver doesn't need to change if a proper clock > > driver ever gets added. > > > > Or am I missing anything? Perhaps the issue is that the default clock > > rate for the PWM clock isn't usable? That would still not prevent the > > driver from being merged. > > > > Thierry > > > The pwm clock is default unusable. To let the pwm clock run, It's necessary to change some > clock registers. I've added the clk_prepare_enable and the clk_disable_unprepare functions. So the > driver is able to work with a proper clock driver in the future. Okay. Sounds good to me. Thierry
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 + +Required properties: +- compatible: should be "bcrm,pwm-bcm2835" +- reg: physical base address and length of the controller's registers + +Examples: + +pwm@0x2020C000 { + compatible = "bcrm,pwm-bcm2835"; + reg = <0x2020C000 0x28>; + clocks = <&clk_pwm>; +}; 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) + Only 1 channel is implemented. + + The pwm core is tested with a pwm basic frequency of 1Mhz. + Use period above 1000ns + + To compile this driver as a module, choose M here: the module + will be called pwm-bcm2835. + config PWM_BFIN tristate "Blackfin PWM support" depends on BFIN_GPTIMERS diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index d8906ec..9863590 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_PWM_SYSFS) += sysfs.o obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o +obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o obj-$(CONFIG_PWM_BFIN) += pwm-bfin.o obj-$(CONFIG_PWM_EP93XX) += pwm-ep93xx.o obj-$(CONFIG_PWM_IMX) += pwm-imx.o 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) + * + * Copyright (C) 2014 Thomas more + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2. + */ + +#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*/ + +#define DUTY 0x14 +#define PERIOD 0x10 +#define CHANNEL 0x10 + +#define PWM_ENABLE 0x00000001 +#define PWM_POLARITY 0x00000010 + +#define MASK_CTL_PWM 0x000000FF +#define CTL_PWM 0x00000081 + +#define DRIVER_AUTHOR "Bart Tanghe <bart.tanghe@thomasmore.be>" +#define DRIVER_DESC "A bcm2835 pwm driver - raspberry pi development platform" + +struct bcm2835_pwm_chip { + struct pwm_chip chip; + struct device *dev; + int channel; + int scaler; + void __iomem *mmio_base; +}; + +static inline struct bcm2835_pwm_chip *to_bcm2835_pwm_chip( + struct pwm_chip *chip){ + + return container_of(chip, struct bcm2835_pwm_chip, chip); +} + +static int bcm2835_pwm_config(struct pwm_chip *chip, + struct pwm_device *pwm, + int duty_ns, int period_ns){ + + struct bcm2835_pwm_chip *pc; + + pc = container_of(chip, struct bcm2835_pwm_chip, chip); + + iowrite32(duty_ns/pc->scaler, pc->mmio_base + DUTY); + iowrite32(period_ns/pc->scaler, pc->mmio_base + PERIOD); + + return 0; +} + +static int bcm2835_pwm_enable(struct pwm_chip *chip, + struct pwm_device *pwm){ + + struct bcm2835_pwm_chip *pc; + + pc = container_of(chip, struct bcm2835_pwm_chip, chip); + + iowrite32(ioread32(pc->mmio_base) | PWM_ENABLE, pc->mmio_base); + return 0; +} + +static void bcm2835_pwm_disable(struct pwm_chip *chip, + struct pwm_device *pwm) +{ + struct bcm2835_pwm_chip *pc; + + pc = to_bcm2835_pwm_chip(chip); + + iowrite32(ioread32(pc->mmio_base) & ~PWM_ENABLE, pc->mmio_base); +} + +static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm, + enum pwm_polarity polarity) +{ + struct bcm2835_pwm_chip *pc; + + pc = to_bcm2835_pwm_chip(chip); + + if (polarity == PWM_POLARITY_NORMAL) + iowrite32((ioread32(pc->mmio_base) & ~PWM_POLARITY), + pc->mmio_base); + else if (polarity == PWM_POLARITY_INVERSED) + iowrite32((ioread32(pc->mmio_base) | PWM_POLARITY), + pc->mmio_base); + + return 0; +} + +static const struct pwm_ops bcm2835_pwm_ops = { + .config = bcm2835_pwm_config, + .enable = bcm2835_pwm_enable, + .disable = bcm2835_pwm_disable, + .set_polarity = bcm2835_set_polarity, + .owner = THIS_MODULE, +}; + +static int bcm2835_pwm_probe(struct platform_device *pdev) +{ + struct bcm2835_pwm_chip *pwm; + + int ret; + struct resource *r; + u32 start, end; + struct clk *clk; + + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); + if (!pwm) { + dev_err(&pdev->dev, "failed to allocate memory\n"); + return -ENOMEM; + } + + pwm->dev = &pdev->dev; + + clk = clk_get(&pdev->dev, NULL); + if (IS_ERR(clk)) { + dev_err(&pdev->dev, "could not find clk: %ld\n", PTR_ERR(clk)); + devm_kfree(&pdev->dev, pwm); + return PTR_ERR(clk); + } + + pwm->scaler = (int)1000000000/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); + 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; + + ret = pwmchip_add(&pwm->chip); + if (ret < 0) { + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); + devm_kfree(&pdev->dev, pwm); + return -1; + } + + /*set the pwm0 configuration*/ + iowrite32((ioread32(pwm->mmio_base) & ~MASK_CTL_PWM) + | CTL_PWM, pwm->mmio_base); + + platform_set_drvdata(pdev, pwm); + + return 0; +} + +static int bcm2835_pwm_remove(struct platform_device *pdev) +{ + + struct bcm2835_pwm_chip *pc; + pc = platform_get_drvdata(pdev); + + if (WARN_ON(!pc)) + return -ENODEV; + + return pwmchip_remove(&pc->chip); +} + +static const struct of_device_id bcm2835_pwm_of_match[] = { + { .compatible = "bcrm,pwm-bcm2835", }, + { /* sentinel */ } +}; + +MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match); + +static struct platform_driver bcm2835_pwm_driver = { + .driver = { + .name = "pwm-bcm2835", + .owner = THIS_MODULE, + .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); -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
Add some better error handling and Device table support Added Documentation/devicetree/bindings/pwm/pwm-bcm2835.txt Signed-off-by: Bart Tanghe <bart.tanghe@thomasmore.be> the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html