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

2017-06-22 Thread 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(>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(>chip);
> >   }
>

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

2017-06-22 Thread 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(>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(>chip);
> >   }
>

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

2017-06-21 Thread 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 
---
  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 
+ * Copyright (C) 2017 Zhi Mao 
   *
   * 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 = >dev;

@@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
ret = pwmchip_add(>chip);
if (ret < 0) {
dev_err(>dev, "pwmchip_add() failed: %d\n",

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

2017-06-21 Thread 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 
---
  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 
+ * Copyright (C) 2017 Zhi Mao 
   *
   * 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 = >dev;

@@ -174,26 +202,15 @@ static int mtk_pwm_probe(struct platform_device *pdev)
ret = pwmchip_add(>chip);
if (ret < 0) {
dev_err(>dev, "pwmchip_add() failed: %d\n", ret);
-   goto disable_clk_main;
+