Re: [PATCH v3 1/3] dt-bindings: timer: atmel,at91sam9260-pit: convert to yaml

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 09/06/2023 12:22, Claudiu.Beznea@microchip.com wrote:
> On 31.05.2023 11:55, Krzysztof Kozlowski wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 30/05/2023 11:07, Claudiu Beznea wrote:
>>> Convert Microchip AT91 PIT bindings to YAML. Along with it clocks and
>>> clock-names bindings were added as the drivers needs it to ensure proper
>>> hardware functionality.
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>> ---
>>>  .../devicetree/bindings/arm/atmel-sysregs.txt | 12 ---
>>>  .../bindings/timer/atmel,at91sam9260-pit.yaml | 99 +++++++++++++++++++
>>>  2 files changed, 99 insertions(+), 12 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/timer/atmel,at91sam9260-pit.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/atmel-sysregs.txt b/Documentation/devicetree/bindings/arm/atmel-sysregs.txt
>>> index 67a66bf74895..54d3f586403e 100644
>>> --- a/Documentation/devicetree/bindings/arm/atmel-sysregs.txt
>>> +++ b/Documentation/devicetree/bindings/arm/atmel-sysregs.txt
>>> @@ -4,18 +4,6 @@ Chipid required properties:
>>>  - compatible: Should be "atmel,sama5d2-chipid" or "microchip,sama7g5-chipid"
>>>  - reg : Should contain registers location and length
>>>
>>> -PIT Timer required properties:
>>> -- compatible: Should be "atmel,at91sam9260-pit"
>>> -- reg: Should contain registers location and length
>>> -- interrupts: Should contain interrupt for the PIT which is the IRQ line
>>> -  shared across all System Controller members.
>>> -
>>> -PIT64B Timer required properties:
>>> -- compatible: Should be "microchip,sam9x60-pit64b"
>>> -- reg: Should contain registers location and length
>>> -- interrupts: Should contain interrupt for PIT64B timer
>>> -- clocks: Should contain the available clock sources for PIT64B timer.
>>> -
>>>  System Timer (ST) required properties:
>>>  - compatible: Should be "atmel,at91rm9200-st", "syscon", "simple-mfd"
>>>  - reg: Should contain registers location and length
>>> diff --git a/Documentation/devicetree/bindings/timer/atmel,at91sam9260-pit.yaml b/Documentation/devicetree/bindings/timer/atmel,at91sam9260-pit.yaml
>>> new file mode 100644
>>> index 000000000000..d0f3f80db4cb
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/timer/atmel,at91sam9260-pit.yaml
>>> @@ -0,0 +1,99 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/timer/atmel,at91sam9260-pit.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Microchip AT91 Periodic Interval Timer (PIT)
>>> +
>>> +maintainers:
>>> +  - Claudiu Beznea <claudiu.beznea@microchip.com>
>>> +
>>> +description:
>>> +  Microchip AT91 periodic interval timer provides the operating system scheduler
>>> +  interrupt. It is designed to offer maximum accuracy and efficient management,
>>> +  even for systems with long response time.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>> +          - const: microchip,sama7g5-pit64b
>>
>> >From where do you have this compatible? Wasn't in old binding and commit
>> msg does not explain it.
> 
> ok, I'll update it in the commit message. It is from the available device
> trees.

Add them in next patch, so the conversion is only conversion.

> 
>>
>>
>>> +          - const: microchip,sam9x60-pit64b
>>> +      - items:
>>> +          enum:
>>
>> These are not items. Just enum.. Does it even work?
> 
> Yes, it compiles w/o issues. I'll update it anyway.

Yeah, but does it work as intended? This should allow any order of below
compatibles - from 1 to 2, so totally not what you wanted.

> 
>>
>>> +            - atmel,at91sam9260-pit
>>> +            - microchip,sam9x60-pit64b

>>> +      properties:
>>> +        clocks:
>>> +          minItems: 2
>>> +        clock-names:
>>> +          items:
>>> +            - const: pclk
>>> +            - const: gclk
>>
>> interrupts? They are still required, so why no description here?
> 
> It was here in the previous versions but Conor suggested to remove it as it
> was nothing specific about this description. For the if-then branch I kept
> it to specify that the interrupt is share with other devices. In this
> branch the interrupt is only for the timer itself. With this, would you
> still prefer to add it back?

I just don't understand why interrupts are in one arm of the if: and not
in the other.

> 
>>
>>> +      required:
>>> +        - clock-names
>>> +
>>> +unevaluatedProperties: false
>>
>> additionalProperties:false instead
> 
> Having additionalProperties:false instead of unevaluatedProperties: false
> thows the following error on make dt_binding_check and make dtbs_check:
> 
> Documentation/devicetree/bindings/timer/atmel,at91sam9260-pit.example.dtb:
> timer@f0028000: 'clock-names' does not match any of the regexes:
> 'pinctrl-[0-9]+'

So it nicely points to something you need to fix in the binding.



Best regards,
Krzysztof