| Message ID | 1f6bf58d76efc2e869b800534b818d1451ef98a2.1634822085.git.geert+renesas@glider.be |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-bindings: display: bridge: nxp,tda998x: Json-schema conversion and fixes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 108 lines checked |
| robh/dt-meta-schema | success | |
| robh/dtbs-check | fail | build log |
On Thu, Oct 21, 2021 at 03:18:53PM +0200, Geert Uytterhoeven wrote: > Convert the NXP TDA998x HDMI transmitter Device Tree binding > documentation to json-schema. > > Add missing "#sound-dai-cells" property. > Add ports hierarchy, as an alternative to port. > Drop pinctrl properties, as they do not belong here. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > RFC as I do not know: > 1. The correct value(s) for '#sound-dai-cells' (the example used 2, > which was IMHO wrong, while all actual users use 0), > 2. The meaning of the various ports subnodes. > --- > .../bindings/display/bridge/nxp,tda998x.yaml | 108 ++++++++++++++++++ > .../bindings/display/bridge/tda998x.txt | 54 --------- > 2 files changed, 108 insertions(+), 54 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml > delete mode 100644 Documentation/devicetree/bindings/display/bridge/tda998x.txt > > diff --git a/Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml b/Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml > new file mode 100644 > index 0000000000000000..87c64edcf5d5617d > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml > @@ -0,0 +1,108 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/bridge/nxp,tda998x.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: NXP TDA998x HDMI transmitter > + > +maintainers: > + - Russell King <linux@armlinux.org.uk> > + > +properties: > + compatible: > + const: nxp,tda998x > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + video-ports: > + default: 0x230145 > + description: > + 24 bits value which defines how the video controller output is wired to > + the TDA998x input. maximum: 0xffffff (at least...) > + > + audio-ports: > + description: > + Array of 8-bit values, 2 values per DAI (Documentation/sound/soc/dai.rst). > + The implementation allows one or two DAIs. > + If two DAIs are defined, they must be of different type. > + $ref: /schemas/types.yaml#/definitions/uint32-matrix > + items: > + minItems: 1 > + maxItems: 2 > + items: > + - description: | > + The first value defines the DAI type: TDA998x_SPDIF or TDA998x_I2S > + (see include/dt-bindings/display/tda998x.h). > + - description: > + The second value defines the tda998x AP_ENA reg content when the > + DAI in question is used. > + > + '#sound-dai-cells': > + enum: [ 0, 1 ] > + > + nxp,calib-gpios: > + maxItems: 1 > + description: > + Calibration GPIO, which must correspond with the gpio used for the > + TDA998x interrupt pin. > + > + port: > + $ref: /schemas/graph.yaml#/properties/port > + > + ports: > + $ref: /schemas/graph.yaml#/properties/ports > + > + properties: > + port@0: > + type: object > + description: FIXME Looks like the input from the example > + > + port@1: > + type: object > + description: FIXME Presumably the output to connector or another bridge. > + > +required: > + - compatible > + - reg > + > +oneOf: > + - required: > + - port > + - required: > + - ports > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/display/tda998x.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + tda998x: hdmi-encoder@70 { > + compatible = "nxp,tda998x"; > + reg = <0x70>; > + interrupt-parent = <&gpio0>; > + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; > + video-ports = <0x230145>; > + > + #sound-dai-cells = <1>; > + /* DAI-format / AP_ENA reg value */ > + audio-ports = <TDA998x_SPDIF 0x04>, > + <TDA998x_I2S 0x03>; > + > + port { > + tda998x_in: endpoint { > + remote-endpoint = <&lcdc_0>; > + }; > + }; > + }; > + };
On Thu, Oct 28, 2021 at 08:04:48PM -0500, Rob Herring wrote: > On Thu, Oct 21, 2021 at 03:18:53PM +0200, Geert Uytterhoeven wrote: > > + properties: > > + port@0: > > + type: object > > + description: FIXME > > Looks like the input from the example > > > + > > + port@1: > > + type: object > > + description: FIXME > > Presumably the output to connector or another bridge. This is changing the binding. The original had: Required node: - port: Input port node with endpoint definition, as described in Documentation/devicetree/bindings/graph.txt The above change appears to require that tda998x now has two ports. This goes against current usage in DT and the example.
Hi Russell, Thanks for your comments! On Fri, Oct 29, 2021 at 10:08 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Thu, Oct 28, 2021 at 08:04:48PM -0500, Rob Herring wrote: > > On Thu, Oct 21, 2021 at 03:18:53PM +0200, Geert Uytterhoeven wrote: > > > + properties: > > > + port@0: > > > + type: object > > > + description: FIXME > > > > Looks like the input from the example > > > > > + > > > + port@1: > > > + type: object > > > + description: FIXME > > > > Presumably the output to connector or another bridge. > > This is changing the binding. The original had: > > Required node: > - port: Input port node with endpoint definition, as described > in Documentation/devicetree/bindings/graph.txt Indeed, cfr. "Add ports hierarchy, as an alternative to port." in the patch description. Some users use port, other use ports with one or two port subnodes. > The above change appears to require that tda998x now has two ports. No, you can still use port: +oneOf: + - required: + - port + - required: + - ports When using ports, no further requirements are set, but perhaps port@0 should be made required in that case? > This goes against current usage in DT and the example. The original example didn't even have the original required input port node, so it was incomplete. arch/arm64/boot/dts/renesas/r8a774c0-cat874.dts has two port subnodes. Is that wrong? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Oct 29, 2021 at 10:28:22AM +0200, Geert Uytterhoeven wrote: > Hi Russell, > > Thanks for your comments! > > No, you can still use port: > > +oneOf: > + - required: > + - port > + - required: > + - ports > > When using ports, no further requirements are set, but perhaps port@0 > should be made required in that case? Maybe I don't understand the binding description then, but to me it looks like you require both port@0 and port@1. The reality of the driver is that it makes almost no use of the graph itself, except via drm_of_find_possible_crtcs() to find the connected CRTCs. If it is connected to an I2S source, then it probably needs a port specification for that. If someone wants to describe the HDMI connector (which I don't see any point in) then they likely need a port specification for that too. However, the driver itself doesn't care about any of those. So, describing the port nodes makes no sense.
Hi Russell, On Fri, Oct 29, 2021 at 11:33 AM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Oct 29, 2021 at 10:28:22AM +0200, Geert Uytterhoeven wrote: > > No, you can still use port: > > > > +oneOf: > > + - required: > > + - port > > + - required: > > + - ports > > > > When using ports, no further requirements are set, but perhaps port@0 > > should be made required in that case? > > Maybe I don't understand the binding description then, but to me it > looks like you require both port@0 and port@1. "make dtbs_check" disagrees. > The reality of the driver is that it makes almost no use of the graph > itself, except via drm_of_find_possible_crtcs() to find the connected > CRTCs. If it is connected to an I2S source, then it probably needs a > port specification for that. If someone wants to describe the HDMI > connector (which I don't see any point in) then they likely need a I can't comment on the point of describing the HDMI connector. > port specification for that too. However, the driver itself doesn't > care about any of those. DT describes hardware, not software limitations. > So, describing the port nodes makes no sense. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Oct 29, 2021 at 11:40:26AM +0200, Geert Uytterhoeven wrote: > Hi Russell, > > On Fri, Oct 29, 2021 at 11:33 AM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Oct 29, 2021 at 10:28:22AM +0200, Geert Uytterhoeven wrote: > > > No, you can still use port: > > > > > > +oneOf: > > > + - required: > > > + - port > > > + - required: > > > + - ports > > > > > > When using ports, no further requirements are set, but perhaps port@0 > > > should be made required in that case? > > > > Maybe I don't understand the binding description then, but to me it > > looks like you require both port@0 and port@1. > > "make dtbs_check" disagrees. > > > The reality of the driver is that it makes almost no use of the graph > > itself, except via drm_of_find_possible_crtcs() to find the connected > > CRTCs. If it is connected to an I2S source, then it probably needs a > > port specification for that. If someone wants to describe the HDMI > > connector (which I don't see any point in) then they likely need a > > I can't comment on the point of describing the HDMI connector. > > > port specification for that too. However, the driver itself doesn't > > care about any of those. > > DT describes hardware, not software limitations. Sigh. There's no point discussing this further, my replies seem to be interpreted out of context.
diff --git a/Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml b/Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml new file mode 100644 index 0000000000000000..87c64edcf5d5617d --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml @@ -0,0 +1,108 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/nxp,tda998x.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: NXP TDA998x HDMI transmitter + +maintainers: + - Russell King <linux@armlinux.org.uk> + +properties: + compatible: + const: nxp,tda998x + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + video-ports: + default: 0x230145 + description: + 24 bits value which defines how the video controller output is wired to + the TDA998x input. + + audio-ports: + description: + Array of 8-bit values, 2 values per DAI (Documentation/sound/soc/dai.rst). + The implementation allows one or two DAIs. + If two DAIs are defined, they must be of different type. + $ref: /schemas/types.yaml#/definitions/uint32-matrix + items: + minItems: 1 + maxItems: 2 + items: + - description: | + The first value defines the DAI type: TDA998x_SPDIF or TDA998x_I2S + (see include/dt-bindings/display/tda998x.h). + - description: + The second value defines the tda998x AP_ENA reg content when the + DAI in question is used. + + '#sound-dai-cells': + enum: [ 0, 1 ] + + nxp,calib-gpios: + maxItems: 1 + description: + Calibration GPIO, which must correspond with the gpio used for the + TDA998x interrupt pin. + + port: + $ref: /schemas/graph.yaml#/properties/port + + ports: + $ref: /schemas/graph.yaml#/properties/ports + + properties: + port@0: + type: object + description: FIXME + + port@1: + type: object + description: FIXME + +required: + - compatible + - reg + +oneOf: + - required: + - port + - required: + - ports + +additionalProperties: false + +examples: + - | + #include <dt-bindings/display/tda998x.h> + #include <dt-bindings/interrupt-controller/irq.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + tda998x: hdmi-encoder@70 { + compatible = "nxp,tda998x"; + reg = <0x70>; + interrupt-parent = <&gpio0>; + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; + video-ports = <0x230145>; + + #sound-dai-cells = <1>; + /* DAI-format / AP_ENA reg value */ + audio-ports = <TDA998x_SPDIF 0x04>, + <TDA998x_I2S 0x03>; + + port { + tda998x_in: endpoint { + remote-endpoint = <&lcdc_0>; + }; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/display/bridge/tda998x.txt b/Documentation/devicetree/bindings/display/bridge/tda998x.txt deleted file mode 100644 index f5a02f61dd36f1c6..0000000000000000 --- a/Documentation/devicetree/bindings/display/bridge/tda998x.txt +++ /dev/null @@ -1,54 +0,0 @@ -Device-Tree bindings for the NXP TDA998x HDMI transmitter - -Required properties; - - compatible: must be "nxp,tda998x" - - - reg: I2C address - -Required node: - - port: Input port node with endpoint definition, as described - in Documentation/devicetree/bindings/graph.txt - -Optional properties: - - interrupts: interrupt number and trigger type - default: polling - - - pinctrl-0: pin control group to be used for - screen plug/unplug interrupt. - - - pinctrl-names: must contain a "default" entry. - - - video-ports: 24 bits value which defines how the video controller - output is wired to the TDA998x input - default: <0x230145> - - - audio-ports: array of 8-bit values, 2 values per one DAI[1]. - The first value defines the DAI type: TDA998x_SPDIF or TDA998x_I2S[2]. - The second value defines the tda998x AP_ENA reg content when the DAI - in question is used. The implementation allows one or two DAIs. If two - DAIs are defined, they must be of different type. - - - nxp,calib-gpios: calibration GPIO, which must correspond with the - gpio used for the TDA998x interrupt pin. - -[1] Documentation/sound/soc/dai.rst -[2] include/dt-bindings/display/tda998x.h - -Example: - -#include <dt-bindings/display/tda998x.h> - - tda998x: hdmi-encoder { - compatible = "nxp,tda998x"; - reg = <0x70>; - interrupt-parent = <&gpio0>; - interrupts = <27 2>; /* falling edge */ - pinctrl-0 = <&pmx_camera>; - pinctrl-names = "default"; - video-ports = <0x230145>; - - #sound-dai-cells = <2>; - /* DAI-format AP_ENA reg value */ - audio-ports = < TDA998x_SPDIF 0x04 - TDA998x_I2S 0x03>; - - };
Convert the NXP TDA998x HDMI transmitter Device Tree binding documentation to json-schema. Add missing "#sound-dai-cells" property. Add ports hierarchy, as an alternative to port. Drop pinctrl properties, as they do not belong here. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- RFC as I do not know: 1. The correct value(s) for '#sound-dai-cells' (the example used 2, which was IMHO wrong, while all actual users use 0), 2. The meaning of the various ports subnodes. --- .../bindings/display/bridge/nxp,tda998x.yaml | 108 ++++++++++++++++++ .../bindings/display/bridge/tda998x.txt | 54 --------- 2 files changed, 108 insertions(+), 54 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/nxp,tda998x.yaml delete mode 100644 Documentation/devicetree/bindings/display/bridge/tda998x.txt