linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
@ 2015-01-15 19:49 Soren Brinkmann
  2015-01-16 11:11 ` Johan Hovold
  0 siblings, 1 reply; 10+ messages in thread
From: Soren Brinkmann @ 2015-01-15 19:49 UTC (permalink / raw)
  To: Linus Walleij, Johan Hovold
  Cc: Alexandre Courbot, linux-api, linux-kernel, linux-gpio,
	linux-doc, Soren Brinkmann

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>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
---
Hi Linus, Johan,

I rebased my patch. And things look good. 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
behavior?

	Thanks,
	Sören

v4:
 - rebased onto gpio/fixes
   - fit into the new attribute framework
   - extend is_visible to limit the wakeup attributes visibility
     according to usability
v3:
 - add documentation
v2:
 - fix error path to unlock mutex before return
---
 Documentation/ABI/testing/sysfs-gpio |  1 +
 Documentation/gpio/sysfs.txt         |  8 +++++
 drivers/gpio/gpiolib-sysfs.c         | 62 ++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-gpio b/Documentation/ABI/testing/sysfs-gpio
index 80f4c94c7bef..4cc7f4b3f724 100644
--- a/Documentation/ABI/testing/sysfs-gpio
+++ b/Documentation/ABI/testing/sysfs-gpio
@@ -20,6 +20,7 @@ Description:
 	    /value ... always readable, writes fail for input GPIOs
 	    /direction ... r/w as: in, out (default low); write: high, low
 	    /edge ... r/w as: none, falling, rising, both
+	    /wakeup ... r/w as: enabled, disabled
 	/gpiochipN ... for each gpiochip; #N is its first GPIO
 	    /base ... (r/o) same as N
 	    /label ... (r/o) descriptive, not necessarily unique
diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index c2c3a97f8ff7..f703377d528f 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -97,6 +97,14 @@ and have the following read/write attributes:
 		for "rising" and "falling" edges will follow this
 		setting.
 
+	"wakeup" ... reads as either "enabled" or "disabled". Write these
+		strings to set/clear the 'wakeup' flag of the IRQ associated
+		with this GPIO. If the IRQ has the 'wakeup' flag set, it can
+		wake the system from sleep states.
+
+		This file exists only if the pin can generate interrupts and
+		the driver implements the required infrastructure.
+
 GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
 controller implementing GPIOs starting at #42) and have the following
 read-only attributes:
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index f62aa115d79a..14b34d15e61c 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -286,6 +286,58 @@ 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 {
+		mutex_unlock(&sysfs_lock);
+		return -EINVAL;
+	}
+
+	ret = irq_set_irq_wake(irq, on);
+
+	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)
 {
@@ -361,6 +413,7 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
 	struct gpio_desc *desc = dev_get_drvdata(dev);
+	struct irq_chip *irqchip = desc->chip->irqchip;
 	umode_t mode = attr->mode;
 	bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
 
@@ -372,6 +425,14 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 			mode = 0;
 		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
 			mode = 0;
+	} else if (attr == &dev_attr_wakeup.attr) {
+		if (gpiod_to_irq(desc) < 0)
+			mode = 0;
+		if (!show_direction && test_bit(FLAG_IS_OUT, &desc->flags))
+			mode = 0;
+		if (!test_bit(IRQCHIP_SKIP_SET_WAKE, &irqchip->flags) &&
+				!irqchip->irq_set_wake)
+			mode = 0;
 	}
 
 	return mode;
@@ -382,6 +443,7 @@ static struct attribute *gpio_attrs[] = {
 	&dev_attr_edge.attr,
 	&dev_attr_value.attr,
 	&dev_attr_active_low.attr,
+	&dev_attr_wakeup.attr,
 	NULL,
 };
 
-- 
2.2.1.1.gb42cc81


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-15 19:49 [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute Soren Brinkmann
@ 2015-01-16 11:11 ` Johan Hovold
  2015-01-16 16:49   ` Sören Brinkmann
  0 siblings, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2015-01-16 11:11 UTC (permalink / raw)
  To: Soren Brinkmann
  Cc: Linus Walleij, Johan Hovold, Alexandre Courbot, linux-api,
	linux-kernel, linux-gpio, linux-doc

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@xilinx.com>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> 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.

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.

[ 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.

> 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.

Johan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-16 11:11 ` Johan Hovold
@ 2015-01-16 16:49   ` Sören Brinkmann
  2015-01-19  4:20     ` Alexandre Courbot
  2015-01-19 10:10     ` Johan Hovold
  0 siblings, 2 replies; 10+ messages in thread
From: Sören Brinkmann @ 2015-01-16 16:49 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Linus Walleij, Alexandre Courbot, linux-api, linux-kernel,
	linux-gpio, linux-doc

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@xilinx.com>
> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> > ---
> > 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.

> 
> > 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.

	Sören

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-16 16:49   ` Sören Brinkmann
@ 2015-01-19  4:20     ` Alexandre Courbot
  2015-01-19  8:54       ` Linus Walleij
  2015-01-19 10:10     ` Johan Hovold
  1 sibling, 1 reply; 10+ messages in thread
From: Alexandre Courbot @ 2015-01-19  4:20 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Johan Hovold, Linus Walleij, linux-api,
	Linux Kernel Mailing List, linux-gpio, linux-doc

On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann
<soren.brinkmann@xilinx.com> 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@xilinx.com>
>> > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>> > ---
>> > 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?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-19  4:20     ` Alexandre Courbot
@ 2015-01-19  8:54       ` Linus Walleij
  2015-01-29 17:23         ` Sören Brinkmann
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-01-19  8:54 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Sören Brinkmann, Johan Hovold, linux-api,
	Linux Kernel Mailing List, linux-gpio, linux-doc

On Mon, Jan 19, 2015 at 5:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
>> On Fri, 2015-01-16 at 12:11PM +0100, Johan Hovold wrote:

>>> 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.
(...)
>>> 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?

There are such cases. Of course keys should be handled by GPIO-keys
and these will trigger the right wakeup events in such cases.

This is for more esoteric cases: we cannot have a kernel module for
everything people want to do with GPIOs, and the use case I accept
is GPIOs used in automatic control etc, think factory lines or doors.
We can't have a "door" driver or "punch arm" or "fire alarm" driver
in the kernel. Those are userspace things.

Still such embedded systems need to be able to go to idle and
sleep to conerve power, and then they need to put wakeups on
these GPIOs.

So it is a feature userspace needs, though as with much of the
sysfs ABI it is very often abused for things like keys and LEDs which
is an abomination but we can't do much about it :(

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-16 16:49   ` Sören Brinkmann
  2015-01-19  4:20     ` Alexandre Courbot
@ 2015-01-19 10:10     ` Johan Hovold
  1 sibling, 0 replies; 10+ messages in thread
From: Johan Hovold @ 2015-01-19 10:10 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Johan Hovold, Linus Walleij, Alexandre Courbot, linux-api,
	linux-kernel, linux-gpio, linux-doc

On Fri, Jan 16, 2015 at 08:49:17AM -0800, Sören Brinkmann 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@xilinx.com>
> > > Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> > > ---
> > > 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.

Not today no, that's why I said it would take some work.

> > 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.

Unclaimed by a proper device and driver in the driver model.

> > [ 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.

I can see that, but this still needs to be implemented properly and not
just as a quick hack on top of the already fragile gpio sysfs-interface.

Since pretty much everyone agrees that the current interface needs to be
replaced, we really shouldn't be adding more stuff to the broken
interface before that happens.

> > > 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.

Again, this is exactly how the interface has always worked, and that's
exactly how your v3, which added the attributes manually, also worked.

The group-visibility mechanism is not broken. What's broken is interface
designs based on attributes magically disappearing and reappearing after
the device has been created.

Johan

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-19  8:54       ` Linus Walleij
@ 2015-01-29 17:23         ` Sören Brinkmann
  2015-02-04  9:19           ` Linus Walleij
  0 siblings, 1 reply; 10+ messages in thread
From: Sören Brinkmann @ 2015-01-29 17:23 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, Johan Hovold, linux-api,
	Linux Kernel Mailing List, linux-gpio, linux-doc

Hi Linus,

On Mon, 2015-01-19 at 09:54AM +0100, Linus Walleij wrote:
> On Mon, Jan 19, 2015 at 5:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> > On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> >> On Fri, 2015-01-16 at 12:11PM +0100, Johan Hovold wrote:
> 
> >>> 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.
> (...)
> >>> 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?
> 
> There are such cases. Of course keys should be handled by GPIO-keys
> and these will trigger the right wakeup events in such cases.
> 
> This is for more esoteric cases: we cannot have a kernel module for
> everything people want to do with GPIOs, and the use case I accept
> is GPIOs used in automatic control etc, think factory lines or doors.
> We can't have a "door" driver or "punch arm" or "fire alarm" driver
> in the kernel. Those are userspace things.
> 
> Still such embedded systems need to be able to go to idle and
> sleep to conerve power, and then they need to put wakeups on
> these GPIOs.
> 
> So it is a feature userspace needs, though as with much of the
> sysfs ABI it is very often abused for things like keys and LEDs which
> is an abomination but we can't do much about it :(

Thanks for clearing that up.
What does that mean for this patch? Are we going ahead, accepting the
extension of this API or do all these use-cases have to wait for the
rewrite of a proper GPIO userspace interface?

	Thanks,
	Sören

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-01-29 17:23         ` Sören Brinkmann
@ 2015-02-04  9:19           ` Linus Walleij
  2015-02-04 18:27             ` Sören Brinkmann
  0 siblings, 1 reply; 10+ messages in thread
From: Linus Walleij @ 2015-02-04  9:19 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Alexandre Courbot, Johan Hovold, linux-api,
	Linux Kernel Mailing List, linux-gpio, linux-doc

On Thu, Jan 29, 2015 at 6:23 PM, Sören Brinkmann
<soren.brinkmann@xilinx.com> wrote:
> On Mon, 2015-01-19 at 09:54AM +0100, Linus Walleij wrote:
>> On Mon, Jan 19, 2015 at 5:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
>> > On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
>> >> On Fri, 2015-01-16 at 12:11PM +0100, Johan Hovold wrote:
>>
>> >>> 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.
>> (...)
>> >>> 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?
>>
>> There are such cases. Of course keys should be handled by GPIO-keys
>> and these will trigger the right wakeup events in such cases.
>>
>> This is for more esoteric cases: we cannot have a kernel module for
>> everything people want to do with GPIOs, and the use case I accept
>> is GPIOs used in automatic control etc, think factory lines or doors.
>> We can't have a "door" driver or "punch arm" or "fire alarm" driver
>> in the kernel. Those are userspace things.
>>
>> Still such embedded systems need to be able to go to idle and
>> sleep to conerve power, and then they need to put wakeups on
>> these GPIOs.
>>
>> So it is a feature userspace needs, though as with much of the
>> sysfs ABI it is very often abused for things like keys and LEDs which
>> is an abomination but we can't do much about it :(
>
> Thanks for clearing that up.
> What does that mean for this patch? Are we going ahead, accepting the
> extension of this API or do all these use-cases have to wait for the
> rewrite of a proper GPIO userspace interface?

What needs to happen (IMHO) is to make gpio_chips properly obeying
the device model, and then add the attributes for fiddling around with
GPIOs to either the *real* device or create a new char device interface.
Whatever works best. These mock devices are fragile and never
worked properly especially in the removal path as Johans recent
fixes has shown.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-02-04  9:19           ` Linus Walleij
@ 2015-02-04 18:27             ` Sören Brinkmann
  2015-02-05 10:33               ` Johan Hovold
  0 siblings, 1 reply; 10+ messages in thread
From: Sören Brinkmann @ 2015-02-04 18:27 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Alexandre Courbot, Johan Hovold, linux-api,
	Linux Kernel Mailing List, linux-gpio, linux-doc

On Wed, 2015-02-04 at 10:19AM +0100, Linus Walleij wrote:
> On Thu, Jan 29, 2015 at 6:23 PM, Sören Brinkmann
> <soren.brinkmann@xilinx.com> wrote:
> > On Mon, 2015-01-19 at 09:54AM +0100, Linus Walleij wrote:
> >> On Mon, Jan 19, 2015 at 5:20 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> >> > On Sat, Jan 17, 2015 at 1:49 AM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote:
> >> >> On Fri, 2015-01-16 at 12:11PM +0100, Johan Hovold wrote:
> >>
> >> >>> 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.
> >> (...)
> >> >>> 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?
> >>
> >> There are such cases. Of course keys should be handled by GPIO-keys
> >> and these will trigger the right wakeup events in such cases.
> >>
> >> This is for more esoteric cases: we cannot have a kernel module for
> >> everything people want to do with GPIOs, and the use case I accept
> >> is GPIOs used in automatic control etc, think factory lines or doors.
> >> We can't have a "door" driver or "punch arm" or "fire alarm" driver
> >> in the kernel. Those are userspace things.
> >>
> >> Still such embedded systems need to be able to go to idle and
> >> sleep to conerve power, and then they need to put wakeups on
> >> these GPIOs.
> >>
> >> So it is a feature userspace needs, though as with much of the
> >> sysfs ABI it is very often abused for things like keys and LEDs which
> >> is an abomination but we can't do much about it :(
> >
> > Thanks for clearing that up.
> > What does that mean for this patch? Are we going ahead, accepting the
> > extension of this API or do all these use-cases have to wait for the
> > rewrite of a proper GPIO userspace interface?
> 
> What needs to happen (IMHO) is to make gpio_chips properly obeying
> the device model, and then add the attributes for fiddling around with
> GPIOs to either the *real* device or create a new char device interface.
> Whatever works best. These mock devices are fragile and never
> worked properly especially in the removal path as Johans recent
> fixes has shown.

Sure, that would be a nice long-term solution. But until then this patch
would probably be welcomed by some people, without making the brokenness
of this interface much worse.

	Sören

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute
  2015-02-04 18:27             ` Sören Brinkmann
@ 2015-02-05 10:33               ` Johan Hovold
  0 siblings, 0 replies; 10+ messages in thread
From: Johan Hovold @ 2015-02-05 10:33 UTC (permalink / raw)
  To: Sören Brinkmann
  Cc: Linus Walleij, Alexandre Courbot, Johan Hovold, linux-api,
	Linux Kernel Mailing List, linux-gpio, linux-doc

On Wed, Feb 04, 2015 at 10:27:15AM -0800, Sören Brinkmann wrote:
> On Wed, 2015-02-04 at 10:19AM +0100, Linus Walleij wrote:

> > What needs to happen (IMHO) is to make gpio_chips properly obeying
> > the device model, and then add the attributes for fiddling around with
> > GPIOs to either the *real* device or create a new char device interface.
> > Whatever works best. These mock devices are fragile and never
> > worked properly especially in the removal path as Johans recent
> > fixes has shown.
> 
> Sure, that would be a nice long-term solution. But until then this patch
> would probably be welcomed by some people, without making the brokenness
> of this interface much worse.

We don't knowingly add broken functionality, and especially not when it
will become ABI that needs to be maintained forever.

And "this might be useful for someone at some point" is certainly not a
reason to break that rule.

Johan

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2015-02-05 10:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-15 19:49 [PATCH v4] gpio: lib-sysfs: Add 'wakeup' attribute Soren Brinkmann
2015-01-16 11:11 ` Johan Hovold
2015-01-16 16:49   ` Sören Brinkmann
2015-01-19  4:20     ` Alexandre Courbot
2015-01-19  8:54       ` Linus Walleij
2015-01-29 17:23         ` Sören Brinkmann
2015-02-04  9:19           ` Linus Walleij
2015-02-04 18:27             ` Sören Brinkmann
2015-02-05 10:33               ` Johan Hovold
2015-01-19 10:10     ` Johan Hovold

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).