Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] First frame kick is permanently lost if the hardware is busy at the first timer tick, deadlocking the display. - [Medium] TCON TOP DSI clock gate is leaked because it is never disabled. - [Low] DT schema validation bypass due to missing compatible strings in conditional blocks. --
commit 17cfa6c0fbcb618f9439977751e662fb57699cdd Author: leow149 <[email protected]> arm64: allwinner: add TrimUI Smart Pro (A133) board support This commit adds board bring-up support for the TrimUI Smart Pro handheld device, implementing the display pipeline, panel driver, and PWM backlight. It introduces the A133 DE2 mixer configurations, TCON TOP quirks, and handles the CPU/8080-interface DSI output. > diff --git > a/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml > > b/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml > index cbd18fd83e527..ce72c1ba71f15 100644 > --- > a/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml > +++ > b/Documentation/devicetree/bindings/display/allwinner,sun8i-a83t-de2-mixer.yaml > @@ -23,6 +23,8 @@ properties: > - allwinner,sun20i-d1-de2-mixer-1 > - allwinner,sun50i-a64-de2-mixer-0 > - allwinner,sun50i-a64-de2-mixer-1 > + - allwinner,sun50i-a133-de2-mixer-0 > + - allwinner,sun50i-a133-de2-mixer-1 [Severity: Low] Is this missing from the conditional if: properties: compatible: contains blocks further down in the schema? By adding these new compatible strings to the global enum without integrating them into the generation-based if-then conditional blocks, device trees with invalid configurations for the A133 mixer may bypass variant-specific validation checks and silently pass. [ ... ] > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > index bf405a2aa3249..8b7dff89070dd 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c [ ... ] > @@ -713,11 +1054,31 @@ void sun4i_tcon_mode_set(struct sun4i_tcon *tcon, [ ... ] > case DRM_MODE_ENCODER_DSI: [ ... ] > sun4i_tcon0_mode_set_cpu(tcon, encoder, mode); > sun4i_tcon_set_mux(tcon, 0, encoder); > + sun4i_tcon0_set_dsi_gate(tcon, true); > + sun4i_tcon0_set_de0_port(tcon); > break; [Severity: Medium] Does this code leak the TCON TOP DSI clock gate? The gate is explicitly enabled here via sun4i_tcon0_set_dsi_gate(tcon, true), but it doesn't appear that sun4i_tcon0_set_dsi_gate(tcon, false) is ever called during DPMS OFF or teardown logic (e.g., in sun4i_tcon_set_status). This could leave the clock permanently enabled after the first modeset. [ ... ] > +static enum hrtimer_restart sun4i_tcon_dsi_retrigger_timer_fn(struct hrtimer > *timer) > +{ > + struct sun4i_tcon *tcon = container_of(timer, struct sun4i_tcon, > + dsi_retrigger_timer); > + unsigned int cpu_if_before; > + unsigned int tick = tcon->dsi_retrigger_ticks; > + u32 dsi_int; > + bool busy; > + > + regmap_read(tcon->regs, SUN4I_TCON0_CPU_IF_REG, &cpu_if_before); > + busy = cpu_if_before & SUN4I_TCON0_CPU_IF_TRI_START; > + > + if (tick && (!busy || tcon->dsi_irq_retriggered)) { > + /* > + * Either TRIGGER_START self-cleared, or (see the comment on > + * dsi_irq_retriggered in sun4i_tcon.h) sun4i_tcon_handler() > + * already re-triggered a frame on its own -- either way the > + * IRQ path has taken over from here. > + */ > + return HRTIMER_NORESTART; > + } [ ... ] > + if (tick == 0 && !busy) { > + sun6i_dsi_tri_start(tcon->dsi); > + regmap_update_bits(tcon->regs, SUN4I_TCON0_CPU_IF_REG, > + SUN4I_TCON0_CPU_IF_TRI_START, > + SUN4I_TCON0_CPU_IF_TRI_START); > + } > + > + tcon->dsi_retrigger_ticks = tick + 1; > + hrtimer_forward_now(timer, ms_to_ktime(2)); > + return HRTIMER_RESTART; > +} [Severity: High] Can this deadlock the display if the hardware is busy at the first timer tick? If a previous DPMS OFF interrupted an active frame transfer, the hardware might transiently or permanently remain busy (SUN4I_TCON0_CPU_IF_TRI_START == 1) at tick 0. Because the initial sun6i_dsi_tri_start kick is specifically guarded by if (tick == 0 && !busy), being busy at tick 0 causes the kick to be dropped. On subsequent ticks (tick > 0), the timer bypasses the kick entirely and will continue returning HRTIMER_RESTART endlessly, waiting for the hardware to become not busy. However, if the DSI engine remains idle without the kick, busy evaluates to true forever, permanently losing the frame. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
