Re: [PATCH RFC 2/6] media: qcom: camss: Add C-PHY timing fields and get config from sensor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: qcom: camss: missing V4L2_CID_LINK_FREQ fallback in csiphy_stream_on()
- [Low] media: v4l2-mediabus: missing kernel-doc for C-PHY timing fields
--

commit 0e2de1c89a699407d4b177bc0d1a72cd888312a3
Author: Nihal Kumar Gupta <nihal.gupta@oss.qualcomm.com>

media: qcom: camss: Add C-PHY timing fields and get config from sensor

This commit adds t_prepare_ns and t_preamble_ns timing fields to struct
v4l2_mbus_config_mipi_csi2 and updates the CAMSS CSIPHY driver to accept
C-PHY endpoints, query the remote transmitter media bus configuration,
and pass C-PHY link frequency and timings to the generic PHY.

> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy.c b/drivers/media/platform/qcom/camss/camss-csiphy.c
> index f99eea2ef2289..ed2318d76987d 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy.c
> @@ -313,6 +313,29 @@ static void csiphy_stream_off_legacy(struct csiphy_device *csiphy)
>  	csiphy->res->hw_ops->lanes_disable(csiphy, &csiphy->cfg);
>  }
>  
> +static int csiphy_get_remote_mbus_config(struct csiphy_device *csiphy,
> +					 struct v4l2_mbus_config *cfg)
> +{
> +	struct media_pad *remote;
> +	struct v4l2_subdev *remote_sd;
> +	int ret;
> +
> +	remote = media_pad_remote_pad_first(&csiphy->pads[MSM_CSIPHY_PAD_SINK]);
> +	if (!remote)
> +		return -ENOLINK;
> +
> +	remote_sd = media_entity_to_v4l2_subdev(remote->entity);
> +	if (!remote_sd)
> +		return -EINVAL;
> +
> +	memset(cfg, 0, sizeof(*cfg));
> +	ret = v4l2_subdev_call(remote_sd, pad, get_mbus_config,
> +			      remote->index, cfg);
> +	if (ret == -ENOIOCTLCMD)
> +		return -EOPNOTSUPP;
> +
> +	return ret;
> +}
>  /*
>   * csiphy_stream_on - Enable streaming on CSIPHY module
>   * @csiphy: CSIPHY device
> @@ -324,43 +347,74 @@ static void csiphy_stream_off_legacy(struct csiphy_device *csiphy)
>   */
>  static int csiphy_stream_on(struct csiphy_device *csiphy)
>  {
> +	struct v4l2_mbus_config mbus_cfg = { };
>  	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 };
> +	union phy_configure_opts opts = { 0 };
> +	enum phy_mode mode = PHY_MODE_INVALID;
>  	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);
> +	ret = csiphy_get_remote_mbus_config(csiphy, &mbus_cfg);
> +	if (ret && ret != -EOPNOTSUPP)
> +		return ret;
>  
> -	if (link_freq <= 0) {
> -		dev_err(dev,
> -			"Cannot get CSI2 transmitter's link frequency\n");
> -		return -EINVAL;
> +	if (ret == -EOPNOTSUPP && lncfg->phy_type == V4L2_MBUS_CSI2_CPHY) {
> +		dev_err(dev, "C-PHY transmitter does not report media bus configuration\n");
> +		return ret;
>  	}
>  
> -	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 (!ret && mbus_cfg.type == V4L2_MBUS_CSI2_CPHY) {
> +		struct phy_configure_opts_mipi_cphy *cphy = &opts.mipi_cphy;
> +
> +		link_freq = mbus_cfg.link_freq;
> +		if (link_freq <= 0) {
> +			dev_err(dev,
> +				"Cannot get CSI2 transmitter's link frequency\n");
> +			return -EINVAL;
> +		}

[Severity: Medium]
What happens in csiphy_stream_on() if a C-PHY sensor or bridge implements
get_mbus_config() to report V4L2_MBUS_CSI2_CPHY and C-PHY timings, but
exposes its link frequency through V4L2_CID_LINK_FREQ rather than setting
mbus_cfg.link_freq?

Per Documentation/driver-api/media/tx-rx.rst and include/media/v4l2-common.h,
transmitters with configurable link frequencies use V4L2_CID_LINK_FREQ
and leave v4l2_mbus_config::link_freq unset, while receivers are expected
to query the frequency via v4l2_get_link_freq() (which checks
mbus_config.link_freq first and falls back to V4L2_CID_LINK_FREQ).

In CAMSS, both csid_set_clock_rates() in
drivers/media/platform/qcom/camss/camss-csid.c:

	link_freq = camss_get_link_freq(&csid->subdev.entity, fmt->bpp,
					csid->phy.lane_cnt);

and the D-PHY branch below call camss_get_link_freq() -> v4l2_get_link_freq().
Should the C-PHY branch also fall back to camss_get_link_freq() when
mbus_cfg.link_freq is not populated (which would also make use of bpp
computed at the top of csiphy_stream_on())?

[ ... ]

> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 24c738cd78940..42543ec98a839 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -109,6 +109,8 @@ struct v4l2_mbus_config_mipi_csi2 {
>  	unsigned char num_data_lanes;
>  	bool lane_polarities[1 + V4L2_MBUS_CSI2_MAX_DATA_LANES];
>  	enum v4l2_mbus_csi2_cphy_line_orders_type line_orders[V4L2_MBUS_CSI2_MAX_DATA_LANES];
> +	u32 t_prepare_ns;
> +	u32 t_preamble_ns;
>  };

[Severity: Low]
struct v4l2_mbus_config_mipi_csi2 has a kernel-doc comment block right above
it, and include/media/v4l2-mediabus.h is included by
Documentation/driver-api/media/v4l2-mediabus.rst. Should @t_prepare_ns and
@t_preamble_ns be added to the kernel-doc comment to avoid kernel-doc build
warnings?

>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017-cphy_support-v1-0-8b0e1237ce95@oss.qualcomm.com?part=2

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