| Message ID | 20200925155330.32301-2-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PWM i.MX27 fix disabled state for inverted signals | expand |
On Fri, Sep 25, 2020 at 05:53:26PM +0200, Marco Felsch wrote: > The current implementation enables the clock if the current PWM state > is '!enabled' to ensure the register access and left the clock on if the > new state is 'enabled'. Further apply calls don't enable the clock since > they relying on the fact the the clock is already running. Change this s/relying/rely/ > behaviour since it is not very intuitive. > > This commit changes this behaviour. Now the clocks are unconditional s/unconditional/unconditionally/ > enabled/disabled before/after the register access. If the PWM should be > turned on (state.enabled) we enable the clock again and vice versa if > the PWM should be turned off (!state.enabled). > > Therefore I added the enable member to the driver state struct since > the usage of cstate and pwm_get_state() is a layer violation. I removed > this violation while on it. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> I like it. Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards and thanks Uwe
Hello, On Fri, Sep 25, 2020 at 05:53:26PM +0200, Marco Felsch wrote: > The current implementation enables the clock if the current PWM state > is '!enabled' to ensure the register access and left the clock on if the > new state is 'enabled'. Further apply calls don't enable the clock since > they relying on the fact the the clock is already running. Change this > behaviour since it is not very intuitive. > > This commit changes this behaviour. Now the clocks are unconditional > enabled/disabled before/after the register access. If the PWM should be > turned on (state.enabled) we enable the clock again and vice versa if > the PWM should be turned off (!state.enabled). > > Therefore I added the enable member to the driver state struct since > the usage of cstate and pwm_get_state() is a layer violation. I removed > this violation while on it. while looking through patch 2 I found something missing here: You don't initialize .enabled in .probe(). Best regards Uwe
On 20-09-26 15:48, Uwe Kleine-König wrote: > Hello, > > On Fri, Sep 25, 2020 at 05:53:26PM +0200, Marco Felsch wrote: > > The current implementation enables the clock if the current PWM state > > is '!enabled' to ensure the register access and left the clock on if the > > new state is 'enabled'. Further apply calls don't enable the clock since > > they relying on the fact the the clock is already running. Change this > > behaviour since it is not very intuitive. > > > > This commit changes this behaviour. Now the clocks are unconditional > > enabled/disabled before/after the register access. If the PWM should be > > turned on (state.enabled) we enable the clock again and vice versa if > > the PWM should be turned off (!state.enabled). > > > > Therefore I added the enable member to the driver state struct since > > the usage of cstate and pwm_get_state() is a layer violation. I removed > > this violation while on it. > > while looking through patch 2 I found something missing here: > You don't initialize .enabled in .probe(). Arg.. you are right. Thanks for covering this. Didn't recognized it since I added this in patch 4/5. Any comments left on this series? Regards, Marco > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c index c50d453552bd..7edac4ac6395 100644 --- a/drivers/pwm/pwm-imx27.c +++ b/drivers/pwm/pwm-imx27.c @@ -91,6 +91,7 @@ struct pwm_imx27_chip { * value to return in that case. */ unsigned int duty_cycle; + bool enabled; }; #define to_pwm_imx27_chip(chip) container_of(chip, struct pwm_imx27_chip, chip) @@ -217,13 +218,14 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, { unsigned long period_cycles, duty_cycles, prescale; struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip); - struct pwm_state cstate; unsigned long long c; unsigned long long clkrate; int ret; u32 cr; - pwm_get_state(pwm, &cstate); + ret = pwm_imx27_clk_prepare_enable(imx); + if (ret) + return ret; clkrate = clk_get_rate(imx->clk_per); c = clkrate * state->period; @@ -251,15 +253,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, * Wait for a free FIFO slot if the PWM is already enabled, and flush * the FIFO if the PWM was disabled and is about to be enabled. */ - if (cstate.enabled) { + if (imx->enabled) pwm_imx27_wait_fifo_slot(chip, pwm); - } else { - ret = pwm_imx27_clk_prepare_enable(imx); - if (ret) - return ret; - + else pwm_imx27_sw_reset(chip); - } writel(duty_cycles, imx->mmio_base + MX3_PWMSAR); writel(period_cycles, imx->mmio_base + MX3_PWMPR); @@ -284,10 +281,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, writel(cr, imx->mmio_base + MX3_PWMCR); - if (!state->enabled) - pwm_imx27_clk_disable_unprepare(imx); + if (imx->enabled != state->enabled) { + if (state->enabled) { + ret = pwm_imx27_clk_prepare_enable(imx); + if (ret) + goto out; + } else { + pwm_imx27_clk_disable_unprepare(imx); + } + imx->enabled = state->enabled; + } - return 0; +out: + pwm_imx27_clk_disable_unprepare(imx); + + return ret; } static const struct pwm_ops pwm_imx27_ops = {
The current implementation enables the clock if the current PWM state is '!enabled' to ensure the register access and left the clock on if the new state is 'enabled'. Further apply calls don't enable the clock since they relying on the fact the the clock is already running. Change this behaviour since it is not very intuitive. This commit changes this behaviour. Now the clocks are unconditional enabled/disabled before/after the register access. If the PWM should be turned on (state.enabled) we enable the clock again and vice versa if the PWM should be turned off (!state.enabled). Therefore I added the enable member to the driver state struct since the usage of cstate and pwm_get_state() is a layer violation. I removed this violation while on it. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- v2: - use enable var which can be shared later on - remove cstate and pwm_get_state() layer violation - explicite enable/disable the clock twice if the pwm should be enabled/disabled rather than tracking the clock usage within the pwm_imx27_clk_prepare_enable() state. - rename commit message drivers/pwm/pwm-imx27.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)