Re: [rfc]pwm: add BCM2835 PWM driver

[Tim Kryger]

On Thu, Apr 3, 2014 at 6:44 AM, Bart Tanghe <bart.tanghe@xxxxxxxxxxxxx> wrote:
> need some recommendation
> the memory mapped io registers of the bcm2835 pwm hardware are spreaded
> over the memory mapped io
> gpio config 0x20200004 - clk config 0x201010A0 - pwm configuration 0x2020C000
> to handle this, I've used the base address of the memory mapped io
> so I can use positive offsets

So the registers for this PWM are located in three distinct memory regions?

Just define those three regions in your platform data or device tree.

> +/*mmio regiser mapping*/
> +#define OFFSET_PWM             0x0020C000

You won't need this guy.

> +#define DUTY                   0x14
> +#define PERIOD                 0x10
> +#define CHANNEL                0x10
> +
> +#define OFFSET_CLK             0x001010A0

Also unnecessary.

> +#define DIV                    0x04
> +
> +#define OFFSET_ALT             0x00200004

This can go too.

> +
> +/*Value defines*/
> +/*pwm clock configuration*/
> +#define PWMCLK_CNTL_OFF (0x5A000000 | (1 << 5))
> +#define PWMCLK_CNTL_ON (0x5A000000 | 0x11)
> +
> +#define PWM_ENABLE     0x00000001
> +#define PWM_POLARITY   0x00000010
> +/*+-1MHz pwm clock*/
> +#define PWMCLK_DIV     (0x5A000000 | (19 << 12))
> +/*ALT5 mask gpio18*/
> +#define ALTOR          0x02000000
> +#define ALTAND         0xFAFFFFFF
> +/*pwm configuration*/
> +#define MASK_CTL_PWM   0x000000FF
> +#define CTL_PWM        0x00000081
> +
> +#define DRIVER_AUTHOR "Bart Tanghe <bart.tanghe@xxxxxxxxxxxxx>"
> +#define DRIVER_DESC   "A bcm2835 pwm driver - raspberry pi development platform\
> +- only gpio 18 channel0 available"
> +
> +unsigned long *ptrPWM;
> +unsigned long *ptrPERIOD;
> +unsigned long *ptrDUTY;
> +unsigned long *ptrCLK;
> +unsigned long *ptrALT;
> +unsigned long *ptrDIV;

You don't need any of these pointers and they most certainly should
not be globals.

> +
> +struct bcm2835_pwm_chip {
> +       struct pwm_chip chip;
> +       struct device *dev;
> +       int channel;
> +       void __iomem *mmio;

One pointer isn't going to be enough.  You need three.

I suggest renaming the first and adding two more:

void __iomem *base_pwm;
void __iomem *base_clk;
void __iomem *base_alt;

> +};
> +
> +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){

Please align arguments on subsequent lines with those on the first line.

> +
> +       struct bcm2835_pwm_chip *pc;
> +
> +       pc = container_of(chip, struct bcm2835_pwm_chip, chip);

You defined to_bcm2835_pwm_chip but aren't using it here.  Why?

> +
> +       iowrite32(duty_ns/1000, ptrDUTY);
> +       iowrite32(period_ns/1000, ptrPERIOD);
> +
> +       #ifdef DEBUG
> +               printk(KERN_DEBUG "period %x\n", (unsigned int)ptrPERIOD);
> +               printk(KERN_DEBUG "duty %x\n", (unsigned int)ptrDUTY);
> +       #endif
> +
> +       return 0;
> +}
> +
> +static int bcm2835_pwm_enable(struct pwm_chip *chip,
> +struct pwm_device *pwm){

Again, please line up arguments on subsequent lines with those on the first.

This applies to pretty much all subsequent functions as well.

> +       struct bcm2835_pwm_chip *pc;
> +
> +       pc = container_of(chip, struct bcm2835_pwm_chip, chip);
> +
> +       /*TODO: channel 1 enable*/
> +       #ifdef DEBUG
> +               printk(KERN_DEBUG "pwm label: %s\n", pwm->label);
> +               printk(KERN_DEBUG "pwm hwpwm: %d\n", pwm->hwpwm);
> +               printk(KERN_DEBUG "pwm pwm: %d\n", pwm->pwm);
> +       #endif
> +
> +       iowrite32(ioread32(ptrPWM) | PWM_ENABLE, ptrPWM);
> +       #ifdef DEBUG
> +               printk(KERN_DEBUG "pwm: %x\n", ioread32(ptrPWM));
> +       #endif
> +       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);
> +
> +       #ifdef DEBUG
> +               printk(KERN_DEBUG "pwm label: %s\n", pwm->label);
> +               printk(KERN_DEBUG "pwm hwpwm: %d\n", pwm->hwpwm);
> +               printk(KERN_DEBUG "pwm pwm: %d\n", pwm->pwm);
> +       #endif
> +
> +       iowrite32(ioread32(ptrPWM) & ~PWM_ENABLE, ptrPWM);
> +}
> +
> +static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +enum pwm_polarity polarity)
> +{
> +       if (polarity == PWM_POLARITY_NORMAL)
> +               iowrite32((ioread32(ptrPWM) & ~PWM_POLARITY), ptrPWM);
> +       else if (polarity == PWM_POLARITY_INVERSED)
> +               iowrite32((ioread32(ptrPWM) | PWM_POLARITY), ptrPWM);
> +
> +       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;
> +
> +       printk(KERN_DEBUG "pwm probe\n");

This seems a bit unnecessary and also not particularly informative.

> +
> +       pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +       if (!pwm) {
> +               dev_err(&pdev->dev, "failed to allocate memory\n");

No need to print your own warning here, you get one for free.

> +               return -ENOMEM;
> +       }
> +
> +       pwm->dev = &pdev->dev;
> +
> +       #ifdef DEBUG
> +               printk(KERN_DEBUG "id:%d\n", pdev->id);
> +       #endif
> +
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       pwm->mmio = devm_ioremap_resource(&pdev->dev, r);

You need to do this two more times for the other two regions.

something like:

r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
pwm->base_pwm = devm_ioremap_resource(&pdev->dev, r);

r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
pwm->base_clk = devm_ioremap_resource(&pdev->dev, r);

r = platform_get_resource(pdev, IORESOURCE_MEM, 2);
pwm->base_alt = devm_ioremap_resource(&pdev->dev, r);


> +       if (IS_ERR(pwm->mmio))
> +               return PTR_ERR(pwm->mmio);
> +
> +       start = r->start;
> +       end = r->end;

You don't need these.

Perform reads/writes with the base_pwm, base_clk, and base_alt pointers.

> +       ptrPWM = (long *)ioremap_nocache(start + OFFSET_PWM, 4);
> +       if (ptrPWM == NULL) {
> +               printk(KERN_ERR "ioremap REG_PWM failed\n");
> +               goto map_failed;
> +       }
> +
> +       ptrDUTY = (long *)ioremap_nocache(start + OFFSET_PWM + DUTY, 4);
> +       if (ptrDUTY == NULL) {
> +               printk(KERN_ERR "ioremap REG_DUTY failed\n");
> +               goto map_failed;
> +       }
> +
> +       ptrPERIOD = (long *)ioremap_nocache(
> +               start + OFFSET_PWM + PERIOD, 4);
> +       if (ptrDUTY == NULL) {
> +               printk(KERN_ERR "ioremap REG_DUTY failed\n");
> +               goto map_failed;
> +       }
> +
> +       ptrCLK = (long *)ioremap_nocache(start + OFFSET_CLK, 4);
> +       if (ptrCLK == NULL) {
> +               printk(KERN_ERR "ioremap PWMCLK_CNTL failed\n");
> +               goto map_failed;
> +       }
> +
> +       ptrALT = (long *)ioremap_nocache(start + OFFSET_ALT, 4);
> +       if (ptrALT == NULL) {
> +               printk(KERN_ERR "ioremap FUNC_SLCT_HEAT_PWM failed\n");
> +               goto map_failed;
> +       }
> +
> +       ptrDIV = (long *)ioremap_nocache(start + OFFSET_CLK + DIV, 4);
> +       if (ptrALT == NULL) {
> +               printk(KERN_ERR "ioremap pwmDIV failed\n");
> +               goto map_failed;
> +       }

You can remove all of these ioremap_nocache calls.

> +
> +       #ifdef DEBUG
> +               printk(KERN_DEBUG "io mem adres:%x %x %x\n",
> +                       (unsigned int)start+OFFSET_PWM, (unsigned int)start +
> +                       OFFSET_CLK, (unsigned int)start + OFFSET_ALT);
> +               printk(KERN_DEBUG "%x %x %x\n", (unsigned int)ptrPWM,
> +                       (unsigned int)ptrCLK, (unsigned int)ptrALT);
> +       #endif
> +
> +
> +       iowrite32((ioread32(ptrALT) & ALTAND) | ALTOR, ptrALT);
> +       /*disable the clock to set the dividere*/
> +       iowrite32(PWMCLK_CNTL_OFF, ptrCLK);
> +       /*pwm clock set to 1Mhz.*/
> +       iowrite32(PWMCLK_DIV, ptrDIV);
> +       /*enable the clock, load the new configuration*/
> +       iowrite32(PWMCLK_CNTL_ON, ptrCLK);
> +       /*set the pwm0 configuration*/
> +       iowrite32((ioread32(ptrPWM) & ~MASK_CTL_PWM) | CTL_PWM, ptrPWM);
> +
> +       #ifdef DEBUG
> +               /*duty cycle 1ms*/
> +               iowrite32(100000/1000, (unsigned long *)ptrDUTY);
> +               /*period 300 us*/
> +               iowrite32(300000/1000, (unsigned long *)ptrPERIOD);
> +               iowrite32(ioread32(ptrPWM) | PWM_ENABLE, ptrPWM);
> +       #endif

What is going on here?

> +
> +       return 0;
> +
> +map_failed:
> +       pwmchip_remove(&pwm->chip);
> +
> +chip_failed:
> +       devm_kfree(&pdev->dev, pwm);
> +       return -1;
> +
> +}
> +
> +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;
> +
> +       iounmap(ptrALT);
> +       iounmap(ptrPWM);
> +       iounmap(ptrCLK);
> +       iounmap(ptrDUTY);
> +       iounmap(ptrPERIOD);

Not necessary since you will be removing all the ioremaps

> +
> +
> +       return pwmchip_remove(&pc->chip);
> +}
> +
> +static const struct of_device_id bcm2835_pwm_of_match[] = {
> +       { .compatible = "rpi,pwm-bcm2835" },

Compatible string should be "brcm,bcm2835-pwm" in order to follow the
vendor prefix docs and align with what is done elsewhere.

Where is your binding description doc?

> +       { }
> +};
> +
> +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);

Additionally, you should switch over to using dev_dbg and pr_err as appropriate.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/