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

[Bryan O'Donoghue]

On 19/07/2026 14:59, sashiko-bot at kernel.org wrote:
> 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 at 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.

By design in fact as a result of a Sashiko review as I recall. Context 
window between runs seems required for this tool.

> 
>> 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.

Yeah, I guess a zero settle count should be legitimate.

> 
>> 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.

clock-names are required in the schema but sure I add a check for this.

This ping-pong with this tool is taking forever, I need to figure out 
how to run this locally to stop wasting my own time.

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