| Message ID | 20220805165033.140958-9-ben.dooks@sifive.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/8] dt-bindings: pwm: Document Synopsys DesignWare snps,pwm-dw-apb-timers-pwm2 | expand |
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
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 + * based on the timer load-count only. + */ + if (ctrl & DWC_TIM_CTRL_PWM) { + duty = ld; + duty += 1; + duty *= dwc->clk_ns; + + 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);
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(-)