[PATCH v3 0/3] leds: rgb: leds-qcom-lpg: PWM fixes

[PATCH v3 0/3] leds: rgb: leds-qcom-lpg: PWM fixes

[Abel Vesa]

The PWM allow configuring the PWM resolution from 8 bits PWM
values up to 15 bits values, for the Hi-Res PWMs, and then either
6-bit or 9-bit for the normal PWMs. The current implementation loops
through all possible resolutions (PWM sizes), for the PWM subtype, on top
of the already existing process of determining the prediv, exponent and
refclk.

The first and second issues are related to capping the computed PWM
value.

The third issue is that it uses the wrong maximum possible PWM
value for determining the best matched period.

Fix all of them.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v3:
- Added a new patch that fixes the normal PWMs, since they now support
  6-bit resolution as well. Added it as first patch.
- Re-worded the second patch. Included Bjorn's suggestion and R-b tag.
- Link to v2: https://lore.kernel.org/r/20250226-leds-qcom-lpg-fix-max-pwm-on-hi-res-v2-0-7af5ef5d220b@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-a161ec670ea5@linaro.org

---
Abel Vesa (3):
      leds: rgb: leds-qcom-lpg: Fix pwm resolution max for normal PWMs
      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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
---
base-commit: cd3215bbcb9d4321def93fea6cfad4d5b42b9d1d
change-id: 20250220-leds-qcom-lpg-fix-max-pwm-on-hi-res-067e8782a79b

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>

[PATCH v3 1/3] leds: rgb: leds-qcom-lpg: Fix pwm resolution max for normal PWMs

[Abel Vesa]

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.

In case of normal PWMs (non Hi-Res), the current implementation is
capping the PWM value at a 9-bit resolution, even when the 6-bit
resolution is selected.

Fix the issue by capping the PWM value to the maximum value allowed by
the selected resolution.

Fixes: 7a3350495d9a ("leds: rgb: leds-qcom-lpg: Add support for 6-bit PWM resolution")
Suggested-by: Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>
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 4e5c56ded1f0412c9913670699e912b24f3408bd..4454fc6a38480b61916318dd170f3eddc32976d6 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -533,7 +533,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
 		max = LPG_RESOLUTION_15BIT - 1;
 		clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
 	} else {
-		max = LPG_RESOLUTION_9BIT - 1;
+		max = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;
 		clk_rate = lpg_clk_rates[chan->clk_sel];
 	}
 

-- 
2.34.1

Re: [PATCH v3 1/3] leds: rgb: leds-qcom-lpg: Fix pwm resolution max for normal PWMs

[Sebastian Reichel]

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

Hi,

On Mon, Mar 03, 2025 at 01:52:50PM +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.
> 
> In case of normal PWMs (non Hi-Res), the current implementation is
> capping the PWM value at a 9-bit resolution, even when the 6-bit
> resolution is selected.
> 
> Fix the issue by capping the PWM value to the maximum value allowed by
> the selected resolution.
> 
> Fixes: 7a3350495d9a ("leds: rgb: leds-qcom-lpg: Add support for 6-bit PWM resolution")
> Suggested-by: Anjelique Melendez <anjelique.melendez@oss.qualcomm.com>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

Reviewed-by: Sebastian Reichel <sre@kernel.org>

Greetings,

-- Sebastian

>  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 4e5c56ded1f0412c9913670699e912b24f3408bd..4454fc6a38480b61916318dd170f3eddc32976d6 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -533,7 +533,7 @@ static void lpg_calc_duty(struct lpg_channel *chan, uint64_t duty)
>  		max = LPG_RESOLUTION_15BIT - 1;
>  		clk_rate = lpg_clk_rates_hi_res[chan->clk_sel];
>  	} else {
> -		max = LPG_RESOLUTION_9BIT - 1;
> +		max = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;
>  		clk_rate = lpg_clk_rates[chan->clk_sel];
>  	}
>  
> 
> -- 
> 2.34.1
> 

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

[PATCH v3 2/3] leds: rgb: leds-qcom-lpg: Fix pwm resolution max for Hi-Res PWMs

[Abel Vesa]

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.

In case of Hi-Res PWMs, the current implementation is capping the PWM
value at a 15-bit resolution, even when the lower resolutions are
selected.

Fix the issue by capping the PWM value to the maximum value allowed by
the selected resolution.

Cc: stable@vger.kernel.org    # 6.4
Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
Reviewed-by: Bjorn Andersson <andersson@kernel.org>
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 4454fc6a38480b61916318dd170f3eddc32976d6..0b6310184988c299d82ee7181982c03d306407a4 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -530,7 +530,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 = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;

-- 
2.34.1

Re: [PATCH v3 2/3] leds: rgb: leds-qcom-lpg: Fix pwm resolution max for Hi-Res PWMs

[Sebastian Reichel]

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

Hi,

On Mon, Mar 03, 2025 at 01:52:51PM +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.
> 
> In case of Hi-Res PWMs, the current implementation is capping the PWM
> value at a 15-bit resolution, even when the lower resolutions are
> selected.
> 
> Fix the issue by capping the PWM value to the maximum value allowed by
> the selected resolution.
> 
> Cc: stable@vger.kernel.org    # 6.4
> Fixes: b00d2ed37617 ("leds: rgb: leds-qcom-lpg: Add support for high resolution PWM")
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---

Reviewed-by: Sebastian Reichel <sre@kernel.org>

Greetings,

-- Sebastian

>  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 4454fc6a38480b61916318dd170f3eddc32976d6..0b6310184988c299d82ee7181982c03d306407a4 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -530,7 +530,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 = BIT(lpg_pwm_resolution[chan->pwm_resolution_sel]) - 1;
> 
> -- 
> 2.34.1
> 

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

[PATCH v3 3/3] leds: rgb: leds-qcom-lpg: Fix calculation of best period Hi-Res PWMs

[Abel Vesa]

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 0b6310184988c299d82ee7181982c03d306407a4..4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -462,7 +462,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;
@@ -483,7 +483,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];
 
@@ -1292,7 +1292,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 {

-- 
2.34.1

Re: [PATCH v3 3/3] leds: rgb: leds-qcom-lpg: Fix calculation of best period Hi-Res PWMs

[Sebastian Reichel]

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

Hi,

On Mon, Mar 03, 2025 at 01:52:52PM +0200, Abel Vesa wrote:
> 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>
> ---

Reviewed-by: Sebastian Reichel <sre@kernel.org>

Greetings,

-- Sebastian

>  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 0b6310184988c299d82ee7181982c03d306407a4..4f2a178e3d265a2cc88e651d3e2ca6ae3dfac2e2 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> @@ -462,7 +462,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;
> @@ -483,7 +483,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];
>  
> @@ -1292,7 +1292,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 {
> 
> -- 
> 2.34.1
> 

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

Re: [PATCH v3 0/3] leds: rgb: leds-qcom-lpg: PWM fixes

[Uwe Kleine-König]

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

Hello,

On Mon, Mar 03, 2025 at 01:52:49PM +0200, Abel Vesa wrote:
> The PWM allow configuring the PWM resolution from 8 bits PWM
> values up to 15 bits values, for the Hi-Res PWMs, and then either
> 6-bit or 9-bit for the normal PWMs. The current implementation loops
> through all possible resolutions (PWM sizes), for the PWM subtype, on top
> of the already existing process of determining the prediv, exponent and
> refclk.
> 
> The first and second issues are related to capping the computed PWM
> value.

I just took a very quick look. I'd say squash the first and second patch
into a single one. Splitting a change that fixes the same issue in the
two branches of an if condition has no benefit.

Other than that this patch set would also benefit from what I wrote in
the review of the other patch you send: Please mention a request where
the result becomes wrong. This considerably simplifies understanding
your changes.

Thanks
Uwe

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

Re: [PATCH v3 0/3] leds: rgb: leds-qcom-lpg: PWM fixes

[Abel Vesa]

On 25-03-04 07:29:46, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Mar 03, 2025 at 01:52:49PM +0200, Abel Vesa wrote:
> > The PWM allow configuring the PWM resolution from 8 bits PWM
> > values up to 15 bits values, for the Hi-Res PWMs, and then either
> > 6-bit or 9-bit for the normal PWMs. The current implementation loops
> > through all possible resolutions (PWM sizes), for the PWM subtype, on top
> > of the already existing process of determining the prediv, exponent and
> > refclk.
> > 
> > The first and second issues are related to capping the computed PWM
> > value.
> 
> I just took a very quick look. I'd say squash the first and second patch
> into a single one. Splitting a change that fixes the same issue in the
> two branches of an if condition has no benefit.

Actually, the first two patches fix different commits.
The first patch fixes a commit that is only in linux-next for now,
while the second patch fixes a commit that has been merged in 6.4.

So they need to be separate patches.

> 
> Other than that this patch set would also benefit from what I wrote in
> the review of the other patch you send: Please mention a request where
> the result becomes wrong. This considerably simplifies understanding
> your changes.

Sure. Will describe the 5ms vs 4.26ms period scenario. Hope that's OK.

> 
> Thanks
> Uwe

Thanks for reviewing,
Abel