Re: [PATCH v17 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 1 potential issue(s) to consider:
- [Medium] The MIPI D-PHY configuration ignores the requested number of active lanes, enabling all physically mapped lanes instead.
--

commit 80184194d436b4ea64c9ebc79907070b59c9de53
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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

Adds a new MIPI CSI2 driver in DPHY mode by importing existing CAMSS CSI
PHY init sequences. Supports the qcom,x1e80100-csi2-phy device and includes
data/clock lane positioning and polarity selection.

> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
[ ... ]
> +static int phy_qcom_mipi_csi2_configure(struct phy *phy,
> +					union phy_configure_opts *opts)
> +{
> +	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
> +	struct phy_configure_opts_mipi_dphy *dphy_cfg = &opts->mipi_dphy;
> +	struct mipi_csi2phy_stream_cfg *stream_cfg = &csi2phy->stream_cfg;
> +	int ret;
> +
> +	ret = phy_mipi_dphy_config_validate(dphy_cfg);
> +	if (ret)
> +		return ret;
> +
> +	if (dphy_cfg->lanes < 1 || dphy_cfg->lanes > CSI2_MAX_DATA_LANES)
> +		return -EINVAL;
> +
> +	stream_cfg->link_freq = dphy_cfg->hs_clk_rate;

[Severity: Medium]
Should the requested active lane count from opts->mipi_dphy.lanes be stored
in stream_cfg here? Currently, dphy_cfg->lanes is validated but the value
does not appear to be saved for later use by the PHY enable routines.

> +
> +	return 0;
> +}
[ ... ]
> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
> --- /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;
[ ... ]
> +	/*
> +	 * 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: Medium]
Does this loop unconditionally enable all physically mapped lanes instead
of the dynamically requested lane count?

If a MIPI CSI-2 consumer calls phy_configure() requesting fewer active lanes
than the hardware maximum mapped in the device tree, the PHY will still
power up all of them here since cfg->num_data_lanes is derived from the
device tree parsing. Should this use a dynamically configured lane count,
or at least reject the configuration in phy_qcom_mipi_csi2_configure if it
doesn't match the hardware?

> +
> +	writel(val, csi2phy->base +
> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->common_regs_offset, 5));
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-x1e-csi2-phy-v17-0-26606fa9a039@linaro.org?part=2

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy