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

[Depeng Shao <quic_depengs@quicinc.com>]

Hi Vladimir,

On 8/14/2024 7:13 PM, Vladimir Zapolskiy wrote:
> Hi Depeng,
> 
> please find a few review comments, all asked changes are non-functional.
> 

>> +void camss_reg_update(struct camss *camss, int hw_id, int port_id, 
>> bool is_clear)
> 
> Please let it be just a declarative 'clear' instead of questioning 
> '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.
> 
>>   void camss_buf_done(struct camss *camss, int hw_id, int port_id)
>>   {
>>       struct vfe_device *vfe;

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@kernel.org/

https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9-c44aab8147e5@kernel.org/


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.


Thanks,
Depeng