Re: [PATCH v17 08/11] media: qcom: camss: tpg: Add support for v2.4.0 TPG

[Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>]

On 9/29/2026 2:12 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] media: camss: uninitialized tpg->hw_version used in tpg_reset()
> --
> 
> commit e2a0aed20f3751127d25b2596032eac51d537c58
> Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
> 
> media: qcom: camss: tpg: Add support for v2.4.0 TPG
> 
> This commit adds support for the TPG hardware version 2.4.0 found on
> Kaanapali. It updates the configuration and reset logic to use the new
> TPG_CTRL_CMD register instead of TPG_CTRL.
> 
>> diff --git a/drivers/media/platform/qcom/camss/camss-tpg-gen1.c b/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
>> index d29de5f93c18e..770a9e5d5ba5f 100644
>> --- a/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
>> +++ b/drivers/media/platform/qcom/camss/camss-tpg-gen1.c
> 
> [ ... ]
> 
>> @@ -164,18 +167,30 @@ 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);
>> +	/*
>> +	 * On TPG older than v2.4.0 test-enable lives in TPG_CTRL, so clear it
>> +	 * first; v2.4.0+ drives both test-enable and reset through TPG_CTRL_CMD.
>> +	 */
>> +	if (tpg->hw_version < TPG_HW_VER_2_4_0)
>> +		writel(0, tpg->base + TPG_CTRL);
> 
> [Severity: Medium]
> Will this execute the legacy reset logic on v2.4.0+ hardware during the
> first power-on sequence?
> 
> When the driver powers up the device, tpg_set_power() calls the reset
> function before calling hw_version():
> 
> drivers/media/platform/qcom/camss/camss-tpg.c:tpg_set_power() {
> 	...
> 	tpg->res->hw_ops->reset(tpg);
> 	tpg->res->hw_ops->hw_version(tpg);
> 	...
> }
> 
> Because the tpg object is zero-initialized and tpg->hw_version is not
> populated until after the reset function is called, tpg->hw_version
> is strictly 0 during the first execution of tpg_reset().
> 
> Does this mean the condition here evaluates to true for all hardware
> versions on the initial power-up, inadvertently causing an unintended
> write to TPG_CTRL?
> 
>> +
>> +	writel(TPG_CTRL_CMD_HW_RESET, tpg->base + TPG_CTRL_CMD);
>>   
>>   	return 0;
>>   }
> 

No. The version data is always valid no matter the reset is called or not.

Best Regards,
Hangxiang