Re: [PATCH v13 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 2 potential issue(s) to consider:
- [High] Valid calculated settle_cnt of 0 is treated as an error
- [Medium] Polarity configuration overwrites the entire CTRL9 register instead of using read-modify-write
--

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

Add a new MIPI CSI2 driver in DPHY mode initially. The entire set of
existing CAMSS CSI PHY init sequences are imported in order to save time
and effort in later patches.

> 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;
> +
> +	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: High]
Should a return value of 0 from phy_qcom_mipi_csi2_settle_cnt_calc() be
considered valid?

If the timing parameters yield exactly 6 for t_hs_settle_ps / timer_period_ps,
the calculation evaluates to 0. This then causes the lane enable sequence to
fail here with -ENODEV.

Could phy_qcom_mipi_csi2_settle_cnt_calc() return a negative error code for
invalid timings instead of overloading 0 as an error value?

> +
> +	/*
> +	 * 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);
> +
> +	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 * 2));

[Severity: Medium]
Does this writel() unintentionally clobber other bits in the
CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n register?

A direct write of BIT(2) will zero out all other bits in the register. Should
this be a read-modify-write sequence to preserve hardware defaults or
calibrations?

Additionally, if polarity configuration is disabled, the register is not
written at all, which might leave it in an inconsistent state if it was
previously modified.

> +		}
> +	}
> +
> +	if (lane_cfg->clk.pol)
> +		writel(BIT(2), csi2phy->base + CSIPHY_2PH_LN_CSI_2PHASE_CTRL9n(lane_cfg->clk.pos));
> +
> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-x1e-csi2-phy-v13-0-160c31958863@linaro.org?part=2