lkml.org 
[lkml]   [2022]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 8/8] pwm: dwc: add PWM bit unset in get_state call
Hello,

On Fri, Aug 05, 2022 at 05:50:33PM +0100, Ben Dooks wrote:
> If we are not in PWM mode, then the output is technically a 50%
> output based on a single timer instead of the high-low based on
> the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
> and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.
>
> This may only be an issue on initialisation, as the rest of the
> code currently assumes we're always going to have the extended
> PWM mode using two counters.
>
> Signed-off-by: Ben Dooks <ben.dooks@sifive.com>
> ---
> drivers/pwm/pwm-dwc.c | 34 +++++++++++++++++++++++-----------
> 1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index 5edfb8f8acbf..49e666be7afd 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -171,23 +171,35 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct dwc_pwm *dwc = to_dwc_pwm(chip);
> u64 duty, period;
> + u32 ctrl, ld, ld2;
>
> pm_runtime_get_sync(chip->dev);
>
> - state->enabled = !!(dwc_pwm_readl(dwc,
> - DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
> + ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
> + ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> + ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
>
> - duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> - duty += 1;
> - duty *= dwc->clk_ns;
> - state->duty_cycle = duty;
> + state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
>
> - period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
> - period += 1;
> - period *= dwc->clk_ns;
> - period += duty;
> - state->period = period;
> + /* If we're not in PWM, technically the output is a 50-50

Huh, I expected checkpatch to warn about that. AFAIK the /* is supposed
to be on a line for itself?!

> + * based on the timer load-count only.
> + */
> + if (ctrl & DWC_TIM_CTRL_PWM) {
> + duty = ld;
> + duty += 1;
> + duty *= dwc->clk_ns;

I would prefer to write that as:

duty = (ld + 1) * dwc->clk_ns;

given that todays compilers are clever enough to optimize that just fine
and this version is better readable for humans.

> +
> + period = ld2;
> + period += 1;
> + period *= dwc->clk_ns;
> + period += duty;
> + } else {
> + duty = (ld + 1) * dwc->clk_ns;
> + period = duty * 2;
> + }
>
> + state->period = period;
> + state->duty_cycle = duty;
> state->polarity = PWM_POLARITY_INVERSED;
>
> pm_runtime_put_sync(chip->dev);
> --
> 2.35.1

I'm marking all patches in this series as "changes requested" even
though not all patches were commented. I assume that you continue to
care for all of them for the next revision. Please make sure to pass -v4
to git format-patch (or git send-email) then.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2022-08-06 12:23    [W:0.155 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site