Re: [PATCH v2 1/2] dt-bindings: improve wb,mbl-gpio binding documentation.

[Leonard, Niall]

On 01/02/2023 08:27, Krzysztof Kozlowski wrote:
> *External Message* - Use caution before opening links or attachments
> 
> On 31/01/2023 14:49, Niall Leonard via B4 Submission Endpoint wrote:
>> From: Niall Leonard <nl250060@xxxxxxx>
>>
>> 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@xxxxxxx>
>> ---
>>   .../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: https://urldefense.com/v3/__http://devicetree.org/schemas/gpio/wd,mbl-gpio.yaml*__;Iw!!In4Qlw!qBa7N0ccPrNQmjMeU9Hi-Ao3KxY4Fj2fXQutcNmpGxlXF7GLy7qjRk7k2ABk3yN7oi7tio6PoZyY-lsOTTreIaCxp-hI$
>> +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!In4Qlw!qBa7N0ccPrNQmjMeU9Hi-Ao3KxY4Fj2fXQutcNmpGxlXF7GLy7qjRk7k2ABk3yN7oi7tio6PoZyY-lsOTTreIbT2TzBT$
>> +
>> +title: Western Digital's MyBook Live memory-mapped GPIO controllers.
> 
> Drop full stop.
> 
>> +
>> +maintainers:
>> +  - Niall Leonard <nl250060@xxxxxxx>
>> +
>> +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
> 
Thanks for reviewing. I'll make the suggested changes.

You might note that I added myself as maintainer of this file (as it 
wouldn't pass the dt checks otherwise) but that can't really be the case 
going forwards. I have only converted it from txt to yaml. Who should be 
the maintainer ? There was no-one listed as maintainer previously ?

Regards,
Niall.