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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <282a5a62-ce09-4735-a1ad-bb2cd65fbc3f@quicinc.com>
Date: Mon, 23 Dec 2024 21:09:09 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
        <todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <vladimir.zapolskiy@...aro.org>
CC: <quic_eberman@...cinc.com>, <linux-media@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <kernel@...cinc.com>
Subject: Re: [PATCH 11/16] media: qcom: camss: csid: Add v4l2 ctrl if TPG mode
 isn't disabled

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