[RFC, PATCH 1/3] gpiodev - API definitions

[RFC, PATCH 1/3] gpiodev - API definitions

[Paul Sokolovsky]

Hello linux-arm-kernel,

GPIODEV API: Core API definitions. Provided are:
1. struct gpiodev_ops which must be included into platform_data structure
of a device which will provide GPIODEV API; driver for a device must
initialize this structure.
2. Structural definition of generalized GPIO identifier (struct gpio).
2. Set of API calls for clients. This fully follow Generic GPIO API
naming and semantics, except that they have "gpiodev" prefix and
accept struct gpio instead of integer gpio identifiers.


Index: include/linux/gpiodev.h
===================================================================
RCS file: include/linux/gpiodev.h
diff -N include/linux/gpiodev.h
--- /dev/null   1 Jan 1970 00:00:00 -0000
+++ include/linux/gpiodev.h     10 Apr 2007 19:17:12 -0000      1.5
@@ -0,0 +1,43 @@
+#ifndef __GPIODEV_H
+#define __GPIODEV_H
+
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <asm/gpio.h>
+
+/* Interface */
+
+/* This structure must be first member of device platform_data structure 
+   of a device which provides gpiodev interface */
+struct gpiodev_ops {
+        int (*get)(struct device *this, int gpio_no);
+        void (*set)(struct device *this, int gpio_no, int val);
+        int (*to_irq)(struct device *this, int gpio_no);
+};
+       
+/* Generalized GPIO structure */
+
+struct gpio {
+       struct platform_device *gpio_dev;
+       int gpio_no;
+};
+
+/* API functions */
+
+static inline int gpiodev_get_value(struct gpio *gpio)
+{
+       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;
+       return ops->get(&gpio->gpio_dev->dev, gpio->gpio_no);
+}
+static inline void gpiodev_set_value(struct gpio *gpio, int val)
+{
+       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;
+       ops->set(&gpio->gpio_dev->dev, gpio->gpio_no, val);
+}
+static inline int gpiodev_to_irq(struct gpio *gpio)
+{
+       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;
+       return ops->to_irq(&gpio->gpio_dev->dev, gpio->gpio_no);
+}
+
+#endif /* __GPIODEV_H */


-- 
Best regards,
 Paul                          mailto:pmiscml@gmail.com

Re: [RFC, PATCH 1/3] gpiodev - API definitions

[Eric Miao]

it looks ok, but I have several questions:

1. why should we bind this to platform_device, what if the gpio device
is not actually a "platform_device", say, a I2C device, a SPI device or
even a USB device?

2. I still doubt the benefit of using of a structure for a gpio, isn't a gpio
number not enough?

3. If one is going to use a GPIO, he has to initialize a "struct gpio"
before that, how is he suppose to know the value for "gpio->gpio_dev"?

4. how can we optimize to a direct register access instruction (e.g.
to GPDR in PXA) for bit-banging operation (pardon me, I don't exactly
remember the name for such operation, maybe bit-banging)

Re: [RFC, PATCH 1/3] gpiodev - API definitions

[Paul Sokolovsky]

Hello Eric,

Wednesday, April 11, 2007, 3:30:45 AM, you wrote:

> it looks ok, but I have several questions:

> 1. why should we bind this to platform_device, what if the gpio device
> is not actually a "platform_device", say, a I2C device, a SPI device or
> even a USB device?

  Good point. That was handhelds.org-specific "optimization", as we so
far mostly dealing with GPIO devices on platform bus, and could save
us writing "&pdev.dev". This must be changed to use struct device,
sure.

> 2. I still doubt the benefit of using of a structure for a gpio, isn't a gpio
> number not enough?

  Well, I replied to David's earlier post to LAKML after posting this,
and discussed both approach in more detail (and even more color). To
sum up, yes, we can use either structures for GPIOs, or scalar ints.
But both these approaches leads to some tradeoffs. Our argument is
that "scalar" (aka flat) approach leads to more tradeoffs - you need
to "flatten" your GPIO space first, and this is least evil, as indeed
this could be done using compile-time defines. But then you need to
"unflatten" it, and this happens at runtime, wasting cycles.

  Using structure GPIO ids skips this "flattening"/"unflattening"
(maybe multiplexing/demultiplexing are better terms) phases. Trade off
is necessity to write:

.gpio = {&gpio_device, GPIO_DEVICE_GPIO_1}

instead of:

#define BASE_FOR_GPIO_DEVICE_ON_MY_BOARD 1000
.gpio = BASE_FOR_GPIO_DEVICE_ON_MY_BOARD + GPIO_DEVICE_GPIO_1


  IMHO, pretty acceptable syntactical tradeoff ;-)

> 3. If one is going to use a GPIO, he has to initialize a "struct gpio"
> before that, how is he suppose to know the value for "gpio->gpio_dev"?

  Yes, so the target usage of GPIODEV API is to write
device-independent driver. Such driver simply shouldn't care what
device is used for GPIO - it should be able to accept any, - and how
exactly GPIO ops are performed.

  But something should care what actual GPIO device is being used. And
we know what's that - something which people call "platform code", but
I better call old good "machine definition file", to remove any
ambiguities. Such a definition is written for a specific machine, so
obviously you know what GPIO devices it has, and for what purpose pins
of them I used for. So, you have something like:

struct platform_device my_gpio_device = { .dev = {.name = "asic_foo"}};

struct some_driver_platform_data some_driver_pdata = {
...
   .pullup = {&my_gpio_device.dev, ASIC_FOO_GPIO_PULLUP},
...
}


  Important thing to note here is that structured GPIO id's are still
constants! Link-time constant, not compile-time, as scalar id's would
be, but still there's zero overhead at runtime.


> 4. how can we optimize to a direct register access instruction (e.g.
> to GPDR in PXA) for bit-banging operation (pardon me, I don't exactly
> remember the name for such operation, maybe bit-banging)

  Well, you should first decide what exactly you want. If you want to
do bit-banging on PXA, then well, - you don't need GPIODEV and its
polymorphic support at all! If you know GPIO# beforehand, just use
David's GPIO API - it will constant-optimize it to GPCR/GPSR access
rigth away.

  If you need to support arbitrary GPIO#, you can get better
frequency/throughput by skipping GPIO API still, like in the following
pseudocode:

  MASK = 1 << GPIO#;
  for (;count < SIZE; count--) {
    GPSR = MASK;
    nop;
    GPCR = MASK;
  }

  If you'd use GPIO API, it would recompute MASK on each call, wasting
your cycles.


  But now it's completely different story if you have a need to
write a driver which will bang bits on any GPIO of any device,
including such devices which you can't imagine at the time of driver's
writing at all - that's the GPIODEV estate. Obviously, for such general
usage it doesn't make too much sense to speak about optimization for a one
tiny specific device out of infinity of its domain - it's purpose is
exactly generality, not optimality, and it's well known that these
notions are reciprocals.

  If you still want super-optimal handling for PXA case, while being
able to still support anything else, solution is also known - have two
drivers, one fast and PXA-adhoc, another generic but slower, and use
extraneous logic to select which one to use.


  Back to GPIODEV's traits, it's still offers best performance which
can be achieved for infinitely extendable solution (within bounds of
the interface defined, of course), because it uses best known solution
for such problem (and such solution is simple and fast) - indirect
function call, and all data needed for such call are available right
away due to the magic of structural GPIO id's.

  In this regard, your implementation draft, which uses scalars and
looping to find out owning GPIO device/handler, pretty bad suited for
bit-banging at all: just consider that GPIO operation overhead in this
case is dependent on number of GPIO devices present. Add new one, and
your (supposedly) carefully tuned latencies and delay "float".

  David's code, which is essentially a simple hashing scheme, at least
has O(1) complexity in regard to number of GPIO devices present, but
still wastes precious cycles for useless things, like making a lookup
in hash table and subtracting GPIO base.


-- 
Best regards,
 Paul                            mailto:pmiscml@gmail.com

Re: [RFC, PATCH 1/3] gpiodev - API definitions

[Juergen Schindele]

Am Dienstag, 10. April 2007 23:30 schrieb Paul Sokolovsky:
> Hello linux-arm-kernel,
>
> GPIODEV API: Core API definitions. Provided are:
> 1. struct gpiodev_ops which must be included into platform_data structure
> of a device which will provide GPIODEV API; driver for a device must
> initialize this structure.
> 2. Structural definition of generalized GPIO identifier (struct gpio).
> 2. Set of API calls for clients. This fully follow Generic GPIO API
> naming and semantics, except that they have "gpiodev" prefix and
> accept struct gpio instead of integer gpio identifiers.
>
>
> Index: include/linux/gpiodev.h
> ===================================================================
> RCS file: include/linux/gpiodev.h
> diff -N include/linux/gpiodev.h
> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ include/linux/gpiodev.h     10 Apr 2007 19:17:12 -0000      1.5
> @@ -0,0 +1,43 @@
> +#ifndef __GPIODEV_H
> +#define __GPIODEV_H
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <asm/gpio.h>
> +
> +/* Interface */
> +
> +/* This structure must be first member of device platform_data structure
> +   of a device which provides gpiodev interface */
> +struct gpiodev_ops {
> +        int (*get)(struct device *this, int gpio_no);
> +        void (*set)(struct device *this, int gpio_no, int val);
> +        int (*to_irq)(struct device *this, int gpio_no);
> +};
> +
> +/* Generalized GPIO structure */
> +
> +struct gpio {
> +       struct platform_device *gpio_dev;
> +       int gpio_no;
> +};
> +
> +/* API functions */
> +
> +static inline int gpiodev_get_value(struct gpio *gpio)
> +{
> +       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;

wouldn't it be more sure to verify if xxx function is NOT null
before using it ?? Perhaps something like that
	     BUG_ON(!ops->get);

> +       return ops->get(&gpio->gpio_dev->dev, gpio->gpio_no);
> +}
> +static inline void gpiodev_set_value(struct gpio *gpio, int val)
> +{
> +       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;
> +       ops->set(&gpio->gpio_dev->dev, gpio->gpio_no, val);
> +}
> +static inline int gpiodev_to_irq(struct gpio *gpio)
> +{
> +       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;
> +       return ops->to_irq(&gpio->gpio_dev->dev, gpio->gpio_no);
> +}
> +
> +#endif /* __GPIODEV_H */

-- 
--------------------------------------------------------------
Jürgen Schindele  
Software-Entwicklung

NENTEC Netzwerktechnologie GmbH
Greschbachstr. 12
76229 Karlsruhe
Deutschland
Telefon: +49/721/94249-51
Telefax: +49/721/94249-10
E-Mail:  schindele@nentec.de
WEB:     www.nentec.de
 
Geschäftsführung: Klaus Becker, Roland Knapp
Sitz der Gesellschaft: Karlsruhe
Handelsregister: Amtsgericht Mannheim HRB 107658
 
Diese E-Mail enthält vertrauliche oder rechtlich geschützte Informationen.
Wenn Sie nicht der vorgesehene Empfänger sind, informieren Sie bitte sofort
den Absender und löschen Sie diese E-Mail. Das unbefugte Kopieren dieser E-Mail
oder die unbefugte Weitergabe der enthaltenen Informationen ist nicht gestattet.
 
The information contained in this message is confidential or protected by law.
If you are not the intended recipient, please contact the sender and delete this
message. Any unauthorised copying of this message or unauthorised distribution of
the information contained herein is prohibited.
--------------------------------------------------------------

Re: [RFC, PATCH 1/3] gpiodev - API definitions

[Russell King]

On Wed, Apr 11, 2007 at 08:47:01AM +0200, Juergen Schindele wrote:
> Am Dienstag, 10. April 2007 23:30 schrieb Paul Sokolovsky:
> > +static inline int gpiodev_get_value(struct gpio *gpio)
> > +{
> > +       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;
> 
> wouldn't it be more sure to verify if xxx function is NOT null
> before using it ?? Perhaps something like that
> 	     BUG_ON(!ops->get);
> 
> > +       return ops->get(&gpio->gpio_dev->dev, gpio->gpio_no);

What does the BUG_ON buy us that oopsing due to a NULL pointer deref
doesn't?  Both cases you end up with a register dump and backtrace.
In fact, on ARM the NULL pointer deref provides a more accurate
register dump and backtrace than BUG_ON() can ever do.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [RFC, PATCH 1/3] gpiodev - API definitions

[Paul Sokolovsky]

Hello Juergen,

Wednesday, April 11, 2007, 9:47:01 AM, you wrote:

> Am Dienstag, 10. April 2007 23:30 schrieb Paul Sokolovsky:
>> Hello linux-arm-kernel,
>>
>> GPIODEV API: Core API definitions. Provided are:
>> 1. struct gpiodev_ops which must be included into platform_data structure
>> of a device which will provide GPIODEV API; driver for a device must
>> initialize this structure.
>> 2. Structural definition of generalized GPIO identifier (struct gpio).
>> 2. Set of API calls for clients. This fully follow Generic GPIO API
>> naming and semantics, except that they have "gpiodev" prefix and
>> accept struct gpio instead of integer gpio identifiers.
>>
>>
[]
>> +/* API functions */
>> +
>> +static inline int gpiodev_get_value(struct gpio *gpio)
>> +{
>> +       struct gpiodev_ops *ops = gpio->gpio_dev->dev.platform_data;

> wouldn't it be more sure to verify if xxx function is NOT null
> before using it ?? Perhaps something like that
>              BUG_ON(!ops->get);

  GPIODEV is considered to be low-level one and critical for speed, so
all method pointers assumed to be set properly. In particular, if some
operation is not available for a device (say, GPI/GPO case), a method
must be set to a stab function.

  BUG_ON would be acceptable, as it can be compiled out based on
CONFIG setting, but as was pointed out, doesn't add much into picture
anyway.

  But I'd be happy to add comment to struct gpiodev_ops declaration
about the described method constraints, thanks for comment.

[]


-- 
Best regards,
 Paul                            mailto:pmiscml@gmail.com