On Mon, Oct 19, 2026 at 10:36:09AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] ARM: dts: imx6q-apalis: VGA data-enable-active ignored by
> simple-bridge
> --
>
> commit a66c8dd73182954ba43ed81cada90ae73edf93a0
> Author: Leonardo Costa <[email protected]>
>
> ARM: dts: imx6q-apalis: Add VGA overlay
>
> This commit adds a device tree overlay for the VGA output on the Apalis
> Evaluation Board, connecting the parallel RGB display interface to a
> non-programmable video DAC and VGA connector with an active-low data-enable
> signal, and wires up composite DTB targets in the Makefile.
>
> > diff --git a/arch/arm/boot/dts/nxp/imx/imx6q-apalis-eval-vga.dtso
> > b/arch/arm/boot/dts/nxp/imx/imx6q-apalis-eval-vga.dtso
> > new file mode 100644
> > index 0000000000000..991c2b7c27773
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/nxp/imx/imx6q-apalis-eval-vga.dtso
> > @@ -0,0 +1,80 @@
>
> [ ... ]
>
> > + vga-encoder {
> > + compatible = "dumb-vga-dac";
> > +
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + port@0 {
> > + reg = <0>;
> > +
> > + vga_enc_in: endpoint {
> > + data-enable-active = <0>;
> > + remote-endpoint = <&vga_display_out>;
> > + };
> > + };
>
> [Severity: High]
> Does data-enable-active = <0> on vga_enc_in actually reach the IPUv3
> CRTC?
>
> In simple_bridge_probe() in drivers/gpu/drm/bridge/simple-bridge.c, the
> data-enable-active property is parsed into timings->input_bus_flags:
>
> if (de_present && !sbridge->bridge.timings) {
> struct drm_bridge_timings *timings;
>
> timings = devm_kzalloc(&pdev->dev, sizeof(*timings),
> GFP_KERNEL);
> if (!timings)
> return -ENOMEM;
>
> timings->input_bus_flags = de ? DRM_BUS_FLAG_DE_HIGH :
> DRM_BUS_FLAG_DE_LOW;
> sbridge->bridge.timings = timings;
> }
>
> However, simple_bridge_bridge_funcs registers atomic bridge state
> callbacks without an .atomic_check callback:
>
> static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
> .atomic_create_state = drm_atomic_helper_bridge_create_state,
> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> .attach = simple_bridge_attach,
> .atomic_enable = simple_bridge_enable,
> .atomic_disable = simple_bridge_disable,
> };
>
> During drm_atomic_bridge_chain_check() ->
> drm_atomic_bridge_propagate_bus_flags(), sbridge's
> bridge_state->input_bus_cfg.flags is overwritten with the downstream
> display-connector bridge's input_bus_cfg.flags (which is 0), and
> sbridge->bridge.timings->input_bus_flags is never copied into
> bridge_state->input_bus_cfg.flags.
>
> Then, upstream in imx_pd_bridge_atomic_check() in
> drivers/gpu/drm/imx/ipuv3/parallel-display.c:
>
> if (next_bridge)
> next_bridge_state =
> drm_atomic_get_new_bridge_state(crtc_state->state,
>
> next_bridge);
>
> if (next_bridge_state)
> bus_flags = next_bridge_state->input_bus_cfg.flags;
> else
> bus_flags = di->bus_flags;
>
> Because next_bridge_state is non-NULL, bus_flags is read as 0 from
> next_bridge_state->input_bus_cfg.flags without consulting
> next_bridge->timings->input_bus_flags.
>
> When ipu_crtc_mode_set_nofb() -> ipu_di_init_sync_panel() runs,
> sig_cfg.enable_pol = !(imx_crtc_state->bus_flags & DRM_BUS_FLAG_DE_LOW)
> evaluates to 1, driving the data-enable pin active-high instead of
> active-low and gating off the VGA DAC output during active video.
>
> Should simple-bridge implement .atomic_check to populate
> bridge_state->input_bus_cfg.flags from bridge->timings->input_bus_flags
> so the active-low polarity is propagated to imx-parallel-display?
>
> [ ... ]
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/[email protected]?part=3
This and the other comment regarding atomic_check are addressed by the
patch series below, which is listed as a dependency for this patch series:
https://lore.kernel.org/all/20261002-v1-simple-bridge-atomic-check-v1-1-ef5d8332b...@toradex.com/T/#u
Leonardo
