Hi Krzysztof, On 10/01/24 11:31 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 atmel,hlcdc binding to DT schema format. >> >> Signed-off-by: Dharma Balasubiramani <dharma.b@xxxxxxxxxxxxx> >> --- >> .../devicetree/bindings/mfd/atmel,hlcdc.yaml | 106 ++++++++++++++++++ >> .../devicetree/bindings/mfd/atmel-hlcdc.txt | 56 --------- >> 2 files changed, 106 insertions(+), 56 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml >> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt >> >> diff --git a/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml b/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml >> new file mode 100644 >> index 000000000000..555d6faa9104 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml > > This looks not tested, so limited review follows: I acknowledge that I didn't test the patches individually. I appreciate your understanding. Taken care in v2. > >> @@ -0,0 +1,106 @@ >> +# 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/mfd/atmel,hlcdc.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Atmel's HLCDC (High LCD Controller) MFD driver > > Drop "MFD driver" and rather describe/name the hardware. MFD is Linux > term, so I really doubt that's how this was called. 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 (High LCD Controller) MFD driver. > > Drop Done. > >> + The HLCDC IP exposes two subdevices: >> + # a PWM chip: see ../pwm/atmel,hlcdc-pwm.yaml >> + # a Display Controller: see ../display/atmel/atmel,hlcdc-dc.yaml > > Rephrase to describe hardware. Drop redundant paths. Sure, I will truncate this to "subdevices: a PWM chip and a display controller." & drop the |. I added description about those two subdevices below. > >> + >> +properties: >> + compatible: >> + enum: >> + - atmel,at91sam9n12-hlcdc >> + - atmel,at91sam9x5-hlcdc >> + - atmel,sama5d2-hlcdc >> + - atmel,sama5d3-hlcdc >> + - atmel,sama5d4-hlcdc >> + - microchip,sam9x60-hlcdc >> + - microchip,sam9x75-xlcdc >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 3 >> + >> + clock-names: >> + anyOf: >> + - items: >> + - enum: >> + - sys_clk >> + - lvds_pll_clk > > Old binding was not mentioning this and you did not describe differences > against pure conversion. You have entire commit msg for this... Done, added commit message in v2. > >> + - contains: >> + const: periph_clk >> + - contains: >> + const: slow_clk >> + maxItems: 3 > > Why it has to be so complicated? I doubt that same devices have > different inputs. I will modify it to clock-names: items: - const: periph_clk - enum: [sys_clk, lvds_pll_clk] - const: slow_clk in v3. > >> + >> + hlcdc-display-controller: > > Does anything depend on the name? If not, then just display-controller Got an error "'hlcdc-display-controller' does not match any of the regexes: 'pinctrl-[0-9]+'" so I retained it in v2,but as conor advised to have node names generic and we can easily adopt, I will modify it in v3. > >> + $ref: /schemas/display/atmel/atmel,hlcdc-dc.yaml >> + >> + hlcdc-pwm: > > Same comment. Sure, I will modify it in v3. > >> + $ref: /schemas/pwm/atmel,hlcdc-pwm.yaml > > There is no such file. This occurred because I added it before pwm patch. In v2, I introduced 'display' and 'pwm' before 'mfd' so that I could reference them here. > >> + >> +required: >> + - compatible >> + - reg >> + - clocks >> + - clock-names >> + - interrupts >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/clock/at91.h> >> + #include <dt-bindings/dma/at91.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> + 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 Done. > > >> + compatible = "atmel,sama5d3-hlcdc"; >> + reg = <0xf0030000 0x2000>; >> + clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; >> + clock-names = "periph_clk","sys_clk", "slow_clk"; > > Missing space after , Sure, fixed in v2. -- With Best Regards, Dharma B. > >> + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; >> + >> + 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>; >> + }; >> + }; >> + }; >> + > > > Best regards, > Krzysztof >
