lkml.org 
[lkml]   [2014]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH RESEND] gpio: lib-sysfs: Add 'wakeup' attribute
On Tue, Oct 14, 2014 at 8:47 AM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> Hi Alexandre,
>
> On Sat, 2014-10-11 at 01:54PM +0900, Alexandre Courbot wrote:
>> On Fri, Sep 5, 2014 at 2:58 AM, Soren Brinkmann
>> <soren.brinkmann@xilinx.com> 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@xilinx.com>
>> > ---
>> > Hi all,
>> >
>> > I originally submitted this patch with a few fixes for Zynq's GPIO driver
>> > (https://lkml.org/lkml/2014/8/29/391). Since this change is not just
>> > Zynq-related and has broader impact, Linus asked me to post this again, separate
>> > from the Zynq series.
>> >
>> > Let me just quote myself from the original submission:
>> > "I'm still not fully convinced that the gpio_keys are the best
>> > replacement for the sysfs interface when it comes to inputs. For that
>> > reason and to have a way to do some quick wake testing, I'd like to
>> > propose adding the ability to control wake through the sysfs interface
>> > (patch 3)."
>>
>> I'm really sorry that I did not provide feedback sooner. This is the
>> kind of area (IRQ) where I am not too confident and typically like to
>> hear what Linus has to say first. But I also have a few questions that
>> you could maybe answer for my own education. :)
>>
>> >
>> > Thanks,
>> > Sören
>> >
>> > drivers/gpio/gpiolib-sysfs.c | 75 ++++++++++++++++++++++++++++++++++++++++----
>> > 1 file changed, 69 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> > index 5f2150b619a7..aaf021eaaff5 100644
>> > --- a/drivers/gpio/gpiolib-sysfs.c
>> > +++ b/drivers/gpio/gpiolib-sysfs.c
>> > @@ -286,6 +286,56 @@ found:
>> >
>> > static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
>> >
>> > +static ssize_t gpio_wakeup_show(struct device *dev,
>> > + struct device_attribute *attr, char *buf)
>> > +{
>> > + ssize_t status;
>> > + const struct gpio_desc *desc = dev_get_drvdata(dev);
>> > + int irq = gpiod_to_irq(desc);
>> > + struct irq_desc *irq_desc = irq_to_desc(irq);
>> > +
>> > + mutex_lock(&sysfs_lock);
>> > +
>> > + if (irqd_is_wakeup_set(&irq_desc->irq_data))
>> > + status = sprintf(buf, "enabled\n");
>> > + else
>> > + status = sprintf(buf, "disabled\n");
>> > +
>> > + mutex_unlock(&sysfs_lock);
>> > +
>> > + return status;
>> > +}
>> > +
>> > +static ssize_t gpio_wakeup_store(struct device *dev,
>> > + struct device_attribute *attr, const char *buf, size_t size)
>> > +{
>> > + int ret;
>> > + unsigned int on;
>> > + struct gpio_desc *desc = dev_get_drvdata(dev);
>> > + int irq = gpiod_to_irq(desc);
>> > +
>> > + mutex_lock(&sysfs_lock);
>> > +
>> > + if (sysfs_streq("enabled", buf))
>> > + on = true;
>> > + else if (sysfs_streq("disabled", buf))
>> > + on = false;
>> > + else
>> > + return -EINVAL;
>>
>> You forgot to release sysfs_lock before returning here.
>
> Right, I will fix this and send a v2. Good catch.
>
>>
>> > +
>> > + ret = irq_set_irq_wake(irq, on);
>>
>> Just wondering: is it always safe to set the wake property of an IRQ
>> even if the direction of its associated GPIO is output? Does it make
>> sense at all to have the "wakeup" attribute file visible if the GPIO
>> is an output one?
>>
>> > +
>> > + mutex_unlock(&sysfs_lock);
>> > +
>> > + if (ret)
>> > + pr_warn("%s: failed to %s wake\n", __func__,
>> > + on ? "enable" : "disable");
>> > +
>> > + return size;
>> > +}
>> > +
>> > +static DEVICE_ATTR(wakeup, 0644, gpio_wakeup_show, gpio_wakeup_store);
>> > +
>> > static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
>> > int value)
>> > {
>> > @@ -526,7 +576,7 @@ static struct class gpio_class = {
>> > int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> > {
>> > unsigned long flags;
>> > - int status;
>> > + int status, irq;
>> > const char *ioname = NULL;
>> > struct device *dev;
>> > int offset;
>> > @@ -582,11 +632,24 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>> > goto fail_unregister_device;
>> > }
>> >
>> > - if (gpiod_to_irq(desc) >= 0 && (direction_may_change ||
>> > - !test_bit(FLAG_IS_OUT, &desc->flags))) {
>> > - status = device_create_file(dev, &dev_attr_edge);
>> > - if (status)
>> > - goto fail_unregister_device;
>> > + irq = gpiod_to_irq(desc);
>> > + if (irq >= 0) {
>> > + struct irq_desc *irq_desc = irq_to_desc(irq);
>> > + struct irq_chip *irqchip = irq_desc_get_chip(irq_desc);
>> > +
>> > + if (direction_may_change ||
>> > + !test_bit(FLAG_IS_OUT, &desc->flags)) {
>> > + status = device_create_file(dev, &dev_attr_edge);
>> > + if (status)
>> > + goto fail_unregister_device;
>> > + }
>> > +
>> > + if (irqchip->flags & IRQCHIP_SKIP_SET_WAKE ||
>> > + irqchip->irq_set_wake) {
>> > + status = device_create_file(dev, &dev_attr_wakeup);
>> > + if (status)
>> > + goto fail_unregister_device;
>> > + }
>>
>> Ok, I guess that answers my question. The edge property is always
>> visible so it probably doesn't hurt if the wakeup property also is.
>
> Right, it is probably not optimal, but serves the purpose. Users still
> might have to turn on their brain. But in general, setting those
> properties should be safe, IMHO.
> I think being able to mess with GPIO's direction and value is a much
> bigger risk since they might be used for something more critical than
> just LEDs :)
> So, this does not really add a lot of more potential to break things :)

Good point.

>
>>
>> The only thing that bothers me a little bit is that both "edge" and
>> "wakeup" are not very explicit names, but I guess it can't be helped.
>
> I guess edge is ABI and that ship has sailed. For 'wakeup' we can still
> discuss the name. But wakeup is the standard property that is exposed in
> sysfs for other devices that support wake and choose to expose
> user-space control for it. So, I thought 'wakeup' would be appropriate
> here as well.

... and another good point.

Let's see what Linus thinks about this, but as far as I'm concerned
your v2 looks ok.

Cheers,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2014-10-14 08:42    [W:0.060 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site