Re: [PATCH v20 2/6] pwm: driver for qualcomm ipq6018 pwm block

[George Moussalem]

Hi Uwe,

On 4/2/2026 5:35 PM, Uwe Kleine-König wrote:
> Hello,
> 
> I applied the patch and reviewed it in my editor. Here is the resulting
> diff:
> 
> diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> index b944ecb456d5..4818d0170d53 100644
> --- a/drivers/pwm/pwm-ipq.c
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -97,9 +97,10 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->polarity != PWM_POLARITY_NORMAL)
>  		return -EINVAL;
>  
> -	if (!ipq_chip->clk_rate)
> -		return -EINVAL;
> -
> +	/*
> +	 * XXX Why? A comment please. (Is this already covered by the checks
> +	 * below?)
> +	 */

This check can be safely removed as it is indeed covered by the check
where the period_ns is limited to IPQ_PWM_MAX_PERIOD_NS which equals to
NSEC_PER_SEC as per macro definition above.

>  	if (state->period < DIV64_U64_ROUND_UP(NSEC_PER_SEC,
>  					       ipq_chip->clk_rate))
>  		return -ERANGE;
> @@ -107,18 +108,29 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
>  	duty_ns = min(state->duty_cycle, period_ns);
>  
> +	/*
> +	 * Pick the maximal value for PWM_DIV that still allows a
> +	 * 100% relative duty cycle. This allows a fine grained
> +	 * selection of duty cycles.
> +	 */
>  	pwm_div = IPQ_PWM_MAX_DIV - 1;
> +
> +	/*
> +	 * XXX mul_u64_u64_div_u64 returns an u64, this might overflow the
> +	 * unsigned int pre_div.
> +	 */

Theoretically, yes, but in practice it won't due to above constraints.
Take the max period of 10^9 (NSEC_PER_SEC) * max clock rate of 10^9 (1
GHz), then the numerator becomes 10^18. Divide that by 10^9
(NSEC_PER_SEC) * 65,535 (IPQ_PWM_MAX_DIV) and that fits well into a
32-bit integer.

>  	pre_div = mul_u64_u64_div_u64(period_ns, ipq_chip->clk_rate,
>  				      (u64)NSEC_PER_SEC * (pwm_div + 1));
> -	pre_div = (pre_div > 0) ? pre_div - 1 : 0;
> +
> +	if (!pre_div)
> +		return -ERANGE;
> +
> +	pre_div -= 1;
>  
>  	if (pre_div > IPQ_PWM_MAX_DIV)
>  		pre_div = IPQ_PWM_MAX_DIV;
>  
> -	/*
> -	 * high duration = pwm duty * (pwm div + 1)
> -	 * pwm duty = duty_ns / period_ns
> -	 */
> +	/* pwm duty = HI_DUR * (PRE_DIV + 1) / clk_rate */
>  	hi_dur = mul_u64_u64_div_u64(duty_ns, ipq_chip->clk_rate,
>  				     (u64)(pre_div + 1) * NSEC_PER_SEC);
>  
> @@ -161,6 +173,10 @@ static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
>  
>  	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
> +
> +	/*
> +	 * effective_div <= 0x100000000, so the multiplication doesn't overflow.
> +	 */
>  	state->period = DIV64_U64_ROUND_UP(effective_div * NSEC_PER_SEC,
>  					   ipq_chip->clk_rate);
>  
> @@ -210,6 +226,8 @@ static int ipq_pwm_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret, "Failed to lock clock rate\n");
>  
>  	pwm->clk_rate = clk_get_rate(clk);
> +	if (!pwm->clk_rate)
> +		return dev_err_probe(dev, -EINVAL, "Failed due to clock rate being zero\n");
>  
>  	chip->ops = &ipq_pwm_ops;
>  
> 
> Comments with XXX need more code adaptions (or a comment why my concern
> isn't justified).

Do you want me to send a v21 or can you apply the diff in your tree with
above deletion and comment?

> 
> Best regards
> Uwe

Best regards,
George