Re: [PATCH v16 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 CSI-2 PHY driver validates the dynamically requested lane count in `phy_configure` but then drops it, always powering up all statically connected lanes.
--

commit 195a47fa2ff62bda0e1e24d4911794d18fcb86c8
Author: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

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-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
> new file mode 100644
> index 0000000000000..24ccd3264686f
> --- /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;
> +
> +	return 0;
> +}

[Severity: Medium]
Does this code inadvertently drop the requested lane count from
opts->mipi_dphy.lanes?

In phy_qcom_mipi_csi2_configure(), dphy_cfg->lanes is bounds-checked but
never assigned to stream_cfg->num_data_lanes. This causes the dynamic lane
count to be dropped.

Because it is dropped, the driver later relies on the statically probed count
when enabling the lanes:

drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c:phy_qcom_mipi_csi2_lanes_enable() {
    ...
    val = BIT(lane_cfg->clk.pos);
    for (i = 0; i < cfg->num_data_lanes; i++)
        val |= BIT(lane_cfg->data[i].pos * 2);
    ...
}

Should stream_cfg->num_data_lanes be updated here so that the PHY only
powers up the requested subset of physically connected lanes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-x1e-csi2-phy-v16-0-dd863b929080@linaro.org?part=2