| Message ID | 20210315114214.3096-1-noltari@gmail.com |
|---|---|
| Headers | show |
| Series | pinctrl: add BCM63XX pincontrol support | expand |
On Mon, Mar 15, 2021 at 12:42 PM Álvaro Fernández Rojas
<noltari@gmail.com> wrote:
> v7: introduce changes suggested by Rob Herring.
If Rob is happy with the bindings like this (GPIO as parallel node rathern
than subnode) I am ready to merge this.
As long as the bindings are OK I am pretty sure any remaining nits can
be fixed in-tree.
Yours,
Linus Walleij
On Mon, Mar 15, 2021 at 12:41:57PM +0100, Álvaro Fernández Rojas wrote: > Add binding documentation for the pincontrol core found in BCM6328 SoCs. > > Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com> > Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > v7: add changes suggested by Rob Herring > v6: add changes suggested by Rob Herring > v5: change Documentation to dt-bindings in commit title > v4: no changes > v3: add new gpio node > v2: remove interrupts > > .../pinctrl/brcm,bcm6328-pinctrl.yaml | 132 ++++++++++++++++++ > 1 file changed, 132 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > > diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > new file mode 100644 > index 000000000000..e1ecdc578f32 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > @@ -0,0 +1,132 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom BCM6328 pin controller > + > +maintainers: > + - Álvaro Fernández Rojas <noltari@gmail.com> > + - Jonas Gorski <jonas.gorski@gmail.com> > + > +description: |+ > + The pin controller node should be the child of a syscon node. > + > + Refer to the the bindings described in > + Documentation/devicetree/bindings/mfd/syscon.yaml > + > +properties: > + compatible: > + const: brcm,bcm6328-pinctrl > + > + reg: > + maxItems: 1 > + > +patternProperties: > + '^.*-pins$': '-pins$' is equivalent. > + if: > + type: object > + then: You don't need this if/then. This should be: '-pins$': type: object $ref: pinmux-node.yaml# additionalProperties: false properties: ... > + properties: > + function: > + $ref: "pinmux-node.yaml#/properties/function" Drop this as you need the $ref up a level. > + enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq, > + led, ephy0_act_led, ephy1_act_led, ephy2_act_led, > + ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ] > + > + pins: > + $ref: "pinmux-node.yaml#/properties/pins" > + enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19, > + gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1, > + usb_port1 ] > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + pinctrl@18 { > + compatible = "brcm,bcm6328-pinctrl"; > + reg = <0x18 0x10>; > + > + pinctrl_serial_led: serial_led-pins { > + pinctrl_serial_led_data: serial_led_data-pins { > + function = "serial_led_data"; > + pins = "gpio6"; > + }; > + > + pinctrl_serial_led_clk: serial_led_clk-pins { > + function = "serial_led_clk"; > + pins = "gpio7"; > + }; > + }; > + > + pinctrl_inet_act_led: inet_act_led-pins { > + function = "inet_act_led"; > + pins = "gpio11"; > + }; > + > + pinctrl_pcie_clkreq: pcie_clkreq-pins { > + function = "pcie_clkreq"; > + pins = "gpio16"; > + }; > + > + pinctrl_ephy0_spd_led: ephy0_spd_led-pins { > + function = "led"; > + pins = "gpio17"; > + }; > + > + pinctrl_ephy1_spd_led: ephy1_spd_led-pins { > + function = "led"; > + pins = "gpio18"; > + }; > + > + pinctrl_ephy2_spd_led: ephy2_spd_led-pins { > + function = "led"; > + pins = "gpio19"; > + }; > + > + pinctrl_ephy3_spd_led: ephy3_spd_led-pins { > + function = "led"; > + pins = "gpio20"; > + }; > + > + pinctrl_ephy0_act_led: ephy0_act_led-pins { > + function = "ephy0_act_led"; > + pins = "gpio25"; > + }; > + > + pinctrl_ephy1_act_led: ephy1_act_led-pins { > + function = "ephy1_act_led"; > + pins = "gpio26"; > + }; > + > + pinctrl_ephy2_act_led: ephy2_act_led-pins { > + function = "ephy2_act_led"; > + pins = "gpio27"; > + }; > + > + pinctrl_ephy3_act_led: ephy3_act_led-pins { > + function = "ephy3_act_led"; > + pins = "gpio28"; > + }; > + > + pinctrl_hsspi_cs1: hsspi_cs1-pins { > + function = "hsspi_cs1"; > + pins = "hsspi_cs1"; > + }; > + > + pinctrl_usb_port1_device: usb_port1_device-pins { > + function = "usb_device_port"; > + pins = "usb_port1"; > + }; > + > + pinctrl_usb_port1_host: usb_port1_host-pins { > + function = "usb_host_port"; > + pins = "usb_port1"; > + }; > + }; > -- > 2.20.1 >
On Mon, Mar 15, 2021 at 5:42 AM Álvaro Fernández Rojas <noltari@gmail.com> wrote: > > Add binding documentation for the GPIO sysctl found in BCM6318 SoCs. > > Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> > --- > v7: add changes suggested by Rob Herring > > .../mfd/brcm,bcm6318-gpio-sysctl.yaml | 179 ++++++++++++++++++ > 1 file changed, 179 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml > > diff --git a/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml b/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml > new file mode 100644 > index 000000000000..7056a490a27d > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/brcm,bcm6318-gpio-sysctl.yaml > @@ -0,0 +1,179 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mfd/brcm,bcm6318-gpio-sysctl.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom BCM6318 GPIO System Controller Device Tree Bindings > + > +maintainers: > + - Álvaro Fernández Rojas <noltari@gmail.com> > + - Jonas Gorski <jonas.gorski@gmail.com> > + > +description: > + Broadcom BCM6318 SoC GPIO system controller which provides a register map > + for controlling the GPIO and pins of the SoC. Perhaps a blurb about other registers in this block. From the registers, it looked like LED and PHY control at least. > + > +properties: > + "#address-cells": true > + > + "#size-cells": true > + > + compatible: > + items: > + - const: brcm,bcm6318-gpio-sysctl > + - const: syscon > + - const: simple-mfd > + > + ranges: > + maxItems: 1 > + > + reg: > + maxItems: 1 > + > +patternProperties: > + "^gpio@[0-9a-f]+$": > + # Child node > + type: object > + $ref: "../gpio/brcm,bcm63xx-gpio.yaml" > + description: > + GPIO controller for the SoC GPIOs. This child node definition > + should follow the bindings specified in > + Documentation/devicetree/bindings/gpio/brcm,bcm63xx-gpio.yaml. > + > + "^pinctrl@[0-9a-f]+$": > + # Child node > + type: object > + $ref: "../pinctrl/brcm,bcm6318-pinctrl.yaml" > + description: > + Pin controller for the SoC pins. This child node definition > + should follow the bindings specified in > + Documentation/devicetree/bindings/pinctrl/brcm,bcm6318-pinctrl.yaml. > + > +required: > + - "#address-cells" > + - compatible > + - ranges > + - reg > + - "#size-cells" > + > +additionalProperties: false > + > +examples: > + - | > + syscon@10000080 { > + #address-cells = <1>; > + #size-cells = <1>; > + compatible = "brcm,bcm6318-gpio-sysctl", "syscon", "simple-mfd"; > + reg = <0x10000080 0x80>; > + ranges = <0 0x10000080 0x80>; > + > + gpio@0 { > + compatible = "brcm,bcm6318-gpio"; > + reg = <0x0 0x10>; > + > + data = <0xc>; > + dirout = <0x4>; > + > + gpio-controller; > + gpio-ranges = <&pinctrl 0 0 50>; > + #gpio-cells = <2>; > + }; > + > + pinctrl: pinctrl@10 { > + compatible = "brcm,bcm6318-pinctrl"; > + reg = <0x18 0x10>, <0x54 0x18>; > + > + pinctrl_ephy0_spd_led: ephy0_spd_led-pins { > + function = "ephy0_spd_led"; > + pins = "gpio0"; > + }; > + > + pinctrl_ephy1_spd_led: ephy1_spd_led-pins { > + function = "ephy1_spd_led"; > + pins = "gpio1"; > + }; > + > + pinctrl_ephy2_spd_led: ephy2_spd_led-pins { > + function = "ephy2_spd_led"; > + pins = "gpio2"; > + }; > + > + pinctrl_ephy3_spd_led: ephy3_spd_led-pins { > + function = "ephy3_spd_led"; > + pins = "gpio3"; > + }; > + > + pinctrl_ephy0_act_led: ephy0_act_led-pins { > + function = "ephy0_act_led"; > + pins = "gpio4"; > + }; > + > + pinctrl_ephy1_act_led: ephy1_act_led-pins { > + function = "ephy1_act_led"; > + pins = "gpio5"; > + }; > + > + pinctrl_ephy2_act_led: ephy2_act_led-pins { > + function = "ephy2_act_led"; > + pins = "gpio6"; > + }; > + > + pinctrl_ephy3_act_led: ephy3_act_led-pins { > + function = "ephy3_act_led"; > + pins = "gpio7"; > + }; > + > + pinctrl_serial_led: serial_led-pins { > + pinctrl_serial_led_data: serial_led_data-pins { > + function = "serial_led_data"; > + pins = "gpio6"; > + }; > + > + pinctrl_serial_led_clk: serial_led_clk-pins { > + function = "serial_led_clk"; > + pins = "gpio7"; > + }; > + }; > + > + pinctrl_inet_act_led: inet_act_led-pins { > + function = "inet_act_led"; > + pins = "gpio8"; > + }; > + > + pinctrl_inet_fail_led: inet_fail_led-pins { > + function = "inet_fail_led"; > + pins = "gpio9"; > + }; > + > + pinctrl_dsl_led: dsl_led-pins { > + function = "dsl_led"; > + pins = "gpio10"; > + }; > + > + pinctrl_post_fail_led: post_fail_led-pins { > + function = "post_fail_led"; > + pins = "gpio11"; > + }; > + > + pinctrl_wlan_wps_led: wlan_wps_led-pins { > + function = "wlan_wps_led"; > + pins = "gpio12"; > + }; > + > + pinctrl_usb_pwron: usb_pwron-pins { > + function = "usb_pwron"; > + pins = "gpio13"; > + }; > + > + pinctrl_usb_device_led: usb_device_led-pins { > + function = "usb_device_led"; > + pins = "gpio13"; > + }; > + > + pinctrl_usb_active: usb_active-pins { > + function = "usb_active"; > + pins = "gpio40"; > + }; > + }; > + }; > -- > 2.20.1 >
Hi Linus, > El 16 mar 2021, a las 11:13, Linus Walleij <linus.walleij@linaro.org> escribió: > > On Mon, Mar 15, 2021 at 12:42 PM Álvaro Fernández Rojas > <noltari@gmail.com> wrote: > >> v7: introduce changes suggested by Rob Herring. > > If Rob is happy with the bindings like this (GPIO as parallel node rathern > than subnode) I am ready to merge this. I appreciate that, but I’m having a hard time in understanding what Rob wants and since there are no examples available on most of the stuff he’s requesting this is really frustrating... > > As long as the bindings are OK I am pretty sure any remaining nits can > be fixed in-tree. > > Yours, > Linus Walleij Best regards, Álvaro.
Hi Rob, > El 16 mar 2021, a las 21:59, Rob Herring <robh@kernel.org> escribió: > > On Mon, Mar 15, 2021 at 12:41:57PM +0100, Álvaro Fernández Rojas wrote: >> Add binding documentation for the pincontrol core found in BCM6328 SoCs. >> >> Co-developed-by: Jonas Gorski <jonas.gorski@gmail.com> >> Signed-off-by: Jonas Gorski <jonas.gorski@gmail.com> >> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> >> --- >> v7: add changes suggested by Rob Herring >> v6: add changes suggested by Rob Herring >> v5: change Documentation to dt-bindings in commit title >> v4: no changes >> v3: add new gpio node >> v2: remove interrupts >> >> .../pinctrl/brcm,bcm6328-pinctrl.yaml | 132 ++++++++++++++++++ >> 1 file changed, 132 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml >> new file mode 100644 >> index 000000000000..e1ecdc578f32 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml >> @@ -0,0 +1,132 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/pinctrl/brcm,bcm6328-pinctrl.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Broadcom BCM6328 pin controller >> + >> +maintainers: >> + - Álvaro Fernández Rojas <noltari@gmail.com> >> + - Jonas Gorski <jonas.gorski@gmail.com> >> + >> +description: |+ >> + The pin controller node should be the child of a syscon node. >> + >> + Refer to the the bindings described in >> + Documentation/devicetree/bindings/mfd/syscon.yaml >> + >> +properties: >> + compatible: >> + const: brcm,bcm6328-pinctrl >> + >> + reg: >> + maxItems: 1 >> + >> +patternProperties: >> + '^.*-pins$': > > '-pins$' is equivalent. > >> + if: >> + type: object >> + then: > > You don't need this if/then. This should be: > > '-pins$': > type: object > $ref: pinmux-node.yaml# > > additionalProperties: false > properties: > ... If I add "additionalProperties: false" I get the following error: /home/noltari/workspace/linux/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.example.dt.yaml: pinctrl@18: serial_led-pins: 'serial_led_clk-pins', 'serial_led_data-pins' do not match any of the regexes: 'pinctrl-[0-9]+' From schema: /home/noltari/workspace/linux/Documentation/devicetree/bindings/pinctrl/brcm,bcm6328-pinctrl.yaml > >> + properties: >> + function: >> + $ref: "pinmux-node.yaml#/properties/function" > > Drop this as you need the $ref up a level. > >> + enum: [ serial_led_data, serial_led_clk, inet_act_led, pcie_clkreq, >> + led, ephy0_act_led, ephy1_act_led, ephy2_act_led, >> + ephy3_act_led, hsspi_cs1, usb_device_port, usb_host_port ] >> + >> + pins: >> + $ref: "pinmux-node.yaml#/properties/pins" >> + enum: [ gpio6, gpio7, gpio11, gpio16, gpio17, gpio18, gpio19, >> + gpio20, gpio25, gpio26, gpio27, gpio28, hsspi_cs1, >> + usb_port1 ] >> + >> +required: >> + - compatible >> + - reg >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + pinctrl@18 { >> + compatible = "brcm,bcm6328-pinctrl"; >> + reg = <0x18 0x10>; >> + >> + pinctrl_serial_led: serial_led-pins { >> + pinctrl_serial_led_data: serial_led_data-pins { >> + function = "serial_led_data"; >> + pins = "gpio6"; >> + }; >> + >> + pinctrl_serial_led_clk: serial_led_clk-pins { >> + function = "serial_led_clk"; >> + pins = "gpio7"; >> + }; >> + }; >> + >> + pinctrl_inet_act_led: inet_act_led-pins { >> + function = "inet_act_led"; >> + pins = "gpio11"; >> + }; >> + >> + pinctrl_pcie_clkreq: pcie_clkreq-pins { >> + function = "pcie_clkreq"; >> + pins = "gpio16"; >> + }; >> + >> + pinctrl_ephy0_spd_led: ephy0_spd_led-pins { >> + function = "led"; >> + pins = "gpio17"; >> + }; >> + >> + pinctrl_ephy1_spd_led: ephy1_spd_led-pins { >> + function = "led"; >> + pins = "gpio18"; >> + }; >> + >> + pinctrl_ephy2_spd_led: ephy2_spd_led-pins { >> + function = "led"; >> + pins = "gpio19"; >> + }; >> + >> + pinctrl_ephy3_spd_led: ephy3_spd_led-pins { >> + function = "led"; >> + pins = "gpio20"; >> + }; >> + >> + pinctrl_ephy0_act_led: ephy0_act_led-pins { >> + function = "ephy0_act_led"; >> + pins = "gpio25"; >> + }; >> + >> + pinctrl_ephy1_act_led: ephy1_act_led-pins { >> + function = "ephy1_act_led"; >> + pins = "gpio26"; >> + }; >> + >> + pinctrl_ephy2_act_led: ephy2_act_led-pins { >> + function = "ephy2_act_led"; >> + pins = "gpio27"; >> + }; >> + >> + pinctrl_ephy3_act_led: ephy3_act_led-pins { >> + function = "ephy3_act_led"; >> + pins = "gpio28"; >> + }; >> + >> + pinctrl_hsspi_cs1: hsspi_cs1-pins { >> + function = "hsspi_cs1"; >> + pins = "hsspi_cs1"; >> + }; >> + >> + pinctrl_usb_port1_device: usb_port1_device-pins { >> + function = "usb_device_port"; >> + pins = "usb_port1"; >> + }; >> + >> + pinctrl_usb_port1_host: usb_port1_host-pins { >> + function = "usb_host_port"; >> + pins = "usb_port1"; >> + }; >> + }; >> -- >> 2.20.1
On Wed, Mar 17, 2021 at 12:20 PM Álvaro Fernández Rojas <noltari@gmail.com> wrote: > I appreciate that, but I’m having a hard time in understanding what > Rob wants and since there are no examples available on most of the > stuff he’s requesting this is really frustrating... I am sorry that the situation can be stressful. This is not Rob's fault, at least it's also mine. The real problem we have is lack of hardware people reviewing hardware descriptions, to put it simple. As reviewers we get a bit confused, then we try to make a mind map of the hardware as most driver developers do, but as we are not chip designers we will make mistakes and get confused and there will be a bit of back-and-forth and inconsistencies. The bindings have very high ambitions (to describe all hardware) but it's a bit like food: the less you know about how it's produced, the better the taste. In fact it is a best effort and involves a bit of guesswork and group effort and you are part of the group effort now :) Yours, Linus Walleij