| Message ID | 20200910175733.11046-1-krzk@kernel.org |
|---|---|
| Headers | show |
| Series | ARM: dts: / gpio: Add dtschema for NXP PCA953x and correct dts | expand |
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?
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..
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 >
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
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
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 >
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
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
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>
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>; > >
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