[PATCH linux dev-4.7 v3 0/2] enable gpio-keys

[PATCH linux dev-4.7 v3 0/2] enable gpio-keys

[Brad Bishop]

Enable the gpio-keys and evdev drivers on ast24xx generally.

Bind gpio-key events to the checkstop and air-water cooled gpios on the
Witherspoon system.

v3:
    Drop phandle and flags from linux,code.
    Assign gpios to their own evdev.
v2:
    Use gpio numbers for keycodes.

Brad Bishop (2):
  ARM: configs: aspeed: Enable keyboard-gpio
  ARM: dts: Enable checkstop and cooling gpio keys

 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 20 ++++++++++++++++++++
 arch/arm/configs/aspeed_g4_defconfig             |  8 +++++++-
 arch/arm/configs/aspeed_g5_defconfig             |  8 +++++++-
 3 files changed, 34 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH linux dev-4.7 v3 1/2] ARM: configs: aspeed: Enable keyboard-gpio

[Brad Bishop]

Some OpenBMC systems will be using keyboard-gpio so enable it
in our configs.

Signed-off-by: Brad Bishop <bradleyb@fuzziesquirrel.com>
---
 arch/arm/configs/aspeed_g4_defconfig | 8 +++++++-
 arch/arm/configs/aspeed_g5_defconfig | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/arm/configs/aspeed_g4_defconfig b/arch/arm/configs/aspeed_g4_defconfig
index 552297b..4350f67 100644
--- a/arch/arm/configs/aspeed_g4_defconfig
+++ b/arch/arm/configs/aspeed_g4_defconfig
@@ -103,7 +103,12 @@ CONFIG_FTGMAC100=y
 CONFIG_BROADCOM_PHY=y
 CONFIG_REALTEK_PHY=y
 # CONFIG_WLAN is not set
-# CONFIG_INPUT is not set
+# CONFIG_INPUT_LEDS is not set
+# CONFIG_INPUT_MOUSEDEV is not set
+CONFIG_INPUT_EVDEV=y
+# CONFIG_KEYBOARD_ATKBD is not set
+CONFIG_KEYBOARD_GPIO=y
+# CONFIG_INPUT_MOUSE is not set
 # CONFIG_SERIO is not set
 # CONFIG_VT is not set
 # CONFIG_LEGACY_PTYS is not set
@@ -138,6 +143,7 @@ CONFIG_SENSORS_ADM1275=y
 CONFIG_SENSORS_LM25066=y
 CONFIG_SENSORS_UCD9000=y
 CONFIG_SENSORS_TMP421=y
+# CONFIG_HID is not set
 # CONFIG_USB_SUPPORT is not set
 CONFIG_NEW_LEDS=y
 CONFIG_LEDS_CLASS=y
diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
index c5e7295..b6b3ca0 100644
--- a/arch/arm/configs/aspeed_g5_defconfig
+++ b/arch/arm/configs/aspeed_g5_defconfig
@@ -102,7 +102,12 @@ CONFIG_FTGMAC100=y
 CONFIG_BROADCOM_PHY=y
 CONFIG_REALTEK_PHY=y
 # CONFIG_WLAN is not set
-# CONFIG_INPUT is not set
+# CONFIG_INPUT_LEDS is not set
+# CONFIG_INPUT_MOUSEDEV is not set
+CONFIG_INPUT_EVDEV=y
+# CONFIG_KEYBOARD_ATKBD is not set
+CONFIG_KEYBOARD_GPIO=y
+# CONFIG_INPUT_MOUSE is not set
 # CONFIG_SERIO is not set
 # CONFIG_VT is not set
 # CONFIG_LEGACY_PTYS is not set
@@ -135,6 +140,7 @@ CONFIG_PMBUS=y
 CONFIG_SENSORS_LM25066=y
 CONFIG_SENSORS_UCD9000=y
 CONFIG_SENSORS_TMP421=y
+# CONFIG_HID is not set
 # CONFIG_USB_SUPPORT is not set
 CONFIG_NEW_LEDS=y
 CONFIG_LEDS_CLASS=y
-- 
1.8.3.1

[PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Brad Bishop]

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 {
+		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";
 
-- 
1.8.3.1

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

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";
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Brad Bishop]

> 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";

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Brad Bishop]

> 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

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4456 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Joel Stanley]

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

Re: [PATCH linux dev-4.7 v3 2/2] ARM: dts: Enable checkstop and cooling gpio keys

[Brad Bishop]

> 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