Re: [PATCH 2/5] clk: qcom: Add a driver for PDM GP_MN fractional clock divider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] `gp_mnd_clk_get_duty_cycle` incorrectly returns the low-phase ratio instead of the high-phase ratio, violating the CCF API.
- [High] `determine_rate` and `set_rate` fail to reject configurations where `N <= 2M`, leading to invalid hardware states and integer underflow.
- [Medium] Relying on `parent_data.index = 0` is brittle and may select the wrong parent clock from the DT node.
--

commit 7c3592c8935c3d0c132fc4ece0d8bc638c3fc17a
Author: Taniya Das <taniya.das@oss.qualcomm.com>

clk: qcom: Add a driver for PDM GP_MN fractional clock divider

This commit adds a standalone platform driver for the PDM GP_MN clock
divider. It provides rate and duty-cycle configuration by programming
the MDIV, NDIV, and DUTY registers.

> diff --git a/drivers/clk/qcom/clk-gp-mnd.c b/drivers/clk/qcom/clk-gp-mnd.c
> new file mode 100644
> index 0000000000000..826b6b62ddc7b
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-gp-mnd.c

[ ... ]

> +static int gp_mnd_clk_determine_rate(struct clk_hw *hw,
> +				     struct clk_rate_request *req)
> +{
> +	unsigned long m = 0, n = 0;
> +
> +	rational_best_approximation(req->rate, req->best_parent_rate,
> +				    (unsigned long)GP_MND_MAX_M,
> +				    (unsigned long)GP_MND_MAX_N,
> +				    &m, &n);
> +
> +	if (!m || !n)
> +		return -EINVAL;
> +
> +	/* N = 2M + 1 leaves no valid D satisfying M < D < (N - M) */
> +	if (n == 2 * m + 1)
> +		return -EINVAL;

[Severity: High]
Does this check fail to reject all invalid configurations where n <= 2 * m?

The hardware constraint M < D < (N - M) requires N >= 2M + 2, but this code
only checks for exact equality.

[ ... ]

> +static int gp_mnd_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +		unsigned long parent_rate)
> +{
> +	struct clk_gp_mnd *gp = to_clk_gp_mnd(hw);
> +	unsigned long m = 0, n = 0;
> +	unsigned int d_val, n_val;
> +	int ret;

[ ... ]

> +	/*
> +	 * When N = 2M + 1 the valid D range [M+1, M] is empty; no duty
> +	 * cycle can satisfy M < D < (N - M).  Reject before touching hw.
> +	 */
> +	if (n == 2 * m + 1)
> +		return -EINVAL;
> +
> +	ret = clk_prepare_enable(gp->pdm_ahb_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(gp->regmap, GP_MND_MDIV_REG, m);
> +	if (ret)
> +		goto err_unprepare;
> +
> +	/* N divider holds the 1's complement of (N - M), N_WIDTH bits wide */
> +	n_val = ~(n - m) & GP_MND_MAX_N;
> +	ret = regmap_write(gp->regmap, GP_MND_NDIV_REG, n_val);

[Severity: High]
Can this unsigned underflow when the requested rate is higher than the
parent rate, causing m > n?

Since n <= 2 * m is not fully rejected above, (n - m) could wrap around and
program a corrupted value into GP_MND_NDIV_REG.

[ ... ]

> +static int gp_mnd_clk_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
> +{
> +	struct clk_gp_mnd *gp = to_clk_gp_mnd(hw);
> +	unsigned int d_val;
> +	int ret;
> +
> +	if (!gp->n_val) {
> +		duty->num = 1;
> +		duty->den = 2;
> +		return 0;
> +	}
> +
> +	ret = clk_prepare_enable(gp->pdm_ahb_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(gp->regmap, GP_MND_DUTY_REG, &d_val);
> +
> +	clk_disable_unprepare(gp->pdm_ahb_clk);
> +
> +	if (ret)
> +		return ret;
> +
> +	duty->num = d_val;

[Severity: High]
Does this report an inverted duty cycle to the clock framework?

The hardware DUTY register holds the low-phase clock cycle count:

gp_mnd_clk_set_duty_cycle() {
    ...
    /* D = (1 - duty) * N, giving the low-phase count */
    d_val = DIV_ROUND_UP((u64)(duty->den - duty->num) * gp->n_val, duty->den);
    ...
}

Assigning d_val directly to duty->num here appears to return the low-phase
ratio instead of the active high-phase ratio.

[ ... ]

> +static int clk_gp_mnd_probe(struct platform_device *pdev)
> +{
> +	struct clk_parent_data parent_data = { .index = 0 };

[Severity: Medium]
Can this select the wrong parent clock if ahb_clk is listed first in the
device tree?

Relying on .index = 0 might cause the framework to use the AHB bus clock
instead of pdm_clk for rate calculations. Would it be safer to use
.fw_name = "pdm_clk" to deterministically bind the parent?

[ ... ]

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