Message-ID: <4df3ec7a-e4af-89bc-9eda-21150395a935@linaro.org>
Date:   Wed, 1 Feb 2023 09:27:05 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding
 documentation.
Content-Language: en-US
To:     nl250060@ncr.com, Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc:     linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20230126-gpio-mmio-fix-v2-0-38397aace340@ncr.com>
 <20230126-gpio-mmio-fix-v2-1-38397aace340@ncr.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230126-gpio-mmio-fix-v2-1-38397aace340@ncr.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 31/01/2023 14:49, Niall Leonard via B4 Submission Endpoint wrote:
> From: Niall Leonard <nl250060@ncr.com>
> 
> Convert existing wd,mbl-gpio binding documentation to YAML and add
> new optional propery "no-input".

Subject: drop full stop

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Subject: improve is vague. Instead: convert to DT schema


> 
> Signed-off-by: Niall Leonard <nl250060@ncr.com>
> ---
>  .../devicetree/bindings/gpio/wd,mbl-gpio.txt       | 38 -----------
>  .../devicetree/bindings/gpio/wd,mbl-gpio.yaml      | 78 ++++++++++++++++++++++
>  2 files changed, 78 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
> deleted file mode 100644
> index 038c3a6a1f4d..000000000000
> --- a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
> +++ /dev/null
> @@ -1,38 +0,0 @@
> -Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.
> -
> -The Western Digital MyBook Live has two memory-mapped GPIO controllers.
> -Both GPIO controller only have a single 8-bit data register, where GPIO
> -state can be read and/or written.
> -
> -Required properties:
> -	- compatible: should be "wd,mbl-gpio"
> -	- reg-names: must contain
> -		"dat" - data register
> -	- reg: address + size pairs describing the GPIO register sets;
> -		order must correspond with the order of entries in reg-names
> -	- #gpio-cells: must be set to 2. The first cell is the pin number and
> -			the second cell is used to specify the gpio polarity:
> -			0 = active high
> -			1 = active low
> -	- gpio-controller: Marks the device node as a gpio controller.
> -
> -Optional properties:
> -	- no-output: GPIOs are read-only.
> -
> -Examples:
> -	gpio0: gpio0@e0000000 {
> -		compatible = "wd,mbl-gpio";
> -		reg-names = "dat";
> -		reg = <0xe0000000 0x1>;
> -		#gpio-cells = <2>;
> -		gpio-controller;
> -	};
> -
> -	gpio1: gpio1@e0100000 {
> -		compatible = "wd,mbl-gpio";
> -		reg-names = "dat";
> -		reg = <0xe0100000 0x1>;
> -		#gpio-cells = <2>;
> -		gpio-controller;
> -		no-output;
> -	};
> diff --git a/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
> new file mode 100644
> index 000000000000..d1c72a42c5bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/wd,mbl-gpio.yaml
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/wd,mbl-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Western Digital's MyBook Live memory-mapped GPIO controllers.

Drop full stop.

> +
> +maintainers:
> +  - Niall Leonard <nl250060@ncr.com>
> +
> +description: |+
> +  Bindings for the Western Digital's MyBook Live memory-mapped GPIO controllers.

Drop "Bindings for". Actually drop entire line - it's redundant, you
repeat the title.

> +
> +  The Western Digital MyBook Live has two memory-mapped GPIO controllers.
> +  Both GPIO controller only have a single 8-bit data register, where GPIO
> +  state can be read and/or written.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - wd,mbl-gpio
> +
> +  reg-names:
> +    items:
> +      - const: dat
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  gpio-controller: true
> +
> +  no-output:
> +    description: GPIOs are read-only.
> +
> +  no-input:
> +    description: GPIOs are write-only.


Split adding new property into separate patch. Each patch should do one
logical change. New feature is another logical change. Conversion is
that logical change (with any fixes needed for successful conversion,
but not with new features).


> +
> +required:
> +  - compatible
> +  - reg-names
> +  - reg
> +  - '#gpio-cells'

Use consistent quotes - either ' or "

> +  - gpio-controller
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    gpio0: gpio0@e0000000 {

gpio@

(0 is not correct name of node)

> +        compatible = "wd,mbl-gpio";
> +        reg-names = "dat";
> +        reg = <0xe0000000 0x1>;
> +        #gpio-cells = <2>;
> +        gpio-controller;
> +    };
> +
> +    gpio1: gpio1@e0100000 {

gpio@

> +        compatible = "wd,mbl-gpio";
> +        reg-names = "dat";
> +        reg = <0xe0100000 0x1>;
> +        #gpio-cells = <2>;
> +        gpio-controller;
> +        no-output;
> +    };
> +

Drop all examples below, they are not needed. Actually even these two
above could be combined as they differ with only one property.

Best regards,
Krzysztof