Re: [PATCH] gpiolib: Allow user-selection

Previous thread: [PATCH 04/11] x86/pci: Makefile merge: Decoupling options for mp_bus_to_node.o by Robert Richter on Wednesday, July 2, 2008 - 1:50 pm. (16 messages)

Next thread: [PATCH] x86: fix Intel Mac booting with EFI by Hugh Dickins on Wednesday, July 2, 2008 - 2:48 pm. (3 messages)
From: Michael Buesch
Date: Wednesday, July 2, 2008 - 2:46 pm

This patch adds functionality to the gpio-lib subsystem to
make it possible to enable the gpio-lib code even if the
architecture code didn't request to get it built in.

The archtitecture code does still need to implement the gpiolib
accessor functions in its asm/gpio.h file.
This patch adds the implementations for x86 and PPC.

With these changes it is possible to run generic GPIO expansion
cards on every architecture that implements the trivial wrapper
functions. Support for more architectures can easily be added.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---

This patch should be scheduled for the next kernel feature merge window.


Index: linux-2.6/arch/x86/Kconfig
===================================================================
--- linux-2.6.orig/arch/x86/Kconfig	2008-07-02 23:12:59.000000000 +0200
+++ linux-2.6/arch/x86/Kconfig	2008-07-02 23:15:33.000000000 +0200
@@ -25,6 +25,7 @@ config X86
 	select HAVE_KRETPROBES
 	select HAVE_KVM if ((X86_32 && !X86_VOYAGER && !X86_VISWS && !X86_NUMAQ) || X86_64)
 	select HAVE_ARCH_KGDB if !X86_VOYAGER
+	select ARCH_WANT_OPTIONAL_GPIOLIB if !X86_RDC321X
 
 config ARCH_DEFCONFIG
 	string
Index: linux-2.6/include/asm-x86/gpio.h
===================================================================
--- linux-2.6.orig/include/asm-x86/gpio.h	2008-07-02 23:12:59.000000000 +0200
+++ linux-2.6/include/asm-x86/gpio.h	2008-07-02 23:31:42.000000000 +0200
@@ -1,6 +1,62 @@
+/*
+ * Generic GPIO API implementation for x86.
+ *
+ * Derived from the generic GPIO API for powerpc:
+ *
+ * Copyright (c) 2007-2008  MontaVista Software, Inc.
+ *
+ * Author: Anton Vorontsov <avorontsov@ru.mvista.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
 #ifndef _ASM_I386_GPIO_H
 #define _ASM_I386_GPIO_H
 
+#ifdef ...
From: Andrew Morton
Date: Wednesday, July 2, 2008 - 5:04 pm

On Wed, 2 Jul 2008 23:46:53 +0200

drivers/gpio/gpiolib.c: In function 'gpio_export':
drivers/gpio/gpiolib.c:432: error: 'struct class' has no member named 'devices'
drivers/gpio/gpiolib.c:456: error: implicit declaration of function 'device_create'
drivers/gpio/gpiolib.c:457: warning: assignment makes pointer from integer without a cast
drivers/gpio/gpiolib.c: In function 'gpio_unexport':
drivers/gpio/gpiolib.c:509: warning: passing argument 2 of 'class_find_device' from incompatible pointer type
drivers/gpio/gpiolib.c:509: error: too few arguments to function 'class_find_device'
drivers/gpio/gpiolib.c: In function 'gpiochip_export':
drivers/gpio/gpiolib.c:536: error: 'struct class' has no member named 'devices'
drivers/gpio/gpiolib.c:542: warning: assignment makes pointer from integer without a cast
drivers/gpio/gpiolib.c: In function 'gpiochip_unexport':
drivers/gpio/gpiolib.c:575: warning: passing argument 2 of 'class_find_device' from incompatible pointer type
drivers/gpio/gpiolib.c:575: error: too few arguments to function 'class_find_device'

I assume this patch was prepared against some ancient out-of-date
kernel such as current Linus mainline.

Guys, we have a new development tree now.
--

From: Michael Buesch
Date: Wednesday, July 2, 2008 - 5:26 pm

Oh well. Let me diff it against linux-next then. I didn't know Linus'
tree doesn't get any development updates anymore in time and so is

-- 
Greetings Michael.
--

From: David Brownell
Date: Wednesday, July 2, 2008 - 10:00 pm

May be, but shuffling headers around would not have caused
that type of breakage.

I'm thinking some driver model changes broke the gpio sysfs
interface code, and this happens to show up right now because
that code wasn't previously getting built.

Grumph.  I can easily switch the device_create() over to
use device_create_drvdata() -- didn't I already send in
a patch like that? -- but the other stuff is completely
backwards-incompatible.



--

From: Andrew Morton
Date: Wednesday, July 2, 2008 - 10:08 pm

beats me ididntdoitnobodysawmedoit.

But what I am repeatedly seeing is people cheerfully raising 2.6.27
patches against the 2.6.26 tree when we have a nice 2.6.27 tree for
developing against.  Those days are over, guys.



I'm also seeing obvious signs that developers aren't _testing_ their
new code within the context of the 2.6.27 tree.  They're obviously
testing their stuff against 2.6.26 and then hoping and praying, only it
doesn't always work out for them.

--

From: David Brownell
Date: Wednesday, July 2, 2008 - 10:41 pm

In this case, the contract seems to have changed.  Previously,
it was that folk doing the API changes would change everyone
using the APIs ... in this case, that's not happened.  (And I
can sort of understand why.  No matter how it's done, someone

As they most certainly should.  That's already a lot of
variables in the test process, and anyone wanting to
actually *USE* that code right now will probably not want

The new "-next" process is still working out.  Backwards-incompatible
changes will *always* make problems.  In this case there seem to
have been three such changes:

	- device_create() vanishing, even for users which did the
	  locking correctly ... this was at least something of a
	  graceful migration, although it would have been better
	  if it were deprecated first.  Change can work against
	  the current (2.6.26) code base.

	- class.devices vanishing ... what I really needed was
	  more of a "is this class initialized yet" call, so
	  testing for class->devices.next non-null can be replaced
	  by a test for class->p non-null.  (Or maybe this should
	  be viewed as needing a "real" driver model call, so that
	  code which must run before driver model init can more
	  easily cooperate with driver model stuff that has to
	  run much later, after the guts are initialized.)

	- Extra parameter to class_find_device().  This could
	  have been done in a backwards-compatible manner, like
	  device_create() was, but ... it wasn't.

When I finish pulling linux-next, I'll make an updated patch.
And probably send in an updated version of Michaels patch;
though I imagine the update will be no more than a s-o-b.

- dave
--

From: Greg KH
Date: Thursday, July 3, 2008 - 12:37 pm

device_create() did disappear in -next, but when it goes to Linus it
will not, it will come back as a #define and everything will be
converted back.  That was to catch the build errors when I audited the

If you need such a function, please let me know and I can provide it, I
was not aware of anyone using class.devices, it is an

This was simpler as there were less users of this function.  And the
whole tree was fixed up.  If there are trees on top of -next that I
can't see, that's pretty hard for me to fix :)

thanks,

greg k-h
--

From: David Brownell
Date: Thursday, July 3, 2008 - 2:28 pm

When the fields are there, it's hard to say what's "internal"
in the absense of comments ... :)

The scenario is that gpios sometimes get initialized very early,
as part of board setup before kmalloc or irqs work, so when they
get registered sometimes there's work left over for later.  All
that's needed is a predicate to test whether that class can be
used as a parameter to stuff like device_create_drvdata() and

This was in the MM tree with other GPIO patches.

- Dave

--

From: Greg KH
Date: Thursday, July 3, 2008 - 4:08 pm

Because either way you are going to have to change your code to be able
to build properly on top of it.  So what could would the compatibility
macro do for you now.  With the code it way it is, you instantly see the
issue, instead of trying to figure out a warning or other parameter
error.



If you want to see if a class has any devices in it, just do a search
for any device instead of poking into that field.  Or I can wrap that up
in the driver core if needed.

thanks,

greg k-h
--

From: David Brownell
Date: Friday, July 11, 2008 - 10:32 pm

That would require taking driver model mutexes before
tasking is working and driver_init() is called.  GPIOs
are regularly used *really* early in board setup ... even
before IRQs are working.

Are driver model searches really expected to work then??

Seems simpler to me to just check "is pointer NULL",
when we know that the pointer is initialized by the
class initialization, and (because the relevant struct
is zero-initialized) null beforehand.  That works more
or less any point during kernel initialization ... not
just after the first task is running.  Plus it's not
as expensive.  :)

- Dave
--

From: Rene Herman
Date: Thursday, July 3, 2008 - 1:36 am

Developing against -next is even worse than developping against an -rc1. 
You normally want to be running the code you develop meaning you'd very 
frequently drown in everyone else's bugs without getting to your own if 
you do. Development needs to happen against something relatively stable 
really and the last release would generally seem to be the best choice.

Now ofcourse, _porting_ it to -next before submitting makes lots of 
sense but positioning -next as THE devel tree a bit less I feel...

Rene.
--

From: Andrew Morton
Date: Thursday, July 3, 2008 - 2:01 am

Well one can develop against mainline and port to linux-next and then
do a quick test.  But it seems like pointless additional overhead to
me.

What else can we do?  People are frequently sending patches which don't
even apply to the tree for which they're intended.  And some which

If one wishes.

It's readily obvious that people (ie: top-level maintainers) aren't
even compile-testing their own stuff once it's merged into linux-next. 
You say (but don't provide evidence that) linux-next is too unstable to
develop against.  Well guess why?  Because people are choosing to let
it be that way.


Now the upshot of this might be that people end up having to spend more
time testing and less time developing.  It is fairly apparent that this
is a desirable outcome.
--

From: Rene Herman
Date: Thursday, July 3, 2008 - 3:19 am

Other than developer needing a relatively stable base to develop against 
and run so as to concentrate on the effects of own code certainly people 
that said developer wants to test that which is in development want such 
a base. Fewer testers can/will be testing a particular feature if to do 
so they need to first drag in megabytes upon megabytes of other changes 
as well.

Last iteration I looked at and tested PNP patches and was using .25 as 
base which went smoothly. Then at 26-rc1 I needed to start running that 
since patches were against that which delayed things for a long time 
while I needed to find time to deal with that many unrelated changes 
some of which rather violently broke stuff here (ISA DMA most profoundly).

So should I have caught the ISA DMA breakage before it hit -rc1? Given 
that I tend to care about sound/isa which is its main user, perhaps. But 
the point was that I was trying to test ISAPnP, yet another thing that 
sound/isa is a main user of...

Many users/testers simply do not want to be running bleedin' edge all of 
the time because they'd be spending way too much time fighting bugs in 
places that are of no particular interest to them and when features are 
developed against said edge, you'd have just limited your tester base 
further to those who either will, or have enough sophistication to take 
the change in isolation and backport it to their current kernel 
themselves. And do it again on the next iteration of the feature.

As recently as yesterday I stalled on testing an ALSA patch when I saw 
it fail against mainline and wished for some other model than always 
having to run the very latest.

Not to sound overly pompous (I hope) but not everyone can be responsible 
for all these integration pains. People in corner A tend to care about 
corner A and might consider corners B, C and D unwelcome distractions 
mostly. Linux then has people like you and Linus to stand in the middle 
and make all the corners get along again.

Yes, porting to ...
From: David Brownell
Date: Thursday, July 3, 2008 - 1:25 am

Should be addressed by the following, at least in
terms of build problems aginst linux-next.

The resulting kernel doesn't seem bootable on any
board I currently have set up for testing gpio calls.
The build dies in the TTY stack.

- Dave


=============	CUT HERE
Cope with some backwards-incompatible driver model API changes
now in the linux-next tree:

 - device_create() is going away, even for drivers using it safely,
   in favor of device_create_drvdata().
 - class->devices is gone, but testing class->p serves the same
   purpose (non-null when class is usable with driver model calls).
 - class_find_device() needs a new argument #2 (NULL)

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/gpio/gpiolib.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

--- a/drivers/gpio/gpiolib.c	2008-07-02 23:29:58.000000000 -0700
+++ b/drivers/gpio/gpiolib.c	2008-07-03 00:04:57.000000000 -0700
@@ -429,7 +429,7 @@ int gpio_export(unsigned gpio, bool dire
 	int			status = -EINVAL;
 
 	/* can't export until sysfs is available ... */
-	if (!gpio_class.devices.next) {
+	if (!gpio_class.p) {
 		pr_debug("%s: called too early!\n", __func__);
 		return -ENOENT;
 	}
@@ -453,10 +453,9 @@ int gpio_export(unsigned gpio, bool dire
 	if (status == 0) {
 		struct device	*dev;
 
-		dev = device_create(&gpio_class, desc->chip->dev, 0,
-				"gpio%d", gpio);
+		dev = device_create_drvdata(&gpio_class, desc->chip->dev, 0,
+				desc, "gpio%d", gpio);
 		if (dev) {
-			dev_set_drvdata(dev, desc);
 			if (direction_may_change)
 				status = sysfs_create_group(&dev->kobj,
 						&gpio_attr_group);
@@ -506,7 +505,7 @@ void gpio_unexport(unsigned gpio)
 	if (test_bit(FLAG_EXPORT, &desc->flags)) {
 		struct device	*dev = NULL;
 
-		dev = class_find_device(&gpio_class, desc, match_export);
+		dev = class_find_device(&gpio_class, NULL, desc, match_export);
 		if (dev) {
 			clear_bit(FLAG_EXPORT, &desc->flags);
 			put_device(dev);
@@ ...
From: Michael Buesch
Date: Thursday, July 3, 2008 - 1:42 am

Thanks a lot Dave.



-- 
Greetings Michael.
--

Previous thread: [PATCH 04/11] x86/pci: Makefile merge: Decoupling options for mp_bus_to_node.o by Robert Richter on Wednesday, July 2, 2008 - 1:50 pm. (16 messages)

Next thread: [PATCH] x86: fix Intel Mac booting with EFI by Hugh Dickins on Wednesday, July 2, 2008 - 2:48 pm. (3 messages)