[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

change in v3:
1. add pwm clk disable in function:mtk_pwm_config()
   for error parameter checking case

Zhi Mao (6):
  pwm: kconfig: modify mediatek information
  pwm: mediatek: fix pwm source clock selection
  pwm: mediatek: fix clock control issue
  pwm: bindings: add MT2712/MT7622 information
  pwm: mediatek: add PWM_CLK_DIV_MAX
  pwm: mediatek: add MT2712/MT7622 support

 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
 drivers/pwm/Kconfig                                |    2 +-
 drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
 3 files changed, 104 insertions(+), 37 deletions(-)

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

change in v3:
1. add pwm clk disable in function:mtk_pwm_config()
   for error parameter checking case

Zhi Mao (6):
  pwm: kconfig: modify mediatek information
  pwm: mediatek: fix pwm source clock selection
  pwm: mediatek: fix clock control issue
  pwm: bindings: add MT2712/MT7622 information
  pwm: mediatek: add PWM_CLK_DIV_MAX
  pwm: mediatek: add MT2712/MT7622 support

 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
 drivers/pwm/Kconfig                                |    2 +-
 drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
 3 files changed, 104 insertions(+), 37 deletions(-)

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

change in v3:
1. add pwm clk disable in function:mtk_pwm_config()
   for error parameter checking case

Zhi Mao (6):
  pwm: kconfig: modify mediatek information
  pwm: mediatek: fix pwm source clock selection
  pwm: mediatek: fix clock control issue
  pwm: bindings: add MT2712/MT7622 information
  pwm: mediatek: add PWM_CLK_DIV_MAX
  pwm: mediatek: add MT2712/MT7622 support

 .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
 drivers/pwm/Kconfig                                |    2 +-
 drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
 3 files changed, 104 insertions(+), 37 deletions(-)

[PATCH v3 1/6] 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 v3 1/6] 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 v3 1/6] 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

Re: [PATCH v3 1/6] pwm: kconfig: modify mediatek information

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:16PM +0800, Zhi Mao wrote:
> modify mediatek information
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/Kconfig |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, with a slightly modified subject and commit
message.

Thanks,
Thierry

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

[PATCH v3 1/6] pwm: kconfig: modify mediatek information

[Thierry Reding]

On Fri, Jun 30, 2017 at 02:05:16PM +0800, Zhi Mao wrote:
> modify mediatek information
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/Kconfig |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, with a slightly modified subject and commit
message.

Thanks,
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/f3479853/attachment.sig>

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

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 |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..d08b5b3 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -91,7 +91,7 @@ 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);
 
-- 
1.7.9.5

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

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 |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..d08b5b3 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -91,7 +91,7 @@ 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);
 
-- 
1.7.9.5

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

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 |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 5c11bc7..d08b5b3 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -91,7 +91,7 @@ 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);
 
-- 
1.7.9.5

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Matthias Brugger]

On 06/30/2017 08:05 AM, Zhi Mao wrote:
> 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 |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5c11bc7..d08b5b3 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -91,7 +91,7 @@ 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);

Just for clarification, BIT(15) enables old PWM mode, which ignores 
CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
can be discarded.

Am I correct? I took mt7623n datasheet for reference.

Regards,
Matthias

>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
>   
> 

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Matthias Brugger]

On 06/30/2017 08:05 AM, Zhi Mao wrote:
> 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 |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 5c11bc7..d08b5b3 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -91,7 +91,7 @@ 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);

Just for clarification, BIT(15) enables old PWM mode, which ignores 
CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
can be discarded.

Am I correct? I took mt7623n datasheet for reference.

Regards,
Matthias

>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
>   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
>   
> 

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> 
> On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > 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 |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 5c11bc7..d08b5b3 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -91,7 +91,7 @@ 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);
> 
> Just for clarification, BIT(15) enables old PWM mode, which ignores 
> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
> can be discarded.
> 
> Am I correct? I took mt7623n datasheet for reference.
> 
> Regards,
> Matthias
> 
Yes, remove setting bit<3> will not take any effect. 
PWMCON bit<3> is pwm source clock selecting register.
You can check the datasheet of MT7623 for details.

Regards
Zhi

> >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> >   
> > 

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> 
> On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > 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 |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 5c11bc7..d08b5b3 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -91,7 +91,7 @@ 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);
> 
> Just for clarification, BIT(15) enables old PWM mode, which ignores 
> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
> can be discarded.
> 
> Am I correct? I took mt7623n datasheet for reference.
> 
> Regards,
> Matthias
> 
Yes, remove setting bit<3> will not take any effect. 
PWMCON bit<3> is pwm source clock selecting register.
You can check the datasheet of MT7623 for details.

Regards
Zhi

> >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> >   
> > 

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> 
> On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >   drivers/pwm/pwm-mediatek.c |    2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 5c11bc7..d08b5b3 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -91,7 +91,7 @@ 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);
> 
> Just for clarification, BIT(15) enables old PWM mode, which ignores 
> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
> can be discarded.
> 
> Am I correct? I took mt7623n datasheet for reference.
> 
> Regards,
> Matthias
> 
Yes, remove setting bit<3> will not take any effect. 
PWMCON bit<3> is pwm source clock selecting register.
You can check the datasheet of MT7623 for details.

Regards
Zhi

> >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> >   
> > 


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

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> > 
> > On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > > 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 |    2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index 5c11bc7..d08b5b3 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -91,7 +91,7 @@ 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);
> > 
> > Just for clarification, BIT(15) enables old PWM mode, which ignores 
> > CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
> > can be discarded.
> > 
> > Am I correct? I took mt7623n datasheet for reference.
> > 
> > Regards,
> > Matthias
> > 
> Yes, remove setting bit<3> will not take any effect. 
> PWMCON bit<3> is pwm source clock selecting register.
> You can check the datasheet of MT7623 for details.
> 
> Regards
> Zhi
> 
Hi Mattias,

Ignore the above reply, I explain this bit<3> issue for you.
In the data sheet of MT7623: 
PWMCON bit<3> is PWM source clock selecting register
0: CLK=CLKSRC
1: CLK=CLKSRC/1625
for example,
bit<3>=0, PWM clk source is 26M
bit<3>=1, PWM clk source is 16K
The frequency of PWM output will based on this clock source
if set bit<3>=1, it will cause the frequency of PWM output is not
correct. I also use the oscilloscope device to measure the output,     
set bit<3>=0, the output meet expectation. 
 
Regards
Zhi
> > >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> > >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> > >   
> > > 
> 

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> > 
> > On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > > 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 |    2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index 5c11bc7..d08b5b3 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -91,7 +91,7 @@ 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);
> > 
> > Just for clarification, BIT(15) enables old PWM mode, which ignores 
> > CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
> > can be discarded.
> > 
> > Am I correct? I took mt7623n datasheet for reference.
> > 
> > Regards,
> > Matthias
> > 
> Yes, remove setting bit<3> will not take any effect. 
> PWMCON bit<3> is pwm source clock selecting register.
> You can check the datasheet of MT7623 for details.
> 
> Regards
> Zhi
> 
Hi Mattias,

Ignore the above reply, I explain this bit<3> issue for you.
In the data sheet of MT7623: 
PWMCON bit<3> is PWM source clock selecting register
0: CLK=CLKSRC
1: CLK=CLKSRC/1625
for example,
bit<3>=0, PWM clk source is 26M
bit<3>=1, PWM clk source is 16K
The frequency of PWM output will based on this clock source
if set bit<3>=1, it will cause the frequency of PWM output is not
correct. I also use the oscilloscope device to measure the output,     
set bit<3>=0, the output meet expectation. 
 
Regards
Zhi
> > >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> > >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> > >   
> > > 
> 

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Zhi Mao]

On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
> > 
> > On 06/30/2017 08:05 AM, Zhi Mao wrote:
> > > 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > ---
> > >   drivers/pwm/pwm-mediatek.c |    2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > > index 5c11bc7..d08b5b3 100644
> > > --- a/drivers/pwm/pwm-mediatek.c
> > > +++ b/drivers/pwm/pwm-mediatek.c
> > > @@ -91,7 +91,7 @@ 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);
> > 
> > Just for clarification, BIT(15) enables old PWM mode, which ignores 
> > CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and 
> > can be discarded.
> > 
> > Am I correct? I took mt7623n datasheet for reference.
> > 
> > Regards,
> > Matthias
> > 
> Yes, remove setting bit<3> will not take any effect. 
> PWMCON bit<3> is pwm source clock selecting register.
> You can check the datasheet of MT7623 for details.
> 
> Regards
> Zhi
> 
Hi Mattias,

Ignore the above reply, I explain this bit<3> issue for you.
In the data sheet of MT7623: 
PWMCON bit<3> is PWM source clock selecting register
0: CLK=CLKSRC
1: CLK=CLKSRC/1625
for example,
bit<3>=0, PWM clk source is 26M
bit<3>=1, PWM clk source is 16K
The frequency of PWM output will based on this clock source
if set bit<3>=1, it will cause the frequency of PWM output is not
correct. I also use the oscilloscope device to measure the output,     
set bit<3>=0, the output meet expectation. 
 
Regards
Zhi
> > >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMDWIDTH, period_ns / resolution);
> > >   	mtk_pwm_writel(pc, pwm->hwpwm, PWMTHRES, duty_ns / resolution);
> > >   
> > > 
> 


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

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Matthias Brugger]

On 07/06/2017 08:43 AM, Zhi Mao wrote:
> On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
>> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
>>>
>>> On 06/30/2017 08:05 AM, Zhi Mao wrote:
>>>> 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 |    2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>>>> index 5c11bc7..d08b5b3 100644
>>>> --- a/drivers/pwm/pwm-mediatek.c
>>>> +++ b/drivers/pwm/pwm-mediatek.c
>>>> @@ -91,7 +91,7 @@ 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);
>>>
>>> Just for clarification, BIT(15) enables old PWM mode, which ignores
>>> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and
>>> can be discarded.
>>>
>>> Am I correct? I took mt7623n datasheet for reference.
>>>
>>> Regards,
>>> Matthias
>>>
>> Yes, remove setting bit<3> will not take any effect.
>> PWMCON bit<3> is pwm source clock selecting register.
>> You can check the datasheet of MT7623 for details.
>>
>> Regards
>> Zhi
>>
> Hi Mattias,
> 
> Ignore the above reply, I explain this bit<3> issue for you.
> In the data sheet of MT7623:
> PWMCON bit<3> is PWM source clock selecting register
> 0: CLK=CLKSRC
> 1: CLK=CLKSRC/1625
> for example,
> bit<3>=0, PWM clk source is 26M
> bit<3>=1, PWM clk source is 16K
> The frequency of PWM output will based on this clock source
> if set bit<3>=1, it will cause the frequency of PWM output is not
> correct. I also use the oscilloscope device to measure the output,
> set bit<3>=0, the output meet expectation.
>   

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Matthias Brugger]

On 07/06/2017 08:43 AM, Zhi Mao wrote:
> On Thu, 2017-07-06 at 14:16 +0800, Zhi Mao wrote:
>> On Wed, 2017-07-05 at 13:09 +0200, Matthias Brugger wrote:
>>>
>>> On 06/30/2017 08:05 AM, Zhi Mao wrote:
>>>> 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 |    2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
>>>> index 5c11bc7..d08b5b3 100644
>>>> --- a/drivers/pwm/pwm-mediatek.c
>>>> +++ b/drivers/pwm/pwm-mediatek.c
>>>> @@ -91,7 +91,7 @@ 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);
>>>
>>> Just for clarification, BIT(15) enables old PWM mode, which ignores
>>> CLKSEL (BIT(3)). Therefore setting BIT(3) does not have any effect and
>>> can be discarded.
>>>
>>> Am I correct? I took mt7623n datasheet for reference.
>>>
>>> Regards,
>>> Matthias
>>>
>> Yes, remove setting bit<3> will not take any effect.
>> PWMCON bit<3> is pwm source clock selecting register.
>> You can check the datasheet of MT7623 for details.
>>
>> Regards
>> Zhi
>>
> Hi Mattias,
> 
> Ignore the above reply, I explain this bit<3> issue for you.
> In the data sheet of MT7623:
> PWMCON bit<3> is PWM source clock selecting register
> 0: CLK=CLKSRC
> 1: CLK=CLKSRC/1625
> for example,
> bit<3>=0, PWM clk source is 26M
> bit<3>=1, PWM clk source is 16K
> The frequency of PWM output will based on this clock source
> if set bit<3>=1, it will cause the frequency of PWM output is not
> correct. I also use the oscilloscope device to measure the output,
> set bit<3>=0, the output meet expectation.
>   

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:17PM +0800, Zhi Mao wrote:
> 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 |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, thanks.

Thierry

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

[PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Thierry Reding]

On Fri, Jun 30, 2017 at 02:05:17PM +0800, Zhi Mao wrote:
> 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 |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, thanks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/085d50f4/attachment.sig>

Re: [PATCH v3 2/6] pwm: mediatek: fix pwm source clock selection

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:17PM +0800, Zhi Mao wrote:
> 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/pwm/pwm-mediatek.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-4.14/drivers, thanks.

Thierry

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

[PATCH v3 3/6] pwm: mediatek: fix clock control issue

[Zhi Mao]

1. 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.
2. pwm clock should be enabled before setting pwm registers
   in function: mtk_pwm_config().
3. delete "pwm_disable" in function:mtk_pwm_remove(),
   as framework should disable all the pwms, before remove them.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index d08b5b3..554a042 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,42 @@ 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)
+		goto disable_clk_top;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0)
+		goto disable_clk_main;
+
+	return 0;
+
+disable_clk_main:
+	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+disable_clk_top:
+	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+
+	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)
 {
@@ -80,6 +117,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
 	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
 	u32 resolution, clkdiv = 0;
+	int ret;
+
+	ret = mtk_pwm_clk_enable(chip, pwm);
+	if (ret < 0)
+		return ret;
 
 	resolution = NSEC_PER_SEC / clk_get_rate(clk);
 
@@ -95,6 +137,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	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;
 }
 
@@ -104,7 +148,7 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	u32 value;
 	int ret;
 
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	ret = mtk_pwm_clk_enable(chip, pwm);
 	if (ret < 0)
 		return ret;
 
@@ -124,7 +168,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 +200,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 +210,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 v3 3/6] pwm: mediatek: fix clock control issue

[Zhi Mao]

1. 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.
2. pwm clock should be enabled before setting pwm registers
   in function: mtk_pwm_config().
3. delete "pwm_disable" in function:mtk_pwm_remove(),
   as framework should disable all the pwms, before remove them.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index d08b5b3..554a042 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,42 @@ 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)
+		goto disable_clk_top;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0)
+		goto disable_clk_main;
+
+	return 0;
+
+disable_clk_main:
+	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+disable_clk_top:
+	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+
+	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)
 {
@@ -80,6 +117,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
 	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
 	u32 resolution, clkdiv = 0;
+	int ret;
+
+	ret = mtk_pwm_clk_enable(chip, pwm);
+	if (ret < 0)
+		return ret;
 
 	resolution = NSEC_PER_SEC / clk_get_rate(clk);
 
@@ -95,6 +137,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	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;
 }
 
@@ -104,7 +148,7 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	u32 value;
 	int ret;
 
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	ret = mtk_pwm_clk_enable(chip, pwm);
 	if (ret < 0)
 		return ret;
 
@@ -124,7 +168,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 +200,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 +210,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 v3 3/6] pwm: mediatek: fix clock control issue

[Zhi Mao]

1. 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.
2. pwm clock should be enabled before setting pwm registers
   in function: mtk_pwm_config().
3. delete "pwm_disable" in function:mtk_pwm_remove(),
   as framework should disable all the pwms, before remove them.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index d08b5b3..554a042 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,42 @@ 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)
+		goto disable_clk_top;
+
+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0)
+		goto disable_clk_main;
+
+	return 0;
+
+disable_clk_main:
+	clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+disable_clk_top:
+	clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+
+	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)
 {
@@ -80,6 +117,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct mtk_pwm_chip *pc = to_mtk_pwm_chip(chip);
 	struct clk *clk = pc->clks[MTK_CLK_PWM1 + pwm->hwpwm];
 	u32 resolution, clkdiv = 0;
+	int ret;
+
+	ret = mtk_pwm_clk_enable(chip, pwm);
+	if (ret < 0)
+		return ret;
 
 	resolution = NSEC_PER_SEC / clk_get_rate(clk);
 
@@ -95,6 +137,8 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	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;
 }
 
@@ -104,7 +148,7 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	u32 value;
 	int ret;
 
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	ret = mtk_pwm_clk_enable(chip, pwm);
 	if (ret < 0)
 		return ret;
 
@@ -124,7 +168,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 +200,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 +210,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

Re: [PATCH v3 3/6] pwm: mediatek: fix clock control issue

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:18PM +0800, Zhi Mao wrote:
> 1. 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.
> 2. pwm clock should be enabled before setting pwm registers
>    in function: mtk_pwm_config().
> 3. delete "pwm_disable" in function:mtk_pwm_remove(),
>    as framework should disable all the pwms, before remove them.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |   69 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 47 insertions(+), 22 deletions(-)

I think the commit description could be better. Try to avoid enumerating
changes like you did. Not only is it tedious to read, but this kind of
enumeration is often a sign that you can split the commit up into
multiple logical changes.

Especially the change described in 3) is not related to the clock. It's
also not a quite correct description, because there is no code in the
framework that disables PWMs on chip removal. Users should've disabled
the PWMs that they were using.

The framework could probably warn about misuse, though.

That said, I've left the change as-is, and changed the commit message to
this:

--- >8 ---
pwm: mediatek: Fix clock control issue

In order to save some power, do not prepare the top and main clocks
during mtk_pwm_probe(). Instead, prepare the clocks only when necessary
and also make sure to enable the clocks to match the semantics of the
common clock framework.

While at it, don't explicitly disable all PWM channels in ->remove()
because all users should have done that already.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
--- 8< ---

Try to keep this in mind for future patch submissions.

Applied to for-4.14/drivers, thanks.

Thierry

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

[PATCH v3 3/6] pwm: mediatek: fix clock control issue

[Thierry Reding]

On Fri, Jun 30, 2017 at 02:05:18PM +0800, Zhi Mao wrote:
> 1. 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.
> 2. pwm clock should be enabled before setting pwm registers
>    in function: mtk_pwm_config().
> 3. delete "pwm_disable" in function:mtk_pwm_remove(),
>    as framework should disable all the pwms, before remove them.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |   69 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 47 insertions(+), 22 deletions(-)

I think the commit description could be better. Try to avoid enumerating
changes like you did. Not only is it tedious to read, but this kind of
enumeration is often a sign that you can split the commit up into
multiple logical changes.

Especially the change described in 3) is not related to the clock. It's
also not a quite correct description, because there is no code in the
framework that disables PWMs on chip removal. Users should've disabled
the PWMs that they were using.

The framework could probably warn about misuse, though.

That said, I've left the change as-is, and changed the commit message to
this:

--- >8 ---
pwm: mediatek: Fix clock control issue

In order to save some power, do not prepare the top and main clocks
during mtk_pwm_probe(). Instead, prepare the clocks only when necessary
and also make sure to enable the clocks to match the semantics of the
common clock framework.

While at it, don't explicitly disable all PWM channels in ->remove()
because all users should have done that already.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
--- 8< ---

Try to keep this in mind for future patch submissions.

Applied to for-4.14/drivers, thanks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/1a5ffacc/attachment.sig>

Re: [PATCH v3 3/6] pwm: mediatek: fix clock control issue

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:18PM +0800, Zhi Mao wrote:
> 1. 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.
> 2. pwm clock should be enabled before setting pwm registers
>    in function: mtk_pwm_config().
> 3. delete "pwm_disable" in function:mtk_pwm_remove(),
>    as framework should disable all the pwms, before remove them.
> 
> Signed-off-by: Zhi Mao <zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/pwm/pwm-mediatek.c |   69 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 47 insertions(+), 22 deletions(-)

I think the commit description could be better. Try to avoid enumerating
changes like you did. Not only is it tedious to read, but this kind of
enumeration is often a sign that you can split the commit up into
multiple logical changes.

Especially the change described in 3) is not related to the clock. It's
also not a quite correct description, because there is no code in the
framework that disables PWMs on chip removal. Users should've disabled
the PWMs that they were using.

The framework could probably warn about misuse, though.

That said, I've left the change as-is, and changed the commit message to
this:

--- >8 ---
pwm: mediatek: Fix clock control issue

In order to save some power, do not prepare the top and main clocks
during mtk_pwm_probe(). Instead, prepare the clocks only when necessary
and also make sure to enable the clocks to match the semantics of the
common clock framework.

While at it, don't explicitly disable all PWM channels in ->remove()
because all users should have done that already.

Signed-off-by: Zhi Mao <zhi.mao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
Acked-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
Signed-off-by: Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--- 8< ---

Try to keep this in mind for future patch submissions.

Applied to for-4.14/drivers, thanks.

Thierry

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

[PATCH v3 4/6] pwm: bindings: add MT2712/MT7622 information

[Zhi Mao]

add MT2712/MT7622 pwm information

Acked-by: Rob Herring <robh@kernel.org>
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 v3 4/6] pwm: bindings: add MT2712/MT7622 information

[Zhi Mao]

add MT2712/MT7622 pwm information

Acked-by: Rob Herring <robh@kernel.org>
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 v3 4/6] pwm: bindings: add MT2712/MT7622 information

[Zhi Mao]

add MT2712/MT7622 pwm information

Acked-by: Rob Herring <robh@kernel.org>
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

Re: [PATCH v3 4/6] pwm: bindings: add MT2712/MT7622 information

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:19PM +0800, Zhi Mao wrote:
> add MT2712/MT7622 pwm information

Please avoid duplicating the subject in the description. You're not
adding any meaningful information. I've changed the commit to this:

--- >8 ---
dt-bindings: pwm: Add MT2712/MT7622 information

Enhance the MediaTek PWM binding with details about the IP found in the
MT2712 and MT7622 SoCs.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
--- 8< ---

Applied to for-4.14/drivers, thanks.

Thierry

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

[PATCH v3 4/6] pwm: bindings: add MT2712/MT7622 information

[Thierry Reding]

On Fri, Jun 30, 2017 at 02:05:19PM +0800, Zhi Mao wrote:
> add MT2712/MT7622 pwm information

Please avoid duplicating the subject in the description. You're not
adding any meaningful information. I've changed the commit to this:

--- >8 ---
dt-bindings: pwm: Add MT2712/MT7622 information

Enhance the MediaTek PWM binding with details about the IP found in the
MT2712 and MT7622 SoCs.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
--- 8< ---

Applied to for-4.14/drivers, thanks.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/73927736/attachment.sig>

[PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

[Zhi Mao]

1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
to improve the code readablity.
2. add pwm clk disable in function:mtk_pwm_config()
for error parameter checking case.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 554a042..1d78ab1 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -30,6 +30,8 @@
 #define PWMDWIDTH		0x2c
 #define PWMTHRES		0x30
 
+#define PWM_CLK_DIV_MAX		7
+
 enum {
 	MTK_CLK_MAIN = 0,
 	MTK_CLK_TOP,
@@ -130,8 +132,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv++;
 	}
 
-	if (clkdiv > 7)
+	if (clkdiv > PWM_CLK_DIV_MAX) {
+		mtk_pwm_clk_disable(chip, pwm);
+		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);
-- 
1.7.9.5

[PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

[Zhi Mao]

1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
to improve the code readablity.
2. add pwm clk disable in function:mtk_pwm_config()
for error parameter checking case.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 554a042..1d78ab1 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -30,6 +30,8 @@
 #define PWMDWIDTH		0x2c
 #define PWMTHRES		0x30
 
+#define PWM_CLK_DIV_MAX		7
+
 enum {
 	MTK_CLK_MAIN = 0,
 	MTK_CLK_TOP,
@@ -130,8 +132,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv++;
 	}
 
-	if (clkdiv > 7)
+	if (clkdiv > PWM_CLK_DIV_MAX) {
+		mtk_pwm_clk_disable(chip, pwm);
+		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);
-- 
1.7.9.5

[PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

[Zhi Mao]

1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
to improve the code readablity.
2. add pwm clk disable in function:mtk_pwm_config()
for error parameter checking case.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 554a042..1d78ab1 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -30,6 +30,8 @@
 #define PWMDWIDTH		0x2c
 #define PWMTHRES		0x30
 
+#define PWM_CLK_DIV_MAX		7
+
 enum {
 	MTK_CLK_MAIN = 0,
 	MTK_CLK_TOP,
@@ -130,8 +132,11 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv++;
 	}
 
-	if (clkdiv > 7)
+	if (clkdiv > PWM_CLK_DIV_MAX) {
+		mtk_pwm_clk_disable(chip, pwm);
+		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);
-- 
1.7.9.5

Re: [PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:20PM +0800, Zhi Mao wrote:
> 1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
> to improve the code readablity.
> 2. add pwm clk disable in function:mtk_pwm_config()
> for error parameter checking case.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Same comment as before. You've got two logical, unrelated changes in
this one commit. In this case you're mixing a cosmetic change with an
actual bug fix. And to make things worse, the commit subject mentions
only the cosmetic change, while the more important changes is only
described in a drive-by fashion.

You get another free pass this time, but please be more conscious about
these things in the future.

I've applied this to for-4.14/drivers with the following commit message:

--- >8 ---
pwm: mediatek: Disable clock on PWM configuration failure

Make sure to disable the PWM clock if the PWM cannot be configured due
to the clock divider exceeding the maximum value.

While at it, replace the hardcoded maximum clock divider with a defined
constant to improve code readability.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
--- 8< ---

Thierry

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

[PATCH v3 5/6] pwm: mediatek: add PWM_CLK_DIV_MAX

[Thierry Reding]

On Fri, Jun 30, 2017 at 02:05:20PM +0800, Zhi Mao wrote:
> 1. Replace "7" with "PWM_CLK_DIV_MAX" in function:mtk_pwm_config()
> to improve the code readablity.
> 2. add pwm clk disable in function:mtk_pwm_config()
> for error parameter checking case.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Same comment as before. You've got two logical, unrelated changes in
this one commit. In this case you're mixing a cosmetic change with an
actual bug fix. And to make things worse, the commit subject mentions
only the cosmetic change, while the more important changes is only
described in a drive-by fashion.

You get another free pass this time, but please be more conscious about
these things in the future.

I've applied this to for-4.14/drivers with the following commit message:

--- >8 ---
pwm: mediatek: Disable clock on PWM configuration failure

Make sure to disable the PWM clock if the PWM cannot be configured due
to the clock divider exceeding the maximum value.

While at it, replace the hardcoded maximum clock divider with a defined
constant to improve code readability.

Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
Acked-by: John Crispin <john@phrozen.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
--- 8< ---

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/83b70fd6/attachment-0001.sig>

[PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

1. support multiple chip(MT2712, MT7622, MT7623)
2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
   the register offset address of pwm8 for MT2712 is not fixed 0x40
   and they are not the same as pwm0~6.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 1d78ab1..2c9ce24 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>
@@ -40,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;
 };
 
 /**
@@ -57,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)
@@ -103,14 +117,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,
@@ -194,23 +208,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) {
@@ -228,9 +247,23 @@ 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 v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

1. support multiple chip(MT2712, MT7622, MT7623)
2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
   the register offset address of pwm8 for MT2712 is not fixed 0x40
   and they are not the same as pwm0~6.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 1d78ab1..2c9ce24 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>
@@ -40,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;
 };
 
 /**
@@ -57,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)
@@ -103,14 +117,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,
@@ -194,23 +208,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) {
@@ -228,9 +247,23 @@ 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 v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

1. support multiple chip(MT2712, MT7622, MT7623)
2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
   the register offset address of pwm8 for MT2712 is not fixed 0x40
   and they are not the same as pwm0~6.

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

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 1d78ab1..2c9ce24 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>
@@ -40,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;
 };
 
 /**
@@ -57,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)
@@ -103,14 +117,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,
@@ -194,23 +208,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) {
@@ -228,9 +247,23 @@ 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 v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Thierry Reding]

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

On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> 1. support multiple chip(MT2712, MT7622, MT7623)
> 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
>    the register offset address of pwm8 for MT2712 is not fixed 0x40
>    and they are not the same as pwm0~6.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |   55 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 1d78ab1..2c9ce24 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>
> @@ -40,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"

You're wrapping these lines at arbitrary boundaries. Make sure to use
all of the 80 columns at your disposal.

> +};
> +
> +struct mtk_com_pwm_data {

What does the _com stand for in the above?

> +	unsigned int pwm_nums;
>  };

Maybe name this num_pwms for consistency with other drivers?

>  
>  /**
> @@ -57,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[] = {

There's another of these _com that I don't understand what it means.
Also since these are all fairly small offsets, these can simply be
unsigned int.

> +	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
>  };
>  
>  static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> @@ -103,14 +117,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,
> @@ -194,23 +208,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]));

Why include the "[PWM] " prefix in the above message?

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

No need to move the location of the platform_set_drvdata() call. It's
needless churn.

>  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},
> +	{},

Spaces after { and before }, please.

Thierry

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

[PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Thierry Reding]

On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> 1. support multiple chip(MT2712, MT7622, MT7623)
> 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
>    the register offset address of pwm8 for MT2712 is not fixed 0x40
>    and they are not the same as pwm0~6.
> 
> Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> ---
>  drivers/pwm/pwm-mediatek.c |   55 +++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 1d78ab1..2c9ce24 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>
> @@ -40,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"

You're wrapping these lines at arbitrary boundaries. Make sure to use
all of the 80 columns at your disposal.

> +};
> +
> +struct mtk_com_pwm_data {

What does the _com stand for in the above?

> +	unsigned int pwm_nums;
>  };

Maybe name this num_pwms for consistency with other drivers?

>  
>  /**
> @@ -57,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[] = {

There's another of these _com that I don't understand what it means.
Also since these are all fairly small offsets, these can simply be
unsigned int.

> +	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
>  };
>  
>  static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> @@ -103,14 +117,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,
> @@ -194,23 +208,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]));

Why include the "[PWM] " prefix in the above message?

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

No need to move the location of the platform_set_drvdata() call. It's
needless churn.

>  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},
> +	{},

Spaces after { and before }, please.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/72c1f9de/attachment.sig>

Re: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

Hi Thierry,

Thanks for your review  code.
I will modify the code as you comment in the next release.

Regards
Zhi

On Mon, 2017-08-21 at 10:05 +0200, Thierry Reding wrote:
> On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> > 1. support multiple chip(MT2712, MT7622, MT7623)
> > 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
> >    the register offset address of pwm8 for MT2712 is not fixed 0x40
> >    and they are not the same as pwm0~6.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  drivers/pwm/pwm-mediatek.c |   55 +++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 44 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 1d78ab1..2c9ce24 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>
> > @@ -40,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"
> 
> You're wrapping these lines at arbitrary boundaries. Make sure to use
> all of the 80 columns at your disposal.
> 
> > +};
> > +
> > +struct mtk_com_pwm_data {
> 
> What does the _com stand for in the above?
> 
> > +	unsigned int pwm_nums;
> >  };
> 
> Maybe name this num_pwms for consistency with other drivers?
> 
> >  
> >  /**
> > @@ -57,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[] = {
> 
> There's another of these _com that I don't understand what it means.
> Also since these are all fairly small offsets, these can simply be
> unsigned int.
> 
> > +	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
> >  };
> >  
> >  static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> > @@ -103,14 +117,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,
> > @@ -194,23 +208,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]));
> 
> Why include the "[PWM] " prefix in the above message?
> 
> >  			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);
> 
> No need to move the location of the platform_set_drvdata() call. It's
> needless churn.
> 
> >  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},
> > +	{},
> 
> Spaces after { and before }, please.
> 
> Thierry

[PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

Hi Thierry,

Thanks for your review  code.
I will modify the code as you comment in the next release.

Regards
Zhi

On Mon, 2017-08-21 at 10:05 +0200, Thierry Reding wrote:
> On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> > 1. support multiple chip(MT2712, MT7622, MT7623)
> > 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
> >    the register offset address of pwm8 for MT2712 is not fixed 0x40
> >    and they are not the same as pwm0~6.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  drivers/pwm/pwm-mediatek.c |   55 +++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 44 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 1d78ab1..2c9ce24 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>
> > @@ -40,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"
> 
> You're wrapping these lines at arbitrary boundaries. Make sure to use
> all of the 80 columns at your disposal.
> 
> > +};
> > +
> > +struct mtk_com_pwm_data {
> 
> What does the _com stand for in the above?
> 
> > +	unsigned int pwm_nums;
> >  };
> 
> Maybe name this num_pwms for consistency with other drivers?
> 
> >  
> >  /**
> > @@ -57,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[] = {
> 
> There's another of these _com that I don't understand what it means.
> Also since these are all fairly small offsets, these can simply be
> unsigned int.
> 
> > +	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
> >  };
> >  
> >  static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> > @@ -103,14 +117,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,
> > @@ -194,23 +208,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]));
> 
> Why include the "[PWM] " prefix in the above message?
> 
> >  			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);
> 
> No need to move the location of the platform_set_drvdata() call. It's
> needless churn.
> 
> >  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},
> > +	{},
> 
> Spaces after { and before }, please.
> 
> Thierry

Re: [PATCH v3 6/6] pwm: mediatek: add MT2712/MT7622 support

[Zhi Mao]

Hi Thierry,

Thanks for your review  code.
I will modify the code as you comment in the next release.

Regards
Zhi

On Mon, 2017-08-21 at 10:05 +0200, Thierry Reding wrote:
> On Fri, Jun 30, 2017 at 02:05:21PM +0800, Zhi Mao wrote:
> > 1. support multiple chip(MT2712, MT7622, MT7623)
> > 2. add mtk_pwm_com_reg for match the registers of MT2712 pwm8
> >    the register offset address of pwm8 for MT2712 is not fixed 0x40
> >    and they are not the same as pwm0~6.
> > 
> > Signed-off-by: Zhi Mao <zhi.mao@mediatek.com>
> > ---
> >  drivers/pwm/pwm-mediatek.c |   55 +++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 44 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 1d78ab1..2c9ce24 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>
> > @@ -40,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"
> 
> You're wrapping these lines at arbitrary boundaries. Make sure to use
> all of the 80 columns at your disposal.
> 
> > +};
> > +
> > +struct mtk_com_pwm_data {
> 
> What does the _com stand for in the above?
> 
> > +	unsigned int pwm_nums;
> >  };
> 
> Maybe name this num_pwms for consistency with other drivers?
> 
> >  
> >  /**
> > @@ -57,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[] = {
> 
> There's another of these _com that I don't understand what it means.
> Also since these are all fairly small offsets, these can simply be
> unsigned int.
> 
> > +	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
> >  };
> >  
> >  static inline struct mtk_pwm_chip *to_mtk_pwm_chip(struct pwm_chip *chip)
> > @@ -103,14 +117,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,
> > @@ -194,23 +208,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]));
> 
> Why include the "[PWM] " prefix in the above message?
> 
> >  			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);
> 
> No need to move the location of the platform_set_drvdata() call. It's
> needless churn.
> 
> >  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},
> > +	{},
> 
> Spaces after { and before }, please.
> 
> Thierry

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Hi John, Matthais & PWM Maintainers

Just a Gentle ping on this issue :)
Is there anything should be modified?

Regards,
Zhi


On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> change in v3:
> 1. add pwm clk disable in function:mtk_pwm_config()
>    for error parameter checking case
> 
> Zhi Mao (6):
>   pwm: kconfig: modify mediatek information
>   pwm: mediatek: fix pwm source clock selection
>   pwm: mediatek: fix clock control issue
>   pwm: bindings: add MT2712/MT7622 information
>   pwm: mediatek: add PWM_CLK_DIV_MAX
>   pwm: mediatek: add MT2712/MT7622 support
> 
>  .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
>  drivers/pwm/Kconfig                                |    2 +-
>  drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
>  3 files changed, 104 insertions(+), 37 deletions(-)
> 

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Hi John, Matthais & PWM Maintainers

Just a Gentle ping on this issue :)
Is there anything should be modified?

Regards,
Zhi


On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> change in v3:
> 1. add pwm clk disable in function:mtk_pwm_config()
>    for error parameter checking case
> 
> Zhi Mao (6):
>   pwm: kconfig: modify mediatek information
>   pwm: mediatek: fix pwm source clock selection
>   pwm: mediatek: fix clock control issue
>   pwm: bindings: add MT2712/MT7622 information
>   pwm: mediatek: add PWM_CLK_DIV_MAX
>   pwm: mediatek: add MT2712/MT7622 support
> 
>  .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
>  drivers/pwm/Kconfig                                |    2 +-
>  drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
>  3 files changed, 104 insertions(+), 37 deletions(-)
> 

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Hi John, Matthais & PWM Maintainers

Just a Gentle ping on this issue :)
Is there anything should be modified?

Regards,
Zhi


On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> change in v3:
> 1. add pwm clk disable in function:mtk_pwm_config()
>    for error parameter checking case
> 
> Zhi Mao (6):
>   pwm: kconfig: modify mediatek information
>   pwm: mediatek: fix pwm source clock selection
>   pwm: mediatek: fix clock control issue
>   pwm: bindings: add MT2712/MT7622 information
>   pwm: mediatek: add PWM_CLK_DIV_MAX
>   pwm: mediatek: add MT2712/MT7622 support
> 
>  .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
>  drivers/pwm/Kconfig                                |    2 +-
>  drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
>  3 files changed, 104 insertions(+), 37 deletions(-)
> 

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Hi John, Matthais & Thierry,
> 
> Just a gentle ping on this issue again.
> Do you have any update?
> 
> Regards,
> Zhi


On Mon, 2017-07-17 at 11:16 +0800, Zhi Mao wrote:
> Hi John, Matthais & PWM Maintainers
> 
> Just a Gentle ping on this issue :)
> Is there anything should be modified?
> 
> Regards,
> Zhi
> 
> 
> On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> > change in v3:
> > 1. add pwm clk disable in function:mtk_pwm_config()
> >    for error parameter checking case
> > 
> > Zhi Mao (6):
> >   pwm: kconfig: modify mediatek information
> >   pwm: mediatek: fix pwm source clock selection
> >   pwm: mediatek: fix clock control issue
> >   pwm: bindings: add MT2712/MT7622 information
> >   pwm: mediatek: add PWM_CLK_DIV_MAX
> >   pwm: mediatek: add MT2712/MT7622 support
> > 
> >  .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
> >  drivers/pwm/Kconfig                                |    2 +-
> >  drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
> >  3 files changed, 104 insertions(+), 37 deletions(-)
> > 
> 

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Hi John, Matthais & Thierry,
> 
> Just a gentle ping on this issue again.
> Do you have any update?
> 
> Regards,
> Zhi


On Mon, 2017-07-17 at 11:16 +0800, Zhi Mao wrote:
> Hi John, Matthais & PWM Maintainers
> 
> Just a Gentle ping on this issue :)
> Is there anything should be modified?
> 
> Regards,
> Zhi
> 
> 
> On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> > change in v3:
> > 1. add pwm clk disable in function:mtk_pwm_config()
> >    for error parameter checking case
> > 
> > Zhi Mao (6):
> >   pwm: kconfig: modify mediatek information
> >   pwm: mediatek: fix pwm source clock selection
> >   pwm: mediatek: fix clock control issue
> >   pwm: bindings: add MT2712/MT7622 information
> >   pwm: mediatek: add PWM_CLK_DIV_MAX
> >   pwm: mediatek: add MT2712/MT7622 support
> > 
> >  .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
> >  drivers/pwm/Kconfig                                |    2 +-
> >  drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
> >  3 files changed, 104 insertions(+), 37 deletions(-)
> > 
> 

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Hi John, Matthais & Thierry,
> 
> Just a gentle ping on this issue again.
> Do you have any update?
> 
> Regards,
> Zhi


On Mon, 2017-07-17 at 11:16 +0800, Zhi Mao wrote:
> Hi John, Matthais & PWM Maintainers
> 
> Just a Gentle ping on this issue :)
> Is there anything should be modified?
> 
> Regards,
> Zhi
> 
> 
> On Fri, 2017-06-30 at 14:05 +0800, Zhi Mao wrote:
> > change in v3:
> > 1. add pwm clk disable in function:mtk_pwm_config()
> >    for error parameter checking case
> > 
> > Zhi Mao (6):
> >   pwm: kconfig: modify mediatek information
> >   pwm: mediatek: fix pwm source clock selection
> >   pwm: mediatek: fix clock control issue
> >   pwm: bindings: add MT2712/MT7622 information
> >   pwm: mediatek: add PWM_CLK_DIV_MAX
> >   pwm: mediatek: add MT2712/MT7622 support
> > 
> >  .../devicetree/bindings/pwm/pwm-mediatek.txt       |    6 +-
> >  drivers/pwm/Kconfig                                |    2 +-
> >  drivers/pwm/pwm-mediatek.c                         |  133 ++++++++++++++------
> >  3 files changed, 104 insertions(+), 37 deletions(-)
> > 
> 

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[John Crispin]

On 02/08/17 09:19, Zhi Mao wrote:
> Hi John, Matthais & Thierry,
>> Just a gentle ping on this issue again.
>> Do you have any update?
>>
>> Regards,
>> Zhi
Hi Zhi,

looks good to me

Acked-by: John Crispin <john@phrozen.org>

     John

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[John Crispin]

On 02/08/17 09:19, Zhi Mao wrote:
> Hi John, Matthais & Thierry,
>> Just a gentle ping on this issue again.
>> Do you have any update?
>>
>> Regards,
>> Zhi
Hi Zhi,

looks good to me

Acked-by: John Crispin <john@phrozen.org>

     John

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[John Crispin]

On 02/08/17 09:19, Zhi Mao wrote:
> Hi John, Matthais & Thierry,
>> Just a gentle ping on this issue again.
>> Do you have any update?
>>
>> Regards,
>> Zhi
Hi Zhi,

looks good to me

Acked-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>

     John



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

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Thanks John's reply.

Hi Thierry & Matthais,

What's your opinion?


On Wed, 2017-08-02 at 16:42 +0800, John Crispin wrote:
> 
> On 02/08/17 09:19, Zhi Mao wrote:
> > Hi John, Matthais & Thierry,
> >> Just a gentle ping on this issue again.
> >> Do you have any update?
> >>
> >> Regards,
> >> Zhi
> Hi Zhi,
> 
> looks good to me
> 
> Acked-by: John Crispin <john@phrozen.org>
> 
>      John
> 
> 
> 

[PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Thanks John's reply.

Hi Thierry & Matthais,

What's your opinion?


On Wed, 2017-08-02 at 16:42 +0800, John Crispin wrote:
> 
> On 02/08/17 09:19, Zhi Mao wrote:
> > Hi John, Matthais & Thierry,
> >> Just a gentle ping on this issue again.
> >> Do you have any update?
> >>
> >> Regards,
> >> Zhi
> Hi Zhi,
> 
> looks good to me
> 
> Acked-by: John Crispin <john@phrozen.org>
> 
>      John
> 
> 
> 

Re: [PATCH v3 0/6] mediatek: pwm driver add MT2712/MT7622 support

[Zhi Mao]

Thanks John's reply.

Hi Thierry & Matthais,

What's your opinion?


On Wed, 2017-08-02 at 16:42 +0800, John Crispin wrote:
> 
> On 02/08/17 09:19, Zhi Mao wrote:
> > Hi John, Matthais & Thierry,
> >> Just a gentle ping on this issue again.
> >> Do you have any update?
> >>
> >> Regards,
> >> Zhi
> Hi Zhi,
> 
> looks good to me
> 
> Acked-by: John Crispin <john-Pj+rj9U5foFAfugRpC6u6w@public.gmane.org>
> 
>      John
> 
> 
> 


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