Re: [PATCH v2] leds: rgb: leds-qcom-lpg: Add PWM frequency mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The use of truncated, pre-calculated constants in `lpg_clk_period_ns` causes the generated hardware PWM period to exceed the requested period, violating the PWM framework constraint.
- [Medium] Disabling a PWM channel unconditionally clears the hardware FM mode bit but leaves other frequency registers unchanged, causing subsequent get_state() calls to report incorrect values.
- [Medium] Unprivileged user space or client drivers can spam the kernel log by repeatedly requesting valid PWM periods that trigger an expected fallback path from FM mode to standard mode.
--

--- Patch [1]: [PATCH v2] leds: rgb: leds-qcom-lpg: Add PWM frequency mode support ---
commit 130380cf6f5abd5ee895e64c041b5980974d3c55
Author: Fenglin Wu <fenglin.wu@oss.qualcomm.com>

    leds: rgb: leds-qcom-lpg: Add PWM frequency mode support
    
    Certain PWM channels on a PMIC (e.g. PM8350C PWM4) support a Frequency
    Mode (FM) that can generate waveforms with more frequency points than
    the standard LPG PWM mode. The trade-off is that the duty cycle can
    only be fixed at 50%. Add the FM support. When the PWM channel is
    requested to set a duty cycle to exactly 50%, use FM mode by default
    as it provides a finer-grained frequency resolution in that case.

> diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> index d7d6518de30f2..4d39cb8f21673 100644
> --- a/drivers/leds/rgb/leds-qcom-lpg.c
> +++ b/drivers/leds/rgb/leds-qcom-lpg.c
[ ... ]
> @@ -413,10 +425,105 @@ static int lpg_lut_sync(struct lpg *lpg, unsigned int mask)
>  
>  static const unsigned int lpg_clk_rates[] = {0, 1024, 32768, 19200000};
>  static const unsigned int lpg_clk_rates_hi_res[] = {0, 1024, 32768, 19200000, 76800000};
> +static const unsigned int lpg_clk_period_ns[] = {0, 976562, 30517, 52};

[Severity: Medium]
Will this truncated precision in lpg_clk_period_ns[] cause the generated hardware period to exceed the requested period?

The PWM framework requires the generated hardware period to be equal to or less than the requested period. By pre-calculating the clock period and truncating it (e.g., 1000000000 / 19200000 = 52.0833 ns truncated to 52 ns), the division in lpg_calc_freq_fm() uses a smaller period. This yields a larger multiplier, which can result in an actual hardware period that exceeds the originally requested period.

[ ... ]
> +		for (exp = 0; exp <= LPG_MAX_M; exp++) {
> +			/* Calculate pwm_value_lsb for this (clk, exp) pair */
> +			lsb_tmp = period_ns;
> +			lsb_tmp = div64_u64(lsb_tmp, clk_period_arr[clk]);
> +			lsb_tmp >>= (exp + 1);
[ ... ]
> +	if (!found) {
> +		dev_err(chan->lpg->dev,
> +			"FM: Cannot generate period %llu ns\n", period_ns);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Is it necessary to log a dev_err() here when a period cannot be generated in Frequency Mode?

Looking at lpg_pwm_apply() below, when this fails, it explicitly falls back to standard mode gracefully without failing the overall PWM request. This could cause kernel log spam if a user-space application or client driver repeatedly requests valid PWM periods that happen to fall outside of the Frequency Mode hardware limits.

> +
> +	chan->clk_sel = best_clk;
[ ... ]
> @@ -784,11 +891,23 @@ static void lpg_apply_dtest(struct lpg_channel *chan)
>  		     chan->dtest_value);
>  }
>  
> +static void lpg_apply_frequency_mode(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg = chan->lpg;
> +
> +	if (!chan->fm_capable)
> +		return;
> +
> +	regmap_write(lpg->map, chan->base + PWM_FM_MODE_REG,
> +		     chan->use_fm ? PWM_FM_ENABLE : 0);
> +}

[Severity: Medium]
Does this unconditionally clear the FM mode bit when a channel is disabled?

In lpg_pwm_apply() when disabling a channel (state->enabled is false), use_fm starts as false. The function sets chan->use_fm = false and calls lpg_apply(). 

Unlike lpg_apply_freq() or lpg_apply_pwm_value(), which check if (!chan->enabled) and return early, lpg_apply_frequency_mode() will execute the regmap_write() and clear the PWM_FM_MODE_REG.

This leaves the other frequency and PWM value registers untouched. When lpg_pwm_get_state() is later called to read the state of the disabled channel, it will see FM mode is disabled and incorrectly interpret the lingering hardware register values using the standard mode formulas.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-lpg-pwm-fm-support-v2-1-bc4d1041eeda@oss.qualcomm.com?part=1