Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute

[Alexandre Courbot <gnurou@xxxxxxxxx>]

On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann
<soren.brinkmann@xxxxxxxxxx> wrote:
> On Fri, 2015-01-16 at 12:11PM +0100, Johan Hovold wrote:
>> On Thu, Jan 15, 2015 at 11:49:49AM -0800, Soren Brinkmann wrote:
>> > Add an attribute 'wakeup' to the GPIO sysfs interface which allows
>> > marking/unmarking a GPIO as wake IRQ.
>> > The file 'wakeup' is created in each exported GPIOs directory, if an IRQ
>> > is associated with that GPIO and the irqchip implements set_wake().
>> > Writing 'enabled' to that file will enable wake for that GPIO, while
>> > writing 'disabled' will disable wake.
>> > Reading that file will return either 'disabled' or 'enabled' depening on
>> > the currently set flag for the GPIO's IRQ.
>> >
>> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xxxxxxxxxx>
>> > Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> > ---
>> > Hi Linus, Johan,
>> >
>> > I rebased my patch. And things look good.
>>
>> I took at closer look at this patch now and I really don't think it
>> should be merged at all.
>>
>> We have a mechanism for handling wake-up sources (documented in
>> Documentation/power/devices.txt) as well as an ABI to enable/disable
>> them using the power/wakeup device attribute from userspace.
>
> Doesn't work for GPIOs AFAIK.
>
>>
>> Implementing proper wakeup support for unclaimed GPIOs would take some
>> work (if at all desired), but that is not a reason to be adding custom
>> implementations that violates the kernel's power policies and new ABIs
>> that would need to be maintained forever.
>
> These are claimed, by the sysfs interface.
>
>>
>> [ And we really shouldn't be adding anything to the broken gpio sysfs
>> interface until it's been redesigned. ]
>>
>> Meanwhile you can (should) use gpio-keys if you need to wake your system
>> on gpio events.
>
> We had that discussion and I don't think GPIO keys is the right solution
> for every use-case.

Sorry, it has been a while - can you remind us of why?

>>
>> > But the 'is_visible' things does not behave the way I expected it to.
>> > It seems to be only triggered on an export but not when attributes
>> > change. Hence, in my case, everything was visiible since the inital
>> > state matches that, but even when changing the direction or things
>> > like that, attributes don't disappear. Is that something still worked
>> > on? Expected
>>
>> That's expected. We generally don't want attributes to appear or
>> disappear after the device has been registered (although there is a
>> mechanism for cases were it makes sense). This is no different from
>> how your v3 patch worked either.
>
> Sure, but the is_visible thing is effectively broken for GPIO. I think a
> GPIO is in a defined state when exported and the checks all work on that
> state during export. But then this state can be changed through the
> sysfs interface. So, if the initial state hides something it becomes
> unavailable for all times and, vice versa, if the initial state makes
> something visible, it will stay even when it is no longer a valid
> property to change.

Is there anything that prevents you from patching other GPIO sysfs
hooks to remove/reenable the wakeup property as needed?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html