| Message ID | 20250303-leds-qcom-lpg-fix-max-pwm-on-hi-res-v3-0-62703c0ab76a@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | leds: rgb: leds-qcom-lpg: PWM fixes | expand |
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
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
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,