Patchwork [resend,rfc,v3] pwm: add BCM2835 PWM driver

login
register
mail settings
Submitter Bart Tanghe
Date April 28, 2014, 12:54 p.m.
Message ID <1398689686-30317-1-git-send-email-bart.tanghe@thomasmore.be>
Download mbox | patch
Permalink /patch/343382/
State Rejected
Headers show

Comments

Bart Tanghe - April 28, 2014, 12:54 p.m.
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
Thierry Reding - Aug. 25, 2014, 1:19 p.m.
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
Bart Tanghe - Sept. 4, 2014, 10:05 a.m.
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
Stephen Warren - Sept. 4, 2014, 3:06 p.m.
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

Patch

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