Re: [PATCH 4/5] arm64: dts: qcom: Add gp_mn pin state for GP M/N clock output

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The clock rate calculation logic fails to reject `N` and `M` combinations where `n <= 2 * m`, violating the hardware's duty cycle constraint and writing invalid states.
- [High] Invalid pinctrl function configuration for `kodiak.dtsi`. The `gp_mn` function is mapped to `gpio35`, but the SoC (SC7280) only supports it on `gpio60`.
- [Medium] `gp_mnd_clk_get_duty_cycle` incorrectly returns the low-phase duty cycle ratio, creating a broken, asymmetric API contract with `gp_mnd_clk_set_duty_cycle`.
- [Low] The 'gp_mn' function is not added to the devicetree bindings for the QCS8300, SA8775P, or SC7280 TLMM schemas, which will cause dtbs_check errors.
- [Low] The newly added pinctrl state nodes use underscores in their names (`gp_mn_active-state`), violating devicetree node naming conventions.
--

commit 39d6c11fb73944a2319cdb23d2516a67b5ac2e61
Author: Taniya Das <taniya.das@oss.qualcomm.com>

arm64: dts: qcom: Add gp_mn pin state for GP M/N clock output
    
This commit adds pinctrl states for the GP M/N divider clock output pin
across multiple Qualcomm SoCs. It wires the GP M/N clock controller node
via pinctrl-0 for the kodiak, lemans, and monaco device trees.

>     - kodiak (sa8775p): Add gp_mn_active state on gpio35 (gp_mn function).

[Severity: High]
The commit message indicates kodiak is an sa8775p board, but kodiak.dtsi is an
sc7280 device tree. Does the sc7280 SoC support the gp_mn function on gpio35,
or is it exclusively available on gpio60? Applying this DT state might cause a
runtime pinmux failure.

> diff --git a/arch/arm64/boot/dts/qcom/kodiak.dtsi b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> index fa540d8c2615d..1ff9e1598d004 100644
> --- a/arch/arm64/boot/dts/qcom/kodiak.dtsi
> +++ b/arch/arm64/boot/dts/qcom/kodiak.dtsi
> @@ -5908,6 +5908,13 @@ edp_hot_plug_det: edp-hot-plug-det-state {
>  				function = "edp_hot";
>  			};
>  
> +			gp_mn_active: gp_mn_active-state {

[Severity: Low]
Could this node name be updated to use hyphens instead of underscores to match
devicetree naming conventions?

> +				pins = "gpio35";
> +				function = "gp_mn";

[Severity: Low]
Is the gp_mn function missing from the devicetree binding schemas for the
SC7280 TLMM? This might cause dtbs_check errors.

> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/lemans.dtsi b/arch/arm64/boot/dts/qcom/lemans.dtsi
> index 353a6e6fd3acb..19f8cf4e15482 100644
> --- a/arch/arm64/boot/dts/qcom/lemans.dtsi
> +++ b/arch/arm64/boot/dts/qcom/lemans.dtsi
> @@ -6022,6 +6022,13 @@ dp1_hot_plug_det: dp1-hot-plug-det-state {
>  				bias-disable;
>  			};
>  
> +			gp_mn_active: gp_mn_active-state {

[Severity: Low]
Should this node name be updated to use hyphens instead of underscores to match
devicetree naming conventions?

> +				pins = "gpio35";
> +				function = "gp_mn";

[Severity: Low]
Might the gp_mn function be missing from the devicetree binding schemas for the
SA8775P TLMM? This could lead to dtbs_check errors.

> +				drive-strength = <2>;
> +				bias-disable;
> +			};
> +
[ ... ]
> diff --git a/arch/arm64/boot/dts/qcom/monaco.dtsi b/arch/arm64/boot/dts/qcom/monaco.dtsi
> index e4c8466f941bd..ebe5889daa530 100644
> --- a/arch/arm64/boot/dts/qcom/monaco.dtsi
> +++ b/arch/arm64/boot/dts/qcom/monaco.dtsi
> @@ -6433,6 +6433,13 @@ dp_hot_plug_det: dp-hot-plug-det-state {
>  				bias-disable;
>  			};
>  
> +			gp_mn_active: gp_mn_active-state {

[Severity: Low]
Are underscores permitted in this devicetree node name, or should they be
replaced with hyphens?

> +				pins = "gpio32";
> +				function = "gp_mn";

[Severity: Low]
Could the gp_mn function also be missing from the QCS8300 TLMM binding schemas?

> +				drive-strength = <2>;
> +				bias-disable;
> +			};


[Severity: High]
Are combinations where n <= 2 * m properly handled in the clock rate calculation
logic in gp_mnd_clk_determine_rate()?

drivers/clk/qcom/clk-gp-mnd.c:gp_mnd_clk_determine_rate() {
	...
	/* N = 2M + 1 leaves no valid D satisfying M < D < (N - M) */
	if (n == 2 * m + 1)
		return -EINVAL;
	...
}

If an invalid rate is not rejected here, gp_mnd_clk_set_rate() calculates
d_val = n / 2 and blind-writes it to the hardware, which violates the M < D
hardware requirement:

drivers/clk/qcom/clk-gp-mnd.c:gp_mnd_clk_set_rate() {
	...
	if (n == 2 * m + 1)
		return -EINVAL;

	...
	/* Program the closest-to-50% duty cycle. */
	d_val = n / 2;
	ret = regmap_write(gp->regmap, GP_MND_DUTY_REG, d_val);
	...
}


[Severity: Medium]
Will gp_mnd_clk_get_duty_cycle() incorrectly return the low-phase duty cycle
ratio instead of the high-phase ratio?

drivers/clk/qcom/clk-gp-mnd.c:gp_mnd_clk_get_duty_cycle() {
	...
	duty->num = d_val;
	duty->den = gp->n_val;

	return 0;
}

In gp_mnd_clk_set_duty_cycle(), duty->num is treated as the high-phase ratio
and the low-phase cycle count (d_val) is computed using duty->den - duty->num.
By returning d_val directly as duty->num in gp_mnd_clk_get_duty_cycle(), a
consumer setting a 75% duty cycle will read back 25%, which breaks the Common
Clock Framework semantics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-pdm_clk_gp_mnd_v1-v1-0-1522662b6c53@oss.qualcomm.com?part=4