| Message ID | 20230116110820.2615650-1-treapking@chromium.org |
|---|---|
| Headers | show |
| Series | Add generic-display-mux driver and bindings | expand |
On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote: > From: Nicolas Boichat <drinkcat@chromium.org> > > Add bindings for Generic GPIO mux driver. > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > --- > > 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@chromium.org> > + > +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. 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. > + > + 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). > + > + 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 >
Hi Laurent, On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote: > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote: > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote: > > > > From: Nicolas Boichat <drinkcat@chromium.org> > > > > > > > > Add bindings for Generic GPIO mux driver. > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > > --- > > > > > > > > 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@chromium.org> > > > > + > > > > +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@mail.gmail.com/ > > > > > 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. > > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus > property of the next DT node in the OF graph ? Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a similar approach as well. [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500 [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240 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>; > > > > + }; > > > > + }; > > > > + }; > > > > + }; > > -- > Regards, > > Laurent Pinchart
Hello Pin-yen, On Tue, Feb 07, 2023 at 06:30:36PM +0800, Pin-yen Lin wrote: > On Tue, Feb 7, 2023 at 6:25 PM Laurent Pinchart wrote: > > On Tue, Feb 07, 2023 at 06:07:44PM +0800, Pin-yen Lin wrote: > > > On Wed, Jan 18, 2023 at 4:17 AM Rob Herring wrote: > > > > On Mon, Jan 16, 2023 at 07:08:19PM +0800, Pin-yen Lin wrote: > > > > > From: Nicolas Boichat <drinkcat@chromium.org> > > > > > > > > > > Add bindings for Generic GPIO mux driver. > > > > > > > > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > > > > Signed-off-by: Pin-yen Lin <treapking@chromium.org> > > > > > --- > > > > > > > > > > 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@chromium.org> > > > > > + > > > > > +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@mail.gmail.com/ > > > > > > > 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. > > > > How does that work, does the HDMI encoder driver parse the ddc-i2c-bus > > property of the next DT node in the OF graph ? > > Yes. In our use case, mtk_hdmi.c[2] checks the remote node of its > output port to get the bus phandle. sun4i_hdmi_enc.c[3] seems to use a > similar approach as well. Peeking into nodes of other devices is a bad practice. I don't know how the code you mention below got merged, but I'm pretty sure I would have flagged it if I had reviewed the patches :-) The ddc-i2c-bus property should instead be specified in the node where it logically belongs (in this case, the connector node), and handled by the connector driver. You can then use drm_bridge operations to tie things together, like done in the drm_bridge_connector helper. I'd recommend using the drm_bridge_connector helper if you can, either as-is, or by extending it. > [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/mediatek/mtk_hdmi.c#L1500 > [3]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c#L240 > > > > > > + > > > > > + 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>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + };
This is the v2 the mux driver part of v1. This series is developed for and tested on MT8173 board, whose layout looks like: /-- anx7688 -- MT8173 HDMI bridge -- GPIO mux \-- native HDMI v1: https://patchwork.kernel.org/project/dri-devel/cover/20191211061911.238393-1-hsinyi@chromium.org/ The other drm bridge callbacks is dropped in this version because: - The non-atomic callbacks are deprecated - It would be complicated to pass the atomic state to the downstream - We actually don't have the hardware to test them Changes in v2: - Referenced existing dt-binding schemas from graph.yaml - Added ddc-i2c-bus into the bindings - Dropped attach/mode_set/enable/disable callbacks - Fixed style issues - Removed the special case for the HDMI connector - Made the driver only read the GPIO status in IRQ handler - Rebased to drm-misc-next - Updated the license: "GPL v2" --> "GPL" Nicolas Boichat (2): dt-bindings: display: bridge: Add GPIO display mux binding drm: bridge: Generic GPIO mux driver .../bindings/display/bridge/gpio-mux.yaml | 95 +++++++++ drivers/gpu/drm/bridge/Kconfig | 10 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/generic-gpio-mux.c | 201 ++++++++++++++++++ 4 files changed, 307 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/gpio-mux.yaml create mode 100644 drivers/gpu/drm/bridge/generic-gpio-mux.c