[PATCH v3 0/7] Add rk3328 pwm support

[PATCH v3 0/7] Add rk3328 pwm support

[David Wu]

There are two features of rk3328 pwm module.
 - PWM APB and function clocks are different.
 - Add pwm atomic hardware update

David Wu (7):
  pwm: rockchip: Add APB and function both clocks support
  pwm: rockchip: Remove the judge from return value of pwm_config
  pwm: rockchip: Use pwm_apply instead of the pwm_enable
  pwm: rockchip: Move the configuration of polarity from
    rockchip_pwm_set_enable() to rockchip_pwm_config()
  pwm: rockchip: Use same pwm ops for each IP
  pwm: rockchip: Add rk3328 pwm support
  arm64: dts: rockchip: Add pwm nodes for rk3328

 .../devicetree/bindings/pwm/pwm-rockchip.txt       |   8 +-
 arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  45 ++++
 drivers/pwm/pwm-rockchip.c                         | 281 +++++++++++----------
 3 files changed, 199 insertions(+), 135 deletions(-)

-- 
1.9.1

[PATCH v3 1/7] pwm: rockchip: Add APB and function both clocks support

[David Wu]

New PWM module provides two individual clocks for APB clock
and function clock.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>
---
changes in v3:
 - clk_get with NULL argument again if it was error, when clk_get with "pwm" argument.
 - To get clk count, use of_count_phandle_with_args instead of of_property_count_strings.

 .../devicetree/bindings/pwm/pwm-rockchip.txt       |  8 ++-
 drivers/pwm/pwm-rockchip.c                         | 58 ++++++++++++++++++----
 2 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
index b8be3d0..2350ef9 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -6,7 +6,13 @@ Required properties:
    "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
+ - clocks: See ../clock/clock-bindings.txt
+   - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288, rk3399):
+     - There is one clock that's used both to derive the functional clock
+       for the device and as the bus clock.
+   - For newer hardware (rk3328 and future socs): specified by name
+     - "pwm": This is used to derive the functional clock.
+     - "pclk": This is the APB bus clock.
  - #pwm-cells: must be 2 (rk2928) or 3 (rk3288). See pwm.txt in this directory
    for a description of the cell format.
 
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 744d561..ac3cd5e 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -33,6 +33,7 @@
 struct rockchip_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
+	struct clk *pclk;
 	const struct rockchip_pwm_data *data;
 	void __iomem *base;
 };
@@ -145,7 +146,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	u64 tmp;
 	int ret;
 
-	ret = clk_enable(pc->clk);
+	ret = clk_enable(pc->pclk);
 	if (ret)
 		return;
 
@@ -161,7 +162,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 
 	pc->data->get_state(chip, pwm, state);
 
-	clk_disable(pc->clk);
+	clk_disable(pc->pclk);
 }
 
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -224,7 +225,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 
-	ret = clk_enable(pc->clk);
+	ret = clk_enable(pc->pclk);
 	if (ret)
 		return ret;
 
@@ -257,7 +258,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	rockchip_pwm_get_state(chip, pwm, state);
 
 out:
-	clk_disable(pc->clk);
+	clk_disable(pc->pclk);
 
 	return ret;
 }
@@ -328,7 +329,7 @@ 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;
+	int ret, count;
 
 	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
 	if (!id)
@@ -343,13 +344,43 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->base))
 		return PTR_ERR(pc->base);
 
-	pc->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pc->clk))
-		return PTR_ERR(pc->clk);
+	pc->clk = devm_clk_get(&pdev->dev, "pwm");
+	if (IS_ERR(pc->clk)) {
+		pc->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(pc->clk)) {
+			ret = PTR_ERR(pc->clk);
+			if (ret != -EPROBE_DEFER)
+				dev_err(&pdev->dev, "Can't get bus clk: %d\n",
+					ret);
+			return ret;
+		}
+	}
+
+	count = of_count_phandle_with_args(pdev->dev.of_node,
+					   "clocks", "#clock-cells");
+	if (count == 2)
+		pc->pclk = devm_clk_get(&pdev->dev, "pclk");
+	else
+		pc->pclk = pc->clk;
+
+	if (IS_ERR(pc->pclk)) {
+		ret = PTR_ERR(pc->pclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
+		return ret;
+	}
 
 	ret = clk_prepare_enable(pc->clk);
-	if (ret)
+	if (ret) {
+		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
 		return ret;
+	}
+
+	ret = clk_prepare(pc->pclk);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't prepare APB clk: %d\n", ret);
+		goto err_clk;
+	}
 
 	platform_set_drvdata(pdev, pc);
 
@@ -368,12 +399,20 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		clk_unprepare(pc->clk);
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		goto err_pclk;
 	}
 
 	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
 	if (!pwm_is_enabled(pc->chip.pwms))
 		clk_disable(pc->clk);
 
+	return 0;
+
+err_pclk:
+	clk_unprepare(pc->pclk);
+err_clk:
+	clk_disable_unprepare(pc->clk);
+
 	return ret;
 }
 
@@ -395,6 +434,7 @@ static int rockchip_pwm_remove(struct platform_device *pdev)
 	if (pwm_is_enabled(pc->chip.pwms))
 		clk_disable(pc->clk);
 
+	clk_unprepare(pc->pclk);
 	clk_unprepare(pc->clk);
 
 	return pwmchip_remove(&pc->chip);
-- 
1.9.1

Re: [PATCH v3 1/7] pwm: rockchip: Add APB and function both clocks support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

On Tue, Aug 08, 2017 at 11:38:29PM +0800, David Wu wrote:
> New PWM module provides two individual clocks for APB clock
> and function clock.
> 
> Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> changes in v3:
>  - clk_get with NULL argument again if it was error, when clk_get with "pwm" argument.
>  - To get clk count, use of_count_phandle_with_args instead of of_property_count_strings.
> 
>  .../devicetree/bindings/pwm/pwm-rockchip.txt       |  8 ++-
>  drivers/pwm/pwm-rockchip.c                         | 58 ++++++++++++++++++----
>  2 files changed, 56 insertions(+), 10 deletions(-)

Applied to for-4.14/drivers, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 2/7] pwm: rockchip: Remove the judge from return value of pwm_config

[David Wu]

It seems the rockchip_pwm_config always returns the result 0,
so remove the judge.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-rockchip.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index ac3cd5e..33bbb5a 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -165,7 +165,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	clk_disable(pc->pclk);
 }
 
-static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			       int duty_ns, int period_ns)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
@@ -188,8 +188,6 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
-
-	return 0;
 }
 
 static int rockchip_pwm_enable(struct pwm_chip *chip,
@@ -236,13 +234,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		enabled = false;
 	}
 
-	ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
-	if (ret) {
-		if (enabled != curstate.enabled)
-			rockchip_pwm_enable(chip, pwm, !enabled,
-				      state->polarity);
-		goto out;
-	}
+	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
 
 	if (state->enabled != enabled) {
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-- 
1.9.1

Re: [PATCH v3 2/7] pwm: rockchip: Remove the judge from return value of pwm_config

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Tue, Aug 08, 2017 at 11:38:30PM +0800, David Wu wrote:
> It seems the rockchip_pwm_config always returns the result 0,
> so remove the judge.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)

I don't understand what the "judge" is supposed to be, but I've applied
this to for-4.14/drivers anyway. Thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/7] pwm: rockchip: Remove the judge from return value of pwm_config

[Brian Norris]

On Fri, Aug 18, 2017 at 05:32:55PM +0200, Thierry Reding wrote:
> I don't understand what the "judge" is supposed to be, but I've applied
> this to for-4.14/drivers anyway. Thanks.

Probably just an artifact of non-native English.

Judging by context, David probably meant more like "Remove the check for
the return value". I believe I've seen other non-native English speakers
use "judge" in a way similar to "check" or "test" in the context of
comparisons and error checking. (A search through my mailbox for "judge"
confirms this.)

Regards,
Brian

Re: [PATCH v3 2/7] pwm: rockchip: Remove the judge from return value of pwm_config

[David.Wu]

Hi Thierry,

在 2017/8/19 0:28, Brian Norris 写道:
> On Fri, Aug 18, 2017 at 05:32:55PM +0200, Thierry Reding wrote:
>> I don't understand what the "judge" is supposed to be, but I've applied
>> this to for-4.14/drivers anyway. Thanks.
> 
> Probably just an artifact of non-native English.
> 
> Judging by context, David probably meant more like "Remove the check for
> the return value". I believe I've seen other non-native English speakers
> use "judge" in a way similar to "check" or "test" in the context of
> comparisons and error checking. (A search through my mailbox for "judge"
> confirms this.)
> 

Yeap, as Brain said, the "Remove the check for the return value "will be 
better, the "judge" looks like a grammatical error here now.

> Regards,
> Brian
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 4/7] pwm: rockchip: Move the configuration of polarity from rockchip_pwm_set_enable() to rockchip_pwm_config()

[David Wu]

It is usually possible to configure the polarity, cycle and duty all at once,
so that the polarity and cycle and duty should be binding together. Move it
into rockchip_pwm_config(), as well as prepared for the next atomic update
commit.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 drivers/pwm/pwm-rockchip.c | 48 +++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 4bbecbd..fbd9c1a 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -27,6 +27,7 @@
 #define PWM_DUTY_NEGATIVE	(0 << 3)
 #define PWM_INACTIVE_NEGATIVE	(0 << 4)
 #define PWM_INACTIVE_POSITIVE	(1 << 4)
+#define PWM_POLARITY_MASK	(PWM_DUTY_POSITIVE | PWM_INACTIVE_POSITIVE)
 #define PWM_OUTPUT_LEFT		(0 << 5)
 #define PWM_LP_DISABLE		(0 << 8)
 
@@ -123,11 +124,12 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 }
 
 static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       int duty_ns, int period_ns)
+			       struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	unsigned long period, duty;
 	u64 clk_rate, div;
+	u32 ctrl;
 
 	clk_rate = clk_get_rate(pc->clk);
 
@@ -136,22 +138,31 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * bits, every possible input period can be obtained using the
 	 * default prescaler value for all practical clock rate values.
 	 */
-	div = clk_rate * period_ns;
+	div = clk_rate * state->period;
 	period = DIV_ROUND_CLOSEST_ULL(div,
 				       pc->data->prescaler * NSEC_PER_SEC);
 
-	div = clk_rate * duty_ns;
+	div = clk_rate * state->duty_cycle;
 	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
 
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
+
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if (pc->data->supports_polarity) {
+		ctrl &= ~PWM_POLARITY_MASK;
+		if (state->polarity == PWM_POLARITY_INVERSED)
+			ctrl |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+		else
+			ctrl |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
+	}
+	writel(ctrl, pc->base + pc->data->regs.ctrl);
 }
 
 static int rockchip_pwm_enable(struct pwm_chip *chip,
-			 struct pwm_device *pwm,
-			 bool enable,
-			 enum pwm_polarity polarity,
-			 u32 enable_conf)
+			       struct pwm_device *pwm,
+			       bool enable,
+			       u32 enable_conf)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	int ret;
@@ -163,15 +174,6 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 			return ret;
 	}
 
-	if (pc->data->supports_polarity) {
-		if (polarity == PWM_POLARITY_INVERSED)
-			enable_conf |= PWM_DUTY_NEGATIVE |
-				       PWM_INACTIVE_POSITIVE;
-		else
-			enable_conf |= PWM_DUTY_POSITIVE |
-				       PWM_INACTIVE_NEGATIVE;
-	}
-
 	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
 
 	if (enable)
@@ -199,17 +201,16 @@ static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 	enabled = curstate.enabled;
 
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
-					  enable_conf);
+		ret = rockchip_pwm_enable(chip, pwm, false, enable_conf);
 		if (ret)
 			return ret;
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	rockchip_pwm_config(chip, pwm, state);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-				    state->polarity, enable_conf);
+					  enable_conf);
 
 	return ret;
 }
@@ -227,17 +228,16 @@ static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	enabled = curstate.enabled;
 
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
-					  enable_conf);
+		ret = rockchip_pwm_enable(chip, pwm, false, enable_conf);
 		if (ret)
 			return ret;
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	rockchip_pwm_config(chip, pwm, state);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-					  state->polarity, enable_conf);
+					  enable_conf);
 
 	return ret;
 }
-- 
1.9.1

Re: [PATCH v3 4/7] pwm: rockchip: Move the configuration of polarity from rockchip_pwm_set_enable() to rockchip_pwm_config()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

On Tue, Aug 08, 2017 at 11:38:32PM +0800, David Wu wrote:
> It is usually possible to configure the polarity, cycle and duty all at once,
> so that the polarity and cycle and duty should be binding together. Move it
> into rockchip_pwm_config(), as well as prepared for the next atomic update
> commit.
> 
> Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>  drivers/pwm/pwm-rockchip.c | 48 +++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 5/7] pwm: rockchip: Use same pwm ops for each IP

[David Wu]

Just use the same pwm ops for each IP, and get rid of the
ops in the struct of rockchip_pwm_data, but still define
the 3 different rockchip_pwm_data to use common interface
for each IP.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 drivers/pwm/pwm-rockchip.c | 147 ++++++++++++---------------------------------
 1 file changed, 38 insertions(+), 109 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index fbd9c1a..a3fcb40 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -50,12 +50,7 @@ struct rockchip_pwm_data {
 	struct rockchip_pwm_regs regs;
 	unsigned int prescaler;
 	bool supports_polarity;
-	const struct pwm_ops *ops;
-
-	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
-			  struct pwm_state *state);
-	int (*pwm_apply)(struct pwm_chip *chip, struct pwm_device *pwm,
-			 struct pwm_state *state);
+	u32 enable_conf;
 };
 
 static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
@@ -63,45 +58,15 @@ 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_get_state_v1(struct pwm_chip *chip,
-				      struct pwm_device *pwm,
-				      struct pwm_state *state)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
-	u32 val;
-
-	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
-	if ((val & enable_conf) == enable_conf)
-		state->enabled = true;
-}
-
-static void rockchip_pwm_get_state_v2(struct pwm_chip *chip,
-				      struct pwm_device *pwm,
-				      struct pwm_state *state)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
-			  PWM_CONTINUOUS;
-	u32 val;
-
-	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
-	if ((val & enable_conf) != enable_conf)
-		return;
-
-	state->enabled = true;
-
-	if (!(val & PWM_DUTY_POSITIVE))
-		state->polarity = PWM_POLARITY_INVERSED;
-}
-
 static void rockchip_pwm_get_state(struct pwm_chip *chip,
 				   struct pwm_device *pwm,
 				   struct pwm_state *state)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = pc->data->enable_conf;
 	unsigned long clk_rate;
 	u64 tmp;
+	u32 val;
 	int ret;
 
 	ret = clk_enable(pc->pclk);
@@ -116,9 +81,20 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 
 	tmp = readl_relaxed(pc->base + pc->data->regs.duty);
 	tmp *= pc->data->prescaler * NSEC_PER_SEC;
-	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
+	state->duty_cycle =  DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
 
-	pc->data->get_state(chip, pwm, state);
+	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if (pc->data->supports_polarity)
+		state->enabled = ((val & enable_conf) != enable_conf) ?
+				 false : true;
+	else
+		state->enabled = ((val & enable_conf) == enable_conf) ?
+				 true : false;
+
+	if (pc->data->supports_polarity) {
+		if (!(val & PWM_DUTY_POSITIVE))
+			state->polarity = PWM_POLARITY_INVERSED;
+	}
 
 	clk_disable(pc->pclk);
 }
@@ -161,10 +137,10 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 static int rockchip_pwm_enable(struct pwm_chip *chip,
 			       struct pwm_device *pwm,
-			       bool enable,
-			       u32 enable_conf)
+			       bool enable)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = pc->data->enable_conf;
 	int ret;
 	u32 val;
 
@@ -189,73 +165,35 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 	return 0;
 }
 
-static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
-				 struct pwm_state *state)
+static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
 {
-	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	struct pwm_state curstate;
 	bool enabled;
 	int ret = 0;
 
+	ret = clk_enable(pc->pclk);
+	if (ret)
+		return ret;
+
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, enable_conf);
+		ret = rockchip_pwm_enable(chip, pwm, false);
 		if (ret)
-			return ret;
+			goto out;
 		enabled = false;
 	}
 
 	rockchip_pwm_config(chip, pwm, state);
-	if (state->enabled != enabled)
-		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-					  enable_conf);
-
-	return ret;
-}
-
-static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
-				 struct pwm_state *state)
-{
-	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
-		  PWM_CONTINUOUS;
-	struct pwm_state curstate;
-	bool enabled;
-	int ret = 0;
-
-	pwm_get_state(pwm, &curstate);
-	enabled = curstate.enabled;
-
-	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, enable_conf);
+	if (state->enabled != enabled) {
+		ret = rockchip_pwm_enable(chip, pwm, state->enabled);
 		if (ret)
-			return ret;
-		enabled = false;
+			goto out;
 	}
 
-	rockchip_pwm_config(chip, pwm, state);
-	if (state->enabled != enabled)
-		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-					  enable_conf);
-
-	return ret;
-}
-
-static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			      struct pwm_state *state)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	int ret;
-
-	ret = clk_enable(pc->pclk);
-	if (ret)
-		return ret;
-
-	ret = pc->data->pwm_apply(chip, pwm, state);
-	if (ret)
-		goto out;
-
 	/*
 	 * Update the state with the real hardware, which can differ a bit
 	 * because of period/duty_cycle approximation.
@@ -268,13 +206,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
-static const struct pwm_ops rockchip_pwm_ops_v1 = {
-	.get_state = rockchip_pwm_get_state,
-	.apply = rockchip_pwm_apply,
-	.owner = THIS_MODULE,
-};
-
-static const struct pwm_ops rockchip_pwm_ops_v2 = {
+static const struct pwm_ops rockchip_pwm_ops = {
 	.get_state = rockchip_pwm_get_state,
 	.apply = rockchip_pwm_apply,
 	.owner = THIS_MODULE,
@@ -288,9 +220,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		.ctrl = 0x0c,
 	},
 	.prescaler = 2,
-	.ops = &rockchip_pwm_ops_v1,
-	.get_state = rockchip_pwm_get_state_v1,
-	.pwm_apply = rockchip_pwm_apply_v1,
+	.supports_polarity = false,
+	.enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {
@@ -302,9 +233,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 1,
 	.supports_polarity = true,
-	.ops = &rockchip_pwm_ops_v2,
-	.get_state = rockchip_pwm_get_state_v2,
-	.pwm_apply = rockchip_pwm_apply_v2,
+	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		       PWM_CONTINUOUS,
 };
 
 static const struct rockchip_pwm_data pwm_data_vop = {
@@ -316,9 +246,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 1,
 	.supports_polarity = true,
-	.ops = &rockchip_pwm_ops_v2,
-	.get_state = rockchip_pwm_get_state_v2,
-	.pwm_apply = rockchip_pwm_apply_v2,
+	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		       PWM_CONTINUOUS,
 };
 
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
@@ -391,7 +320,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 
 	pc->data = id->data;
 	pc->chip.dev = &pdev->dev;
-	pc->chip.ops = pc->data->ops;
+	pc->chip.ops = &rockchip_pwm_ops;
 	pc->chip.base = -1;
 	pc->chip.npwm = 1;
 
-- 
1.9.1

Re: [PATCH v3 5/7] pwm: rockchip: Use same pwm ops for each IP

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On Tue, Aug 08, 2017 at 11:41:28PM +0800, David Wu wrote:
> Just use the same pwm ops for each IP, and get rid of the
> ops in the struct of rockchip_pwm_data, but still define
> the 3 different rockchip_pwm_data to use common interface
> for each IP.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 147 ++++++++++++---------------------------------
>  1 file changed, 38 insertions(+), 109 deletions(-)

Applied to for-4.14/drivers, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 5/7] pwm: rockchip: Use same pwm ops for each IP

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 510 bytes --]

On Tue, Aug 08, 2017 at 11:41:28PM +0800, David Wu wrote:
> Just use the same pwm ops for each IP, and get rid of the
> ops in the struct of rockchip_pwm_data, but still define
> the 3 different rockchip_pwm_data to use common interface
> for each IP.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 147 ++++++++++++---------------------------------
>  1 file changed, 38 insertions(+), 109 deletions(-)

Applied to for-4.14/drivers, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 6/7] pwm: rockchip: Add rk3328 pwm support

[David Wu]

The rk3328 soc supports atomic update, we could lock the configuration
of period and duty at first, after unlock is configured, the period and
duty are effective at the same time.

If the polarity, period and duty need to be configured together,
the way for atomic update is "configure lock and old polarity" ->
"configure period and duty" -> "configure unlock and new polarity".

Signed-off-by: David Wu <david.wu@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 drivers/pwm/pwm-rockchip.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index a3fcb40..4d99d46 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -29,6 +29,7 @@
 #define PWM_INACTIVE_POSITIVE	(1 << 4)
 #define PWM_POLARITY_MASK	(PWM_DUTY_POSITIVE | PWM_INACTIVE_POSITIVE)
 #define PWM_OUTPUT_LEFT		(0 << 5)
+#define PWM_LOCK_EN		(1 << 6)
 #define PWM_LP_DISABLE		(0 << 8)
 
 struct rockchip_pwm_chip {
@@ -50,6 +51,7 @@ struct rockchip_pwm_data {
 	struct rockchip_pwm_regs regs;
 	unsigned int prescaler;
 	bool supports_polarity;
+	bool supports_lock;
 	u32 enable_conf;
 };
 
@@ -121,10 +123,19 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	div = clk_rate * state->duty_cycle;
 	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
 
+	/*
+	 * Lock the period and duty of previous configuration, then
+	 * change the duty and period, that would not be effective.
+	 */
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if (pc->data->supports_lock) {
+		ctrl |= PWM_LOCK_EN;
+		writel_relaxed(ctrl, pc->base + pc->data->regs.ctrl);
+	}
+
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
 
-	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	if (pc->data->supports_polarity) {
 		ctrl &= ~PWM_POLARITY_MASK;
 		if (state->polarity == PWM_POLARITY_INVERSED)
@@ -132,6 +143,15 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		else
 			ctrl |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
 	}
+
+	/*
+	 * Unlock and set polarity at the same time,
+	 * the configuration of duty, period and polarity
+	 * would be effective together at next period.
+	 */
+	if (pc->data->supports_lock)
+		ctrl &= ~PWM_LOCK_EN;
+
 	writel(ctrl, pc->base + pc->data->regs.ctrl);
 }
 
@@ -180,7 +200,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 
-	if (state->polarity != curstate.polarity && enabled) {
+	if (state->polarity != curstate.polarity && enabled &&
+	    !pc->data->supports_lock) {
 		ret = rockchip_pwm_enable(chip, pwm, false);
 		if (ret)
 			goto out;
@@ -221,6 +242,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 2,
 	.supports_polarity = false,
+	.supports_lock = false,
 	.enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN,
 };
 
@@ -233,6 +255,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 1,
 	.supports_polarity = true,
+	.supports_lock = false,
 	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
 		       PWM_CONTINUOUS,
 };
@@ -246,6 +269,21 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 1,
 	.supports_polarity = true,
+	.supports_lock = false,
+	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		       PWM_CONTINUOUS,
+};
+
+static const struct rockchip_pwm_data pwm_data_v3 = {
+	.regs = {
+		.duty = 0x08,
+		.period = 0x04,
+		.cntr = 0x00,
+		.ctrl = 0x0c,
+	},
+	.prescaler = 1,
+	.supports_polarity = true,
+	.supports_lock = true,
 	.enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
 		       PWM_CONTINUOUS,
 };
@@ -254,6 +292,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	{ .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},
 	{ .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},
 	{ .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},
+	{ .compatible = "rockchip,rk3328-pwm", .data = &pwm_data_v3},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
-- 
1.9.1

Re: [PATCH v3 6/7] pwm: rockchip: Add rk3328 pwm support

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

On Tue, Aug 08, 2017 at 11:42:47PM +0800, David Wu wrote:
> The rk3328 soc supports atomic update, we could lock the configuration
> of period and duty at first, after unlock is configured, the period and
> duty are effective at the same time.
> 
> If the polarity, period and duty need to be configured together,
> the way for atomic update is "configure lock and old polarity" ->
> "configure period and duty" -> "configure unlock and new polarity".
> 
> Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/pwm/pwm-rockchip.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 41 insertions(+), 2 deletions(-)

Applied to for-4.14/drivers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 7/7] arm64: dts: rockchip: Add pwm nodes for rk3328

[David Wu]

There are 4 pwm channels built in rk3328 soc, need to configure
the both APB clock and bus clock.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 45 ++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 7e69f1f..1f945d3 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -308,6 +308,51 @@
 		interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
+	pwm0: pwm@ff1b0000 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0000 0x0 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm0_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
+	pwm1: pwm@ff1b0010 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0010 0x0 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm1_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
+	pwm2: pwm@ff1b0020 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0020 0x0 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm2_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
+	pwm3: pwm@ff1b0030 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0030 0x0 0x10>;
+		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwmir_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
 	saradc: adc@ff280000 {
 		compatible = "rockchip,rk3328-saradc", "rockchip,rk3399-saradc";
 		reg = <0x0 0xff280000 0x0 0x100>;
-- 
1.9.1

Re: [PATCH v3 7/7] arm64: dts: rockchip: Add pwm nodes for rk3328

[Heiko Stuebner]

Am Dienstag, 8. August 2017, 23:43:15 CEST schrieb David Wu:
> There are 4 pwm channels built in rk3328 soc, need to configure
> the both APB clock and bus clock.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>

applied for 4.14 with some property movements.


Thanks
Heiko

Re: [PATCH v3 0/7] Add rk3328 pwm support

[David.Wu]

Hi Boris && thierry,

Do you have any other suggestion for the patches?

在 2017/8/8 23:38, David Wu 写道:
> There are two features of rk3328 pwm module.
>   - PWM APB and function clocks are different.
>   - Add pwm atomic hardware update
> 
> David Wu (7):
>    pwm: rockchip: Add APB and function both clocks support
>    pwm: rockchip: Remove the judge from return value of pwm_config
>    pwm: rockchip: Use pwm_apply instead of the pwm_enable
>    pwm: rockchip: Move the configuration of polarity from
>      rockchip_pwm_set_enable() to rockchip_pwm_config()
>    pwm: rockchip: Use same pwm ops for each IP
>    pwm: rockchip: Add rk3328 pwm support
>    arm64: dts: rockchip: Add pwm nodes for rk3328
> 
>   .../devicetree/bindings/pwm/pwm-rockchip.txt       |   8 +-
>   arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  45 ++++
>   drivers/pwm/pwm-rockchip.c                         | 281 +++++++++++----------
>   3 files changed, 199 insertions(+), 135 deletions(-)
> 

[PATCH v3 3/7] pwm: rockchip: Use pwm_apply instead of the pwm_enable

[David Wu]

Drop the custom hook of pwm_enable and implement
pwm_apply_v1 and pwm_apply_v2 instead.

Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 drivers/pwm/pwm-rockchip.c | 141 +++++++++++++++++++++++++--------------------
 1 file changed, 77 insertions(+), 64 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 33bbb5a..4bbecbd 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -51,11 +51,10 @@ struct rockchip_pwm_data {
 	bool supports_polarity;
 	const struct pwm_ops *ops;
 
-	void (*set_enable)(struct pwm_chip *chip,
-			   struct pwm_device *pwm, bool enable,
-			   enum pwm_polarity polarity);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
+	int (*pwm_apply)(struct pwm_chip *chip, struct pwm_device *pwm,
+			 struct pwm_state *state);
 };
 
 static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
@@ -63,24 +62,6 @@ 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,
-				       struct pwm_device *pwm, bool enable,
-				       enum pwm_polarity polarity)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
-	u32 val;
-
-	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_get_state_v1(struct pwm_chip *chip,
 				      struct pwm_device *pwm,
 				      struct pwm_state *state)
@@ -94,30 +75,6 @@ static void rockchip_pwm_get_state_v1(struct pwm_chip *chip,
 		state->enabled = true;
 }
 
-static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
-				       struct pwm_device *pwm, bool enable,
-				       enum pwm_polarity polarity)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
-			  PWM_CONTINUOUS;
-	u32 val;
-
-	if (polarity == PWM_POLARITY_INVERSED)
-		enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
-	else
-		enable_conf |= 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_get_state_v2(struct pwm_chip *chip,
 				      struct pwm_device *pwm,
 				      struct pwm_state *state)
@@ -193,10 +150,12 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int rockchip_pwm_enable(struct pwm_chip *chip,
 			 struct pwm_device *pwm,
 			 bool enable,
-			 enum pwm_polarity polarity)
+			 enum pwm_polarity polarity,
+			 u32 enable_conf)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	int ret;
+	u32 val;
 
 	if (enable) {
 		ret = clk_enable(pc->clk);
@@ -204,7 +163,23 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 			return ret;
 	}
 
-	pc->data->set_enable(chip, pwm, enable, polarity);
+	if (pc->data->supports_polarity) {
+		if (polarity == PWM_POLARITY_INVERSED)
+			enable_conf |= PWM_DUTY_NEGATIVE |
+				       PWM_INACTIVE_POSITIVE;
+		else
+			enable_conf |= 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);
 
 	if (!enable)
 		clk_disable(pc->clk);
@@ -212,37 +187,75 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 	return 0;
 }
 
-static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			      struct pwm_state *state)
+static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
 {
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
 	struct pwm_state curstate;
 	bool enabled;
-	int ret;
+	int ret = 0;
 
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 
-	ret = clk_enable(pc->pclk);
-	if (ret)
-		return ret;
-
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity);
+		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
+					  enable_conf);
 		if (ret)
-			goto out;
+			return ret;
 		enabled = false;
 	}
 
 	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
-
-	if (state->enabled != enabled) {
+	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-				    state->polarity);
+				    state->polarity, enable_conf);
+
+	return ret;
+}
+
+static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		  PWM_CONTINUOUS;
+	struct pwm_state curstate;
+	bool enabled;
+	int ret = 0;
+
+	pwm_get_state(pwm, &curstate);
+	enabled = curstate.enabled;
+
+	if (state->polarity != curstate.polarity && enabled) {
+		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
+					  enable_conf);
 		if (ret)
-			goto out;
+			return ret;
+		enabled = false;
 	}
 
+	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (state->enabled != enabled)
+		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
+					  state->polarity, enable_conf);
+
+	return ret;
+}
+
+static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	int ret;
+
+	ret = clk_enable(pc->pclk);
+	if (ret)
+		return ret;
+
+	ret = pc->data->pwm_apply(chip, pwm, state);
+	if (ret)
+		goto out;
+
 	/*
 	 * Update the state with the real hardware, which can differ a bit
 	 * because of period/duty_cycle approximation.
@@ -276,8 +289,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 2,
 	.ops = &rockchip_pwm_ops_v1,
-	.set_enable = rockchip_pwm_set_enable_v1,
 	.get_state = rockchip_pwm_get_state_v1,
+	.pwm_apply = rockchip_pwm_apply_v1,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {
@@ -290,8 +303,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	.prescaler = 1,
 	.supports_polarity = true,
 	.ops = &rockchip_pwm_ops_v2,
-	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
+	.pwm_apply = rockchip_pwm_apply_v2,
 };
 
 static const struct rockchip_pwm_data pwm_data_vop = {
@@ -304,8 +317,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	.prescaler = 1,
 	.supports_polarity = true,
 	.ops = &rockchip_pwm_ops_v2,
-	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
+	.pwm_apply = rockchip_pwm_apply_v2,
 };
 
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
-- 
1.9.1

Re: [PATCH v3 3/7] pwm: rockchip: Use pwm_apply instead of the pwm_enable

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

On Tue, Aug 08, 2017 at 11:38:31PM +0800, David Wu wrote:
> Drop the custom hook of pwm_enable and implement
> pwm_apply_v1 and pwm_apply_v2 instead.
> 
> Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>  drivers/pwm/pwm-rockchip.c | 141 +++++++++++++++++++++++++--------------------
>  1 file changed, 77 insertions(+), 64 deletions(-)

Applied to for-4.14/drivers, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/7] Add rk3328 pwm support

[David.Wu]

Hi Thierry,

For the PWM patches, whether we let the first patch land in, that is PWM 
APB and function clocks are different in rk3328 and rv1108 SOCs. If the 
first patch was merged before 4.14, it will guarantee that the PWM is 
working, and the lock function you might review it later.

在 2017/8/8 23:38, David Wu 写道:
> There are two features of rk3328 pwm module.
>   - PWM APB and function clocks are different.
>   - Add pwm atomic hardware update
> 
> David Wu (7):
>    pwm: rockchip: Add APB and function both clocks support
>    pwm: rockchip: Remove the judge from return value of pwm_config
>    pwm: rockchip: Use pwm_apply instead of the pwm_enable
>    pwm: rockchip: Move the configuration of polarity from
>      rockchip_pwm_set_enable() to rockchip_pwm_config()
>    pwm: rockchip: Use same pwm ops for each IP
>    pwm: rockchip: Add rk3328 pwm support
>    arm64: dts: rockchip: Add pwm nodes for rk3328
> 
>   .../devicetree/bindings/pwm/pwm-rockchip.txt       |   8 +-
>   arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  45 ++++
>   drivers/pwm/pwm-rockchip.c                         | 281 +++++++++++----------
>   3 files changed, 199 insertions(+), 135 deletions(-)
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html