Messages in this thread | | | Date | Tue, 11 Feb 2020 14:07:39 -0300 | From | Paul Cercueil <> | Subject | Re: [PATCH v3 1/3] pwm: jz4740: Use clocks from TCU driver |
| |
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
| |