Re: [RFC PATCH v2] arm64: allwinner: add TrimUI Smart Pro (A133) board support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] DPMS off/on cycles permanently disable the TCON VBLANK interrupt due to unbalanced enable/disable logic.
- [High] TRI_FINISH_ENABLE is explicitly disabled, making the hardware incapable of triggering subsequent frames without an interrupt storm.
- [High] The driver relies on a 1000ms wall-clock timer to sequence the initial DSI hardware trigger, creating a race condition against encoder initialization.
- [High] Sleeping functions (msleep, mutex_lock, disable_irq) are called from display driver atomic commit paths, violating subsystem rules.
--

commit 80c8d8e264d674c3f29098c9376ede5cd167b9ab
Author: leow149 <leow149@lucidmail.xyz>
arm64: allwinner: add TrimUI Smart Pro (A133) board support

This commit adds board support for the TrimUI Smart Pro handheld based
on the Allwinner A133 Plus SoC. It introduces a new device tree, a new
DSI panel driver, a new PWM driver, and modifies sun4i_tcon and 
sun6i_mipi_dsi to support the CPU/8080-interface DSI path.

> 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
[ ... ]
> @@ -221,6 +221,23 @@ void sun4i_tcon_set_status(struct sun4i_tcon *tcon,
>  		sun4i_tcon_lvds_set_status(tcon, encoder, true);
>  
>  	sun4i_tcon_channel_set_status(tcon, channel, enabled);
> +
> +	/*
> +	 * On disable, stop the DSI/CPU-mode retrigger timer before anything
> +	 * downstream (clocks, D-PHY) gets gated off. Left running, it keeps
> +	 * touching tcon->regs/dsi->regs every 2ms regardless of the CRTC's
> +	 * state; hitting gated hardware from there is a real bus-abort risk,
> +	 * not just wasted work. hrtimer_cancel() on an already-inactive timer
> +	 * is a safe no-op, so this is fine to call unconditionally rather
> +	 * than gating it on dsi_cpu_mode.
> +	 */
> +	if (!enabled) {
> +		hrtimer_cancel(&tcon->dsi_retrigger_timer);
> +		if (tcon->irq_enabled) {
> +			disable_irq(tcon->irq);
> +			tcon->irq_enabled = false;

[Severity: High]
Is it safe to call disable_irq() here?

This function is reached from atomic disable paths like
sun4i_crtc_atomic_disable. Since disable_irq() can sleep, calling it from
atomic context violates display driver rules and could cause deadlocks or
kernel warnings.

[Severity: High]
Does this disable path lead to permanently disabled VBLANK interrupts after
DPMS off/on cycles?

Because the IRQ is explicitly disabled and tcon->irq_enabled cleared here,
it needs to be re-enabled on resume. However, enable_irq() is only present
in sun4i_tcon_mode_set(). 

During a simple DPMS ON cycle (which only changes active state, not the
mode), DRM skips mode_set_nofb, bypassing sun4i_tcon_mode_set() entirely.
Could this leave the IRQ and timer permanently stopped after the display is
turned back on?

> +		}
> +	}
>  }
>  
>  void sun4i_tcon_enable_vblank(struct sun4i_tcon *tcon, bool enable)
> @@ -229,6 +246,18 @@ void sun4i_tcon_enable_vblank(struct sun4i_tcon *tcon, bool enable)
>  
>  	DRM_DEBUG_DRIVER("%sabling VBLANK interrupt\n", enable ? "En" : "Dis");
>  
> +	/*
> +	 * The DSI/CPU-interface path never touches TCON0's own GINT0
> +	 * enable bits: for this panel, the vendor implementation dispatches
> +	 * IRQ enable/query entirely to DSI's own interrupt register instead
> +	 * (confirmed against a live working reference system, where GINT0's
> +	 * enable bits read zero). The real per-frame retrigger source is
> +	 * SUN6I_DSI_INT_REG -- see sun6i_mipi_dsi.c and
> +	 * sun4i_tcon_dsi_retrigger_timer_fn() below.
> +	 */
> +	if (tcon->dsi_cpu_mode)
> +		return;

[Severity: High]
Does bypassing this hardware enable prevent frames from triggering without a
storm?

This early return prevents SUN4I_TCON_GINT0_TCON0_TRI_FINISH_ENABLE from being
set in SUN4I_TCON_GINT0_REG. However, the timer callback 
sun4i_tcon_dsi_retrigger_timer_fn() eventually stops itself when
TRIGGER_START self-clears, delegating continuous retriggering to the IRQ
handler. 

Since the hardware interrupt enable bit is skipped here, the TCON will never
assert the interrupt line when a frame finishes. Could the retrigger loop
only be functioning right now because of the unintended FSYNC_INT hardware
storm keeping the IRQ line asserted?

> +
>  	mask = SUN4I_TCON_GINT0_VBLANK_ENABLE(0) |
>  		SUN4I_TCON_GINT0_VBLANK_ENABLE(1) |
>  		SUN4I_TCON_GINT0_TCON0_TRI_FINISH_ENABLE;
[ ... ]
> @@ -402,24 +556,61 @@ static void sun4i_tcon0_mode_set_cpu(struct sun4i_tcon *tcon,
>  	regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI1_REG,
>  		     SUN4I_TCON0_CPU_TRI1_BLOCK_NUM(mode->vdisplay));
>  
[ ... ]
> +	/*
> +	 * This timer's first tick fires the real first TRIGGER_START, once
> +	 * sun6i_dsi_encoder_enable() has actually finished rather than just
> +	 * started -- its drm_panel_prepare() call alone (reset pulse + DCS
> +	 * init) totals close to 300ms of msleep() for this panel. 1000ms
> +	 * gives ample margin over that as a one-time cost at boot.
> +	 */
> +	tcon->dsi_retrigger_ticks = 0;
> +	hrtimer_start(&tcon->dsi_retrigger_timer, ms_to_ktime(1000),
> +		      HRTIMER_MODE_REL);

[Severity: High]
Could this wall-clock timer race against encoder initialization?

sun4i_tcon0_mode_set_cpu() arms this timer to fire in 1000ms, while the
encoder's atomic_enable hook (sun6i_dsi_encoder_enable) prepares the panel.
If the process-context encoder initialization takes longer than 1000ms due
to system load, scheduler delays, or slow panel interactions, this timer
callback will fire in hardirq context concurrently. 

This would cause the timer to call sun6i_dsi_tri_start(), overwriting
SUN6I_DSI_INST_JUMP_SEL_REG while the process context is actively writing
to the same register during DCS transfers. Can this corrupt the DSI
controller's instruction state machine?

> +
> +	/*
> +	 * enable_irq(tcon->irq) is deliberately not called here:
[ ... ]
> diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm1289a.c b/drivers/gpu/drm/panel/panel-orisetech-otm1289a.c
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-orisetech-otm1289a.c
> @@ -0,0 +1,424 @@
[ ... ]
> +static int otm1289a_prepare(struct drm_panel *panel)
> +{
> +	struct otm1289a *ctx = panel_to_otm1289a(panel);
> +	int ret;
> +
> +	ret = regulator_enable(ctx->supply);
> +	if (ret < 0) {
> +		dev_err(panel->dev, "failed to enable supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (ctx->reset_gpio) {
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		msleep(20);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +		msleep(100);

[Severity: High]
Are these sleeping delays safe to call from atomic context?

Subsystem guidelines forbid sleeping in CRTC, plane, and encoder atomic
callbacks during non-blocking commits. This panel driver's prepare() uses
msleep() directly. 

It is invoked from sun6i_dsi_encoder_enable() via drm_panel_prepare(),
which runs in atomic commit paths. Could this trigger kernel warnings or
system instability by sleeping where it isn't allowed?

> +	}
> +
> +	ret = otm1289a_init_sequence_send(ctx);
> +	if (ret) {
[ ... ]
> diff --git a/drivers/pwm/pwm-sun50i-a133.c b/drivers/pwm/pwm-sun50i-a133.c
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i-a133.c
> @@ -0,0 +1,317 @@
[ ... ]
> +static int pwm_a133_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
> +	struct pwm_a133 *pc = to_pwm_a133(chip);
> +	unsigned int ch = pwm->hwpwm;
> +	u32 entire_cycle, act_cycle, prescale_k, val;
> +	u32 pccr_reg = (ch < 2) ? PWM_A133_PCCR01_REG : PWM_A133_PCCR23_REG;
> +	int ret;
> +
> +	if (!state->enabled) {
> +		mutex_lock(&pc->lock);
> +		val = pwm_a133_readl(pc, PWM_A133_PER_REG);
> +		val &= ~PWM_A133_PER_EN(ch);
> +		pwm_a133_writel(pc, PWM_A133_PER_REG, val);
> +		mutex_unlock(&pc->lock);
> +		return 0;
> +	}
> +
> +	ret = pwm_a133_calc(state->period, state->duty_cycle, &entire_cycle,
> +			    &act_cycle, &prescale_k);
> +	if (ret) {
> +		dev_err(pwmchip_parent(chip), "period exceeds the maximum value\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * PWM_A133_PCCR01_REG/PCCR23_REG (shared by a channel pair) and
> +	 * PWM_A133_PCGR_REG/PWM_A133_PER_REG (shared by all 16 channels) are
> +	 * each read-modify-written below; the PWM core doesn't serialize
> +	 * .apply() calls across different channels on the same chip, so this
> +	 * whole sequence needs its own lock against a concurrent apply() on
> +	 * another channel racing on the same register.
> +	 */
> +	mutex_lock(&pc->lock);

[Severity: High]
Is it safe to acquire a mutex in this PWM apply callback?

When used as a backlight, pwm_a133_apply() can be reached from the display
driver's atomic commit tail: sun6i_dsi_encoder_enable() ->
drm_panel_enable() -> backlight_enable(). 

Because encoder atomic callbacks run in atomic context where sleeping is
forbidden, taking a mutex here could lead to deadlocks or scheduling while
atomic errors.

> +
> +	/* Clock source: OSC24M for the whole pair this channel belongs to. */
> +	val = pwm_a133_readl(pc, pccr_reg);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912075706.86857-1-leow149@lucidmail.xyz?part=1