| Message ID | 1492575497-1419-3-git-send-email-bradleyb@fuzziesquirrel.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Brad, If you do future revisions of these patches, can you please Cc me? On Wed, 2017-04-19 at 00:18 -0400, Brad Bishop wrote: > Enable gpio-keys events for the checkstop and water/air cooled > gpios for use by applications on the Witherspoon system. > > > Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com> > --- > arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > index e3a7b77..aa1708e 100644 > --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > @@ -31,6 +31,26 @@ > > }; > > }; > > + gpio-keys@0 { does the @0 give us a stable device name for userspace to open? Do we really want to go this way? We now have useful unique codes for the "key"s, why not use the one node? Or is your concern we've now tied the function to a value that might vary across platforms? If so, are you relying on always specifying "air-water" in @0? If you are, I think I'd prefer the arbitrary codes, rather than this. Andrew > + compatible = "gpio-keys"; > + > > + air-water { > > + label = "air-water"; > > + gpios = <&gpio ASPEED_GPIO(B, 5) GPIO_ACTIVE_LOW>; > > + linux,code = <ASPEED_GPIO(B, 5)>; > > + }; > > + }; > + > > + gpio-keys@1 { > > + compatible = "gpio-keys"; > + > > + checkstop { > > + label = "checkstop"; > > + gpios = <&gpio ASPEED_GPIO(J, 2) GPIO_ACTIVE_LOW>; > > + linux,code = <ASPEED_GPIO(J, 2)>; > > + }; > > + }; > + > > leds { > > compatible = "gpio-leds"; >
> On Apr 19, 2017, at 10:29 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > Hi Brad, > > If you do future revisions of these patches, can you please Cc me? will do. > > On Wed, 2017-04-19 at 00:18 -0400, Brad Bishop wrote: >> Enable gpio-keys events for the checkstop and water/air cooled >> gpios for use by applications on the Witherspoon system. >> >>> Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com> >> --- >> arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >> index e3a7b77..aa1708e 100644 >> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >> @@ -31,6 +31,26 @@ >>> }; >>> }; >> >> + gpio-keys@0 { > > does the @0 give us a stable device name for userspace to open? No it doesn’t. This is my lack of DT skillz showing. I was looking into how we can give userspace a stable device name. I was going down the udev path but I can’t find any any of the information from these DT nodes in the udev event. > > Do we really want to go this way? We now have useful unique codes for > the "key"s, why not use the one node? Or is your concern we've now tied Each gpio will have an application waiting for its state to change. My concern was waking up x number of applications every time any gpio changes state. Is that a valid concern? > the function to a value that might vary across platforms? If so, are No, not worried about this. Applications will be getting their gpio number on the command line anyway. > you relying on always specifying "air-water" in @0? If you are, I think > I'd prefer the arbitrary codes, rather than this. > > Andrew > >> + compatible = "gpio-keys"; >> + >>> + air-water { >>> + label = "air-water"; >>> + gpios = <&gpio ASPEED_GPIO(B, 5) GPIO_ACTIVE_LOW>; >>> + linux,code = <ASPEED_GPIO(B, 5)>; >>> + }; >>> + }; >> + >>> + gpio-keys@1 { >>> + compatible = "gpio-keys"; >> + >>> + checkstop { >>> + label = "checkstop"; >>> + gpios = <&gpio ASPEED_GPIO(J, 2) GPIO_ACTIVE_LOW>; >>> + linux,code = <ASPEED_GPIO(J, 2)>; >>> + }; >>> + }; >> + >>> leds { >>> compatible = "gpio-leds";
On Wed, 2017-04-19 at 10:38 -0400, Brad Bishop wrote: > > > > On Apr 19, 2017, at 10:29 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > Hi Brad, > > > > If you do future revisions of these patches, can you please Cc me? > > will do. > > > > > On Wed, 2017-04-19 at 00:18 -0400, Brad Bishop wrote: > > > Enable gpio-keys events for the checkstop and water/air cooled > > > gpios for use by applications on the Witherspoon system. > > > > > > > Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com> > > > > > > --- > > > arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > > > index e3a7b77..aa1708e 100644 > > > --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > > > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > > > @@ -31,6 +31,26 @@ > > > > > > > > }; > > > > }; > > > > > > > > > + gpio-keys@0 { > > > > does the @0 give us a stable device name for userspace to open? > > No it doesn’t. This is my lack of DT skillz showing. I was looking > into how we can give userspace a stable device name. I was going > down the udev path but I can’t find any any of the information from > these DT nodes in the udev event. > > > > > Do we really want to go this way? We now have useful unique codes for > > the "key"s, why not use the one node? Or is your concern we've now tied > > Each gpio will have an application waiting for its state to change. My > concern was waking up x number of applications every time any gpio changes > state. Is that a valid concern? How many applications do we expect to be reading the evdev? What are our expected interrupt rates? What are the worst expected cases? It's hard to judge without knowing the numbers, but considering the chips we run on I agree we should generally favour performance if design is getting in the way. But to make that trade off we should be clear on the performance numbers. Andrew
> On Apr 20, 2017, at 12:34 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > On Wed, 2017-04-19 at 10:38 -0400, Brad Bishop wrote: >>>>> On Apr 19, 2017, at 10:29 AM, Andrew Jeffery <andrew@aj.id.au> wrote: >>> >>> Hi Brad, >>> >>> If you do future revisions of these patches, can you please Cc me? >> >> will do. >> >>> >>> On Wed, 2017-04-19 at 00:18 -0400, Brad Bishop wrote: >>>> Enable gpio-keys events for the checkstop and water/air cooled >>>> gpios for use by applications on the Witherspoon system. >>>> >>>>> Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com> >>>> >>>> --- >>>> arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++ >>>> 1 file changed, 20 insertions(+) >>>> >>>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >>>> index e3a7b77..aa1708e 100644 >>>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >>>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts >>>> @@ -31,6 +31,26 @@ >>>>>>>>> }; >>>>> }; >>>> >>>> >>>> + gpio-keys@0 { >>> >>> does the @0 give us a stable device name for userspace to open? >> >> No it doesn’t. This is my lack of DT skillz showing. I was looking >> into how we can give userspace a stable device name. I was going >> down the udev path but I can’t find any any of the information from >> these DT nodes in the udev event. >> >>> >>> Do we really want to go this way? We now have useful unique codes for >>> the "key"s, why not use the one node? Or is your concern we've now tied >> >> Each gpio will have an application waiting for its state to change. My >> concern was waking up x number of applications every time any gpio changes >> state. Is that a valid concern? > > How many applications do we expect to be reading the evdev? What are > our expected interrupt rates? What are the worst expected cases? For Witherspoon/Zaius or any OpenBMC platform? I obviously can’t say for the latter case. On a bigger/enterprise systems I can see on the order of 10 to 100 applications looking at these. I don’t have a good sense of how often. On Witherspoon/Zaius, sure, we only have a handful of applications and the interrupt rate should be very infrequent. I think you typically see all the gpio keys mapped to a single device because the usual thing to do is have a single application (Xorg) treat it like a keyboard. We aren’t structured that way so rethinking the usual approach seems reasonable. Another reason I went for per gpio devices because it prevents applications from reacting to each others events without any special library code. I’m not saying there aren’t disadvantages to this approach - I just don’t know what they are? > > It's hard to judge without knowing the numbers, but considering the > chips we run on I agree we should generally favour performance if > design is getting in the way. But to make that trade off we should be Again, what exactly is being traded-off ? > clear on the performance numbers. > > Andrew
On Thu, 2017-04-20 at 10:24 -0400, Brad Bishop wrote: > > > > On Apr 20, 2017, at 12:34 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > On Wed, 2017-04-19 at 10:38 -0400, Brad Bishop wrote: > > > > > > On Apr 19, 2017, at 10:29 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > Hi Brad, > > > > > > > > If you do future revisions of these patches, can you please Cc me? > > > > > > will do. > > > > > > > > > > > On Wed, 2017-04-19 at 00:18 -0400, Brad Bishop wrote: > > > > > Enable gpio-keys events for the checkstop and water/air cooled > > > > > gpios for use by applications on the Witherspoon system. > > > > > > > > > > > Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com> > > > > > > > > > > --- > > > > > arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++ > > > > > 1 file changed, 20 insertions(+) > > > > > > > > > > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > > > > > index e3a7b77..aa1708e 100644 > > > > > --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > > > > > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts > > > > > @@ -31,6 +31,26 @@ > > > > > > > > > > }; > > > > > > > > > > > > }; > > > > > > > > > > > > > > > + gpio-keys@0 { > > > > > > > > does the @0 give us a stable device name for userspace to open? > > > > > > No it doesn’t. This is my lack of DT skillz showing. I was looking > > > into how we can give userspace a stable device name. I was going > > > down the udev path but I can’t find any any of the information from > > > these DT nodes in the udev event. > > > > > > > > > > > Do we really want to go this way? We now have useful unique codes for > > > > the "key"s, why not use the one node? Or is your concern we've now tied > > > > > > Each gpio will have an application waiting for its state to change. My > > > concern was waking up x number of applications every time any gpio changes > > > state. Is that a valid concern? > > > > How many applications do we expect to be reading the evdev? What are > > our expected interrupt rates? What are the worst expected cases? > > For Witherspoon/Zaius or any OpenBMC platform? Yes, this is what I was asking about, given they are concrete systems. > I obviously can’t say for the > latter case. Yep. > On a bigger/enterprise systems I can see on the order of 10 to 100 > applications looking at these. Okay; having 10-100 processes woken is something we'd want to avoid. > I don’t have a good sense of how often. > > On Witherspoon/Zaius, sure, we only have a handful of applications and the > interrupt rate should be very infrequent. That was my thought, which was why I wanted to check. While it might be low, if we're waking up to 100 applications it's not ideal. > > I think you typically see all the gpio keys mapped to a single device because > the usual thing to do is have a single application (Xorg) treat it like a keyboard. > > We aren’t structured that way so rethinking the usual approach seems reasonable. Sure; I was never against rethinking it. I just wanted a clear explanation of why we would choose this route. > > Another reason I went for per gpio devices because it prevents applications from > reacting to each others events without any special library code. > > I’m not saying there aren’t disadvantages to this approach - I just don’t know what > they are? It's a bit strange from a design perspective as we now have two unique identifiers for an event; the input device *and* the keycode emitted by the event on the input device, which is the *only* keycode emitted by the device. For someone new to the system I expect this would seem a strange redundancy if they weren't aware of the performance implications inherent to the distributed nature of the application design. > > > > > It's hard to judge without knowing the numbers, but considering the > > chips we run on I agree we should generally favour performance if > > design is getting in the way. But to make that trade off we should be > > Again, what exactly is being traded-off ? The "oddity" in the design with respect to the "redundancy" I described above. Anyway, I think what you have outlined is reasonable. Cheers, Andrew
Hi Brad, On Thu, Apr 20, 2017 at 11:54 PM, Brad Bishop <bradleyb@fuzziesquirrel.com> wrote: >>> Each gpio will have an application waiting for its state to change. My >>> concern was waking up x number of applications every time any gpio changes >>> state. Is that a valid concern? >> >> How many applications do we expect to be reading the evdev? What are >> our expected interrupt rates? What are the worst expected cases? > > For Witherspoon/Zaius or any OpenBMC platform? I obviously can’t say for the > latter case. On a bigger/enterprise systems I can see on the order of 10 to 100 > applications looking at these. I don’t have a good sense of how often. > > On Witherspoon/Zaius, sure, we only have a handful of applications and the > interrupt rate should be very infrequent. > > I think you typically see all the gpio keys mapped to a single device because > the usual thing to do is have a single application (Xorg) treat it like a keyboard. I've been following the discussion you've been having. I went to merge the patches today, but first decided to satisfy myself that this was the right way to go. I didn't arrive at the same conclusion as you and Andrew. The userspace use cases you've presented would be served well by a single node. We have the flexibility to revisit this decision when someone builds a machine that has 100 GPIOs they want to monitor; in addition I think we would revisit the decision to have 100 userspace programs. By having separate nodes, you're creating a new instance of the kernel driver for each node. The driver, and the binding, is designed to support your use case - multiple key events generated by the hardware. Finally, I looked at the other machines in the tree. None of them have separate gpio-keys nodes arch/arm/boot/dts/armada-xp-netgear-rn2120.dts: gpio-keys { compatible = "gpio-keys"; pinctrl-0 = <&power_button_pin &reset_button_pin>; pinctrl-names = "default"; power-button { label = "Power Button"; linux,code = <KEY_POWER>; gpios = <&gpio0 27 GPIO_ACTIVE_HIGH>; }; reset-button { label = "Reset Button"; linux,code = <KEY_RESTART>; gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; }; }; arch/arm/boot/dts/at91sam9261ek.dts gpio_keys { compatible = "gpio-keys"; button_0 { label = "button_0"; gpios = <&pioA 27 GPIO_ACTIVE_LOW>; linux,code = <256>; wakeup-source; }; button_1 { label = "button_1"; gpios = <&pioA 26 GPIO_ACTIVE_LOW>; linux,code = <257>; wakeup-source; }; button_2 { label = "button_2"; gpios = <&pioA 25 GPIO_ACTIVE_LOW>; linux,code = <258>; wakeup-source; }; button_3 { label = "button_3"; gpios = <&pioA 24 GPIO_ACTIVE_LOW>; linux,code = <259>; wakeup-source; }; }; Cheers, Joel > > We aren’t structured that way so rethinking the usual approach seems reasonable. > Another reason I went for per gpio devices because it prevents applications from > reacting to each others events without any special library code. > > I’m not saying there aren’t disadvantages to this approach - I just don’t know what > they are? > >> >> It's hard to judge without knowing the numbers, but considering the >> chips we run on I agree we should generally favour performance if >> design is getting in the way. But to make that trade off we should be > > Again, what exactly is being traded-off ? > >> clear on the performance numbers. >> >> Andrew
> On Apr 20, 2017, at 10:14 PM, Joel Stanley <joel@jms.id.au> wrote: > > Hi Brad, > > On Thu, Apr 20, 2017 at 11:54 PM, Brad Bishop > <bradleyb@fuzziesquirrel.com> wrote: >>>> Each gpio will have an application waiting for its state to change. My >>>> concern was waking up x number of applications every time any gpio changes >>>> state. Is that a valid concern? >>> >>> How many applications do we expect to be reading the evdev? What are >>> our expected interrupt rates? What are the worst expected cases? >> >> For Witherspoon/Zaius or any OpenBMC platform? I obviously can’t say for the >> latter case. On a bigger/enterprise systems I can see on the order of 10 to 100 >> applications looking at these. I don’t have a good sense of how often. >> >> On Witherspoon/Zaius, sure, we only have a handful of applications and the >> interrupt rate should be very infrequent. >> >> I think you typically see all the gpio keys mapped to a single device because >> the usual thing to do is have a single application (Xorg) treat it like a keyboard. > > I've been following the discussion you've been having. I went to merge > the patches today, but first decided to satisfy myself that this was > the right way to go. I didn't arrive at the same conclusion as you and > Andrew. > > The userspace use cases you've presented would be served well by a > single node. We have the flexibility to revisit this decision when > someone builds a machine that has 100 GPIOs they want to monitor; in > addition I think we would revisit the decision to have 100 userspace > programs. > > By having separate nodes, you're creating a new instance of the kernel > driver for each node. > > The driver, and the binding, is designed to support your use case - > multiple key events generated by the hardware. Honestly I’m OK with either approach. I think we will have userspace configurable enough that switching back and forth would be pretty trivial, if this would need to be revisited. v5 en-route. > > Finally, I looked at the other machines in the tree. None of them have > separate gpio-keys nodes Yeah I noticed this too. I chalked it up to my previously mentioned user-is-probably-Xorg logic, but standard practices are not usually a hard sell for me…. > > arch/arm/boot/dts/armada-xp-netgear-rn2120.dts: > > gpio-keys { > compatible = "gpio-keys"; > pinctrl-0 = <&power_button_pin &reset_button_pin>; > pinctrl-names = "default"; > > power-button { > label = "Power Button"; > linux,code = <KEY_POWER>; > gpios = <&gpio0 27 GPIO_ACTIVE_HIGH>; > }; > > reset-button { > label = "Reset Button"; > linux,code = <KEY_RESTART>; > gpios = <&gpio1 9 GPIO_ACTIVE_LOW>; > }; > }; > > arch/arm/boot/dts/at91sam9261ek.dts > gpio_keys { > compatible = "gpio-keys"; > > button_0 { > label = "button_0"; > gpios = <&pioA 27 GPIO_ACTIVE_LOW>; > linux,code = <256>; > wakeup-source; > }; > > button_1 { > label = "button_1"; > gpios = <&pioA 26 GPIO_ACTIVE_LOW>; > linux,code = <257>; > wakeup-source; > }; > > button_2 { > label = "button_2"; > gpios = <&pioA 25 GPIO_ACTIVE_LOW>; > linux,code = <258>; > wakeup-source; > }; > > button_3 { > label = "button_3"; > gpios = <&pioA 24 GPIO_ACTIVE_LOW>; > linux,code = <259>; > wakeup-source; > }; > }; > > Cheers, > > Joel > > > >> >> We aren’t structured that way so rethinking the usual approach seems reasonable. >> Another reason I went for per gpio devices because it prevents applications from >> reacting to each others events without any special library code. >> >> I’m not saying there aren’t disadvantages to this approach - I just don’t know what >> they are? >> >>> >>> It's hard to judge without knowing the numbers, but considering the >>> chips we run on I agree we should generally favour performance if >>> design is getting in the way. But to make that trade off we should be >> >> Again, what exactly is being traded-off ? >> >>> clear on the performance numbers. >>> >>> Andrew
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts index e3a7b77..aa1708e 100644 --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts @@ -31,6 +31,26 @@ }; }; + gpio-keys@0 { + compatible = "gpio-keys"; + + air-water { + label = "air-water"; + gpios = <&gpio ASPEED_GPIO(B, 5) GPIO_ACTIVE_LOW>; + linux,code = <ASPEED_GPIO(B, 5)>; + }; + }; + + gpio-keys@1 { + compatible = "gpio-keys"; + + checkstop { + label = "checkstop"; + gpios = <&gpio ASPEED_GPIO(J, 2) GPIO_ACTIVE_LOW>; + linux,code = <ASPEED_GPIO(J, 2)>; + }; + }; + leds { compatible = "gpio-leds";
Enable gpio-keys events for the checkstop and water/air cooled gpios for use by applications on the Witherspoon system. Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com> --- arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)