Add support for the PWM controller of the BCM2835 SoC found on Raspberry PI
The driver isn't as much tested as I wanted it to be and devicetree
support is still missing, but I thought it would be nice to have some
comments if I'm in the right direction.
Signed-off-by: Johannes Thumshirn <[email protected]>
---
drivers/pwm/Kconfig | 8 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-bcm2835.c | 275 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 284 insertions(+)
create mode 100644 drivers/pwm/pwm-bcm2835.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 75840b5..5417159 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -224,4 +224,12 @@ config PWM_VT8500
To compile this driver as a module, choose M here: the module
will be called pwm-vt8500.
+config PWM_BCM2835
+ tristate "BCM2835 PWM support"
+ help
+ Generic PWM framework driver for bcm2835 found on the Rasperry PI
+
+ To compile this driver as a module, choose M here: the module will be
+ called pwm-bcm2835
+
endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 77a8c18..0cf8d4d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_PWM_TIPWMSS) += pwm-tipwmss.o
obj-$(CONFIG_PWM_TWL) += pwm-twl.o
obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
+obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
new file mode 100644
index 0000000..8a64666
--- /dev/null
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -0,0 +1,275 @@
+/*
+ * BCM2835 PWM driver
+ *
+ * Derived from the Tegra PWM driver by NVIDIA Corporation.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/pwm.h>
+
+#define NPWM 2
+
+/* Address Mappings (offsets) from Broadcom BCM2835 ARM Periperals Manual
+ * Section 9.6 Page 141ff
+ */
+#define BCM2835_PWM_CTL 0x00 /* Control register */
+#define BCM2835_PWM_STA 0x04 /* Status register */
+#define BCM2835_PWM_DMAC 0x08 /* PWM DMA Configuration */
+#define BCM2835_PWM_RNG1 0x10 /* PWM Channel 1 Range */
+#define BCM2835_PWM_DAT1 0x14 /* PWM Channel 1 Data */
+#define BCM2835_PWM_FIF1 0x18 /* PWM FIFO Input */
+#define BCM2835_PWM_RNG2 0x20 /* PWM Channel 2 Range */
+#define BCM2835_PWM_DAT2 0x24 /* PWM Channel 2 Data */
+
+/* Control registers 0 and 1 are mirrored on a distance of 4 bits */
+#define HWPWM(x) ((x) << 4)
+
+/* TODO: We only need this register set once and use HWPWM macro to
+ * access 2nd output
+ */
+/* Control Register Bits */
+#define BCM2835_PWM_CTL_PWEN1 BIT(0) /* Channel 1 enable (RW) */
+#define BCM2835_PWM_CTL_MODE1 BIT(1) /* Channel 1 mode (RW) */
+#define BCM2835_PWM_CTL_RPTL1 BIT(2) /* Channel 1 repeat last data (RW) */
+#define BCM2835_PWM_CTL_SBIT1 BIT(3) /* Channel 1 silence bit (RW) */
+#define BCM2835_PWM_CTL_POLA1 BIT(4) /* Channel 1 polarity (RW) */
+#define BCM2835_PWM_CTL_USEF1 BIT(5) /* Channel 1 use FIFO (RW) */
+#define BCM2835_PWM_CTL_CLRF1 BIT(6) /* Channel 1 clear FIFO (RO) */
+#define BCM2835_PWM_CTL_MSEN1 BIT(7) /* Channel 1 M/S enable (RW) */
+#define BCM2835_PWM_CTL_PWEN2 BIT(8) /* Channel 2 enable (RW) */
+#define BCM2835_PWM_CTL_MODE2 BIT(9) /* Channel 2 mode (RW) */
+#define BCM2835_PWM_CTL_RPTL2 BIT(10) /* Channel 2 repeat last data (RW) */
+#define BCM2835_PWM_CTL_SBIT2 BIT(11) /* Channel 2 silence bit (RW) */
+#define BCM2835_PWM_CTL_POLA2 BIT(12) /* Channel 2 polarity (RW) */
+#define BCM2835_PWM_CTL_USEF2 BIT(13) /* Channel 2 use FIFO (RW) */
+/* Bit 14 is reserved */
+#define BCM2835_PWM_MSEN2 BIT(15) /* Channel 2 M/S enable (RW) */
+/* Bits 16 - 31 are reserved */
+
+/* Status Register Bits */
+#define BCM2835_PWM_STA_FULL1 BIT(0) /* FIFO full flag (RW) */
+#define BCM2835_PWM_STA_EMPT1 BIT(1) /* FIFO empty flag (RW) */
+#define BCM2835_PWM_STA_WERR1 BIT(2) /* FIFO write error flag (RW) */
+#define BCM2835_PWM_STA_RERR1 BIT(3) /* FIFO read error flag (RW) */
+#define BCM2835_PWM_STA_GAPO1 BIT(4) /* Channel 1 gap occured (RW) */
+#define BCM2835_PWM_STA_GAPO2 BIT(5) /* Channel 2 gap occured (RW) */
+#define BCM2835_PWM_STA_GAPO3 BIT(6) /* Channel 3 gap occured (RW) */
+#define BCM2835_PWM_STA_GAPO4 BIT(7) /* Channel 4 gap occured (RW) */
+#define BCM2835_PWM_STA_BERR BIT(8) /* Bus error flag (RW) */
+#define BCM2835_PWM_STA_STA1 BIT(9) /* Channel 1 state (RW) */
+#define BCM2835_PWM_STA_STA2 BIT(10) /* Channel 1 state (RW) */
+#define BCM2835_PWM_STA_STA3 BIT(11) /* Channel 1 state (RW) */
+#define BCM2835_PWM_STA_STA4 BIT(12) /* Channel 1 state (RW) */
+/* Bits 13 - 31 are reserved */
+
+struct bcm2835_pwm_dev {
+ struct pwm_chip chip;
+ struct device *dev;
+ struct clk *clk;
+ void __iomem *mmio_base;
+};
+
+static inline struct bcm2835_pwm_dev *to_bcm(struct pwm_chip *chip)
+{
+ return container_of(chip, struct bcm2835_pwm_dev, chip);
+}
+
+static inline u32 bcm2835_readl(struct bcm2835_pwm_dev *chip,
+ unsigned int hwpwm, unsigned int num)
+{
+ num <<= HWPWM(hwpwm);
+ return readl(chip->mmio_base + num);
+}
+
+static inline void bcm2835_writel(struct bcm2835_pwm_dev *chip,
+ unsigned int hwpwm, unsigned int num,
+ unsigned long val)
+{
+ num <<= HWPWM(hwpwm);
+ writel(val, chip->mmio_base + num);
+}
+
+static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+ int duty_ns, int period_ns)
+{
+ struct bcm2835_pwm_dev *bcm = to_bcm(chip);
+
+ if (WARN_ON(!bcm))
+ return -ENODEV;
+
+ if (duty_ns < 1) {
+ dev_err(bcm->dev, "duty is out of range: %d < 1\n", duty_ns);
+ return -ERANGE;
+ }
+
+ if (period_ns < 1) {
+ dev_err(bcm->dev, "period is out of range: %d < 1\n",
+ period_ns);
+ return -ERANGE;
+ }
+
+ /* disable PWM */
+ bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, 0);
+
+ /* write period */
+ bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_RNG1, period_ns);
+
+ /* write duty */
+ bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_DAT1, duty_ns);
+
+ /* start PWM */
+ bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, BCM2835_PWM_CTL_PWEN1);
+
+ return 0;
+}
+
+static int bcm2835_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct bcm2835_pwm_dev *bcm = to_bcm(chip);
+ int rc = 0;
+ u32 val;
+
+ if (WARN_ON(!bcm))
+ return -ENODEV;
+
+ rc = clk_prepare_enable(bcm->clk);
+ if (rc < 0)
+ return rc;
+
+ val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
+ val |= BCM2835_PWM_CTL_PWEN1;
+ bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, val);
+
+ return 0;
+}
+
+static void bcm2835_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct bcm2835_pwm_dev *bcm = to_bcm(chip);
+ u32 val;
+
+ if (WARN_ON(!bcm))
+ return;
+
+ val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
+ val &= ~BCM2835_PWM_CTL_PWEN1;
+ bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, val);
+
+ clk_disable_unprepare(bcm->clk);
+}
+
+static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+ enum pwm_polarity polarity)
+{
+ struct bcm2835_pwm_dev *bcm = to_bcm(chip);
+ u32 val;
+
+ if (WARN_ON(!bcm))
+ return -ENODEV;
+
+ val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
+
+ if (polarity == PWM_POLARITY_NORMAL)
+ val |= BCM2835_PWM_CTL_POLA1;
+ else
+ val &= ~BCM2835_PWM_CTL_POLA1;
+
+ 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,
+};
+
+static int bcm2835_pwm_probe(struct platform_device *pdev)
+{
+ struct bcm2835_pwm_dev *bcm;
+ struct resource *r;
+ int ret;
+
+ bcm = devm_kzalloc(&pdev->dev, sizeof(*bcm), GFP_KERNEL);
+ if (!bcm)
+ return -ENOMEM;
+
+ r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!r) {
+ dev_err(&pdev->dev, "no memory resource defined\n");
+ return -ENODEV;
+ }
+
+ bcm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
+ if (IS_ERR(bcm->mmio_base))
+ return PTR_ERR(bcm->mmio_base);
+
+ bcm->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(bcm->clk))
+ return PTR_ERR(bcm->clk);
+
+ clk_prepare_enable(bcm->clk);
+
+ bcm->chip.ops = &bcm2835_pwm_ops;
+ bcm->chip.dev = &pdev->dev;
+ bcm->chip.base = -1;
+ bcm->chip.npwm = NPWM;
+
+ ret = pwmchip_add(&bcm->chip);
+ if (ret < 0)
+ return ret;
+
+ platform_set_drvdata(pdev, bcm);
+
+ return 0;
+}
+
+static int bcm2835_pwm_remove(struct platform_device *pdev)
+{
+ struct bcm2835_pwm_dev *bcm = platform_get_drvdata(pdev);
+
+ if (WARN_ON(!bcm))
+ return -ENODEV;
+
+ clk_disable_unprepare(bcm->clk);
+
+ return pwmchip_remove(&bcm->chip);
+}
+
+static const struct of_device_id bcm2835_pwm_of_match[] = {
+ { .compatible = "brcm,bcm2835-pwm" },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match);
+
+static struct platform_driver bcm2835_pwm_driver = {
+ .probe = bcm2835_pwm_probe,
+ .remove = bcm2835_pwm_remove,
+ .driver = {
+ .name = "pwm-bcm2835",
+ .owner = THIS_MODULE,
+ .of_match_table = bcm2835_pwm_of_match,
+ },
+};
+
+module_platform_driver(bcm2835_pwm_driver);
+
+MODULE_AUTHOR("Johannes Thumshirn <[email protected]>");
+MODULE_DESCRIPTION("BCM2835 PWM driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:pwm-bcm2835");
--
1.8.4
On Sat, Sep 21, 2013 at 12:09:02PM +0200, Johannes Thumshirn wrote:
The subject prefix should be "pwm: ". Also something like this might be
better as a subject:
pwm: Add BCM2835 SoC PWM driver
Then go on to describe that the BCM2835 is used in the Raspberry Pi.
> Add support for the PWM controller of the BCM2835 SoC found on Raspberry PI
>
> The driver isn't as much tested as I wanted it to be and devicetree
> support is still missing, but I thought it would be nice to have some
> comments if I'm in the right direction.
>
> Signed-off-by: Johannes Thumshirn <[email protected]>
> ---
> drivers/pwm/Kconfig | 8 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-bcm2835.c | 275 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 284 insertions(+)
> create mode 100644 drivers/pwm/pwm-bcm2835.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 75840b5..5417159 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -224,4 +224,12 @@ config PWM_VT8500
> To compile this driver as a module, choose M here: the module
> will be called pwm-vt8500.
>
> +config PWM_BCM2835
> + tristate "BCM2835 PWM support"
> + help
> + Generic PWM framework driver for bcm2835 found on the Rasperry PI
> +
> + To compile this driver as a module, choose M here: the module will be
> + called pwm-bcm2835
This uses a mixture of spaces and tabs for indentation. It should use
tabs to indent, plus another 2 spaces for the help description like all
the other entries.
Also, please keep the list sorted alphabetically.
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
[...]
> @@ -20,3 +20,4 @@ obj-$(CONFIG_PWM_TIPWMSS) += pwm-tipwmss.o
> obj-$(CONFIG_PWM_TWL) += pwm-twl.o
> obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
> obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
> +obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
That list should also be ordered alphabetically.
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
[...]
> new file mode 100644
> index 0000000..8a64666
> --- /dev/null
> +++ b/drivers/pwm/pwm-bcm2835.c
> @@ -0,0 +1,275 @@
> +/*
> + * BCM2835 PWM driver
> + *
> + * Derived from the Tegra PWM driver by NVIDIA Corporation.
I don't think you necessarily need to credit here. The general structure
of PWM drivers is dictated by the subsystem, and besides that there's
nothing in here that borrows from the Tegra PWM driver.
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/pwm.h>
These are almost sorted alphabetically. =)
> +
> +#define NPWM 2
You only use this once and the context leaves no room for interpretation
so you might just as well hard-code it.
> +
> +/* Address Mappings (offsets) from Broadcom BCM2835 ARM Periperals Manual
> + * Section 9.6 Page 141ff
> + */
> +#define BCM2835_PWM_CTL 0x00 /* Control register */
> +#define BCM2835_PWM_STA 0x04 /* Status register */
> +#define BCM2835_PWM_DMAC 0x08 /* PWM DMA Configuration */
> +#define BCM2835_PWM_RNG1 0x10 /* PWM Channel 1 Range */
> +#define BCM2835_PWM_DAT1 0x14 /* PWM Channel 1 Data */
> +#define BCM2835_PWM_FIF1 0x18 /* PWM FIFO Input */
> +#define BCM2835_PWM_RNG2 0x20 /* PWM Channel 2 Range */
> +#define BCM2835_PWM_DAT2 0x24 /* PWM Channel 2 Data */
> +
> +/* Control registers 0 and 1 are mirrored on a distance of 4 bits */
> +#define HWPWM(x) ((x) << 4)
That doesn't look right. You use this in the readl() and writel()
functions irrespective of which register is accessed. Given that only
two registers are per-channel, I think the easiest way would be for you
to differentiate between channel 0 and 1 when accessing the registers.
See below.
> +
> +/* TODO: We only need this register set once and use HWPWM macro to
> + * access 2nd output
> + */
> +/* Control Register Bits */
> +#define BCM2835_PWM_CTL_PWEN1 BIT(0) /* Channel 1 enable (RW) */
> +#define BCM2835_PWM_CTL_MODE1 BIT(1) /* Channel 1 mode (RW) */
> +#define BCM2835_PWM_CTL_RPTL1 BIT(2) /* Channel 1 repeat last data (RW) */
> +#define BCM2835_PWM_CTL_SBIT1 BIT(3) /* Channel 1 silence bit (RW) */
> +#define BCM2835_PWM_CTL_POLA1 BIT(4) /* Channel 1 polarity (RW) */
> +#define BCM2835_PWM_CTL_USEF1 BIT(5) /* Channel 1 use FIFO (RW) */
> +#define BCM2835_PWM_CTL_CLRF1 BIT(6) /* Channel 1 clear FIFO (RO) */
> +#define BCM2835_PWM_CTL_MSEN1 BIT(7) /* Channel 1 M/S enable (RW) */
> +#define BCM2835_PWM_CTL_PWEN2 BIT(8) /* Channel 2 enable (RW) */
> +#define BCM2835_PWM_CTL_MODE2 BIT(9) /* Channel 2 mode (RW) */
> +#define BCM2835_PWM_CTL_RPTL2 BIT(10) /* Channel 2 repeat last data (RW) */
> +#define BCM2835_PWM_CTL_SBIT2 BIT(11) /* Channel 2 silence bit (RW) */
> +#define BCM2835_PWM_CTL_POLA2 BIT(12) /* Channel 2 polarity (RW) */
> +#define BCM2835_PWM_CTL_USEF2 BIT(13) /* Channel 2 use FIFO (RW) */
> +/* Bit 14 is reserved */
> +#define BCM2835_PWM_MSEN2 BIT(15) /* Channel 2 M/S enable (RW) */
> +/* Bits 16 - 31 are reserved */
> +
> +/* Status Register Bits */
> +#define BCM2835_PWM_STA_FULL1 BIT(0) /* FIFO full flag (RW) */
> +#define BCM2835_PWM_STA_EMPT1 BIT(1) /* FIFO empty flag (RW) */
> +#define BCM2835_PWM_STA_WERR1 BIT(2) /* FIFO write error flag (RW) */
> +#define BCM2835_PWM_STA_RERR1 BIT(3) /* FIFO read error flag (RW) */
> +#define BCM2835_PWM_STA_GAPO1 BIT(4) /* Channel 1 gap occured (RW) */
> +#define BCM2835_PWM_STA_GAPO2 BIT(5) /* Channel 2 gap occured (RW) */
> +#define BCM2835_PWM_STA_GAPO3 BIT(6) /* Channel 3 gap occured (RW) */
> +#define BCM2835_PWM_STA_GAPO4 BIT(7) /* Channel 4 gap occured (RW) */
> +#define BCM2835_PWM_STA_BERR BIT(8) /* Bus error flag (RW) */
> +#define BCM2835_PWM_STA_STA1 BIT(9) /* Channel 1 state (RW) */
> +#define BCM2835_PWM_STA_STA2 BIT(10) /* Channel 1 state (RW) */
> +#define BCM2835_PWM_STA_STA3 BIT(11) /* Channel 1 state (RW) */
> +#define BCM2835_PWM_STA_STA4 BIT(12) /* Channel 1 state (RW) */
> +/* Bits 13 - 31 are reserved */
Quite a few of these seem to be unused, so you might want to consider
dropping them.
> +
> +struct bcm2835_pwm_dev {
Please drop the _dev suffix. It doesn't add any value besides possibly
leading to confusion vs. struct pwm_device. It's really the chip that
this implements.
> + struct pwm_chip chip;
> + struct device *dev;
This is used but never initialized. But since the same field is already
contained in struct pwm_chip you can probably just use that and drop it
here.
> + struct clk *clk;
> + void __iomem *mmio_base;
I'd go with either "base" or "mmio". I know... other drivers use this as
well, including the Tegra one that this was based on. But I don't see
much gain in the additional 5 characters.
> +static inline struct bcm2835_pwm_dev *to_bcm(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct bcm2835_pwm_dev, chip);
> +}
> +
> +static inline u32 bcm2835_readl(struct bcm2835_pwm_dev *chip,
> + unsigned int hwpwm, unsigned int num)
"unsigned int num" -> "unsigned long offset"? And drop hwpwm since it
doesn't make sense for most registers.
> +{
> + num <<= HWPWM(hwpwm);
> + return readl(chip->mmio_base + num);
As already hinted at above, just make this:
return readl(chip->base + offset);
> +static inline void bcm2835_writel(struct bcm2835_pwm_dev *chip,
> + unsigned int hwpwm, unsigned int num,
> + unsigned long val)
I think it'd be clearer to keep the arguments in the same order as the
writel() function so that there's less potential for confusion:
static inline void bcm2835_writel(struct bcm2835_pwm *chip,
unsigned long value,
unsigned long offset)
> +{
> + num <<= HWPWM(hwpwm);
> + writel(val, chip->mmio_base + num);
> +}
And:
writel(value, chip->base + offset);
Perhaps it's not even worth defining these. Note how it's actually
shorter to write:
writel(value, chip->base + BCM2835_PWM_CTL);
Rather than:
bcm2835_writel(chip, value, BCM2835_PWM_CTL);
And it's much easier to parse because everyone knows readl()/writel().
> +static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> +{
> + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> +
> + if (WARN_ON(!bcm))
> + return -ENODEV;
That's never going to happen.
> +
> + if (duty_ns < 1) {
> + dev_err(bcm->dev, "duty is out of range: %d < 1\n", duty_ns);
> + return -ERANGE;
> + }
> +
> + if (period_ns < 1) {
> + dev_err(bcm->dev, "period is out of range: %d < 1\n",
> + period_ns);
> + return -ERANGE;
> + }
O.o... So you effectively only allow duty_ns == 0 and period_ns = 0
here? That can't work.
> + /* disable PWM */
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, 0);
If you've changed the polarity, that change will be lost here. I suggest
something like:
value = bcm2835_readl(bcm, BCM2835_PWM_CTL);
value &= ~BCM2835_PWM_CTL_EN(pwm->hwpwm);
bcm2835_writel(bcm, value, BCM2835_PWM_CTL);
Where:
#define BCM2835_PWM_CTL_EN(x) (BIT((x) * 8))
> + /* write period */
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_RNG1, period_ns);
> +
> + /* write duty */
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_DAT1, duty_ns);
I'd suggest something like this:
#define BCM2835_PWM_RNG(x) (0x10 + (x) * 16)
#define BCM2835_PWM_DAT(x) (0x14 + (x) * 16)
> + /* start PWM */
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, BCM2835_PWM_CTL_PWEN1);
Similar as for disabling the PWM. And you can get rid of the comments in
my opinion, because it is pretty obvious from the code what's happening.
> +static int bcm2835_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> + int rc = 0;
No need to initialize here.
> + u32 val;
> +
> + if (WARN_ON(!bcm))
> + return -ENODEV;
Never going to happen.
> +
> + rc = clk_prepare_enable(bcm->clk);
> + if (rc < 0)
> + return rc;
I don't think you want to prepare again. This should probably just be
rc = clk_enable(bcm->clk);
Well, it depends. Can the registers be accessed without the clock
running? Or do you need the clock to be running before accessing
registers?
> + val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
> + val |= BCM2835_PWM_CTL_PWEN1;
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, val);
Same comment as for bcm2835_pwm_config().
> +static void bcm2835_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> + u32 val;
> +
> + if (WARN_ON(!bcm))
> + return;
Not needed.
> + val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
> + val &= ~BCM2835_PWM_CTL_PWEN1;
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, val);
Same comment as for bcm2835_pwm_enable().
> +
> + clk_disable_unprepare(bcm->clk);
You don't want to unprepare the clock here. Just disable.
> +}
> +
> +static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> + enum pwm_polarity polarity)
> +{
> + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> + u32 val;
> +
> + if (WARN_ON(!bcm))
> + return -ENODEV;
Not needed.
> +
> + val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
> +
> + if (polarity == PWM_POLARITY_NORMAL)
> + val |= BCM2835_PWM_CTL_POLA1;
> + else
> + val &= ~BCM2835_PWM_CTL_POLA1;
This seems to be the wrong way around.
> +
> + return 0;
And you never write back the new register value, making this function a
no-op.
> +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,
> +};
You need to set the .owner field. And no aligning of the =, please. It's
inconsistent anyway.
> +
> +static int bcm2835_pwm_probe(struct platform_device *pdev)
> +{
> + struct bcm2835_pwm_dev *bcm;
> + struct resource *r;
> + int ret;
> +
> + bcm = devm_kzalloc(&pdev->dev, sizeof(*bcm), GFP_KERNEL);
> + if (!bcm)
> + return -ENOMEM;
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!r) {
> + dev_err(&pdev->dev, "no memory resource defined\n");
> + return -ENODEV;
> + }
devm_ioremap_resource() checks the resource for validity, so you no
longer have to do that yourself.
> + bcm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> + if (IS_ERR(bcm->mmio_base))
> + return PTR_ERR(bcm->mmio_base);
> +
> + bcm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(bcm->clk))
> + return PTR_ERR(bcm->clk);
> +
> + clk_prepare_enable(bcm->clk);
This can fail, so you should check for errors and propagate them. Also
perhaps this should only be clk_prepare()? It depends: if register
accesses need the clock, then it probably makes sense to enable it here
already so you don't have to worry. If you can access the registers if
the clock isn't running, then just enable it when you enable each PWM
channel (in bcm2835_pwm_enable()).
Even if the register accesses require a clock, there's the possibility
to save some amount of power by only enabling the clock when the
registers are actually accessed. But perhaps that's not necessary.
> +static int bcm2835_pwm_remove(struct platform_device *pdev)
> +{
> + struct bcm2835_pwm_dev *bcm = platform_get_drvdata(pdev);
> +
> + if (WARN_ON(!bcm))
> + return -ENODEV;
Don't bother. bcm will never be NULL here.
> + clk_disable_unprepare(bcm->clk);
> +
> + return pwmchip_remove(&bcm->chip);
> +}
> +
> +static const struct of_device_id bcm2835_pwm_of_match[] = {
> + { .compatible = "brcm,bcm2835-pwm" },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match);
I prefer no blank line between these.
> +static struct platform_driver bcm2835_pwm_driver = {
> + .probe = bcm2835_pwm_probe,
> + .remove = bcm2835_pwm_remove,
> + .driver = {
> + .name = "pwm-bcm2835",
This should be "bcm2835-pwm" for consistency with other drivers.
> + .owner = THIS_MODULE,
> + .of_match_table = bcm2835_pwm_of_match,
Please don't align these since you'll eventually fail anyway. Just a
single space before and after the equal sign is just fine.
> + },
> +};
> +
> +module_platform_driver(bcm2835_pwm_driver);
I prefer no blank line between these.
> +
> +MODULE_AUTHOR("Johannes Thumshirn <[email protected]>");
> +MODULE_DESCRIPTION("BCM2835 PWM driver");
> +MODULE_LICENSE("GPL");
According to the header comment, this should actually be "GPL v2".
Thierry
On 09/21/2013 04:09 AM, Johannes Thumshirn wrote:
> Add support for the PWM controller of the BCM2835 SoC found on Raspberry PI
>
> The driver isn't as much tested as I wanted it to be and devicetree
> support is still missing, but I thought it would be nice to have some
> comments if I'm in the right direction.
Presumably if DT support is missing, the driver can't have been tested
at all?
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> +/* Address Mappings (offsets) from Broadcom BCM2835 ARM Periperals Manual
> + * Section 9.6 Page 141ff
s/141ff/141/
> +/* Control registers 0 and 1 are mirrored on a distance of 4 bits */
> +#define HWPWM(x) ((x) << 4)
Distances between registers aren't really measured in bits although
admittedly a certain minimum number of bits would be required to
represent that distance. I would suggest s/4 bits/16 bytes/. People are
likely to be familiar with "<< 4" being equivalent to "* 16".
> +/* TODO: We only need this register set once and use HWPWM macro to
> + * access 2nd output
> + */
I don't really understand what the TODO message says still needs to be
done. The comment format is wrong; /* should be on its own line.
> +static inline u32 bcm2835_readl(struct bcm2835_pwm_dev *chip,
> + unsigned int hwpwm, unsigned int num)
> +{
> + num <<= HWPWM(hwpwm);
This doesn't work for hwpwm==1; HWPWM() already shifted the ID left, so
you want "num += HWPWM(hwpwm)" here, I think. But as Thierry said, this
function really doesn't need to do anything other than take an offset.
Same comment for bcm2835_writel().
BTW, bcm2835_readl() isn't a good name for a PWM-specific function,
since it's pretty generic. While the name is static, so it won't cause
linker problems, it might confuse a symbolic debugger. Better make it
bcm2835_pwm_readl().
> +static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> + int duty_ns, int period_ns)
> + /* write period */
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_RNG1, period_ns);
> +
> + /* write duty */
> + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_DAT1, duty_ns);
Presumably those registers are a count of clock cycles, not an absolute
time in ns. The two are only equal if the PWM module clock runs at 1GHz.
So, don't you need to convert time to clock cycle count here, using
clk_get_rate(pwm_clk) as the ratio?
> +static int bcm2835_pwm_probe(struct platform_device *pdev)
...
> + bcm = devm_kzalloc(&pdev->dev, sizeof(*bcm), GFP_KERNEL);
> + if (!bcm)
> + return -ENOMEM;
...
> + ret = pwmchip_add(&bcm->chip);
> + if (ret < 0)
> + return ret;
> +
> + platform_set_drvdata(pdev, bcm);
You should really set the drvdata as soon as you've allocated it, and at
least before calling pwmchip_add(); as soon as you've added the PWM
chip, the PWM core could call functions on the PWM object, and if those
callbacks rely on the drvdata, then they're going to fail if the drvdata
hasn't been set yet.
> +static const struct of_device_id bcm2835_pwm_of_match[] = {
> + { .compatible = "brcm,bcm2835-pwm" },
> + {},
> +};
You need to write a DT binding document if you're going to ad DT support.
On Mon, Sep 23, 2013 at 11:45:48AM +0200, Thierry Reding wrote:
> On Sat, Sep 21, 2013 at 12:09:02PM +0200, Johannes Thumshirn wrote:
>
> The subject prefix should be "pwm: ". Also something like this might be
> better as a subject:
>
> pwm: Add BCM2835 SoC PWM driver
>
> Then go on to describe that the BCM2835 is used in the Raspberry Pi.
>
> > Add support for the PWM controller of the BCM2835 SoC found on Raspberry PI
> >
> > The driver isn't as much tested as I wanted it to be and devicetree
> > support is still missing, but I thought it would be nice to have some
> > comments if I'm in the right direction.
> >
> > Signed-off-by: Johannes Thumshirn <[email protected]>
> > ---
> > drivers/pwm/Kconfig | 8 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-bcm2835.c | 275 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 284 insertions(+)
> > create mode 100644 drivers/pwm/pwm-bcm2835.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 75840b5..5417159 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -224,4 +224,12 @@ config PWM_VT8500
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-vt8500.
> >
> > +config PWM_BCM2835
> > + tristate "BCM2835 PWM support"
> > + help
> > + Generic PWM framework driver for bcm2835 found on the Rasperry PI
> > +
> > + To compile this driver as a module, choose M here: the module will be
> > + called pwm-bcm2835
>
> This uses a mixture of spaces and tabs for indentation. It should use
> tabs to indent, plus another 2 spaces for the help description like all
> the other entries.
>
> Also, please keep the list sorted alphabetically.
>
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> [...]
> > @@ -20,3 +20,4 @@ obj-$(CONFIG_PWM_TIPWMSS) += pwm-tipwmss.o
> > obj-$(CONFIG_PWM_TWL) += pwm-twl.o
> > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o
> > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o
> > +obj-$(CONFIG_PWM_BCM2835) += pwm-bcm2835.o
>
> That list should also be ordered alphabetically.
>
> > diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> [...]
> > new file mode 100644
> > index 0000000..8a64666
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-bcm2835.c
> > @@ -0,0 +1,275 @@
> > +/*
> > + * BCM2835 PWM driver
> > + *
> > + * Derived from the Tegra PWM driver by NVIDIA Corporation.
>
> I don't think you necessarily need to credit here. The general structure
> of PWM drivers is dictated by the subsystem, and besides that there's
> nothing in here that borrows from the Tegra PWM driver.
>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/pwm.h>
>
> These are almost sorted alphabetically. =)
>
> > +
> > +#define NPWM 2
>
> You only use this once and the context leaves no room for interpretation
> so you might just as well hard-code it.
>
> > +
> > +/* Address Mappings (offsets) from Broadcom BCM2835 ARM Periperals Manual
> > + * Section 9.6 Page 141ff
> > + */
> > +#define BCM2835_PWM_CTL 0x00 /* Control register */
> > +#define BCM2835_PWM_STA 0x04 /* Status register */
> > +#define BCM2835_PWM_DMAC 0x08 /* PWM DMA Configuration */
> > +#define BCM2835_PWM_RNG1 0x10 /* PWM Channel 1 Range */
> > +#define BCM2835_PWM_DAT1 0x14 /* PWM Channel 1 Data */
> > +#define BCM2835_PWM_FIF1 0x18 /* PWM FIFO Input */
> > +#define BCM2835_PWM_RNG2 0x20 /* PWM Channel 2 Range */
> > +#define BCM2835_PWM_DAT2 0x24 /* PWM Channel 2 Data */
> > +
> > +/* Control registers 0 and 1 are mirrored on a distance of 4 bits */
> > +#define HWPWM(x) ((x) << 4)
>
> That doesn't look right. You use this in the readl() and writel()
> functions irrespective of which register is accessed. Given that only
> two registers are per-channel, I think the easiest way would be for you
> to differentiate between channel 0 and 1 when accessing the registers.
> See below.
>
> > +
> > +/* TODO: We only need this register set once and use HWPWM macro to
> > + * access 2nd output
> > + */
> > +/* Control Register Bits */
> > +#define BCM2835_PWM_CTL_PWEN1 BIT(0) /* Channel 1 enable (RW) */
> > +#define BCM2835_PWM_CTL_MODE1 BIT(1) /* Channel 1 mode (RW) */
> > +#define BCM2835_PWM_CTL_RPTL1 BIT(2) /* Channel 1 repeat last data (RW) */
> > +#define BCM2835_PWM_CTL_SBIT1 BIT(3) /* Channel 1 silence bit (RW) */
> > +#define BCM2835_PWM_CTL_POLA1 BIT(4) /* Channel 1 polarity (RW) */
> > +#define BCM2835_PWM_CTL_USEF1 BIT(5) /* Channel 1 use FIFO (RW) */
> > +#define BCM2835_PWM_CTL_CLRF1 BIT(6) /* Channel 1 clear FIFO (RO) */
> > +#define BCM2835_PWM_CTL_MSEN1 BIT(7) /* Channel 1 M/S enable (RW) */
> > +#define BCM2835_PWM_CTL_PWEN2 BIT(8) /* Channel 2 enable (RW) */
> > +#define BCM2835_PWM_CTL_MODE2 BIT(9) /* Channel 2 mode (RW) */
> > +#define BCM2835_PWM_CTL_RPTL2 BIT(10) /* Channel 2 repeat last data (RW) */
> > +#define BCM2835_PWM_CTL_SBIT2 BIT(11) /* Channel 2 silence bit (RW) */
> > +#define BCM2835_PWM_CTL_POLA2 BIT(12) /* Channel 2 polarity (RW) */
> > +#define BCM2835_PWM_CTL_USEF2 BIT(13) /* Channel 2 use FIFO (RW) */
> > +/* Bit 14 is reserved */
> > +#define BCM2835_PWM_MSEN2 BIT(15) /* Channel 2 M/S enable (RW) */
> > +/* Bits 16 - 31 are reserved */
> > +
> > +/* Status Register Bits */
> > +#define BCM2835_PWM_STA_FULL1 BIT(0) /* FIFO full flag (RW) */
> > +#define BCM2835_PWM_STA_EMPT1 BIT(1) /* FIFO empty flag (RW) */
> > +#define BCM2835_PWM_STA_WERR1 BIT(2) /* FIFO write error flag (RW) */
> > +#define BCM2835_PWM_STA_RERR1 BIT(3) /* FIFO read error flag (RW) */
> > +#define BCM2835_PWM_STA_GAPO1 BIT(4) /* Channel 1 gap occured (RW) */
> > +#define BCM2835_PWM_STA_GAPO2 BIT(5) /* Channel 2 gap occured (RW) */
> > +#define BCM2835_PWM_STA_GAPO3 BIT(6) /* Channel 3 gap occured (RW) */
> > +#define BCM2835_PWM_STA_GAPO4 BIT(7) /* Channel 4 gap occured (RW) */
> > +#define BCM2835_PWM_STA_BERR BIT(8) /* Bus error flag (RW) */
> > +#define BCM2835_PWM_STA_STA1 BIT(9) /* Channel 1 state (RW) */
> > +#define BCM2835_PWM_STA_STA2 BIT(10) /* Channel 1 state (RW) */
> > +#define BCM2835_PWM_STA_STA3 BIT(11) /* Channel 1 state (RW) */
> > +#define BCM2835_PWM_STA_STA4 BIT(12) /* Channel 1 state (RW) */
> > +/* Bits 13 - 31 are reserved */
>
> Quite a few of these seem to be unused, so you might want to consider
> dropping them.
>
> > +
> > +struct bcm2835_pwm_dev {
>
> Please drop the _dev suffix. It doesn't add any value besides possibly
> leading to confusion vs. struct pwm_device. It's really the chip that
> this implements.
>
> > + struct pwm_chip chip;
> > + struct device *dev;
>
> This is used but never initialized. But since the same field is already
> contained in struct pwm_chip you can probably just use that and drop it
> here.
>
> > + struct clk *clk;
> > + void __iomem *mmio_base;
>
> I'd go with either "base" or "mmio". I know... other drivers use this as
> well, including the Tegra one that this was based on. But I don't see
> much gain in the additional 5 characters.
>
> > +static inline struct bcm2835_pwm_dev *to_bcm(struct pwm_chip *chip)
> > +{
> > + return container_of(chip, struct bcm2835_pwm_dev, chip);
> > +}
> > +
> > +static inline u32 bcm2835_readl(struct bcm2835_pwm_dev *chip,
> > + unsigned int hwpwm, unsigned int num)
>
> "unsigned int num" -> "unsigned long offset"? And drop hwpwm since it
> doesn't make sense for most registers.
>
> > +{
> > + num <<= HWPWM(hwpwm);
> > + return readl(chip->mmio_base + num);
>
> As already hinted at above, just make this:
>
> return readl(chip->base + offset);
>
> > +static inline void bcm2835_writel(struct bcm2835_pwm_dev *chip,
> > + unsigned int hwpwm, unsigned int num,
> > + unsigned long val)
>
> I think it'd be clearer to keep the arguments in the same order as the
> writel() function so that there's less potential for confusion:
>
> static inline void bcm2835_writel(struct bcm2835_pwm *chip,
> unsigned long value,
> unsigned long offset)
>
> > +{
> > + num <<= HWPWM(hwpwm);
> > + writel(val, chip->mmio_base + num);
> > +}
>
> And:
>
> writel(value, chip->base + offset);
>
> Perhaps it's not even worth defining these. Note how it's actually
> shorter to write:
>
> writel(value, chip->base + BCM2835_PWM_CTL);
>
> Rather than:
>
> bcm2835_writel(chip, value, BCM2835_PWM_CTL);
>
> And it's much easier to parse because everyone knows readl()/writel().
>
> > +static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > + int duty_ns, int period_ns)
> > +{
> > + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> > +
> > + if (WARN_ON(!bcm))
> > + return -ENODEV;
>
> That's never going to happen.
>
> > +
> > + if (duty_ns < 1) {
> > + dev_err(bcm->dev, "duty is out of range: %d < 1\n", duty_ns);
> > + return -ERANGE;
> > + }
> > +
> > + if (period_ns < 1) {
> > + dev_err(bcm->dev, "period is out of range: %d < 1\n",
> > + period_ns);
> > + return -ERANGE;
> > + }
>
> O.o... So you effectively only allow duty_ns == 0 and period_ns = 0
> here? That can't work.
>
> > + /* disable PWM */
> > + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, 0);
>
> If you've changed the polarity, that change will be lost here. I suggest
> something like:
>
> value = bcm2835_readl(bcm, BCM2835_PWM_CTL);
> value &= ~BCM2835_PWM_CTL_EN(pwm->hwpwm);
> bcm2835_writel(bcm, value, BCM2835_PWM_CTL);
>
> Where:
>
> #define BCM2835_PWM_CTL_EN(x) (BIT((x) * 8))
>
> > + /* write period */
> > + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_RNG1, period_ns);
> > +
> > + /* write duty */
> > + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_DAT1, duty_ns);
>
> I'd suggest something like this:
>
> #define BCM2835_PWM_RNG(x) (0x10 + (x) * 16)
> #define BCM2835_PWM_DAT(x) (0x14 + (x) * 16)
>
> > + /* start PWM */
> > + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, BCM2835_PWM_CTL_PWEN1);
>
> Similar as for disabling the PWM. And you can get rid of the comments in
> my opinion, because it is pretty obvious from the code what's happening.
>
> > +static int bcm2835_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> > + int rc = 0;
>
> No need to initialize here.
>
> > + u32 val;
> > +
> > + if (WARN_ON(!bcm))
> > + return -ENODEV;
>
> Never going to happen.
>
> > +
> > + rc = clk_prepare_enable(bcm->clk);
> > + if (rc < 0)
> > + return rc;
>
> I don't think you want to prepare again. This should probably just be
>
> rc = clk_enable(bcm->clk);
>
> Well, it depends. Can the registers be accessed without the clock
> running? Or do you need the clock to be running before accessing
> registers?
>
> > + val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
> > + val |= BCM2835_PWM_CTL_PWEN1;
> > + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, val);
>
> Same comment as for bcm2835_pwm_config().
>
> > +static void bcm2835_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> > + u32 val;
> > +
> > + if (WARN_ON(!bcm))
> > + return;
>
> Not needed.
>
> > + val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
> > + val &= ~BCM2835_PWM_CTL_PWEN1;
> > + bcm2835_writel(bcm, pwm->hwpwm, BCM2835_PWM_CTL, val);
>
> Same comment as for bcm2835_pwm_enable().
>
> > +
> > + clk_disable_unprepare(bcm->clk);
>
> You don't want to unprepare the clock here. Just disable.
>
> > +}
> > +
> > +static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > + enum pwm_polarity polarity)
> > +{
> > + struct bcm2835_pwm_dev *bcm = to_bcm(chip);
> > + u32 val;
> > +
> > + if (WARN_ON(!bcm))
> > + return -ENODEV;
>
> Not needed.
>
> > +
> > + val = bcm2835_readl(bcm, pwm->hwpwm, BCM2835_PWM_CTL);
> > +
> > + if (polarity == PWM_POLARITY_NORMAL)
> > + val |= BCM2835_PWM_CTL_POLA1;
> > + else
> > + val &= ~BCM2835_PWM_CTL_POLA1;
>
> This seems to be the wrong way around.
>
> > +
> > + return 0;
>
> And you never write back the new register value, making this function a
> no-op.
>
> > +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,
> > +};
>
> You need to set the .owner field. And no aligning of the =, please. It's
> inconsistent anyway.
>
> > +
> > +static int bcm2835_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct bcm2835_pwm_dev *bcm;
> > + struct resource *r;
> > + int ret;
> > +
> > + bcm = devm_kzalloc(&pdev->dev, sizeof(*bcm), GFP_KERNEL);
> > + if (!bcm)
> > + return -ENOMEM;
> > +
> > + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!r) {
> > + dev_err(&pdev->dev, "no memory resource defined\n");
> > + return -ENODEV;
> > + }
>
> devm_ioremap_resource() checks the resource for validity, so you no
> longer have to do that yourself.
>
> > + bcm->mmio_base = devm_ioremap_resource(&pdev->dev, r);
> > + if (IS_ERR(bcm->mmio_base))
> > + return PTR_ERR(bcm->mmio_base);
> > +
> > + bcm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(bcm->clk))
> > + return PTR_ERR(bcm->clk);
> > +
> > + clk_prepare_enable(bcm->clk);
>
> This can fail, so you should check for errors and propagate them. Also
> perhaps this should only be clk_prepare()? It depends: if register
> accesses need the clock, then it probably makes sense to enable it here
> already so you don't have to worry. If you can access the registers if
> the clock isn't running, then just enable it when you enable each PWM
> channel (in bcm2835_pwm_enable()).
>
> Even if the register accesses require a clock, there's the possibility
> to save some amount of power by only enabling the clock when the
> registers are actually accessed. But perhaps that's not necessary.
>
> > +static int bcm2835_pwm_remove(struct platform_device *pdev)
> > +{
> > + struct bcm2835_pwm_dev *bcm = platform_get_drvdata(pdev);
> > +
> > + if (WARN_ON(!bcm))
> > + return -ENODEV;
>
> Don't bother. bcm will never be NULL here.
>
> > + clk_disable_unprepare(bcm->clk);
> > +
> > + return pwmchip_remove(&bcm->chip);
> > +}
> > +
> > +static const struct of_device_id bcm2835_pwm_of_match[] = {
> > + { .compatible = "brcm,bcm2835-pwm" },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, bcm2835_pwm_of_match);
>
> I prefer no blank line between these.
>
> > +static struct platform_driver bcm2835_pwm_driver = {
> > + .probe = bcm2835_pwm_probe,
> > + .remove = bcm2835_pwm_remove,
> > + .driver = {
> > + .name = "pwm-bcm2835",
>
> This should be "bcm2835-pwm" for consistency with other drivers.
>
> > + .owner = THIS_MODULE,
> > + .of_match_table = bcm2835_pwm_of_match,
>
> Please don't align these since you'll eventually fail anyway. Just a
> single space before and after the equal sign is just fine.
>
> > + },
> > +};
> > +
> > +module_platform_driver(bcm2835_pwm_driver);
>
> I prefer no blank line between these.
>
> > +
> > +MODULE_AUTHOR("Johannes Thumshirn <[email protected]>");
> > +MODULE_DESCRIPTION("BCM2835 PWM driver");
> > +MODULE_LICENSE("GPL");
>
> According to the header comment, this should actually be "GPL v2".
>
> Thierry
Thanks for the review Thierry and Stephen. I'll post an update based
on your input (including dts bindings) as soon as possible. I also
have an oszilloscope now, so I can test.
Johannes