Re: [PATCH 2/6] dt-bindings: timer: Add schema for realtek,otto-timer

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/06/2024 06:27, Chris Packham wrote:
> Add the devicetree schema for the realtek,otto-timer present on a number
> of Realtek SoCs.

Please order your patches correctly: bindings always go before users.

A nit, subject: drop second/last, redundant "schema for". The
"dt-bindings" prefix is already stating that these are bindings (so schema).
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  .../bindings/timer/realtek,otto-timer.yaml    | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml
> 
> diff --git a/Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml b/Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml
> new file mode 100644
> index 000000000000..b6e85aadbc99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/realtek,otto-timer.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/realtek,otto-timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek Otto SoCs Timer/Counter
> +
> +description:
> +  Realtek SoCs support a number of timers/counters. These are used
> +  as a per CPU clock event generator and an overall CPU clocksource.
> +
> +maintainers:
> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
> +
> +properties:
> +  $nodename:
> +    pattern: "^timer@[0-9a-f]+$"
> +
> +  compatible:
> +    items:
> +      - enum:
> +          - realtek,rtl930x-timer

No wildcards.

> +      - const: realtek,otto-timer

Do you have access to datasheet of all Otto SoCs and can you confirm
that all of them have the same timer programming interface? Just drop
generic compatible and use SoCs compatible.

Blank line.

> +  reg:
> +    minItems: 5
> +    maxItems: 5

Instead list and describe the items.

> +
> +  clocks:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 5
> +    maxItems: 5

Instead list and describe the items.


> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    timer0: timer@3200 {

Drop unused label

> +      compatible = "realtek,rtl930x-timer", "realtek,otto-timer";
> +      reg = <0x3200 0x10>, <0x3210 0x10>, <0x3220 0x10>,
> +            <0x3230 0x10>, <0x3240 0x10>;
> +
> +      interrupt-parent = <&intc>;
> +      interrupts = <7 4>, <8 4>, <9 4>, <10 4>, <11 4>;

Use proper defines for the flags.

> +      clocks = <&lx_clk>;
> +    };

Best regards,
Krzysztof