Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
New issues: - [Critical] Missing timer cancellation on CRTC disable causes a fatal bus hang. - [Critical] Vblank interrupts are permanently broken for all non-DSI panels because the TCON IRQ is never enabled. - [Critical] Moving the HS clock enable (DSI_START_HSC) before drm_panel_prepare() breaks DCS initialization for existing panels. - [High] Using regmap_update_bits() on a Write-1-to-Clear (W1C) register silently fails to clear status bits. - [High] Manually modifying the TCON TOP DSI gate bypasses the clock framework, causing clk_disable_unused to wedge the display. - [High] Contradictory retriggering configuration creates dead code and guarantees a stalled transfer. - [High] Acknowledging FSYNC_INT in the interrupt handler inadvertently triggers spurious vblank and page-flip events. - [Medium] Unbalanced IRQ Enable causes warning and stack dump on subsequent mode sets. - [Medium] Missing synchronization for read-modify-write on shared PWM registers. - [Medium] Setting a 100% duty cycle on the PWM results in a 0% duty cycle due to a 16-bit integer truncation. - [Low] The A133 TCON TOP compatible string lacks an if-then block in the DT binding, causing schema validation to fail. Pre-existing issues: - [High] Division by zero occurs if the requested display mode pixel clock is less than 1 MHz. -- commit 99fbb5d85e5546f268568ad90dca77472cfb738d Author: leow149 <[email protected]> Subject: arm64: allwinner: add TrimUI Smart Pro (A133) board support This commit introduces board support for the TrimUI Smart Pro handheld. It adds a new device tree, a driver for the OTM1289A DSI panel, a new A133 PWM driver, and modifies sun4i DRM drivers to support the display pipeline. It also adds TCON TOP quirks and alters the DSI initialization sequence. > diff --git > a/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml > b/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml > --- > a/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml > +++ > b/Documentation/devicetree/bindings/display/allwinner,sun8i-r40-tcon-top.yaml > @@ -43,6 +43,7 @@ properties: > - allwinner,sun8i-r40-tcon-top > - allwinner,sun20i-d1-tcon-top > - allwinner,sun50i-h6-tcon-top > + - allwinner,sun50i-a133-tcon-top [Severity: Low] Because the base schema unconditionally requires HDMI ports that the A133 does not have, will this cause device tree schema validation to fail when dt_binding_check runs? The new A133 compatible string is missing from the allOf conditional blocks to relax these constraints. [ ... ] > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > @@ -498,6 +498,11 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon > *tcon, > regmap_write(tcon->regs, SUN4I_TCON0_CPU_IF_REG, > SUN4I_TCON0_CPU_IF_MODE_DSI | > SUN4I_TCON0_CPU_IF_TRI_FIFO_FLUSH | > SUN4I_TCON0_CPU_IF_TRI_FIFO_EN | > SUN4I_TCON0_CPU_IF_TRI_EN); [Severity: High] With SUN4I_TCON0_CPU_IF_AUTO explicitly omitted here, does this make the fallback logic in the interrupt handler unreachable? sun4i_tcon_enable_vblank() returns early for DSI CPU mode, meaning TRI_FINISH_ENABLE is never written. It seems this prevents any frames from being sent after the first kick. [ ... ] > @@ -547,6 +547,8 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon > *tcon, > */ > start_delay = (mode->crtc_vtotal - mode->crtc_vdisplay - 9); > start_delay = start_delay * mode->crtc_htotal * 300; > start_delay = start_delay / (mode->crtc_clock / 1000) / 8; [Severity: High] This is a pre-existing issue, but if userspace passes a custom mode with a pixel clock less than 1 MHz, mode->crtc_clock / 1000 evaluates to zero. Can this trigger a kernel panic due to division by zero? [ ... ] > @@ -572,6 +572,8 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon > *tcon, > */ > tcon->dsi_retrigger_ticks = 0; > hrtimer_start(&tcon->dsi_retrigger_timer, ms_to_ktime(1000), > HRTIMER_MODE_REL); [Severity: Critical] Is this timer explicitly canceled via hrtimer_cancel() when the CRTC is disabled? If the display goes to sleep and the DSI clocks are gated off, the timer will eventually attempt to read the DSI interrupt register, which could cause a fatal synchronous external abort. [ ... ] > @@ -956,6 +956,11 @@ static irqreturn_t sun4i_tcon_handler(int irq, void > *private) > * clearing it below) keeps the line alive. > */ > handled = status & (SUN4I_TCON_GINT0_VBLANK_INT(0) | > SUN4I_TCON_GINT0_VBLANK_INT(1) | > SUN4I_TCON_GINT0_TCON0_TRI_FINISH_INT | > SUN4I_TCON_GINT0_TCON0_FSYNC_INT); > > if (!handled) > return IRQ_NONE; > > drm_crtc_handle_vblank(&scrtc->crtc); [Severity: High] If the interrupt fires solely due to FSYNC_INT, the handler will incorrectly proceed to call drm_crtc_handle_vblank() and sun4i_tcon_finish_page_flip(). Given that FSYNC_INT is described as constantly asserted, won't this translate into a storm of spurious vblank events that disrupts display timing and confuses userspace compositors? [ ... ] > @@ -1053,6 +1053,8 @@ static enum hrtimer_restart > sun4i_tcon_dsi_retrigger_timer_fn(struct hrtimer *ti > * atomic commit machinery. > */ > if (tick == 0 && tcon->irq) > enable_irq(tcon->irq); [Severity: Medium] Is there a corresponding disable_irq() call when the CRTC is disabled or during teardown? If a user triggers a DPMS off/on cycle, the timer will run again and call enable_irq() a second time, which might decrement the IRQ depth below zero and trigger a core IRQ subsystem warning and stack dump. [ ... ] > @@ -1124,10 +1124,11 @@ static int sun4i_tcon_init_irq(struct device *dev, > irq = platform_get_irq(pdev, 0); > if (irq < 0) > return irq; > > - ret = devm_request_irq(dev, irq, sun4i_tcon_handler, 0, > + ret = devm_request_irq(dev, irq, sun4i_tcon_handler, IRQF_NO_AUTOEN, > dev_name(dev), tcon); [Severity: Critical] Since enable_irq() is only called from the DSI-specific retrigger timer, won't requesting the interrupt with IRQF_NO_AUTOEN unconditionally for all TCON instances leave the vblank interrupt permanently disabled for all existing non-DSI panels (like LVDS, RGB, and HDMI), completely breaking page flips for those legacy boards? [ ... ] > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c > @@ -808,6 +808,10 @@ void sun6i_dsi_clear_int_status(struct sun6i_dsi *dsi) > { > unsigned int val = 0; > > regmap_read(dsi->regs, SUN6I_DSI_INT_REG, &val); > regmap_update_bits(dsi->regs, SUN6I_DSI_INT_REG, > GENMASK(31, 16), val & GENMASK(31, 16)); [Severity: High] Since SUN6I_DSI_INT_REG is a write-1-to-clear register, won't regmap_update_bits() skip the write if it detects that the cached value matches the new value? This optimization would cause the status bits to remain uncleared, breaking the edge detection logic in the retrigger timer. [ ... ] > @@ -898,12 +898,13 @@ static void sun6i_dsi_encoder_enable(struct drm_encoder > *encoder) > * puts the clock lane into its continuous-HS state, and DCS commands > * use the entirely separate LPTX/LPDT instruction tables. > */ > sun6i_dsi_start(dsi, DSI_START_HSC); > > udelay(1000); > > if (dsi->panel) > drm_panel_prepare(dsi->panel); [Severity: Critical] For legacy panels that require sending DCS commands during their prepare phase, won't moving DSI_START_HSC before drm_panel_prepare() cause initialization to fail? The host controller cannot transmit LPTX commands once the clock lane is forced into continuous HS mode. [ ... ] > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c > @@ -166,6 +166,10 @@ int sun8i_tcon_top_set_dsi_gate(struct device *dev, bool > enable) > > val = readl(tcon_top->regs + TCON_TOP_GATE_SRC_REG); > if (enable) > val |= BIT(TCON_TOP_TCON_DSI_GATE); > else > val &= ~BIT(TCON_TOP_TCON_DSI_GATE); > writel(val, tcon_top->regs + TCON_TOP_GATE_SRC_REG); [Severity: High] Because this bit is also registered as a standard clock with the common clock framework, won't the late-init clk_disable_unused() sweep see a reference count of zero and forcefully turn this gate off, wedging the display pipeline shortly after boot? [ ... ] > diff --git a/drivers/pwm/pwm-sun50i-a133.c b/drivers/pwm/pwm-sun50i-a133.c > --- a/drivers/pwm/pwm-sun50i-a133.c > +++ b/drivers/pwm/pwm-sun50i-a133.c > @@ -148,6 +148,9 @@ static int pwm_a133_apply(struct pwm_chip *chip, struct > pwm_device *pwm, > pwm_a133_writel(pc, pccr_reg, val); > > /* Gate the channel clock off while changing prescale/period/duty. */ > val = pwm_a133_readl(pc, PWM_A133_PCGR_REG); > val &= ~(PWM_A133_PCGR_GATING(ch) | PWM_A133_PCGR_BYPASS(ch)); > pwm_a133_writel(pc, PWM_A133_PCGR_REG, val); [Severity: Medium] Since the PWM core only serializes updates per channel, could concurrent updates to different channels on the same chip race and corrupt the hardware state here, potentially disabling the wrong channel? [ ... ] > @@ -156,6 +156,9 @@ static int pwm_a133_apply(struct pwm_chip *chip, struct > pwm_device *pwm, > val |= PWM_A133_PCR_ACT_STA_HIGH; > pwm_a133_writel(pc, PWM_A133_PCR_REG(ch), val); > > val = (entire_cycle << PWM_A133_PPR_ENTIRE_CYCLE_SHIFT) & > PWM_A133_PPR_ENTIRE_CYCLE_MASK; > val |= act_cycle & PWM_A133_PPR_ACT_CYCLE_MASK; > pwm_a133_writel(pc, PWM_A133_PPR_REG(ch), val); [Severity: Medium] If a 100 percent duty cycle is requested, act_cycle evaluates to 65536. Won't this bitwise AND with PWM_A133_PPR_ACT_CYCLE_MASK (which is 0xFFFF) truncate the value to 0, causing the backlight to turn completely off when maximum brightness is requested? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
