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