Re: [PATCH v1 1/3] dt-bindings: watchdog: Add watchdog for StarFive

[Xingyu Wu]

On 2022/12/2 18:46, Krzysztof Kozlowski wrote:
> On 02/12/2022 10:39, xingu.wu wrote:
>> From: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> 
>> Add bindings to describe the watchdog for the StarFive SoCs.
>> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> ---
>>  .../bindings/watchdog/starfive,wdt.yaml       | 77 +++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
>> new file mode 100644
>> index 000000000000..ece3e80153a0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/watchdog/starfive,wdt.yaml
> 
> Filename should be based on compatible. You do not allow here any other
> models... If you intent and you are 100% sure you will grow with new
> models, make it maybe a family-based name.

First, thank you for your reply. We have some other SoCs like JH7100 would use
this watchdog driver, but we now use JH7110 as our main release chip.
As you say, should we use "starfive,jh71xx-wdt.yaml" as filename?


>> @@ -0,0 +1,77 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/watchdog/starfive,wdt.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive Watchdog
>> +
>> +allOf:
>> +  - $ref: "watchdog.yaml#"
> 
> Drop quotes.

Will fix.
>> +
>> +maintainers:
>> +  - Samin Guo <samin.guo@xxxxxxxxxxxxxxxx>
>> +  - Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> +
>> +properties:
>> +  compatible:
>> +    const: starfive,jh7110-wdt
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: Core clock
>> +      - description: APB clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: core_clk
> 
> Drop _clk

Will fix.

> 
>> +      - const: apb_clk
> 
> Drop _clk

Will fix.

> 
>> +
>> +  resets:
>> +    items:
>> +      - description: APB reset
>> +      - description: Core reset
>> +
>> +  reset-names:
>> +    items:
>> +      - const: rst_apb
> 
> Drop rst_

Will fix.

> 
>> +      - const: rst_core
> 
> Ditto

Will fix.

> 
>> +
>> +  timeout-sec: true
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - resets
>> +  - reset-names
>> +  - timeout-sec
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/starfive-jh7110.h>
>> +    #include <dt-bindings/reset/starfive-jh7110.h>
>> +
>> +    watchdog@13070000 {
>> +            compatible = "starfive,jh7110-wdt";
> 
> Use 4 spaces for example indentation.

Will fix.

> 
>> +            reg = <0x13070000 0x10000>;
>> +            interrupts = <68>;
>> +            clocks = <&syscrg_clk JH7110_SYSCLK_WDT_CORE>,
>> +                     <&syscrg_clk JH7110_SYSCLK_WDT_APB>;
>> +            clock-names = "core_clk", "apb_clk";
>> +            resets = <&syscrg_rst JH7110_SYSRST_WDT_APB>,
>> +                     <&syscrg_rst JH7110_SYSRST_WDT_CORE>;
>> +            reset-names = "rst_apb", "rst_core";
>> +            timeout-sec = <15>;
>> +    };
>> +
> 
> Drop trailing line.

Will fix.

Best regards,
Xingyu Wu