Re: [PATCH v9 3/6] pwm: dwc: add PWM bit unset in get_state call

[Ben Dooks]

On 22/09/2023 18:35, Uwe Kleine-König wrote:
> Hello,
>
> [dropping William Salmon and Jude Onyenegecha from Cc: as in the other
> mails before]
>
> I'd change the Subject to:
>
> 	pwm: dwc: Support DWC_TIM_CTRL_PWM unset in .get_state()
>
> On Thu, Sep 07, 2023 at 05:12:39PM +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@xxxxxxxxxxxxxxx>
> > ---
> > v9:
> >   - fixed multi-line comment
> >   - put authour back to codethink email from sifive
> > v8:
> >   - fixed rename issues
> > v4:
> >   - fixed review comment on mulit-line calculations
> > ---
> >   drivers/pwm/pwm-dwc-core.c | 30 +++++++++++++++++++-----------
> >   1 file changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-dwc-core.c b/drivers/pwm/pwm-dwc-core.c
> > index 4b4b7b9e1d82..3fc281a78c9a 100644
> > --- a/drivers/pwm/pwm-dwc-core.c
> > +++ b/drivers/pwm/pwm-dwc-core.c
> > @@ -122,24 +122,32 @@ static int 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 + 1) * dwc->clk_ns;
> > +		period = (ld2 + 1)  * dwc->clk_ns;
> > +		period += duty;
> > +	} else {
> > +		duty = (ld + 1) * dwc->clk_ns;
> > +		period = duty * 2;
> > +	}
> >   
> >   	state->polarity = PWM_POLARITY_INVERSED;
> > +	state->period = period;
> > +	state->duty_cycle = duty;
> >   
> >   	pm_runtime_put_sync(chip->dev);
>
> The change looks right,
>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
>
> Do you intend to address the review feedback for the other patches in
> this series? It would be sad if you efforts didn't result in these
> improvements getting in.

I'm going to try and get through the review comments this week,
I've been ill and then on leave so not had any time to look at
this.


--
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html