Re: [PATCH] drm/bridge: ps8640: Drop the ability of ps8640 to fetch the EDID

[Sam Ravnborg <sam@ravnborg.org>]

Hi Douglas,

On Mon, Jun 12, 2023 at 04:32:57PM -0700, Douglas Anderson wrote:
> In order to read the EDID from an eDP panel, you not only need to
> power on the bridge chip itself but also the panel. In the ps8640
> driver, this was made to work by having the bridge chip manually power
> the panel on by calling pre_enable() on everything connectorward on
> the bridge chain. This worked OK, but...
> 
> ...when trying to do the same thing on ti-sn65dsi86, feedback was that
> this wasn't a great idea. As a result, we designed the "DP AUX"
> bus. With the design we ended up with the panel driver itself was in
> charge of reading the EDID. The panel driver could power itself on and
> the bridge chip was able to power itself on because it implemented the
> DP AUX bus.
> 
> Despite the fact that we came up with a new scheme, implemented in on
> ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
> the old code around. This was because the new scheme required a DT
> change. Previously the panel was a simple "platform_device" and in DT
> at the top level. With the new design the panel needs to be listed in
> DT under the DP controller node. The old code allowed us to properly
> fetch EDIDs with ps8640 with the old DTs.
> 
> Unfortunately, the old code stopped working as of commit 102e80d1fa2c
> ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
> are cases at bootup where connector->state->state is NULL and the
> kernel crashed at:
> * drm_atomic_bridge_chain_pre_enable
> * drm_atomic_get_old_bridge_state
> * drm_atomic_get_old_private_obj_state
> 
> A bit of digging was done to see if there was an easy fix but there
> was nothing obvious. Instead, the only device using ps8640 the "old"
> way had its DT updated so that the panel was no longer a simple
> "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
> number in DT").
> 
> Let's delete the old, crashing code so nobody gets tempted to copy it
> or figure out how it works (since it doesn't).
> 
> NOTE: from a device tree "purist" point of view, we're supposed to
> keep old device trees working and this patch is technically "against
> policy". Reasons I'm still proposing it anyway:
> 1. Officially, old mt8173-elm device trees worked via the "little
>    white lie" approach. The DT would list an arbitrary/representative
>    panel that would be used for power sequencing. The mode information
>    in the panel driver would then be ignored / overridden by the EDID
>    reading code in ps8640. I don't feel too terrible breaking DTs that
>    contained the wrong "compatible" string to begin with. NOTE that
>    any old device trees that _didn't_ lie about their compatible will
>    still work because the mode information will come from the
>    hardcoded panels in panel-edp.
> 2. The only users of the old code were Chromebooks and Chromebooks
>    don't bake their DTs into the BIOS (they are bundled with the
>    kernel). Thus we don't need to worry about breaking someone using
>    an old DT with a new kernel.
> 3. The old code was crashing anyway. If someone wants to fix the old
>    code instead of deleting it then they have my blessing, but without
>    a proper fix the old code isn't useful.
> 
> I'll list this as "Fixing" the code that made the old code start
> failing. There's not lots of reason to bring this back any further
> than that.
> 
> Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thanks for fixing this!
Change looks good, and I like that it deletes stuff.

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>