Re: [PATCH 3/6] 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: uninitialized hw_version check in tpg_reset()
- [Low] media: qcom: camss: commit message describes omitted Kaanapali changes
--

commit 9872eb254e7eae29edd30791f0f0da3fb4571071
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

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

This commit adds hardware support for the v2.4.0 Test Pattern Generator (TPG)
in the Qualcomm CAMSS driver by handling test-enable and hardware reset via
the TPG_CTRL_CMD register.

>     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.
>     
>     Add the TPG resources to the Kaanapali camss resources so that the TPG
>     subdevices are created on this SoC.
>     
>     Signed-off-by: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
>     Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>     [vz: extracted pure TPG hardware support part from the change]
>     Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

[Severity: Low]
This isn't a bug, but since only the TPG v2.4.0 hardware support was extracted
into this commit, should the commit message be updated to drop the description
of the CSID 1080 RX mux routing and the Kaanapali camss resources?

> 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);
> +
> +	writel(TPG_CTRL_CMD_HW_RESET, tpg->base + TPG_CTRL_CMD);
>  
>  	return 0;
>  }

[Severity: Medium]
Does checking tpg->hw_version in tpg_reset() work as intended on the initial
power-on?

In drivers/media/platform/qcom/camss/camss-tpg.c, tpg_set_power() invokes
hw_ops->reset() before hw_ops->hw_version():

drivers/media/platform/qcom/camss/camss-tpg.c:tpg_set_power() {
	if (on) {
		...
		tpg->res->hw_ops->reset(tpg);

		tpg->res->hw_ops->hw_version(tpg);
	}
}

Because tpg_hw_version() in drivers/media/platform/qcom/camss/camss-tpg-gen1.c
is the only place that reads TPG_HW_VERSION and populates tpg->hw_version,
tpg->hw_version is still 0 when tpg_reset() runs during the first power-on. As
a result, tpg->hw_version < TPG_HW_VER_2_4_0 evaluates to true and writes 0 to
TPG_CTRL on v2.4.0 hardware as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031112203.545319-1-vladimir.zapolskiy@linaro.org?part=3