[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 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 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

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);
>   }

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);
>   

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 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 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);
> >   }
>