Re: [PATCH v14 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] Legitimate settle count of 0 is treated as an error, breaking MIPI CSI2 configurations that evaluate to exactly 6 timer periods.
- [Medium] The PHY configuration ignores the dynamically requested number of data lanes, always enabling all lanes defined in the device tree.
--

--- Patch [2]: [PATCH v14 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI DPHY driver ---
commit 33347f4ee71f45db321e44127a30dfe4e40a2c84
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 driver in DPHY mode, importing the existing
CAMSS CSI PHY init sequences. It supports configuring data-lane and clock
lane positioning and polarity selection.

> 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
> index 0000000000000..966d79c98f9b2
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-3ph-dphy.c
[ ... ]
> +static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 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: High]
Does this incorrectly reject a valid register configuration?

When the calculated timer period ratio (t_hs_settle_ps / timer_period_ps)
evaluates to exactly 6 in phy_qcom_mipi_csi2_settle_cnt_calc(), the function
returns a settle_cnt of 0.

By treating !settle_cnt as a failure here in phy_qcom_mipi_csi2_lanes_enable(),
it seems configurations that result in exactly 6 timer periods will fail to
initialize with -ENODEV, even though 0 is a mathematically valid outcome.

> +
> +	/*
> +	 * 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
> +	 */
[ ... ]
> 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..02f34b7de05f5
> --- /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]
Should the number of data lanes from the consumer also be recorded here?

The dphy_cfg->lanes parameter is validated, but it is not assigned to
stream_cfg->num_data_lanes. If a consumer calls phy_configure() requesting
fewer lanes than the device tree's maximum, the PHY will ignore the dynamic
request and later power up all lanes defined in the DT during
phy_qcom_mipi_csi2_lanes_enable().

Could this omission lead to a transmitter and receiver expecting a mismatched
number of active lanes, or result in unnecessary power waste?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-x1e-csi2-phy-v14-0-27d1bf477593@linaro.org?part=2