Date:	Tue, 24 Sep 2013 20:05:33 +0200
From:	Johannes Thumshirn <morbidrsa@...il.com>
To:	Thierry Reding <thierry.reding@...il.com>,
	Stephen Warren <swarren@...dotorg.org>
Cc:	linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
	linux-rpi-kernel@...ts.infradead.org,
	Johannes Thumshirn <morbidrsa@...il.com>
Subject: Re: [RFC] PWM: Add support for pwm-bcm2835

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 <morbidrsa@...il.com>
> > ---
> >  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 <morbidrsa@...il.com>");
> > +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists