mbox series

[v2,00/15] ARM: dts: / gpio: Add dtschema for NXP PCA953x and correct dts

Message ID 20200910175733.11046-1-krzk@kernel.org
Headers show
Series ARM: dts: / gpio: Add dtschema for NXP PCA953x and correct dts | expand

Message

Krzysztof Kozlowski Sept. 10, 2020, 5:57 p.m. UTC
Hi,

Changes since v1:
1. Patch 1: Use additionalProperties, Add wakeup-source, Add hogs, Extend example with hogs.
2. New patches: 3, 4, 5, 6, 7, 9, 10, 12, 14 and 15.

The patches could be picked up independently if dtschema makes sense.
The fixes for pins make sense anyway, regardless of dtschema.

Best regards,
Krzysztof


Krzysztof Kozlowski (15):
  dt-bindings: gpio: convert bindings for NXP PCA953x family to dtschema
  dt-bindings: gpio: convert bindings for Maxim MAX732x family to
    dtschema
  arm64: dts: mediatek: fix tca6416 reset GPIOs in pumpkin
  arm64: dts: mediatek: align GPIO hog names with dtschema
  arm64: dts: renesas: align GPIO hog names with dtschema
  arm64: dts: ti: align GPIO hog names with dtschema
  arm64: dts: xilinx: align GPIO hog names with dtschema
  ARM: dts: am335x: lxm: fix PCA9539 GPIO expander properties
  ARM: dts: am335x: t335: align GPIO hog names with dtschema
  ARM: dts: am3874: iceboard: fix GPIO expander reset GPIOs
  ARM: dts: aspeed: fix PCA95xx GPIO expander properties on Portwell
  ARM: dts: aspeed: align GPIO hog names with dtschema
  ARM: dts: dove: fix PCA95xx GPIO expander properties on A510
  ARM: dts: armada: align GPIO hog names with dtschema
  ARM: dts: imx6q: align GPIO hog names with dtschema

 .../devicetree/bindings/gpio/gpio-max732x.txt |  58 -----
 .../devicetree/bindings/gpio/gpio-pca953x.txt |  90 -------
 .../bindings/gpio/gpio-pca95xx.yaml           | 234 ++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |   4 -
 arch/arm/boot/dts/am335x-lxm.dts              |   4 +
 arch/arm/boot/dts/am335x-sbc-t335.dts         |   4 +-
 arch/arm/boot/dts/am3874-iceboard.dts         |   8 +-
 arch/arm/boot/dts/armada-388-clearfog.dts     |   4 +-
 arch/arm/boot/dts/armada-388-clearfog.dtsi    |  10 +-
 arch/arm/boot/dts/armada-388-helios4.dts      |   6 +-
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts  |   2 +-
 arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts   |  16 +-
 .../boot/dts/aspeed-bmc-portwell-neptune.dts  |   2 +
 arch/arm/boot/dts/dove-sbc-a510.dts           |   1 +
 arch/arm/boot/dts/imx6q-b450v3.dts            |  14 +-
 arch/arm/boot/dts/imx6q-b650v3.dts            |  12 +-
 arch/arm/boot/dts/imx6q-b850v3.dts            |   4 +-
 arch/arm/boot/dts/imx6q-bx50v3.dtsi           |  12 +-
 .../boot/dts/mediatek/pumpkin-common.dtsi     |  28 +--
 .../boot/dts/renesas/r8a77951-salvator-xs.dts |   2 +-
 .../boot/dts/renesas/r8a77965-salvator-xs.dts |   2 +-
 arch/arm64/boot/dts/renesas/ulcb-kf.dtsi      |  14 +-
 .../dts/ti/k3-j721e-common-proc-board.dts     |   4 +-
 .../boot/dts/xilinx/zynqmp-zcu102-revA.dts    |   8 +-
 24 files changed, 316 insertions(+), 227 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-max732x.txt
 delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml

Comments

Nishanth Menon Sept. 10, 2020, 6:28 p.m. UTC | #1
On 19:57-20200910, Krzysztof Kozlowski wrote:
[...]
> +  wakeup-source:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +
> +patternProperties:
> +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":

I wonder if "hog" is too generic and might clash with "something-hog" in
the future?
Nishanth Menon Sept. 10, 2020, 7:13 p.m. UTC | #2
On 20:53-20200910, Krzysztof Kozlowski wrote:
> On Thu, 10 Sep 2020 at 20:28, Nishanth Menon <nm@ti.com> wrote:
> >
> > On 19:57-20200910, Krzysztof Kozlowski wrote:
> > [...]
> > > +  wakeup-source:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +
> > > +patternProperties:
> > > +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> >
> > I wonder if "hog" is too generic and might clash with "something-hog" in
> > the future?
> 
> This pattern is already used in
> Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml. It will
> match only children and so far it did not find any other nodes in ARM
> and ARM64 dts. I don't expect clashes. Also the question is then - if
> one adds a child of GPIO expander named "foobar-hog" and it is not a
> GPIO hog, then what is it?

Probably a nitpick.. but then,.. I have'nt seen us depend on hierarchy
for uniqueness of naming.. we choose for example "bus" no matter where
in the hierarchy it falls in, as long it is a bus.. etc.. same argument
holds good for properties as well.. "gpio-hog;" is kinda redundant if
you think of it for a compatible that is already gpio ;)..

I did'nt mean to de-rail the discussion, but was curious what the DT
maintainers think..
Joel Stanley Sept. 11, 2020, 12:58 a.m. UTC | #3
On Thu, 10 Sep 2020 at 17:59, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> dtschema for pca95xx expects GPIO hogs to end with 'hog' prefix.

This is a bit ugly. Do we have to go down this path?


>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts |  2 +-
>  arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts  | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> index 1fa233d2da26..0aa437486a0d 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> @@ -560,7 +560,7 @@
>                 gpio-controller;
>                 #gpio-cells = <2>;
>
> -               smbus0 {
> +               smbus0-hog {
>                         gpio-hog;
>                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                         output-high;
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts b/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
> index cb85168f6761..577c211c469e 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts
> @@ -827,7 +827,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus0 {
> +                               smbus0-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -852,7 +852,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus1 {
> +                               smbus1-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -900,7 +900,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus2 {
> +                               smbus2-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -925,7 +925,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus3 {
> +                               smbus3-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -992,7 +992,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus4 {
> +                               smbus4-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -1017,7 +1017,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus5 {
> +                               smbus5-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -1065,7 +1065,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus6 {
> +                               smbus6-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> @@ -1090,7 +1090,7 @@
>                                 gpio-controller;
>                                 #gpio-cells = <2>;
>
> -                               smbus7 {
> +                               smbus7-hog {
>                                         gpio-hog;
>                                         gpios = <4 GPIO_ACTIVE_HIGH>;
>                                         output-high;
> --
> 2.17.1
>
Joel Stanley Sept. 11, 2020, 6:24 a.m. UTC | #4
On Thu, 10 Sep 2020 at 17:57, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Convert the NXP PCA953x family of GPIO expanders bindings to device tree
> schema.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

> +patternProperties:
> +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> +    type: object
> +    properties:
> +      gpio-hog: true
> +      gpios: true
> +      input: true
> +      output-high: true
> +      output-low: true
> +      line-name: true
> +
> +    required:
> +      - gpio-hog
> +      - gpios
> +

> +            usb3-sata-sel-hog {
> +                gpio-hog;
> +                gpios = <4 GPIO_ACTIVE_HIGH>;
> +                output-low;
> +                line-name = "usb3_sata_sel";

I would prefer we didn't require the addition of hte -hog prefix. It's
mostly just a matter of taste, but I can think of a few more concrete
reasons:

We don't require -high or -low prefixes, so the node name doesn't need
to describe the properties that will be found below.

Changing around node names for existing boards carries with it the
chance of userspace breakage (as sysfs paths change). I would prefer
we avoid that if possible.

Cheers,

Joel
Krzysztof Kozlowski Sept. 11, 2020, 6:54 a.m. UTC | #5
On Fri, 11 Sep 2020 at 08:42, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Krzysztof,
>
> On Thu, Sep 10, 2020 at 8:54 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Thu, 10 Sep 2020 at 20:28, Nishanth Menon <nm@ti.com> wrote:
> > > On 19:57-20200910, Krzysztof Kozlowski wrote:
> > > [...]
> > > > +  wakeup-source:
> > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > +
> > > > +patternProperties:
> > > > +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> > >
> > > I wonder if "hog" is too generic and might clash with "something-hog" in
> > > the future?
> >
> > This pattern is already used in
> > Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml. It will
> > match only children and so far it did not find any other nodes in ARM
> > and ARM64 dts. I don't expect clashes. Also the question is then - if
> > one adds a child of GPIO expander named "foobar-hog" and it is not a
> > GPIO hog, then what is it?
>
> Perhaps you didn't find any other nodes as children of pca953x
> controllers?

There shouldn't be.. unless one makes some i2c-gpio controller under
such GPIO expander. But now it wouldn't be instantiated as expander is
not a bus.

> There are other hog nodes in other types of GPIO controllers. Typically
> they're named after the purpose, e.g. "wifi-disable", "i2c3_mux_oe_n",
> "pcie_sata_switch", "lcd0_mux".
>
> IMHO it's a hog if it contains a "gpio-hog" property, regardless of node
> naming.

Yes. The question is then whether to expect the "hog" in name. Just
like we expect for all other device nodes to represent the class.

Best regards,
Krzysztof
Grygorii Strashko Sept. 11, 2020, 9:53 a.m. UTC | #6
On 11/09/2020 09:52, Krzysztof Kozlowski wrote:
> On Fri, 11 Sep 2020 at 08:24, Joel Stanley <joel@jms.id.au> wrote:
>>
>> On Thu, 10 Sep 2020 at 17:57, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>>
>>> Convert the NXP PCA953x family of GPIO expanders bindings to device tree
>>> schema.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>>
>>> +patternProperties:
>>> +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
>>> +    type: object
>>> +    properties:
>>> +      gpio-hog: true
>>> +      gpios: true
>>> +      input: true
>>> +      output-high: true
>>> +      output-low: true
>>> +      line-name: true
>>> +
>>> +    required:
>>> +      - gpio-hog
>>> +      - gpios
>>> +
>>
>>> +            usb3-sata-sel-hog {
>>> +                gpio-hog;
>>> +                gpios = <4 GPIO_ACTIVE_HIGH>;
>>> +                output-low;
>>> +                line-name = "usb3_sata_sel";
>>
>> I would prefer we didn't require the addition of hte -hog prefix. It's
>> mostly just a matter of taste, but I can think of a few more concrete
>> reasons:
>>
>> We don't require -high or -low prefixes, so the node name doesn't need
>> to describe the properties that will be found below.
> 
> Thanks for the comments.
> 
> It is not about properties (high or low) but the role of a device
> node. The node names should represent a generic class of device (ePAPR
> and device tree spec) and "hog" is such class.
> 
> The Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml already
> uses such naming so the best would be to unify.

In my opinion, It's not right to define this on per gpio-controller and introduce such
per gpio-controller restrictions.

More over, there is already generic schema for gpio hogs: gpio-hog.yaml
Originally, gpio bindings were defined without restricting gpio hog node names and,
generic schema follows this.

I think, the generic "gpio-hogs" sub-node may be introduced to place gpio hogs child nodes,
if gpio hogs node names restriction need to be introduces (*which i'm not sure is reasonable*).

gpio@20 {
	gpio-hogs {
		yyy-hog {
                         gpio-hog;
                         gpios
		}
	}

But this require as gpio code as generic gpio schema update (with backward compatibility in mind).


> 
>>
>> Changing around node names for existing boards carries with it the
>> chance of userspace breakage (as sysfs paths change). I would prefer
>> we avoid that if possible.
> 
> The impact on userspace is indeed important, but are you sure that
> hogs are visible to user-space via sysfs and configurable? I guess you
> think of deprecated CONFIG_GPIO_SYSFS?
> 
> Rob,
> Any hints from you about hog-naming?
> 
> Best regards,
> Krzysztof
>
Grygorii Strashko Sept. 14, 2020, 9:04 a.m. UTC | #7
On 12/09/2020 13:07, Linus Walleij wrote:
> On Fri, Sep 11, 2020 at 11:54 AM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> More over, there is already generic schema for gpio hogs: gpio-hog.yaml
> 
> Where is this? I don't have it in my GPIO devel branch for sure, and
> it is not in linux-next either so not in Bartosz' tree.
> 
> I did suggest that I want a gpio-common.yaml file which includes the
> hogs.

There it is (am I missing smth?):
pip3 install git+https://github.com/devicetree-org/dt-schema.git@master

as per:
https://www.kernel.org/doc/html/latest/devicetree/writing-schema.html
Rob Herring (Arm) Sept. 15, 2020, 7:47 p.m. UTC | #8
On Thu, Sep 10, 2020 at 02:13:05PM -0500, Nishanth Menon wrote:
> On 20:53-20200910, Krzysztof Kozlowski wrote:
> > On Thu, 10 Sep 2020 at 20:28, Nishanth Menon <nm@ti.com> wrote:
> > >
> > > On 19:57-20200910, Krzysztof Kozlowski wrote:
> > > [...]
> > > > +  wakeup-source:
> > > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > > +
> > > > +patternProperties:
> > > > +  "^(hog-[0-9]+|.+-hog(-[0-9]+)?)$":
> > >
> > > I wonder if "hog" is too generic and might clash with "something-hog" in
> > > the future?
> > 
> > This pattern is already used in
> > Documentation/devicetree/bindings/gpio/fsl-imx-gpio.yaml. It will
> > match only children and so far it did not find any other nodes in ARM
> > and ARM64 dts. I don't expect clashes. Also the question is then - if
> > one adds a child of GPIO expander named "foobar-hog" and it is not a
> > GPIO hog, then what is it?
> 
> Probably a nitpick.. but then,.. I have'nt seen us depend on hierarchy
> for uniqueness of naming.. we choose for example "bus" no matter where
> in the hierarchy it falls in, as long it is a bus.. etc.. same argument
> holds good for properties as well.. "gpio-hog;" is kinda redundant if
> you think of it for a compatible that is already gpio ;)..
> 
> I did'nt mean to de-rail the discussion, but was curious what the DT
> maintainers think..

Not really a fan of gpio-hog binding to have another type of hog nor can 
I imagine what that would be.

Rob
Rob Herring (Arm) Sept. 15, 2020, 7:59 p.m. UTC | #9
On Thu, 10 Sep 2020 19:57:19 +0200, Krzysztof Kozlowski wrote:
> Convert the NXP PCA953x family of GPIO expanders bindings to device tree
> schema.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> ---
> 
> Changes since v1:
> 1. Use additionalProperties.
> 2. Add wakeup-source.
> 3. Add hogs.
> 4. Extend example with hogs.
> ---
>  .../devicetree/bindings/gpio/gpio-pca953x.txt |  90 ----------
>  .../bindings/gpio/gpio-pca95xx.yaml           | 166 ++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |   4 -
>  3 files changed, 166 insertions(+), 94 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-pca953x.txt
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-pca95xx.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Matthias Brugger Sept. 21, 2020, 9:46 a.m. UTC | #10
On 10/09/2020 19:57, Krzysztof Kozlowski wrote:
> Correct the property for reset GPIOs of tca6416 GPIO expander.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

Applied to v5.9-next/dts64

Thanks!

> ---
>   arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi b/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
> index dfceffe6950a..29d8cf6df46b 100644
> --- a/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
> @@ -56,7 +56,7 @@
>   	tca6416: gpio@20 {
>   		compatible = "ti,tca6416";
>   		reg = <0x20>;
> -		rst-gpio = <&pio 65 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&pio 65 GPIO_ACTIVE_HIGH>;
>   		pinctrl-names = "default";
>   		pinctrl-0 = <&tca6416_pins>;
>   
>