| Message ID | 20240110102535.246177-2-dharma.b@microchip.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Convert Microchip's HLCDC Text based DT bindings to JSON schema | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 133 lines checked |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
On 10/01/2024 11:25, Dharma Balasubiramani wrote: > Convert the existing DT binding to DT schema of the Atmel's HLCDC display > controller. > > Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com> > --- > .../display/atmel/atmel,hlcdc-dc.yaml | 133 ++++++++++++++++++ > .../bindings/display/atmel/hlcdc-dc.txt | 75 ---------- > 2 files changed, 133 insertions(+), 75 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml > delete mode 100644 Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt > > diff --git a/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml > new file mode 100644 > index 000000000000..49ef28646c48 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml > @@ -0,0 +1,133 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright (C) 2024 Microchip Technology, Inc. and its subsidiaries What about original copyrights from TXT file? Although conversion is quite independent, I could imagine some lawyer would call it a derivative work of original TXT. If you decide to add explicit copyrights (which anyway I do not understand why), then please make it signed off by some of your lawyers to be sure that you really claim that, in respect of other people copyrights. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/atmel/atmel,hlcdc-dc.yaml# Filename like compatible. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Atmel's HLCDC (High LCD Controller) DRM driver Driver as Linux driver? Not suitable for bindings, so please drop. > + > +maintainers: > + - Nicolas Ferre <nicolas.ferre@microchip.com> > + - Alexandre Belloni <alexandre.belloni@bootlin.com> > + - Claudiu Beznea <claudiu.beznea@tuxon.dev> > + > +description: | > + Device-Tree bindings for Atmel's HLCDC DRM driver. The Atmel HLCDC Display Drop entire first sentence and instead describe hardware. > + Controller is a subdevice of the HLCDC MFD device. > + # See ../../mfd/atmel,hlcdc.yaml for more details. Full paths please. > + > +properties: > + compatible: > + const: atmel,hlcdc-display-controller > + > + pinctrl-names: > + const: default > + > + pinctrl-0: true Why do you need these two? Are they really required? > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + port@0: > + $ref: /schemas/graph.yaml#/$defs/port-base > + unevaluatedProperties: false > + description: > + Output endpoint of the controller, connecting the LCD panel signals. > + > + properties: > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + reg: > + maxItems: 1 > + > + endpoint: > + $ref: /schemas/graph.yaml#/$defs/endpoint-base Hm, why do you reference endpoint-base? This looks oddly different than all other bindings for such devices, so please explain why. > + unevaluatedProperties: false > + description: > + Endpoint connecting the LCD panel signals. > + > + properties: > + bus-width: > + description: | > + Any endpoint grandchild node may specify a desired video interface according to > + ../../media/video-interfaces.yaml, specifically "bus-width" whose recognized Drop redundant information. Don't you miss some $ref? > + values are <12>, <16>, <18> and <24>, and override any output mode selection > + heuristic, forcing "rgb444","rgb565", "rgb666" and "rgb888" respectively. > + enum: [ 12, 16, 18, 24 ] > + > +additionalProperties: false This goes after required: > + > +required: > + - '#address-cells' > + - '#size-cells' > + - compatible > + - pinctrl-names > + - pinctrl-0 > + - port@0 > + > +examples: > + - | > + #include <dt-bindings/clock/at91.h> > + #include <dt-bindings/dma/at91.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + //Example 1 Drop > + hlcdc: hlcdc@f0030000 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > + compatible = "atmel,sama5d3-hlcdc"; > + reg = <0xf0030000 0x2000>; > + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; > + clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; > + clock-names = "periph_clk","sys_clk", "slow_clk"; This part does not look related... If this is part of other device, usually it is enough to have just one complete example. Also, fix coding style - space after , > + > + hlcdc-display-controller { > + compatible = "atmel,hlcdc-display-controller"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + port@0 { > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0>; > + > + hlcdc_panel_output: endpoint@0 { > + reg = <0>; > + remote-endpoint = <&panel_input>; > + }; > + }; > + }; > + > + hlcdc_pwm: hlcdc-pwm { > + compatible = "atmel,hlcdc-pwm"; How is this related? > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_lcd_pwm>; > + #pwm-cells = <3>; > + }; > + }; > + - | > + //Example 2 With a video interface override to force rgb565 > + hlcdc-display-controller { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>; And how is this? Where is the compatible? Maybe just drop second example, what are the differences? Are you sure your Microchip folks reviewed it before? Best regards, Krzysztof
On Wed, 10 Jan 2024 15:55:33 +0530, Dharma Balasubiramani wrote: > Convert the existing DT binding to DT schema of the Atmel's HLCDC display > controller. > > Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com> > --- > .../display/atmel/atmel,hlcdc-dc.yaml | 133 ++++++++++++++++++ > .../bindings/display/atmel/hlcdc-dc.txt | 75 ---------- > 2 files changed, 133 insertions(+), 75 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml > delete mode 100644 Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt > 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: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.example.dtb: /example-0/hlcdc@f0030000: failed to match any schema with compatible: ['atmel,sama5d3-hlcdc'] Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.example.dtb: /example-0/hlcdc@f0030000/hlcdc-pwm: failed to match any schema with compatible: ['atmel,hlcdc-pwm'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240110102535.246177-2-dharma.b@microchip.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. 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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi Krzysztof, On 10/01/24 4:09 pm, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 10/01/2024 11:25, Dharma Balasubiramani wrote: >> Convert the existing DT binding to DT schema of the Atmel's HLCDC display >> controller. >> >> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com> >> --- >> .../display/atmel/atmel,hlcdc-dc.yaml | 133 ++++++++++++++++++ >> .../bindings/display/atmel/hlcdc-dc.txt | 75 ---------- >> 2 files changed, 133 insertions(+), 75 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml >> delete mode 100644 Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt >> >> diff --git a/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml >> new file mode 100644 >> index 000000000000..49ef28646c48 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml >> @@ -0,0 +1,133 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright (C) 2024 Microchip Technology, Inc. and its subsidiaries > > What about original copyrights from TXT file? Although conversion is > quite independent, I could imagine some lawyer would call it a > derivative work of original TXT. > > If you decide to add explicit copyrights (which anyway I do not > understand why), then please make it signed off by some of your lawyers > to be sure that you really claim that, in respect of other people > copyrights. I omitted it in v2, but given the approval from Conor and Alexandre to include the copyrights, I will reintroduce it in v3. > >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/atmel/atmel,hlcdc-dc.yaml# > > Filename like compatible I modified it in v2. > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Atmel's HLCDC (High LCD Controller) DRM driver > > Driver as Linux driver? Not suitable for bindings, so please drop. Done. > >> + >> +maintainers: >> + - Nicolas Ferre <nicolas.ferre@microchip.com> >> + - Alexandre Belloni <alexandre.belloni@bootlin.com> >> + - Claudiu Beznea <claudiu.beznea@tuxon.dev> >> + >> +description: | >> + Device-Tree bindings for Atmel's HLCDC DRM driver. The Atmel HLCDC Display > > Drop entire first sentence and instead describe hardware. Done. > >> + Controller is a subdevice of the HLCDC MFD device. >> + # See ../../mfd/atmel,hlcdc.yaml for more details. > > Full paths please. Considering that the MFD's YAML comes after this patch, I have decided to remove it here. > >> + >> +properties: >> + compatible: >> + const: atmel,hlcdc-display-controller >> + >> + pinctrl-names: >> + const: default >> + >> + pinctrl-0: true > > Why do you need these two? Are they really required? No, I removed it in v2. > >> + >> + '#address-cells': >> + const: 1 >> + >> + '#size-cells': >> + const: 0 >> + >> + port@0: >> + $ref: /schemas/graph.yaml#/$defs/port-base >> + unevaluatedProperties: false >> + description: >> + Output endpoint of the controller, connecting the LCD panel signals. >> + >> + properties: >> + '#address-cells': >> + const: 1 >> + >> + '#size-cells': >> + const: 0 >> + >> + reg: >> + maxItems: 1 >> + >> + endpoint: >> + $ref: /schemas/graph.yaml#/$defs/endpoint-base > > Hm, why do you reference endpoint-base? This looks oddly different than > all other bindings for such devices, so please explain why. My apologies, it should be '$ref: /schemas/media/video-interfaces.yaml#' I will correct this in v3. > >> + unevaluatedProperties: false >> + description: >> + Endpoint connecting the LCD panel signals. >> + >> + properties: >> + bus-width: >> + description: | >> + Any endpoint grandchild node may specify a desired video interface according to >> + ../../media/video-interfaces.yaml, specifically "bus-width" whose recognized > > Drop redundant information. Don't you miss some $ref? Sure, I will drop the description in v3. > > >> + values are <12>, <16>, <18> and <24>, and override any output mode selection >> + heuristic, forcing "rgb444","rgb565", "rgb666" and "rgb888" respectively. >> + enum: [ 12, 16, 18, 24 ] >> + >> +additionalProperties: false > > This goes after required: Done. > >> + >> +required: >> + - '#address-cells' >> + - '#size-cells' >> + - compatible >> + - pinctrl-names >> + - pinctrl-0 >> + - port@0 >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/at91.h> >> + #include <dt-bindings/dma/at91.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + //Example 1 > > Drop Sure. > >> + hlcdc: hlcdc@f0030000 { > > Node names should be generic. See also an explanation and list of > examples (not exhaustive) in DT specification: > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation > > >> + compatible = "atmel,sama5d3-hlcdc"; >> + reg = <0xf0030000 0x2000>; >> + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; >> + clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; >> + clock-names = "periph_clk","sys_clk", "slow_clk"; > > This part does not look related... If this is part of other device, > usually it is enough to have just one complete example. Sure, I will have one complete example in mfd binding and drop the other example here. > > Also, fix coding style - space after , Sure. > >> + >> + hlcdc-display-controller { >> + compatible = "atmel,hlcdc-display-controller"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + port@0 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + reg = <0>; >> + >> + hlcdc_panel_output: endpoint@0 { >> + reg = <0>; >> + remote-endpoint = <&panel_input>; >> + }; >> + }; >> + }; >> + >> + hlcdc_pwm: hlcdc-pwm { >> + compatible = "atmel,hlcdc-pwm"; > > How is this related? Not related here, thanks, removed it in v2. > >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_lcd_pwm>; >> + #pwm-cells = <3>; >> + }; >> + }; >> + - | >> + //Example 2 With a video interface override to force rgb565 >> + hlcdc-display-controller { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>; > > And how is this? Where is the compatible? Maybe just drop second > example, what are the differences? bus-width is the only property that is different between the examples, I will drop the example 2 in v3. > > Are you sure your Microchip folks reviewed it before? I acknowledge my oversight; I should have sought their review beforehand. I appreciate your understanding.
Hi Rob, On 10/01/24 5:27 pm, Rob Herring wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 10 Jan 2024 15:55:33 +0530, Dharma Balasubiramani wrote: >> Convert the existing DT binding to DT schema of the Atmel's HLCDC display >> controller. >> >> Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com> >> --- >> .../display/atmel/atmel,hlcdc-dc.yaml | 133 ++++++++++++++++++ >> .../bindings/display/atmel/hlcdc-dc.txt | 75 ---------- >> 2 files changed, 133 insertions(+), 75 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml >> delete mode 100644 Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt >> > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > I conducted the tests after applying all three of them, so I didn't notice these errors. I have addressed this in v2. Thank you.
diff --git a/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml new file mode 100644 index 000000000000..49ef28646c48 --- /dev/null +++ b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml @@ -0,0 +1,133 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (C) 2024 Microchip Technology, Inc. and its subsidiaries +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/atmel/atmel,hlcdc-dc.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Atmel's HLCDC (High LCD Controller) DRM driver + +maintainers: + - Nicolas Ferre <nicolas.ferre@microchip.com> + - Alexandre Belloni <alexandre.belloni@bootlin.com> + - Claudiu Beznea <claudiu.beznea@tuxon.dev> + +description: | + Device-Tree bindings for Atmel's HLCDC DRM driver. The Atmel HLCDC Display + Controller is a subdevice of the HLCDC MFD device. + # See ../../mfd/atmel,hlcdc.yaml for more details. + +properties: + compatible: + const: atmel,hlcdc-display-controller + + pinctrl-names: + const: default + + pinctrl-0: true + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + port@0: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: + Output endpoint of the controller, connecting the LCD panel signals. + + properties: + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + reg: + maxItems: 1 + + endpoint: + $ref: /schemas/graph.yaml#/$defs/endpoint-base + unevaluatedProperties: false + description: + Endpoint connecting the LCD panel signals. + + properties: + bus-width: + description: | + Any endpoint grandchild node may specify a desired video interface according to + ../../media/video-interfaces.yaml, specifically "bus-width" whose recognized + values are <12>, <16>, <18> and <24>, and override any output mode selection + heuristic, forcing "rgb444","rgb565", "rgb666" and "rgb888" respectively. + enum: [ 12, 16, 18, 24 ] + +additionalProperties: false + +required: + - '#address-cells' + - '#size-cells' + - compatible + - pinctrl-names + - pinctrl-0 + - port@0 + +examples: + - | + #include <dt-bindings/clock/at91.h> + #include <dt-bindings/dma/at91.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + //Example 1 + hlcdc: hlcdc@f0030000 { + compatible = "atmel,sama5d3-hlcdc"; + reg = <0xf0030000 0x2000>; + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; + clock-names = "periph_clk","sys_clk", "slow_clk"; + + hlcdc-display-controller { + compatible = "atmel,hlcdc-display-controller"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>; + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + + hlcdc_panel_output: endpoint@0 { + reg = <0>; + remote-endpoint = <&panel_input>; + }; + }; + }; + + hlcdc_pwm: hlcdc-pwm { + compatible = "atmel,hlcdc-pwm"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_lcd_pwm>; + #pwm-cells = <3>; + }; + }; + - | + //Example 2 With a video interface override to force rgb565 + hlcdc-display-controller { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>; + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + #address-cells = <1>; + #size-cells = <0>; + reg = <0>; + hlcdc_panel_output2: endpoint@0 { + reg = <0>; + remote-endpoint = <&panel_input>; + bus-width = <16>; + }; + }; + }; diff --git a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt b/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt deleted file mode 100644 index 923aea25344c..000000000000 --- a/Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt +++ /dev/null @@ -1,75 +0,0 @@ -Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver - -The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. -See ../../mfd/atmel-hlcdc.txt for more details. - -Required properties: - - compatible: value should be "atmel,hlcdc-display-controller" - - pinctrl-names: the pin control state names. Should contain "default". - - pinctrl-0: should contain the default pinctrl states. - - #address-cells: should be set to 1. - - #size-cells: should be set to 0. - -Required children nodes: - Children nodes are encoding available output ports and their connections - to external devices using the OF graph representation (see ../graph.txt). - At least one port node is required. - -Optional properties in grandchild nodes: - Any endpoint grandchild node may specify a desired video interface - according to ../../media/video-interfaces.txt, specifically - - bus-width: recognized values are <12>, <16>, <18> and <24>, and - override any output mode selection heuristic, forcing "rgb444", - "rgb565", "rgb666" and "rgb888" respectively. - -Example: - - hlcdc: hlcdc@f0030000 { - compatible = "atmel,sama5d3-hlcdc"; - reg = <0xf0030000 0x2000>; - interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; - clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; - clock-names = "periph_clk","sys_clk", "slow_clk"; - - hlcdc-display-controller { - compatible = "atmel,hlcdc-display-controller"; - pinctrl-names = "default"; - pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>; - #address-cells = <1>; - #size-cells = <0>; - - port@0 { - #address-cells = <1>; - #size-cells = <0>; - reg = <0>; - - hlcdc_panel_output: endpoint@0 { - reg = <0>; - remote-endpoint = <&panel_input>; - }; - }; - }; - - hlcdc_pwm: hlcdc-pwm { - compatible = "atmel,hlcdc-pwm"; - pinctrl-names = "default"; - pinctrl-0 = <&pinctrl_lcd_pwm>; - #pwm-cells = <3>; - }; - }; - -Example 2: With a video interface override to force rgb565; as above -but with these changes/additions: - - &hlcdc { - hlcdc-display-controller { - pinctrl-names = "default"; - pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>; - - port@0 { - hlcdc_panel_output: endpoint@0 { - bus-width = <16>; - }; - }; - }; - };
Convert the existing DT binding to DT schema of the Atmel's HLCDC display controller. Signed-off-by: Dharma Balasubiramani <dharma.b@microchip.com> --- .../display/atmel/atmel,hlcdc-dc.yaml | 133 ++++++++++++++++++ .../bindings/display/atmel/hlcdc-dc.txt | 75 ---------- 2 files changed, 133 insertions(+), 75 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml delete mode 100644 Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt