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: Move mode_valid logic to per-output clock limits Date: Sat, 16 May 2026 13:13:42 +1000 Message-ID: In-Reply-To: <20260512144104.761531-5-prabhakar.mahadev-lad.rj@bp.renesas.com> References: <20260512144104.761531-1-prabhakar.mahadev-lad.rj@bp.renesas.com> <20260512144104.761531-5-prabhakar.mahadev-lad.rj@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 **Status: Looks good, well designed.** This is the key refactoring patch. It replaces the hard-coded clock check: ```c - if (renc->output =3D=3D RZG2L_DU_OUTPUT_DPAD0 && mode->clock > 83500) + if (route->mode_clock_min && mode->clock < route->mode_clock_min) + return MODE_CLOCK_LOW; + if (route->mode_clock_max && mode->clock > route->mode_clock_max) return MODE_CLOCK_HIGH; ``` The per-output routing struct gets new fields: ```c + int mode_clock_min; + int mode_clock_max; ``` The existing SoC infos are updated with the same limits that were previousl= y hard-coded (20875/83500 for RZ/G2UL and RZ/G2L DPAD0). The zero-means-unb= ounded convention is clean and natural for C static initializers. **One observation:** `mode_clock_min` and `mode_clock_max` are declared as = `int` but are always used as unsigned values (kHz). Using `unsigned int` wo= uld be more semantically correct, though it's a minor style point. The exis= ting fields in the struct (`possible_outputs`, `port`) use `unsigned int`. The approach of encoding constraints per-output rather than per-SoC is a go= od design choice =E2=80=94 the cover letter mentions this was moved from `d= evice_info` to `output_routing` in v3, which is the right level of granular= ity since different outputs on the same SoC could have different clock limi= ts. Note that the `r9a09g057` (RZ/V2H) info has no clock limits set (all zeros)= , so no mode validation occurs for that SoC's DSI0 output. This is presumab= ly intentional since the DSI bridge driver would handle its own clock const= raints. --- Generated by Claude Code Patch Reviewer