Re: [PATCH v2 1/2] dt-bindings: display: bridge: Add GPIO display mux binding

[Pin-yen Lin]

Hi Rob,

Thanks for the review.


On Wed, Jan 18, 2023 at 4:17 AM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote:
> > From: Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
> >
> > Add bindings for Generic GPIO mux driver.
> >
> > Signed-off-by: Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
> > Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - Referenced existing dt-binding schemas from graph.yaml
> > - Added ddc-i2c-bus into the bindings
> >
> >  .../bindings/display/bridge/gpio-mux.yaml     | 95 +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > new file mode 100644
> > index 000000000000..da29ba078f05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/bridge/gpio-mux.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic display mux (1 input, 2 outputs)
> > +
> > +maintainers:
> > +  - Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
> > +
> > +description: |
> > +  This bindings describes a simple display (e.g. HDMI) mux, that has 1
> > +  input, and 2 outputs. The mux status is controlled by hardware, and
> > +  its status is read back using a GPIO.
> > +
> > +properties:
> > +  compatible:
> > +    const: gpio-display-mux
> > +
> > +  detect-gpios:
> > +    maxItems: 1
> > +    description: GPIO that indicates the active output
>
> What are we detecting? That implies an input, but this is selecting the
> output path, right? Or what does 'mux status is controlled by hardware'
> mean exactly? Something else? That does not sound very generic.

The GPIO (or any kind of MUX) is an input that indicates where the
output should go. The actual "output selection" procedure is done in
the driver. That is, the driver monitors this GPIO and selects the
output path accordingly. In our use case, the GPIO is reported by the
embedded controller on the device.

[1] listed other similar bridges that can leverage this driver, so we
called this driver "generic".

[1]: https://lore.kernel.org/all/CAJMQK-jGw8kJFNjoHjeZUL+3NCiOS2hgGERnAnMwNsL_cm_J=Q@xxxxxxxxxxxxxx/

>
> In any case, we have a common mux binding so any kind of mux control
> could be used here, not just GPIO. Then you can make this just a generic
> display mux.

Thanks for sharing this, I'll update the binding in the next version.
>
> > +
> > +  ddc-i2c-bus:
> > +    description: phandle link to the I2C controller used for DDC EDID probing
> > +    $ref: /schemas/types.yaml#/definitions/phandle
>
> This belongs in the connector node(s).

The HDMI bridge before the MUX doesn't (and doesn't have to) know that
its next bridge is a MUX. We put it here so that the HDMI bridge can
parse the phandle and get the bus node.

Regards,
Pin-yen

>
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: |
> > +          Video port for input.
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: |
> > +          2 video ports for output.
> > +          The reg value in the endpoints matches the GPIO status: when
> > +          GPIO is asserted, endpoint with reg value <1> is selected.
> > +
> > +    required:
> > +      - port@0
> > +      - port@1
> > +
> > +required:
> > +  - compatible
> > +  - detect-gpios
> > +  - ports
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    hdmi_mux: hdmi_mux {
> > +      compatible = "gpio-display-mux";
> > +      detect-gpios = <&pio 36 GPIO_ACTIVE_HIGH>;
> > +      pinctrl-names = "default";
> > +      pinctrl-0 = <&hdmi_mux_pins>;
> > +      ddc-i2c-bus = <&hdmiddc0>;
> > +
> > +      ports {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        port@0 { /* input */
> > +          reg = <0>;
> > +
> > +          hdmi_mux_in: endpoint {
> > +            remote-endpoint = <&hdmi0_out>;
> > +          };
> > +        };
> > +
> > +        port@1 { /* output */
> > +          reg = <1>;
> > +
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          hdmi_mux_out_anx: endpoint@0 {
> > +            reg = <0>;
> > +            remote-endpoint = <&dp_bridge_in>;
> > +          };
> > +
> > +          hdmi_mux_out_hdmi: endpoint@1 {
> > +            reg = <1>;
> > +            remote-endpoint = <&hdmi_connector_in>;
> > +          };
> > +        };
> > +      };
> > +    };
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >