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@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. 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@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. 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. -- With Best Regards, Dharma B. > > Best regards, > Krzysztof >
