| Message ID | 6c8b72a03703de54fa02b29c1a53c84ca0889e50.1632761067.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: JZ4780 and CI20 HDMI | expand |
Hi, On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote: > From: Sam Ravnborg <sam@ravnborg.org> > > Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. > Based on .txt binding from Zubair Lutfullah Kakakhel > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > --- > .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml > > diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml > new file mode 100644 > index 000000000000..5e60cdac4f63 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml > @@ -0,0 +1,85 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Bindings for Ingenic JZ4780 HDMI Transmitter > + > +maintainers: > + - H. Nikolaus Schaller <hns@goldelico.com> > + > +description: | > + The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4 > + TX controller IP with accompanying PHY IP. > + > +allOf: > + - $ref: panel/panel-common.yaml# Is it a panel though? > +properties: > + compatible: > + items: > + - const: ingenic,jz4780-dw-hdmi This can just be a const, there's no need for the items > + > + reg: > + maxItems: 1 > + description: the address & size of the LCD controller registers There's no need for that description, it's obvious enough > + reg-io-width: > + const: 4 If it's fixed, why do you need it in the first place? > + interrupts: > + maxItems: 1 > + description: Specifies the interrupt provided by parent There's no need for that description, it's obvious enough > + clocks: > + maxItems: 2 > + description: Clock specifiers for isrf and iahb clocks This can be defined as clocks: items: - description: isrf - description: iahb A better description about what these clocks are would be nice as well > + clock-names: > + items: > + - const: isfr Is it isfr or isrf? > + - const: iahb > + > + hdmi-regulator: true > + description: Optional regulator to provide +5V at the connector regulators need to be suffixed by -supply You also can just provide the description, you don't need the true there > + ddc-i2c-bus: true ditto > + description: An I2C interface if the internal DDC I2C driver is not to be used > + ports: true If there's a single port, you don't need ports You should also include /schemas/graph.yaml#/$defs/port-base Maxime
On Mon, 27 Sep 2021 18:44:21 +0200, H. Nikolaus Schaller wrote: > From: Sam Ravnborg <sam@ravnborg.org> > > Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. > Based on .txt binding from Zubair Lutfullah Kakakhel > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > Cc: Rob Herring <robh@kernel.org> > Cc: devicetree@vger.kernel.org > --- > .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++ > 1 file changed, 85 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml:45:16: [error] syntax error: mapping values are not allowed here (syntax) dtschema/dtc warnings/errors: make[1]: *** Deleting file 'Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dts' Traceback (most recent call last): File "/usr/local/bin/dt-extract-example", line 45, in <module> binding = yaml.load(open(args.yamlfile, encoding='utf-8').read()) File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load return constructor.get_single_data() File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 120, in get_single_data node = self.composer.get_single_node() File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event ruamel.yaml.scanner.ScannerError: mapping values are not allowed in this context in "<unicode string>", line 45, column 16 make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... ./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml: mapping values are not allowed in this context in "<unicode string>", line 45, column 16 /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml: ignoring, error parsing file warning: no schema found in file: ./Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml make: *** [Makefile:1441: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1533471 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hi, > Am 27.09.2021 um 19:07 schrieb maxime@cerno.tech: > > Hi, > > On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote: >> From: Sam Ravnborg <sam@ravnborg.org> >> >> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. >> Based on .txt binding from Zubair Lutfullah Kakakhel >> >> Signed-off-by: Sam Ravnborg <sam@ravnborg.org> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> Cc: Rob Herring <robh@kernel.org> >> Cc: devicetree@vger.kernel.org >> --- >> .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++++++++++++++++++ >> 1 file changed, 85 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> >> diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> new file mode 100644 >> index 000000000000..5e60cdac4f63 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> @@ -0,0 +1,85 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Bindings for Ingenic JZ4780 HDMI Transmitter >> + >> +maintainers: >> + - H. Nikolaus Schaller <hns@goldelico.com> >> + >> +description: | >> + The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4 >> + TX controller IP with accompanying PHY IP. >> + >> +allOf: >> + - $ref: panel/panel-common.yaml# > > Is it a panel though? Good question. Appears to have to be changed to - $ref: bridge/synopsys,dw-hdmi.yaml# > >> +properties: >> + compatible: >> + items: >> + - const: ingenic,jz4780-dw-hdmi > > This can just be a const, there's no need for the items Maybe starting with an enum is better if more compatible strings are to be added. > >> + >> + reg: >> + maxItems: 1 >> + description: the address & size of the LCD controller registers > > There's no need for that description, it's obvious enough Indeed. > >> + reg-io-width: >> + const: 4 > > If it's fixed, why do you need it in the first place? There is a fixed default of 1 if not specified. > >> + interrupts: >> + maxItems: 1 >> + description: Specifies the interrupt provided by parent > > There's no need for that description, it's obvious enough Indeed. > >> + clocks: >> + maxItems: 2 >> + description: Clock specifiers for isrf and iahb clocks > > This can be defined as > > clocks: > items: > - description: isrf > - description: iahb > > A better description about what these clocks are would be nice as well Generally I see that this all is nowadays not independent of Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml where there is already a description. On the other hand every SoC specialization runs its own copy. e.g. Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam > >> + clock-names: >> + items: >> + - const: isfr > > Is it isfr or isrf? isfr. Seems to be a typo in the description. See bridge/synopsys,dw-hdmi.yaml# One question to the yaml specialists: since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we have to repeat? Or can we reduce to just the changes? [I am still not familiar enough with the yaml stuff to understand if it has sort of inheritance like device tree include files, so that you just have to change relevant properties] > >> + - const: iahb would it make sense to add additionalItems: false here? In the jz4780 case there are just two clocks while other specializations use more and synopsys,dw-hdmi.yaml# defines additionalItems: true. >> + >> + hdmi-regulator: true >> + description: Optional regulator to provide +5V at the connector > > regulators need to be suffixed by -supply My omission... And, it should be "hdmi-5v-supply" to match driver and device tree. > > You also can just provide the description, you don't need the true there > >> + ddc-i2c-bus: true > > ditto Ok > >> + description: An I2C interface if the internal DDC I2C driver is not to be used >> + ports: true > > If there's a single port, you don't need ports There can be two ports - one for input from LCDC and one for output (HDMI connector). But explicitly defining an output port is optional to some extent (depending on driver structure). > > You should also include /schemas/graph.yaml#/$defs/port-base Ok. BR and thanks, Nikolaus
On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote: > >> +properties: > >> + compatible: > >> + items: > >> + - const: ingenic,jz4780-dw-hdmi > > > > This can just be a const, there's no need for the items > > Maybe starting with an enum is better if more compatible strings are to be added. it's still fairly easy to change if needed, there's no need to confuse anyone. > > > >> + reg-io-width: > >> + const: 4 > > > > If it's fixed, why do you need it in the first place? > > There is a fixed default of 1 if not specified. My point was more about why do you need to have that property at all? Can't you just drop it and assume that the register width is 32 bits if it's all you will ever run on? > >> + clocks: > >> + maxItems: 2 > >> + description: Clock specifiers for isrf and iahb clocks > > > > This can be defined as > > > > clocks: > > items: > > - description: isrf > > - description: iahb > > > > A better description about what these clocks are would be nice as well > > Generally I see that this all is nowadays not independent of > > Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > > where there is already a description. Ok, good then > On the other hand every SoC specialization runs its own copy. e.g. > > Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml > Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam > > > > >> + clock-names: > >> + items: > >> + - const: isfr > > > > Is it isfr or isrf? > > isfr. Seems to be a typo in the description. See > bridge/synopsys,dw-hdmi.yaml# > > One question to the yaml specialists: > > since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we > have to repeat? Or can we reduce to just the changes? If you add the ref you mentionned above, you don't have to repeat yourself indeed. You can just put clock-names: true > [I am still not familiar enough with the yaml stuff to understand if > it has sort of inheritance like device tree include files, so that you > just have to change relevant properties] Kind of, but not entirely. schemas are all applied separately, unlike DT includes that will just expand to one big DT. In practice, it means that your device must validate against all the schemas, not just the sum of them. For example, if you have a generic schema that has: properties: compatible: const: vendor,my-generic-compatible and your schema that extends the generic binding, with a ref to the generic one that has: properties: compatible: items: - const: other-vendor,my-device-compatible - const: vendor,my-generic-compatible It will still fail since the generic schema expects only a single compatible, whereas your device would have two. > > > >> + - const: iahb > > would it make sense to add additionalItems: false here? > > In the jz4780 case there are just two clocks while other specializations > use more and synopsys,dw-hdmi.yaml# defines additionalItems: true. If you want to refine the generic one, and it's all the clocks you ever expect then there's no need for additionalItems > > > >> + description: An I2C interface if the internal DDC I2C driver is not to be used > >> + ports: true > > > > If there's a single port, you don't need ports > > There can be two ports - one for input from LCDC and one > for output (HDMI connector). But explicitly defining an output > port is optional to some extent (depending on driver structure). This needs to be defined then (and port@0 made mandatory) Maxime
> Am 28.09.2021 um 11:18 schrieb Maxime Ripard <maxime@cerno.tech>: > > On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote: >>>> +properties: >>>> + compatible: >>>> + items: >>>> + - const: ingenic,jz4780-dw-hdmi >>> >>> This can just be a const, there's no need for the items >> >> Maybe starting with an enum is better if more compatible strings are to be added. > > it's still fairly easy to change if needed, there's no need to confuse > anyone. > >>> >>>> + reg-io-width: >>>> + const: 4 >>> >>> If it's fixed, why do you need it in the first place? >> >> There is a fixed default of 1 if not specified. > > My point was more about why do you need to have that property at all? > Can't you just drop it and assume that the register width is 32 bits if > it's all you will ever run on? No, please see bridge/synopsys,dw-hdmi.yaml where it is derived from: reg-io-width: description: Width (in bytes) of the registers specified by the reg property. allOf: - $ref: /schemas/types.yaml#/definitions/uint32 - enum: [1, 4] default: 1 Other bindings define it explicitly to be 4, e.g. Documentation//devicetree/bindings/display/intel,keembay-msscam.yaml: reg-io-width: Documentation//devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml: reg-io-width: Therefore I'd assume that a regmap is not properly set up if we don't require the DTS to include it with const: 4. > >>>> + clocks: >>>> + maxItems: 2 >>>> + description: Clock specifiers for isrf and iahb clocks >>> >>> This can be defined as >>> >>> clocks: >>> items: >>> - description: isrf >>> - description: iahb >>> >>> A better description about what these clocks are would be nice as well >> >> Generally I see that this all is nowadays not independent of >> >> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml >> >> where there is already a description. > > Ok, good then > >> On the other hand every SoC specialization runs its own copy. e.g. >> >> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml >> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam >> >>> >>>> + clock-names: >>>> + items: >>>> + - const: isfr >>> >>> Is it isfr or isrf? >> >> isfr. Seems to be a typo in the description. See >> bridge/synopsys,dw-hdmi.yaml# >> >> One question to the yaml specialists: >> >> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we >> have to repeat? Or can we reduce to just the changes? > > If you add the ref you mentionned above, you don't have to repeat Nice. It defines: clocks: minItems: 2 maxItems: 5 items: - description: The bus clock for either AHB and APB - description: The internal register configuration clock additionalItems: true > yourself indeed. You can just put clock-names: true Or should we do clocks: maxItems: 2 additionalItems: false > >> [I am still not familiar enough with the yaml stuff to understand if >> it has sort of inheritance like device tree include files, so that you >> just have to change relevant properties] > > Kind of, but not entirely. schemas are all applied separately, unlike DT > includes that will just expand to one big DT. In practice, it means that > your device must validate against all the schemas, not just the sum of > them. > > For example, if you have a generic schema that has: > > properties: > compatible: > const: vendor,my-generic-compatible > > > and your schema that extends the generic binding, with a ref to the > generic one that has: > > properties: > compatible: > items: > - const: other-vendor,my-device-compatible > - const: vendor,my-generic-compatible > > > It will still fail since the generic schema expects only a single > compatible, whereas your device would have two. Ok, I see. it is not a simple "overwrite" rule. > >>> >>>> + - const: iahb >> >> would it make sense to add additionalItems: false here? >> >> In the jz4780 case there are just two clocks while other specializations >> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true. > > If you want to refine the generic one, and it's all the clocks you ever > expect then there's no need for additionalItems Ok. > >>> >>>> + description: An I2C interface if the internal DDC I2C driver is not to be used >>>> + ports: true >>> >>> If there's a single port, you don't need ports >> >> There can be two ports - one for input from LCDC and one >> for output (HDMI connector). But explicitly defining an output >> port is optional to some extent (depending on driver structure). > > This needs to be defined then (and port@0 made mandatory) Ok. I'll try to make the best out of it for v5 series. Maybe it is still not perfect by then, but close... > > Maxime BR and thanks, Nikolaus
diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml new file mode 100644 index 000000000000..5e60cdac4f63 --- /dev/null +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml @@ -0,0 +1,85 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Bindings for Ingenic JZ4780 HDMI Transmitter + +maintainers: + - H. Nikolaus Schaller <hns@goldelico.com> + +description: | + The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4 + TX controller IP with accompanying PHY IP. + +allOf: + - $ref: panel/panel-common.yaml# + +properties: + compatible: + items: + - const: ingenic,jz4780-dw-hdmi + + reg: + maxItems: 1 + description: the address & size of the LCD controller registers + + reg-io-width: + const: 4 + + interrupts: + maxItems: 1 + description: Specifies the interrupt provided by parent + + clocks: + maxItems: 2 + description: Clock specifiers for isrf and iahb clocks + + clock-names: + items: + - const: isfr + - const: iahb + + hdmi-regulator: true + description: Optional regulator to provide +5V at the connector + ddc-i2c-bus: true + description: An I2C interface if the internal DDC I2C driver is not to be used + ports: true + +required: + - compatible + - clocks + - clock-names + - ports + - reg-io-width + +additionalProperties: false + +examples: + - | + #include <dt-bindings/clock/jz4780-cgu.h> + + hdmi: hdmi@10180000 { + compatible = "ingenic,jz4780-dw-hdmi"; + reg = <0x10180000 0x8000>; + reg-io-width = <4>; + ddc-i2c-bus = <&i2c4>; + interrupt-parent = <&intc>; + interrupts = <3>; + clocks = <&cgu JZ4780_CLK_HDMI>, <&cgu JZ4780_CLK_AHB0>; + clock-names = "isfr", "iahb"; + + ports { + hdmi_in: port { + #address-cells = <1>; + #size-cells = <0>; + hdmi_in_lcd: endpoint@0 { + reg = <0>; + remote-endpoint = <&jz4780_out_hdmi>; + }; + }; + }; + }; + +...