Re: [PATCH v21 10/14] media: qcom: camss: tpg: Add support for v2.4.0 TPG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: qcom: camss: csid: incorrect fallback to physical PHY when TPG disabled
- [Medium] media: qcom: camss: uninitialized hw_version used in tpg_reset
--

commit 83c15a87930a3b292c2eeb9b034d6436828fd5de
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: tpg: Add support for v2.4.0 TPG

Add support for the TPG found on Kaanapali. This TPG uses hardware
version 2.4.0, which drives test-enable and reset through the
TPG_CTRL_CMD register at offset 0x1f4 instead of the TEST_EN bit in
TPG_CTRL and the TPG_CLEAR register. Its output is routed into the
CSID 1080 RX through the CSI2 TPG mux, so the CSID selects the TPG as
the data source when a test pattern is selected.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1080.c b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> index 46e443e05195a..851b3cd712f52 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-1080.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1080.c
[ ... ]
> @@ -148,8 +151,16 @@ static void __csid_configure_rx(struct csid_device *csid,
>  
>  	val = (lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
>  	val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
> -	val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX)
> -	       << CSI2_RX_CFG0_PHY_NUM_SEL;
> +
> +	if (camss->tpg && csid->tpg_linked &&
> +	    camss->tpg[phy->csiphy_id].testgen.mode != TPG_PAYLOAD_MODE_DISABLED) {
> +		val |= FIELD_PREP(CSI2_RX_CFG0_TPG_MUX_SEL, phy->csiphy_id + 1);
> +		val |= CSI2_RX_CFG0_TPG_MUX_EN;
> +	} else {
> +		val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX)
> +			<< CSI2_RX_CFG0_PHY_NUM_SEL;
> +	}
> +

[Severity: Medium]
When csid->tpg_linked is true, csid_link_setup() has already overwritten
phy->csiphy_id with the tpg->id. If a user leaves the TPG in the default
disabled mode (testgen.mode == TPG_PAYLOAD_MODE_DISABLED), does this
fallback to the else branch mistakenly use the tpg->id as a physical
CSIPHY index?

This appears to misconfigure the hardware to route data from a physical
camera sensor that happens to share the same numeric ID as the requested
TPG, silently bypassing the TPG entirely.

>  	writel(val, csid->base + CSID_CSI2_RX_CFG0);
>  
>  	val = CSI2_RX_CFG1_ECC_CORRECTION_EN;

> diff --git a/drivers/media/platform/qcom/camss/camss-tpg-gen1.c b/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
> index d29de5f93c18e..cabbb2638cf38 100644
> --- a/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
> +++ b/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
[ ... ]
> @@ -164,18 +167,26 @@ static int tpg_stream_on(struct tpg_device *tpg)
[ ... ]
>  static int tpg_reset(struct tpg_device *tpg)
>  {
> -	writel(0, tpg->base + TPG_CTRL);
> -	writel(1, tpg->base + TPG_CLEAR);
> +	if (tpg->hw_version < TPG_HW_VER_2_4_0)
> +		writel(0, tpg->base + TPG_CTRL);

[Severity: Medium]
Is tpg->hw_version properly initialized when this function is called for
the first time? Looking at tpg_set_power() in camss-tpg.c, it invokes
the reset callback before hw_version is populated on power-on:

tpg_set_power()
   ...
   tpg->res->hw_ops->reset(tpg);
   tpg->res->hw_ops->hw_version(tpg);

Since the struct is dynamically allocated and zeroed, the first
tpg_reset() will evaluate tpg->hw_version as 0. This satisfies the
condition and incorrectly executes the legacy write to TPG_CTRL even on
newer v2.4.0 hardware.

> +
> +	writel(TPG_CTRL_CMD_HW_RESET, tpg->base + TPG_CTRL_CMD);
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011-kaanapali-camss-v21-0-04fd28d815c6@oss.qualcomm.com?part=10

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