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

Hi

comments inline

quoted

+static int mtk_pwm_clk_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{

quoted

+	ret = clk_prepare_enable(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
+	if (ret < 0) {
+		clk_disable_unprepare(pc->clks[MTK_CLK_MAIN]);
+		clk_disable_unprepare(pc->clks[MTK_CLK_TOP]);
+		return ret;
+	}
+

Rather than disabling the already prepared clks in each error path and 
then returning, you should use goto err_clk_{top,main,pwm1} in the same 
style as what this patch removes from mtk_pwm_probe()

quoted

+	return ret;
+}

quoted

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

quoted

@@ -91,10 +128,12 @@ static int mtk_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
  	if (clkdiv > 7)
  		return -EINVAL;
  
-	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | BIT(3) | clkdiv);
+	mtk_pwm_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);

this chunk needs to go into its own patch

quoted

@@ -102,11 +141,8 @@ static int mtk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
  {
  
-	ret = clk_prepare(pc->clks[MTK_CLK_PWM1 + pwm->hwpwm]);
-	if (ret < 0)
-		return ret;
+	mtk_pwm_clk_enable(chip, pwm);

You need to check the error code here and return if clk enabling failed

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

quoted

  
  static int mtk_pwm_remove(struct platform_device *pdev)
  {
  	struct mtk_pwm_chip *pc = platform_get_drvdata(pdev);
-	unsigned int i;
-
-	for (i = 0; i < pc->chip.npwm; i++)
-		pwm_disable(&pc->chip.pwms[i]);

why are you removing this chunk ?

     John

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

Regards,
Zhi

quoted

  
  	return pwmchip_remove(&pc->chip);
  }