linux-kernel - Re: [PATCH 1/7] dt-bindings: watchdog: dw-wdt: Replace legacy bindings file with YAML-based one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <8c10be04-5aa2-af5e-7b15-262dcdc04875@roeck-us.net>
Date:   Fri, 6 Mar 2020 07:18:35 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     Sergey.Semin@...kalelectronics.ru,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>
Cc:     Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] dt-bindings: watchdog: dw-wdt: Replace legacy
 bindings file with YAML-based one

On 3/6/20 5:27 AM, Sergey.Semin@...kalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> 
> Modern device tree bindings are supposed to be created as YAML-files
> in accordane with dt-schema. This commit replaces the DW Watchdog
> legacy bare text bindings with YAML file. As before the the bindings
> states that the corresponding dts node is supposed to have a registers
> range reference, at least one clocks phandle reference, optional reset
> lines. Seeing all the platforms with DW Watchdog provide the watchdog
> interrupt property and since in further commit we'll alter the driver
> to use it for pre-timeout functionality implementation, lets declare
> the IRQ property to be required.
> 

First, this is not just a replacement - it changes semantics.

Second, I disagree with making interrupts mandatory. They are only needed
for pretimeout functionality, and not everyone may want to enable that.
I don't see the point of forcing everyone to enable and provide functionality
that is neither wanted or needed for a given use case. Yes, the interrupt
is provided by all users today, but we may have one coming up tomorrow
where the interrupt line is not even wired up. What then ?

Guenter

> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@...ha.franken.de>
> Cc: Paul Burton <paulburton@...nel.org>
> Cc: Ralf Baechle <ralf@...ux-mips.org>
> ---
>  .../devicetree/bindings/watchdog/dw_wdt.txt   | 24 -------
>  .../bindings/watchdog/snps,dw-wdt.yaml        | 66 +++++++++++++++++++
>  2 files changed, 66 insertions(+), 24 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/watchdog/dw_wdt.txt
>  create mode 100644 Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt b/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> deleted file mode 100644
> index eb0914420c7c..000000000000
> --- a/Documentation/devicetree/bindings/watchdog/dw_wdt.txt
> +++ /dev/null
> @@ -1,24 +0,0 @@
> -Synopsys Designware Watchdog Timer
> -
> -Required Properties:
> -
> -- compatible	: Should contain "snps,dw-wdt"
> -- reg		: Base address and size of the watchdog timer registers.
> -- clocks	: phandle + clock-specifier for the clock that drives the
> -		watchdog timer.
> -
> -Optional Properties:
> -
> -- interrupts	: The interrupt used for the watchdog timeout warning.
> -- resets	: phandle pointing to the system reset controller with
> -		line index for the watchdog.
> -
> -Example:
> -
> -	watchdog0: wd@...02000 {
> -		compatible = "snps,dw-wdt";
> -		reg = <0xffd02000 0x1000>;
> -		interrupts = <0 171 4>;
> -		clocks = <&per_base_clk>;
> -		resets = <&rst WDT0_RESET>;
> -	};
> diff --git a/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> new file mode 100644
> index 000000000000..8b30f9601c38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/snps,dw-wdt.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Copyright (C) 2019 BAIKAL ELECTRONICS, JSC
> +#
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/snps,dw-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Synopsys Designware Watchdog Timer
> +
> +allOf:
> +  - $ref: "watchdog.yaml#"
> +
> +maintainers:
> +  - Jamie Iles <jamie@...ieiles.com>
> +
> +properties:
> +  compatible:
> +    const: snps,dw-wdt
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description: DW Watchdog pre-timeout interrupts.
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    items:
> +      - description: Watchdog timer reference clock.
> +      - description: APB3 interface clock.
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: tclk
> +      - const: pclk
> +
> +  assigned-clocks: true
> +
> +  assigned-clock-rates: true
> +
> +  resets:
> +    description: Phandle to the DW Watchdog reset lane.
> +    maxItems: 1
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +examples:
> +  - |
> +    watchdog0: watchdog@...02000 {
> +      compatible = "snps,dw-wdt";
> +      reg = <0xffd02000 0x1000>;
> +      interrupts = <0 171 4>;
> +      clocks = <&per_base_clk>;
> +      resets = <&wdt_rst>;
> +    };
> +...
>