* [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).