[PATCH linux dev-4.10 v6 1/2] ARM: dts: aspeed: Witherspoon add gpio-keys-polled for fan presence

[Christopher Bostic]

On 8/2/17 3:20 AM, Cédric Le Goater wrote:
> On 08/02/2017 09:10 AM, Cédric Le Goater wrote:
>> On 08/02/2017 04:33 AM, Christopher Bostic wrote:
>>>
>>> On 7/31/17 2:48 AM, Cédric Le Goater wrote:
>>>> On 07/29/2017 08:36 PM, Christopher Bostic wrote:
>>>>> Add gpio-keys-polled  node for pca955x fan presence lines.
>>>>> Add GPIO details to pca955x for the presence lines as well.
>>>>>
>>>>> Polling period of 1 second determined to be the max acceptable
>>>>> value based on discussions with Brad Bishop and Matt Spinler.
>>>>> Tested with gpiomon program - verified.
>>>>>
>>>>> Signed-off-by: Christopher Bostic <cbostic at linux.vnet.ibm.com>
>>>>> ---
>>>>> v6 - Add gpio pin details for fan presence to pca955x node
>>>>> v5 - Fix gpio-keys-polled driver probe errors.  Missing pca955x
>>>>>        gpio cells property and wrong format for poll-interval.
>>>>> v4 - Add details regarding polling period to commit message.
>>>>> v3 - Remove 'autorepeat'
>>>>>      - Increase polling period to 1 second.
>>>>>      - Remove GPIO specifiers from pca955x in dev tree
>>>>> v2 - Change 'linux,code' from a unique global value to a local one
>>>>>        for all the gpio-keys-polled devices.
>>>>> ---
>>>>>    arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 52 ++++++++++++++++++++++++
>>>>>    1 file changed, 52 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
>>>>> index 5afc03a..70328668 100644
>>>>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
>>>>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
>>>>> @@ -56,6 +56,37 @@
>>>>>            };
>>>>>        };
>>>>>    +    gpio-keys-polled {
>>>>> +        compatible = "gpio-keys-polled";
>>>>> +        #address-cells = <1>;
>>>>> +        #size-cells = <0>;
>>>>> +        poll-interval = <1000>;
>>>>> +
>>>>> +        fan0-presence {
>>>>> +            label = "fan0-presence";
>>>>> +            gpios = <&pca0 4 GPIO_ACTIVE_LOW>;
>>>>> +            linux,code = <4>;
>>>>> +        };
>>>>> +
>>>>> +        fan1-presence {
>>>>> +            label = "fan1-presence";
>>>>> +            gpios = <&pca0 5 GPIO_ACTIVE_LOW>;
>>>>> +            linux,code = <5>;
>>>>> +        };
>>>>> +
>>>>> +        fan2-presence {
>>>>> +            label = "fan2-presence";
>>>>> +            gpios = <&pca0 6 GPIO_ACTIVE_LOW>;
>>>>> +            linux,code = <6>;
>>>>> +        };
>>>>> +
>>>>> +        fan3-presence {
>>>>> +            label = "fan3-presence";
>>>>> +            gpios = <&pca0 7 GPIO_ACTIVE_LOW>;
>>>>> +            linux,code = <7>;
>>>>> +        };
>>>>> +    };
>>>>> +
>>>>>        leds {
>>>>>            compatible = "gpio-leds";
>>>>>    @@ -277,6 +308,7 @@
>>>>>            reg = <0x60>;
>>>>>            #address-cells = <1>;
>>>>>            #size-cells = <0>;
>>>>> +        #gpio-cells = <2>;
>>>> you should also add a :
>>>>
>>>>          gpio-controller;
>>>>
>>>> if you want to use the lines as GPIOs
>>>>
>>>> C.
>>> Hi Cedric,
>>>
>>> If I add 'gpio-controller' here are you indicating that would not
>>> require any modifications I made to the leds-pca955x.c file?
>> yes.
>>
>>> I've tried that and found I still need that leds-pca955x.c change
>>> even if I add 'gpio-controller' attribute to get gpio-keys-polled
>>>   device driver to properly probe the pca955x lines.
>> OK. I will give it a try on a QEMU. Maybe there are some incompatibilities
>> with the gpio-keys driver bindings
> It fails indeed, but the fix you are proposing does not look right
> either.
>
> Maybe we should fully define the pca9552 as a gpio controller and
> then define a gpio-leds and a gpio-keys-polled on top of it ?
Hi Cedric,

What would be entailed in fully defining the pca9552 as a gpio 
controller?   Any other driver examples would help if you know of any.

Currently I see that it has a struct gpio_chip embedded in its 955x chip 
structure and that it defines its various properties, .direction_input, 
.direction_output, etc...   The struct gpio_chip ngpio field is set to a 
subset of pins that are not labeled as LEDS.  Hence my added change to 
count LEDS towards total ngpio's. Given that the pins I'm interested in 
are 4-7 this falls outside the bounds of what the gpio chip considers 
total gpio count.   As a result gpio-keys-polled driver probe fails.

Maybe a quick conference call might be useful?

Thanks,
Chris


> Thanks,
>
> C.
>
>
>> C.
>>
>>> Thanks
>>> Chris
>>>
>>>>>              fan0: led at 0 {
>>>>>                label = "fan0";
>>>>> @@ -302,6 +334,26 @@
>>>>>                reg = <3>;
>>>>>                type = <PCA955X_TYPE_LED>;
>>>>>            };
>>>>> +        fan0_pres_gpio: gpio at 4 {
>>>>> +            label = "fan0_presence";
>>>>> +            reg = <4>;
>>>>> +            type = <PCA955X_TYPE_GPIO>;
>>>>> +        };
>>>>> +        fan1_pres_gpio: gpio at 5 {
>>>>> +            label = "fan1_presence";
>>>>> +            reg = <5>;
>>>>> +            type = <PCA955X_TYPE_GPIO>;
>>>>> +        };
>>>>> +        fan2_pres_gpio: gpio at 6 {
>>>>> +            label = "fan2_presence";
>>>>> +            reg = <6>;
>>>>> +            type = <PCA955X_TYPE_GPIO>;
>>>>> +        };
>>>>> +        fan3_pres_gpio: gpio at 7 {
>>>>> +            label = "fan3_presence";
>>>>> +            reg = <7>;
>>>>> +            type = <PCA955X_TYPE_GPIO>;
>>>>> +        };
>>>>>            front_fault: led at 13 {
>>>>>                label = "front-fault";
>>>>>                default-state = "keep";
>>>>>