| Message ID | 1432865650-4062-4-git-send-email-gregory.0xf0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote: > Some brcmstb GPIO controllers can be used to wake from suspend, so use the > de facto standard property 'wakeup-source' to mark the nodes of controllers > with that capability. > > Also document interrupts-extended, which will be used for wakeup handling > because the interrupt parent for the wake IRQ is different from the regular > IRQ. > > Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com> > --- > New in v2. > > .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 26 +++++++++++++++++++++- > 1 file changed, 25 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt > index 435f1bc..568814f 100644 > --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt > +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt > @@ -33,6 +33,12 @@ Optional properties: > - interrupt-parent: > phandle of the parent interrupt controller > > +- interrupts-extended: > + Alternate form of specifying interrupts and parents that allows for > + multiple parents. This takes precedence over 'interrupts' and > + 'interrupt-parent'. This probably must be used if the wakeup-source > + property is provided because that may have a different interrupt parent. > + "This probably must be used" seems a little awkward, especially when you're just explaining an implementation detail of our SoCs, rather than something unique about this binding. Maybe: "Wakeup-capable GPIO controllers often route their wakeup interrupt lines through a different interrupt controller than the primary interrupt line, making this property necessary." > - #interrupt-cells: > Should be <2>. The first cell is the GPIO number, the second should specify > flags. The following subset of flags is supported: > @@ -48,7 +54,10 @@ Optional properties: > Marks the device node as an interrupt controller > > - interrupt-names: > - The name of the IRQ resource used by this controller > + The names of the IRQ resources used by this controller If you're specifying names, you should list them here. > + > +- wakeup-source: > + GPIOs for this controller can be used as a wakeup source > > Example: > upg_gio: gpio@f040a700 { > @@ -63,3 +72,18 @@ Example: > interrupt-names = "upg_gio"; > brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>; > }; > + > + upg_gio_aon: gpio@f04172c0 { > + #gpio-cells = <0x2>; > + #interrupt-cells = <0x2>; Might use decimal instead of hex for the above 2 lines? > + compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio"; > + gpio-controller; > + interrupt-controller; > + reg = <0xf04172c0 0x40>; > + interrupt-parent = <0xc>; That should be a phandle, not an int (I realize phandles resolve down to an integer, but we're speaking DTS, not DTB). > + interrupts = <0x6>; > + interrupts-extended = <0xc 0x6 0xa 0x5>; Same here (phandles). Also, even though the interrupt binding semantics specify precedence between interrupts and interrupts-extended, I'd think an example should stick to one or the other, no? > + interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup"; > + wakeup-source; > + brcm,gpio-bank-widths = <0x12 0x4>; > + }; Reviewed-by: Brian Norris <computersforpeace@gmail.com>
On Fri, May 29, 2015 at 5:36 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote: >> Some brcmstb GPIO controllers can be used to wake from suspend, so use the >> de facto standard property 'wakeup-source' to mark the nodes of controllers >> with that capability. >> >> Also document interrupts-extended, which will be used for wakeup handling >> because the interrupt parent for the wake IRQ is different from the regular >> IRQ. >> >> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com> >> --- >> New in v2. >> >> .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 26 +++++++++++++++++++++- >> 1 file changed, 25 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt >> index 435f1bc..568814f 100644 >> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt >> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt >> @@ -33,6 +33,12 @@ Optional properties: >> - interrupt-parent: >> phandle of the parent interrupt controller >> >> +- interrupts-extended: >> + Alternate form of specifying interrupts and parents that allows for >> + multiple parents. This takes precedence over 'interrupts' and >> + 'interrupt-parent'. This probably must be used if the wakeup-source >> + property is provided because that may have a different interrupt parent. >> + > > "This probably must be used" seems a little awkward, especially when > you're just explaining an implementation detail of our SoCs, rather than > something unique about this binding. Maybe: > > "Wakeup-capable GPIO controllers often route their wakeup interrupt > lines through a different interrupt controller than the primary > interrupt line, making this property necessary." That wording does seem better, will change. > >> - #interrupt-cells: >> Should be <2>. The first cell is the GPIO number, the second should specify >> flags. The following subset of flags is supported: >> @@ -48,7 +54,10 @@ Optional properties: >> Marks the device node as an interrupt controller >> >> - interrupt-names: >> - The name of the IRQ resource used by this controller >> + The names of the IRQ resources used by this controller > > If you're specifying names, you should list them here. I was wondering about that. Some bindings have them listed, some don't. In this case I know what names currently exist but there could certainly be different ones in the future. How does that work? Or am I misunderstanding what this field is used for? Where are the documented rules for this? > >> + >> +- wakeup-source: >> + GPIOs for this controller can be used as a wakeup source >> >> Example: >> upg_gio: gpio@f040a700 { >> @@ -63,3 +72,18 @@ Example: >> interrupt-names = "upg_gio"; >> brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>; >> }; >> + >> + upg_gio_aon: gpio@f04172c0 { >> + #gpio-cells = <0x2>; >> + #interrupt-cells = <0x2>; > > Might use decimal instead of hex for the above 2 lines? Sure. > >> + compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio"; >> + gpio-controller; >> + interrupt-controller; >> + reg = <0xf04172c0 0x40>; >> + interrupt-parent = <0xc>; > > That should be a phandle, not an int (I realize phandles resolve down to > an integer, but we're speaking DTS, not DTB). OK. > >> + interrupts = <0x6>; >> + interrupts-extended = <0xc 0x6 0xa 0x5>; > > Same here (phandles). > > Also, even though the interrupt binding semantics specify precedence > between interrupts and interrupts-extended, I'd think an example should > stick to one or the other, no? This is the output that we actually get from the bootloader. But regardless, IMO the example should have both cases: precedence is well-defined, both sets of information are valid, and the driver can handle the case where interrupts-extended is not an understood property. > >> + interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup"; >> + wakeup-source; >> + brcm,gpio-bank-widths = <0x12 0x4>; >> + }; > > Reviewed-by: Brian Norris <computersforpeace@gmail.com> Thanks for the review, Gregory
On Fri, May 29, 2015 at 05:57:50PM -0700, Gregory Fong wrote: > On Fri, May 29, 2015 at 5:36 PM, Brian Norris > <computersforpeace@gmail.com> wrote: > > On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote: > >> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt > >> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt > >> @@ -33,6 +33,12 @@ Optional properties: ... > >> - #interrupt-cells: > >> Should be <2>. The first cell is the GPIO number, the second should specify > >> flags. The following subset of flags is supported: > >> @@ -48,7 +54,10 @@ Optional properties: > >> Marks the device node as an interrupt controller > >> > >> - interrupt-names: > >> - The name of the IRQ resource used by this controller > >> + The names of the IRQ resources used by this controller > > > > If you're specifying names, you should list them here. > > I was wondering about that. Some bindings have them listed, some > don't. In this case I know what names currently exist but there could > certainly be different ones in the future. How does that work? Or am > I misunderstanding what this field is used for? Where are the > documented rules for this? The only documentation I see is: Documentation/devicetree/bindings/resource-names.txt That documents the basics of the *-names properties, not their expected usage. In practice, they're only useful if you have enough optional resources that fixed indexing isn't sufficient, and you need to use platform_get_resource_byname(). So IMO, their purposes seems to be one of these: (1) functional (e.g., for get_resource_byname(), when you have more than one optional resource) (2) self-documentation (which might run counter to #1, as you begin generating too many unique names) (3) no purpose So IMO, if you ever want (1), they shouldn't have instance-specific naming, but should use something generic to the device class. Otherwise, they are just self-documentation, and aren't functionally useful. So IMO, these sorts of names: interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup"; work better as functional descriptions: interrupt-names = "gio", "wakeup"; But in the end, I wouldn't foresee you needing to do (1), so you're left with (2) or (3), at which point I'm not sure if you should even mention the property. Just my 2 cents (and those cents may not even be worth face value), Brian
On Fri, May 29, 2015 at 6:37 PM, Brian Norris <computersforpeace@gmail.com> wrote: > On Fri, May 29, 2015 at 05:57:50PM -0700, Gregory Fong wrote: >> On Fri, May 29, 2015 at 5:36 PM, Brian Norris >> <computersforpeace@gmail.com> wrote: >> > On Thu, May 28, 2015 at 07:14:07PM -0700, Gregory Fong wrote: >> >> --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt >> >> +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt >> >> @@ -33,6 +33,12 @@ Optional properties: > ... >> >> - #interrupt-cells: >> >> Should be <2>. The first cell is the GPIO number, the second should specify >> >> flags. The following subset of flags is supported: >> >> @@ -48,7 +54,10 @@ Optional properties: >> >> Marks the device node as an interrupt controller >> >> >> >> - interrupt-names: >> >> - The name of the IRQ resource used by this controller >> >> + The names of the IRQ resources used by this controller >> > >> > If you're specifying names, you should list them here. >> >> I was wondering about that. Some bindings have them listed, some >> don't. In this case I know what names currently exist but there could >> certainly be different ones in the future. How does that work? Or am >> I misunderstanding what this field is used for? Where are the >> documented rules for this? > > The only documentation I see is: > Documentation/devicetree/bindings/resource-names.txt > > That documents the basics of the *-names properties, not their expected > usage. > > In practice, they're only useful if you have enough optional resources > that fixed indexing isn't sufficient, and you need to use > platform_get_resource_byname(). > > So IMO, their purposes seems to be one of these: > (1) functional (e.g., for get_resource_byname(), when you have more than > one optional resource) > (2) self-documentation (which might run counter to #1, as you begin > generating too many unique names) > (3) no purpose > > So IMO, if you ever want (1), they shouldn't have instance-specific > naming, but should use something generic to the device class. Otherwise, > they are just self-documentation, and aren't functionally useful. So > IMO, these sorts of names: > > interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup"; > > work better as functional descriptions: > > interrupt-names = "gio", "wakeup"; > > But in the end, I wouldn't foresee you needing to do (1), so you're left > with (2) or (3), at which point I'm not sure if you should even mention > the property. I'm fine with leaving out the interrupt-names property, since we're not using it here anyway. Unless there are serious objections, I'll plan on remove it from the bindings doc next round. Thanks, Gregory
diff --git a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt index 435f1bc..568814f 100644 --- a/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt +++ b/Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt @@ -33,6 +33,12 @@ Optional properties: - interrupt-parent: phandle of the parent interrupt controller +- interrupts-extended: + Alternate form of specifying interrupts and parents that allows for + multiple parents. This takes precedence over 'interrupts' and + 'interrupt-parent'. This probably must be used if the wakeup-source + property is provided because that may have a different interrupt parent. + - #interrupt-cells: Should be <2>. The first cell is the GPIO number, the second should specify flags. The following subset of flags is supported: @@ -48,7 +54,10 @@ Optional properties: Marks the device node as an interrupt controller - interrupt-names: - The name of the IRQ resource used by this controller + The names of the IRQ resources used by this controller + +- wakeup-source: + GPIOs for this controller can be used as a wakeup source Example: upg_gio: gpio@f040a700 { @@ -63,3 +72,18 @@ Example: interrupt-names = "upg_gio"; brcm,gpio-bank-widths = <0x20 0x20 0x20 0x18>; }; + + upg_gio_aon: gpio@f04172c0 { + #gpio-cells = <0x2>; + #interrupt-cells = <0x2>; + compatible = "brcm,bcm7445-gpio", "brcm,brcmstb-gpio"; + gpio-controller; + interrupt-controller; + reg = <0xf04172c0 0x40>; + interrupt-parent = <0xc>; + interrupts = <0x6>; + interrupts-extended = <0xc 0x6 0xa 0x5>; + interrupt-names = "upg_gio_aon", "upg_gio_aon_wakeup"; + wakeup-source; + brcm,gpio-bank-widths = <0x12 0x4>; + };
Some brcmstb GPIO controllers can be used to wake from suspend, so use the de facto standard property 'wakeup-source' to mark the nodes of controllers with that capability. Also document interrupts-extended, which will be used for wakeup handling because the interrupt parent for the wake IRQ is different from the regular IRQ. Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com> --- New in v2. .../devicetree/bindings/gpio/brcm,brcmstb-gpio.txt | 26 +++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)