| Message ID | 1399504115-16257-2-git-send-email-b.galvani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 08, 2014 at 01:08:33AM +0200, Beniamino Galvani wrote: > This commit adds a driver for the PWM controller found on Rockchip > RK29, RK30 and RK31 SoCs. > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > --- > drivers/pwm/Kconfig | 8 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-rockchip.c | 180 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 189 insertions(+) > create mode 100644 drivers/pwm/pwm-rockchip.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 5b34ff2..2e92245 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -197,6 +197,14 @@ config PWM_RENESAS_TPU > To compile this driver as a module, choose M here: the module > will be called pwm-renesas-tpu. > > +config PWM_ROCKCHIP > + tristate "Rockchip PWM support" > + depends on ARCH_ROCKCHIP > + depends on OF It seems like ARCH_ROCKCHIP depends on OF already (via ARCH_MULTI_V7 and ARCH_MULTIPLATFORM), so having the dependency explicitly here seems redundant. > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c [...] > +#define PRESCALER 2 > +#define NSECS_PER_SEC 1000000000 You should use NSEC_PER_SEC from include/linux/time.h. > +struct rockchip_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + void __iomem *base; > +}; I prefer no artificial padding within structure definitions. > +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) > +{ > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > + unsigned long clk_rate, period, duty; > + u64 div; > + int ret; > + > + clk_rate = clk_get_rate(pc->clk); > + > + /* > + * Since period and duty cycle registers have a width of 32 > + * bits, every possible input period can be obtained using the > + * default prescaler value for all practical clock rate values. > + */ > + div = clk_rate; > + div *= period_ns; Perhaps shorten this to "div = clk_rate * period_ns;"? > + do_div(div, PRESCALER * NSECS_PER_SEC); > + period = div; > + > + div = clk_rate; > + div *= duty_ns; And this to "div = clk_rate * duty_ns;"? > + do_div(div, PRESCALER * NSECS_PER_SEC); > + duty = div; > + > + ret = clk_enable(pc->clk); > + if (ret) > + return ret; > + > + writel(period, pc->base + PWM_LRC); > + writel(duty, pc->base + PWM_HRC); > + writel(0, pc->base + PWM_CNTR); > + > + clk_disable(pc->clk); > + > + return 0; > +} [...] > +static struct pwm_ops rockchip_pwm_ops = { static const please. > + .config = rockchip_pwm_config, There's a tab between .config and = above. It should be a space. > +static const struct of_device_id rockchip_pwm_dt_ids[] = { > + { .compatible = "rockchip,rk2928-pwm" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, rockchip_pwm_id_ids); The name in the MODULE_DEVICE_TABLE doesn't match the array name above. Does this even build? > +static struct platform_driver rockchip_pwm_driver = { > + .driver = { > + .name = "rockchip-pwm", > + .owner = THIS_MODULE, You no longer need to initialize this explicitly, module_platform_driver does it for you. > + .of_match_table = rockchip_pwm_dt_ids, > + }, > + .probe = rockchip_pwm_probe, There's another tab instead of space here. > + .remove = rockchip_pwm_remove, > +}; > +module_platform_driver(rockchip_pwm_driver); > + > +MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>"); > +MODULE_DESCRIPTION("Rockchip PWM driver"); Perhaps "Rockchip SoC PWM driver"? Thierry
On Tue, Jun 17, 2014 at 11:42:58PM +0200, Thierry Reding wrote: > On Thu, May 08, 2014 at 01:08:33AM +0200, Beniamino Galvani wrote: > > This commit adds a driver for the PWM controller found on Rockchip > > RK29, RK30 and RK31 SoCs. > > > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > > --- > > drivers/pwm/Kconfig | 8 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-rockchip.c | 180 ++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 189 insertions(+) > > create mode 100644 drivers/pwm/pwm-rockchip.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 5b34ff2..2e92245 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -197,6 +197,14 @@ config PWM_RENESAS_TPU > > To compile this driver as a module, choose M here: the module > > will be called pwm-renesas-tpu. > > > > +config PWM_ROCKCHIP > > + tristate "Rockchip PWM support" > > + depends on ARCH_ROCKCHIP > > + depends on OF > > It seems like ARCH_ROCKCHIP depends on OF already (via ARCH_MULTI_V7 and > ARCH_MULTIPLATFORM), so having the dependency explicitly here seems > redundant. Right, I will remove the dependency. > > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > [...] > > +#define PRESCALER 2 > > +#define NSECS_PER_SEC 1000000000 > > You should use NSEC_PER_SEC from include/linux/time.h. Ok. > > > +struct rockchip_pwm_chip { > > + struct pwm_chip chip; > > + struct clk *clk; > > + void __iomem *base; > > +}; > > I prefer no artificial padding within structure definitions. > > > +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > > + int duty_ns, int period_ns) > > +{ > > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > > + unsigned long clk_rate, period, duty; > > + u64 div; > > + int ret; > > + > > + clk_rate = clk_get_rate(pc->clk); > > + > > + /* > > + * Since period and duty cycle registers have a width of 32 > > + * bits, every possible input period can be obtained using the > > + * default prescaler value for all practical clock rate values. > > + */ > > + div = clk_rate; > > + div *= period_ns; > > Perhaps shorten this to "div = clk_rate * period_ns;"? I will change this, adding a cast to avoid the truncation of the result to 32 bits: "div = (u64)clk_rate * period_ns;" > > > + do_div(div, PRESCALER * NSECS_PER_SEC); > > + period = div; > > + > > + div = clk_rate; > > + div *= duty_ns; > > And this to "div = clk_rate * duty_ns;"? Ditto. > > > + do_div(div, PRESCALER * NSECS_PER_SEC); > > + duty = div; > > + > > + ret = clk_enable(pc->clk); > > + if (ret) > > + return ret; > > + > > + writel(period, pc->base + PWM_LRC); > > + writel(duty, pc->base + PWM_HRC); > > + writel(0, pc->base + PWM_CNTR); > > + > > + clk_disable(pc->clk); > > + > > + return 0; > > +} > [...] > > +static struct pwm_ops rockchip_pwm_ops = { > > static const please. > > > + .config = rockchip_pwm_config, > > There's a tab between .config and = above. It should be a space. > > > +static const struct of_device_id rockchip_pwm_dt_ids[] = { > > + { .compatible = "rockchip,rk2928-pwm" }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, rockchip_pwm_id_ids); > > The name in the MODULE_DEVICE_TABLE doesn't match the array name above. > Does this even build? Sorry, I didn't test the driver as a module, which indeed doesn't compile. > > > +static struct platform_driver rockchip_pwm_driver = { > > + .driver = { > > + .name = "rockchip-pwm", > > + .owner = THIS_MODULE, > > You no longer need to initialize this explicitly, module_platform_driver > does it for you. > > > + .of_match_table = rockchip_pwm_dt_ids, > > + }, > > + .probe = rockchip_pwm_probe, > > There's another tab instead of space here. > > > + .remove = rockchip_pwm_remove, > > +}; > > +module_platform_driver(rockchip_pwm_driver); > > + > > +MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>"); > > +MODULE_DESCRIPTION("Rockchip PWM driver"); > > Perhaps "Rockchip SoC PWM driver"? Seems reasonable; I will fix this and the other issues in v2, thanks. Beniamino
On Sat, Jun 21, 2014 at 12:00:36AM +0200, Beniamino Galvani wrote: > On Tue, Jun 17, 2014 at 11:42:58PM +0200, Thierry Reding wrote: > > On Thu, May 08, 2014 at 01:08:33AM +0200, Beniamino Galvani wrote: [...] > > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c [...] > > > +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > > > + int duty_ns, int period_ns) > > > +{ > > > + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); > > > + unsigned long clk_rate, period, duty; > > > + u64 div; > > > + int ret; > > > + > > > + clk_rate = clk_get_rate(pc->clk); > > > + > > > + /* > > > + * Since period and duty cycle registers have a width of 32 > > > + * bits, every possible input period can be obtained using the > > > + * default prescaler value for all practical clock rate values. > > > + */ > > > + div = clk_rate; > > > + div *= period_ns; > > > > Perhaps shorten this to "div = clk_rate * period_ns;"? > > I will change this, adding a cast to avoid the truncation of the > result to 32 bits: "div = (u64)clk_rate * period_ns;" Alternatively you could simply make clk_rate a u64 since it's only used in this context anyway. Thierry
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 5b34ff2..2e92245 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -197,6 +197,14 @@ config PWM_RENESAS_TPU To compile this driver as a module, choose M here: the module will be called pwm-renesas-tpu. +config PWM_ROCKCHIP + tristate "Rockchip PWM support" + depends on ARCH_ROCKCHIP + depends on OF + help + Generic PWM framework driver for the PWM controller found on + Rockchip SoCs. + config PWM_SAMSUNG tristate "Samsung PWM support" depends on PLAT_SAMSUNG diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index e57d2c3..2e347f7 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -17,6 +17,7 @@ obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o obj-$(CONFIG_PWM_PUV3) += pwm-puv3.o obj-$(CONFIG_PWM_PXA) += pwm-pxa.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o +obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c new file mode 100644 index 0000000..9b51828 --- /dev/null +++ b/drivers/pwm/pwm-rockchip.c @@ -0,0 +1,180 @@ +/* + * PWM driver for Rockchip SoCs + * + * Copyright (C) 2014 Beniamino Galvani <b.galvani@gmail.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + */ + +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#define PWM_CNTR 0x00 /* Counter register */ +#define PWM_HRC 0x04 /* High reference register */ +#define PWM_LRC 0x08 /* Low reference register */ +#define PWM_CTRL 0x0c /* Control register */ +#define PWM_CTRL_TIMER_EN (1 << 0) +#define PWM_CTRL_OUTPUT_EN (1 << 3) + +#define PRESCALER 2 +#define NSECS_PER_SEC 1000000000 + +struct rockchip_pwm_chip { + struct pwm_chip chip; + struct clk *clk; + void __iomem *base; +}; + +static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c) +{ + return container_of(c, struct rockchip_pwm_chip, chip); +} + +static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, + int duty_ns, int period_ns) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + unsigned long clk_rate, period, duty; + u64 div; + int ret; + + clk_rate = clk_get_rate(pc->clk); + + /* + * Since period and duty cycle registers have a width of 32 + * bits, every possible input period can be obtained using the + * default prescaler value for all practical clock rate values. + */ + div = clk_rate; + div *= period_ns; + do_div(div, PRESCALER * NSECS_PER_SEC); + period = div; + + div = clk_rate; + div *= duty_ns; + do_div(div, PRESCALER * NSECS_PER_SEC); + duty = div; + + ret = clk_enable(pc->clk); + if (ret) + return ret; + + writel(period, pc->base + PWM_LRC); + writel(duty, pc->base + PWM_HRC); + writel(0, pc->base + PWM_CNTR); + + clk_disable(pc->clk); + + return 0; +} + +static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + int ret; + u32 val; + + ret = clk_enable(pc->clk); + if (ret) + return ret; + + val = readl_relaxed(pc->base + PWM_CTRL); + val |= PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; + writel_relaxed(val, pc->base + PWM_CTRL); + + return 0; +} + +static void rockchip_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + u32 val; + + val = readl_relaxed(pc->base + PWM_CTRL); + val &= ~(PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN); + writel_relaxed(val, pc->base + PWM_CTRL); + + clk_disable(pc->clk); +} + +static struct pwm_ops rockchip_pwm_ops = { + .config = rockchip_pwm_config, + .enable = rockchip_pwm_enable, + .disable = rockchip_pwm_disable, + .owner = THIS_MODULE, +}; + +static int rockchip_pwm_probe(struct platform_device *pdev) +{ + struct rockchip_pwm_chip *pc; + struct resource *r; + int ret; + + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); + if (!pc) + return -ENOMEM; + + r = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pc->base = devm_ioremap_resource(&pdev->dev, r); + if (IS_ERR(pc->base)) + return PTR_ERR(pc->base); + + pc->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(pc->clk)) + return PTR_ERR(pc->clk); + + ret = clk_prepare(pc->clk); + if (ret) + return ret; + + platform_set_drvdata(pdev, pc); + + pc->chip.dev = &pdev->dev; + pc->chip.ops = &rockchip_pwm_ops; + pc->chip.base = -1; + pc->chip.npwm = 1; + + ret = pwmchip_add(&pc->chip); + if (ret < 0) { + clk_unprepare(pc->clk); + dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); + } + + return ret; +} + +static int rockchip_pwm_remove(struct platform_device *pdev) +{ + struct rockchip_pwm_chip *pc = platform_get_drvdata(pdev); + + clk_unprepare(pc->clk); + + return pwmchip_remove(&pc->chip); +} + +static const struct of_device_id rockchip_pwm_dt_ids[] = { + { .compatible = "rockchip,rk2928-pwm" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rockchip_pwm_id_ids); + +static struct platform_driver rockchip_pwm_driver = { + .driver = { + .name = "rockchip-pwm", + .owner = THIS_MODULE, + .of_match_table = rockchip_pwm_dt_ids, + }, + .probe = rockchip_pwm_probe, + .remove = rockchip_pwm_remove, +}; +module_platform_driver(rockchip_pwm_driver); + +MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>"); +MODULE_DESCRIPTION("Rockchip PWM driver"); +MODULE_LICENSE("GPL v2");
This commit adds a driver for the PWM controller found on Rockchip RK29, RK30 and RK31 SoCs. Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> --- drivers/pwm/Kconfig | 8 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-rockchip.c | 180 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 drivers/pwm/pwm-rockchip.c