Re: [PATCH] dt-bindings: timer: Update TI timer to yaml and add compatible for am6

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

On 08/04/2022 10:12, Tony Lindgren wrote:
> Let's update the TI timer binding to use yaml. And add compatible for
> ti,am654-timer for TI am64, am65 and j72 SoCs. As the timer hardware is
> the same between am64, am65 and j72 we use the compatible name for the
> earliest SoC with this timer.
> 
> As this binding is specific to the TI dual-mode timers also known
> as dm-timers, let's use ti,timer-dm.yaml naming for the new file.

Thank you for your patch. There is something to discuss/improve.

> ---
>  .../bindings/timer/ti,timer-dm.yaml           | 105 ++++++++++++++++++
>  .../devicetree/bindings/timer/ti,timer.txt    |  44 --------

pwm-omap-dmtimer.txt references old path.


>  2 files changed, 105 insertions(+), 44 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/timer/ti,timer-dm.yaml
>  delete mode 100644 Documentation/devicetree/bindings/timer/ti,timer.txt
> 
> diff --git a/Documentation/devicetree/bindings/timer/ti,timer-dm.yaml b/Documentation/devicetree/bindings/timer/ti,timer-dm.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ti,timer-dm.yaml
> @@ -0,0 +1,105 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/ti,timer-dm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Binding for TI dual-mode timer

"TI dual-mode timer"

> +
> +maintainers:
> +  - Tony Lindgren <tony@atomide.com>
> +
> +description: |
> +  The TI dual-mode timer is a general purpose timer with PWM capabilities.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,omap2420-timer
> +      - ti,omap3430-timer
> +      - ti,omap4430-timer
> +      - ti,omap5430-timer
> +      - ti,am335x-timer
> +      - ti,am335x-timer-1ms
> +      - ti,am654-timer

How about ordering the entries by name (so amxxx before omapxxx)?

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +    description: Timer IO register range

This was maxItems:1 in old binding, so please mention briefly in commit
msg why the change is needed. If only some versions need it, then add
allOf:if:then: constraints.

> +
> +  '#address-cells':
> +    enum: [ 1, 2 ]
> +
> +  '#size-cells':
> +    enum: [ 1, 2 ]

The same. Are these changes to the binding an effect of new compatible?
If yes, better to split it into two patches. One for old binding
(passing dtbs_check on old compatibles) and one for new compatible with
new properties.

> +
> +  clocks:
> +    description:
> +      The functional clock for the timer. Some SoCs like omap24xx also have a
> +      separate interface clock, and some clocks may be only defined for the
> +      interconnect target module parent.
> +    minItems: 1
> +    maxItems: 2

The same - not mentioned in commit msg.

> +
> +  clock-names:
> +    description:
> +      Timer clock names like "fck", "timer_sys_ck".
> +    oneOf:
> +      - enum: [ ick, fck ]
> +      - items:
> +          - const: fck
> +          - enum: [ ick, timer_sys_ck ]

Are the combinations depending on compatible? If so, you need allOf:if:then:

> +
> +  interrupts:
> +    description:
> +      Interrupt if available. The timer PWM features may be usable
> +      in a limited way even without interrupts.
> +    maxItems: 1
> +
> +  ti,timer-alwon:
> +    description:
> +      Timer is always enabled when the SoC is powered. Note that some SoCs like
> +      am335x can suspend to PM coprocessor RTC only mode and in that case the
> +      SoC power is cut including timers.
> +    type: boolean
> +
> +  ti,timer-dsp:
> +    description:
> +      Timer is routable to the DSP in addition to the operating system.
> +    type: boolean
> +
> +  ti,timer-pwm:
> +    description:
> +      Timer has been wired for PWM capability.
> +    type: boolean
> +
> +  ti,timer-secure:
> +    description:
> +      Timer access has been limited to secure mode only.
> +    type: boolean
> +
> +  ti,hwmods:
> +    description:
> +      Name of the HWMOD associated with timer. This is for legacy
> +      omap2/3 platforms only.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    deprecated: true
> +
> +required:
> +  - compatible
> +  - reg

Missing interrupts - they were required. Aren't anymore?

> +
> +additionalProperties: false
> +


Best regards,
Krzysztof