| Message ID | 20200204213037.42100-1-geissonator@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [linux,dev-5.4] ARM: dts: aspeed: witherspoon: Add gpio line names | expand |
On Tue, Feb 04, 2020 at 03:30:37PM -0600, Andrew Geissler wrote: > From: Andrew Geissler <geissonator@yahoo.com> > > Name the gpios so libgiod will work with them > > Signed-off-by: Andrew Geissler <geissonator@yahoo.com> Great! I love that you used logical names here rather than schematic names too, so that the userspace functionality is more likely to be common across machines. It'd be great if we could start to document a list of "commonly used logical GPIO names" (ex. power-button) to facilitate this sharing. > --- > .../boot/dts/aspeed-bmc-opp-witherspoon.dts | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) >
On Tue, 4 Feb 2020 at 21:30, Andrew Geissler <geissonator@gmail.com> wrote: > > From: Andrew Geissler <geissonator@yahoo.com> > > Name the gpios so libgiod will work with them > > Signed-off-by: Andrew Geissler <geissonator@yahoo.com> Please send to the upstream lists for review. CC the GPIO maintainers too, so we can get their feedback. > --- > .../boot/dts/aspeed-bmc-opp-witherspoon.dts | 41 +++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > index 515f0f208ee6..d3bbd4fc2539 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > @@ -193,6 +193,47 @@ > > }; > > +&gpio { > + status = "okay"; > + gpio-line-names = > + /*A0-A7*/ "","cfam-reset","","","","","mux","", fsi-mux > + /*B0-B7*/ "","","","","","air-water","","", > + /*C0-C7*/ "","","","","","","","", > + /*D0-D7*/ "enable","","","","","","","", > + /*E0-E7*/ "data","","","","","","","", fsi-enable, fsi-data. > + /*F0-F7*/ "","","","","","","","", > + /*G0-G7*/ "","","","","","","","", > + /*H0-H7*/ "","","","","","","","", > + /*I0-I7*/ "","","","","","","","", > + /*J0-J7*/ "","","checkstop","","","","","", > + /*K0-K7*/ "","","","","","","","", > + /*L0-L7*/ "","","","","","","","", > + /*M0-M7*/ "","","","","","","","", > + /*N0-N7*/ "ps1-presence","","rear-fault","rear-power","rear-id","","","", These last three are LEDs? perhaps we could adopt a convention where we call them led-<name>. > + /*O0-O7*/ "","","","","","","","", > + /*P0-P7*/ "","","","","","","","ps0-presence", > + /*Q0-Q7*/ "","","","","","","","", > + /*R0-R7*/ "","","trans","","","power-button","","", fsi-trans > + /*S0-S7*/ "","","","","","","","", > + /*T0-T7*/ "","","","","","","","", > + /*U0-U7*/ "","","","","","","","", > + /*V0-V7*/ "","","","","","","","", > + /*W0-W7*/ "","","","","","","","", > + /*X0-X7*/ "","","","","","","","", > + /*Y0-Y7*/ "","","","","","","","", > + /*Z0-Z7*/ "","","","","","","","", > + /*AA0-AA7*/ "clock","","","","","","","", fsi-clock > + /*AB0-AB7*/ "","","","","","","","", > + /*AC0-AC7*/ "","","","","","","",""; > + > + pin_gpio_a1 { > + gpios = <ASPEED_GPIO(A, 1) GPIO_ACTIVE_HIGH>; > + output-high; > + line-name = "cfam-reset"; > + }; It think you want to drop this part. > + > +}; > + > &fmc { > status = "okay"; > > -- > 2.21.0 (Apple Git-122) You're building kernels on your mac? :D
> On Feb 4, 2020, at 3:59 PM, Patrick Williams <patrick@stwcx.xyz> wrote: > > On Tue, Feb 04, 2020 at 03:30:37PM -0600, Andrew Geissler wrote: >> From: Andrew Geissler <geissonator@yahoo.com> >> >> Name the gpios so libgiod will work with them >> >> Signed-off-by: Andrew Geissler <geissonator@yahoo.com> > > Great! I love that you used logical names here rather than > schematic names too, so that the userspace functionality is > more likely to be common across machines. It'd be great if > we could start to document a list of "commonly used logical > GPIO names" (ex. power-button) to facilitate this sharing. Yeah, I hadn’t really thought of that aspect but it’s a good point. Joel seemed to have some thoughts on improving conventions with naming in his review comments. I’ll address those and send upstream. If upstream seems ok with this direction then I can create a docs repo commit that documents the conventions. > >> --- >> .../boot/dts/aspeed-bmc-opp-witherspoon.dts | 41 +++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> > > -- > Patrick Williams
> On Feb 4, 2020, at 4:14 PM, Joel Stanley <joel@jms.id.au> wrote: > > On Tue, 4 Feb 2020 at 21:30, Andrew Geissler <geissonator@gmail.com <mailto:geissonator@gmail.com>> wrote: >> >> From: Andrew Geissler <geissonator@yahoo.com <mailto:geissonator@yahoo.com>> >> >> Name the gpios so libgiod will work with them >> >> Signed-off-by: Andrew Geissler <geissonator@yahoo.com <mailto:geissonator@yahoo.com>> > > Please send to the upstream lists for review. CC the GPIO maintainers > too, so we can get their feedback. ack > >> --- >> .../boot/dts/aspeed-bmc-opp-witherspoon.dts | 41 +++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> >> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >> index 515f0f208ee6..d3bbd4fc2539 100644 >> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >> @@ -193,6 +193,47 @@ >> >> }; >> >> +&gpio { >> + status = "okay"; >> + gpio-line-names = >> + /*A0-A7*/ "","cfam-reset","","","","","mux","", > > fsi-mux ack I’m wondering if I should also change the labels to match the names? For example gpioinfo will show this now: line 6: "fsi-mux" "mux" output active-high [used] > >> + /*B0-B7*/ "","","","","","air-water","","", >> + /*C0-C7*/ "","","","","","","","", >> + /*D0-D7*/ "enable","","","","","","","", >> + /*E0-E7*/ "data","","","","","","","", > > fsi-enable, fsi-data. ack >> + /*F0-F7*/ "","","","","","","","", >> + /*G0-G7*/ "","","","","","","","", >> + /*H0-H7*/ "","","","","","","","", >> + /*I0-I7*/ "","","","","","","","", >> + /*J0-J7*/ "","","checkstop","","","","","", >> + /*K0-K7*/ "","","","","","","","", >> + /*L0-L7*/ "","","","","","","","", >> + /*M0-M7*/ "","","","","","","","", >> + /*N0-N7*/ "ps1-presence","","rear-fault","rear-power","rear-id","","","", > > These last three are LEDs? perhaps we could adopt a convention where > we call them led-<name>. makes sense to me along those same lines, I’m going to put “presence” first for those gpios > >> + /*O0-O7*/ "","","","","","","","", >> + /*P0-P7*/ "","","","","","","","ps0-presence", >> + /*Q0-Q7*/ "","","","","","","","", >> + /*R0-R7*/ "","","trans","","","power-button","","", > > fsi-trans ack > >> + /*S0-S7*/ "","","","","","","","", >> + /*T0-T7*/ "","","","","","","","", >> + /*U0-U7*/ "","","","","","","","", >> + /*V0-V7*/ "","","","","","","","", >> + /*W0-W7*/ "","","","","","","","", >> + /*X0-X7*/ "","","","","","","","", >> + /*Y0-Y7*/ "","","","","","","","", >> + /*Z0-Z7*/ "","","","","","","","", >> + /*AA0-AA7*/ "clock","","","","","","","", > > fsi-clock ack > >> + /*AB0-AB7*/ "","","","","","","","", >> + /*AC0-AC7*/ "","","","","","","",""; >> + >> + pin_gpio_a1 { >> + gpios = <ASPEED_GPIO(A, 1) GPIO_ACTIVE_HIGH>; >> + output-high; >> + line-name = "cfam-reset"; >> + }; > > It think you want to drop this part. ok, my lack of understanding of what config is required for gpio’s in dts def shows here. > >> + >> +}; >> + >> &fmc { >> status = "okay"; >> >> -- >> 2.21.0 (Apple Git-122) > > You're building kernels on your mac? :D Hah, yeah, writing the code on it at least
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts index 515f0f208ee6..d3bbd4fc2539 100644 --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts @@ -193,6 +193,47 @@ }; +&gpio { + status = "okay"; + gpio-line-names = + /*A0-A7*/ "","cfam-reset","","","","","mux","", + /*B0-B7*/ "","","","","","air-water","","", + /*C0-C7*/ "","","","","","","","", + /*D0-D7*/ "enable","","","","","","","", + /*E0-E7*/ "data","","","","","","","", + /*F0-F7*/ "","","","","","","","", + /*G0-G7*/ "","","","","","","","", + /*H0-H7*/ "","","","","","","","", + /*I0-I7*/ "","","","","","","","", + /*J0-J7*/ "","","checkstop","","","","","", + /*K0-K7*/ "","","","","","","","", + /*L0-L7*/ "","","","","","","","", + /*M0-M7*/ "","","","","","","","", + /*N0-N7*/ "ps1-presence","","rear-fault","rear-power","rear-id","","","", + /*O0-O7*/ "","","","","","","","", + /*P0-P7*/ "","","","","","","","ps0-presence", + /*Q0-Q7*/ "","","","","","","","", + /*R0-R7*/ "","","trans","","","power-button","","", + /*S0-S7*/ "","","","","","","","", + /*T0-T7*/ "","","","","","","","", + /*U0-U7*/ "","","","","","","","", + /*V0-V7*/ "","","","","","","","", + /*W0-W7*/ "","","","","","","","", + /*X0-X7*/ "","","","","","","","", + /*Y0-Y7*/ "","","","","","","","", + /*Z0-Z7*/ "","","","","","","","", + /*AA0-AA7*/ "clock","","","","","","","", + /*AB0-AB7*/ "","","","","","","","", + /*AC0-AC7*/ "","","","","","","",""; + + pin_gpio_a1 { + gpios = <ASPEED_GPIO(A, 1) GPIO_ACTIVE_HIGH>; + output-high; + line-name = "cfam-reset"; + }; + +}; + &fmc { status = "okay";