Message-ID: <7130beef-7787-42a1-85c8-f27574241ba7@linaro.org>
Date: Thu, 15 Aug 2024 17:57:42 +0300
From: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
To: Depeng Shao <quic_depengs@...cinc.com>, 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 Depeng,

On 8/15/24 17:42, Depeng Shao wrote:
> 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.

let it be so.

I have another ask about it, please move new camss_reg_update() out from
camss.c into camss-csid.c, and camss_buf_done() from camss.c into camss-vfe.c

Thank you.

--
Best wishes,
Vladimir

Powered by blists - more mailing lists