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

[Brad Bishop <bradleyb@fuzziesquirrel.com>]

> 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