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

[Uwe Kleine-König]

Hello,

On Mon, Apr 06, 2026 at 10:24:39PM +0200, George Moussalem via B4 Relay wrote:
> From: Devi Priya <quic_devipriy@xxxxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Baruch Siach <baruch.siach@xxxxxxxxx>
> Signed-off-by: Devi Priya <quic_devipriy@xxxxxxxxxxx>
> Reviewed-by: Bjorn Andersson <andersson@xxxxxxxxxx>
> Signed-off-by: George Moussalem <george.moussalem@xxxxxxxxxxx>

I have a few remaining nitpicks. If you're ok I'll squash the following
diff into this patch and apply it:

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

Attachment: signature.asc
Description: PGP signature