Re: [PATCH RFC 4/4] media: qcom: camss: Configure C-PHY through the PHY API

[Bryan O'Donoghue]

On 15/10/2026 23:20, George Neacsu via B4 Relay wrote:
> From: George Neacsu <geocausa@xxxxxxxxx>
>
> With a PHY API CSIPHY, csiphy_stream_on() always asks for D-PHY. Ask for
> C-PHY when the endpoint is C-PHY, and pass the trio count and the symbol
> rate, which by the V4L2 convention is twice the link frequency.
>
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: George Neacsu <geocausa@xxxxxxxxx>
> ---
>   drivers/media/platform/qcom/camss/camss-csiphy.c | 35 ++++++++++++++----------
>   1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index f44f2aeb9..8e1b4335a 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -329,17 +329,15 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
>   	u8 bpp = csiphy_get_bpp(csiphy->res->formats->formats, csiphy->res->formats->nformats,
>   				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
>   	struct csiphy_lanes_cfg *lncfg = &csiphy->cfg.csi2->lane_cfg;
> -	struct phy_configure_opts_mipi_dphy *dphy_cfg;
> -	union phy_configure_opts dphy_opts = { 0 };
> +	const bool cphy = lncfg->phy_cfg == V4L2_MBUS_CSI2_CPHY;
> +	union phy_configure_opts opts = { 0 };
>   	struct device *dev = csiphy->camss->dev;
>   	u8 num_lanes = lncfg->num_data;
>   	s64 link_freq;
>   	int ret;
>
> -	dphy_cfg = &dphy_opts.mipi_dphy;
> -
>   	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes,
> -					csiphy->cfg.csi2->lane_cfg.phy_cfg == V4L2_MBUS_CSI2_CPHY);
> +					cphy);

That's a nice reduction, yikes who let that statement into the kernel :)

>   	if (link_freq <= 0) {
>   		dev_err(dev,
> @@ -347,23 +345,32 @@ static int csiphy_stream_on(struct csiphy_device *csiphy)
>   		return -EINVAL;
>   	}
>
> -	ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq, num_lanes,
> -							 dphy_cfg);
> -	if (ret < 0) {
> -		dev_err(dev, "Invalid config for link frequency %lld\n",
> -			link_freq);
> -		return ret;
> +	if (cphy) {
> +		/* A C-PHY link frequency is half the symbol rate. */

A comment that should live in a helper.

> +		opts.mipi_cphy.symbol_rate = link_freq * 2;

Sounds like something which should have a helper coming from the PHY layer.

The analogue for DPHY is phy_mipi_dphy_get_default_config_for_hsclk()

> +		opts.mipi_cphy.lanes = num_lanes;
> +	} else {
> +		ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq,
> +								 num_lanes,
> +								 &opts.mipi_dphy);
> +		if (ret < 0) {
> +			dev_err(dev, "Invalid config for link frequency %lld\n",
> +				link_freq);
> +			return ret;
> +		}
>   	}
>
> -	ret = phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
> +	ret = phy_set_mode(csiphy->phy,
> +			   cphy ? PHY_MODE_MIPI_CPHY : PHY_MODE_MIPI_DPHY);

Instead of a bool if you define an int which carriers the type you can 
still disjunct on that all you like but ultimately pass it into 
phy_set_mode() directly - skipping at least this ternary statement.

>   	if (ret) {
>   		dev_err(dev, "Failed to set phy mode\n");
>   		return ret;
>   	}
>
> -	ret = phy_configure(csiphy->phy, &dphy_opts);
> +	ret = phy_configure(csiphy->phy, &opts);
>   	if (ret) {
> -		dev_err(dev, "failed to configure MIPI D-PHY\n");
> +		dev_err(dev, "failed to configure MIPI %s\n",
> +			cphy ? "C-PHY" : "D-PHY");
>   		goto error;
>   	}
>
>
> --
> 2.53.0

---
bod