* [PATCH v4 0/2] This series adds support for RK3288 SoC integrated PWM @ 2014-07-24 10:21 Caesar Wang 2014-07-24 10:21 ` [PATCH v4 1/2] pwm: rockchip: document RK3288 SoC compatible Caesar Wang 2014-07-24 10:21 ` [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC Caesar Wang 0 siblings, 2 replies; 16+ messages in thread From: Caesar Wang @ 2014-07-24 10:21 UTC (permalink / raw) To: linux-arm-kernel This patch will make applying on the top of Beniamino's submission, the Beniamino's submission come from [1]. [1]: https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git/log/?h=for-next Beniamino's submission won't be used from genenation Rockchip SoCs. So I have to add the new pwm for next genenation Rockchip SoCs. Tested on RK3288 SDK board. Changes in v4: * address comments from Heiko St?bner: - fix the copyright for ROCKCHIP, Inc. - remove rockchip_pwm_set_enable_vop,then it instead of rockchip_pwm_set_enable_v2. Changes in v3: * address comments from Thierry Reding: - fix PWM document deccribes. - add a description for [PATCH v3 2/2]. - renamed the PWM registers - Changed in rockchip_pwm_data struct - remove the devm_ioremap(),fixed in lcdc driver. Changes in v2: * address comments from Beniamino Galvani: - remove #include <linux/of_address.h>. - of_iomap be removed,and devm_ioremap replace it. - remove a line no be used. Caesar Wang (2): pwm: rockchip: document RK3288 SoC compatible pwm: rockchip: Added to support for RK3288 SoC .../devicetree/bindings/pwm/pwm-rockchip.txt | 5 +- drivers/pwm/pwm-rockchip.c | 124 +++++++++++++++++---- 2 files changed, 109 insertions(+), 20 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/2] pwm: rockchip: document RK3288 SoC compatible 2014-07-24 10:21 [PATCH v4 0/2] This series adds support for RK3288 SoC integrated PWM Caesar Wang @ 2014-07-24 10:21 ` Caesar Wang 2014-07-24 10:21 ` [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC Caesar Wang 1 sibling, 0 replies; 16+ messages in thread From: Caesar Wang @ 2014-07-24 10:21 UTC (permalink / raw) To: linux-arm-kernel Document new compatible for PWM founding on RK3288 SoC Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> --- Documentation/devicetree/bindings/pwm/pwm-rockchip.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt index 3182126..d47d15a 100644 --- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt +++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt @@ -1,7 +1,10 @@ Rockchip PWM controller Required properties: - - compatible: should be "rockchip,rk2928-pwm" + - compatible: should be "rockchip,<name>-pwm" + "rockchip,rk2928-pwm": found on RK29XX,RK3066 and RK3188 SoCs + "rockchip,rk3288-pwm": found on RK3288 SoC + "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC - reg: physical base address and length of the controller's registers - clocks: phandle and clock specifier of the PWM reference clock - #pwm-cells: should be 2. See pwm.txt in this directory for a -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-07-24 10:21 [PATCH v4 0/2] This series adds support for RK3288 SoC integrated PWM Caesar Wang 2014-07-24 10:21 ` [PATCH v4 1/2] pwm: rockchip: document RK3288 SoC compatible Caesar Wang @ 2014-07-24 10:21 ` Caesar Wang 2014-08-06 22:46 ` Doug Anderson 2014-08-07 6:18 ` Thierry Reding 1 sibling, 2 replies; 16+ messages in thread From: Caesar Wang @ 2014-07-24 10:21 UTC (permalink / raw) To: linux-arm-kernel This patch added to support the PWM controller found on RK3288 SoC. Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> --- drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 105 insertions(+), 19 deletions(-) diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c index eec2145..59c2513 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.galvani@gmail.com> + * Copyright (C) 2014 ROCKCHIP, Inc. * * 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,72 @@ #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 int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns) { @@ -52,20 +109,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 +133,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 +146,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 +159,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_v2, +}; + +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 +224,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 +248,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", -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-07-24 10:21 ` [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC Caesar Wang @ 2014-08-06 22:46 ` Doug Anderson 2014-08-07 1:27 ` caesar 2014-08-07 6:18 ` Thierry Reding 1 sibling, 1 reply; 16+ messages in thread From: Doug Anderson @ 2014-08-06 22:46 UTC (permalink / raw) To: linux-arm-kernel Caesar, On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang <caesar.wang@rock-chips.com> wrote: > +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, Did you really mean to flip CTRL and CNTR here? If so, that's super confusing and deserves a comment. AKA, I think the above should not be: + .regs.cntr = PWM_CTRL, + .regs.ctrl = PWM_CNTR, ...but should be + .regs.cntr = PWM_CNTR, + .regs.ctrl = PWM_CTRL, If you didn't mean to flip CTRL and CNTR here, then just get rid of pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you totally remove the "rockchip,vop-pwm" since there's nothing different between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". Have you validated Thierry's suggestion to allow you to access your memory range? -Doug ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-06 22:46 ` Doug Anderson @ 2014-08-07 1:27 ` caesar 2014-08-07 2:16 ` Doug Anderson 0 siblings, 1 reply; 16+ messages in thread From: caesar @ 2014-08-07 1:27 UTC (permalink / raw) To: linux-arm-kernel Doug, ? 2014?08?07? 06:46, Doug Anderson ??: > Caesar, > > On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang <caesar.wang@rock-chips.com> wrote: >> +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, > Did you really mean to flip CTRL and CNTR here? If so, that's super > confusing and deserves a comment. AKA, I think the above should not > be: > > + .regs.cntr = PWM_CTRL, > + .regs.ctrl = PWM_CNTR, > > ...but should be > > + .regs.cntr = PWM_CNTR, > + .regs.ctrl = PWM_CTRL, > > If you didn't mean to flip CTRL and CNTR here, then just get rid of > pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you > totally remove the "rockchip,vop-pwm" since there's nothing different > between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and "rockchip,vop-pwm" are seperate PWM controllers. They are just different registers address between CNTR and CTRL . > > Have you validated Thierry's suggestion to allow you to access your > memory range? Yes,we have solve it in lcdc driver. The Mark Yao have the submission in [0]. [0]: https://lkml.org/lkml/2014/8/4/20 > > -Doug > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 1:27 ` caesar @ 2014-08-07 2:16 ` Doug Anderson 2014-08-07 3:23 ` caesar 0 siblings, 1 reply; 16+ messages in thread From: Doug Anderson @ 2014-08-07 2:16 UTC (permalink / raw) To: linux-arm-kernel Caesar, On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> wrote: > Doug, > > ? 2014?08?07? 06:46, Doug Anderson ??: > >> Caesar, >> >> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang <caesar.wang@rock-chips.com> >> wrote: >>> >>> +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, >> >> Did you really mean to flip CTRL and CNTR here? If so, that's super >> confusing and deserves a comment. AKA, I think the above should not >> be: >> >> + .regs.cntr = PWM_CTRL, >> + .regs.ctrl = PWM_CNTR, >> >> ...but should be >> >> + .regs.cntr = PWM_CNTR, >> + .regs.ctrl = PWM_CTRL, >> >> If you didn't mean to flip CTRL and CNTR here, then just get rid of >> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you >> totally remove the "rockchip,vop-pwm" since there's nothing different >> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". > > > Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and > "rockchip,vop-pwm" are seperate PWM controllers. > They are just different registers address between CNTR and CTRL . OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on the vop. So I think that the only change you need is to add: #define PWM_VOP_CTRL 0x00 #define PWM_VOP_CNTR 0x0c ...then use these new #defines for the vop structure. As you have the code written right now it's very confusing. The new #defines will fix this. >> Have you validated Thierry's suggestion to allow you to access your >> memory range? > > Yes,we have solve it in lcdc driver. > The Mark Yao have the submission in [0]. > > [0]: https://lkml.org/lkml/2014/8/4/20 Excellent! Then we should be able to land after you fix the above. -Doug ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 2:16 ` Doug Anderson @ 2014-08-07 3:23 ` caesar 2014-08-07 3:26 ` Doug Anderson 0 siblings, 1 reply; 16+ messages in thread From: caesar @ 2014-08-07 3:23 UTC (permalink / raw) To: linux-arm-kernel ? 2014?08?07? 10:16, Doug Anderson ??: > Caesar, > > On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> wrote: >> Doug, >> >> ? 2014?08?07? 06:46, Doug Anderson ??: >> >>> Caesar, >>> >>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang <caesar.wang@rock-chips.com> >>> wrote: >>>> +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, >>> Did you really mean to flip CTRL and CNTR here? If so, that's super >>> confusing and deserves a comment. AKA, I think the above should not >>> be: >>> >>> + .regs.cntr = PWM_CTRL, >>> + .regs.ctrl = PWM_CNTR, >>> >>> ...but should be >>> >>> + .regs.cntr = PWM_CNTR, >>> + .regs.ctrl = PWM_CTRL, >>> >>> If you didn't mean to flip CTRL and CNTR here, then just get rid of >>> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you >>> totally remove the "rockchip,vop-pwm" since there's nothing different >>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". >> >> Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and >> "rockchip,vop-pwm" are seperate PWM controllers. >> They are just different registers address between CNTR and CTRL . > OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on > the vop. So I think that the only change you need is to add: > > #define PWM_VOP_CTRL 0x00 > #define PWM_VOP_CNTR 0x0c > > ...then use these new #defines for the vop structure. > > > As you have the code written right now it's very confusing. The new > #defines will fix this. > yeah, I think they can be used in the same context. I will fix it in patch v5 if it is really need. > >>> Have you validated Thierry's suggestion to allow you to access your >>> memory range? >> Yes,we have solve it in lcdc driver. >> The Mark Yao have the submission in [0]. >> >> [0]: https://lkml.org/lkml/2014/8/4/20 > Excellent! Then we should be able to land after you fix the above. > > -Doug > > > OK,Thanks! ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 3:23 ` caesar @ 2014-08-07 3:26 ` Doug Anderson 2014-08-07 3:37 ` caesar 2014-08-07 6:12 ` Thierry Reding 0 siblings, 2 replies; 16+ messages in thread From: Doug Anderson @ 2014-08-07 3:26 UTC (permalink / raw) To: linux-arm-kernel caesar, On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@rock-chips.com> wrote: > > ? 2014?08?07? 10:16, Doug Anderson ??: > >> Caesar, >> >> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> wrote: >>> >>> Doug, >>> >>> ? 2014?08?07? 06:46, Doug Anderson ??: >>> >>>> Caesar, >>>> >>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang >>>> <caesar.wang@rock-chips.com> >>>> wrote: >>>>> >>>>> +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, >>>> >>>> Did you really mean to flip CTRL and CNTR here? If so, that's super >>>> confusing and deserves a comment. AKA, I think the above should not >>>> be: >>>> >>>> + .regs.cntr = PWM_CTRL, >>>> + .regs.ctrl = PWM_CNTR, >>>> >>>> ...but should be >>>> >>>> + .regs.cntr = PWM_CNTR, >>>> + .regs.ctrl = PWM_CTRL, >>>> >>>> If you didn't mean to flip CTRL and CNTR here, then just get rid of >>>> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you >>>> totally remove the "rockchip,vop-pwm" since there's nothing different >>>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". >>> >>> >>> Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and >>> "rockchip,vop-pwm" are seperate PWM controllers. >>> They are just different registers address between CNTR and CTRL . >> >> OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on >> the vop. So I think that the only change you need is to add: >> >> #define PWM_VOP_CTRL 0x00 >> #define PWM_VOP_CNTR 0x0c >> >> ...then use these new #defines for the vop structure. >> >> >> As you have the code written right now it's very confusing. The new >> #defines will fix this. >> > yeah, I think they can be used in the same context. > > I will fix it in patch v5 if it is really need. I think you should fix this, but if Thierry doesn't think so then it's really his decision. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 3:26 ` Doug Anderson @ 2014-08-07 3:37 ` caesar 2014-08-07 3:46 ` Doug Anderson 2014-08-07 6:12 ` Thierry Reding 1 sibling, 1 reply; 16+ messages in thread From: caesar @ 2014-08-07 3:37 UTC (permalink / raw) To: linux-arm-kernel Doug, ? 2014?08?07? 11:26, Doug Anderson ??: > caesar, > > On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@rock-chips.com> wrote: >> ? 2014?08?07? 10:16, Doug Anderson ??: >> >>> Caesar, >>> >>> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> wrote: >>>> Doug, >>>> >>>> ? 2014?08?07? 06:46, Doug Anderson ??: >>>> >>>>> Caesar, >>>>> >>>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang >>>>> <caesar.wang@rock-chips.com> >>>>> wrote: >>>>>> +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, >>>>> Did you really mean to flip CTRL and CNTR here? If so, that's super >>>>> confusing and deserves a comment. AKA, I think the above should not >>>>> be: >>>>> >>>>> + .regs.cntr = PWM_CTRL, >>>>> + .regs.ctrl = PWM_CNTR, >>>>> >>>>> ...but should be >>>>> >>>>> + .regs.cntr = PWM_CNTR, >>>>> + .regs.ctrl = PWM_CTRL, >>>>> >>>>> If you didn't mean to flip CTRL and CNTR here, then just get rid of >>>>> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you >>>>> totally remove the "rockchip,vop-pwm" since there's nothing different >>>>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". >>>> >>>> Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and >>>> "rockchip,vop-pwm" are seperate PWM controllers. >>>> They are just different registers address between CNTR and CTRL . >>> OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on >>> the vop. So I think that the only change you need is to add: >>> >>> #define PWM_VOP_CTRL 0x00 >>> #define PWM_VOP_CNTR 0x0c >>> >>> ...then use these new #defines for the vop structure. >>> >>> >>> As you have the code written right now it's very confusing. The new >>> #defines will fix this. >>> >> yeah, I think they can be used in the same context. >> >> I will fix it in patch v5 if it is really need. > I think you should fix this, but if Thierry doesn't think so then it's > really his decision. I remember In patch v2 [1],Thierry suggests me to fix it so if I have no to get wrong. [1]: https://lkml.org/lkml/2014/7/21/113 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 3:37 ` caesar @ 2014-08-07 3:46 ` Doug Anderson 2014-08-07 4:05 ` caesar 0 siblings, 1 reply; 16+ messages in thread From: Doug Anderson @ 2014-08-07 3:46 UTC (permalink / raw) To: linux-arm-kernel Caesar, On Wed, Aug 6, 2014 at 8:37 PM, caesar <caesar.wang@rock-chips.com> wrote: > Doug, > > ? 2014?08?07? 11:26, Doug Anderson ??: > >> caesar, >> >> On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@rock-chips.com> wrote: >>> >>> ? 2014?08?07? 10:16, Doug Anderson ??: >>> >>>> Caesar, >>>> >>>> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> >>>> wrote: >>>>> >>>>> Doug, >>>>> >>>>> ? 2014?08?07? 06:46, Doug Anderson ??: >>>>> >>>>>> Caesar, >>>>>> >>>>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang >>>>>> <caesar.wang@rock-chips.com> >>>>>> wrote: >>>>>>> >>>>>>> +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, >>>>>> >>>>>> Did you really mean to flip CTRL and CNTR here? If so, that's super >>>>>> confusing and deserves a comment. AKA, I think the above should not >>>>>> be: >>>>>> >>>>>> + .regs.cntr = PWM_CTRL, >>>>>> + .regs.ctrl = PWM_CNTR, >>>>>> >>>>>> ...but should be >>>>>> >>>>>> + .regs.cntr = PWM_CNTR, >>>>>> + .regs.ctrl = PWM_CTRL, >>>>>> >>>>>> If you didn't mean to flip CTRL and CNTR here, then just get rid of >>>>>> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you >>>>>> totally remove the "rockchip,vop-pwm" since there's nothing different >>>>>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". >>>>> >>>>> >>>>> Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and >>>>> "rockchip,vop-pwm" are seperate PWM controllers. >>>>> They are just different registers address between CNTR and CTRL . >>>> >>>> OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on >>>> the vop. So I think that the only change you need is to add: >>>> >>>> #define PWM_VOP_CTRL 0x00 >>>> #define PWM_VOP_CNTR 0x0c >>>> >>>> ...then use these new #defines for the vop structure. >>>> >>>> >>>> As you have the code written right now it's very confusing. The new >>>> #defines will fix this. >>>> >>> yeah, I think they can be used in the same context. >>> >>> I will fix it in patch v5 if it is really need. >> >> I think you should fix this, but if Thierry doesn't think so then it's >> really his decision. > > I remember In patch v2 [1],Thierry suggests me to fix it so if I have no to > get wrong. > > [1]: https://lkml.org/lkml/2014/7/21/113 I think Thierry might not have realized that they were flipped... -Doug ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 3:46 ` Doug Anderson @ 2014-08-07 4:05 ` caesar 0 siblings, 0 replies; 16+ messages in thread From: caesar @ 2014-08-07 4:05 UTC (permalink / raw) To: linux-arm-kernel Doug, ? 2014?08?07? 11:46, Doug Anderson ??: > Caesar, > > On Wed, Aug 6, 2014 at 8:37 PM, caesar <caesar.wang@rock-chips.com> wrote: >> Doug, >> >> ? 2014?08?07? 11:26, Doug Anderson ??: >> >>> caesar, >>> >>> On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@rock-chips.com> wrote: >>>> ? 2014?08?07? 10:16, Doug Anderson ??: >>>> >>>>> Caesar, >>>>> >>>>> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> >>>>> wrote: >>>>>> Doug, >>>>>> >>>>>> ? 2014?08?07? 06:46, Doug Anderson ??: >>>>>> >>>>>>> Caesar, >>>>>>> >>>>>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang >>>>>>> <caesar.wang@rock-chips.com> >>>>>>> wrote: >>>>>>>> +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, >>>>>>> Did you really mean to flip CTRL and CNTR here? If so, that's super >>>>>>> confusing and deserves a comment. AKA, I think the above should not >>>>>>> be: >>>>>>> >>>>>>> + .regs.cntr = PWM_CTRL, >>>>>>> + .regs.ctrl = PWM_CNTR, >>>>>>> >>>>>>> ...but should be >>>>>>> >>>>>>> + .regs.cntr = PWM_CNTR, >>>>>>> + .regs.ctrl = PWM_CTRL, >>>>>>> >>>>>>> If you didn't mean to flip CTRL and CNTR here, then just get rid of >>>>>>> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you >>>>>>> totally remove the "rockchip,vop-pwm" since there's nothing different >>>>>>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". >>>>>> >>>>>> Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and >>>>>> "rockchip,vop-pwm" are seperate PWM controllers. >>>>>> They are just different registers address between CNTR and CTRL . >>>>> OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on >>>>> the vop. So I think that the only change you need is to add: >>>>> >>>>> #define PWM_VOP_CTRL 0x00 >>>>> #define PWM_VOP_CNTR 0x0c >>>>> >>>>> ...then use these new #defines for the vop structure. >>>>> >>>>> >>>>> As you have the code written right now it's very confusing. The new >>>>> #defines will fix this. >>>>> >>>> yeah, I think they can be used in the same context. >>>> >>>> I will fix it in patch v5 if it is really need. >>> I think you should fix this, but if Thierry doesn't think so then it's >>> really his decision. >> I remember In patch v2 [1],Thierry suggests me to fix it so if I have no to >> get wrong. >> >> [1]: https://lkml.org/lkml/2014/7/21/113 > I think Thierry might not have realized that they were flipped... > > -Doug > > > Maybe you are right. I will sent patch v5 fix the about tomorrow if it has no other problems. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 3:26 ` Doug Anderson 2014-08-07 3:37 ` caesar @ 2014-08-07 6:12 ` Thierry Reding 1 sibling, 0 replies; 16+ messages in thread From: Thierry Reding @ 2014-08-07 6:12 UTC (permalink / raw) To: linux-arm-kernel On Wed, Aug 06, 2014 at 08:26:51PM -0700, Doug Anderson wrote: > caesar, > > On Wed, Aug 6, 2014 at 8:23 PM, caesar <caesar.wang@rock-chips.com> wrote: > > > > ? 2014?08?07? 10:16, Doug Anderson ??: > > > >> Caesar, > >> > >> On Wed, Aug 6, 2014 at 6:27 PM, caesar <caesar.wang@rock-chips.com> wrote: > >>> > >>> Doug, > >>> > >>> ? 2014?08?07? 06:46, Doug Anderson ??: > >>> > >>>> Caesar, > >>>> > >>>> On Thu, Jul 24, 2014 at 3:21 AM, Caesar Wang > >>>> <caesar.wang@rock-chips.com> > >>>> wrote: > >>>>> > >>>>> +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, > >>>> > >>>> Did you really mean to flip CTRL and CNTR here? If so, that's super > >>>> confusing and deserves a comment. AKA, I think the above should not > >>>> be: > >>>> > >>>> + .regs.cntr = PWM_CTRL, > >>>> + .regs.ctrl = PWM_CNTR, > >>>> > >>>> ...but should be > >>>> > >>>> + .regs.cntr = PWM_CNTR, > >>>> + .regs.ctrl = PWM_CTRL, > >>>> > >>>> If you didn't mean to flip CTRL and CNTR here, then just get rid of > >>>> pwm_data_vop and refer to pwm_data_v2. In fact, I'd suggest that you > >>>> totally remove the "rockchip,vop-pwm" since there's nothing different > >>>> between "rockchip,rk3288-pwm" and "rockchip,vop-pwm". > >>> > >>> > >>> Sorry,I think it's no problem. the "rockchip,rk3288-pwm" and > >>> "rockchip,vop-pwm" are seperate PWM controllers. > >>> They are just different registers address between CNTR and CTRL . > >> > >> OK, I looked up in the TRM. Right, the CNTR and CTRL are flipped on > >> the vop. So I think that the only change you need is to add: > >> > >> #define PWM_VOP_CTRL 0x00 > >> #define PWM_VOP_CNTR 0x0c > >> > >> ...then use these new #defines for the vop structure. > >> > >> > >> As you have the code written right now it's very confusing. The new > >> #defines will fix this. > >> > > yeah, I think they can be used in the same context. > > > > I will fix it in patch v5 if it is really need. > > I think you should fix this, but if Thierry doesn't think so then it's > really his decision. Frankly, I'm fine if these don't use symbolic names at all since only the structure fields are used to refer to them now. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140807/92aefac6/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-07-24 10:21 ` [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC Caesar Wang 2014-08-06 22:46 ` Doug Anderson @ 2014-08-07 6:18 ` Thierry Reding 2014-08-07 13:04 ` caesar 1 sibling, 1 reply; 16+ messages in thread From: Thierry Reding @ 2014-08-07 6:18 UTC (permalink / raw) To: linux-arm-kernel On Thu, Jul 24, 2014 at 06:21:35PM +0800, Caesar Wang wrote: > This patch added to support the PWM controller found on > RK3288 SoC. > > Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> > --- > drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 105 insertions(+), 19 deletions(-) > > diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c > index eec2145..59c2513 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.galvani@gmail.com> > + * Copyright (C) 2014 ROCKCHIP, Inc. > * > * 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,72 @@ > > #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 int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > int duty_ns, int period_ns) > { > @@ -52,20 +109,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 +133,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 +146,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 +159,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, Perhaps a slightly more idiomatic way to write this would be: static const struct rockchip_pwm_data pwm_data_v1 = { .regs = { .duty = PWM_HRC, .period = PWM_LRC, .cntr = PWM_CNTR, .ctrl = PWM_CTRL, }, ... }; And similar for the v2 and vop structures. And like I said in another reply, since the defines are now only used in this structure it's a little redundant to give them symbolic names, so the above could equally well be: static const struct rockchip_pwm_data pwm_data_v1 = { .regs = { .duty = 0x04, .period = 0x08, .cntr = 0x00, .ctrl = 0x0c, }, ... }; > + .prescaler = PRESCALER, Similarly for the prescaler value, it can now simply be 2 here. > + .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, And 1 here. > + .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, And 1 here. > + .set_enable = rockchip_pwm_set_enable_v2, > +}; No need for the double indirection. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140807/d0da6e17/attachment-0001.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 6:18 ` Thierry Reding @ 2014-08-07 13:04 ` caesar 2014-08-07 13:14 ` Thierry Reding 0 siblings, 1 reply; 16+ messages in thread From: caesar @ 2014-08-07 13:04 UTC (permalink / raw) To: linux-arm-kernel Thierry, ? 2014?08?07? 14:18, Thierry Reding ??: > On Thu, Jul 24, 2014 at 06:21:35PM +0800, Caesar Wang wrote: >> This patch added to support the PWM controller found on >> RK3288 SoC. >> >> Signed-off-by: Caesar Wang <caesar.wang@rock-chips.com> >> --- >> drivers/pwm/pwm-rockchip.c | 124 ++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 105 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c >> index eec2145..59c2513 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.galvani@gmail.com> >> + * Copyright (C) 2014 ROCKCHIP, Inc. >> * >> * 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,72 @@ >> >> #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 int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, >> int duty_ns, int period_ns) >> { >> @@ -52,20 +109,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 +133,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 +146,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 +159,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, > Perhaps a slightly more idiomatic way to write this would be: > > static const struct rockchip_pwm_data pwm_data_v1 = { > .regs = { > .duty = PWM_HRC, > .period = PWM_LRC, > .cntr = PWM_CNTR, > .ctrl = PWM_CTRL, > }, > ... > }; > > And similar for the v2 and vop structures. And like I said in another > reply, since the defines are now only used in this structure it's a > little redundant to give them symbolic names, so the above could equally > well be: > > static const struct rockchip_pwm_data pwm_data_v1 = { > .regs = { > .duty = 0x04, > .period = 0x08, > .cntr = 0x00, > .ctrl = 0x0c, > }, > ... > }; > >> + .prescaler = PRESCALER, > Similarly for the prescaler value, it can now simply be 2 here. > >> + .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, > And 1 here. > >> + .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, > And 1 here. As you say, I will rewrite the about if it's really need do so it. For example: static const struct rockchip_pwm_data pwm_data_v1 = { .regs = { .duty = 0x04, .period = 0x08, .cntr = 0x00, .ctrl = 0x0c, }, .prescaler = 2, .set_enable = rockchip_pwm_set_enable_v1, }; static const struct rockchip_pwm_data pwm_data_v2 = { .regs = { .duty = 0x08, .period = 0x04, .cntr = 0x00, .ctrl = 0x0c, }, .prescaler = 1, .set_enable = rockchip_pwm_set_enable_v2, }; static const struct rockchip_pwm_data pwm_data_vop = { .regs = { .duty = 0x08, .period = 0x04, .cntr = 0x0c, .ctrl = 0x00, }, .prescaler = 1, .set_enable = rockchip_pwm_set_enable_v2, }; Is that right? >> + .set_enable = rockchip_pwm_set_enable_v2, >> +}; > No need for the double indirection. Sorry, I think is need if you mean a double indirection for ".set_enable". Caesar > > Thierry ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 13:04 ` caesar @ 2014-08-07 13:14 ` Thierry Reding 2014-08-07 13:55 ` caesar 0 siblings, 1 reply; 16+ messages in thread From: Thierry Reding @ 2014-08-07 13:14 UTC (permalink / raw) To: linux-arm-kernel On Thu, Aug 07, 2014 at 09:04:30PM +0800, caesar wrote: [...] > As you say, I will rewrite the about if it's really need do so it. > For example: > > static const struct rockchip_pwm_data pwm_data_v1 = { > .regs = { > .duty = 0x04, > .period = 0x08, > .cntr = 0x00, > .ctrl = 0x0c, > }, > .prescaler = 2, > .set_enable = rockchip_pwm_set_enable_v1, > }; > > static const struct rockchip_pwm_data pwm_data_v2 = { > .regs = { > .duty = 0x08, > .period = 0x04, > .cntr = 0x00, > .ctrl = 0x0c, > }, > .prescaler = 1, > .set_enable = rockchip_pwm_set_enable_v2, > }; > > static const struct rockchip_pwm_data pwm_data_vop = { > .regs = { > .duty = 0x08, > .period = 0x04, > .cntr = 0x0c, > .ctrl = 0x00, > }, > .prescaler = 1, > .set_enable = rockchip_pwm_set_enable_v2, > }; > > Is that right? Yes. > >>+ .set_enable = rockchip_pwm_set_enable_v2, > >>+}; > >No need for the double indirection. > > Sorry, I think is need if you mean a double indirection for ".set_enable". The "double indirection" was regarding the symbolic names for registers, not the .set_enable(). Sorry. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140807/d1950544/attachment.sig> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC 2014-08-07 13:14 ` Thierry Reding @ 2014-08-07 13:55 ` caesar 0 siblings, 0 replies; 16+ messages in thread From: caesar @ 2014-08-07 13:55 UTC (permalink / raw) To: linux-arm-kernel Thierry, ? 2014?08?07? 21:14, Thierry Reding ??: > On Thu, Aug 07, 2014 at 09:04:30PM +0800, caesar wrote: > [...] >> As you say, I will rewrite the about if it's really need do so it. >> For example: >> >> static const struct rockchip_pwm_data pwm_data_v1 = { >> .regs = { >> .duty = 0x04, >> .period = 0x08, >> .cntr = 0x00, >> .ctrl = 0x0c, >> }, >> .prescaler = 2, >> .set_enable = rockchip_pwm_set_enable_v1, >> }; >> >> static const struct rockchip_pwm_data pwm_data_v2 = { >> .regs = { >> .duty = 0x08, >> .period = 0x04, >> .cntr = 0x00, >> .ctrl = 0x0c, >> }, >> .prescaler = 1, >> .set_enable = rockchip_pwm_set_enable_v2, >> }; >> >> static const struct rockchip_pwm_data pwm_data_vop = { >> .regs = { >> .duty = 0x08, >> .period = 0x04, >> .cntr = 0x0c, >> .ctrl = 0x00, >> }, >> .prescaler = 1, >> .set_enable = rockchip_pwm_set_enable_v2, >> }; >> >> Is that right? > Yes. > >>>> + .set_enable = rockchip_pwm_set_enable_v2, >>>> +}; >>> No need for the double indirection. >> Sorry, I think is need if you mean a double indirection for ".set_enable". > The "double indirection" was regarding the symbolic names for registers, > not the .set_enable(). Sorry. OK,I will fix the about in patch v5 tomorrow if no other problems,Thanks! > Thierry ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-08-07 13:55 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-07-24 10:21 [PATCH v4 0/2] This series adds support for RK3288 SoC integrated PWM Caesar Wang 2014-07-24 10:21 ` [PATCH v4 1/2] pwm: rockchip: document RK3288 SoC compatible Caesar Wang 2014-07-24 10:21 ` [PATCH v4 2/2] pwm: rockchip: Added to support for RK3288 SoC Caesar Wang 2014-08-06 22:46 ` Doug Anderson 2014-08-07 1:27 ` caesar 2014-08-07 2:16 ` Doug Anderson 2014-08-07 3:23 ` caesar 2014-08-07 3:26 ` Doug Anderson 2014-08-07 3:37 ` caesar 2014-08-07 3:46 ` Doug Anderson 2014-08-07 4:05 ` caesar 2014-08-07 6:12 ` Thierry Reding 2014-08-07 6:18 ` Thierry Reding 2014-08-07 13:04 ` caesar 2014-08-07 13:14 ` Thierry Reding 2014-08-07 13:55 ` caesar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).