linux-kernel - Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware version Titan 780

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <82200889-a98d-4815-bc31-f81b15d02513@quicinc.com>
Date: Thu, 15 Aug 2024 22:42:03 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>, <rfoss@...nel.org>,
        <todor.too@...il.com>, <bryan.odonoghue@...aro.org>,
        <mchehab@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>
CC: <linux-arm-msm@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <kernel@...cinc.com>, Yongsheng Li <quic_yon@...cinc.com>
Subject: Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware
 version Titan 780

Hi Vladimir,

On 8/15/2024 7:20 AM, Vladimir Zapolskiy wrote:

>>
>>>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id,
>>>> bool is_clear)
>>>
>>>> +{
>>>> +    struct csid_device *csid;
>>>> +
>>>> +    if (hw_id < camss->res->csid_num) {
>>>> +        csid = &(camss->csid[hw_id]);
>>>> +
>>>> +        csid->res->hw_ops->reg_update(csid, port_id, is_clear);
>>>> +    }
>>>> +}
>>>> +
>>>
>>> Please add the new exported function camss_reg_update() in a separate
>>> preceding commit.

>>
>> Thanks for your comments, I will address them in new series.
>>
>> But I have some concern about above comment, you want to add a separate
>> commit for camss_reg_update, maybe camss_buf_done also need to do this,
>> but I guess I will get new comments from Krzysztof if I make a separate
>> change, Krzysztof posted few comments in v3 series, he asked, "must
>> organize your patches in logical junks" and the code must have a user.
>>
>> Please check below comments.
>>
>> https://lore.kernel.org/all/e1b298df-05da-4881- 
>> a628-149a8a625544@...nel.org/
>>
>> https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9- 
>> c44aab8147e5@...nel.org/
> 
> Krzysztof is absolutely right in his two comments.
> 
>  From what I see there is a difference between his concerns and mine ones
> though, Krzysztof points to unused data, which should raise a build time
> warning, and I asked to make a separate commit for a non-static function,
> I believe it'll be removed by the linker silently...
> 
> The potential runtime logic change introduced by camss_reg_update() in the
> generic code is not trivial, which opens an option to update/fix it lately
> referencing a commit from generic domain rather than platform specific one.
> 
> If someone for whatever reasons wants to merge a new generic and shared
> camss_reg_update() function within a the platform specific code/commit,
> I won't strongly object, let it be merged together then.
> 
>>
>> Or I don't add reg update and buf done functionality in
>> camss-csid-gen3.c and camss-vfe-780.c firstly, then add them in a later
>> commit.
>>
>> Could you please comment on whether this is acceptable? Please also help
>> to common on if one commit to add them or need two separate commits, one
>> is for reg update and the other one is for buf done.
>>
> 
> I would prefer to see two more separate commits within non-platform 
> specific
> code, however as I stated above if it causes anyone's concerns, including
> your own, let it be kept as it is done today. Eventually we do discuss
> a non-functional change.
> 

Thanks for the confirmation, even though I add the rup_update and 
buf_done function in later commits, it is still called in platform 
specific code(camss-vfe-780.c), so I will keep as it is done today.

Thanks,
Depeng