Re: [PATCH v7 2/8] media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Commit message claims to return failure but code does not implement error propagation.
--

commit da5484f83dd0e6203bd1f47a4299fe8d0bb8c433
Author: David Heidelberg <david@ixit.cz>

media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes

This commit adds support for C-PHY lane configuration by utilizing odd
bits instead of the even bits used for D-PHY mode. It updates the lane
mask calculation and enablement routines for C-PHY hardware requirements.

> Since there can be unrecognized configuration allow returning failure.

[Severity: Medium]
The commit message explicitly states "allow returning failure" for
unrecognized configurations, but does the code actually implement this?

Looking at csiphy_get_lane_mask() and csiphy_lanes_enable(), the code does
not appear to propagate any errors. Could the commit message be updated
to reflect the use of warnings, or should the functions be changed to return
an error code?

> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index dac8d2ecf7995..fa24fc9706748 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
[ ... ]
> @@ -1113,13 +1114,22 @@ static void csiphy_gen2_config_lanes(struct csiphy_device *csiphy,
>  
>  static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg)
>  {
> -	u8 lane_mask;
> -	int i;
> +	u8 lane_mask = 0;
> +	u8 offset = 0;
>  
> -	lane_mask = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
> +	switch (lane_cfg->phy_cfg) {
> +	case V4L2_MBUS_CSI2_CPHY:
> +		offset = 1;
> +		break;
> +	case V4L2_MBUS_CSI2_DPHY:
> +		lane_mask = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
> +		break;
> +	default:
> +		break;
> +	}

[Severity: Medium]
In csiphy_get_lane_mask(), the default case silently falls through and
returns 0. Is this intended to be treated as a failure condition?

[ ... ]
> @@ -1160,9 +1170,22 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
[ ... ]
> -	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
> -	for (i = 0; i < c->num_data; i++)
> -		val |= BIT(c->data[i].pos * 2);
> +	val = 0;
> +
> +	switch (c->phy_cfg) {
> +	case V4L2_MBUS_CSI2_CPHY:
> +		for (i = 0; i < c->num_data; i++)
> +			val |= BIT((c->data[i].pos * 2) + 1);
> +		break;
> +	case V4L2_MBUS_CSI2_DPHY:
> +		val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
> +
> +		for (i = 0; i < c->num_data; i++)
> +			val |= BIT(c->data[i].pos * 2);
> +		break;
> +	default:
> +		WARN_ONCE(1, "Unsupported bus type %d!\n", c->phy_cfg);
> +	}

[Severity: Medium]
Similarly, since csiphy_lanes_enable() has a void return type, the caller
has no way to detect this failure. While misconfiguration might be
prevented earlier during device tree parsing, does this function need to be
updated to actually return the failure mentioned in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-qcom-cphy-v7-0-426c37e9008f@ixit.cz?part=2