[PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver

[PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

The ingenic-timer "TCU" driver provides us with clocks, that can be
(un)gated, reparented or reclocked from devicetree, instead of having
these settings hardcoded in this driver.

Each PWM channel's clk pointer is stored in PWM chip data to keep
the code simple. The calls to arch-specific timer code is replaced with
standard clock API calls to start and stop each channel's clock.

While this driver is devicetree-compatible, it is never (as of now)
probed from devicetree, so this change does not introduce a ABI problem
with current devicetree files.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: This patch is now before the patch introducing regmap, so the code
    	has changed a bit.
    v3: - Use %pe printf specifier to print error
    	- Update commit message
    	- Removed call to jz4740_timer_set_ctrl() in jz4740_pwm_free() which
    	  was reseting the clock's parent.

 drivers/pwm/Kconfig      |  1 +
 drivers/pwm/pwm-jz4740.c | 53 +++++++++++++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index bd21655c37a6..15d3816341f3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -225,6 +225,7 @@ config PWM_IMX_TPM
 config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
+	depends on COMMON_CLK
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
 	  machines.
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 9d78cc21cb12..ee50ac5fabb6 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -24,7 +24,6 @@
 
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
-	struct clk *clk;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -34,6 +33,11 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+	struct clk *clk;
+	char clk_name[16];
+	int ret;
+
 	/*
 	 * Timers 0 and 1 are used for system tasks, so they are unavailable
 	 * for use as PWMs.
@@ -41,16 +45,32 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (pwm->hwpwm < 2)
 		return -EBUSY;
 
-	jz4740_timer_start(pwm->hwpwm);
+	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
+
+	clk = clk_get(chip->dev, clk_name);
+	if (IS_ERR(clk)) {
+		if (PTR_ERR(clk) != -EPROBE_DEFER)
+			dev_err(chip->dev, "Failed to get clock: %pe", clk);
+		return PTR_ERR(clk);
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		clk_put(clk);
+		return ret;
+	}
+
+	pwm_set_chip_data(pwm, clk);
 
 	return 0;
 }
 
 static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
+	struct clk *clk = pwm_get_chip_data(pwm);
 
-	jz4740_timer_stop(pwm->hwpwm);
+	clk_disable_unprepare(clk);
+	clk_put(clk);
 }
 
 static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -91,17 +111,22 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
 	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
+	struct clk *clk = pwm_get_chip_data(pwm),
+		   *parent_clk = clk_get_parent(clk);
+	unsigned long rate, period, duty;
 	unsigned long long tmp;
-	unsigned long period, duty;
 	unsigned int prescaler = 0;
 	uint16_t ctrl;
+	int err;
 
-	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
+	rate = clk_get_rate(parent_clk);
+	tmp = (unsigned long long)rate * state->period;
 	do_div(tmp, 1000000000);
 	period = tmp;
 
 	while (period > 0xffff && prescaler < 6) {
 		period >>= 2;
+		rate >>= 2;
 		++prescaler;
 	}
 
@@ -117,14 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	jz4740_pwm_disable(chip, pwm);
 
+	err = clk_set_rate(clk, rate);
+	if (err) {
+		dev_err(chip->dev, "Unable to set rate: %d", err);
+		return err;
+	}
+
 	jz4740_timer_set_count(pwm->hwpwm, 0);
 	jz4740_timer_set_duty(pwm->hwpwm, duty);
 	jz4740_timer_set_period(pwm->hwpwm, period);
 
-	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
-		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
-
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
 
 	switch (state->polarity) {
 	case PWM_POLARITY_NORMAL:
@@ -158,10 +187,6 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
 	if (!jz4740)
 		return -ENOMEM;
 
-	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
-	if (IS_ERR(jz4740->clk))
-		return PTR_ERR(jz4740->clk);
-
 	jz4740->chip.dev = &pdev->dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;
-- 
2.24.0

[PATCH v3 2/3] pwm: jz4740: Obtain regmap from parent node

[Paul Cercueil]

The TCU registers are shared between a handful of drivers, accessing
them through the same regmap.

While this driver is devicetree-compatible, it is never (as of now)
probed from devicetree, so this change does not introduce a ABI problem
with current devicetree files.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: This patch is now after the patch introducing TCU clocks, so the code
    	changed a bit.
    v3: No change

 drivers/pwm/Kconfig      |  1 +
 drivers/pwm/pwm-jz4740.c | 67 ++++++++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 15d3816341f3..5ed3fd7a167a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -226,6 +226,7 @@ config PWM_JZ4740
 	tristate "Ingenic JZ47xx PWM support"
 	depends on MACH_INGENIC
 	depends on COMMON_CLK
+	select MFD_SYSCON
 	help
 	  Generic PWM framework driver for Ingenic JZ47xx based
 	  machines.
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index ee50ac5fabb6..10d7af5c50b5 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -13,17 +13,19 @@
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/kernel.h>
+#include <linux/mfd/ingenic-tcu.h>
+#include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
-
-#include <asm/mach-jz4740/timer.h>
+#include <linux/regmap.h>
 
 #define NUM_PWM 8
 
 struct jz4740_pwm_chip {
 	struct pwm_chip chip;
+	struct regmap *map;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -75,36 +77,39 @@ static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->pwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
+
+	/* Enable PWM output */
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_EN, TCU_TCSR_PWM_EN);
 
-	ctrl |= JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
-	jz4740_timer_enable(pwm->hwpwm);
+	/* Start counter */
+	regmap_write(jz->map, TCU_REG_TESR, BIT(pwm->hwpwm));
 
 	return 0;
 }
 
 static void jz4740_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	uint32_t ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
+	struct jz4740_pwm_chip *jz = to_jz4740(chip);
 
 	/*
 	 * Set duty > period. This trick allows the TCU channels in TCU2 mode to
 	 * properly return to their init level.
 	 */
-	jz4740_timer_set_duty(pwm->hwpwm, 0xffff);
-	jz4740_timer_set_period(pwm->hwpwm, 0x0);
+	regmap_write(jz->map, TCU_REG_TDHRc(pwm->hwpwm), 0xffff);
+	regmap_write(jz->map, TCU_REG_TDFRc(pwm->hwpwm), 0x0);
 
 	/*
 	 * Disable PWM output.
 	 * In TCU2 mode (channel 1/2 on JZ4750+), this must be done before the
 	 * counter is stopped, while in TCU1 mode the order does not matter.
 	 */
-	ctrl &= ~JZ_TIMER_CTRL_PWM_ENABLE;
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
+	regmap_update_bits(jz->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_EN, 0);
 
 	/* Stop counter */
-	jz4740_timer_disable(pwm->hwpwm);
+	regmap_write(jz->map, TCU_REG_TECR, BIT(pwm->hwpwm));
 }
 
 static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -116,7 +121,6 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long rate, period, duty;
 	unsigned long long tmp;
 	unsigned int prescaler = 0;
-	uint16_t ctrl;
 	int err;
 
 	rate = clk_get_rate(parent_clk);
@@ -148,24 +152,32 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return err;
 	}
 
-	jz4740_timer_set_count(pwm->hwpwm, 0);
-	jz4740_timer_set_duty(pwm->hwpwm, duty);
-	jz4740_timer_set_period(pwm->hwpwm, period);
+	/* Reset counter to 0 */
+	regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
+
+	/* Set duty */
+	regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
 
-	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
-	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
+	/* Set period */
+	regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
 
+	/* Set abrupt shutdown */
+	regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+			   TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD);
+
+	/* Set polarity */
 	switch (state->polarity) {
 	case PWM_POLARITY_NORMAL:
-		ctrl &= ~JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+				   TCU_TCSR_PWM_INITL_HIGH, 0);
 		break;
 	case PWM_POLARITY_INVERSED:
-		ctrl |= JZ_TIMER_CTRL_PWM_ACTIVE_LOW;
+		regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm),
+				   TCU_TCSR_PWM_INITL_HIGH,
+				   TCU_TCSR_PWM_INITL_HIGH);
 		break;
 	}
 
-	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
-
 	if (state->enabled)
 		jz4740_pwm_enable(chip, pwm);
 
@@ -182,12 +194,19 @@ static const struct pwm_ops jz4740_pwm_ops = {
 static int jz4740_pwm_probe(struct platform_device *pdev)
 {
 	struct jz4740_pwm_chip *jz4740;
+	struct device *dev = &pdev->dev;
 
-	jz4740 = devm_kzalloc(&pdev->dev, sizeof(*jz4740), GFP_KERNEL);
+	jz4740 = devm_kzalloc(dev, sizeof(*jz4740), GFP_KERNEL);
 	if (!jz4740)
 		return -ENOMEM;
 
-	jz4740->chip.dev = &pdev->dev;
+	jz4740->map = device_node_to_regmap(dev->parent->of_node);
+	if (!jz4740->map) {
+		dev_err(dev, "regmap not found\n");
+		return -EINVAL;
+	}
+
+	jz4740->chip.dev = dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;
 	jz4740->chip.base = -1;
-- 
2.24.0

[PATCH v3 3/3] pwm: jz4740: Allow selection of PWM channels 0 and 1

[Paul Cercueil]

The TCU channels 0 and 1 were previously reserved for system tasks, and
thus unavailable for PWM.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Mathieu Malaterre <malat@debian.org>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

Notes:
    v2: No change
    v3: No change

 drivers/pwm/pwm-jz4740.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 10d7af5c50b5..9f0cb74cfef0 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -33,6 +33,19 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 	return container_of(chip, struct jz4740_pwm_chip, chip);
 }
 
+static bool jz4740_pwm_can_use_chn(struct jz4740_pwm_chip *jz,
+				   unsigned int channel)
+{
+	/* Enable all TCU channels for PWM use by default except channels 0/1 */
+	u32 pwm_channels_mask = GENMASK(NUM_PWM - 1, 2);
+
+	device_property_read_u32(jz->chip.dev->parent,
+				 "ingenic,pwm-channels-mask",
+				 &pwm_channels_mask);
+
+	return !!(pwm_channels_mask & BIT(channel));
+}
+
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
@@ -40,11 +53,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	char clk_name[16];
 	int ret;
 
-	/*
-	 * Timers 0 and 1 are used for system tasks, so they are unavailable
-	 * for use as PWMs.
-	 */
-	if (pwm->hwpwm < 2)
+	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
 		return -EBUSY;
 
 	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
-- 
2.24.0

Re: [PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

Hi,

Any feedback on this patchset?

-Paul


Le mar., déc. 10, 2019 at 16:27, Paul Cercueil <paul@crapouillou.net> 
a écrit :
> The ingenic-timer "TCU" driver provides us with clocks, that can be
> (un)gated, reparented or reclocked from devicetree, instead of having
> these settings hardcoded in this driver.
> 
> Each PWM channel's clk pointer is stored in PWM chip data to keep
> the code simple. The calls to arch-specific timer code is replaced 
> with
> standard clock API calls to start and stop each channel's clock.
> 
> While this driver is devicetree-compatible, it is never (as of now)
> probed from devicetree, so this change does not introduce a ABI 
> problem
> with current devicetree files.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> 
> Notes:
>     v2: This patch is now before the patch introducing regmap, so the 
> code
>     	has changed a bit.
>     v3: - Use %pe printf specifier to print error
>     	- Update commit message
>     	- Removed call to jz4740_timer_set_ctrl() in jz4740_pwm_free() 
> which
>     	  was reseting the clock's parent.
> 
>  drivers/pwm/Kconfig      |  1 +
>  drivers/pwm/pwm-jz4740.c | 53 
> +++++++++++++++++++++++++++++-----------
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index bd21655c37a6..15d3816341f3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MACH_INGENIC
> +	depends on COMMON_CLK
>  	help
>  	  Generic PWM framework driver for Ingenic JZ47xx based
>  	  machines.
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 9d78cc21cb12..ee50ac5fabb6 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -24,7 +24,6 @@
> 
>  struct jz4740_pwm_chip {
>  	struct pwm_chip chip;
> -	struct clk *clk;
>  };
> 
>  static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip 
> *chip)
> @@ -34,6 +33,11 @@ static inline struct jz4740_pwm_chip 
> *to_jz4740(struct pwm_chip *chip)
> 
>  static int jz4740_pwm_request(struct pwm_chip *chip, struct 
> pwm_device *pwm)
>  {
> +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> +	struct clk *clk;
> +	char clk_name[16];
> +	int ret;
> +
>  	/*
>  	 * Timers 0 and 1 are used for system tasks, so they are unavailable
>  	 * for use as PWMs.
> @@ -41,16 +45,32 @@ static int jz4740_pwm_request(struct pwm_chip 
> *chip, struct pwm_device *pwm)
>  	if (pwm->hwpwm < 2)
>  		return -EBUSY;
> 
> -	jz4740_timer_start(pwm->hwpwm);
> +	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
> +
> +	clk = clk_get(chip->dev, clk_name);
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) != -EPROBE_DEFER)
> +			dev_err(chip->dev, "Failed to get clock: %pe", clk);
> +		return PTR_ERR(clk);
> +	}
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		clk_put(clk);
> +		return ret;
> +	}
> +
> +	pwm_set_chip_data(pwm, clk);
> 
>  	return 0;
>  }
> 
>  static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device 
> *pwm)
>  {
> -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> +	struct clk *clk = pwm_get_chip_data(pwm);
> 
> -	jz4740_timer_stop(pwm->hwpwm);
> +	clk_disable_unprepare(clk);
> +	clk_put(clk);
>  }
> 
>  static int jz4740_pwm_enable(struct pwm_chip *chip, struct 
> pwm_device *pwm)
> @@ -91,17 +111,22 @@ static int jz4740_pwm_apply(struct pwm_chip 
> *chip, struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> +	struct clk *clk = pwm_get_chip_data(pwm),
> +		   *parent_clk = clk_get_parent(clk);
> +	unsigned long rate, period, duty;
>  	unsigned long long tmp;
> -	unsigned long period, duty;
>  	unsigned int prescaler = 0;
>  	uint16_t ctrl;
> +	int err;
> 
> -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
> +	rate = clk_get_rate(parent_clk);
> +	tmp = (unsigned long long)rate * state->period;
>  	do_div(tmp, 1000000000);
>  	period = tmp;
> 
>  	while (period > 0xffff && prescaler < 6) {
>  		period >>= 2;
> +		rate >>= 2;
>  		++prescaler;
>  	}
> 
> @@ -117,14 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip 
> *chip, struct pwm_device *pwm,
> 
>  	jz4740_pwm_disable(chip, pwm);
> 
> +	err = clk_set_rate(clk, rate);
> +	if (err) {
> +		dev_err(chip->dev, "Unable to set rate: %d", err);
> +		return err;
> +	}
> +
>  	jz4740_timer_set_count(pwm->hwpwm, 0);
>  	jz4740_timer_set_duty(pwm->hwpwm, duty);
>  	jz4740_timer_set_period(pwm->hwpwm, period);
> 
> -	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> -		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> -
> -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> +	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
> +	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> 
>  	switch (state->polarity) {
>  	case PWM_POLARITY_NORMAL:
> @@ -158,10 +187,6 @@ static int jz4740_pwm_probe(struct 
> platform_device *pdev)
>  	if (!jz4740)
>  		return -ENOMEM;
> 
> -	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
> -	if (IS_ERR(jz4740->clk))
> -		return PTR_ERR(jz4740->clk);
> -
>  	jz4740->chip.dev = &pdev->dev;
>  	jz4740->chip.ops = &jz4740_pwm_ops;
>  	jz4740->chip.npwm = NUM_PWM;
> --
> 2.24.0
> 

Re: [PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver

[Uwe Kleine-König]

hello Paul,

thanks for the ping, I lost your series from my radar.

On Tue, Dec 10, 2019 at 04:27:32PM +0100, Paul Cercueil wrote:
> The ingenic-timer "TCU" driver provides us with clocks, that can be
> (un)gated, reparented or reclocked from devicetree, instead of having
> these settings hardcoded in this driver.
> 
> Each PWM channel's clk pointer is stored in PWM chip data to keep
> the code simple. The calls to arch-specific timer code is replaced with
> standard clock API calls to start and stop each channel's clock.
> 
> While this driver is devicetree-compatible, it is never (as of now)
> probed from devicetree, so this change does not introduce a ABI problem
> with current devicetree files.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> 
> Notes:
>     v2: This patch is now before the patch introducing regmap, so the code
>     	has changed a bit.
>     v3: - Use %pe printf specifier to print error
>     	- Update commit message
>     	- Removed call to jz4740_timer_set_ctrl() in jz4740_pwm_free() which
>     	  was reseting the clock's parent.
> 
>  drivers/pwm/Kconfig      |  1 +
>  drivers/pwm/pwm-jz4740.c | 53 +++++++++++++++++++++++++++++-----------
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index bd21655c37a6..15d3816341f3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>  config PWM_JZ4740
>  	tristate "Ingenic JZ47xx PWM support"
>  	depends on MACH_INGENIC
> +	depends on COMMON_CLK
>  	help
>  	  Generic PWM framework driver for Ingenic JZ47xx based
>  	  machines.
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index 9d78cc21cb12..ee50ac5fabb6 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -24,7 +24,6 @@
>  
>  struct jz4740_pwm_chip {
>  	struct pwm_chip chip;
> -	struct clk *clk;
>  };
>  
>  static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
> @@ -34,6 +33,11 @@ static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
>  
>  static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
> +	struct clk *clk;
> +	char clk_name[16];
> +	int ret;
> +
>  	/*
>  	 * Timers 0 and 1 are used for system tasks, so they are unavailable
>  	 * for use as PWMs.
> @@ -41,16 +45,32 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  	if (pwm->hwpwm < 2)
>  		return -EBUSY;
>  
> -	jz4740_timer_start(pwm->hwpwm);
> +	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
> +
> +	clk = clk_get(chip->dev, clk_name);
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) != -EPROBE_DEFER)
> +			dev_err(chip->dev, "Failed to get clock: %pe", clk);
> +		return PTR_ERR(clk);
> +	}
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		clk_put(clk);
> +		return ret;
> +	}
> +
> +	pwm_set_chip_data(pwm, clk);
>  
>  	return 0;
>  }
>  
>  static void jz4740_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
> +	struct clk *clk = pwm_get_chip_data(pwm);
>  
> -	jz4740_timer_stop(pwm->hwpwm);
> +	clk_disable_unprepare(clk);
> +	clk_put(clk);
>  }
>  
>  static int jz4740_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -91,17 +111,22 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			    const struct pwm_state *state)
>  {
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
> +	struct clk *clk = pwm_get_chip_data(pwm),
> +		   *parent_clk = clk_get_parent(clk);
> +	unsigned long rate, period, duty;
>  	unsigned long long tmp;
> -	unsigned long period, duty;
>  	unsigned int prescaler = 0;
>  	uint16_t ctrl;
> +	int err;
>  
> -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * state->period;
> +	rate = clk_get_rate(parent_clk);
> +	tmp = (unsigned long long)rate * state->period;
>  	do_div(tmp, 1000000000);
>  	period = tmp;
>  
>  	while (period > 0xffff && prescaler < 6) {
>  		period >>= 2;
> +		rate >>= 2;
>  		++prescaler;
>  	}
>  
> @@ -117,14 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	jz4740_pwm_disable(chip, pwm);
>  
> +	err = clk_set_rate(clk, rate);
> +	if (err) {
> +		dev_err(chip->dev, "Unable to set rate: %d", err);
> +		return err;
> +	}

What I don't like here is that you use internal knowledge about the
clock to set a specific rate. Given this is a 1:1 conversion to hide the
previously contained register accesses behind the clk API (which is a
fine move) this is ok. But IMHO this should be cleaned up to rely on the
clk code. The more natural thing to do here would be to calculate the
rate you can work with and request that. Maybe add a todo-comment to
clean this up?

> +
>  	jz4740_timer_set_count(pwm->hwpwm, 0);
>  	jz4740_timer_set_duty(pwm->hwpwm, duty);
>  	jz4740_timer_set_period(pwm->hwpwm, period);
>  
> -	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT |
> -		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
> -
> -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
> +	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
> +	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>  
>  	switch (state->polarity) {
>  	case PWM_POLARITY_NORMAL:
> @@ -158,10 +187,6 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
>  	if (!jz4740)
>  		return -ENOMEM;
>  
> -	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
> -	if (IS_ERR(jz4740->clk))
> -		return PTR_ERR(jz4740->clk);

Huh, jz4740->clk was never enabled?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

Hi Uwe,


Le mar., févr. 11, 2020 at 17:46, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> hello Paul,
> 
> thanks for the ping, I lost your series from my radar.
> 
> On Tue, Dec 10, 2019 at 04:27:32PM +0100, Paul Cercueil wrote:
>>  The ingenic-timer "TCU" driver provides us with clocks, that can be
>>  (un)gated, reparented or reclocked from devicetree, instead of 
>> having
>>  these settings hardcoded in this driver.
>> 
>>  Each PWM channel's clk pointer is stored in PWM chip data to keep
>>  the code simple. The calls to arch-specific timer code is replaced 
>> with
>>  standard clock API calls to start and stop each channel's clock.
>> 
>>  While this driver is devicetree-compatible, it is never (as of now)
>>  probed from devicetree, so this change does not introduce a ABI 
>> problem
>>  with current devicetree files.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Mathieu Malaterre <malat@debian.org>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>> 
>>  Notes:
>>      v2: This patch is now before the patch introducing regmap, so 
>> the code
>>      	has changed a bit.
>>      v3: - Use %pe printf specifier to print error
>>      	- Update commit message
>>      	- Removed call to jz4740_timer_set_ctrl() in jz4740_pwm_free() 
>> which
>>      	  was reseting the clock's parent.
>> 
>>   drivers/pwm/Kconfig      |  1 +
>>   drivers/pwm/pwm-jz4740.c | 53 
>> +++++++++++++++++++++++++++++-----------
>>   2 files changed, 40 insertions(+), 14 deletions(-)
>> 
>>  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>  index bd21655c37a6..15d3816341f3 100644
>>  --- a/drivers/pwm/Kconfig
>>  +++ b/drivers/pwm/Kconfig
>>  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>>   config PWM_JZ4740
>>   	tristate "Ingenic JZ47xx PWM support"
>>   	depends on MACH_INGENIC
>>  +	depends on COMMON_CLK
>>   	help
>>   	  Generic PWM framework driver for Ingenic JZ47xx based
>>   	  machines.
>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>  index 9d78cc21cb12..ee50ac5fabb6 100644
>>  --- a/drivers/pwm/pwm-jz4740.c
>>  +++ b/drivers/pwm/pwm-jz4740.c
>>  @@ -24,7 +24,6 @@
>> 
>>   struct jz4740_pwm_chip {
>>   	struct pwm_chip chip;
>>  -	struct clk *clk;
>>   };
>> 
>>   static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip 
>> *chip)
>>  @@ -34,6 +33,11 @@ static inline struct jz4740_pwm_chip 
>> *to_jz4740(struct pwm_chip *chip)
>> 
>>   static int jz4740_pwm_request(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>  +	struct clk *clk;
>>  +	char clk_name[16];
>>  +	int ret;
>>  +
>>   	/*
>>   	 * Timers 0 and 1 are used for system tasks, so they are 
>> unavailable
>>   	 * for use as PWMs.
>>  @@ -41,16 +45,32 @@ static int jz4740_pwm_request(struct pwm_chip 
>> *chip, struct pwm_device *pwm)
>>   	if (pwm->hwpwm < 2)
>>   		return -EBUSY;
>> 
>>  -	jz4740_timer_start(pwm->hwpwm);
>>  +	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
>>  +
>>  +	clk = clk_get(chip->dev, clk_name);
>>  +	if (IS_ERR(clk)) {
>>  +		if (PTR_ERR(clk) != -EPROBE_DEFER)
>>  +			dev_err(chip->dev, "Failed to get clock: %pe", clk);
>>  +		return PTR_ERR(clk);
>>  +	}
>>  +
>>  +	ret = clk_prepare_enable(clk);
>>  +	if (ret) {
>>  +		clk_put(clk);
>>  +		return ret;
>>  +	}
>>  +
>>  +	pwm_set_chip_data(pwm, clk);
>> 
>>   	return 0;
>>   }
>> 
>>   static void jz4740_pwm_free(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>   {
>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
>>  +	struct clk *clk = pwm_get_chip_data(pwm);
>> 
>>  -	jz4740_timer_stop(pwm->hwpwm);
>>  +	clk_disable_unprepare(clk);
>>  +	clk_put(clk);
>>   }
>> 
>>   static int jz4740_pwm_enable(struct pwm_chip *chip, struct 
>> pwm_device *pwm)
>>  @@ -91,17 +111,22 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>>   			    const struct pwm_state *state)
>>   {
>>   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>  +	struct clk *clk = pwm_get_chip_data(pwm),
>>  +		   *parent_clk = clk_get_parent(clk);
>>  +	unsigned long rate, period, duty;
>>   	unsigned long long tmp;
>>  -	unsigned long period, duty;
>>   	unsigned int prescaler = 0;
>>   	uint16_t ctrl;
>>  +	int err;
>> 
>>  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
>> state->period;
>>  +	rate = clk_get_rate(parent_clk);
>>  +	tmp = (unsigned long long)rate * state->period;
>>   	do_div(tmp, 1000000000);
>>   	period = tmp;
>> 
>>   	while (period > 0xffff && prescaler < 6) {
>>   		period >>= 2;
>>  +		rate >>= 2;
>>   		++prescaler;
>>   	}
>> 
>>  @@ -117,14 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip 
>> *chip, struct pwm_device *pwm,
>> 
>>   	jz4740_pwm_disable(chip, pwm);
>> 
>>  +	err = clk_set_rate(clk, rate);
>>  +	if (err) {
>>  +		dev_err(chip->dev, "Unable to set rate: %d", err);
>>  +		return err;
>>  +	}
> 
> What I don't like here is that you use internal knowledge about the
> clock to set a specific rate. Given this is a 1:1 conversion to hide 
> the
> previously contained register accesses behind the clk API (which is a
> fine move) this is ok. But IMHO this should be cleaned up to rely on 
> the
> clk code. The more natural thing to do here would be to calculate the
> rate you can work with and request that. Maybe add a todo-comment to
> clean this up?

Yes, I am very aware of that - I had a patch to clean it up but we 
disagreed on the algorithm, so I left it for later, since these three 
patches are much more important for now. Not sure it needs a TODO, I 
plan to submit the cleanup patch once these are merged, but I can send 
a v4 if you prefer.

> 
>>  +
>>   	jz4740_timer_set_count(pwm->hwpwm, 0);
>>   	jz4740_timer_set_duty(pwm->hwpwm, duty);
>>   	jz4740_timer_set_period(pwm->hwpwm, period);
>> 
>>  -	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | JZ_TIMER_CTRL_SRC_EXT 
>> |
>>  -		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>>  -
>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
>>  +	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
>>  +	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>> 
>>   	switch (state->polarity) {
>>   	case PWM_POLARITY_NORMAL:
>>  @@ -158,10 +187,6 @@ static int jz4740_pwm_probe(struct 
>> platform_device *pdev)
>>   	if (!jz4740)
>>   		return -ENOMEM;
>> 
>>  -	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
>>  -	if (IS_ERR(jz4740->clk))
>>  -		return PTR_ERR(jz4740->clk);
> 
> Huh, jz4740->clk was never enabled?

Correct. The "ext" clock is an external oscillator that is always 
enabled, so the code just didn't bother calling clk_enable (but it's a 
bug of course).

Cheers,
-Paul

Re: [PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver

[Paul Cercueil]

Hi Uwe,

Le mar., févr. 11, 2020 at 14:07, Paul Cercueil <paul@crapouillou.net> 
a écrit :
> Hi Uwe,
> 
> 
> Le mar., févr. 11, 2020 at 17:46, Uwe Kleine-König 
> <u.kleine-koenig@pengutronix.de> a écrit :
>> hello Paul,
>> 
>> thanks for the ping, I lost your series from my radar.
>> 
>> On Tue, Dec 10, 2019 at 04:27:32PM +0100, Paul Cercueil wrote:
>>>  The ingenic-timer "TCU" driver provides us with clocks, that can be
>>>  (un)gated, reparented or reclocked from devicetree, instead of 
>>> \x7f\x7fhaving
>>>  these settings hardcoded in this driver.
>>> 
>>>  Each PWM channel's clk pointer is stored in PWM chip data to keep
>>>  the code simple. The calls to arch-specific timer code is replaced 
>>> \x7f\x7fwith
>>>  standard clock API calls to start and stop each channel's clock.
>>> 
>>>  While this driver is devicetree-compatible, it is never (as of now)
>>>  probed from devicetree, so this change does not introduce a ABI 
>>> \x7f\x7fproblem
>>>  with current devicetree files.
>>> 
>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>  Tested-by: Mathieu Malaterre <malat@debian.org>
>>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>>  ---
>>> 
>>>  Notes:
>>>      v2: This patch is now before the patch introducing regmap, so 
>>> \x7f\x7fthe code
>>>      	has changed a bit.
>>>      v3: - Use %pe printf specifier to print error
>>>      	- Update commit message
>>>      	- Removed call to jz4740_timer_set_ctrl() in 
>>> jz4740_pwm_free() \x7f\x7fwhich
>>>      	  was reseting the clock's parent.
>>> 
>>>   drivers/pwm/Kconfig      |  1 +
>>>   drivers/pwm/pwm-jz4740.c | 53 
>>> \x7f\x7f+++++++++++++++++++++++++++++-----------
>>>   2 files changed, 40 insertions(+), 14 deletions(-)
>>> 
>>>  diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>>  index bd21655c37a6..15d3816341f3 100644
>>>  --- a/drivers/pwm/Kconfig
>>>  +++ b/drivers/pwm/Kconfig
>>>  @@ -225,6 +225,7 @@ config PWM_IMX_TPM
>>>   config PWM_JZ4740
>>>   	tristate "Ingenic JZ47xx PWM support"
>>>   	depends on MACH_INGENIC
>>>  +	depends on COMMON_CLK
>>>   	help
>>>   	  Generic PWM framework driver for Ingenic JZ47xx based
>>>   	  machines.
>>>  diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
>>>  index 9d78cc21cb12..ee50ac5fabb6 100644
>>>  --- a/drivers/pwm/pwm-jz4740.c
>>>  +++ b/drivers/pwm/pwm-jz4740.c
>>>  @@ -24,7 +24,6 @@
>>> 
>>>   struct jz4740_pwm_chip {
>>>   	struct pwm_chip chip;
>>>  -	struct clk *clk;
>>>   };
>>> 
>>>   static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip 
>>> \x7f\x7f*chip)
>>>  @@ -34,6 +33,11 @@ static inline struct jz4740_pwm_chip 
>>> \x7f\x7f*to_jz4740(struct pwm_chip *chip)
>>> 
>>>   static int jz4740_pwm_request(struct pwm_chip *chip, struct 
>>> \x7f\x7fpwm_device *pwm)
>>>   {
>>>  +	struct jz4740_pwm_chip *jz = to_jz4740(chip);
>>>  +	struct clk *clk;
>>>  +	char clk_name[16];
>>>  +	int ret;
>>>  +
>>>   	/*
>>>   	 * Timers 0 and 1 are used for system tasks, so they are 
>>> \x7f\x7funavailable
>>>   	 * for use as PWMs.
>>>  @@ -41,16 +45,32 @@ static int jz4740_pwm_request(struct pwm_chip 
>>> \x7f\x7f*chip, struct pwm_device *pwm)
>>>   	if (pwm->hwpwm < 2)
>>>   		return -EBUSY;
>>> 
>>>  -	jz4740_timer_start(pwm->hwpwm);
>>>  +	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
>>>  +
>>>  +	clk = clk_get(chip->dev, clk_name);
>>>  +	if (IS_ERR(clk)) {
>>>  +		if (PTR_ERR(clk) != -EPROBE_DEFER)
>>>  +			dev_err(chip->dev, "Failed to get clock: %pe", clk);
>>>  +		return PTR_ERR(clk);
>>>  +	}
>>>  +
>>>  +	ret = clk_prepare_enable(clk);
>>>  +	if (ret) {
>>>  +		clk_put(clk);
>>>  +		return ret;
>>>  +	}
>>>  +
>>>  +	pwm_set_chip_data(pwm, clk);
>>> 
>>>   	return 0;
>>>   }
>>> 
>>>   static void jz4740_pwm_free(struct pwm_chip *chip, struct 
>>> \x7f\x7fpwm_device *pwm)
>>>   {
>>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, 0);
>>>  +	struct clk *clk = pwm_get_chip_data(pwm);
>>> 
>>>  -	jz4740_timer_stop(pwm->hwpwm);
>>>  +	clk_disable_unprepare(clk);
>>>  +	clk_put(clk);
>>>   }
>>> 
>>>   static int jz4740_pwm_enable(struct pwm_chip *chip, struct 
>>> \x7f\x7fpwm_device *pwm)
>>>  @@ -91,17 +111,22 @@ static int jz4740_pwm_apply(struct pwm_chip 
>>> \x7f\x7f*chip, struct pwm_device *pwm,
>>>   			    const struct pwm_state *state)
>>>   {
>>>   	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>>>  +	struct clk *clk = pwm_get_chip_data(pwm),
>>>  +		   *parent_clk = clk_get_parent(clk);
>>>  +	unsigned long rate, period, duty;
>>>   	unsigned long long tmp;
>>>  -	unsigned long period, duty;
>>>   	unsigned int prescaler = 0;
>>>   	uint16_t ctrl;
>>>  +	int err;
>>> 
>>>  -	tmp = (unsigned long long)clk_get_rate(jz4740->clk) * 
>>> \x7f\x7fstate->period;
>>>  +	rate = clk_get_rate(parent_clk);
>>>  +	tmp = (unsigned long long)rate * state->period;
>>>   	do_div(tmp, 1000000000);
>>>   	period = tmp;
>>> 
>>>   	while (period > 0xffff && prescaler < 6) {
>>>   		period >>= 2;
>>>  +		rate >>= 2;
>>>   		++prescaler;
>>>   	}
>>> 
>>>  @@ -117,14 +142,18 @@ static int jz4740_pwm_apply(struct pwm_chip 
>>> \x7f\x7f*chip, struct pwm_device *pwm,
>>> 
>>>   	jz4740_pwm_disable(chip, pwm);
>>> 
>>>  +	err = clk_set_rate(clk, rate);
>>>  +	if (err) {
>>>  +		dev_err(chip->dev, "Unable to set rate: %d", err);
>>>  +		return err;
>>>  +	}
>> 
>> What I don't like here is that you use internal knowledge about the
>> clock to set a specific rate. Given this is a 1:1 conversion to hide 
>> \x7fthe
>> previously contained register accesses behind the clk API (which is a
>> fine move) this is ok. But IMHO this should be cleaned up to rely on 
>> \x7fthe
>> clk code. The more natural thing to do here would be to calculate the
>> rate you can work with and request that. Maybe add a todo-comment to
>> clean this up?
> 
> Yes, I am very aware of that - I had a patch to clean it up but we 
> disagreed on the algorithm, so I left it for later, since these three 
> patches are much more important for now. Not sure it needs a TODO, I 
> plan to submit the cleanup patch once these are merged, but I can 
> send a v4 if you prefer.

Should I respin this?

Cheers,
-Paul


>>>  +
>>>   	jz4740_timer_set_count(pwm->hwpwm, 0);
>>>   	jz4740_timer_set_duty(pwm->hwpwm, duty);
>>>   	jz4740_timer_set_period(pwm->hwpwm, period);
>>> 
>>>  -	ctrl = JZ_TIMER_CTRL_PRESCALER(prescaler) | 
>>> JZ_TIMER_CTRL_SRC_EXT \x7f\x7f|
>>>  -		JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>>>  -
>>>  -	jz4740_timer_set_ctrl(pwm->hwpwm, ctrl);
>>>  +	ctrl = jz4740_timer_get_ctrl(pwm->hwpwm);
>>>  +	ctrl |= JZ_TIMER_CTRL_PWM_ABBRUPT_SHUTDOWN;
>>> 
>>>   	switch (state->polarity) {
>>>   	case PWM_POLARITY_NORMAL:
>>>  @@ -158,10 +187,6 @@ static int jz4740_pwm_probe(struct 
>>> \x7f\x7fplatform_device *pdev)
>>>   	if (!jz4740)
>>>   		return -ENOMEM;
>>> 
>>>  -	jz4740->clk = devm_clk_get(&pdev->dev, "ext");
>>>  -	if (IS_ERR(jz4740->clk))
>>>  -		return PTR_ERR(jz4740->clk);
>> 
>> Huh, jz4740->clk was never enabled?
> 
> Correct. The "ext" clock is an external oscillator that is always 
> enabled, so the code just didn't bother calling clk_enable (but it's 
> a bug of course).
> 
> Cheers,
> -Paul
>