Re: [PATCH v15 04/10] dt-bindings: display: bridge: anx7625: Add mode-switch support

[Pin-yen Lin]

Hi Stephen,

On Wed, Apr 12, 2023 at 10:38 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Pin-yen Lin (2023-03-31 02:11:39)
> > diff --git a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > index b42553ac505c..604c7391d74f 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > @@ -12,7 +12,8 @@ maintainers:
> >
> >  description: |
> >    The ANX7625 is an ultra-low power 4K Mobile HD Transmitter
> > -  designed for portable devices.
> > +  designed for portable devices. Product brief is available at
> > +  https://www.analogix.com/en/system/files/AA-002291-PB-6-ANX7625_ProductBrief.pdf
> >
> >  properties:
> >    compatible:
> > @@ -112,9 +113,40 @@ properties:
> >                data-lanes: true
> >
> >        port@1:
> > -        $ref: /schemas/graph.yaml#/properties/port
> > +        $ref: /schemas/graph.yaml#/$defs/port-base
> >          description:
> > -          Video port for panel or connector.
> > +          Video port for panel or connector. Each endpoint connects to a video
> > +          output downstream, and the "data-lanes" property is used to describe
> > +          the pin connections. 0, 1, 2, 3 in "data-lanes" maps to SSRX1, SSTX1,
> > +          SSRX2, SSTX2, respectively.
> > +
> > +        patternProperties:
> > +          "^endpoint@[01]$":
> > +            $ref: /schemas/media/video-interfaces.yaml#
> > +            properties:
> > +              reg: true
> > +
> > +              remote-endpoint: true
> > +
> > +              data-lanes:
> > +                oneOf:
> > +                  - items:
> > +                      - enum: [0, 1, 2, 3]
> > +
> > +                  - items:
> > +                      - const: 0
> > +                      - const: 1
> > +
> > +                  - items:
> > +                      - const: 2
> > +                      - const: 3
> > +
> > +              mode-switch:
>
> Is it possible to not have this property? Can we have the driver for
> this anx device look at the remote-endpoint and if it sees that it is
> not a drm_bridge or panel on the other end, or a DP connector, that it
> should register a typec mode switch (or two depending on the number of
> endpoints in port@1)? Is there any case where that doesn't hold true?
>
> I see these possible scenarios:
>
> 1. DPI to DP bridge steering DP to one of two usb-c-connectors
>
> In this case, endpoint@0 is connected to one usb-c-connector and
> endpoint@1 is connected to another usb-c-connector. The input endpoint
> is only connected to DPI. The USB endpoint is not present (although I
> don't see this described in the binding either, so we would need a
> port@2, entirely optional to describe USB3 input). The driver will
> register two mode switches.
>
> 2. DPI to DP bridge with USB3 to one usb-c-connector
>
> In this case, endpoint@1 doesn't exist. The SSTX1/2 and SSRX1/2 pins are
> all connected to a usb-c-connector node. The input ports (0 and 2) are
> connected to both DPI and USB. The device acts as both a mode-switch and
> an orientation-switch. It registers both switches. I wonder if there is
> any benefit to describing SBU connections or CC connections? Maybe we
> don't register the orientation-switch if the SBU or CC connection isn't
> described?
>
> 3. DPI to DP bridge connected to eDP panel
>
> In this case, endpoint@1 doesn't exist. The USB endpoint is not present
> (port@2). Depending on how the crosspoint should be configured, we'll
> need to use data-lanes in the port@1 endpoint to describe which SSTRX
> pair to use (1 or 2). Or we'll have to use the endpoint's reg property
> to describe which pair to drive DP on. Presumably the default
> configuration is SSRX2/SSTX2 providing 2 lanes of DP to an eDP panel.
> The endpoint@0 in port@1 will be connected to a drm_panel, and the
> driver will be able to detect this properly by checking for the
> existence of an aux-bus node or the return value of
> of_dp_aux_populate_bus().

Can we assume that the eDP panel always stays behind an `aux-bus`
node? Can't the panel be connected to the bridge directly in the
graph? Though this might not matter if we only register mode switches
when there are usb-c-connectors connected.
>
> 4. DPI to DP bridge connected to DP connector
>
> This is similar to the eDP panel scenario #3. In this case, endpoint@1
> doesn't exist. The USB endpoint is not present (port@2). Same story
> about port@1 and lane configuration, but we don't have an aux-bus node.
> In this case, the drivers/gpu/drm/bridge/display-connector.c driver will
> probe for the dp-connector node and add a drm_bridge. This anx driver
> will similarly add a drm_bridge, but it needs to look at the node
> connected on port@1:endpoint@0 with drm_of_get_bridge() and check if it
> is a drm_bridge (DP connector) or if it is some type-c thing (connector
> or orientation-switch).
>
> I think having this mode-switch property here lets us avoid calling
> drm_of_get_bridge() unconditionally in anx7625_parse_dt().
> drm_of_get_bridge() will always return -EPROBE_DEFER when this is the
> last drm_bridge in the chain and the other side of the endpoint is a
> type-c thing (scenarios #1 and #2). Maybe we should teach
> drm_of_get_bridge() that a drm_bridge might be connected to a type-c
> device and have it not return -EPROBE_DEFER in that case. Or make some
> new API like drm_of_get_bridge_typec() that checks if the typec
> framework knows about the endpoint in question (as either a typec switch
> or a connector) and returns a NULL bridge pointer. If we had that then I
> think this property is not necessary.
>
> Hopefully the usb-c-connector can always be registered with the typec
> framework? I'm worried that the driver that registers the
> usb-c-connector node may want to form a struct typec_port with
> typec_register_port() and that will get stuck in a similar -EPROBE_DEFER
> loop waiting for this mode-switch to appear. So having this property
> also avoids that problem by telling typec framework to wait until this
> driver can register a mode-switch.
>
> TL;DR: Is this mode-switch property a workaround for probe defer? Can we
> figure out where the mode switch is in software and not have the
> property in DT? If we can it would certainly improve things because
> forgetting to add the property can lead to broken behavior, and we don't
> do anything like this for chains of drm_bridge devices. We just describe
> the display chain and let the kernel figure out which bridge should
> handle hpd, edid reading, or mode detection, etc.

Actually the `mode-switch` property here is mainly because
`fwnode_typec_mux_get`[1] and `typec_mux_match`[2] only return matches
when the property is present. I am not sure what side effects would be
if I remove the ID-matching condition in `typec_mux_match`, so I added
the property here.

Is it feasible to remove the `mode-switch` property here given the
existing implementation of the Type-C framework?

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/mux.c#L351
[2]: https://elixir.bootlin.com/linux/latest/source/drivers/usb/typec/mux.c#L290

Best regards,
Pin-yen