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

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_perio=
d_ns` causes the generated hardware PWM period to exceed the requested peri=
od, violating the PWM framework constraint.
- [Medium] Disabling a PWM channel unconditionally clears the hardware FM m=
ode 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 lo=
g by repeatedly requesting valid PWM periods that trigger an expected fallb=
ack 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 <[email protected]>

    leds: rgb: leds-qcom-lpg: Add PWM frequency mode support
   =20
    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-qco=
m-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)
> =20
>  static const unsigned int lpg_clk_rates[] =3D {0, 1024, 32768, 19200000};
>  static const unsigned int lpg_clk_rates_hi_res[] =3D {0, 1024, 32768, 19=
200000, 76800000};
> +static const unsigned int lpg_clk_period_ns[] =3D {0, 976562, 30517, 52};

[Severity: Medium]
Will this truncated precision in lpg_clk_period_ns[] cause the generated ha=
rdware 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 tru=
ncating it (e.g., 1000000000 / 19200000 =3D 52.0833 ns truncated to 52 ns),=
 the division in lpg_calc_freq_fm() uses a smaller period. This yields a la=
rger multiplier, which can result in an actual hardware period that exceeds=
 the originally requested period.

[ ... ]
> +		for (exp =3D 0; exp <=3D LPG_MAX_M; exp++) {
> +			/* Calculate pwm_value_lsb for this (clk, exp) pair */
> +			lsb_tmp =3D period_ns;
> +			lsb_tmp =3D div64_u64(lsb_tmp, clk_period_arr[clk]);
> +			lsb_tmp >>=3D (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 i=
n 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 re=
peatedly requests valid PWM periods that happen to fall outside of the Freq=
uency Mode hardware limits.

> +
> +	chan->clk_sel =3D best_clk;
[ ... ]
> @@ -784,11 +891,23 @@ static void lpg_apply_dtest(struct lpg_channel *cha=
n)
>  		     chan->dtest_value);
>  }
> =20
> +static void lpg_apply_frequency_mode(struct lpg_channel *chan)
> +{
> +	struct lpg *lpg =3D 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 =3D false and calls lpg_=
apply().=20

Unlike lpg_apply_freq() or lpg_apply_pwm_value(), which check if (!chan->en=
abled) 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 ha=
rdware register values using the standard mode formulas.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-lpg-pwm-fm=
[email protected]?part=3D1