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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2582 bytes --]

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?)
+	 */
 	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.
+	 */
 	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).

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]