Hello, On Mon, Apr 06, 2026 at 10:24:39PM +0200, George Moussalem via B4 Relay wrote: > From: Devi Priya > > 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 > Signed-off-by: Baruch Siach > Signed-off-by: Devi Priya > Reviewed-by: Bjorn Andersson > Signed-off-by: George Moussalem 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