Re: [PATCH v6 5/8] media: qcom: camss: csiphy-3ph: Add Gen2 v1.1 MIPI CSI-2 C-PHY init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The patch introduces the use of `CSIPHY_SETTLE_CNT_HIGHER_BYTE` in `lane_regs_sdm845_3ph`, but the driver lacks support for it in `csiphy_gen2_config_lanes()`, creating a brittle hidden fallback.
--

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

media: qcom: camss: csiphy-3ph: Add Gen2 v1.1 MIPI CSI-2 C-PHY init

Add a PHY configuration sequence for the sdm845 which uses a Qualcomm
Gen 2 version 1.1 CSI-2 PHY. The PHY can be configured as two phase or
three phase in C-PHY or D-PHY mode. This configuration supports three-phase
C-PHY mode.

> 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 938c365eb352f..13d7372bd225d 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
[ ... ]
> @@ -220,6 +228,72 @@ csiphy_lane_regs lane_regs_sdm845[] = {
>  	{0x0664, 0x7F, 0x00, CSIPHY_DEFAULT_PARAMS},
>  };
>  
> +/* GEN2 1.0 3PH */
> +/* 3 entries: 3 lanes (C-PHY) */
> +static const struct
> +csiphy_lane_regs lane_regs_sdm845_3ph[] = {
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(23), 0x43, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(26), 0xa0, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(27), 0x25, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(1),  0x06, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(3),  0x12, 0x00, CSIPHY_SETTLE_CNT_LOWER_BYTE},
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(2),  0x00, 0x00, CSIPHY_SETTLE_CNT_HIGHER_BYTE},

[Severity: Low]
Does the driver actually support writing the higher byte of the settle count
here?

Looking at csiphy_gen2_config_lanes() in this file, the switch statement
doesn't handle CSIPHY_SETTLE_CNT_HIGHER_BYTE:

		switch (r->csiphy_param_type) {
		case CSIPHY_SETTLE_CNT_LOWER_BYTE:
			val = settle_cnt & 0xff;
			break;
		case CSIPHY_SKEW_CAL:
			/* TODO: support application of skew from dt flag */
			continue;
		case CSIPHY_DNP_PARAMS:
			continue;
		default:
			val = r->reg_data;
			break;
		}

Since the case is missing, it falls through to the default case and writes
the hardcoded 0x00 value from the array instead of actively evaluating the
higher byte of the calculated settle count.

This happens to work right now because csiphy_settle_cnt_calc() returns a u8,
so the higher byte would always be zero anyway, but it renders the
CSIPHY_SETTLE_CNT_HIGHER_BYTE tag meaningless.

> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(5),  0x20, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{CSIPHY_LN1_CSI_3PH_CTRLn_ADDR(20), 0x00, 0x00, CSIPHY_DEFAULT_PARAMS},

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-qcom-cphy-v6-0-e50de0b557a8@ixit.cz?part=5