[PATCH] gpio sysfs: add a "toggle" value

[PATCH] gpio sysfs: add a "toggle" value

[Mike Frysinger]

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/gpio/gpiolib.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..7f79732 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -274,7 +274,12 @@ static ssize_t gpio_value_store(struct device *dev,
 	else {
 		long		value;
 
-		status = strict_strtol(buf, 0, &value);
+		if (sysfs_streq(buf, "toggle")) {
+			value = !gpio_get_value_cansleep(gpio);
+			status = 0;
+		} else
+			status = strict_strtol(buf, 0, &value);
+
 		if (status == 0) {
 			gpio_set_value_cansleep(gpio, value != 0);
 			status = size;
-- 
1.6.3.1

Re: [PATCH] gpio sysfs: add a "toggle" value

[Andrew Morton]

On Wed,  3 Jun 2009 04:16:59 -0400 Mike Frysinger <vapier@gentoo.org> wrote:

> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

-ENOCHANGELOGAGAIN

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 51a8d41..7f79732 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -274,7 +274,12 @@ static ssize_t gpio_value_store(struct device *dev,
>  	else {
>  		long		value;
>  
> -		status = strict_strtol(buf, 0, &value);
> +		if (sysfs_streq(buf, "toggle")) {
> +			value = !gpio_get_value_cansleep(gpio);
> +			status = 0;
> +		} else
> +			status = strict_strtol(buf, 0, &value);
> +
>  		if (status == 0) {
>  			gpio_set_value_cansleep(gpio, value != 0);
>  			status = size;

A suitable place to document this is Documentation/gpio.txt.

Re: [PATCH] gpio sysfs: add a "toggle" value

[Mike Frysinger]

On Thu, Jun 4, 2009 at 03:54, Andrew Morton wrote:
> On Wed,  3 Jun 2009 04:16:59 -0400 Mike Frysinger wrote:
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> -ENOCHANGELOGAGAIN

it's pretty damned self explanatory

>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 51a8d41..7f79732 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -274,7 +274,12 @@ static ssize_t gpio_value_store(struct device *dev,
>>       else {
>>               long            value;
>>
>> -             status = strict_strtol(buf, 0, &value);
>> +             if (sysfs_streq(buf, "toggle")) {
>> +                     value = !gpio_get_value_cansleep(gpio);
>> +                     status = 0;
>> +             } else
>> +                     status = strict_strtol(buf, 0, &value);
>> +
>>               if (status == 0) {
>>                       gpio_set_value_cansleep(gpio, value != 0);
>>                       status = size;
>
> A suitable place to document this is Documentation/gpio.txt.

yes, but i wanted to make sure David didnt reject the idea before i
spend time writing documentation.
-mike

Re: [PATCH] gpio sysfs: add a "toggle" value

[David Brownell]

On Thursday 04 June 2009, Mike Frysinger wrote:
> >
> > A suitable place to document this is Documentation/gpio.txt.
> 
> yes, but i wanted to make sure David didnt reject the idea before i
> spend time writing documentation.

My initial reaction was that the function is somewhat superfluous,
so having a use case in the patch comment would be good:  why
can't userspace do the toggle itself?

On the other hand, I find it hard to object to such a small feature.
If that's the worst feature bloat in Linux this year, we're overdue
for a big party.  ;)

Re: [PATCH] gpio sysfs: add a "toggle" value

[Mike Frysinger]

On Thu, Jun 4, 2009 at 04:27, David Brownell wrote:
> On Thursday 04 June 2009, Mike Frysinger wrote:
>> > A suitable place to document this is Documentation/gpio.txt.
>>
>> yes, but i wanted to make sure David didnt reject the idea before i
>> spend time writing documentation.
>
> My initial reaction was that the function is somewhat superfluous,
> so having a use case in the patch comment would be good:  why
> can't userspace do the toggle itself?

when people were using the simple-gpio driver i wrote (which had
pretty much function parity with the sysfs implementation), i had a
customer or two ask for toggle for their systems.  i didnt get details
for why they wanted a toggle however ... they wanted it in their app
and adding it was trivial.

> On the other hand, I find it hard to object to such a small feature.
> If that's the worst feature bloat in Linux this year, we're overdue
> for a big party.  ;)

my next request was for a 1 letter command set and chaining
operations.  say you want to toggle a gpio line 4 times, having to go
between kernel and userspace for that seems kind of heavy.
echo TTTT > value
-mike

Re: [PATCH] gpio sysfs: add a "toggle" value

[Andrew Morton]

On Thu, 4 Jun 2009 04:52:35 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> my next request was for a 1 letter command set and chaining
> operations.  say you want to toggle a gpio line 4 times, having to go
> between kernel and userspace for that seems kind of heavy.
> echo TTTT > value

I was wondering whether "t" would be better than "toggle".

If you're going to do that, wouldn't you need a "d"(elay) too?  To
prevent large variations in interval between different platforms.  1
millisec, perhaps.

0d1dd0d1ddd0d1, too.

This could be fun.

Re: [PATCH] gpio sysfs: add a "toggle" value

[David Brownell]

On Thursday 04 June 2009, Mike Frysinger wrote:
> my next request was for a 1 letter command set and chaining
> operations.  say you want to toggle a gpio line 4 times, having to go
> between kernel and userspace for that seems kind of heavy.
> echo TTTT > value

Having that kind of stuff in kernelspace seems wrong;
if you're going that route, embed Guile or something.  ;)

The "TTTT" thing could as easily be scripted in BASH.

Re: [PATCH] gpio sysfs: add a "toggle" value

[Mike Frysinger]

On Thu, Jun 4, 2009 at 05:06, David Brownell wrote:
> On Thursday 04 June 2009, Mike Frysinger wrote:
>> my next request was for a 1 letter command set and chaining
>> operations.  say you want to toggle a gpio line 4 times, having to go
>> between kernel and userspace for that seems kind of heavy.
>> echo TTTT > value
>
> Having that kind of stuff in kernelspace seems wrong;
> if you're going that route, embed Guile or something.  ;)
>
> The "TTTT" thing could as easily be scripted in BASH.

the point was to lower the number of user<->kernel transitions.  yes,
all of this stuff can of course be done in the shell, but it's hard to
push limits when the interface is tying your hands and sitting on your
back.
-mike

Re: [PATCH] gpio sysfs: add a "toggle" value

[David Brownell]

On Thursday 04 June 2009, Mike Frysinger wrote:
> the point was to lower the number of user<->kernel transitions.  yes,
> all of this stuff can of course be done in the shell, but it's hard to
> push limits when the interface is tying your hands and sitting on your
> back.

So what's the application that needs to do all this in
userspace yet which can't hook up a quick kernel driver?

I don't like the idea of embedding any kind of interpreter
in the sysfs interface, but that's the direction you seem
to be heading ...

Re: [PATCH] gpio sysfs: add a "toggle" value

[Mike Frysinger]

On Thu, Jun 4, 2009 at 05:15, David Brownell wrote:
> On Thursday 04 June 2009, Mike Frysinger wrote:
>> the point was to lower the number of user<->kernel transitions.  yes,
>> all of this stuff can of course be done in the shell, but it's hard to
>> push limits when the interface is tying your hands and sitting on your
>> back.
>
> So what's the application that needs to do all this in
> userspace yet which can't hook up a quick kernel driver?
>
> I don't like the idea of embedding any kind of interpreter
> in the sysfs interface, but that's the direction you seem
> to be heading ...

i dont view the userspace GPIO interface as a simple "i want to wiggle
pins for fun and testing".  i see it as a dynamic method for building
userspace drivers for simple devices.  as such, having a very simple
"command language" for optimizing the interaction provides a pretty
good trade off between kernel complexity and lowering overhead imo.
-mike

Re: [PATCH] gpio sysfs: add a "toggle" value

[David Brownell]

On Thursday 04 June 2009, Mike Frysinger wrote:
> > I don't like the idea of embedding any kind of interpreter
> > in the sysfs interface, but that's the direction you seem
> > to be heading ...
> 
> i dont view the userspace GPIO interface as a simple "i want to wiggle
> pins for fun and testing".  i see it as a dynamic method for building
> userspace drivers for simple devices.

I don't.  I see it as an adjunct to real drivers.  Or maybe
we just differ on what "simple" means here.


> as such, having a very simple 
> "command language" for optimizing the interaction provides a pretty
> good trade off between kernel complexity and lowering overhead imo.

If that's what you want, why don't you write a custom driver
that provides the kind of command interpreter you want?

Re: [PATCH] gpio sysfs: add a "toggle" value

[Mike Frysinger]

On Thu, Jun 4, 2009 at 05:48, David Brownell wrote:
> On Thursday 04 June 2009, Mike Frysinger wrote:
>> > I don't like the idea of embedding any kind of interpreter
>> > in the sysfs interface, but that's the direction you seem
>> > to be heading ...
>>
>> i dont view the userspace GPIO interface as a simple "i want to wiggle
>> pins for fun and testing".  i see it as a dynamic method for building
>> userspace drivers for simple devices.
>
> I don't.  I see it as an adjunct to real drivers.  Or maybe
> we just differ on what "simple" means here.
>
>> as such, having a very simple
>> "command language" for optimizing the interaction provides a pretty
>> good trade off between kernel complexity and lowering overhead imo.
>
> If that's what you want, why don't you write a custom driver
> that provides the kind of command interpreter you want?

i had "simple-gpio" but it was rejected as it duplicated too much with
the gpio sysfs interface

does vectored writes (writve) work with sysfs ?  if so, then that
should work for back-to-back commands rather than having sysfs do
stream parsing.
-mike

Re: [PATCH] gpio sysfs: add a "toggle" value

[Andrew Morton]

On Thu, 4 Jun 2009 04:16:14 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Thu, Jun 4, 2009 at 03:54, Andrew Morton wrote:
> > On Wed, __3 Jun 2009 04:16:59 -0400 Mike Frysinger wrote:
> >> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> >
> > -ENOCHANGELOGAGAIN
> 
> it's pretty damned self explanatory

The implementation is simple, but the implementation doesn't tell us why
you believe this feature should be added to Linux.

Is it because doing it from userspace is too slow?

It is because doing it from userspace is racy?  If so, isn't that
userspace already busted?

Isn't a userspace driver which doesn't already know the state of its
GPIO bits rather lame?

etc.