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

[Bryan O'Donoghue]

On 18/07/2026 03:51, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Incorrect translation of logical to physical lanes causes register offset errors and bitwise truncation.
> - [High] Integer truncation in division leads to potential division-by-zero panic in settle count calculation.
> - [Low] Hardware version register read does not mask upper bits, potentially corrupting the version struct.
> --
> 
> --- Patch [2]: [PATCH v10 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver ---
> commit 3a4ef513bbeefe5185b0f08d343f98883cbe66ca
> Author: Bryan O'Donoghue <bryan.odonoghue at linaro.org>
> 
> phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver
> 
> This commit adds a new MIPI CSI2 driver in DPHY mode initially. It supports
> devices like "qcom,x1e80100-csi2-phy" and imports CAMSS CSI PHY init sequences.
> 
>> 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..5ac0d041b36b7
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> 
> [ ... ]
> 
>> +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]
> Is it possible for the hardware register to return garbage or reserved bits in
> the upper bytes here? Since this first read is assigned directly without
> masking (e.g., tmp & 0xFF), any upper garbage bits would remain in hw_version
> and wouldn't be cleared by the subsequent bitwise OR operations.

This is not credible.

>> +
>> +	tmp = readl_relaxed(csi2phy->base +
>> +			    CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->common_regs_offset, 13));
>> +	csi2phy->hw_version |= (tmp << 8) & 0xFF00;
> 
> [ ... ]
> 
>> +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;
>> +
>> +	if (link_freq <= 0)
>> +		return 0;
>> +
>> +	ui_ps = div_u64(PSEC_PER_SEC, link_freq);
> 
> [Severity: High]
> Does this code silently truncate link_freq?
> 
> The div_u64() macro expects a u32 divisor, but link_freq is an s64. This will
> implicitly truncate the 64-bit link_freq value to 32 bits.
> 
> If link_freq happens to be an exact multiple of 2^32, the truncated divisor
> becomes exactly 0, which would cause a division-by-zero panic. Frequencies
> exceeding 4.29 GHz would also be silently corrupted.
> 
> Could this be updated to use div64_u64() or similar to avoid truncation?

This is a pretty outlandish level of defensive coding. Nope, not doing it.

> 
>> +	ui_ps /= 2;
>> +	t_hs_prepare_max_ps = 85000 + 6 * ui_ps;
> 
> [ ... ]
> 
>> +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;
>> +
>> +	/*
>> +	 * CSI_COMMON_CTRL5 is a physical lane power-up bitmap:
>> +	 * - Bits [0,2,4,6] → D-PHY data lanes(LN0, LN2, LN4, LN6)
>> +	 * - Bits [1,3,5] → C-PHY trio lanes(LN1, LN3, LN5)
>> +	 * - Bit [7] → D-PHY clock lane(LNCK) dedicated clock enable
>> +	 */
>> +	val = BIT(lane_cfg->clk.pos);
>> +	for (i = 0; i < cfg->num_data_lanes; i++)
>> +		val |= BIT(lane_cfg->data[i].pos * 2);
> 
> [Severity: High]
> Can this bitmask overflow the u8 val?
> 
> If the device tree uses physical indices (e.g., 4 or 6) for pos, pos * 2
> becomes 8 or 12. Since val is a u8, using BIT(8) or BIT(12) will overflow
> and silently truncate to 0, failing to enable the corresponding hardware lane.
> 

Actually no - the current yaml has a range of data-lanes = <0 1 2 3> => 
3 * 2 == 6

>> +
>> +	writel(val, csi2phy->base +
>> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->common_regs_offset, 5));
>> +
>> +	/* Lane configuration for polarity @ CSIPHY-base + CTRL9 */
>> +	for (i = 0; i < cfg->num_data_lanes; i++) {
>> +		if (lane_cfg->data[i].pol) {
>> +			u8 pos = lane_cfg->data[i].pos;
>> +
>> +			writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(pos));
> 
> [Severity: High]
> Does this correctly map logical to physical lanes?
> 
> The CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(pos) macro computes 0x200 * pos + 0x24.
> However, D-PHY data lane blocks are spaced at offsets 0, 0x400, 0x800, and
> 0xC00.
> 
> If pos is a logical index like 1, this would access offset 0x200 (which is a
> C-PHY lane) instead of the intended 0x400.

True.

Baby don't be sad, cause 1/4 ain't bad.
> 
>> +		}
>> +	}
>> +
>> +	if (lane_cfg->clk.pol)
>> +		writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(lane_cfg->clk.pos));
>