Hi Depeng,
On 8/14/24 16:10, Depeng Shao wrote:
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@xxxxxxxxxx/
https://lore.kernel.org/all/d0f8b72d-4355-43cd-a5f9-c44aab8147e5@xxxxxxxxxx/
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.
--
Best wishes,
Vladimir
