Messages in this thread | | | Date | Mon, 6 Oct 2014 15:24:05 +0200 | From | Thierry Reding <> | Subject | Re: [PATCHv7 1/2] pwm: Add Allwinner SoC support |
| |
On Wed, Sep 17, 2014 at 09:53:57PM +0200, Alexandre Belloni wrote: [...] > diff --git a/drivers/pwm/pwm-sunxi.c b/drivers/pwm/pwm-sunxi.c > new file mode 100644 > index 000000000000..643f84ea013e > --- /dev/null > +++ b/drivers/pwm/pwm-sunxi.c > @@ -0,0 +1,371 @@ > +/* > + * Driver for Allwinner Pulse Width Modulation Controller > + * > + * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
This is somewhat weird. So you are the copyright holder, not your employer? The email address is somewhat misleading.
> +#define BIT_CH(bit, chan) (bit << (chan * PWMCH_OFFSET))
bit and chan could use additional parentheses here for extra safety.
> +static const u32 prescaler_table[] = { > + 120, > + 180, > + 240, > + 360, > + 480, > + 0, > + 0, > + 0, > + 12000, > + 24000, > + 36000, > + 48000, > + 72000, > + 0, > + 0, > + 0, /* Actually 1 but tested separately */ > +};
Did I already mention that this was really weird?
> +static inline u32 sunxi_pwm_readl(struct sunxi_pwm_chip *chip, > + unsigned long offset) > +{ > + return readl(chip->base + offset); > +} > + > +
There's a gratuitous blank line here.
> +static int sunxi_pwm_probe(struct platform_device *pdev) > +{ [...] > + > + ret = pwmchip_add(&pwm->chip); > + if (ret < 0) { > + dev_err(&pdev->dev, "failed to add PWM chip %d\n", ret);
"chip: %d" to make it clear that it's not some kind of chip ID.
> + goto error; > + } > + > + ret = clk_prepare_enable(pwm->clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable PWM clock\n"); > + goto error;
Don't you want to remove the stale PWM chip here? Why not do this at the very end, after everything's been set up properly?
> +MODULE_ALIAS("platform:sunxi-pwm"); > +MODULE_AUTHOR("Alexandre Belloni <alexandre.belloni@free-electrons.com>"); > +MODULE_DESCRIPTION("Allwinner PWM driver");
Perhaps "Allwinner sunxi PWM driver"? Presumably Allwinner could at some point release a different family of SoCs.
Thierry [unhandled content-type:application/pgp-signature] | |