Re: [PATCH v3 03/12] dt-bindings: clock: qcom: Add Qualcomm Shikra Display clock controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `clocks` and `clock-names` properties lack a `minItems` constraint after adding new clocks, breaking validation for legacy device trees.
- [High] Enum indices for GPLL0 parent clocks are swapped compared to the DT bindings, causing MDSS clocks to run at half frequency.
- [Medium] The driver globally modifies a static frequency table during probe based on the device compatible string, causing state contamination.
--

Hello,

I am an automated review bot. I have reviewed the patch series and found a
few regressions across the following commits.

commit 85e4a2f46a61d48ee9c66caba34fbc7a3fa61a61
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

dt-bindings: clock: qcom,qcm2290-dispcc: Add DSI1 PHY and sleep clocks
    
Update the QCM2290 DISPCC binding to document additional clock inputs
supported by the hardware, including DSI1 PHY byte/pixel clocks and
the sleep clock, alongside the existing clock list. This is an ABI
extension, and existing clock inputs ordering is unchanged.

> diff --git a/Documentation/devicetree/bindings/clock/qcom,qcm2290-dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,qcm2290-dispcc.yaml
> index 4a533b45eec2d..24f2cce033f6e 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,qcm2290-dispcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,qcm2290-dispcc.yaml
[ ... ]
> @@ -36,6 +39,9 @@ properties:
>        - const: gcc_disp_gpll0_div_clk_src
>        - const: dsi0_phy_pll_out_byteclk
>        - const: dsi0_phy_pll_out_dsiclk
> +      - const: dsi1_phy_pll_out_byteclk
> +      - const: dsi1_phy_pll_out_dsiclk
> +      - const: sleep_clk

[Severity: High]
Is it possible that omitting a minItems constraint for the clocks and
clock-names arrays breaks validation for legacy device trees?

In dt-schema, an items array without minItems defaults to expecting exactly
the maximum number of items. This causes make dtbs_check to strictly require
exactly 9 clocks for the qcom,qcm2290-dispcc compatible.

Could this break schema validation for legacy device trees that legally
provided only the original 6 clocks, violating DT backward compatibility
rules?

commit 6900a283b3a0edc157171145f5dc5bdc527dae67
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

clk: qcom: dispcc-qcm2290: Switch to DT index based clk lookup
    
Update the QCM2290 DISPCC driver to use the DT index based parent clock
lookup to align with the latest convention. While at it, fix the parent
data of mdss ahb/mdp clocks to use GPLL0 main output as per HW clock plan,
and update frequency table accordingly. Also, add the DSI1 PHY PLL input
clocks support.

> diff --git a/drivers/clk/qcom/dispcc-qcm2290.c b/drivers/clk/qcom/dispcc-qcm2290.c
> index 1c21267ae0f7a..f5dbd19d0a033 100644
> --- a/drivers/clk/qcom/dispcc-qcm2290.c
> +++ b/drivers/clk/qcom/dispcc-qcm2290.c
> @@ -24,6 +24,18 @@
>  #include "gdsc.h"
>  #include "reset.h"
>  
> +enum {
> +	DT_BI_TCXO,
> +	DT_BI_TCXO_AO,
> +	DT_GPLL0_OUT_DIV,
> +	DT_GPLL0,
> +	DT_DSI0_PHY_PLL_OUT_BYTECLK,
> +	DT_DSI0_PHY_PLL_OUT_DSICLK,
> +	DT_DSI1_PHY_PLL_OUT_BYTECLK,
> +	DT_DSI1_PHY_PLL_OUT_DSICLK,
> +	DT_SLEEP_CLK,
> +};
> +
[ ... ]
>  static const struct parent_map disp_cc_parent_map_2[] = {
>  	{ P_BI_TCXO_AO, 0 },
> -	{ P_GPLL0_OUT_DIV, 4 },
> +	{ P_GPLL0_OUT_MAIN, 4 },
>  };
>  
>  static const struct clk_parent_data disp_cc_parent_data_2[] = {
> -	{ .fw_name = "bi_tcxo_ao" },
> -	{ .fw_name = "gcc_disp_gpll0_div_clk_src" },
> +	{ .index = DT_BI_TCXO_AO },
> +	{ .index = DT_GPLL0 },

[Severity: High]
Are the enum indices for the GPLL0 parent clocks swapped compared to the DT
bindings?

The DT binding qcom,qcm2290-dispcc.yaml lists the main clock at index 2
(gcc_disp_gpll0_clk_src) and the div clock at index 3
(gcc_disp_gpll0_div_clk_src).

However, the enum mapping defines DT_GPLL0_OUT_DIV at index 2 and DT_GPLL0
at index 3. Due to this mismatch, DT_GPLL0 fetches the div clock instead
of the main clock.

By setting .index = DT_GPLL0, disp_cc_parent_data_2 incorrectly maps to the
divided output, halving the expected frequency for the MDSS AHB clocks.

[ ... ]
>  static const struct parent_map disp_cc_parent_map_3[] = {
> @@ -101,19 +115,21 @@ static const struct parent_map disp_cc_parent_map_3[] = {
>  };
>  
>  static const struct clk_parent_data disp_cc_parent_data_3[] = {
> -	{ .fw_name = "bi_tcxo" },
> +	{ .index = DT_BI_TCXO },
>  	{ .hw = &disp_cc_pll0.clkr.hw },
> -	{ .fw_name = "gcc_disp_gpll0_clk_src" },
> +	{ .index = DT_GPLL0 },

[Severity: High]
Could this also incorrectly map MDP clocks to the div clock because of the
swapped enum index?

commit 6798f69afeb9c9f6942dbf7fe2d0b171657b2f8f
Author: Imran Shaik <imran.shaik@oss.qualcomm.com>

clk: qcom: Add support for Qualcomm GPU Clock Controller on Shikra
    
The Qualcomm Shikra GPU clock controller is similar to QCM2290 GPUCC
hardware block, with minor differences. Hence add support for Shikra
GPUCC by extending the QCM2290 GPUCC driver.

> diff --git a/drivers/clk/qcom/gpucc-qcm2290.c b/drivers/clk/qcom/gpucc-qcm2290.c
> index 6e696cf672923..f43dd8231fb20 100644
> --- a/drivers/clk/qcom/gpucc-qcm2290.c
> +++ b/drivers/clk/qcom/gpucc-qcm2290.c
[ ... ]
> @@ -357,12 +368,16 @@ static const struct qcom_cc_desc gpu_cc_qcm2290_desc = {
>  
>  static const struct of_device_id gpu_cc_qcm2290_match_table[] = {
>  	{ .compatible = "qcom,qcm2290-gpucc" },
> +	{ .compatible = "qcom,shikra-gpucc" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, gpu_cc_qcm2290_match_table);
>  
>  static int gpu_cc_qcm2290_probe(struct platform_device *pdev)
>  {
> +	if (device_is_compatible(&pdev->dev, "qcom,shikra-gpucc"))
> +		gpu_cc_gx_gfx3d_clk_src.freq_tbl = ftbl_gpu_cc_gx_gfx3d_clk_src_shikra;
> +

[Severity: Medium]
Could updating the global static structure here cause state contamination?

If the compatible string is qcom,shikra-gpucc, it permanently mutates
gpu_cc_gx_gfx3d_clk_src.freq_tbl for the entire driver module. If the driver
is unbound and rebound to a different compatible device, or in a theoretical
multi-instance scenario, the frequency table remains permanently overwritten.

Would it be better to pass a device-specific qcom_cc_desc using
of_device_id.data or dynamically allocate the clock structure to avoid
static state contamination?

>  	return qcom_cc_probe(pdev, &gpu_cc_qcm2290_desc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-shikra-dispcc-gpucc-v3-0-61c1ba3735e8@oss.qualcomm.com?part=3