Re: [PATCH v18 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:
- [Medium] phy: mipi-csi2: valid settle_cnt of 0 is incorrectly treated as an error
- [Low] phy: mipi-csi2: missing bitmask on hardware version read
- [Low] phy: mipi-csi2: unused clk_names array and incomplete clock validation
--

commit 96f989cd7293176051d565c67a575abc0e3f18e1
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver

This commit adds a new MIPI CSI2 DPHY driver for Qualcomm platforms,
supporting the "qcom,x1e80100-csi2-phy" device. It imports existing
CAMSS CSI PHY initialization sequences to configure data and clock lanes.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> index 0000000000000..966d79c98f9b2 100644
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> @@ -0,0 +1,385 @@
[ ... ]
> +static void phy_qcom_mipi_csi2_hw_version_read(struct mipi_csi2phy_device *csi2phy)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +	u32 tmp;
> +
> +	writel(CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID, csi2phy->base +
> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->common_regs_offset, 6));
> +
> +	tmp = readl_relaxed(csi2phy->base +
> +			    CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->common_regs_offset, 12));
> +	csi2phy->hw_version = tmp;

[Severity: Low]
Does this read allow non-version bits to corrupt the final hardware version
value?

The subsequent reads for bytes 1-3 apply a bitmask (e.g., & 0xFF00) to
discard garbage in the upper bits, but this first read assigns the value
directly. Could stray bits in the higher registers from offset 12 corrupt
the assembled version string?

[ ... ]
> +static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
> +{
> +	u32 t_hs_prepare_max_ps;
> +	u32 timer_period_ps;
> +	u32 t_hs_settle_ps;
> +	u8 settle_cnt;
> +	u32 ui_ps;
> +
> +	ui_ps = div64_u64(PSEC_PER_SEC, link_freq);
> +	ui_ps /= 2;
> +	t_hs_prepare_max_ps = 85000 + 6 * ui_ps;
> +	t_hs_settle_ps = t_hs_prepare_max_ps;
> +
> +	timer_period_ps = div_u64(PSEC_PER_SEC, timer_clk_rate);
> +
> +	if ((t_hs_settle_ps / timer_period_ps) < 6)
> +		return 0;
> +
> +	settle_cnt = t_hs_settle_ps / timer_period_ps - 6;
> +
> +	return settle_cnt;
> +}
[ ... ]
> +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;
> +
> +	if (cfg->link_freq <= 0)
> +		return -EINVAL;
> +
> +	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
> +	if (!settle_cnt)
> +		return -ENODEV;

[Severity: Medium]
Will this erroneously reject valid hardware configurations?

The calculation in phy_qcom_mipi_csi2_settle_cnt_calc() evaluates to 0 when
the ratio is exactly 6, but it also returns 0 when the ratio is less than 6
to indicate an error.

Since phy_qcom_mipi_csi2_lanes_enable() checks if (!settle_cnt), does this
overlap cause a valid configuration to be spuriously rejected as an error?

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> index 0000000000000..c0a4fa20eaf0c 100644
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> @@ -0,0 +1,463 @@
[ ... ]
> +static int phy_qcom_mipi_csi2_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	for (i = 0; i < num_clk; i++) {
> +		if (!csi2phy->clks[i].id)
> +			return dev_err_probe(dev, -EINVAL, "Missing clock-names\n");
> +
> +		if (!strcmp(csi2phy->clks[i].id, "timer")) {
> +			csi2phy->timer_clk = csi2phy->clks[i].clk;
> +			break;
> +		}
> +	}

[Severity: Low]
Does this loop exit early upon finding the timer clock, failing to validate
the remaining dynamically loaded clocks?

Because devm_clk_bulk_get_all() does not use the carefully defined
soc_cfg->clk_names array, breaking early here appears to bypass expected
clock validation. Would the driver blindly accept misnamed clocks instead of
enforcing the expected platform clock list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917-x1e-csi2-phy-v18-0-6515b5255fa9@linaro.org?part=2