Re: [PATCH 11/16] media: qcom: camss: csid: Add v4l2 ctrl if TPG mode isn't disabled

[Depeng Shao <quic_depengs@quicinc.com>]

Hi Bryan,

On 12/11/2024 11:44 PM, Bryan O'Donoghue wrote:
> On 11/12/2024 14:07, Depeng Shao wrote:
>> There is no CSID TPG in some modern HW, so the v4l2 ctrl in CSID driver
> 
> "some modern HW" => "on some SoCs"
> 
>> shouldn't be registered. Checking the supported TPG modes to indicate
>> if the TPG HW is existing or not, and only register v4l2 ctrl for CSID
> 
> "TP HW is existing or not, and only register" => "TPG hardware exists or 
> not and oly registering"
> 
> No need to abbreviate hardware to HW.
> 
>>   only when TPG HW is existing.
> 
> "when the TPG hardware exists" you could also say "is present" instead 
> of "exists".
> 
> You have additional whitespace in your log before " only"
> 
>>

Thanks for the suggestion, will update in new version.


>>           }
>>           if (!csid->testgen.enabled &&
>> @@ -838,7 +840,8 @@ static void csid_try_format(struct csid_device *csid,
>>           break;
>>       case MSM_CSID_PAD_SRC:
>> -        if (csid->testgen_mode->cur.val == 0) {
>> +        if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED ||
> 
> if (csid->ctrls ||
> 
> feels like a more natural test. Less cumbersome and also less typing.
> 
> I think that change should be feasible, could you please update your 
> logic from if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED) to if 
> (csid->ctrls)
> 
> Otherwise looks good but, I'll wait to see your next version before 
> giving an RB.
> 

csid->ctrls is not a pointer, so it is always true.

Thanks,
Depeng