From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm: renesas: rz-du: mipi_dsi: Add RZ_MIPI_DSI_FEATURE_GPO0R feature Date: Sun, 12 Apr 2026 12:45:58 +1000 Message-ID: In-Reply-To: <9e0f64dd5e1efb0d27219416121c91a19da96ebd.1775636898.git.tommaso.merciai.xr@bp.renesas.com> References: <9e0f64dd5e1efb0d27219416121c91a19da96ebd.1775636898.git.tommaso.merciai.xr@bp.renesas.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This adds support for the RZ/G3E-specific GPO0R register for selecting the = video input clock, and refactors `vclk` to an array to support two video cl= ocks. **Bug (indentation/scoping)**: ```c dsi->vclk[0] =3D devm_clk_get(dsi->dev, "vclk"); if (IS_ERR(dsi->vclk[0])) return PTR_ERR(dsi->vclk[0]); ``` The `if` and `return` are indented as if inside a block, but there is no en= closing block =E2=80=94 they're at the wrong indentation level. The code wi= ll still compile and work correctly (C doesn't care about whitespace), but = this is a clear coding style error. The `if` should be at the same level as= the `devm_clk_get` call: ```c dsi->vclk[0] =3D devm_clk_get(dsi->dev, "vclk"); if (IS_ERR(dsi->vclk[0])) return PTR_ERR(dsi->vclk[0]); ``` **Issue (potential use of uninitialized variable)**: In `rzg2l_mipi_dsi_get= _input_port()`: ```c bool ep_enabled; int in_port; for_each_endpoint_of_node(np, ep_node) { ... if (ep_enabled) { in_port =3D ep.port; break; } } if (!ep_enabled) return -EINVAL; ``` If the node has no endpoints at all (the `for_each_endpoint_of_node` loop b= ody never executes), `ep_enabled` is used uninitialized. It should be initi= alized to `false`: ```c bool ep_enabled =3D false; ``` Similarly, `in_port` could be used uninitialized in theory, though the `ep_= enabled` check guards against that. **Issue (OF node refcount)**: The `remote_ep` obtained from `of_graph_get_r= emote_endpoint()` is a device node that needs to be properly refcounted. Th= e code does `of_node_put(remote_ep)` which is correct. However, `of_device_= is_available()` checks the `status` property of a node =E2=80=94 but `remot= e_ep` is an endpoint node, not a device node. Endpoint nodes don't typicall= y have a `status` property. This should probably be checking the availabili= ty of the remote device node (the parent of the endpoint), not the endpoint= itself. The function should use something like: ```c remote_dev =3D of_graph_get_port_parent(remote_ep); ep_enabled =3D of_device_is_available(remote_dev); of_node_put(remote_dev); of_node_put(remote_ep); ``` **Issue (missing `of_node_put` on early break)**: `for_each_endpoint_of_nod= e` increments the refcount on `ep_node`. When breaking out of the loop earl= y, the current `ep_node` reference should be released with `of_node_put(ep_= node)`. The code breaks without putting the node. --- Generated by Claude Code Patch Reviewer