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

[George Moussalem <george.moussalem@outlook.com>]

Hello

On 4/13/2026 1:35 PM, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Apr 06, 2026 at 10:24:39PM +0200, George Moussalem via B4 Relay wrote:
>> From: Devi Priya <quic_devipriy@quicinc.com>
>>
>> Driver for the PWM block in Qualcomm IPQ6018 line of SoCs. Based on
>> driver from downstream Codeaurora kernel tree. Removed support for older
>> (V1) variants because I have no access to that hardware.
>>
>> Tested on IPQ5018 and IPQ6010 based hardware.
>>
>> Co-developed-by: Baruch Siach <baruch.siach@siklu.com>
>> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
>> Signed-off-by: Devi Priya <quic_devipriy@quicinc.com>
>> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
>> Signed-off-by: George Moussalem <george.moussalem@outlook.com>
> 
> I have a few remaining nitpicks. If you're ok I'll squash the following
> diff into this patch and apply it:

Just applied it to my own branch, all okay from my side. Thanks for your
guidance and support!

> 
> diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> index b79e5e457d1a..65af19ded72c 100644
> --- a/drivers/pwm/pwm-ipq.c
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
>   *
> - * Hardware notes / Limitations:
> + * Limitations:
>   * - The PWM controller has no publicly available datasheet.
>   * - Each of the four channels is programmed via two 32-bit registers
>   *   (REG0 and REG1 at 8-byte stride).
> 
> This is to make
> 
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> 
> do the right thing. I know "Limitations" isn't a good subject for this,
> but until I come around to pick a better marker, doing the same in all
> drivers is good.
> 
> @@ -44,13 +44,6 @@
>  
>  #define IPQ_PWM_REG1			4
>  #define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
> -
> -/*
> - * The max value specified for each field is based on the number of bits
> - * in the pwm control register for that field (16-bit)
> - */
> -#define IPQ_PWM_MAX_DIV			FIELD_MAX(IPQ_PWM_REG0_PWM_DIV)
> -
>  /*
>   * Enable bit is set to enable output toggling in pwm device.
>   * Update bit is set to trigger the change and is unset automatically
> @@ -59,6 +52,12 @@
>  #define IPQ_PWM_REG1_UPDATE		BIT(30)
>  #define IPQ_PWM_REG1_ENABLE		BIT(31)
>  
> +/*
> + * The max value specified for each field is based on the number of bits
> + * in the pwm control register for that field (16-bit)
> + */
> +#define IPQ_PWM_MAX_DIV			FIELD_MAX(IPQ_PWM_REG0_PWM_DIV)
> +
>  struct ipq_pwm_chip {
>  	void __iomem *mem;
>  	unsigned long clk_rate;
> 
> This is just about ordering definitions taken 1:1 from the manual before
> driver specific stuff.
> 
> @@ -95,6 +94,12 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long val = 0;
>  	unsigned long hi_dur;
>  
> +	if (!state->enabled) {
> +		/* clear IPQ_PWM_REG1_ENABLE */
> +		ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, IPQ_PWM_REG1_UPDATE);
> +		return 0;
> +	}
> +
>  	if (state->polarity != PWM_POLARITY_NORMAL)
>  		return -EINVAL;
>  
> This ensures that the PWM can be disabled even if state->polarity is
> bogus or period and duty_cycle are out of range.
> 
> @@ -102,7 +107,8 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * Check the upper and lower bounds for the period as per
>  	 * hardware limits
>  	 */
> -	period_ns = max(state->period, IPQ_PWM_MIN_PERIOD_NS);
> +	if (state->period < IPQ_PWM_MIN_PERIOD_NS)
> +		return -ERANGE;
>  	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
>  	duty_ns = min(state->duty_cycle, period_ns);
>  
> This is about correctness. A driver is expected to never configure a
> higher value than requested. (And otherwise I would have converted that
> to clamp().)
> 
> @@ -134,7 +140,7 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	/* 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);
> +				     (u64)NSEC_PER_SEC * (pre_div + 1));
>  
>  	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
>  		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
> 
> Just consistency with the period calculation
> 
> @@ -144,9 +150,7 @@ static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
>  
>  	/* PWM enable toggle needs a separate write to REG1 */
> -	val |= IPQ_PWM_REG1_UPDATE;
> -	if (state->enabled)
> -		val |= IPQ_PWM_REG1_ENABLE;
> +	val |= IPQ_PWM_REG1_UPDATE | IPQ_PWM_REG1_ENABLE;
>  	ipq_pwm_reg_write(pwm, IPQ_PWM_REG1, val);
>  
>  	return 0;
> 
> Simplification that is possible after checking for state->enabled early.
> 
> @@ -174,7 +178,7 @@ static int ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
>  	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
>  
> -	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
> +	effective_div = (u64)(pwm_div + 1) * (pre_div + 1)
>  
>  	/*
>  	 * effective_div <= 0x100000000, so the multiplication doesn't overflow.
> 
> Again consistency.
> 
> A nice followup for this patch would be the conversion to the waveform
> API; just in case you're still motivated to work on this driver :-)
> 
> Best regards
> Uwe

Best regards,
George