From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Sean Anderson <sean.anderson@seco.com>,
Vinod Koul <vkoul@kernel.org>,
Kishon Vijay Abraham I <kishon@kernel.org>,
linux-phy@lists.infradead.org
Cc: devicetree@vger.kernel.org, "Fernández Rojas" <noltari@gmail.com>,
"Bagas Sanjaya" <bagasdotme@gmail.com>,
"Madalin Bucur" <madalin.bucur@nxp.com>,
"Linus Walleij" <linus.walleij@linaro.org>,
"Jonas Gorski" <jonas.gorski@gmail.com>,
linux-gpio@vger.kernel.org, "Rob Herring" <robh+dt@kernel.org>,
"Camelia Alexandra Groza" <camelia.groza@nxp.com>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Ioana Ciornei" <ioana.ciornei@nxp.com>,
linuxppc-dev@lists.ozlabs.org,
"Bartosz Golaszewski" <brgl@bgdev.pl>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v11 03/13] dt-bindings: Convert gpio-mmio to yaml
Date: Tue, 14 Mar 2023 19:32:48 +0100 [thread overview]
Message-ID: <ad56ca5e-03f7-5e3d-6547-91c64fdb08d3@linaro.org> (raw)
In-Reply-To: <3c19e6d2-4df2-6187-36d5-98ceef07235a@seco.com>
On 14/03/2023 19:09, Sean Anderson wrote:
> On 3/14/23 13:56, Krzysztof Kozlowski wrote:
>> On 13/03/2023 17:11, Sean Anderson wrote:
>>> This is a generic binding for simple MMIO GPIO controllers. Although we
>>> have a single driver for these controllers, they were previously spread
>>> over several files. Consolidate them. The register descriptions are
>>> adapted from the comments in the source. There is no set order for the
>>> registers, so I have not specified one.
>>>
>>> Rename brcm,bcm6345-gpio to brcm,bcm63xx-gpio to reflect that bcm6345
>>> has moved.
>>>
>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>> Linus or Bartosz, feel free to pick this up as the rest of this series
>>> may not be merged any time soon.
>>>
>>> Changes in v11:
>>> - Keep empty (or almost-empty) properties on a single line
>>> - Don't use | unnecessarily
>>> - Use gpio as the node name for examples
>>> - Rename brcm,bcm6345-gpio.yaml to brcm,bcm63xx-gpio.yaml
>>>
>>> Changes in v10:
>>> - New
>>>
>>> ...m6345-gpio.yaml => brcm,bcm63xx-gpio.yaml} | 16 +--
>>> .../devicetree/bindings/gpio/gpio-mmio.yaml | 134 ++++++++++++++++++
>>> .../bindings/gpio/ni,169445-nand-gpio.txt | 38 -----
>>> .../devicetree/bindings/gpio/wd,mbl-gpio.txt | 38 -----
>>> 4 files changed, 135 insertions(+), 91 deletions(-)
>>> rename Documentation/devicetree/bindings/gpio/{brcm,bcm6345-gpio.yaml => brcm,bcm63xx-gpio.yaml} (78%)
>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mmio.yaml
>>> delete mode 100644 Documentation/devicetree/bindings/gpio/ni,169445-nand-gpio.txt
>>> delete mode 100644 Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.yaml b/Documentation/devicetree/bindings/gpio/brcm,bcm63xx-gpio.yaml
>>> similarity index 78%
>>> rename from Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.yaml
>>> rename to Documentation/devicetree/bindings/gpio/brcm,bcm63xx-gpio.yaml
>>> index 4d69f79df859..e11f4af49c52 100644
>>> --- a/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.yaml
>>> +++ b/Documentation/devicetree/bindings/gpio/brcm,bcm63xx-gpio.yaml
>>
>>
>>> +
>>> +description:
>>> + Some simple GPIO controllers may consist of a single data register or a pair
>>> + of set/clear-bit registers. Such controllers are common for glue logic in
>>> + FPGAs or ASICs. Commonly, these controllers are accessed over memory-mapped
>>> + NAND-style parallel busses.
>>> +
>>> +properties:
>>> + big-endian: true
>>> +
>>> + compatible:
>>
>> Keep compatible as first property.
>
> I thought it was alphabetical.
There is no clear rule, except that compatible is always first. In the
DTS reg is second, in bindings usually as well but not always.
>
>>> + enum:
>>> + - brcm,bcm6345-gpio # Broadcom BCM6345 GPIO controller
>>> + - wd,mbl-gpio # Western Digital MyBook Live memory-mapped GPIO controller
>>> + - ni,169445-nand-gpio # National Instruments 169445 GPIO NAND controller
>>
>> I think you got comment that these comments are making things
>> unreadable. I don't see here improvement.
>
> That was not the comment I got.
OK
>
> | I think you can inline description: statements in the enum instead of
> | the # hash comments, however IIRC you have to use oneOf and
> | const: to do it, like I do in
> | Documentation/devicetree/bindings/input/touchscreen/cypress,cy8ctma340.yaml
> | but don't overinvest in this if it is cumbersome.
>
> I investigated this and determined it was cumbersome.
So just :
# Western Digital MyBook Live memory-mapped GPIO controller
- wd,mbl-gpio
>
>> For example first comment is useless - you say the same as compatible.
>> Same with last one. So only remaining WD comment should be made in new
>> line so everything is nicely readable.
>
> I don't understand what you mean by "made in new line". Anyway, I will
> leave just the WD comment.
>
>> BTW, order the enum by name.
>
> OK
>
>>> +
>>> + '#gpio-cells':
>>> + const: 2
>>> +
>>> + gpio-controller:
>>> + true
>>
>> I am sure I saw comments here...
>>
>> https://lore.kernel.org/all/20230308231018.GA4039466-robh@kernel.org/
>
> OK
>
>>> +
>>> + reg:
>>> + minItems: 1
>>> + description:
>>> + A list of registers in the controller. The width of each register is
>>> + determined by its size.
>>
>> I don't understand this comment. Aren't you describing now what 'reg' is
>> in DT spec? If so, drop. If not, please share more.
>
> Each register describes exactly one hardware register. In some other
> device, when you see `regs = <0x8000000 0x100>`, then you may have 64
> 32-bit registers. But for this device, it would be one 2048-bit
> register.
Ah, so you do not mean here address space size? OK then, thanks for
clarification.
>
>>> All registers must have the same width. The number
>>> + of GPIOs is set by the width, with bit 0 corresponding to GPIO 0.
>>> + items:
>>> + - description:
>>> + Register to READ the value of the GPIO lines. If GPIO line is high,
>>> + the bit will be set. If the GPIO line is low, the bit will be cleared.
>>> + This register may also be used to drive GPIOs if the SET register is
>>> + omitted.
>>> + - description:
>>> + Register to SET the value of the GPIO lines. Setting a bit in this
>>> + register will drive the GPIO line high.
>>> + - description:
>>> + Register to CLEAR the value of the GPIO lines. Setting a bit in this
>>> + register will drive the GPIO line low. If this register is omitted,
>>> + the SET register will be used to clear the GPIO lines as well, by
>>> + actively writing the line with 0.
>>> + - description:
>>> + Register to set the line as OUTPUT. Setting a bit in this register
>>> + will turn that line into an output line. Conversely, clearing a bit
>>> + will turn that line into an input.
>>> + - description:
>>> + Register to set this line as INPUT. Setting a bit in this register
>>> + will turn that line into an input line. Conversely, clearing a bit
>>> + will turn that line into an output.
>>> +
>>> + reg-names:
>>> + minItems: 1
>>> + maxItems: 5
>>> + items:
>>> + enum:
>>
>> Why this is in any order? Other bindings were here specific, your 'reg'
>> is also specific/fixed.
>
> Some devicetrees have dirout first, and other have dat first. There is no
> mandatory order, and some registers can be included or left out as is
> convenient to the devicetree author.
>
> reg is not specific/fixed either. It is just done that way for
> convenience (and to match the names here).
The items have order and usually we require strict order from DTS,
unless there is a reason. If there is no reason, use fixed order and
then fix the DTS.
>
>>> + - dat
>>> + - set
>>> + - clr
>>> + - dirout
>>> + - dirin
>>> +
>>> + native-endian: true
>>> +
>>> + no-output:
>>> + $ref: /schemas/types.yaml#/definitions/flag
>>> + description:
>>> + If this property is present, the controller cannot drive the GPIO lines.
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>> + - reg-names
>>> + - '#gpio-cells'
>>> + - gpio-controller
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + gpio@1f300010 {
>>> + compatible = "ni,169445-nand-gpio";
>>> + reg = <0x1f300010 0x4>;
>>> + reg-names = "dat";
>>> + gpio-controller;
>>> + #gpio-cells = <2>;
>>> + };
>>> +
>>> + gpio@1f300014 {
>>> + compatible = "ni,169445-nand-gpio";
>>> + reg = <0x1f300014 0x4>;
>>> + reg-names = "dat";
>>> + gpio-controller;
>>> + #gpio-cells = <2>;
>>> + no-output;
>>> + };
>>
>> No need to duplicate examples. Keep only one.
>
> OK
>
>> Everything is the same.
>
> Except no-output.
I would argue that even one example with no-output is enough, but sure,
can be two in total.
Best regards,
Krzysztof
next prev parent reply other threads:[~2023-03-14 18:39 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-13 16:11 [PATCH v11 00/13] phy: Add support for Lynx 10G SerDes Sean Anderson
2023-03-13 16:11 ` [PATCH v11 01/13] dt-bindings: phy: Add 2500BASE-X and 10GBASE-R Sean Anderson
2023-03-13 16:11 ` [PATCH v11 02/13] dt-bindings: phy: Add Lynx 10G phy binding Sean Anderson
2023-03-13 16:11 ` [PATCH v11 03/13] dt-bindings: Convert gpio-mmio to yaml Sean Anderson
2023-03-13 21:43 ` Linus Walleij
2023-03-14 17:56 ` Krzysztof Kozlowski
2023-03-14 18:09 ` Sean Anderson
2023-03-14 18:32 ` Krzysztof Kozlowski [this message]
2023-03-14 18:50 ` Sean Anderson
2023-03-14 19:45 ` Krzysztof Kozlowski
2023-03-14 19:52 ` Sean Anderson
2023-03-14 19:59 ` Krzysztof Kozlowski
2023-03-15 12:23 ` kernel test robot
2023-03-13 16:11 ` [PATCH v11 04/13] dt-bindings: gpio-mmio: Add compatible for QIXIS Sean Anderson
2023-03-13 16:11 ` [PATCH v11 05/13] dt-bindings: clock: Add ids for Lynx 10g PLLs Sean Anderson
2023-03-13 16:11 ` [PATCH v11 06/13] clk: Add Lynx 10G SerDes PLL driver Sean Anderson
2023-03-13 16:11 ` [PATCH v11 07/13] phy: fsl: Add Lynx 10G SerDes driver Sean Anderson
2023-03-13 16:11 ` [PATCH v11 08/13] phy: lynx10g: Enable by default on Layerscape Sean Anderson
2023-03-13 16:11 ` [PATCH v11 09/13] arm64: dts: ls1046a: Add serdes nodes Sean Anderson
2023-03-14 7:08 ` Shawn Guo
2023-03-17 19:17 ` Sean Anderson
2023-03-13 16:11 ` [PATCH v11 10/13] arm64: dts: ls1046ardb: Add serdes descriptions Sean Anderson
2023-03-13 16:11 ` [PATCH v11 11/13] arm64: dts: ls1088a: Add serdes nodes Sean Anderson
2023-03-13 16:11 ` [PATCH v11 12/13] arm64: dts: ls1088a: Prevent PCSs from probing as phys Sean Anderson
2023-03-13 16:11 ` [PATCH v11 13/13] arm64: dts: ls1088ardb: Add serdes descriptions Sean Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ad56ca5e-03f7-5e3d-6547-91c64fdb08d3@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=bagasdotme@gmail.com \
--cc=brgl@bgdev.pl \
--cc=camelia.groza@nxp.com \
--cc=devicetree@vger.kernel.org \
--cc=ioana.ciornei@nxp.com \
--cc=jonas.gorski@gmail.com \
--cc=kishon@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-phy@lists.infradead.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=madalin.bucur@nxp.com \
--cc=noltari@gmail.com \
--cc=robh+dt@kernel.org \
--cc=sean.anderson@seco.com \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).