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/amd/display: Deduplicate DCN DDC register assignment Date: Wed, 01 Apr 2026 08:00:39 +1000 Message-ID: In-Reply-To: References: X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Positive aspects:** - The v3 iteration correctly fixes the type mismatch from v1 (which used no= n-existent `struct ddc_shift` / `struct ddc_mask` instead of the actual `st= ruct ddc_sh_mask`). - All 6 converted call sites are identical, making this a textbook deduplic= ation. - Using `static inline` in a header is appropriate for this small function,= avoiding cross-TU linkage issues while still deduplicating the source. **Issues:** 1. **Missing conversion of `dcn42/hw_factory_dcn42.c`**: The `dcn42` varian= t has an identical `define_ddc_registers()` body (lines 184-207 in `hw_fact= ory_dcn42.c`) that was not converted. This appears to be an oversight =E2= =80=94 dcn42 should also be updated to use the shared helper for completene= ss. 2. **Copyright year**: The header says `Copyright 2024` but this is a 2026 = submission. Minor nit but worth correcting to `2025` or `2026`. 3. **Header self-containment**: The new header `hw_factory_dcn_ddc.h` relie= s on types (`struct hw_gpio_pin`, `uint32_t`, `struct ddc_registers`, `stru= ct ddc_sh_mask`, `struct hw_ddc`, `HW_DDC_FROM_BASE`, `GPIO_ID_DDC_DATA`, `= GPIO_ID_DDC_CLOCK`, `ASSERT_CRITICAL`) being defined by headers that the in= cluder has already pulled in. This works in practice because every call sit= e includes `hw_ddc.h` and related headers before this one, but it is fragil= e =E2=80=94 if someone includes this header from a different context it wil= l break. Consider adding a comment at the top noting the prerequisite inclu= des, or adding the necessary `#include` directives to make the header self-= contained. 4. **Coding style nit**: The blank line between `case GPIO_ID_DDC_DATA:` an= d `case GPIO_ID_DDC_CLOCK:` blocks is a style change from the original code= (which had no blank line between cases). This is cosmetic but the original= code was consistent across all 6 files, so matching the original style wou= ld be cleaner for a "no functional changes" patch. --- Generated by Claude Code Patch Reviewer