The PWM Hi-Res allow configuring the PWM resolution from 8 bits PWM values up to 15 bits values. The current implementation loops through all possible resolutions (PWM sizes) on top of the already existing process of determining the prediv, exponent and refclk.
The first issue is that the maximum value used for capping is wrongly hardcoded.
The second issue is that it uses the wrong maximum possible PWM value for determining the best matched period.
Fix both.
Signed-off-by: Abel Vesa abel.vesa@linaro.org --- Changes in v2: - Re-worded the commit to drop the details that are not important w.r.t. what the patch is fixing. - Added another patch which fixes the resolution used for determining best matched period and PWM config. - Link to v1: https://lore.kernel.org/r/20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-v1-1-...
--- Abel Vesa (2): leds: rgb: leds-qcom-lpg: Fix pwm resolution max for Hi-Res PWMs leds: rgb: leds-qcom-lpg: Fix calculation of best period Hi-Res PWMs
drivers/leds/rgb/leds-qcom-lpg.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) --- base-commit: 8433c776e1eb1371f5cd40b5fd3a61f9c7b7f3ad change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b
Best regards,
Ideally, the requested duty cycle should never translate to a PWM value higher than the selected resolution (PWM size), but currently the best matched period is never reported back to the PWM consumer, so the consumer will still be using the requested period which is higher than the best matched one. This will result in PWM consumer requesting duty cycle values higher than the allowed PWM value.
Currently, the consumer driver known to fail this way is the PWM backlight (pwm_bl) and should be reworked in such a way that the best matched period is used instead.
As for the current implementation of the duty cycle calculation, it is capping the max value, fix that by using the resolution to figure out the maximum allowed PWM value.
Cc: stable@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.org --- drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate;
if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) { - max = LPG_RESOLUTION_15BIT - 1; + max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1; clk_rate = lpg_clk_rates_hi_res[chan->clk_sel]; } else { max = LPG_RESOLUTION_9BIT - 1;
On 2/26/2025 5:58 AM, Abel Vesa wrote:
Ideally, the requested duty cycle should never translate to a PWM value higher than the selected resolution (PWM size), but currently the best matched period is never reported back to the PWM consumer, so the consumer will still be using the requested period which is higher than the best matched one. This will result in PWM consumer requesting duty cycle values higher than the allowed PWM value.
Currently, the consumer driver known to fail this way is the PWM backlight (pwm_bl) and should be reworked in such a way that the best matched period is used instead.
As for the current implementation of the duty cycle calculation, it is capping the max value, fix that by using the resolution to figure out the maximum allowed PWM value.
Cc: stable@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.org
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;
I have a patch under review, https://lore.kernel.org/all/20250213003533.1684131-1-anjelique.melendez@oss...., which adds support for 6-bit resolution for regular PWM so this capping problem will also become an issue for regular PWM. I think it would make sense for you to include fixing the max for regular PWM here. Thoughts?
max = LPG_RESOLUTION_9BIT - 1;
On Wed, Feb 26, 2025 at 03:58:54PM +0200, Abel Vesa wrote:
Ideally, the requested duty cycle should never translate to a PWM value higher than the selected resolution (PWM size), but currently the best matched period is never reported back to the PWM consumer, so the consumer will still be using the requested period which is higher than the best matched one. This will result in PWM consumer requesting duty cycle values higher than the allowed PWM value.
Currently, the consumer driver known to fail this way is the PWM backlight (pwm_bl) and should be reworked in such a way that the best matched period is used instead.
As you're rebasing the patch (per Anjelique's addition of 6-bit pwm) I think you should omit this paragraph. Whether we like it or not, this seems to be the way the PWM api is designed, and there's no need to promise a redesign of the API in this bug fix.
As for the current implementation of the duty cycle calculation, it is capping the max value, fix that by using the resolution to figure out the maximum allowed PWM value.
Cc: stable@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.org
Reviewed-by: Bjorn Andersson andersson@kernel.org
Regards, Bjorn
drivers/leds/rgb/leds-qcom-lpg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index f3c9ef2bfa572f9ee86c8b8aa37deb8231965490..146cd9b447787bf170310321e939022dfb176e9f 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -529,7 +529,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty) unsigned int clk_rate; if (chan->subtype == LPG_SUBTYPE_HI_RES_PWM) {
max = LPG_RESOLUTION_15BIT - 1;
max = BIT(lpg_pwm_resolution_hi_res[chan->pwm_resolution_sel]) - 1;-- 2.34.1
When determining the actual best period by looping through all possible PWM configs, the resolution currently used is based on bit shift value which is off-by-one above the possible maximum PWM value allowed.
So subtract one from the resolution before determining the best period so that the maximum duty cycle requested by the PWM user won't result in a value above the maximum allowed.
Cc: stable@vger.kernel.org # 6.4 Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM") Signed-off-by: Abel Vesa abel.vesa@linaro.org --- drivers/leds/rgb/leds-qcom-lpg.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index 146cd9b447787bf170310321e939022dfb176e9f..5d8e27e2e7ae71c19637b90cc15eb363c53317d9 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -461,7 +461,7 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period) max_res = LPG_RESOLUTION_9BIT; }
- min_period = div64_u64((u64)NSEC_PER_SEC * (1 << pwm_resolution_arr[0]), + min_period = div64_u64((u64)NSEC_PER_SEC * ((1 << pwm_resolution_arr[0]) - 1), clk_rate_arr[clk_len - 1]); if (period <= min_period) return -EINVAL; @@ -482,7 +482,7 @@ static int lpg_calc_freq(struct lpg_channel *chan, uint64_t period) */
for (i = 0; i < pwm_resolution_count; i++) { - resolution = 1 << pwm_resolution_arr[i]; + resolution = (1 << pwm_resolution_arr[i]) - 1; for (clk_sel = 1; clk_sel < clk_len; clk_sel++) { u64 numerator = period * clk_rate_arr[clk_sel];
@@ -1291,7 +1291,7 @@ static int lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, if (ret) return ret;
- state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * (1 << resolution) * + state->period = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * ((1 << resolution) - 1) * pre_div * (1 << m), refclk); state->duty_cycle = DIV_ROUND_UP_ULL((u64)NSEC_PER_SEC * pwm_value * pre_div * (1 << m), refclk); } else {
linux-stable-mirror@lists.linaro.org
-
Abel Vesa -
Anjelique Melendez -
Bjorn Andersson