Skip to contentSkip to conversation

arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

PATCH New Ben Hutchings 6 participants 10 messages Sep 5, 2011 – Sep 6, 2011

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

The GPIO implementations for these two machines depend on gpiolib, so they must not select GENERIC_GPIO directly.

---
This hould fix the build failure seen here:
https://buildd.debian.org/status/fetch.php?pkg=linux-2.6&arch=armel&ver=3.1.0%7Erc4-1%7Eexperimental.1&stamp=1315007168

However, I have not yet tested it.

Ben.

 arch/arm/Kconfig |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
arch/arm/Kconfig +2 -2
index f23712d..a1dbc4b 100644
@@ -477,7 +477,7 @@ config ARCH_IXP4XX
 	depends on MMU
 	select CLKSRC_MMIO
 	select CPU_XSCALE
-	select GENERIC_GPIO
+	select ARCH_REQUIRE_GPIOLIB
 	select GENERIC_CLOCKEVENTS
 	select HAVE_SCHED_CLOCK
 	select MIGHT_HAVE_PCI
@@ -830,7 +830,7 @@ config ARCH_U300
 	select GENERIC_CLOCKEVENTS
 	select CLKDEV_LOOKUP
 	select HAVE_MACH_CLKDEV
-	select GENERIC_GPIO
+	select ARCH_REQUIRE_GPIOLIB
 	help
 	  Support for ST-Ericsson U300 series mobile platforms.
 
Signature · 10 lines
1.7.5.4


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 828 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110905/ab978c83/attachment.sig>
#1 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

Ben Hutchings <[email protected]> writes:

Hi,

Quoted 2 lines

The GPIO implementations for these two machines depend on gpiolib, so they must not select GENERIC_GPIO directly.

They're not calling gpiochip_add afaik so they should really only select GENERIC_GPIO.

Quoted 5 lines

---
This hould fix the build failure seen here:
https://buildd.debian.org/status/fetch.php?pkg=linux-2.6&arch=armel&ver=3.1.0%7Erc4-1%7Eexperimental.1&stamp=1315007168

The problem here is that gpio_request_one has been added to the ads7846 driver but gpio_request_one is not defined in GENERIC_GPIO case (I guess that other (arm and non-arm) platforms may hit similar troubles with gpio_request_one. One quick fix would be to add a gpio_request_one function say in asm-generic/gpio.h. One other fix would be to define gpio_request_one and gpio_request_array in each machine/platform/... specific header. Don't know what's the best solution.

Arnaud

#2 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

Quoted 8 lines · Arnaud Patard

The problem here is that gpio_request_one has been added to the ads7846 driver but gpio_request_one is not defined in GENERIC_GPIO case (I guess that other (arm and non-arm) platforms may hit similar troubles with gpio_request_one. One quick fix would be to add a gpio_request_one function say in asm-generic/gpio.h. One other fix would be to define gpio_request_one and gpio_request_array in each machine/platform/... specific header. Don't know what's the best solution.

It looks like gpio_request_one() and gpio_request_array() have been added at the wrong level in the GPIO support code.

There's two levels to the GPIO support code:

1. GENERIC_GPIO - for platforms which use the gpio_* interfaces.
2. GPIOLIB - for platforms which want the gpio library for handling multiple GPIO controllers.

There is nothing GPIOLIB specific to the gpio_request_*() interfaces, so to put them in gpiolib.c and include/asm-generic/gpio.h seems rather silly.

On the other hand, for the single ARM kernel project, we do need everyone to move to GPIOLIB so that the GPIO calls can be bound appropriately at runtime - which means this problem goes away. So probably the right solution is for IXP4xx etc to move over completely to gpiolib.

#3 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

Russell King - ARM Linux <[email protected]> writes:

Hi,

Quoted 19 lines · Arnaud Patard
Quoted 8 lines

The problem here is that gpio_request_one has been added to the ads7846 driver but gpio_request_one is not defined in GENERIC_GPIO case (I guess that other (arm and non-arm) platforms may hit similar troubles with gpio_request_one. One quick fix would be to add a gpio_request_one function say in asm-generic/gpio.h. One other fix would be to define gpio_request_one and gpio_request_array in each machine/platform/... specific header. Don't know what's the best solution.

It looks like gpio_request_one() and gpio_request_array() have been added at the wrong level in the GPIO support code.

There's two levels to the GPIO support code:

1. GENERIC_GPIO - for platforms which use the gpio_* interfaces.
2. GPIOLIB - for platforms which want the gpio library for handling multiple GPIO controllers.

Quoted 7 lines

There is nothing GPIOLIB specific to the gpio_request_*() interfaces, so to put them in gpiolib.c and include/asm-generic/gpio.h seems rather silly.

On the other hand, for the single ARM kernel project, we do need everyone to move to GPIOLIB so that the GPIO calls can be bound appropriately at runtime - which means this problem goes away. So probably the right solution is for IXP4xx etc to move over completely to gpiolib.

I thought about it but I considered it was too late to do that. I don't think that moving to gpiolib u300/ixp4xx in -rc is a good idea. imho doing it for next kernel is better but this won't solve the build failure. Of course, you can tell me I'm plain wrong and I'll look at converting them to gpiolib asap. Then we'll need some testers as I don't have neither u300 nor ixp4xx systems.

Also, moving u300/ixp4xx to gpiolib will solve the issue for ARM but I fear that other architectures may be affected by similar trouble.

Arnaud

#4 lore raw

[RFC/PATCH] ARM: ixp4xx gpiolib support

In reply to Russell King - ARM Linux

This patch adds gpiolib support for the IXP4xx platform

---
 arch/arm/Kconfig                         |    2 +-
 arch/arm/mach-ixp4xx/common.c            |   39 +++++++++++++++++++++++++
 arch/arm/mach-ixp4xx/include/mach/gpio.h |   46 ++++++++++--------------------
 3 files changed, 55 insertions(+), 32 deletions(-)
arch/arm/Kconfig +1 -1
index 3269576..e793a75 100644
@@ -481,7 +481,7 @@ config ARCH_IXP4XX
 	depends on MMU
 	select CLKSRC_MMIO
 	select CPU_XSCALE
-	select GENERIC_GPIO
+	select ARCH_REQUIRE_GPIOLIB
 	select GENERIC_CLOCKEVENTS
 	select HAVE_SCHED_CLOCK
 	select MIGHT_HAVE_PCI
arch/arm/mach-ixp4xx/common.c +39 -0
index 0777257..7603456 100644
@@ -36,6 +36,7 @@
 #include <asm/page.h>
 #include <asm/irq.h>
 #include <asm/sched_clock.h>
+#include <asm/gpio.h>
 
 #include <asm/mach/map.h>
 #include <asm/mach/irq.h>
@@ -375,12 +376,50 @@ static struct platform_device *ixp46x_devices[] __initdata = {
 unsigned long ixp4xx_exp_bus_size;
 EXPORT_SYMBOL(ixp4xx_exp_bus_size);
 
+static int ixp4xx_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+	gpio_line_config(gpio, IXP4XX_GPIO_IN);
+	return 0;
+}
+
+static int ixp4xx_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int level)
+{
+	gpio_line_set(gpio, level);
+	gpio_line_config(gpio, IXP4XX_GPIO_OUT);
+	return 0;
+}
+
+static int ixp4xx_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
+{
+	int value;
+
+	gpio_line_get(gpio, &value);
+	return value;
+}
+
+static void ixp4xx_gpio_set_value(struct gpio_chip *chip, unsigned gpio, int value)
+{
+	gpio_line_set(gpio, value);
+}
+
+static struct gpio_chip ixp4xx_gpio_chip = {
+	.label			= "IXP4XX_GPIO_CHIP",
+	.direction_input	= ixp4xx_gpio_direction_input,
+	.direction_output	= ixp4xx_gpio_direction_output,
+	.get			= ixp4xx_gpio_get_value,
+	.set			= ixp4xx_gpio_set_value,
+	.base			= 0,
+	.ngpio			= 16,
+};
+
 void __init ixp4xx_sys_init(void)
 {
 	ixp4xx_exp_bus_size = SZ_16M;
 
 	platform_add_devices(ixp4xx_devices, ARRAY_SIZE(ixp4xx_devices));
 
+	gpiochip_add(&ixp4xx_gpio_chip);
+
 	if (cpu_is_ixp46x()) {
 		int region;
 
arch/arm/mach-ixp4xx/include/mach/gpio.h +15 -31

62 lines not on this page · raw message

Signature · 1 line
1.7.1
#5 lore raw

[RFC/PATCH] ARM: ixp4xx gpiolib support

Imre Kaloz <[email protected]> writes:

Hi,

I can't test it but it looks good. Just small nitpicks. See below.

Quoted 62 lines

This patch adds gpiolib support for the IXP4xx platform

---
 arch/arm/Kconfig                         |    2 +-
 arch/arm/mach-ixp4xx/common.c            |   39 +++++++++++++++++++++++++
 arch/arm/mach-ixp4xx/include/mach/gpio.h |   46 ++++++++++--------------------
 3 files changed, 55 insertions(+), 32 deletions(-)
arch/arm/Kconfig +1 -1
index 3269576..e793a75 100644
@@ -481,7 +481,7 @@ config ARCH_IXP4XX
 	depends on MMU
 	select CLKSRC_MMIO
 	select CPU_XSCALE
-	select GENERIC_GPIO
+	select ARCH_REQUIRE_GPIOLIB
 	select GENERIC_CLOCKEVENTS
 	select HAVE_SCHED_CLOCK
 	select MIGHT_HAVE_PCI
arch/arm/mach-ixp4xx/common.c +25 -0
index 0777257..7603456 100644
@@ -36,6 +36,7 @@
 #include <asm/page.h>
 #include <asm/irq.h>
 #include <asm/sched_clock.h>
+#include <asm/gpio.h>
 
 #include <asm/mach/map.h>
 #include <asm/mach/irq.h>
@@ -375,12 +376,50 @@ static struct platform_device *ixp46x_devices[] __initdata = {
 unsigned long ixp4xx_exp_bus_size;
 EXPORT_SYMBOL(ixp4xx_exp_bus_size);
 
+static int ixp4xx_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+	gpio_line_config(gpio, IXP4XX_GPIO_IN);
+	return 0;
+}
+
+static int ixp4xx_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int level)
+{
+	gpio_line_set(gpio, level);
+	gpio_line_config(gpio, IXP4XX_GPIO_OUT);
+	return 0;
+}
+
+static int ixp4xx_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
+{
+	int value;
+
+	gpio_line_get(gpio, &value);
+	return value;
+}
+
+static void ixp4xx_gpio_set_value(struct gpio_chip *chip, unsigned gpio, int value)
+{
+	gpio_line_set(gpio, value);

arch/arm/mach-ixp4xx/common.c static struct platform_device *ixp46x_devices[] __initdata = { · line 412

Quoted 10 lines
arch/arm/mach-ixp4xx/common.c +10 -0
+}
+
+static struct gpio_chip ixp4xx_gpio_chip = {
+	.label			= "IXP4XX_GPIO_CHIP",
+	.direction_input	= ixp4xx_gpio_direction_input,
+	.direction_output	= ixp4xx_gpio_direction_output,
+	.get			= ixp4xx_gpio_get_value,
+	.set			= ixp4xx_gpio_set_value,
+	.base			= 0,
+	.ngpio			= 16,

Use NR_BUILTIN_GPIO instead of 16 ?

Quoted 54 lines
arch/arm/mach-ixp4xx/common.c +4 -0
+};
+
 void __init ixp4xx_sys_init(void)
 {
 	ixp4xx_exp_bus_size = SZ_16M;
 
 	platform_add_devices(ixp4xx_devices, ARRAY_SIZE(ixp4xx_devices));
 
+	gpiochip_add(&ixp4xx_gpio_chip);
+
 	if (cpu_is_ixp46x()) {
 		int region;
 
arch/arm/mach-ixp4xx/include/mach/gpio.h +2 -27
index a5f87de..86f3596 100644
@@ -27,47 +27,31 @@
 
 #include <linux/kernel.h>
 #include <mach/hardware.h>
+#include <asm-generic/gpio.h>			/* cansleep wrappers */
 
-static inline int gpio_request(unsigned gpio, const char *label)
-{
-	return 0;
-}
-
-static inline void gpio_free(unsigned gpio)
-{
-	might_sleep();
-
-	return;
-}
-
-static inline int gpio_direction_input(unsigned gpio)
-{
-	gpio_line_config(gpio, IXP4XX_GPIO_IN);
-	return 0;
-}
-
-static inline int gpio_direction_output(unsigned gpio, int level)
-{
-	gpio_line_set(gpio, level);
-	gpio_line_config(gpio, IXP4XX_GPIO_OUT);
-	return 0;
-}
+#define NR_BUILTIN_GPIO 16
 
 static inline int gpio_get_value(unsigned gpio)
 {
-	int value;
-
-	gpio_line_get(gpio, &value);

arch/arm/mach-ixp4xx/include/mach/gpio.h line 43

Quoted 10 lines
arch/arm/mach-ixp4xx/include/mach/gpio.h +8 -2
-
-	return value;
+	if (gpio < NR_BUILTIN_GPIO)
+	{
+		int value;
+		gpio_line_get(gpio, &value);
+		return value;
+	}
+	else
+		return __gpio_get_value(gpio);

Please use

       if () {
       } else

I would also put the 'int value' declaration outside the if ().

Thanks,
Arnaud

#6 lore raw

[RFC/PATCH] ARM: ixp4xx gpiolib support

In reply to Imre Kaloz

Hello,

Quoted 119 lines · Imre Kaloz

This patch adds gpiolib support for the IXP4xx platform

---
 arch/arm/Kconfig                         |    2 +-
 arch/arm/mach-ixp4xx/common.c            |   39 +++++++++++++++++++++++++
 arch/arm/mach-ixp4xx/include/mach/gpio.h |   46 ++++++++++--------------------
 3 files changed, 55 insertions(+), 32 deletions(-)
arch/arm/Kconfig +1 -1
index 3269576..e793a75 100644
@@ -481,7 +481,7 @@ config ARCH_IXP4XX
 	depends on MMU
 	select CLKSRC_MMIO
 	select CPU_XSCALE
-	select GENERIC_GPIO
+	select ARCH_REQUIRE_GPIOLIB
 	select GENERIC_CLOCKEVENTS
 	select HAVE_SCHED_CLOCK
 	select MIGHT_HAVE_PCI
arch/arm/mach-ixp4xx/common.c +39 -0
index 0777257..7603456 100644
@@ -36,6 +36,7 @@
 #include <asm/page.h>
 #include <asm/irq.h>
 #include <asm/sched_clock.h>
+#include <asm/gpio.h>
 
 #include <asm/mach/map.h>
 #include <asm/mach/irq.h>
@@ -375,12 +376,50 @@ static struct platform_device *ixp46x_devices[] __initdata = {
 unsigned long ixp4xx_exp_bus_size;
 EXPORT_SYMBOL(ixp4xx_exp_bus_size);
 
+static int ixp4xx_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
+{
+	gpio_line_config(gpio, IXP4XX_GPIO_IN);
+	return 0;
+}
+
+static int ixp4xx_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, int level)
+{
+	gpio_line_set(gpio, level);
+	gpio_line_config(gpio, IXP4XX_GPIO_OUT);
+	return 0;
+}
+
+static int ixp4xx_gpio_get_value(struct gpio_chip *chip, unsigned gpio)
+{
+	int value;
+
+	gpio_line_get(gpio, &value);
+	return value;
+}
+
+static void ixp4xx_gpio_set_value(struct gpio_chip *chip, unsigned gpio, int value)
+{
+	gpio_line_set(gpio, value);
+}
+
+static struct gpio_chip ixp4xx_gpio_chip = {
+	.label			= "IXP4XX_GPIO_CHIP",
+	.direction_input	= ixp4xx_gpio_direction_input,
+	.direction_output	= ixp4xx_gpio_direction_output,
+	.get			= ixp4xx_gpio_get_value,
+	.set			= ixp4xx_gpio_set_value,
+	.base			= 0,
+	.ngpio			= 16,
+};
+
 void __init ixp4xx_sys_init(void)
 {
 	ixp4xx_exp_bus_size = SZ_16M;
 
 	platform_add_devices(ixp4xx_devices, ARRAY_SIZE(ixp4xx_devices));
 
+	gpiochip_add(&ixp4xx_gpio_chip);
+
 	if (cpu_is_ixp46x()) {
 		int region;
 
arch/arm/mach-ixp4xx/include/mach/gpio.h +1 -24
index a5f87de..86f3596 100644
@@ -27,47 +27,31 @@
 
 #include <linux/kernel.h>
 #include <mach/hardware.h>
+#include <asm-generic/gpio.h>			/* cansleep wrappers */
 
-static inline int gpio_request(unsigned gpio, const char *label)
-{
-	return 0;
-}
-
-static inline void gpio_free(unsigned gpio)
-{
-	might_sleep();
-
-	return;
-}
-
-static inline int gpio_direction_input(unsigned gpio)
-{
-	gpio_line_config(gpio, IXP4XX_GPIO_IN);
-	return 0;
-}
-
-static inline int gpio_direction_output(unsigned gpio, int level)
-{
-	gpio_line_set(gpio, level);
-	gpio_line_config(gpio, IXP4XX_GPIO_OUT);
-	return 0;
-}

arch/arm/mach-ixp4xx/include/mach/gpio.h line 36

Quoted 10 lines
arch/arm/mach-ixp4xx/include/mach/gpio.h +2 -5
+#define NR_BUILTIN_GPIO 16
 
 static inline int gpio_get_value(unsigned gpio)
 {
-	int value;
-
-	gpio_line_get(gpio, &value);
-
-	return value;
+	if (gpio < NR_BUILTIN_GPIO)

you might want to test the impact of using

	if (__builtin_constant_p(gpio) && gpio < NR_BUILTIN_GPIO)

here. I don't know what to expect from it, but this is the idiom I saw when I implemented gpio stuff some years ago.

arch/arm/mach-ixp4xx/include/mach/gpio.h line 40

Quoted 4 lines
arch/arm/mach-ixp4xx/include/mach/gpio.h +4 -0
+	{
+		int value;
+		gpio_line_get(gpio, &value);
+		return value;

I wonder about the return value of gpio_line_get. If it's void why not change it to return the value instead of using an output parameter.

Quoted 9 lines
arch/arm/mach-ixp4xx/include/mach/gpio.h +4 -1
+	}
+	else
+		return __gpio_get_value(gpio);
 }
 
 static inline void gpio_set_value(unsigned gpio, int value)
 {
-	gpio_line_set(gpio, value);
+	if (gpio < NR_BUILTIN_GPIO)

arch/arm/mach-ixp4xx/include/mach/gpio.h line 57

Quoted 10 lines
arch/arm/mach-ixp4xx/include/mach/gpio.h +4 -1
+		gpio_line_set(gpio, value);
+	else
+		__gpio_set_value(gpio, value);
 }
 
-#include <asm-generic/gpio.h>			/* cansleep wrappers */
+#define gpio_cansleep __gpio_cansleep
 
 extern int gpio_to_irq(int gpio);
 extern int irq_to_gpio(unsigned int irq);

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#7 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

In reply to Ben Hutchings
Quoted 8 lines · Ben Hutchings
(quoted diff) +0 -0
@@ -830,7 +830,7 @@ config ARCH_U300

? ? ? ?select GENERIC_CLOCKEVENTS
? ? ? ?select CLKDEV_LOOKUP
? ? ? ?select HAVE_MACH_CLKDEV
- ? ? ? select GENERIC_GPIO
+ ? ? ? select ARCH_REQUIRE_GPIOLIB
? ? ? ?help
? ? ? ? ?Support for ST-Ericsson U300 series mobile platforms.

Please don't do that, it really is no gpiolib implementation.

In the archives you can find patches I already submitted switching the U300 GPIO over to use gpiolib.

However it fell to the ground due to the shortage of a mechanism in gpiolib to configure pin bias. I could try to hack around it but it would still be a hack :-(

I sent several patches trying to adress that issue, none have been accepted.

So instead I venture into inventing the pin control subsystem which is intended to handle complex controllers like these. It is at version 7 right now.

Thanks,
Linus Walleij

#8 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

Quoted 13 lines · Linus Walleij
Quoted 8 lines · Ben Hutchings
(quoted diff) +0 -0
@@ -830,7 +830,7 @@ config ARCH_U300

? ? ? ?select GENERIC_CLOCKEVENTS
? ? ? ?select CLKDEV_LOOKUP
? ? ? ?select HAVE_MACH_CLKDEV
- ? ? ? select GENERIC_GPIO
+ ? ? ? select ARCH_REQUIRE_GPIOLIB
? ? ? ?help
? ? ? ? ?Support for ST-Ericsson U300 series mobile platforms.

Please don't do that, it really is no gpiolib implementation.

Quoted 2 lines

In the archives you can find patches I already submitted switching the U300 GPIO over to use gpiolib.

[...]

OK. But it doesn't seem to work without gpiolib either:

CC [M] drivers/input/touchscreen/ads7846.o
drivers/input/touchscreen/ads7846.c: In function 'ads7846_setup_pendown':
drivers/input/touchscreen/ads7846.c:970:3: error: implicit declaration of function 'gpio_request_one'

Ben.

Signature · 3 lines
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus
#9 lore raw

PATCH 1/1 arm: ixp4xx, u300: Select ARCH_REQUIRE_GPIOLIB, not GENERIC_GPIO

Quoted 16 lines · Ben Hutchings
Quoted 13 lines · Linus Walleij
Quoted 8 lines · Ben Hutchings
(quoted diff) +0 -0
@@ -830,7 +830,7 @@ config ARCH_U300

? ? ? ?select GENERIC_CLOCKEVENTS
? ? ? ?select CLKDEV_LOOKUP
? ? ? ?select HAVE_MACH_CLKDEV
- ? ? ? select GENERIC_GPIO
+ ? ? ? select ARCH_REQUIRE_GPIOLIB
? ? ? ?help
? ? ? ? ?Support for ST-Ericsson U300 series mobile platforms.

Please don't do that, it really is no gpiolib implementation.

Quoted 2 lines

In the archives you can find patches I already submitted switching the U300 GPIO over to use gpiolib.

Quoted 7 lines

[...]

OK. ?But it doesn't seem to work without gpiolib either:

?CC [M] ?drivers/input/touchscreen/ads7846.o
drivers/input/touchscreen/ads7846.c: In function 'ads7846_setup_pendown':
drivers/input/touchscreen/ads7846.c:970:3: error: implicit declaration of function 'gpio_request_one'

Yes.

I don't know a good way out of this really :-(

I'll start looking for the bad ways out, like hacking the GPIO driver to do GPIOLIB in a not-so-elegant way.

Yours,
Linus Walleij