From: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"krzysztof.kozlowski+dt@linaro.org"
<krzysztof.kozlowski+dt@linaro.org>,
"linus.walleij@linaro.org" <linus.walleij@linaro.org>,
"brgl@bgdev.pl" <brgl@bgdev.pl>,
"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
"u.kleine-koenig@pengutronix.de" <u.kleine-koenig@pengutronix.de>,
"lee.jones@linaro.org" <lee.jones@linaro.org>,
"andrew@lunn.ch" <andrew@lunn.ch>,
Vadym Kochan <vadym.kochan@plvision.eu>,
"enachman@marvell.com" <enachman@marvell.com>
Cc: "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>
Subject: Re: [PATCH v3 1/2] dt-bindings: gpio: gpio-mvebu: convert txt binding to YAML
Date: Sat, 14 May 2022 02:20:52 +0000 [thread overview]
Message-ID: <e87482cb-20b1-fe09-7233-d56786d5eda6@alliedtelesis.co.nz> (raw)
In-Reply-To: <32aab734-5890-99b2-09c9-8ec7418c7649@linaro.org>
On 13/05/22 20:38, Krzysztof Kozlowski wrote:
> On 12/05/2022 11:41, Chris Packham wrote:
>> Convert the existing device tree binding to YAML format.
>>
>> The old binding listed the interrupt-controller and related properties
>> as required but there are sufficiently many existing usages without it
>> that the YAML binding does not make the interrupt properties required.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> ---
>>
>> Notes:
>> Changes in v3:
>> - Correct indent in example
>> - Move offset and marvell,pwm-offset to separate patch
>> - Correct some documentation cross references
> Thank you for your patch. There is something to discuss/improve.
>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
>> new file mode 100644
>> index 000000000000..2d95ef707f53
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-mvebu.yaml
>> @@ -0,0 +1,143 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://scanmail.trustwave.com/?c=20988&d=rJn-4g1s6Eg0HzmHuA8bPCoTV-chhtJg5SGZN2xCmw&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fgpio%2fgpio-mvebu%2eyaml%23
>> +$schema: http://scanmail.trustwave.com/?c=20988&d=rJn-4g1s6Eg0HzmHuA8bPCoTV-chhtJg5SXKYz9BlA&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>> +
>> +title: Marvell EBU GPIO controller
>> +
>> +maintainers:
>> + - Thierry Reding <thierry.reding@gmail.com>
>> + - Lee Jones <lee.jones@linaro.org>
> These should be rather platform or driver maintainers, not subsystem
> folks. Unless it happens that Thierry and Lee are for platform?
Based on lines authored that would be Thomas and Andrew. But perhaps
someone from Marvell or PLVision have an interest in adopting the driver
and it's binding?
For now I'll put Thomas and Andrew until someone else steps up.
>
>> +
>> +properties:
>> + compatible:
>> + oneOf:
>> + - enum:
>> + - marvell,armada-8k-gpio
>> + - marvell,orion-gpio
>> +
>> + - items:
>> + - enum:
>> + - marvell,mv78200-gpio
>> + - marvell,armada-370-gpio
>> + - marvell,armadaxp-gpio
>> + - const: marvell,orion-gpio
>> +
>> + reg:
>> + description: |
>> + Address and length of the register set for the device. Not used for
>> + marvell,armada-8k-gpio.
>> +
>> + For the "marvell,armadaxp-gpio" variant a second entry is expected for
>> + the per-cpu registers. For other variants second entry can be provided,
>> + for the PWM function using the GPIO Blink Counter on/off registers.
>> + minItems: 1
>> + maxItems: 2
>> +
>> + reg-names:
>> + items:
>> + - const: gpio
>> + - const: pwm
>> + minItems: 1
>> +
>> + interrupts:
>> + description: |
>> + The list of interrupts that are used for all the pins managed by this
>> + GPIO bank. There can be more than one interrupt (example: 1 interrupt
>> + per 8 pins on Armada XP, which means 4 interrupts per bank of 32
>> + GPIOs).
>> + minItems: 1
>> + maxItems: 4
>> +
>> + interrupt-controller: true
>> +
>> + "#interrupt-cells":
>> + const: 2
>> +
>> + gpio-controller: true
>> +
>> + ngpios:
>> + minimum: 1
>> + maximum: 32
>> +
>> + "#gpio-cells":
>> + const: 2
>> +
>> + "#pwm-cells":
>> + description:
>> + The first cell is the GPIO line number. The second cell is the period
>> + in nanoseconds.
>> + const: 2
>> +
>> + clocks:
>> + description:
>> + Clock(s) used for PWM function.
>> + items:
>> + - description: Core clock
>> + - description: AXI bus clock
>> + minItems: 1
>> +
>> + clock-names:
>> + items:
>> + - const: core
>> + - const: axi
>> + minItems: 1
>> +
>> +required:
>> + - compatible
>> + - gpio-controller
>> + - ngpios
>> + - "#gpio-cells"
>> +
>> +allOf:
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: marvell,armada-8k-gpio
>> + then:
>> + required:
>> + - offset
>> + else:
>> + required:
>> + - reg
> one blank line please
>
>> + - if:
>> + properties:
>> + compatible:
>> + contains:
>> + const: marvell,armadaxp-gpio
> Original bindings are saying that second reg is optional for
> marvell,armada-370-gpio. What about other cases, e.g. mv78200-gpio? Is
> it also allowed (and optional) there?
This is where things get interesting. The armadaxp (and only the
armadaxp) requires a second register value for some per-cpu registers.
All of the other SoCs can have an optional 2nd register value if they
want to use the PWM function. I guess that implies that the armadaxp
can't do PWM.
>> + then:
>> + properties:
>> + reg:
>> + minItems: 2
> Then you also should require two reg-names.
Simple enough to add. But currently we've said that the reg-names are
"gpio" and "pwm" but on the armadaxp the 2nd one is not "pwm" but
something else ("per-cpu" perhaps?)
On the other hand this is all completely moot because the
armada-xp-mv78*.dtsi actually use the "marvell,armada-370-gpio"
compatible so this appears to be documenting something that is no longer
used. Indeed it appears that the armadaxp specific usage was remove in
5f79c651e81e ("arm: mvebu: use global interrupts for GPIOs on Armada XP").
So perhaps the best course of action is to drop marvell,armadaxp-gpio
from the new binding (noting that we've done so in the commit message).
>
>
> Best regards,
> Krzysztof
next prev parent reply other threads:[~2022-05-14 2:56 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-12 9:41 [PATCH v3 1/2] dt-bindings: gpio: gpio-mvebu: convert txt binding to YAML Chris Packham
2022-05-12 9:41 ` [PATCH v3 2/2] dt-bindings: gpio: gpio-mvebu: document offset and marvell,pwm-offset Chris Packham
2022-05-13 8:40 ` Krzysztof Kozlowski
2022-05-13 8:38 ` [PATCH v3 1/2] dt-bindings: gpio: gpio-mvebu: convert txt binding to YAML Krzysztof Kozlowski
2022-05-14 2:20 ` Chris Packham [this message]
2022-05-14 20:28 ` Krzysztof Kozlowski
2022-05-15 21:20 ` Chris Packham
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=e87482cb-20b1-fe09-7233-d56786d5eda6@alliedtelesis.co.nz \
--to=chris.packham@alliedtelesis.co.nz \
--cc=andrew@lunn.ch \
--cc=brgl@bgdev.pl \
--cc=devicetree@vger.kernel.org \
--cc=enachman@marvell.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=krzysztof.kozlowski@linaro.org \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pwm@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=thierry.reding@gmail.com \
--cc=u.kleine-koenig@pengutronix.de \
--cc=vadym.kochan@plvision.eu \
/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).