Re: [PATCH v12 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Valid calculated settle_cnt of 0 is erroneously rejected as an error, causing PHY initialization to fail.
- [High] Potential NULL pointer dereference in clock name comparison during probe when parsing malformed device trees.
- [Medium] Missing PM_OPP Kconfig dependency causes unconditional runtime probe failures.
--

--- Patch [2]: [PATCH v12 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver ---
commit 7fdedc509474184e47a62af468ae08b71f96dfd5
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Subject: phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

This commit adds a new MIPI CSI2 driver in DPHY mode. It imports existing
CAMSS CSI PHY initialization sequences to support devices like the
qcom,x1e80100-csi2-phy.

> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index 60a0ead127fa9..779a3511ba852 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -28,6 +28,20 @@ config PHY_QCOM_EDP
>  	  Enable this driver to support the Qualcomm eDP PHY found in various
>  	  Qualcomm chipsets.
>  
> +config PHY_QCOM_MIPI_CSI2
> +	tristate "Qualcomm MIPI CSI2 PHY driver"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on OF
> +	depends on PM
> +	depends on COMMON_CLK
> +	select GENERIC_PHY
> +	select GENERIC_PHY_MIPI_DPHY

[Severity: Medium]
Should this Kconfig entry select or depend on PM_OPP?

Without CONFIG_PM_OPP enabled, the stub for devm_pm_opp_of_add_table()
used during probe will return -EOPNOTSUPP, causing the driver probe to
unconditionally fail with an "invalid OPP table" error.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c

[ ... ]

> +static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
> +					   struct mipi_csi2phy_stream_cfg *cfg)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
> +	u8 settle_cnt;
> +	u8 val;
> +	int i;
> +
> +	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
> +	if (!settle_cnt)
> +		return -EINVAL;

[Severity: High]
Does this incorrectly reject a valid settle count of zero?

Looking at the math in phy_qcom_mipi_csi2_settle_cnt_calc():

phy_qcom_mipi_csi2_settle_cnt_calc() {
        ...
        settle_cnt = t_hs_settle_ps / timer_period_ps - 6;
        return settle_cnt;
}

If the link frequency and timer clock rate yield exactly 6 timer periods
for t_hs_settle, the calculation results in 0. By rejecting 0 here,
legitimate MIPI CSI2 configurations might fail to initialize.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c

[ ... ]

> +static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = devm_clk_bulk_get_all(dev, &csi2phy->clks);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get clocks\n");
> +
> +	if (num_clk != ret) {
> +		return dev_err_probe(dev, -ENODEV, "clock count %d expected %d\n",
> +				     ret, num_clk);
> +	}
> +
> +	for (i = 0; i < num_clk; i++) {
> +		if (!strcmp(csi2phy->clks[i].id, "timer")) {

[Severity: High]
Could csi2phy->clks[i].id be NULL here?

The clk_bulk_get_all() infrastructure can leave the .id field as NULL if
the device tree node has a "clocks" property but is missing "clock-names".
Calling strcmp() on a NULL pointer will cause a kernel panic during probe
when parsing malformed device trees.

> +			csi2phy->timer_clk = csi2phy->clks[i].clk;
> +			break;
> +		}
> +	}
> +	if (!csi2phy->timer_clk)
> +		return dev_err_probe(dev, -ENODEV, "no timer clock\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260719-x1e-csi2-phy-v12-0-cc7311326d1b@linaro.org?part=2