On 04.05.2023 15:47, Krzysztof Kozlowski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 04/05/2023 08:07, Claudiu Beznea wrote: >> Convert Atmel PMC documentation to yaml. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx> > > Thank you for your patch. There is something to discuss/improve. > > >> diff --git a/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml >> new file mode 100644 >> index 000000000000..c4023c3a85f2 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/atmel,at91rm9200-pmc.yaml >> @@ -0,0 +1,161 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/clock/atmel,at91rm9200-pmc.yaml#"; >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"; > > Drop quotes from both. > >> + >> +title: Atmel Power Management Controller (PMC) >> + >> +maintainers: >> + - Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx> >> + >> +description: >> + The power management controller optimizes power consumption by controlling all >> + system and user peripheral clocks. The PMC enables/disables the clock inputs >> + to many of the peripherals and to the processor. >> + >> +properties: >> + compatible: >> + oneOf: >> + - items: >> + - const: atmel,at91sam9260-pmc > > Why this is separate, not part of bottom enum? Current device trees uses the following compatibles (among others): - "atmel,at91sam9260-pmc, "syscon" or - "atmel,at91sam9g20-pmc", "atmel,at91sam9260-pmc, "syscon" I haven't found another way to make dtbs_check happy. Is there another way for this? > >> + - const: syscon > >> + - items: >> + - enum: >> + - atmel,at91sam9g20-pmc >> + - enum: >> + - atmel,at91sam9260-pmc > > This should be const. You cannot have here different compatibles. > >> + - const: syscon >> + - items: >> + - enum: >> + - atmel,at91sam9g15-pmc >> + - atmel,at91sam9g25-pmc >> + - atmel,at91sam9g35-pmc >> + - atmel,at91sam9x25-pmc >> + - atmel,at91sam9x35-pmc >> + - enum: >> + - atmel,at91sam9x5-pmc >> + - const: syscon >> + - items: >> + - enum: >> + - atmel,at91sam9g45-pmc >> + - atmel,at91sam9n12-pmc >> + - atmel,at91sam9rl-pmc >> + - atmel,at91rm9200-pmc > > Order by name? > >> + - atmel,sama5d4-pmc >> + - atmel,sama5d3-pmc >> + - atmel,sama5d2-pmc >> + - microchip,sam9x60-pmc >> + - microchip,sama7g5-pmc >> + - const: syscon >> + >> + reg: >> + maxItems: 1 >> + >> + "#clock-cells": >> + const: 2 > > Explain what the cells are for in description. Having '2' for clock > controller is not obvious. > >> + >> + clocks: >> + minItems: 2 >> + maxItems: 3 >> + >> + clock-names: >> + minItems: 2 >> + maxItems: 3 >> + >> + interrupts: >> + maxItems: 1 >> + >> + atmel,osc-bypass: >> + type: boolean >> + description: set when a clock signal is directly provided on XIN >> + >> + > > Just one blank line. > >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - "#clock-cells" >> + - clocks >> + - clock-names > > Keep the same order here as they appear in properties:. > > >> + >> +allOf: >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - microchip,sam9x60-pmc >> + - microchip,sama7g5-pmc >> + then: >> + properties: >> + clocks: >> + minItems: 3 >> + maxItems: 3 >> + clock-names: >> + items: >> + - const: td_slck >> + - const: md_slck >> + - const: main_xtal >> + required: >> + - clock-names >> + - clocks > > Drop required: here. It's already in top-level. Same in places below. > >> + >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - atmel,at91rm9200-pmc >> + - atmel,at91sam9260-pmc >> + - atmel,at91sam9g20-pmc >> + then: >> + properties: >> + clocks: >> + minItems: 2 >> + maxItems: 2 >> + clock-names: >> + items: >> + - const: slow_xtal >> + - const: main_xtal >> + required: >> + - clock-names >> + - clocks >> + >> + - if: >> + properties: >> + compatible: >> + contains: >> + enum: >> + - atmel,sama5d4-pmc >> + - atmel,sama5d3-pmc >> + - atmel,sama5d2-pmc >> + then: >> + properties: >> + clocks: >> + minItems: 2 >> + maxItems: 2 >> + clock-names: >> + items: >> + - const: slow_clk >> + - const: main_xtal >> + required: >> + - clock-names >> + - clocks >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + >> + pmc: clock-controller@f0018000 { >> + compatible = "atmel,sama5d4-pmc", "syscon"; >> + reg = <0xf0018000 0x120>; >> + interrupts = <1 IRQ_TYPE_LEVEL_HIGH 7>; > > interrupt looks a bit odd. Are you sure it is correct? This example is from SAMA5D4 SoC which uses a vendor specific interrupt controller (Atmel AIC) where: - 1st cell is the interrupt number - 2nd cell is the interrupt type (level/edge sensitive) - 3rd cell is the IRQ priority Thank you, Claudiu > >> + #clock-cells = <2>; >> + clocks = <&clk32k>, <&main_xtal>; >> + clock-names = "slow_clk", "main_xtal"; >> + }; >> + >> +... > > Best regards, > Krzysztof >
