Hi Heiko & thierry, Thank you for your suggestion.
于 2014年07月24日 00:01, Heiko Stübner 写道:
Hi Caesar. Am Mittwoch, 23. Juli 2014, 14:38:41 schrieb Caesar Wang:This patch added to support the PWM controller found on RK3288 SoC. Signed-off-by: Caesar Wang <caesar.w...@rock-chips.com> --- drivers/pwm/pwm-rockchip.c | 141 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 122 insertions(+), 19 deletions(-) diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index eec2145..8d72a98 100644 --- a/drivers/pwm/pwm-rockchip.c +++ b/drivers/pwm/pwm-rockchip.c @@ -2,6 +2,7 @@ * PWM driver for Rockchip SoCs * * Copyright (C) 2014 Beniamino Galvani <b.galv...@gmail.com> + * Copyright (C) 2014 Caesar Wang <caesar.w...@rock-chips.com>you might want to check who actually holds the copyright for your contributions, I guess a "Copyright (C) 2014 Rockchip"-something would be more appropriate?
Yes,you are right.
* * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -12,6 +13,7 @@ #include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pwm.h> #include <linux/time.h> @@ -25,17 +27,89 @@ #define PRESCALER 2 +#define PWM_ENABLE (1 << 0) +#define PWM_CONTINUOUS (1 << 1) +#define PWM_DUTY_POSITIVE (1 << 3) +#define PWM_INACTIVE_NEGATIVE (0 << 4) +#define PWM_OUTPUT_LEFT (0 << 5) +#define PWM_LP_DISABLE (0 << 8) + struct rockchip_pwm_chip { struct pwm_chip chip; struct clk *clk; + const struct rockchip_pwm_data *data; void __iomem *base; }; +struct rockchip_pwm_regs { + unsigned long duty; + unsigned long period; + unsigned long cntr; + unsigned long ctrl; +}; + +struct rockchip_pwm_data { + struct rockchip_pwm_regs regs; + unsigned int prescaler; + + void (*set_enable)(struct pwm_chip *chip, bool enable); +}; + static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c) { return container_of(c, struct rockchip_pwm_chip, chip); } +static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip, bool enable) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + u32 val = 0; + u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN; + + val = readl_relaxed(pc->base + pc->data->regs.ctrl); + + if (enable) + val |= enable_conf; + else + val &= ~enable_conf; + + writel_relaxed(val, pc->base + pc->data->regs.ctrl); +} + +static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip, bool enable) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + u32 val = 0; + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE | + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE; + + val = readl_relaxed(pc->base + pc->data->regs.ctrl); + + if (enable) + val |= enable_conf; + else + val &= ~enable_conf; + + writel_relaxed(val, pc->base + pc->data->regs.ctrl); +} + +static void rockchip_pwm_set_enable_vop(struct pwm_chip *chip, bool enable) +{ + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip); + u32 val = 0; + u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE | + PWM_CONTINUOUS | PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE; + + val = readl_relaxed(pc->base + pc->data->regs.ctrl); + + if (enable) + val |= enable_conf; + else + val &= ~enable_conf; + + writel_relaxed(val, pc->base + pc->data->regs.ctrl); +}not sure if I'm just blind ... do rockchip_pwm_set_enable_v2 and rockchip_pwm_set_enable_vop differ at all? If they don't differ, I guess pwm_data_vop should just use rockchip_pwm_set_enable_v2 instead of duplicating it. Heiko
Yes, the rockchip_pwm_set_enable_v1 & v2 & vop is similar.
So my v2 patch use "u32 enable_conf" instead of it .
+struct rockchip_pwm_data {
> + .........
> + u32 enable_conf;
> +};
The thierry has suggested it [1] in my v2 patch:
For this I think it would be more readable to provide function pointers
rather than a variable. That is:
struct rockchip_pwm_data {
...
int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
int (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
};
Then you can implement these for each variant of the chip and call them
from the common rockchip_pwm_enable(), somewhat like this.
Perhaps,thierry's suggestion I got it wrong.
Hi thierry& Heiko :-)
Maybe,could you suggest solve it reasonable? thanks.
[1]: https://lkml.org/lkml/2014/7/21/113
+ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { @@ -52,20 +126,20 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, * default prescaler value for all practical clock rate values. */ div = clk_rate * period_ns; - do_div(div, PRESCALER * NSEC_PER_SEC); + do_div(div, pc->data->prescaler * NSEC_PER_SEC); period = div; div = clk_rate * duty_ns; - do_div(div, PRESCALER * NSEC_PER_SEC); + do_div(div, pc->data->prescaler * NSEC_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); + writel(period, pc->base + pc->data->regs.period); + writel(duty, pc->base + pc->data->regs.duty); + writel(0, pc->base + pc->data->regs.cntr); clk_disable(pc->clk); @@ -76,15 +150,12 @@ 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); + pc->data->set_enable(chip, true); return 0; } @@ -92,11 +163,8 @@ static int rockchip_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) 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); + pc->data->set_enable(chip, false); clk_disable(pc->clk); } @@ -108,12 +176,52 @@ static const struct pwm_ops rockchip_pwm_ops = { .owner = THIS_MODULE, }; +static const struct rockchip_pwm_data pwm_data_v1 = { + .regs.duty = PWM_HRC, + .regs.period = PWM_LRC, + .regs.cntr = PWM_CNTR, + .regs.ctrl = PWM_CTRL, + .prescaler = PRESCALER, + .set_enable = rockchip_pwm_set_enable_v1, +}; + +static const struct rockchip_pwm_data pwm_data_v2 = { + .regs.duty = PWM_LRC, + .regs.period = PWM_HRC, + .regs.cntr = PWM_CNTR, + .regs.ctrl = PWM_CTRL, + .prescaler = PRESCALER-1, + .set_enable = rockchip_pwm_set_enable_v2, +}; + +static const struct rockchip_pwm_data pwm_data_vop = { + .regs.duty = PWM_LRC, + .regs.period = PWM_HRC, + .regs.cntr = PWM_CTRL, + .regs.ctrl = PWM_CNTR, + .prescaler = PRESCALER-1, + .set_enable = rockchip_pwm_set_enable_vop, +}; + +static const struct of_device_id rockchip_pwm_dt_ids[] = { + { .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1}, + { .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2}, + { .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop}, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids); + static int rockchip_pwm_probe(struct platform_device *pdev) { + const struct of_device_id *id; struct rockchip_pwm_chip *pc; struct resource *r; int ret; + id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev); + if (!id) + return -EINVAL; + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); if (!pc) return -ENOMEM; @@ -133,6 +241,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev) platform_set_drvdata(pdev, pc); + pc->data = id->data; pc->chip.dev = &pdev->dev; pc->chip.ops = &rockchip_pwm_ops; pc->chip.base = -1; @@ -156,12 +265,6 @@ static int rockchip_pwm_remove(struct platform_device *pdev) 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_dt_ids); - static struct platform_driver rockchip_pwm_driver = { .driver = { .name = "rockchip-pwm",
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
