login
Header Space

 
 

Re: [PATCH 1/1] [GPIO]: new arch-independent simple-gpio driver

Previous thread: [PATCH 1/1] Input/Joystick Driver: add support AD7142 joystick driver by Bryan Wu on Wednesday, March 26, 2008 - 9:00 pm. (5 messages)

Next thread: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory by Bryan Wu on Wednesday, March 26, 2008 - 9:08 pm. (9 messages)
To: <dbrownell@...>, <linux-kernel@...>
Cc: Mike Frysinger <vapier.adi@...>, Bryan Wu <cooloney@...>
Date: Wednesday, March 26, 2008 - 9:05 pm

From: Mike Frysinger &lt;vapier.adi@gmail.com&gt;

Signed-off-by: Mike Frysinger &lt;vapier.adi@gmail.com&gt;
Signed-off-by: Bryan Wu &lt;cooloney@kernel.org&gt;
---
 drivers/char/Kconfig       |   14 ++
 drivers/char/Makefile      |    1 +
 drivers/char/simple-gpio.c |  308 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 323 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/simple-gpio.c

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 47c6be8..dd17d06 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -4,6 +4,20 @@
 
 menu "Character devices"
 
+config SIMPLE_GPIO
+	tristate "Simple GPIO char interface"
+	depends on GENERIC_GPIO
+	default n
+	help
+	  If you say Y here, you will get support for a character device
+	  interface to the GPIO pins which will allow you to read and
+	  write GPIO values from userspace.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simple-gpio.
+
+	  If unsure, it is safe to say Y.
+
 config VT
 	bool "Virtual terminal" if EMBEDDED
 	depends on !S390
diff --git a/drivers/char/Makefile b/drivers/char/Makefile
index 5407b76..e087ec1 100644
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -97,6 +97,7 @@ obj-$(CONFIG_CS5535_GPIO)	+= cs5535_gpio.o
 obj-$(CONFIG_GPIO_VR41XX)	+= vr41xx_giu.o
 obj-$(CONFIG_GPIO_TB0219)	+= tb0219.o
 obj-$(CONFIG_TELCLOCK)		+= tlclk.o
+obj-$(CONFIG_SIMPLE_GPIO)	+= simple-gpio.o
 
 obj-$(CONFIG_MWAVE)		+= mwave/
 obj-$(CONFIG_AGP)		+= agp/
diff --git a/drivers/char/simple-gpio.c b/drivers/char/simple-gpio.c
new file mode 100644
index 0000000..36204ca
--- /dev/null
+++ b/drivers/char/simple-gpio.c
@@ -0,0 +1,308 @@
+/*
+ * Simple character interface to GPIOs.  Allows you to read/write values and
+ * control the direction.  Maybe add wakeup when gpio framework supports it ...
+ *
+ * To use, just declare in your board resources:
+ * static struct resource foo_resources[] = {
+ *     .start = 0,
+ * ...
To: Bryan Wu <cooloney@...>, Mike Frysinger <vapier.adi@...>
Cc: <dbrownell@...>, <linux-kernel@...>
Date: Thursday, March 27, 2008 - 1:52 am

Hi,


Considered putting this in drivers/gpio?  Not a real problem, up to you

Userspace notification of a GPIO IRQ?  Not too 'simple' but very
worthwhile.

If you're feeling keen (and it doesn't violate the 'simple' in the name)
then I think it would be well worth being able to attach a string to
each gpio entry in the platform_data and make them available through
sysfs.  Very few userspace users will know what gpio_id they actually
want unless they can see a "PortA-05" tag attached to it somewhere.

This is extra-especially true for dynamically allocated gpio ids

This don't sit right with me; I reckon an IORESOURCE_GPIO may not be a
bad move but that's for a different patch.  In the mean time getting the

Hmm, a bit of a cleanup regarding messaging is needed IMO.

Should your pr_*init macros be accepted somewhere higher up the tree?
Either that or dropped, it doesn't seem right wedging them in here.
Sure it might cost you a few hundred bytes of RAM but would be nice to

Hmm, coding style question marks around a 2- or even 3-entry switch

Was it a conscious thing to only allow 1 range of gpios per device?  I
can imagine that it's quite likely that people are going to want to
expose all unused gpios on a SoC to userspace.  This is going to mean
lots of small ranges split either side of pre-reserved pins and one


Making assumptions about the format of a dev_t is Bad.  Sure it's a bit
of a pain constructing the correct node with MKDEV/MAJOR macros but it's

--
To: Ben Nizette <bn@...>, Mike Frysinger <vapier.adi@...>
Cc: Bryan Wu <cooloney@...>, lkml <linux-kernel@...>
Date: Saturday, April 5, 2008 - 6:58 pm

I'd expect to see a generic GPIO mechanism like this
in drivers/gpio, yes.  But it should be a bit more

As I said in the other thread, this is what I most dislike about
this particular driver:  the need for board-specific setup.  The
need for a character device inode per GPIO could probably be lived
with, if that bigger issue were resolved.

The userspace GPIO access scenarios which seem most compelling to
me involve filling in gaps in board support.  Expecting the folk
who created those gaps (by overlooking them, or not knowing enough
about the system's eventual usage mode) to have enabled resolving
them in this way ... seems unwise.    :)

Building on Ben's comment, it's not just unused SOC GPIOs, it's
all the GPIOs on a board, some of which will be from a SOC and
others of which will be from external chips.

- Dave


--
To: Ben Nizette <bn@...>, Mike Frysinger <vapier.adi@...>
Cc: Bryan Wu <cooloney@...>, <linux-kernel@...>
Date: Saturday, April 5, 2008 - 6:42 pm

Me, I'm all in favor of getting rid of structural code bloat.
A few hundred bytes of such stuff shaved out a dozen drivers
on a given platforms would be almost free page saved!  :)

So I'd be interested in seeing those get submitted ... but as
inline functions, not fancy macros.  (Once they're submitted,
then let the flamage begin ... much less than kernel I18N!)

- Dave

--
Previous thread: [PATCH 1/1] Input/Joystick Driver: add support AD7142 joystick driver by Bryan Wu on Wednesday, March 26, 2008 - 9:00 pm. (5 messages)

Next thread: [PATCH 1/1] [Blackfin try #2] char driver for Blackfin on-chip OTP memory by Bryan Wu on Wednesday, March 26, 2008 - 9:08 pm. (9 messages)
speck-geostationary