Claude review: drm/amd/display: Deduplicate DCN DDC register assignment

Claude Code Review Bot
2026-03-31T22:00:39Z

Patch Review

**Positive aspects:**
- The v3 iteration correctly fixes the type mismatch from v1 (which used non-existent `struct ddc_shift` / `struct ddc_mask` instead of the actual `struct ddc_sh_mask`).
- All 6 converted call sites are identical, making this a textbook deduplication.
- 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` variant has an identical `define_ddc_registers()` body (lines 184-207 in `hw_factory_dcn42.c`) that was not converted. This appears to be an oversight — dcn42 should also be updated to use the shared helper for completeness.

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` relies on types (`struct hw_gpio_pin`, `uint32_t`, `struct ddc_registers`, `struct 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 includer has already pulled in. This works in practice because every call site includes `hw_ddc.h` and related headers before this one, but it is fragile — if someone includes this header from a different context it will break. Consider adding a comment at the top noting the prerequisite includes, 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:` and `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 would be cleaner for a "no functional changes" patch.