Re: [PATCH 2/5] dt-bindings: clocks: atmel,at91rm9200-pmc: convert to yaml

[Krzysztof Kozlowski]

On 05/05/2023 14:46, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
> 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?

I mean the enum at the bottom of all compatibles.

>>> +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

ok

Best regards,
Krzysztof