[PATCH RESEND 0/4] mediatek: pwm driver add MT2712/MT7622 support

[PATCH RESEND 0/4] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

pwm driver add MT2712/MT7622 support, fix clock control issue.
dtsi file will be upload latter, after the clock define is ready.

Zhi Mao (4):
  pwm: kconfig: modify mediatek information
  pwm: mediatek: fix clk issue
  pwm: bindings: add MT2712/MT7622 information
  pwm: mediatek: add MT2712/MT7622 support

 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
 drivers/pwm/Kconfig                                |    2 +-
 drivers/pwm/pwm-mediatek.c                         |  132 ++++++++++++++------
 3 files changed, 100 insertions(+), 40 deletions(-)

[PATCH RESEND 0/4] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

pwm driver add MT2712/MT7622 support, fix clock control issue.
dtsi file will be upload latter, after the clock define is ready.

Zhi Mao (4):
  pwm: kconfig: modify mediatek information
  pwm: mediatek: fix clk issue
  pwm: bindings: add MT2712/MT7622 information
  pwm: mediatek: add MT2712/MT7622 support

 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
 drivers/pwm/Kconfig                                |    2 +-
 drivers/pwm/pwm-mediatek.c                         |  132 ++++++++++++++------
 3 files changed, 100 insertions(+), 40 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

[PATCH RESEND 0/4] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

pwm driver add MT2712/MT7622 support, fix clock control issue.
dtsi file will be upload latter, after the clock define is ready.

Zhi Mao (4):
  pwm: kconfig: modify mediatek information
  pwm: mediatek: fix clk issue
  pwm: bindings: add MT2712/MT7622 information
  pwm: mediatek: add MT2712/MT7622 support

 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
 drivers/pwm/Kconfig                                |    2 +-
 drivers/pwm/pwm-mediatek.c                         |  132 ++++++++++++++------
 3 files changed, 100 insertions(+), 40 deletions(-)

[PATCH RESEND 1/4] pwm: kconfig: modify mediatek information

[Zhi Mao]

modify mediatek information

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 313c107..45cdf2a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -300,7 +300,7 @@ config PWM_MEDIATEK
 	  Generic PWM framework driver for Mediatek ARM SoC.
 
 	  To compile this driver as a module, choose M here: the module
-	  will be called pwm-mxs.
+	  will be called pwm-mediatek.
 
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
-- 
1.7.9.5

[PATCH RESEND 1/4] pwm: kconfig: modify mediatek information

[Zhi Mao]

modify mediatek information

Signed-off-by: Zhi Mao <zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 drivers/pwm/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 313c107..45cdf2a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -300,7 +300,7 @@ config PWM_MEDIATEK
 	  Generic PWM framework driver for Mediatek ARM SoC.
 
 	  To compile this driver as a module, choose M here: the module
-	  will be called pwm-mxs.
+	  will be called pwm-mediatek.
 
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
-- 
1.7.9.5

[PATCH RESEND 1/4] pwm: kconfig: modify mediatek information

[Zhi Mao]

modify mediatek information

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 313c107..45cdf2a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -300,7 +300,7 @@ config PWM_MEDIATEK
 	  Generic PWM framework driver for Mediatek ARM SoC.
 
 	  To compile this driver as a module, choose M here: the module
-	  will be called pwm-mxs.
+	  will be called pwm-mediatek.
 
 config PWM_MXS
 	tristate "Freescale MXS PWM support"
-- 
1.7.9.5

[PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[Zhi Mao]

1.fix clock control
 - prepare top/main clk in mtk_pwm_probe() function,
   it will increase power consumption
   and in original code these clocks is only prepeare but never enabled
 - pwm clock should be enabled before setting pwm registers
   in function: mtk_pwm_config()

2.fix pwm source clock selection
 - in original code, the pwm output frequency is not correct
   when set bit<3>=1 to PWMCON register.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c |   69 +++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..c803ff6 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -2,6 +2,7 @@
  * Mediatek Pulse Width Modulator driver
  *
  * Copyright (C) 2015 John Crispin <blogic@openwrt.org>
+ * Copyright (C) 2017 Zhi Mao <zhi.mao@mediatek.com>
  *
  * This file is licensed under the terms of the GNU General Public
  * License version 2. This program is licensed "as is" without any
@@ -61,6 +62,40 @@ static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct mtk_pwm_chip, chip);
 }
 
+static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+	int ret = 0;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+
+	return ret;
+}
+
+static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+
+	clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+}
+
 static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
 				unsigned int offset)
 {
@@ -81,6 +116,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
 	u32 resolution, clkdiv = 0;
 
+	mtk_pwm_clk_enable(chip, pwm);
+
 	resolution = NSEC_PER_SEC / clk_get_rate(clk);
 
 	while (period_ns / resolution > 8191) {
@@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (clkdiv > 7)
 		return -EINVAL;
 
-	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
+	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
 
+	mtk_pwm_clk_disable(chip, pwm);
+
 	return 0;
 }
 
@@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
 	u32 value;
-	int ret;
 
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
-	if (ret < 0)
-		return ret;
+	mtk_pwm_clk_enable(chip, pwm);
 
 	value = readl(pc->regs);
 	value |= BIT(pwm->hwpwm);
@@ -124,7 +160,7 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	value &= ~BIT(pwm->hwpwm);
 	writel(value, pc->regs);
 
-	clk_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	mtk_pwm_clk_disable(chip, pwm);
 }
 
 static const struct pwm_ops mtk_pwm_ops = {
@@ -156,14 +192,6 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 			return PTR_ERR(pc->clks[i]);
 	}
 
-	ret = clk_prepare(pc->clks[MTK_CLK_TOP]);
-	if (ret < 0)
-		return ret;
-
-	ret = clk_prepare(pc->clks[MTK_CLK_MAIN]);
-	if (ret < 0)
-		goto disable_clk_top;
-
 	platform_set_drvdata(pdev, pc);
 
 	pc->chip.dev = &pdev->dev;
@@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		goto disable_clk_main;
+		return ret;
 	}
 
 	return 0;
-
-disable_clk_main:
-	clk_unprepare(pc->clks[MTK_CLK_MAIN]);
-disable_clk_top:
-	clk_unprepare(pc->clks[MTK_CLK_TOP]);
-
-	return ret;
 }
 
 static int mtk_pwm_remove(struct platform_device *pdev)
 {
 	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
-	unsigned int i;
-
-	for (i = 0; i < pc->chip.npwm; i++)
-		pwm_disable(&pc->chip.pwms[i]);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
1.7.9.5

[PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[Zhi Mao]

1.fix clock control
 - prepare top/main clk in mtk_pwm_probe() function,
   it will increase power consumption
   and in original code these clocks is only prepeare but never enabled
 - pwm clock should be enabled before setting pwm registers
   in function: mtk_pwm_config()

2.fix pwm source clock selection
 - in original code, the pwm output frequency is not correct
   when set bit<3>=1 to PWMCON register.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c |   69 +++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..c803ff6 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -2,6 +2,7 @@
  * Mediatek Pulse Width Modulator driver
  *
  * Copyright (C) 2015 John Crispin <blogic@openwrt.org>
+ * Copyright (C) 2017 Zhi Mao <zhi.mao@mediatek.com>
  *
  * This file is licensed under the terms of the GNU General Public
  * License version 2. This program is licensed "as is" without any
@@ -61,6 +62,40 @@ static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct mtk_pwm_chip, chip);
 }
 
+static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+	int ret = 0;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+
+	return ret;
+}
+
+static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+
+	clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+}
+
 static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
 				unsigned int offset)
 {
@@ -81,6 +116,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
 	u32 resolution, clkdiv = 0;
 
+	mtk_pwm_clk_enable(chip, pwm);
+
 	resolution = NSEC_PER_SEC / clk_get_rate(clk);
 
 	while (period_ns / resolution > 8191) {
@@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (clkdiv > 7)
 		return -EINVAL;
 
-	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
+	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
 
+	mtk_pwm_clk_disable(chip, pwm);
+
 	return 0;
 }
 
@@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
 	u32 value;
-	int ret;
 
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
-	if (ret < 0)
-		return ret;
+	mtk_pwm_clk_enable(chip, pwm);
 
 	value = readl(pc->regs);
 	value |= BIT(pwm->hwpwm);
@@ -124,7 +160,7 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	value &= ~BIT(pwm->hwpwm);
 	writel(value, pc->regs);
 
-	clk_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	mtk_pwm_clk_disable(chip, pwm);
 }
 
 static const struct pwm_ops mtk_pwm_ops = {
@@ -156,14 +192,6 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 			return PTR_ERR(pc->clks[i]);
 	}
 
-	ret = clk_prepare(pc->clks[MTK_CLK_TOP]);
-	if (ret < 0)
-		return ret;
-
-	ret = clk_prepare(pc->clks[MTK_CLK_MAIN]);
-	if (ret < 0)
-		goto disable_clk_top;
-
 	platform_set_drvdata(pdev, pc);
 
 	pc->chip.dev = &pdev->dev;
@@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		goto disable_clk_main;
+		return ret;
 	}
 
 	return 0;
-
-disable_clk_main:
-	clk_unprepare(pc->clks[MTK_CLK_MAIN]);
-disable_clk_top:
-	clk_unprepare(pc->clks[MTK_CLK_TOP]);
-
-	return ret;
 }
 
 static int mtk_pwm_remove(struct platform_device *pdev)
 {
 	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
-	unsigned int i;
-
-	for (i = 0; i < pc->chip.npwm; i++)
-		pwm_disable(&pc->chip.pwms[i]);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
1.7.9.5

[PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[Zhi Mao]

1.fix clock control
 - prepare top/main clk in mtk_pwm_probe() function,
   it will increase power consumption
   and in original code these clocks is only prepeare but never enabled
 - pwm clock should be enabled before setting pwm registers
   in function: mtk_pwm_config()

2.fix pwm source clock selection
 - in original code, the pwm output frequency is not correct
   when set bit<3>=1 to PWMCON register.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c |   69 +++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..c803ff6 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -2,6 +2,7 @@
  * Mediatek Pulse Width Modulator driver
  *
  * Copyright (C) 2015 John Crispin <blogic@openwrt.org>
+ * Copyright (C) 2017 Zhi Mao <zhi.mao@mediatek.com>
  *
  * This file is licensed under the terms of the GNU General Public
  * License version 2. This program is licensed "as is" without any
@@ -61,6 +62,40 @@ static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct mtk_pwm_chip, chip);
 }
 
+static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+	int ret = 0;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
+	if (ret < 0)
+		return ret;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+
+	return ret;
+}
+
+static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
+
+	clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+}
+
 static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
 				unsigned int offset)
 {
@@ -81,6 +116,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
 	u32 resolution, clkdiv = 0;
 
+	mtk_pwm_clk_enable(chip, pwm);
+
 	resolution = NSEC_PER_SEC / clk_get_rate(clk);
 
 	while (period_ns / resolution > 8191) {
@@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (clkdiv > 7)
 		return -EINVAL;
 
-	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
+	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
 
+	mtk_pwm_clk_disable(chip, pwm);
+
 	return 0;
 }
 
@@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
 	u32 value;
-	int ret;
 
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
-	if (ret < 0)
-		return ret;
+	mtk_pwm_clk_enable(chip, pwm);
 
 	value = readl(pc->regs);
 	value |= BIT(pwm->hwpwm);
@@ -124,7 +160,7 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	value &= ~BIT(pwm->hwpwm);
 	writel(value, pc->regs);
 
-	clk_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	mtk_pwm_clk_disable(chip, pwm);
 }
 
 static const struct pwm_ops mtk_pwm_ops = {
@@ -156,14 +192,6 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 			return PTR_ERR(pc->clks[i]);
 	}
 
-	ret = clk_prepare(pc->clks[MTK_CLK_TOP]);
-	if (ret < 0)
-		return ret;
-
-	ret = clk_prepare(pc->clks[MTK_CLK_MAIN]);
-	if (ret < 0)
-		goto disable_clk_top;
-
 	platform_set_drvdata(pdev, pc);
 
 	pc->chip.dev = &pdev->dev;
@@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		goto disable_clk_main;
+		return ret;
 	}
 
 	return 0;
-
-disable_clk_main:
-	clk_unprepare(pc->clks[MTK_CLK_MAIN]);
-disable_clk_top:
-	clk_unprepare(pc->clks[MTK_CLK_TOP]);
-
-	return ret;
 }
 
 static int mtk_pwm_remove(struct platform_device *pdev)
 {
 	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
-	unsigned int i;
-
-	for (i = 0; i < pc->chip.npwm; i++)
-		pwm_disable(&pc->chip.pwms[i]);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
1.7.9.5

[PATCH RESEND 3/4] pwm: bindings: add MT2712/MT7622 information

[Zhi Mao]

add MT2712/MT7622 pwm information

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 54c59b0..ef8bd3c 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -2,6 +2,8 @@ MediaTek PWM controller
 
 Required properties:
  - compatible: should be "mediatek,<name>-pwm":
+   - "mediatek,mt2712-pwm": found on mt2712 SoC.
+   - "mediatek,mt7622-pwm": found on mt7622 SoC.
    - "mediatek,mt7623-pwm": found on mt7623 SoC.
  - reg: physical base address and length of the controller's registers.
  - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
@@ -10,7 +12,9 @@ Required properties:
  - clock-names: must contain the following:
    - "top": the top clock generator
    - "main": clock used by the PWM core
-   - "pwm1-5": the five per PWM clocks
+   - "pwm1-8": the eight per PWM clocks for mt2712
+   - "pwm1-6": the six per PWM clocks for mt7622
+   - "pwm1-5": the five per PWM clocks for mt7623
  - pinctrl-names: Must contain a "default" entry.
  - pinctrl-0: One property must exist for each entry in pinctrl-names.
    See pinctrl/pinctrl-bindings.txt for details of the property values.
-- 
1.7.9.5

[PATCH RESEND 3/4] pwm: bindings: add MT2712/MT7622 information

[Zhi Mao]

add MT2712/MT7622 pwm information

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 54c59b0..ef8bd3c 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -2,6 +2,8 @@ MediaTek PWM controller
 
 Required properties:
  - compatible: should be "mediatek,<name>-pwm":
+   - "mediatek,mt2712-pwm": found on mt2712 SoC.
+   - "mediatek,mt7622-pwm": found on mt7622 SoC.
    - "mediatek,mt7623-pwm": found on mt7623 SoC.
  - reg: physical base address and length of the controller's registers.
  - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
@@ -10,7 +12,9 @@ Required properties:
  - clock-names: must contain the following:
    - "top": the top clock generator
    - "main": clock used by the PWM core
-   - "pwm1-5": the five per PWM clocks
+   - "pwm1-8": the eight per PWM clocks for mt2712
+   - "pwm1-6": the six per PWM clocks for mt7622
+   - "pwm1-5": the five per PWM clocks for mt7623
  - pinctrl-names: Must contain a "default" entry.
  - pinctrl-0: One property must exist for each entry in pinctrl-names.
    See pinctrl/pinctrl-bindings.txt for details of the property values.
-- 
1.7.9.5

[PATCH RESEND 3/4] pwm: bindings: add MT2712/MT7622 information

[Zhi Mao]

add MT2712/MT7622 pwm information

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
index 54c59b0..ef8bd3c 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt
@@ -2,6 +2,8 @@ MediaTek PWM controller
 
 Required properties:
  - compatible: should be "mediatek,<name>-pwm":
+   - "mediatek,mt2712-pwm": found on mt2712 SoC.
+   - "mediatek,mt7622-pwm": found on mt7622 SoC.
    - "mediatek,mt7623-pwm": found on mt7623 SoC.
  - reg: physical base address and length of the controller's registers.
  - #pwm-cells: must be 2. See pwm.txt in this directory for a description of
@@ -10,7 +12,9 @@ Required properties:
  - clock-names: must contain the following:
    - "top": the top clock generator
    - "main": clock used by the PWM core
-   - "pwm1-5": the five per PWM clocks
+   - "pwm1-8": the eight per PWM clocks for mt2712
+   - "pwm1-6": the six per PWM clocks for mt7622
+   - "pwm1-5": the five per PWM clocks for mt7623
  - pinctrl-names: Must contain a "default" entry.
  - pinctrl-0: One property must exist for each entry in pinctrl-names.
    See pinctrl/pinctrl-bindings.txt for details of the property values.
-- 
1.7.9.5

[PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

support multiple chip(MT2712, MT7622, MT7623)

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index c803ff6..d520356 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
@@ -30,6 +31,8 @@
 #define PWMDWIDTH		0x2c
 #define PWMTHRES		0x30
 
+#define PWM_CLK_DIV_MAX		7
+
 enum {
 	MTK_CLK_MAIN = 0,
 	MTK_CLK_TOP,
@@ -38,11 +41,19 @@ enum {
 	MTK_CLK_PWM3,
 	MTK_CLK_PWM4,
 	MTK_CLK_PWM5,
+	MTK_CLK_PWM6,
+	MTK_CLK_PWM7,
+	MTK_CLK_PWM8,
 	MTK_CLK_MAX,
 };
 
-static const char * const mtk_pwm_clk_name[] = {
-	"main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5"
+static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = {
+	"main", "top", "pwm1", "pwm2", "pwm3", "pwm4",
+	"pwm5", "pwm6", "pwm7", "pwm8"
+};
+
+struct mtk_com_pwm_data {
+	unsigned int pwm_nums;
 };
 
 /**
@@ -55,6 +66,11 @@ struct mtk_pwm_chip {
 	struct pwm_chip chip;
 	void __iomem *regs;
 	struct clk *clks[MTK_CLK_MAX];
+	const struct mtk_com_pwm_data *data;
+};
+
+static const unsigned long mtk_pwm_com_reg[] = {
+	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
 };
 
 static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
@@ -99,14 +115,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
 				unsigned int offset)
 {
-	return readl(chip->regs + 0x10 + (num * 0x40) + offset);
+	return readl(chip->regs + mtk_pwm_com_reg[num] + offset);
 }
 
 static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip,
 				  unsigned int num, unsigned int offset,
 				  u32 value)
 {
-	writel(value, chip->regs + 0x10 + (num * 0x40) + offset);
+	writel(value, chip->regs + mtk_pwm_com_reg[num] + offset);
 }
 
 static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -125,8 +141,10 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv++;
 	}
 
-	if (clkdiv > 7)
+	if (clkdiv > PWM_CLK_DIV_MAX) {
+		dev_err(chip->dev, "period %d not supported\n", period_ns);
 		return -EINVAL;
+	}
 
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
@@ -181,23 +199,28 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	if (!pc)
 		return -ENOMEM;
 
+	pc->data = of_device_get_match_data(&pdev->dev);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pc->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(pc->regs))
 		return PTR_ERR(pc->regs);
 
-	for (i = 0; i < MTK_CLK_MAX; i++) {
+	for (i = 0; i < pc->data->pwm_nums + 2; i++) {
 		pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
-		if (IS_ERR(pc->clks[i]))
+		if (IS_ERR(pc->clks[i])) {
+			dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n",
+				mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
 			return PTR_ERR(pc->clks[i]);
+		}
 	}
 
-	platform_set_drvdata(pdev, pc);
-
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &mtk_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = 5;
+	pc->chip.npwm = pc->data->pwm_nums;
+
+	platform_set_drvdata(pdev, pc);
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
@@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
 	return pwmchip_remove(&pc->chip);
 }
 
+/*==========================================*/
+static const struct mtk_com_pwm_data mt2712_pwm_data = {
+	.pwm_nums = 8,
+};
+
+static const struct mtk_com_pwm_data mt7622_pwm_data = {
+	.pwm_nums = 6,
+};
+
+static const struct mtk_com_pwm_data mt7623_pwm_data = {
+	.pwm_nums = 5,
+};
+/*==========================================*/
+
 static const struct of_device_id mtk_pwm_of_match[] = {
-	{ .compatible = "mediatek,mt7623-pwm" },
-	{ }
+	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
+	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
+	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
+	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
 
-- 
1.7.9.5

[PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

support multiple chip(MT2712, MT7622, MT7623)

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index c803ff6..d520356 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
@@ -30,6 +31,8 @@
 #define PWMDWIDTH		0x2c
 #define PWMTHRES		0x30
 
+#define PWM_CLK_DIV_MAX		7
+
 enum {
 	MTK_CLK_MAIN = 0,
 	MTK_CLK_TOP,
@@ -38,11 +41,19 @@ enum {
 	MTK_CLK_PWM3,
 	MTK_CLK_PWM4,
 	MTK_CLK_PWM5,
+	MTK_CLK_PWM6,
+	MTK_CLK_PWM7,
+	MTK_CLK_PWM8,
 	MTK_CLK_MAX,
 };
 
-static const char * const mtk_pwm_clk_name[] = {
-	"main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5"
+static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = {
+	"main", "top", "pwm1", "pwm2", "pwm3", "pwm4",
+	"pwm5", "pwm6", "pwm7", "pwm8"
+};
+
+struct mtk_com_pwm_data {
+	unsigned int pwm_nums;
 };
 
 /**
@@ -55,6 +66,11 @@ struct mtk_pwm_chip {
 	struct pwm_chip chip;
 	void __iomem *regs;
 	struct clk *clks[MTK_CLK_MAX];
+	const struct mtk_com_pwm_data *data;
+};
+
+static const unsigned long mtk_pwm_com_reg[] = {
+	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
 };
 
 static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
@@ -99,14 +115,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
 				unsigned int offset)
 {
-	return readl(chip->regs + 0x10 + (num * 0x40) + offset);
+	return readl(chip->regs + mtk_pwm_com_reg[num] + offset);
 }
 
 static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip,
 				  unsigned int num, unsigned int offset,
 				  u32 value)
 {
-	writel(value, chip->regs + 0x10 + (num * 0x40) + offset);
+	writel(value, chip->regs + mtk_pwm_com_reg[num] + offset);
 }
 
 static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -125,8 +141,10 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv++;
 	}
 
-	if (clkdiv > 7)
+	if (clkdiv > PWM_CLK_DIV_MAX) {
+		dev_err(chip->dev, "period %d not supported\n", period_ns);
 		return -EINVAL;
+	}
 
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
@@ -181,23 +199,28 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	if (!pc)
 		return -ENOMEM;
 
+	pc->data = of_device_get_match_data(&pdev->dev);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pc->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(pc->regs))
 		return PTR_ERR(pc->regs);
 
-	for (i = 0; i < MTK_CLK_MAX; i++) {
+	for (i = 0; i < pc->data->pwm_nums + 2; i++) {
 		pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
-		if (IS_ERR(pc->clks[i]))
+		if (IS_ERR(pc->clks[i])) {
+			dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n",
+				mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
 			return PTR_ERR(pc->clks[i]);
+		}
 	}
 
-	platform_set_drvdata(pdev, pc);
-
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &mtk_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = 5;
+	pc->chip.npwm = pc->data->pwm_nums;
+
+	platform_set_drvdata(pdev, pc);
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
@@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
 	return pwmchip_remove(&pc->chip);
 }
 
+/*==========================================*/
+static const struct mtk_com_pwm_data mt2712_pwm_data = {
+	.pwm_nums = 8,
+};
+
+static const struct mtk_com_pwm_data mt7622_pwm_data = {
+	.pwm_nums = 6,
+};
+
+static const struct mtk_com_pwm_data mt7623_pwm_data = {
+	.pwm_nums = 5,
+};
+/*==========================================*/
+
 static const struct of_device_id mtk_pwm_of_match[] = {
-	{ .compatible = "mediatek,mt7623-pwm" },
-	{ }
+	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
+	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
+	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
+	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
 
-- 
1.7.9.5

[PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

support multiple chip(MT2712, MT7622, MT7623)

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
---
 drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index c803ff6..d520356 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/clk.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/slab.h>
@@ -30,6 +31,8 @@
 #define PWMDWIDTH		0x2c
 #define PWMTHRES		0x30
 
+#define PWM_CLK_DIV_MAX		7
+
 enum {
 	MTK_CLK_MAIN = 0,
 	MTK_CLK_TOP,
@@ -38,11 +41,19 @@ enum {
 	MTK_CLK_PWM3,
 	MTK_CLK_PWM4,
 	MTK_CLK_PWM5,
+	MTK_CLK_PWM6,
+	MTK_CLK_PWM7,
+	MTK_CLK_PWM8,
 	MTK_CLK_MAX,
 };
 
-static const char * const mtk_pwm_clk_name[] = {
-	"main", "top", "pwm1", "pwm2", "pwm3", "pwm4", "pwm5"
+static const char * const mtk_pwm_clk_name[MTK_CLK_MAX] = {
+	"main", "top", "pwm1", "pwm2", "pwm3", "pwm4",
+	"pwm5", "pwm6", "pwm7", "pwm8"
+};
+
+struct mtk_com_pwm_data {
+	unsigned int pwm_nums;
 };
 
 /**
@@ -55,6 +66,11 @@ struct mtk_pwm_chip {
 	struct pwm_chip chip;
 	void __iomem *regs;
 	struct clk *clks[MTK_CLK_MAX];
+	const struct mtk_com_pwm_data *data;
+};
+
+static const unsigned long mtk_pwm_com_reg[] = {
+	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
 };
 
 static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
@@ -99,14 +115,14 @@ static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
 				unsigned int offset)
 {
-	return readl(chip->regs + 0x10 + (num * 0x40) + offset);
+	return readl(chip->regs + mtk_pwm_com_reg[num] + offset);
 }
 
 static inline void mtk_pwm_writel(struct mtk_pwm_chip *chip,
 				  unsigned int num, unsigned int offset,
 				  u32 value)
 {
-	writel(value, chip->regs + 0x10 + (num * 0x40) + offset);
+	writel(value, chip->regs + mtk_pwm_com_reg[num] + offset);
 }
 
 static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -125,8 +141,10 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv++;
 	}
 
-	if (clkdiv > 7)
+	if (clkdiv > PWM_CLK_DIV_MAX) {
+		dev_err(chip->dev, "period %d not supported\n", period_ns);
 		return -EINVAL;
+	}
 
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
 	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
@@ -181,23 +199,28 @@ static int mtk_pwm_probe(struct platform_device *pdev)
 	if (!pc)
 		return -ENOMEM;
 
+	pc->data = of_device_get_match_data(&pdev->dev);
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pc->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(pc->regs))
 		return PTR_ERR(pc->regs);
 
-	for (i = 0; i < MTK_CLK_MAX; i++) {
+	for (i = 0; i < pc->data->pwm_nums + 2; i++) {
 		pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]);
-		if (IS_ERR(pc->clks[i]))
+		if (IS_ERR(pc->clks[i])) {
+			dev_err(&pdev->dev, "[PWM] clock: %s fail: %ld\n",
+				mtk_pwm_clk_name[i], PTR_ERR(pc->clks[i]));
 			return PTR_ERR(pc->clks[i]);
+		}
 	}
 
-	platform_set_drvdata(pdev, pc);
-
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &mtk_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = 5;
+	pc->chip.npwm = pc->data->pwm_nums;
+
+	platform_set_drvdata(pdev, pc);
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
@@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
 	return pwmchip_remove(&pc->chip);
 }
 
+/*==========================================*/
+static const struct mtk_com_pwm_data mt2712_pwm_data = {
+	.pwm_nums = 8,
+};
+
+static const struct mtk_com_pwm_data mt7622_pwm_data = {
+	.pwm_nums = 6,
+};
+
+static const struct mtk_com_pwm_data mt7623_pwm_data = {
+	.pwm_nums = 5,
+};
+/*==========================================*/
+
 static const struct of_device_id mtk_pwm_of_match[] = {
-	{ .compatible = "mediatek,mt7623-pwm" },
-	{ }
+	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
+	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
+	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
+	{},
 };
 MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
 
-- 
1.7.9.5

Re: [PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[John Crispin]

Hi

comments inline


On 21/06/17 10:11, Zhi Mao wrote:
> 1.fix clock control
>   - prepare top/main clk in mtk_pwm_probe() function,
>     it will increase power consumption
>     and in original code these clocks is only prepeare but never enabled
>   - pwm clock should be enabled before setting pwm registers
>     in function: mtk_pwm_config()
>
> 2.fix pwm source clock selection
>   - in original code, the pwm output frequency is not correct
>     when set bit<3>=1 to PWMCON register.
>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>   drivers/pwm/pwm-mediatek.c |   69 +++++++++++++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5c11bc7..c803ff6 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -2,6 +2,7 @@
>    * Mediatek Pulse Width Modulator driver
>    *
>    * Copyright (C) 2015 John Crispin <blogic@openwrt.org>
> + * Copyright (C) 2017 Zhi Mao <zhi.mao@mediatek.com>
>    *
>    * This file is licensed under the terms of the GNU General Public
>    * License version 2. This program is licensed "as is" without any
> @@ -61,6 +62,40 @@ static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
>   	return container_of(chip, struct mtk_pwm_chip, chip);
>   }
>   
> +static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
> +	int ret = 0;
> +
> +	ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
> +	if (ret < 0) {
> +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> +	if (ret < 0) {
> +		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> +		return ret;
> +	}
> +
Rather than disabling the already prepared clks in each error path and 
then returning, you should use goto err_clk_{top,main,pwm1} in the same 
style as what this patch removes from mtk_pwm_probe()

> +	return ret;
> +}
> +
> +static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
> +
> +	clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> +	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> +	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> +}
> +
>   static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
>   				unsigned int offset)
>   {
> @@ -81,6 +116,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
>   	u32 resolution, clkdiv = 0;
>   
> +	mtk_pwm_clk_enable(chip, pwm);
> +
>   	resolution = NSEC_PER_SEC / clk_get_rate(clk);
>   
>   	while (period_ns / resolution > 8191) {
> @@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	if (clkdiv > 7)
>   		return -EINVAL;
>   
> -	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> +	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);

this chunk needs to go into its own patch

>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
>   
> +	mtk_pwm_clk_disable(chip, pwm);
> +
>   	return 0;
>   }
>   
> @@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
>   	u32 value;
> -	int ret;
>   
> -	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> -	if (ret < 0)
> -		return ret;
> +	mtk_pwm_clk_enable(chip, pwm);
You need to check the error code here and return if clk enabling failed

>   
>   	value = readl(pc->regs);
>   	value |= BIT(pwm->hwpwm);
> @@ -124,7 +160,7 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>   	value &= ~BIT(pwm->hwpwm);
>   	writel(value, pc->regs);
>   
> -	clk_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> +	mtk_pwm_clk_disable(chip, pwm);
>   }
>   
>   static const struct pwm_ops mtk_pwm_ops = {
> @@ -156,14 +192,6 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>   			return PTR_ERR(pc->clks[i]);
>   	}
>   
> -	ret = clk_prepare(pc->clks[MTK_CLK_TOP]);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = clk_prepare(pc->clks[MTK_CLK_MAIN]);
> -	if (ret < 0)
> -		goto disable_clk_top;
> -
>   	platform_set_drvdata(pdev, pc);
>   
>   	pc->chip.dev = &pdev->dev;
> @@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>   	ret = pwmchip_add(&pc->chip);
>   	if (ret < 0) {
>   		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> -		goto disable_clk_main;
> +		return ret;
>   	}
>   
>   	return 0;
> -
> -disable_clk_main:
> -	clk_unprepare(pc->clks[MTK_CLK_MAIN]);
> -disable_clk_top:
> -	clk_unprepare(pc->clks[MTK_CLK_TOP]);
> -
> -	return ret;
>   }
>   
>   static int mtk_pwm_remove(struct platform_device *pdev)
>   {
>   	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
> -	unsigned int i;
> -
> -	for (i = 0; i < pc->chip.npwm; i++)
> -		pwm_disable(&pc->chip.pwms[i]);
why are you removing this chunk ?

     John

>   
>   	return pwmchip_remove(&pc->chip);
>   }

[PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[John Crispin]

Hi

comments inline


On 21/06/17 10:11, Zhi Mao wrote:
> 1.fix clock control
>   - prepare top/main clk in mtk_pwm_probe() function,
>     it will increase power consumption
>     and in original code these clocks is only prepeare but never enabled
>   - pwm clock should be enabled before setting pwm registers
>     in function: mtk_pwm_config()
>
> 2.fix pwm source clock selection
>   - in original code, the pwm output frequency is not correct
>     when set bit<3>=1 to PWMCON register.
>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>   drivers/pwm/pwm-mediatek.c |   69 +++++++++++++++++++++++++++-----------------
>   1 file changed, 43 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5c11bc7..c803ff6 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -2,6 +2,7 @@
>    * Mediatek Pulse Width Modulator driver
>    *
>    * Copyright (C) 2015 John Crispin <blogic@openwrt.org>
> + * Copyright (C) 2017 Zhi Mao <zhi.mao@mediatek.com>
>    *
>    * This file is licensed under the terms of the GNU General Public
>    * License version 2. This program is licensed "as is" without any
> @@ -61,6 +62,40 @@ static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
>   	return container_of(chip, struct mtk_pwm_chip, chip);
>   }
>   
> +static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
> +	int ret = 0;
> +
> +	ret = clk_prepare_enable(pc->clks[MTK_CLK_TOP]);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = clk_prepare_enable(pc->clks[MTK_CLK_MAIN]);
> +	if (ret < 0) {
> +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> +	if (ret < 0) {
> +		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> +		return ret;
> +	}
> +
Rather than disabling the already prepared clks in each error path and 
then returning, you should use goto err_clk_{top,main,pwm1} in the same 
style as what this patch removes from mtk_pwm_probe()

> +	return ret;
> +}
> +
> +static void mtk_pwm_clk_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
> +
> +	clk_disable_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> +	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> +	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> +}
> +
>   static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
>   				unsigned int offset)
>   {
> @@ -81,6 +116,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
>   	u32 resolution, clkdiv = 0;
>   
> +	mtk_pwm_clk_enable(chip, pwm);
> +
>   	resolution = NSEC_PER_SEC / clk_get_rate(clk);
>   
>   	while (period_ns / resolution > 8191) {
> @@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	if (clkdiv > 7)
>   		return -EINVAL;
>   
> -	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> +	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);

this chunk needs to go into its own patch

>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
>   
> +	mtk_pwm_clk_disable(chip, pwm);
> +
>   	return 0;
>   }
>   
> @@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>   {
>   	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
>   	u32 value;
> -	int ret;
>   
> -	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> -	if (ret < 0)
> -		return ret;
> +	mtk_pwm_clk_enable(chip, pwm);
You need to check the error code here and return if clk enabling failed

>   
>   	value = readl(pc->regs);
>   	value |= BIT(pwm->hwpwm);
> @@ -124,7 +160,7 @@ static void mtk_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>   	value &= ~BIT(pwm->hwpwm);
>   	writel(value, pc->regs);
>   
> -	clk_unprepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> +	mtk_pwm_clk_disable(chip, pwm);
>   }
>   
>   static const struct pwm_ops mtk_pwm_ops = {
> @@ -156,14 +192,6 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>   			return PTR_ERR(pc->clks[i]);
>   	}
>   
> -	ret = clk_prepare(pc->clks[MTK_CLK_TOP]);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = clk_prepare(pc->clks[MTK_CLK_MAIN]);
> -	if (ret < 0)
> -		goto disable_clk_top;
> -
>   	platform_set_drvdata(pdev, pc);
>   
>   	pc->chip.dev = &pdev->dev;
> @@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
>   	ret = pwmchip_add(&pc->chip);
>   	if (ret < 0) {
>   		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> -		goto disable_clk_main;
> +		return ret;
>   	}
>   
>   	return 0;
> -
> -disable_clk_main:
> -	clk_unprepare(pc->clks[MTK_CLK_MAIN]);
> -disable_clk_top:
> -	clk_unprepare(pc->clks[MTK_CLK_TOP]);
> -
> -	return ret;
>   }
>   
>   static int mtk_pwm_remove(struct platform_device *pdev)
>   {
>   	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
> -	unsigned int i;
> -
> -	for (i = 0; i < pc->chip.npwm; i++)
> -		pwm_disable(&pc->chip.pwms[i]);
why are you removing this chunk ?

     John

>   
>   	return pwmchip_remove(&pc->chip);
>   }

Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 21/06/17 10:11, Zhi Mao wrote:
> support multiple chip(MT2712, MT7622, MT7623)
This patch does more than add extra SoC support. It also
* adds PWM_CLK_DIV_MAX which really should go into its own patch
* adds mtk_pwm_com_reg which should also go into its own patch

more comments inline

>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index c803ff6..d520356 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -16,6 +16,7 @@
>   #include <linux/module.h>
>   #include <linux/clk.h>
>   #include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <linux/pwm.h>
>   #include <linux/slab.h>
[...]
> @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>   	return pwmchip_remove(&pc->chip);
>   }
>   
> +/*==========================================*/

please remove these comment lines

     John
> +static const struct mtk_com_pwm_data mt2712_pwm_data = {
> +	.pwm_nums = 8,
> +};
> +
> +static const struct mtk_com_pwm_data mt7622_pwm_data = {
> +	.pwm_nums = 6,
> +};
> +
> +static const struct mtk_com_pwm_data mt7623_pwm_data = {
> +	.pwm_nums = 5,
> +};
> +/*==========================================*/
> +
>   static const struct of_device_id mtk_pwm_of_match[] = {
> -	{ .compatible = "mediatek,mt7623-pwm" },
> -	{ }
> +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
> +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
> +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
> +	{},
>   };
>   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>   

[PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 21/06/17 10:11, Zhi Mao wrote:
> support multiple chip(MT2712, MT7622, MT7623)
This patch does more than add extra SoC support. It also
* adds PWM_CLK_DIV_MAX which really should go into its own patch
* adds mtk_pwm_com_reg which should also go into its own patch

more comments inline

>
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>   1 file changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index c803ff6..d520356 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -16,6 +16,7 @@
>   #include <linux/module.h>
>   #include <linux/clk.h>
>   #include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/platform_device.h>
>   #include <linux/pwm.h>
>   #include <linux/slab.h>
[...]
> @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>   	return pwmchip_remove(&pc->chip);
>   }
>   
> +/*==========================================*/

please remove these comment lines

     John
> +static const struct mtk_com_pwm_data mt2712_pwm_data = {
> +	.pwm_nums = 8,
> +};
> +
> +static const struct mtk_com_pwm_data mt7622_pwm_data = {
> +	.pwm_nums = 6,
> +};
> +
> +static const struct mtk_com_pwm_data mt7623_pwm_data = {
> +	.pwm_nums = 5,
> +};
> +/*==========================================*/
> +
>   static const struct of_device_id mtk_pwm_of_match[] = {
> -	{ .compatible = "mediatek,mt7623-pwm" },
> -	{ }
> +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
> +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
> +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
> +	{},
>   };
>   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>   

Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

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

Hi John,

Thanks for your review the code and feedback.
There are 3 issues in this patch:
1.adds PWM_CLK_DIV_MAX which really should go into its own patch
2.adds mtk_pwm_com_reg which should also go into its own patch
3.remove comments inline /*===*/

for #1 and #3, I will modify them in the next release.
but for #2, I want to discuss with you, 
adding the structure  "mtk_pwm_com_reg" is only for the registers of
MT2712 PWM8, 
so we want to keep this modification in this patch.

what's your opinion about it? 
Any reply is welcome.


Regards
Zhi





On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:

> 
> On 21/06/17 10:11, Zhi Mao wrote:
> > support multiple chip(MT2712, MT7622, MT7623)
> This patch does more than add extra SoC support. It also
> * adds PWM_CLK_DIV_MAX which really should go into its own patch
> * adds mtk_pwm_com_reg which should also go into its own patch
> 
> more comments inline
> 
> >
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
> >   1 file changed, 51 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index c803ff6..d520356 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -16,6 +16,7 @@
> >   #include <linux/module.h>
> >   #include <linux/clk.h>
> >   #include <linux/of.h>
> > +#include <linux/of_device.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/pwm.h>
> >   #include <linux/slab.h>
> [...]
> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
> >   	return pwmchip_remove(&pc->chip);
> >   }
> >   
> > +/*==========================================*/
> 
> please remove these comment lines
> 
>      John
> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
> > +	.pwm_nums = 8,
> > +};
> > +
> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
> > +	.pwm_nums = 6,
> > +};
> > +
> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
> > +	.pwm_nums = 5,
> > +};
> > +/*==========================================*/
> > +
> >   static const struct of_device_id mtk_pwm_of_match[] = {
> > -	{ .compatible = "mediatek,mt7623-pwm" },
> > -	{ }
> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
> > +	{},
> >   };
> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
> >   
> 



[-- Attachment #2: Type: text/html, Size: 3119 bytes --]

Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 22/06/17 08:09, Zhi Mao wrote:
> Hi John,
>
> Thanks for your review the code and feedback.
> There are 3 issues in this patch:
> 1.adds PWM_CLK_DIV_MAX which really should go into its own patch
> 2.adds mtk_pwm_com_reg which should also go into its own patch
> 3.remove comments inline /*===*/
>
> for #1 and #3, I will modify them in the next release.
> but for #2, I want to discuss with you,
> adding the structure  "mtk_pwm_com_reg" is only for the registers of 
> MT2712 PWM8,
> so we want to keep this modification in this patch.
>
> what's your opinion about it?
> Any reply is welcome.
>
>
> Regards
> Zhi
>
>
>
>

Hi Zhi,

I just had another look and noticed that the CON registers are not at a 
fixed offset of 0x40 for the new pwm8 register so having 2) inside this 
patch makes sense. please explain in the description that this is the case

     John

>
> On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:
>> On 21/06/17 10:11, Zhi Mao wrote:
>> > support multiple chip(MT2712, MT7622, MT7623)
>> This patch does more than add extra SoC support. It also
>> * adds PWM_CLK_DIV_MAX which really should go into its own patch
>> * adds mtk_pwm_com_reg which should also go into its own patch
>>
>> more comments inline
>>
>> >
>> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com <mailto:zhi.mao@mediatek.com>>
>> > ---
>> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>> >   1 file changed, 51 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>> > index c803ff6..d520356 100644
>> > --- a/drivers/pwm/pwm-mediatek.c
>> > +++ b/drivers/pwm/pwm-mediatek.c
>> > @@ -16,6 +16,7 @@
>> >   #include <linux/module.h>
>> >   #include <linux/clk.h>
>> >   #include <linux/of.h>
>> > +#include <linux/of_device.h>
>> >   #include <linux/platform_device.h>
>> >   #include <linux/pwm.h>
>> >   #include <linux/slab.h>
>> [...]
>> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>> >   	return pwmchip_remove(&pc->chip);
>> >   }
>> >
>> > +/*==========================================*/
>>
>> please remove these comment lines
>>
>>       John
>> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
>> > +	.pwm_nums = 8,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
>> > +	.pwm_nums = 6,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
>> > +	.pwm_nums = 5,
>> > +};
>> > +/*==========================================*/
>> > +
>> >   static const struct of_device_id mtk_pwm_of_match[] = {
>> > -	{ .compatible = "mediatek,mt7623-pwm" },
>> > -	{ }
>> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
>> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
>> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
>> > +	{},
>> >   };
>> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>> >
>>
>

Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 22/06/17 08:09, Zhi Mao wrote:
> Hi John,
>
> Thanks for your review the code and feedback.
> There are 3 issues in this patch:
> 1.adds PWM_CLK_DIV_MAX which really should go into its own patch
> 2.adds mtk_pwm_com_reg which should also go into its own patch
> 3.remove comments inline /*===*/
>
> for #1 and #3, I will modify them in the next release.
> but for #2, I want to discuss with you,
> adding the structure  "mtk_pwm_com_reg" is only for the registers of 
> MT2712 PWM8,
> so we want to keep this modification in this patch.
>
> what's your opinion about it?
> Any reply is welcome.
>
>
> Regards
> Zhi
>
>
>
>

Hi Zhi,

I just had another look and noticed that the CON registers are not at a 
fixed offset of 0x40 for the new pwm8 register so having 2) inside this 
patch makes sense. please explain in the description that this is the case

     John

>
> On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:
>> On 21/06/17 10:11, Zhi Mao wrote:
>> > support multiple chip(MT2712, MT7622, MT7623)
>> This patch does more than add extra SoC support. It also
>> * adds PWM_CLK_DIV_MAX which really should go into its own patch
>> * adds mtk_pwm_com_reg which should also go into its own patch
>>
>> more comments inline
>>
>> >
>> > Signed-off-by: Zhi Mao <zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org <mailto:zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>>
>> > ---
>> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>> >   1 file changed, 51 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>> > index c803ff6..d520356 100644
>> > --- a/drivers/pwm/pwm-mediatek.c
>> > +++ b/drivers/pwm/pwm-mediatek.c
>> > @@ -16,6 +16,7 @@
>> >   #include <linux/module.h>
>> >   #include <linux/clk.h>
>> >   #include <linux/of.h>
>> > +#include <linux/of_device.h>
>> >   #include <linux/platform_device.h>
>> >   #include <linux/pwm.h>
>> >   #include <linux/slab.h>
>> [...]
>> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>> >   	return pwmchip_remove(&pc->chip);
>> >   }
>> >
>> > +/*==========================================*/
>>
>> please remove these comment lines
>>
>>       John
>> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
>> > +	.pwm_nums = 8,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
>> > +	.pwm_nums = 6,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
>> > +	.pwm_nums = 5,
>> > +};
>> > +/*==========================================*/
>> > +
>> >   static const struct of_device_id mtk_pwm_of_match[] = {
>> > -	{ .compatible = "mediatek,mt7623-pwm" },
>> > -	{ }
>> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
>> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
>> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
>> > +	{},
>> >   };
>> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>> >
>>
>

--
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 RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 22/06/17 08:09, Zhi Mao wrote:
> Hi John,
>
> Thanks for your review the code and feedback.
> There are 3 issues in this patch:
> 1.adds PWM_CLK_DIV_MAX which really should go into its own patch
> 2.adds mtk_pwm_com_reg which should also go into its own patch
> 3.remove comments inline /*===*/
>
> for #1 and #3, I will modify them in the next release.
> but for #2, I want to discuss with you,
> adding the structure  "mtk_pwm_com_reg" is only for the registers of 
> MT2712 PWM8,
> so we want to keep this modification in this patch.
>
> what's your opinion about it?
> Any reply is welcome.
>
>
> Regards
> Zhi
>
>
>
>

Hi Zhi,

I just had another look and noticed that the CON registers are not at a 
fixed offset of 0x40 for the new pwm8 register so having 2) inside this 
patch makes sense. please explain in the description that this is the case

     John

>
> On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:
>> On 21/06/17 10:11, Zhi Mao wrote:
>> > support multiple chip(MT2712, MT7622, MT7623)
>> This patch does more than add extra SoC support. It also
>> * adds PWM_CLK_DIV_MAX which really should go into its own patch
>> * adds mtk_pwm_com_reg which should also go into its own patch
>>
>> more comments inline
>>
>> >
>> > Signed-off-by: Zhi Mao <zhi.mao at mediatek.com <mailto:zhi.mao@mediatek.com>>
>> > ---
>> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>> >   1 file changed, 51 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>> > index c803ff6..d520356 100644
>> > --- a/drivers/pwm/pwm-mediatek.c
>> > +++ b/drivers/pwm/pwm-mediatek.c
>> > @@ -16,6 +16,7 @@
>> >   #include <linux/module.h>
>> >   #include <linux/clk.h>
>> >   #include <linux/of.h>
>> > +#include <linux/of_device.h>
>> >   #include <linux/platform_device.h>
>> >   #include <linux/pwm.h>
>> >   #include <linux/slab.h>
>> [...]
>> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>> >   	return pwmchip_remove(&pc->chip);
>> >   }
>> >
>> > +/*==========================================*/
>>
>> please remove these comment lines
>>
>>       John
>> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
>> > +	.pwm_nums = 8,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
>> > +	.pwm_nums = 6,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
>> > +	.pwm_nums = 5,
>> > +};
>> > +/*==========================================*/
>> > +
>> >   static const struct of_device_id mtk_pwm_of_match[] = {
>> > -	{ .compatible = "mediatek,mt7623-pwm" },
>> > -	{ }
>> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
>> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
>> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
>> > +	{},
>> >   };
>> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>> >
>>
>

Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 22/06/17 08:09, Zhi Mao wrote:
> Hi John,
>
> Thanks for your review the code and feedback.
> There are 3 issues in this patch:
> 1.adds PWM_CLK_DIV_MAX which really should go into its own patch
> 2.adds mtk_pwm_com_reg which should also go into its own patch
> 3.remove comments inline /*===*/
>
> for #1 and #3, I will modify them in the next release.
> but for #2, I want to discuss with you,
> adding the structure  "mtk_pwm_com_reg" is only for the registers of 
> MT2712 PWM8,
> so we want to keep this modification in this patch.
>
> what's your opinion about it?
> Any reply is welcome.
>
>
> Regards
> Zhi
>
>

Hi Zhi,

I just had a closer look and noticed that the new CON registers are not 
at the same fixed 0x40 offset as the pwm0-5 ones. sorry i did not notice 
this. 2) needs to be part of this patch in that case. please add a short 
info to the patches description explaining this.

     John


>
>
>
> On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:
>> On 21/06/17 10:11, Zhi Mao wrote:
>> > support multiple chip(MT2712, MT7622, MT7623)
>> This patch does more than add extra SoC support. It also
>> * adds PWM_CLK_DIV_MAX which really should go into its own patch
>> * adds mtk_pwm_com_reg which should also go into its own patch
>>
>> more comments inline
>>
>> >
>> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com <mailto:zhi.mao@mediatek.com>>
>> > ---
>> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>> >   1 file changed, 51 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>> > index c803ff6..d520356 100644
>> > --- a/drivers/pwm/pwm-mediatek.c
>> > +++ b/drivers/pwm/pwm-mediatek.c
>> > @@ -16,6 +16,7 @@
>> >   #include <linux/module.h>
>> >   #include <linux/clk.h>
>> >   #include <linux/of.h>
>> > +#include <linux/of_device.h>
>> >   #include <linux/platform_device.h>
>> >   #include <linux/pwm.h>
>> >   #include <linux/slab.h>
>> [...]
>> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>> >   	return pwmchip_remove(&pc->chip);
>> >   }
>> >
>> > +/*==========================================*/
>>
>> please remove these comment lines
>>
>>       John
>> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
>> > +	.pwm_nums = 8,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
>> > +	.pwm_nums = 6,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
>> > +	.pwm_nums = 5,
>> > +};
>> > +/*==========================================*/
>> > +
>> >   static const struct of_device_id mtk_pwm_of_match[] = {
>> > -	{ .compatible = "mediatek,mt7623-pwm" },
>> > -	{ }
>> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
>> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
>> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
>> > +	{},
>> >   };
>> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>> >
>>
>

Re: [PATCH RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 22/06/17 08:09, Zhi Mao wrote:
> Hi John,
>
> Thanks for your review the code and feedback.
> There are 3 issues in this patch:
> 1.adds PWM_CLK_DIV_MAX which really should go into its own patch
> 2.adds mtk_pwm_com_reg which should also go into its own patch
> 3.remove comments inline /*===*/
>
> for #1 and #3, I will modify them in the next release.
> but for #2, I want to discuss with you,
> adding the structure  "mtk_pwm_com_reg" is only for the registers of 
> MT2712 PWM8,
> so we want to keep this modification in this patch.
>
> what's your opinion about it?
> Any reply is welcome.
>
>
> Regards
> Zhi
>
>

Hi Zhi,

I just had a closer look and noticed that the new CON registers are not 
at the same fixed 0x40 offset as the pwm0-5 ones. sorry i did not notice 
this. 2) needs to be part of this patch in that case. please add a short 
info to the patches description explaining this.

     John


>
>
>
> On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:
>> On 21/06/17 10:11, Zhi Mao wrote:
>> > support multiple chip(MT2712, MT7622, MT7623)
>> This patch does more than add extra SoC support. It also
>> * adds PWM_CLK_DIV_MAX which really should go into its own patch
>> * adds mtk_pwm_com_reg which should also go into its own patch
>>
>> more comments inline
>>
>> >
>> > Signed-off-by: Zhi Mao <zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org <mailto:zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>>
>> > ---
>> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>> >   1 file changed, 51 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>> > index c803ff6..d520356 100644
>> > --- a/drivers/pwm/pwm-mediatek.c
>> > +++ b/drivers/pwm/pwm-mediatek.c
>> > @@ -16,6 +16,7 @@
>> >   #include <linux/module.h>
>> >   #include <linux/clk.h>
>> >   #include <linux/of.h>
>> > +#include <linux/of_device.h>
>> >   #include <linux/platform_device.h>
>> >   #include <linux/pwm.h>
>> >   #include <linux/slab.h>
>> [...]
>> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>> >   	return pwmchip_remove(&pc->chip);
>> >   }
>> >
>> > +/*==========================================*/
>>
>> please remove these comment lines
>>
>>       John
>> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
>> > +	.pwm_nums = 8,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
>> > +	.pwm_nums = 6,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
>> > +	.pwm_nums = 5,
>> > +};
>> > +/*==========================================*/
>> > +
>> >   static const struct of_device_id mtk_pwm_of_match[] = {
>> > -	{ .compatible = "mediatek,mt7623-pwm" },
>> > -	{ }
>> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
>> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
>> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
>> > +	{},
>> >   };
>> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>> >
>>
>

--
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 RESEND 4/4] pwm: mediatek: add MT2712/MT7622 support

[John Crispin]

On 22/06/17 08:09, Zhi Mao wrote:
> Hi John,
>
> Thanks for your review the code and feedback.
> There are 3 issues in this patch:
> 1.adds PWM_CLK_DIV_MAX which really should go into its own patch
> 2.adds mtk_pwm_com_reg which should also go into its own patch
> 3.remove comments inline /*===*/
>
> for #1 and #3, I will modify them in the next release.
> but for #2, I want to discuss with you,
> adding the structure  "mtk_pwm_com_reg" is only for the registers of 
> MT2712 PWM8,
> so we want to keep this modification in this patch.
>
> what's your opinion about it?
> Any reply is welcome.
>
>
> Regards
> Zhi
>
>

Hi Zhi,

I just had a closer look and noticed that the new CON registers are not 
at the same fixed 0x40 offset as the pwm0-5 ones. sorry i did not notice 
this. 2) needs to be part of this patch in that case. please add a short 
info to the patches description explaining this.

     John


>
>
>
> On Wed, 2017-06-21 at 20:22 +0800, John Crispin wrote:
>> On 21/06/17 10:11, Zhi Mao wrote:
>> > support multiple chip(MT2712, MT7622, MT7623)
>> This patch does more than add extra SoC support. It also
>> * adds PWM_CLK_DIV_MAX which really should go into its own patch
>> * adds mtk_pwm_com_reg which should also go into its own patch
>>
>> more comments inline
>>
>> >
>> > Signed-off-by: Zhi Mao <zhi.mao at mediatek.com <mailto:zhi.mao@mediatek.com>>
>> > ---
>> >   drivers/pwm/pwm-mediatek.c |   63 +++++++++++++++++++++++++++++++++++---------
>> >   1 file changed, 51 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>> > index c803ff6..d520356 100644
>> > --- a/drivers/pwm/pwm-mediatek.c
>> > +++ b/drivers/pwm/pwm-mediatek.c
>> > @@ -16,6 +16,7 @@
>> >   #include <linux/module.h>
>> >   #include <linux/clk.h>
>> >   #include <linux/of.h>
>> > +#include <linux/of_device.h>
>> >   #include <linux/platform_device.h>
>> >   #include <linux/pwm.h>
>> >   #include <linux/slab.h>
>> [...]
>> > @@ -215,9 +238,25 @@ static int mtk_pwm_remove(struct platform_device *pdev)
>> >   	return pwmchip_remove(&pc->chip);
>> >   }
>> >
>> > +/*==========================================*/
>>
>> please remove these comment lines
>>
>>       John
>> > +static const struct mtk_com_pwm_data mt2712_pwm_data = {
>> > +	.pwm_nums = 8,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7622_pwm_data = {
>> > +	.pwm_nums = 6,
>> > +};
>> > +
>> > +static const struct mtk_com_pwm_data mt7623_pwm_data = {
>> > +	.pwm_nums = 5,
>> > +};
>> > +/*==========================================*/
>> > +
>> >   static const struct of_device_id mtk_pwm_of_match[] = {
>> > -	{ .compatible = "mediatek,mt7623-pwm" },
>> > -	{ }
>> > +	{.compatible = "mediatek,mt2712-pwm", .data = &mt2712_pwm_data},
>> > +	{.compatible = "mediatek,mt7622-pwm", .data = &mt7622_pwm_data},
>> > +	{.compatible = "mediatek,mt7623-pwm", .data = &mt7623_pwm_data},
>> > +	{},
>> >   };
>> >   MODULE_DEVICE_TABLE(of, mtk_pwm_of_match);
>> >
>>
>

Re: [PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[Zhi Mao]

On Wed, 2017-06-21 at 20:07 +0800, John Crispin wrote:
> Hi
> 
> comments inline
> 
> 
> > +static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{

> > +	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> > +	if (ret < 0) {
> > +		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> > +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> > +		return ret;
> > +	}
> > +
> Rather than disabling the already prepared clks in each error path and 
> then returning, you should use goto err_clk_{top,main,pwm1} in the same 
> style as what this patch removes from mtk_pwm_probe()
> 
> > +	return ret;
> > +}

> >   static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
> >   				unsigned int offset)
> >   {

> > @@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >   	if (clkdiv > 7)
> >   		return -EINVAL;
> >   
> > -	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> > +	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> 
> this chunk needs to go into its own patch
> 

> >   
> > @@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >   {
> >   
> > -	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> > -	if (ret < 0)
> > -		return ret;
> > +	mtk_pwm_clk_enable(chip, pwm);
> You need to check the error code here and return if clk enabling failed
> 

<....>
Hi John,
For these above comments, I will modified as your suggestions in the
next release.

> > 
> >   
> >   static int mtk_pwm_remove(struct platform_device *pdev)
> >   {
> >   	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
> > -	unsigned int i;
> > -
> > -	for (i = 0; i < pc->chip.npwm; i++)
> > -		pwm_disable(&pc->chip.pwms[i]);
> why are you removing this chunk ?
> 
>      John
> 

After refering to some other vendor's pwm driver, we think the
"pwm_disable" is no need, and framework control flow should disable all
the pwms before removing them,
so we remove it.

Regards,
Zhi

> >   
> >   	return pwmchip_remove(&pc->chip);
> >   }
> 

Re: [PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[Zhi Mao]

On Wed, 2017-06-21 at 20:07 +0800, John Crispin wrote:
> Hi
> 
> comments inline
> 
> 
> > +static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{

> > +	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> > +	if (ret < 0) {
> > +		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> > +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> > +		return ret;
> > +	}
> > +
> Rather than disabling the already prepared clks in each error path and 
> then returning, you should use goto err_clk_{top,main,pwm1} in the same 
> style as what this patch removes from mtk_pwm_probe()
> 
> > +	return ret;
> > +}

> >   static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
> >   				unsigned int offset)
> >   {

> > @@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >   	if (clkdiv > 7)
> >   		return -EINVAL;
> >   
> > -	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> > +	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> 
> this chunk needs to go into its own patch
> 

> >   
> > @@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >   {
> >   
> > -	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> > -	if (ret < 0)
> > -		return ret;
> > +	mtk_pwm_clk_enable(chip, pwm);
> You need to check the error code here and return if clk enabling failed
> 

<....>
Hi John,
For these above comments, I will modified as your suggestions in the
next release.

> > 
> >   
> >   static int mtk_pwm_remove(struct platform_device *pdev)
> >   {
> >   	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
> > -	unsigned int i;
> > -
> > -	for (i = 0; i < pc->chip.npwm; i++)
> > -		pwm_disable(&pc->chip.pwms[i]);
> why are you removing this chunk ?
> 
>      John
> 

After refering to some other vendor's pwm driver, we think the
"pwm_disable" is no need, and framework control flow should disable all
the pwms before removing them,
so we remove it.

Regards,
Zhi

> >   
> >   	return pwmchip_remove(&pc->chip);
> >   }
> 

[PATCH RESEND 2/4] pwm: mediatek: fix clk issue

[Zhi Mao]

On Wed, 2017-06-21 at 20:07 +0800, John Crispin wrote:
> Hi
> 
> comments inline
> 
> 
> > +static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{

> > +	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> > +	if (ret < 0) {
> > +		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
> > +		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
> > +		return ret;
> > +	}
> > +
> Rather than disabling the already prepared clks in each error path and 
> then returning, you should use goto err_clk_{top,main,pwm1} in the same 
> style as what this patch removes from mtk_pwm_probe()
> 
> > +	return ret;
> > +}

> >   static inline u32 mtk_pwm_readl(struct mtk_pwm_chip *chip, unsigned int num,
> >   				unsigned int offset)
> >   {

> > @@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >   	if (clkdiv > 7)
> >   		return -EINVAL;
> >   
> > -	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
> > +	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> 
> this chunk needs to go into its own patch
> 

> >   
> > @@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >   {
> >   
> > -	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
> > -	if (ret < 0)
> > -		return ret;
> > +	mtk_pwm_clk_enable(chip, pwm);
> You need to check the error code here and return if clk enabling failed
> 

<....>
Hi John,
For these above comments, I will modified as your suggestions in the
next release.

> > 
> >   
> >   static int mtk_pwm_remove(struct platform_device *pdev)
> >   {
> >   	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
> > -	unsigned int i;
> > -
> > -	for (i = 0; i < pc->chip.npwm; i++)
> > -		pwm_disable(&pc->chip.pwms[i]);
> why are you removing this chunk ?
> 
>      John
> 

After refering to some other vendor's pwm driver, we think the
"pwm_disable" is no need, and framework control flow should disable all
the pwms before removing them,
so we remove it.

Regards,
Zhi

> >   
> >   	return pwmchip_remove(&pc->chip);
> >   }
>