[PATCH v5 0/3] fix the clks on/off mismatch issue and switch pwm-mtk-disp to atomic APIs

[PATCH v5 0/3] fix the clks on/off mismatch issue and switch pwm-mtk-disp to atomic APIs

[Jitao Shi]

Change since v4:
 - Squash the commit "move the commit to clock enabled" to "adjust the clocks to avoid them mismatch".
 - Drop the useless comment about MT2701.
 - Reenable the clks "mm" and "main" in .enable().
 - Fix typo.
 - Seperate get_state() operation as single patch.

Change since v3:
 - Seperate the clock sequence as single patch.
 - Fixup the reg commit when clocks sequence changed.
 - Merge the apply and get_state as single patch.

Change since v2:
 - Change commit messages to remove the clock operations for atomic APIs.
 - Rebase to v5.13 rc1.

Changes since v1:
 - Seperate clock operation as single patch.
 - Seperate apply() as single patch.
 - Seperate get_state() operation as single patch.

Jitao Shi (3):
  pwm: mtk-disp: adjust the clocks to avoid them mismatch
  pwm: mtk_disp: implement atomic API .apply()
  pwm: mtk_disp: implement atomic API .get_state()

 drivers/pwm/pwm-mtk-disp.c | 168 +++++++++++++++++++------------------
 1 file changed, 85 insertions(+), 83 deletions(-)

-- 
2.25.1

[PATCH v5 1/3] pwm: mtk-disp: adjust the clocks to avoid them mismatch

[Jitao Shi]

The clks "main" and "mm" are prepared in .probe() (and
unprepared in .remove()). This results in the clocks being on
during suspend which results in unnecessarily increased power
consumption.

Remove the clock operations from .probe() and .remove().
Add the clk_prepare_enable() in .config().
Add the clk_disable_unprepare() in .disable().
Remove the MT2701 double buffer comment.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
---
 drivers/pwm/pwm-mtk-disp.c | 116 ++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 59 deletions(-)

diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
index 9b3ba401a3db..204d76beeb26 100644
--- a/drivers/pwm/pwm-mtk-disp.c
+++ b/drivers/pwm/pwm-mtk-disp.c
@@ -47,6 +47,7 @@ struct mtk_disp_pwm {
 	struct clk *clk_main;
 	struct clk *clk_mm;
 	void __iomem *base;
+	bool enabled;
 };
 
 static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
@@ -74,6 +75,22 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 div, rate;
 	int err;
 
+	if (!mdp->enabled) {
+		err = clk_prepare_enable(mdp->clk_main);
+		if (err < 0) {
+			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
+				ERR_PTR(err));
+			return err;
+		}
+		err = clk_prepare_enable(mdp->clk_mm);
+		if (err < 0) {
+			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
+				ERR_PTR(err));
+			clk_disable_unprepare(mdp->clk_main);
+			return err;
+		}
+	}
+
 	/*
 	 * Find period, high_width and clk_div to suit duty_ns and period_ns.
 	 * Calculate proper div value to keep period value in the bound.
@@ -87,9 +104,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	rate = clk_get_rate(mdp->clk_main);
 	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
 			  PWM_PERIOD_BIT_WIDTH;
-	if (clk_div > PWM_CLKDIV_MAX)
+	if (clk_div > PWM_CLKDIV_MAX) {
+		if (!mdp->enabled) {
+			clk_disable_unprepare(mdp->clk_mm);
+			clk_disable_unprepare(mdp->clk_main);
+		}
 		return -EINVAL;
-
+	}
 	div = NSEC_PER_SEC * (clk_div + 1);
 	period = div64_u64(rate * period_ns, div);
 	if (period > 0)
@@ -98,16 +119,6 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	high_width = div64_u64(rate * duty_ns, div);
 	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
 
-	err = clk_enable(mdp->clk_main);
-	if (err < 0)
-		return err;
-
-	err = clk_enable(mdp->clk_mm);
-	if (err < 0) {
-		clk_disable(mdp->clk_main);
-		return err;
-	}
-
 	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
 				 PWM_CLKDIV_MASK,
 				 clk_div << PWM_CLKDIV_SHIFT);
@@ -122,10 +133,19 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		mtk_disp_pwm_update_bits(mdp, mdp->data->commit,
 					 mdp->data->commit_mask,
 					 0x0);
+	} else {
+		mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug,
+					 mdp->data->bls_debug_mask,
+					 mdp->data->bls_debug_mask);
+		mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
+					 mdp->data->con0_sel,
+					 mdp->data->con0_sel);
 	}
 
-	clk_disable(mdp->clk_mm);
-	clk_disable(mdp->clk_main);
+	if (!mdp->enabled) {
+		clk_disable_unprepare(mdp->clk_mm);
+		clk_disable_unprepare(mdp->clk_main);
+	}
 
 	return 0;
 }
@@ -135,18 +155,25 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
 	int err;
 
-	err = clk_enable(mdp->clk_main);
-	if (err < 0)
-		return err;
-
-	err = clk_enable(mdp->clk_mm);
-	if (err < 0) {
-		clk_disable(mdp->clk_main);
-		return err;
+	if (!mdp->enabled) {
+		err = clk_prepare_enable(mdp->clk_main);
+		if (err < 0) {
+			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
+				ERR_PTR(err));
+			return err;
+		}
+		err = clk_prepare_enable(mdp->clk_mm);
+		if (err < 0) {
+			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
+				ERR_PTR(err));
+			clk_disable_unprepare(mdp->clk_main);
+			return err;
+		}
 	}
 
 	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
 				 mdp->data->enable_mask);
+	mdp->enabled = true;
 
 	return 0;
 }
@@ -158,8 +185,11 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
 				 0x0);
 
-	clk_disable(mdp->clk_mm);
-	clk_disable(mdp->clk_main);
+	if (mdp->enabled) {
+		clk_disable_unprepare(mdp->clk_mm);
+		clk_disable_unprepare(mdp->clk_main);
+	}
+	mdp->enabled = false;
 }
 
 static const struct pwm_ops mtk_disp_pwm_ops = {
@@ -192,58 +222,26 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(mdp->clk_mm))
 		return PTR_ERR(mdp->clk_mm);
 
-	ret = clk_prepare(mdp->clk_main);
-	if (ret < 0)
-		return ret;
-
-	ret = clk_prepare(mdp->clk_mm);
-	if (ret < 0)
-		goto disable_clk_main;
-
 	mdp->chip.dev = &pdev->dev;
 	mdp->chip.ops = &mtk_disp_pwm_ops;
 	mdp->chip.npwm = 1;
 
 	ret = pwmchip_add(&mdp->chip);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		goto disable_clk_mm;
+		dev_err(&pdev->dev, "pwmchip_add() failed: %pe\n", ERR_PTR(ret));
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, mdp);
 
-	/*
-	 * For MT2701, disable double buffer before writing register
-	 * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH.
-	 */
-	if (!mdp->data->has_commit) {
-		mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug,
-					 mdp->data->bls_debug_mask,
-					 mdp->data->bls_debug_mask);
-		mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
-					 mdp->data->con0_sel,
-					 mdp->data->con0_sel);
-	}
-
 	return 0;
-
-disable_clk_mm:
-	clk_unprepare(mdp->clk_mm);
-disable_clk_main:
-	clk_unprepare(mdp->clk_main);
-	return ret;
 }
 
 static int mtk_disp_pwm_remove(struct platform_device *pdev)
 {
 	struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev);
-	int ret;
-
-	ret = pwmchip_remove(&mdp->chip);
-	clk_unprepare(mdp->clk_mm);
-	clk_unprepare(mdp->clk_main);
 
-	return ret;
+	return pwmchip_remove(&mdp->chip);
 }
 
 static const struct mtk_pwm_data mt2701_pwm_data = {
-- 
2.25.1

[PATCH v5 2/3] pwm: mtk_disp: implement atomic API .apply()

[Jitao Shi]

Switch the driver to support the .apply() method.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
---
 drivers/pwm/pwm-mtk-disp.c | 69 ++++++++++----------------------------
 1 file changed, 17 insertions(+), 52 deletions(-)

diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
index 204d76beeb26..a4766e931b68 100644
--- a/drivers/pwm/pwm-mtk-disp.c
+++ b/drivers/pwm/pwm-mtk-disp.c
@@ -67,14 +67,25 @@ static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp, u32 offset,
 	writel(value, address);
 }
 
-static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       int duty_ns, int period_ns)
+static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
 {
 	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
 	u32 clk_div, period, high_width, value;
 	u64 div, rate;
 	int err;
 
+	if (!state->enabled) {
+		mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
+					 0x0);
+		if (mdp->enabled) {
+			clk_disable_unprepare(mdp->clk_mm);
+			clk_disable_unprepare(mdp->clk_main);
+		}
+		mdp->enabled = false;
+		return 0;
+	}
+
 	if (!mdp->enabled) {
 		err = clk_prepare_enable(mdp->clk_main);
 		if (err < 0) {
@@ -102,7 +113,7 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
 	 */
 	rate = clk_get_rate(mdp->clk_main);
-	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
+	clk_div = div_u64(rate * state->period, NSEC_PER_SEC) >>
 			  PWM_PERIOD_BIT_WIDTH;
 	if (clk_div > PWM_CLKDIV_MAX) {
 		if (!mdp->enabled) {
@@ -112,11 +123,11 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -EINVAL;
 	}
 	div = NSEC_PER_SEC * (clk_div + 1);
-	period = div64_u64(rate * period_ns, div);
+	period = div64_u64(rate * state->period, div);
 	if (period > 0)
 		period--;
 
-	high_width = div64_u64(rate * duty_ns, div);
+	high_width = div64_u64(rate * state->duty_cycle, div);
 	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
 
 	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
@@ -141,36 +152,6 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 					 mdp->data->con0_sel,
 					 mdp->data->con0_sel);
 	}
-
-	if (!mdp->enabled) {
-		clk_disable_unprepare(mdp->clk_mm);
-		clk_disable_unprepare(mdp->clk_main);
-	}
-
-	return 0;
-}
-
-static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
-	int err;
-
-	if (!mdp->enabled) {
-		err = clk_prepare_enable(mdp->clk_main);
-		if (err < 0) {
-			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
-				ERR_PTR(err));
-			return err;
-		}
-		err = clk_prepare_enable(mdp->clk_mm);
-		if (err < 0) {
-			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
-				ERR_PTR(err));
-			clk_disable_unprepare(mdp->clk_main);
-			return err;
-		}
-	}
-
 	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
 				 mdp->data->enable_mask);
 	mdp->enabled = true;
@@ -178,24 +159,8 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	return 0;
 }
 
-static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
-
-	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
-				 0x0);
-
-	if (mdp->enabled) {
-		clk_disable_unprepare(mdp->clk_mm);
-		clk_disable_unprepare(mdp->clk_main);
-	}
-	mdp->enabled = false;
-}
-
 static const struct pwm_ops mtk_disp_pwm_ops = {
-	.config = mtk_disp_pwm_config,
-	.enable = mtk_disp_pwm_enable,
-	.disable = mtk_disp_pwm_disable,
+	.apply = mtk_disp_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.25.1

[PATCH v5 3/3] pwm: mtk_disp: implement atomic API .get_state()

[Jitao Shi]

Switch the driver to support the .get_state() method.

Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
---
 drivers/pwm/pwm-mtk-disp.c | 39 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
index a4766e931b68..d60a6b379683 100644
--- a/drivers/pwm/pwm-mtk-disp.c
+++ b/drivers/pwm/pwm-mtk-disp.c
@@ -159,8 +159,47 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
+	u32 clk_div, con0, con1;
+	u64 rate, period, high_width;
+	int err;
+
+	if (!mdp->enabled) {
+		err = clk_prepare_enable(mdp->clk_main);
+		if (err < 0) {
+			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n", ERR_PTR(err));
+			return;
+		}
+		err = clk_prepare_enable(mdp->clk_mm);
+		if (err < 0) {
+			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n", ERR_PTR(err));
+			clk_disable_unprepare(mdp->clk_main);
+			return;
+		}
+	}
+	rate = clk_get_rate(mdp->clk_main);
+	con0 = readl(mdp->base + mdp->data->con0);
+	con1 = readl(mdp->base + mdp->data->con1);
+	state->enabled = !!(con0 & BIT(0));
+	clk_div = FIELD_GET(PWM_CLKDIV_MASK, con0);
+	period = con1 & PWM_PERIOD_MASK;
+	state->period = DIV64_U64_ROUND_UP(period * (clk_div + 1) * NSEC_PER_SEC, rate);
+	high_width = FIELD_GET(PWM_HIGH_WIDTH_MASK, con1);
+	state->duty_cycle = DIV64_U64_ROUND_UP(high_width * (clk_div + 1) * NSEC_PER_SEC,
+					       rate);
+	if (!mdp->enabled) {
+		clk_disable_unprepare(mdp->clk_mm);
+		clk_disable_unprepare(mdp->clk_main);
+	}
+}
+
 static const struct pwm_ops mtk_disp_pwm_ops = {
 	.apply = mtk_disp_pwm_apply,
+	.get_state = mtk_disp_pwm_get_state,
 	.owner = THIS_MODULE,
 };
 
-- 
2.25.1

Re: [PATCH v5 1/3] pwm: mtk-disp: adjust the clocks to avoid them mismatch

[Uwe Kleine-König]

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

On Wed, Jun 16, 2021 at 04:52:22PM +0800, Jitao Shi wrote:
> The clks "main" and "mm" are prepared in .probe() (and
> unprepared in .remove()). This results in the clocks being on
> during suspend which results in unnecessarily increased power
> consumption.
> 
> Remove the clock operations from .probe() and .remove().
> Add the clk_prepare_enable() in .config().
> Add the clk_disable_unprepare() in .disable().
> Remove the MT2701 double buffer comment.
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
>  drivers/pwm/pwm-mtk-disp.c | 116 ++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 9b3ba401a3db..204d76beeb26 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -47,6 +47,7 @@ struct mtk_disp_pwm {
>  	struct clk *clk_main;
>  	struct clk *clk_mm;
>  	void __iomem *base;
> +	bool enabled;
>  };
>  
>  static inline struct mtk_disp_pwm *to_mtk_disp_pwm(struct pwm_chip *chip)
> @@ -74,6 +75,22 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	u64 div, rate;
>  	int err;
>  
> +	if (!mdp->enabled) {
> +		err = clk_prepare_enable(mdp->clk_main);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
> +				ERR_PTR(err));
> +			return err;
> +		}
> +		err = clk_prepare_enable(mdp->clk_mm);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
> +				ERR_PTR(err));
> +			clk_disable_unprepare(mdp->clk_main);
> +			return err;
> +		}
> +	}
> +

The logic becomes a tad simpler if you prepare_enable unconditionally on
entry and disable_unprepare on exit. Given that in the mdp->enabled ==
true the clocks are already on they are a noop in this case and the
matching disable just decreases the usage count.

>  	/*
>  	 * Find period, high_width and clk_div to suit duty_ns and period_ns.
>  	 * Calculate proper div value to keep period value in the bound.
> @@ -87,9 +104,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	rate = clk_get_rate(mdp->clk_main);
>  	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
>  			  PWM_PERIOD_BIT_WIDTH;
> -	if (clk_div > PWM_CLKDIV_MAX)
> +	if (clk_div > PWM_CLKDIV_MAX) {
> +		if (!mdp->enabled) {
> +			clk_disable_unprepare(mdp->clk_mm);
> +			clk_disable_unprepare(mdp->clk_main);
> +		}
>  		return -EINVAL;
> -
> +	}
>  	div = NSEC_PER_SEC * (clk_div + 1);
>  	period = div64_u64(rate * period_ns, div);
>  	if (period > 0)
> @@ -98,16 +119,6 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	high_width = div64_u64(rate * duty_ns, div);
>  	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
>  
> -	err = clk_enable(mdp->clk_main);
> -	if (err < 0)
> -		return err;
> -
> -	err = clk_enable(mdp->clk_mm);
> -	if (err < 0) {
> -		clk_disable(mdp->clk_main);
> -		return err;
> -	}
> -
>  	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
>  				 PWM_CLKDIV_MASK,
>  				 clk_div << PWM_CLKDIV_SHIFT);
> @@ -122,10 +133,19 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		mtk_disp_pwm_update_bits(mdp, mdp->data->commit,
>  					 mdp->data->commit_mask,
>  					 0x0);
> +	} else {
> +		mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug,
> +					 mdp->data->bls_debug_mask,
> +					 mdp->data->bls_debug_mask);
> +		mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> +					 mdp->data->con0_sel,
> +					 mdp->data->con0_sel);

This is unrelated for this patch, isn't it?

>  	}
>  
> -	clk_disable(mdp->clk_mm);
> -	clk_disable(mdp->clk_main);
> +	if (!mdp->enabled) {
> +		clk_disable_unprepare(mdp->clk_mm);
> +		clk_disable_unprepare(mdp->clk_main);
> +	}
>  
>  	return 0;
>  }
> @@ -135,18 +155,25 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>  	int err;
>  
> -	err = clk_enable(mdp->clk_main);
> -	if (err < 0)
> -		return err;
> -
> -	err = clk_enable(mdp->clk_mm);
> -	if (err < 0) {
> -		clk_disable(mdp->clk_main);
> -		return err;
> +	if (!mdp->enabled) {
> +		err = clk_prepare_enable(mdp->clk_main);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n",
> +				ERR_PTR(err));
> +			return err;
> +		}
> +		err = clk_prepare_enable(mdp->clk_mm);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n",
> +				ERR_PTR(err));
> +			clk_disable_unprepare(mdp->clk_main);
> +			return err;
> +		}
>  	}
>  
>  	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
>  				 mdp->data->enable_mask);
> +	mdp->enabled = true;
>  
>  	return 0;
>  }
> @@ -158,8 +185,11 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
>  				 0x0);
>  
> -	clk_disable(mdp->clk_mm);
> -	clk_disable(mdp->clk_main);
> +	if (mdp->enabled) {
> +		clk_disable_unprepare(mdp->clk_mm);
> +		clk_disable_unprepare(mdp->clk_main);
> +	}
> +	mdp->enabled = false;
>  }
>  
>  static const struct pwm_ops mtk_disp_pwm_ops = {
> @@ -192,58 +222,26 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(mdp->clk_mm))
>  		return PTR_ERR(mdp->clk_mm);
>  
> -	ret = clk_prepare(mdp->clk_main);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = clk_prepare(mdp->clk_mm);
> -	if (ret < 0)
> -		goto disable_clk_main;
> -
>  	mdp->chip.dev = &pdev->dev;
>  	mdp->chip.ops = &mtk_disp_pwm_ops;
>  	mdp->chip.npwm = 1;
>  
>  	ret = pwmchip_add(&mdp->chip);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> -		goto disable_clk_mm;
> +		dev_err(&pdev->dev, "pwmchip_add() failed: %pe\n", ERR_PTR(ret));
> +		return ret;
>  	}
>  
>  	platform_set_drvdata(pdev, mdp);
>  
> -	/*
> -	 * For MT2701, disable double buffer before writing register
> -	 * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH.
> -	 */
> -	if (!mdp->data->has_commit) {
> -		mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug,
> -					 mdp->data->bls_debug_mask,
> -					 mdp->data->bls_debug_mask);
> -		mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> -					 mdp->data->con0_sel,
> -					 mdp->data->con0_sel);
> -	}
> -

Similar than above this is unrelated at least to what you described in
the commit log. Maybe this part is better split into a separate patch?


>  	return 0;
> -
> -disable_clk_mm:
> -	clk_unprepare(mdp->clk_mm);
> -disable_clk_main:
> -	clk_unprepare(mdp->clk_main);
> -	return ret;
>  }

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v5 2/3] pwm: mtk_disp: implement atomic API .apply()

[Uwe Kleine-König]

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

On Wed, Jun 16, 2021 at 04:52:23PM +0800, Jitao Shi wrote:
> Switch the driver to support the .apply() method.
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
>  drivers/pwm/pwm-mtk-disp.c | 69 ++++++++++----------------------------
>  1 file changed, 17 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 204d76beeb26..a4766e931b68 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -67,14 +67,25 @@ static void mtk_disp_pwm_update_bits(struct mtk_disp_pwm *mdp, u32 offset,
>  	writel(value, address);
>  }
>  
> -static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			       int duty_ns, int period_ns)
> +static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
>  {
>  	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
>  	u32 clk_div, period, high_width, value;
>  	u64 div, rate;
>  	int err;
>  

Here something like:

	if (state->polarity != PWM_POLARITY_NORMAL)
		return -EINVAL;

is missing.

> +	if (!state->enabled) {
> +		mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask,
> +					 0x0);
> +		if (mdp->enabled) {
> +			clk_disable_unprepare(mdp->clk_mm);
> +			clk_disable_unprepare(mdp->clk_main);
> +		}
> +		mdp->enabled = false;
> +		return 0;
> +	}
> +
>  	if (!mdp->enabled) {
>  		err = clk_prepare_enable(mdp->clk_main);
>  		if (err < 0) {
> @@ -102,7 +113,7 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
>  	 */
>  	rate = clk_get_rate(mdp->clk_main);
> -	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> +	clk_div = div_u64(rate * state->period, NSEC_PER_SEC) >>

This multiplication might overflow. But maybe it's sensible to address
this in a separate commit to keep things simple.

>  			  PWM_PERIOD_BIT_WIDTH;
>  	if (clk_div > PWM_CLKDIV_MAX) {
>  		if (!mdp->enabled) {
> @@ -112,11 +123,11 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return -EINVAL;
>  	}
>  	div = NSEC_PER_SEC * (clk_div + 1);
> -	period = div64_u64(rate * period_ns, div);
> +	period = div64_u64(rate * state->period, div);

ditto.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH v5 3/3] pwm: mtk_disp: implement atomic API .get_state()

[Uwe Kleine-König]

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

On Wed, Jun 16, 2021 at 04:52:24PM +0800, Jitao Shi wrote:
> Switch the driver to support the .get_state() method.
> 
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
>  drivers/pwm/pwm-mtk-disp.c | 39 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index a4766e931b68..d60a6b379683 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -159,8 +159,47 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;
>  }
>  
> +static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> +	u32 clk_div, con0, con1;
> +	u64 rate, period, high_width;
> +	int err;
> +
> +	if (!mdp->enabled) {
> +		err = clk_prepare_enable(mdp->clk_main);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_main: %pe\n", ERR_PTR(err));
> +			return;
> +		}
> +		err = clk_prepare_enable(mdp->clk_mm);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_mm: %pe\n", ERR_PTR(err));
> +			clk_disable_unprepare(mdp->clk_main);
> +			return;
> +		}
> +	}

As already written in reply to one of the previous patches: The
clk_prepare_enable() can be done unconditionally.

> +	rate = clk_get_rate(mdp->clk_main);
> +	con0 = readl(mdp->base + mdp->data->con0);
> +	con1 = readl(mdp->base + mdp->data->con1);
> +	state->enabled = !!(con0 & BIT(0));

Maybe introduce a name for that.

> +	clk_div = FIELD_GET(PWM_CLKDIV_MASK, con0);
> +	period = con1 & PWM_PERIOD_MASK;

FIELD_GET(PWM_PERIOD_MASK, con1) for consistency.

> +	state->period = DIV64_U64_ROUND_UP(period * (clk_div + 1) * NSEC_PER_SEC, rate);

period has 12 bits, clk_div 11 and NSEC_PER_SEC has 30, so this cannot
overflow. Maybe mention this in a comment.

> +	high_width = FIELD_GET(PWM_HIGH_WIDTH_MASK, con1);
> +	state->duty_cycle = DIV64_U64_ROUND_UP(high_width * (clk_div + 1) * NSEC_PER_SEC,
> +					       rate);

state->polarity = PWM_POLARITY_NORMAL;

> +	if (!mdp->enabled) {
> +		clk_disable_unprepare(mdp->clk_mm);
> +		clk_disable_unprepare(mdp->clk_main);
> +	}
> +}
> +
>  static const struct pwm_ops mtk_disp_pwm_ops = {
>  	.apply = mtk_disp_pwm_apply,
> +	.get_state = mtk_disp_pwm_get_state,
>  	.owner = THIS_MODULE,
>  };

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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