From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH 2/2] drm/verisilicon: add support for Nuvoton MA35D1 DCUltra Lite display controller Date: Sat, 16 May 2026 15:45:52 +1000 Message-ID: In-Reply-To: <93e69179dbc495188cfffd8015350b3a55ce7876.camel@iscas.ac.cn> References: <20260511075142.54752-1-a0987203069@gmail.com> <93e69179dbc495188cfffd8015350b3a55ce7876.camel@iscas.ac.cn> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **1. IRQ handler unconditionally returns `IRQ_HANDLED` and does not clear t= he interrupt** ```c + if (dc->info->family =3D=3D VS_DC_FAMILY_DCULTRA_LITE) { + regmap_read(dc->regs, VSDC_DISP_IRQ_STA, &irqs); + if (irqs & BIT(0)) + vs_drm_handle_irq(dc, VSDC_TOP_IRQ_VSYNC(0)); + return IRQ_HANDLED; + } ``` Two issues: - **No IRQ acknowledge/clear:** After reading `VSDC_DISP_IRQ_STA`, the stat= us is never written back or cleared. If this is a level-triggered interrupt= (the DT binding says `IRQ_TYPE_LEVEL_HIGH`), the interrupt will remain ass= erted and cause an IRQ storm. The handler needs to write to the status regi= ster to clear the pending bit (e.g., `regmap_write(dc->regs, VSDC_DISP_IRQ_= STA, irqs)` or whatever the hardware's clear mechanism is). - **Returns `IRQ_HANDLED` even when `irqs =3D=3D 0`:** If no interrupt bits= are set, the handler should return `IRQ_NONE`. This matters if the IRQ lin= e is shared. **2. `vs_crtc_disable_vblank` uses `regmap_write` with 0, clobbering all IR= Q enable bits** ```c + if (dc->info->family =3D=3D VS_DC_FAMILY_DCULTRA_LITE) + regmap_write(dc->regs, VSDC_DISP_IRQ_EN, 0); ``` This writes 0 to the entire IRQ enable register, clearing all interrupt ena= bles =E2=80=94 not just vblank. If other interrupt sources exist in the sam= e register, they'd be silently disabled. Use `regmap_clear_bits(dc->regs, V= SDC_DISP_IRQ_EN, BIT(0))` for consistency and safety. Similarly in `vs_crtc_enable_vblank`: ```c + regmap_write(dc->regs, VSDC_DISP_IRQ_EN, BIT(0)); ``` This overwrites the whole register. Use `regmap_set_bits` instead to be saf= e. **3. Plane disable on DCUltra Lite is effectively a no-op** In `vs_primary_plane_atomic_disable`: ```c - regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), - VSDC_FB_CONFIG_EX_FB_EN); + if (dc->info->has_config_ex) + regmap_set_bits(dc->regs, VSDC_FB_CONFIG_EX(output), + VSDC_FB_CONFIG_EX_FB_EN); vs_primary_plane_commit(dc, output); ``` For DCUltra Lite: `has_config_ex` is false, so the FB_EN set is skipped. Th= en `vs_primary_plane_commit` also does nothing (it's gated by `has_config_e= x`). So disabling the plane writes zero registers. The DCUltra Lite should = presumably clear the `VSDC_FB_CONFIG_ENABLE` bit (BIT(0)) in `VSDC_FB_CONFI= G` to actually disable the framebuffer. Without this, the plane can never b= e turned off. **4. Plane enable sets both ENABLE and RESET bits simultaneously during upd= ate** ```c + /* DCUltra Lite requires explicit enable/reset bits in FB_CONFIG */ + if (!dc->info->has_config_ex) + regmap_set_bits(dc->regs, VSDC_FB_CONFIG(output), + VSDC_FB_CONFIG_ENABLE | VSDC_FB_CONFIG_RESET); ``` Setting the RESET bit (bit 4) every frame update is suspicious. The cover l= etter describes bit 4 as "reset" =E2=80=94 does this reset the framebuffer = pipeline on every update? If RESET is meant to be a one-shot initialization= , it should only be asserted once during enable (or asserted then deasserte= d). Setting it on every `atomic_update` could cause visual glitches or part= ial frame resets. Please clarify the hardware semantics of FB_CONFIG bit 4. **5. The OF match table uses `"verisilicon,dc"` as a standalone compatible** ```c static const struct of_device_id vs_dc_driver_dt_match[] =3D { - { .compatible =3D "verisilicon,dc" }, + { .compatible =3D "verisilicon,dc", .data =3D &vs_dc8000_info }, + { .compatible =3D "nuvoton,ma35d1-dcu", .data =3D &vs_dcultra_lite_info }, ``` The existing DT binding for TH1520 uses a two-element compatible: `"thead,t= h1520-dc8200", "verisilicon,dc"`. The driver matches on the fallback `"veri= silicon,dc"`. This is fine for now, but when/if a TH1520-specific sub-schem= a is added with a different `vs_dc_info`, the match order will matter. No c= hange needed now, just something to keep in mind. **6. `vs_dc8000_info` does not set `display_count` or `formats`** ```c +static const struct vs_dc_info vs_dc8000_info =3D { + .family =3D VS_DC_FAMILY_DC8000, + .has_chip_id =3D true, + .has_config_ex =3D true, + .regmap_cfg =3D &vs_dc8000_regmap_cfg, +}; ``` `display_count` defaults to 0 and `formats` defaults to NULL. For DC8000 th= is is fine because `has_chip_id =3D true` means these are read from hardwar= e registers. But the `vs_dc_info` docstring says `@display_count: number of= display outputs (0 =3D auto-detect from DT/HW)` =E2=80=94 this could be co= nfusing since 0 doesn't mean "auto-detect from DT" here; it means "read fro= m chip identity registers". The comment should be more precise. **7. Error path in `vs_dc_probe` doesn't re-assert resets for DCUltra Lite = (correct but asymmetric)** ```c err_rst_assert: - reset_control_bulk_assert(VSDC_RESET_COUNT, dc->rsts); + if (info->family =3D=3D VS_DC_FAMILY_DC8000) + reset_control_bulk_assert(VSDC_RESET_COUNT, dc->rsts); ``` This is correct =E2=80=94 DCUltra Lite doesn't get resets, so there's nothi= ng to assert. But the pattern of checking `info->family =3D=3D VS_DC_FAMILY= _DC8000` in multiple places for reset handling suggests a helper or a flag = (e.g., `has_resets`) would be cleaner than family enum checks scattered thr= ough probe/remove/error paths. **8. The `vs_bridge_atomic_disable` reordering may change DC8000 behavior** ```c - regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START, - VSDC_DISP_PANEL_START_MULTI_DISP_SYNC | - VSDC_DISP_PANEL_START_RUNNING(output)); regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output), VSDC_DISP_PANEL_CONFIG_RUNNING); - regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id), - VSDC_DISP_PANEL_CONFIG_EX_COMMIT); + if (dc->info->has_config_ex) { + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START, ...); + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc->id), + VSDC_DISP_PANEL_CONFIG_EX_COMMIT); + } ``` The original code cleared `PANEL_START` *before* clearing `PANEL_CONFIG_RUN= NING`. The new code clears `PANEL_CONFIG_RUNNING` first, then `PANEL_START`= (inside the `if`). This reordering may or may not matter for DC8000 hardwa= re, but it's a silent behavioral change for the existing variant. The origi= nal ordering should be preserved for DC8000 to avoid regressions. **Summary of required fixes before this can be merged:** 1. Fix IRQ handler to clear interrupt status and return `IRQ_NONE` when no = interrupt is pending 2. Use `regmap_set_bits`/`regmap_clear_bits` for VSDC_DISP_IRQ_EN instead o= f `regmap_write` 3. Implement actual framebuffer disable for DCUltra Lite (clear VSDC_FB_CON= FIG_ENABLE) 4. Clarify FB_CONFIG RESET bit semantics =E2=80=94 avoid setting it on ever= y frame update if it's a one-shot 5. Preserve the original register write ordering in `vs_bridge_atomic_disab= le` for DC8000 6. Fix `additionalProperties: true` in the base binding (use `unevaluatedPr= operties: false`) --- Generated by Claude Code Patch Reviewer