[PATCH v3 3/5] drm/bridge: Introduce pre_enable_prev_first to alter bridge init order

[Frieder Schrempf]

Hi Dave,

On 02.12.22 15:28, Dave Stevenson wrote:
> DSI sink devices typically want the DSI host powered up and configured
> before they are powered up. pre_enable is the place this would normally
> happen, but they are called in reverse order from panel/connector towards
> the encoder, which is the "wrong" order.
> 
> Add a new flag pre_enable_prev_first that any bridge can set
> to swap the order of pre_enable (and post_disable) for that and the
> immediately previous bridge.
> Should the immediately previous bridge also set the
> pre_enable_prev_first flag, the previous bridge to that will be called
> before either of those which requested pre_enable_prev_first.
> 
> eg:
> - Panel
> - Bridge 1
> - Bridge 2 pre_enable_prev_first
> - Bridge 3
> - Bridge 4 pre_enable_prev_first
> - Bridge 5 pre_enable_prev_first
> - Bridge 6
> - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
> Bridge 2, Bridge 6, Bridge 5, Bridge 4, Encoder.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson at raspberrypi.com>

I tested this patchset together with the pending Samsung DSIM bridge
conversion for i.MX8MM [1] and additional patches to fix the
enable/disable sequence for my setup [2] on a Kontron DL i.MX8MM board
featuring a TI SN65DSI84 and a 7" LVDS panel.

Using this the initialization sequence of the TI SN65DSI84 specified in
the datasheet can be achieved. The correct order was verified by tracing
the function calls and capturing the DSI data signal and the SN65DSI84
enable GPIO (EN) on a scope.

I've also gone through the code of this patch and it all makes sense to
me. There is only one small nitpick (see below).

Tested-by: Frieder Schrempf <frieder.schrempf at kontron.de>
Reviewed-by: Frieder Schrempf <frieder.schrempf at kontron.de>

Thanks
Frieder

[1]
https://patchwork.kernel.org/project/dri-devel/cover/20221110183853.3678209-1-jagan@amarulasolutions.com/
[2]
https://git.kontron-electronics.de/sw/misc/linux/-/commits/v6.1-dsim-mx8mm

> ---
>  drivers/gpu/drm/drm_bridge.c | 144 +++++++++++++++++++++++++++++------
>  include/drm/drm_bridge.h     |   8 ++
>  2 files changed, 128 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index bb7fc09267af..41051869d6bf 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -581,6 +581,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
[...]>
> -			bridge->funcs->atomic_post_disable(bridge,
> -							   old_bridge_state);
> -		} else if (bridge->funcs->post_disable) {
> -			bridge->funcs->post_disable(bridge);
> -		}
> +		if (limit)
> +			bridge = limit;

Nit: Maybe add a comment for the code above to explain that this is for
skipping the already post_disabled bridge. Just like for pre_enable below?

>  	}
>  }
>  EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>  
[...]