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@xxxxxxxxxxxxx> > --- > .../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@xxxxxxxxxxxxx> > + - Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> > + - Claudiu Beznea <claudiu.beznea@xxxxxxxxx> > + > +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
