linux-kernel - Re: [PATCH v9 3/6] pwm: dwc: add PWM bit unset in get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Sep 2023 19:35:56 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Ben Dooks <ben.dooks@...ethink.co.uk>
Cc:     linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Greentime Hu <greentime.hu@...ive.com>,
        jarkko.nikula@...ux.intel.com
Subject: Re: [PATCH v9 3/6] pwm: dwc: add PWM bit unset in get_state call

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@...ethink.co.uk>
> ---
> 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@...gutronix.de>

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)