Quoting Pin-yen Lin (2023-04-13 02:50:44) > 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. The panel is connected to the bridge in the graph. I think we should assume the eDP panel is on an aux-bus. Maybe another scenario is a design that has a DP to HDMI bridge wired down on the device? In which case the output port would be connected to the HDMI bridge. > > > > 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? Omitting the mode-switch property would require changes to the type-c framework. I'm wondering if we can have this anx driver register mode switches for however many endpoints exist in the output port all the time when the aux-bus node doesn't exist. Then the type-c framework can walk from the usb-c-connector to each connected node looking for a device that is both a drm_bridge and a mode-switch. When it finds that combination, it knows that the mode-switch has been found. This hinges on the idea that a device that would have the mode-switch property is a drm_bridge and would register a mode-switch with the type-c framework. It may be a little complicated though, because we would only register one drm_bridge for the input to this anx device. The type-c walking code would need to look at the graph endpoint, and find the parent device to see if it is a drm_bridge.
