Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754385Ab3IXR7q (ORCPT ); Tue, 24 Sep 2013 13:59:46 -0400 Received: from mail-ea0-f170.google.com ([209.85.215.170]:57655 "EHLO mail-ea0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752823Ab3IXR7n (ORCPT ); Tue, 24 Sep 2013 13:59:43 -0400 Date: Tue, 24 Sep 2013 20:05:33 +0200 From: Johannes Thumshirn To: Thierry Reding , Stephen Warren Cc: linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org, linux-rpi-kernel@lists.infradead.org, Johannes Thumshirn Subject: Re: [RFC] PWM: Add support for pwm-bcm2835 Message-ID: <20130924180533.GA2390@sauron.fritz.box> References: <1379758142-7302-1-git-send-email-morbidrsa@gmail.com> <20130923094546.GD11881@ulmo> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130923094546.GD11881@ulmo> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15822 Lines: 477 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 > > --- > > 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > 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 "); > > +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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/