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
