[PATCH] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[PATCH] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Lars-Peter Clausen]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are not initalized for the s3c2442 cpu type. This results in a NULL-pointer
deref upon calling s3c_gpio_setpull.

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or
pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c2442 cpu init function to the new pulldown helper functions.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
 arch/arm/plat-samsung/gpio-config.c                |   43 ++++++++++++++++---
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 4 files changed, 55 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index cd8e7de..136ebd0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -20,6 +20,7 @@ config CPU_S3C2442
 	bool
 	depends on ARCH_S3C2410
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..0dbc91c 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -44,6 +45,10 @@
 #include <plat/clock.h>
 #include <plat/cpu.h>
 
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
+
 /* S3C2442 extended clock support */
 
 static unsigned long s3c2442_camif_upll_round(struct clk *clk,
@@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
 int __init s3c2442_init(void)
 {
 	printk("S3C2442: Initialising architecture\n");
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
 
 	return sysdev_register(&s3c2442_sysdev);
 }
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index 57b68a5..1abc81e 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -230,16 +230,16 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup = __raw_readl(reg);
 
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pup == updown)
 		pup &= ~(1 << off);
 	else if (pup == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
@@ -250,17 +250,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN) */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 3e21c75..9fd2947 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -204,6 +204,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.5.6.5

[PATCH] ARM: s3c24xx: Set ARCH_NR_GPIOS according to the selected SoC types.

[Lars-Peter Clausen]

Currently the code in gpiolib.c tries to register GPIO BANKA to BANKM.
ARCH_NR_GPIOS on the other hand is set only to 256, which would be the
equivalent of BANKA to BANKH. Thus the registration of all other banks will fail.

This patch fixes this by setting S3C_GPIO_END according to the selected SoC
types. S3C_GPIO_END is set to the maximum of the number of GPIOs over all
selected SoC types. Thus it is ensured that memory is not wasted if support for
SoCs with higher GPIO numbers is not built-in.
When registering the banks it is made sure that banks which are outside of that
range are not even tried to be registered. Otherwise there would a problem with
configs where CONFIG_S3C24XX_GPIO_EXTRA is set to a non zero value.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 arch/arm/mach-s3c2410/include/mach/gpio.h |   25 ++++++++++++-------------
 arch/arm/plat-s3c24xx/gpiolib.c           |    2 ++
 2 files changed, 14 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-s3c2410/include/mach/gpio.h b/arch/arm/mach-s3c2410/include/mach/gpio.h
index b649bf2..80dfe25 100644
--- a/arch/arm/mach-s3c2410/include/mach/gpio.h
+++ b/arch/arm/mach-s3c2410/include/mach/gpio.h
@@ -16,22 +16,21 @@
 #define gpio_cansleep	__gpio_cansleep
 #define gpio_to_irq	__gpio_to_irq
 
-/* some boards require extra gpio capacity to support external
- * devices that need GPIO.
- */
+#include <mach/gpio-fns.h>
+#include <mach/gpio-nrs.h>
 
-#ifdef CONFIG_CPU_S3C244X
-#define ARCH_NR_GPIOS	(32 * 9 + CONFIG_S3C24XX_GPIO_EXTRA)
+#if defined(CPU_S3C2416) || defined(CPU_S3C2443)
+#define S3C_GPIO_END	S3C2410_GPM(S3C2410_GPIO_M_NR)
+#elif defined(CONFIG_CPU_S3C244X)
+#define S3C_GPIO_END	S3C2410_GPJ(S3C2410_GPIO_J_NR)
 #else
-#define ARCH_NR_GPIOS	(256 + CONFIG_S3C24XX_GPIO_EXTRA)
+#define S3C_GPIO_END	S3C2410_GPH(S3C2410_GPIO_H_NR)
 #endif
 
+/* some boards require extra gpio capacity to support external
+ * devices that need GPIO.
+ */
+#define ARCH_NR_GPIOS (S3C_GPIO_END + CONFIG_S3C24XX_GPIO_EXTRA)
+
 #include <asm-generic/gpio.h>
-#include <mach/gpio-nrs.h>
-#include <mach/gpio-fns.h>
 
-#ifdef CONFIG_CPU_S3C24XX
-#define S3C_GPIO_END	(S3C2410_GPIO_BANKJ + 32)
-#else
-#define S3C_GPIO_END	(S3C2410_GPIO_BANKH + 32)
-#endif
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 4c0896f..65b6126 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -221,6 +221,8 @@ static __init int s3c24xx_gpiolib_init(void)
 	int gpn;
 
 	for (gpn = 0; gpn < ARRAY_SIZE(s3c24xx_gpios); gpn++, chip++) {
+		if (chip->chip.base >= S3C_GPIO_END)
+			break;
 		if (!chip->config)
 			chip->config = &s3c24xx_gpiocfg_default;
 
-- 
1.5.6.5

[PATCH] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Ben Dooks]

On 20/09/10 23:00, Lars-Peter Clausen wrote:
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
> are not initalized for the s3c2442 cpu type. This results in a NULL-pointer
> deref upon calling s3c_gpio_setpull.
> 
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
> to take an additional parameter deciding whether the pin has a pullup or
> pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> 
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
> in the s3c2442 cpu init function to the new pulldown helper functions.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Initial reading looks ok, will see about adding it to my -next tree.

[PATCH] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Abdoulaye Walsimou GAYE]

On 09/21/2010 01:41 AM, Ben Dooks wrote:
> On 20/09/10 23:00, Lars-Peter Clausen wrote:
>    
>> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
>> are not initalized for the s3c2442 cpu type. This results in a NULL-pointer
>> deref upon calling s3c_gpio_setpull.
>>
>> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
>> The method of controlling them is the same though.
>> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
>> to take an additional parameter deciding whether the pin has a pullup or
>> pulldown.
>> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
>> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
>>
>> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
>> in the s3c2442 cpu init function to the new pulldown helper functions.
>>
>> Signed-off-by: Lars-Peter Clausen<lars@metafoo.de>
>>      
> Initial reading looks ok, will see about adding it to my -next tree.
>    


Hello,
It is very urgent to get rid on these issues in gpiolib, I think they 
prevent many
s3c24xx based boards to boot and I reported it 3 months ago.
So fixes in this area should not go in any kind on of -next branch but 
instead -urgent-fixes.

Best regards,
AWG

[PATCH v2] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error:

arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c2442 cpu init function to the new pulldown helper functions.

Based on patch from Lars-Peter Clausen <lars@metafoo.de>

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: adapted patch for 2.6.37-rc1

 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
 arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
 arch/arm/plat-samsung/gpio-config.c                |   44 ++++++++++++++++---
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 5 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
 config CPU_S3C2442
 	bool
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..0dbc91c 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -44,6 +45,10 @@
 #include <plat/clock.h>
 #include <plat/cpu.h>
 
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
+
 /* S3C2442 extended clock support */
 
 static unsigned long s3c2442_camif_upll_round(struct clk *clk,
@@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
 int __init s3c2442_init(void)
 {
 	printk("S3C2442: Initialising architecture\n");
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
 
 	return sysdev_register(&s3c2442_sysdev);
 }
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 24c6f5a..243b641 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,6 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
 struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
 	.set_config	= s3c_gpio_setcfg_s3c24xx,
 	.get_config	= s3c_gpio_getcfg_s3c24xx,
-	.set_pull	= s3c_gpio_setpull_1up,
-	.get_pull	= s3c_gpio_getpull_1up,
 };
 
 struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index b732b77..ac7f13f 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,16 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull,
+			 s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup = __raw_readl(reg);
 
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pup == updown)
 		pup &= ~(1 << off);
 	else if (pup == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
@@ -300,17 +301,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 8fd65d8..0d2c570 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.7.3.2

[PATCH v2] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Abdoulaye Walsimou GAYE]

On 11/06/2010 08:54 AM, Vasily Khoruzhick wrote:
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> error:
>
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
>
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
> to take an additional parameter deciding whether the pin has a pullup or pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
>
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
> in the s3c2442 cpu init function to the new pulldown helper functions.
>
> Based on patch from Lars-Peter Clausen<lars@metafoo.de>
>
> Signed-off-by: Vasily Khoruzhick<anarsoul@gmail.com>
> ---
>
>    

[...]
> diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
> index 24c6f5a..243b641 100644
> --- a/arch/arm/plat-s3c24xx/gpiolib.c
> +++ b/arch/arm/plat-s3c24xx/gpiolib.c
> @@ -82,8 +82,6 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
>   struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
>   	.set_config	= s3c_gpio_setcfg_s3c24xx,
>   	.get_config	= s3c_gpio_getcfg_s3c24xx,
> -	.set_pull	= s3c_gpio_setpull_1up,
> -	.get_pull	= s3c_gpio_getpull_1up,
>   };
>
>    

This part of your patch will prevent s3c2440 based boards to boot see [1]

[..]

[1] 
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=bdf5005b738c1542a30b41a83069329313fc61f6

Best regards,
AWG

[PATCH v2] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

On Saturday 06 November 2010 18:09:37 Abdoulaye Walsimou GAYE wrote:

Restored CC list, please don't clean it next time.

> > -	.set_pull	= s3c_gpio_setpull_1up,
> > -	.get_pull	= s3c_gpio_getpull_1up,
> > 
> >   };
> 
> This part of your patch will prevent s3c2440 based boards to boot see [1]

Well, your patch breaks compilation for s3c2442 at all (see comment from my 
patch)

Ben, why machine-specific init is called before cpu init?

Regards
Vasily

[PATCH v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when compiling kernel for s3c2442:

arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c2442 cpu init function to the new pulldown helper functions.

Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
 arch/arm/plat-s3c24xx/gpiolib.c                    |    9 +++-
 arch/arm/plat-samsung/gpio-config.c                |   44 ++++++++++++++++---
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 5 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
 config CPU_S3C2442
 	bool
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..0dbc91c 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -44,6 +45,10 @@
 #include <plat/clock.h>
 #include <plat/cpu.h>
 
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
+
 /* S3C2442 extended clock support */
 
 static unsigned long s3c2442_camif_upll_round(struct clk *clk,
@@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
 int __init s3c2442_init(void)
 {
 	printk("S3C2442: Initialising architecture\n");
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
 
 	return sysdev_register(&s3c2442_sysdev);
 }
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 24c6f5a..b805dab 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,13 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
 struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
 	.set_config	= s3c_gpio_setcfg_s3c24xx,
 	.get_config	= s3c_gpio_getcfg_s3c24xx,
-	.set_pull	= s3c_gpio_setpull_1up,
-	.get_pull	= s3c_gpio_getpull_1up,
+#if defined(CONFIG_S3C_GPIO_PULL_UP)
+	.set_pull       = s3c_gpio_setpull_1up,
+	.get_pull       = s3c_gpio_getpull_1up,
+#elif defined(CONFIG_S3C_GPIO_PULL_DOWN)
+	.set_pull	= s3c_gpio_setpull_1down,
+	.get_pull	= s3c_gpio_getpull_1down,
+#endif
 };
 
 struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index b732b77..ac7f13f 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,16 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull,
+			 s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup = __raw_readl(reg);
 
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pup == updown)
 		pup &= ~(1 << off);
 	else if (pup == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
@@ -300,17 +301,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 8fd65d8..0d2c570 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.7.3.2

[PATCH v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Abdoulaye Walsimou GAYE]

On 11/08/2010 09:26 PM, Vasily Khoruzhick wrote:
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> error when compiling kernel for s3c2442:
>
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
>
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
> to take an additional parameter deciding whether the pin has a pullup or pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
>
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
> in the s3c2442 cpu init function to the new pulldown helper functions.
>
> Based on patch from "Lars-Peter Clausen"<lars@metafoo.de>
>
> Signed-off-by: Vasily Khoruzhick<anarsoul@gmail.com>
> ---
> v2: adapt patch for 2.6.37-rc1
> v3: restore default pull callbacks, add default pull callbacks for s3c2442
>   arch/arm/mach-s3c2440/Kconfig                      |    1 +
>   arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
>   arch/arm/plat-s3c24xx/gpiolib.c                    |    9 +++-
>   arch/arm/plat-samsung/gpio-config.c                |   44 ++++++++++++++++---
>   .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
>   5 files changed, 63 insertions(+), 9 deletions(-)
>
>    

Ackey-by: Abdoulaye Walsimou GAYE <awg@embtoolkit.org>

Thanks

[PATCH v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

On Monday 08 November 2010 22:26:23 Vasily Khoruzhick wrote:
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
> structure are initalized via s3c_gpio_{get,set}pull_1up. This results in a
> linker error when compiling kernel for s3c2442:
> 
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
> 
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
> functions to take an additional parameter deciding whether the pin has a
> pullup or pulldown. The s3c_gpio_{get,set}pull_1{down,up} functions then
> wrap that functions passing either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> 
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
> fields in the s3c2442 cpu init function to the new pulldown helper
> functions.
> 
> Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Hi there,

Can any samsung-soc maintainer review/merge this patch?

I should admit that merging patches through samsung maintainers became pretty 
impossible - response latency is about 1 month, some patches (not mine) are 
still pending. I've sent first version of this patch on 6th of November, few 
weeks of standby - and we'll get 2.6.37 with broken s3c2442 support. Yeah, I 
know that maintainers are living people too, and they need to pay attention to 
real life, work, family, etc, but now merging some samsung-soc-related patch 
is really painfull and slow proccess, and it would be nice if it could be 
somehow improved.

Regards
Vasily

[PATCH v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Kukjin Kim]

Vasily Khoruzhick wrote:
> 
> On Monday 08 November 2010 22:26:23 Vasily Khoruzhick wrote:
> > Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
> > structure are initalized via s3c_gpio_{get,set}pull_1up. This results in a
> > linker error when compiling kernel for s3c2442:
> >
> > arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> > `s3c_gpio_getpull_1up'
> > arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> > `s3c_gpio_setpull_1up'
> >
> > The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> > The method of controlling them is the same though.
> > So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
> > functions to take an additional parameter deciding whether the pin has a
> > pullup or pulldown. The s3c_gpio_{get,set}pull_1{down,up} functions then
> > wrap that functions passing either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> >
> > Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
> > fields in the s3c2442 cpu init function to the new pulldown helper
> > functions.
> >
> > Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
> 
> Hi there,
> 
> Can any samsung-soc maintainer review/merge this patch?
> 
> I should admit that merging patches through samsung maintainers became pretty
> impossible - response latency is about 1 month, some patches (not mine) are
> still pending. I've sent first version of this patch on 6th of November, few
> weeks of standby - and we'll get 2.6.37 with broken s3c2442 support. Yeah, I
> know that maintainers are living people too, and they need to pay attention
> to
> real life, work, family, etc, but now merging some samsung-soc-related patch
> is really painfull and slow proccess, and it would be nice if it could be
> somehow improved.
> 
Hi,

Actually, I didn't get your patch through my e-mail.
Maybe you missed my e-mail in Cc at that time and I missed your patch in mailing list.
Could you please re-send it so that can get the patch via e-mail?

Now, thanks for adding me, will review...

I think, we have some time to fix it for 37, so don't worry about broken 37-stable.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when compiling kernel for s3c2442:

arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c2442 cpu init function to the new pulldown helper functions.

Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
 arch/arm/plat-s3c24xx/gpiolib.c                    |    9 +++-
 arch/arm/plat-samsung/gpio-config.c                |   44 ++++++++++++++++---
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 5 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
 config CPU_S3C2442
 	bool
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..0dbc91c 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -44,6 +45,10 @@
 #include <plat/clock.h>
 #include <plat/cpu.h>
 
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
+
 /* S3C2442 extended clock support */
 
 static unsigned long s3c2442_camif_upll_round(struct clk *clk,
@@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
 int __init s3c2442_init(void)
 {
 	printk("S3C2442: Initialising architecture\n");
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
 
 	return sysdev_register(&s3c2442_sysdev);
 }
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 24c6f5a..b805dab 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,13 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
 struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
 	.set_config	= s3c_gpio_setcfg_s3c24xx,
 	.get_config	= s3c_gpio_getcfg_s3c24xx,
-	.set_pull	= s3c_gpio_setpull_1up,
-	.get_pull	= s3c_gpio_getpull_1up,
+#if defined(CONFIG_S3C_GPIO_PULL_UP)
+	.set_pull       = s3c_gpio_setpull_1up,
+	.get_pull       = s3c_gpio_getpull_1up,
+#elif defined(CONFIG_S3C_GPIO_PULL_DOWN)
+	.set_pull	= s3c_gpio_setpull_1down,
+	.get_pull	= s3c_gpio_getpull_1down,
+#endif
 };
 
 struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index b732b77..ac7f13f 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,16 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull,
+			 s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup = __raw_readl(reg);
 
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pup == updown)
 		pup &= ~(1 << off);
 	else if (pup == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
@@ -300,17 +301,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 8fd65d8..0d2c570 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.7.3.2

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Kukjin Kim]

Vasily Khoruzhick wrote:
> 
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
> structure
> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> error when compiling kernel for s3c2442:
> 
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
> 
Yeah, happened build error when selected only S3C2442.

> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
> functions
> to take an additional parameter deciding whether the pin has a pullup or
> pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions
> passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> 
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
> fields
> in the s3c2442 cpu init function to the new pulldown helper functions.
> 
> Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> v2: adapt patch for 2.6.37-rc1
> v3: restore default pull callbacks, add default pull callbacks for s3c2442
>  arch/arm/mach-s3c2440/Kconfig                      |    1 +
>  arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
>  arch/arm/plat-s3c24xx/gpiolib.c                    |    9 +++-
>  arch/arm/plat-samsung/gpio-config.c                |   44
++++++++++++++++--
> -
>  .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
>  5 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
> index ff024a6..478fae0 100644
> --- a/arch/arm/mach-s3c2440/Kconfig
> +++ b/arch/arm/mach-s3c2440/Kconfig
> @@ -18,6 +18,7 @@ config CPU_S3C2440
>  config CPU_S3C2442
>  	bool
>  	select CPU_ARM920T
> +	select S3C_GPIO_PULL_DOWN
>  	select S3C2410_CLOCK
>  	select S3C2410_GPIO
>  	select S3C2410_PM if PM
> diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-
> s3c2440/s3c2442.c
> index 188ad1e..0dbc91c 100644
> --- a/arch/arm/mach-s3c2440/s3c2442.c
> +++ b/arch/arm/mach-s3c2440/s3c2442.c
> @@ -32,6 +32,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
>  #include <linux/mutex.h>
> +#include <linux/gpio.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> 
> @@ -44,6 +45,10 @@
>  #include <plat/clock.h>
>  #include <plat/cpu.h>
> 
> +#include <plat/gpio-core.h>
> +#include <plat/gpio-cfg.h>
> +#include <plat/gpio-cfg-helpers.h>
> +
>  /* S3C2442 extended clock support */
> 
>  static unsigned long s3c2442_camif_upll_round(struct clk *clk,
> @@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
>  int __init s3c2442_init(void)
>  {
>  	printk("S3C2442: Initialising architecture\n");

To add empty line would be helpful to read here.

> +	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
> +	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
> 
Right now, this is ok to me...but I think we need to sort this out with
other S3C SoCs.

>  	return sysdev_register(&s3c2442_sysdev);
>  }
> diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-
> s3c24xx/gpiolib.c
> index 24c6f5a..b805dab 100644
> --- a/arch/arm/plat-s3c24xx/gpiolib.c
> +++ b/arch/arm/plat-s3c24xx/gpiolib.c
> @@ -82,8 +82,13 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
>  struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
>  	.set_config	= s3c_gpio_setcfg_s3c24xx,
>  	.get_config	= s3c_gpio_getcfg_s3c24xx,
> -	.set_pull	= s3c_gpio_setpull_1up,
> -	.get_pull	= s3c_gpio_getpull_1up,
> +#if defined(CONFIG_S3C_GPIO_PULL_UP)
> +	.set_pull       = s3c_gpio_setpull_1up,
> +	.get_pull       = s3c_gpio_getpull_1up,
                 ^^^^^^
Why do you use white space instead of tab?
Original code used tab in there.

> +#elif defined(CONFIG_S3C_GPIO_PULL_DOWN)
> +	.set_pull	= s3c_gpio_setpull_1down,
> +	.get_pull	= s3c_gpio_getpull_1down,
> +#endif

Yeah, this is needed for avoiding build error with only S3C2442.
But please remember, now, its default is CONFIG_S3C_GPIO_PULL_UP when used
s3c2410_defconfig even though selected S3C_GPIO_PULL_DOWN together.

I will thinking about better method for single binary kernel of S3C24XX. As
you know, current your method can not support it.

>  };
> 
>  struct s3c_gpio_chip s3c24xx_gpios[] = {
> diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-
> samsung/gpio-config.c
> index b732b77..ac7f13f 100644
> --- a/arch/arm/plat-samsung/gpio-config.c
> +++ b/arch/arm/plat-samsung/gpio-config.c
> @@ -280,16 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct
> s3c_gpio_chip *chip,
>  }
>  #endif
> 
> -#ifdef CONFIG_S3C_GPIO_PULL_UP
> -int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
> -			 unsigned int off, s3c_gpio_pull_t pull)
> +#if defined(CONFIG_S3C_GPIO_PULL_UP) ||
defined(CONFIG_S3C_GPIO_PULL_DOWN)
> +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull,
> +			 s3c_gpio_pull_t updown)

Hmm...how about s3c_gpio_setpull_1updown(...)?
And actually, not used 3rd argument, "pull" now.
I prefer follwoing.

+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			      unsigned int off, s3c_gpio_pull_t pull)

>  {
>  	void __iomem *reg = chip->base + 0x08;
>  	u32 pup = __raw_readl(reg);
> 
>  	pup = __raw_readl(reg);

Hmm...need twice read?

> 
> -	if (pup == S3C_GPIO_PULL_UP)
> +	if (pup == updown)
>  		pup &= ~(1 << off);
>  	else if (pup == S3C_GPIO_PULL_NONE)
>  		pup |= (1 << off);

Is this right? I think, your code is wrong :-(
Of course, original code has bug too...

Should be like follwoing.
	
+	pup >>= off;
+
+	if ((pull == S3C_GPIO_PULL_UP) || (pull == S3C_GPIO_PULL_DOWN))
+		pup &= ~(1 << off);
+	else if (pull == S3C_GPIO_PULL_NONE)
+		pup |= (1 << off);
+	else
+		return -EINVAL;

	...
But, according to your patch, maybe not called "else if" and "else".
Because you only used "S3C_GPIO_PULL_UP" and "S3C_GPIO_PULL_DOWN" as
argument from caller function.

So should be separate it as Ben's original code.

> @@ -300,17 +301,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
>  	return 0;
>  }
> 
> -s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
> -				     unsigned int off)
> +static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
> +				     unsigned int off, s3c_gpio_pull_t
updown)

s3c_gpio_getpull_1updown(...)?
And...why need 3rd argument, "updown"

+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+					  unsigned int off)

>  {
>  	void __iomem *reg = chip->base + 0x08;
>  	u32 pup = __raw_readl(reg);
> 
>  	pup &= (1 << off);
> -	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
> +	return pup ? S3C_GPIO_PULL_NONE : updown;

Is this right?
Hmm...No...

> +}
> +#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
> +
> +#ifdef CONFIG_S3C_GPIO_PULL_UP
> +s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
> +				     unsigned int off)
> +{
> +	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
> +}
> +
> +int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull)
> +{
> +	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
>  }
>  #endif /* CONFIG_S3C_GPIO_PULL_UP */
> 
> +#ifdef CONFIG_S3C_GPIO_PULL_DOWN
> +s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
> +				     unsigned int off)
> +{
> +	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
> +}
> +
> +int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull)
> +{
> +	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
> +}
> +#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
> +
> +

No need 2 empty lines.

>  #ifdef CONFIG_S5P_GPIO_DRVSTR
>  s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
>  {
> diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> index 8fd65d8..0d2c570 100644
> --- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> +++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> @@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct
> s3c_gpio_chip *chip,
>  					    unsigned int off);
> 
>  /**
> + * s3c_gpio_getpull_1down() - Get configuration for choice of down or
none
> + * @chip: The gpio chip that the GPIO pin belongs to
> + * @off: The offset to the pin to get the configuration of.
> + *
> + * This helper function reads the state of the pull-down resistor for the
> + * given GPIO in the same case as s3c_gpio_setpull_1down.
> +*/
> +extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
> +					    unsigned int off);
> +

Need to add "extern int s3c_gpio_setpull_1down(...);"

> +/**
>   * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
>   * @chip: The gpio chip that is being configured.
>   * @off: The offset for the GPIO being configured.
> --

Please make sure your code has no problem again before submitting.
However, your pointing out is right. Should be fixed this...

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Lars-Peter Clausen]

On 11/30/2010 07:18 AM, Kukjin Kim wrote:
> Vasily Khoruzhick wrote:
>>
>> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
>> structure
>> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
>> error when compiling kernel for s3c2442:
>>
>> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
>> `s3c_gpio_getpull_1up'
>> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
>> `s3c_gpio_setpull_1up'
>>
> Yeah, happened build error when selected only S3C2442.
> 
>> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
>> The method of controlling them is the same though.
>> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
>> functions
>> to take an additional parameter deciding whether the pin has a pullup or
>> pulldown.
>> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions
>> passing
>> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
>>
>> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
>> fields
>> in the s3c2442 cpu init function to the new pulldown helper functions.
>>
>> Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
>>
>> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>> ---
>> v2: adapt patch for 2.6.37-rc1
>> v3: restore default pull callbacks, add default pull callbacks for s3c2442
>>  arch/arm/mach-s3c2440/Kconfig                      |    1 +
>>  arch/arm/mach-s3c2440/s3c2442.c                    |    7 +++
>>  arch/arm/plat-s3c24xx/gpiolib.c                    |    9 +++-
>>  arch/arm/plat-samsung/gpio-config.c                |   44
> ++++++++++++++++--
>> -
>>  .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
>>  5 files changed, 63 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
>> index ff024a6..478fae0 100644
>> --- a/arch/arm/mach-s3c2440/Kconfig
>> +++ b/arch/arm/mach-s3c2440/Kconfig
>> @@ -18,6 +18,7 @@ config CPU_S3C2440
>>  config CPU_S3C2442
>>  	bool
>>  	select CPU_ARM920T
>> +	select S3C_GPIO_PULL_DOWN
>>  	select S3C2410_CLOCK
>>  	select S3C2410_GPIO
>>  	select S3C2410_PM if PM
>> diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-
>> s3c2440/s3c2442.c
>> index 188ad1e..0dbc91c 100644
>> --- a/arch/arm/mach-s3c2440/s3c2442.c
>> +++ b/arch/arm/mach-s3c2440/s3c2442.c
>> @@ -32,6 +32,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/ioport.h>
>>  #include <linux/mutex.h>
>> +#include <linux/gpio.h>
>>  #include <linux/clk.h>
>>  #include <linux/io.h>
>>
>> @@ -44,6 +45,10 @@
>>  #include <plat/clock.h>
>>  #include <plat/cpu.h>
>>
>> +#include <plat/gpio-core.h>
>> +#include <plat/gpio-cfg.h>
>> +#include <plat/gpio-cfg-helpers.h>
>> +
>>  /* S3C2442 extended clock support */
>>
>>  static unsigned long s3c2442_camif_upll_round(struct clk *clk,
>> @@ -160,6 +165,8 @@ static struct sys_device s3c2442_sysdev = {
>>  int __init s3c2442_init(void)
>>  {
>>  	printk("S3C2442: Initialising architecture\n");
> 
> To add empty line would be helpful to read here.
> 
>> +	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
>> +	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
>>
> Right now, this is ok to me...but I think we need to sort this out with
> other S3C SoCs.
> 
>>  	return sysdev_register(&s3c2442_sysdev);
>>  }
>> diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-
>> s3c24xx/gpiolib.c
>> index 24c6f5a..b805dab 100644
>> --- a/arch/arm/plat-s3c24xx/gpiolib.c
>> +++ b/arch/arm/plat-s3c24xx/gpiolib.c
>> @@ -82,8 +82,13 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
>>  struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
>>  	.set_config	= s3c_gpio_setcfg_s3c24xx,
>>  	.get_config	= s3c_gpio_getcfg_s3c24xx,
>> -	.set_pull	= s3c_gpio_setpull_1up,
>> -	.get_pull	= s3c_gpio_getpull_1up,
>> +#if defined(CONFIG_S3C_GPIO_PULL_UP)
>> +	.set_pull       = s3c_gpio_setpull_1up,
>> +	.get_pull       = s3c_gpio_getpull_1up,
>                  ^^^^^^
> Why do you use white space instead of tab?
> Original code used tab in there.
> 
>> +#elif defined(CONFIG_S3C_GPIO_PULL_DOWN)
>> +	.set_pull	= s3c_gpio_setpull_1down,
>> +	.get_pull	= s3c_gpio_getpull_1down,
>> +#endif
> 
> Yeah, this is needed for avoiding build error with only S3C2442.
> But please remember, now, its default is CONFIG_S3C_GPIO_PULL_UP when used
> s3c2410_defconfig even though selected S3C_GPIO_PULL_DOWN together.
> 
> I will thinking about better method for single binary kernel of S3C24XX. As
> you know, current your method can not support it.
> 
>>  };
>>
>>  struct s3c_gpio_chip s3c24xx_gpios[] = {
>> diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-
>> samsung/gpio-config.c
>> index b732b77..ac7f13f 100644
>> --- a/arch/arm/plat-samsung/gpio-config.c
>> +++ b/arch/arm/plat-samsung/gpio-config.c
>> @@ -280,16 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct
>> s3c_gpio_chip *chip,
>>  }
>>  #endif
>>
>> -#ifdef CONFIG_S3C_GPIO_PULL_UP
>> -int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
>> -			 unsigned int off, s3c_gpio_pull_t pull)
>> +#if defined(CONFIG_S3C_GPIO_PULL_UP) ||
> defined(CONFIG_S3C_GPIO_PULL_DOWN)
>> +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
>> +			 unsigned int off, s3c_gpio_pull_t pull,
>> +			 s3c_gpio_pull_t updown)
> 
> Hmm...how about s3c_gpio_setpull_1updown(...)?
> And actually, not used 3rd argument, "pull" now.
> I prefer follwoing.
> 

You need the 4th arguemnt, because the s3c2440 only supports pullups and the s3c2442
only supports pulldowns. So you want to return -EINVAL if somebody tries to set a
pullup on a s3c2442 based board.
Your proposed solution would return 0 and set a pulldown instead.


> +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> +			      unsigned int off, s3c_gpio_pull_t pull)
> 
>>  {
>>  	void __iomem *reg = chip->base + 0x08;
>>  	u32 pup = __raw_readl(reg);
>>
>>  	pup = __raw_readl(reg);
> 
> Hmm...need twice read?
> 
>>
>> -	if (pup == S3C_GPIO_PULL_UP)
>> +	if (pup == updown)
>>  		pup &= ~(1 << off);
>>  	else if (pup == S3C_GPIO_PULL_NONE)
>>  		pup |= (1 << off);
> 
> Is this right? I think, your code is wrong :-(
> Of course, original code has bug too...
> 
> Should be like follwoing.
> 	
> +	pup >>= off;
> +
> +	if ((pull == S3C_GPIO_PULL_UP) || (pull == S3C_GPIO_PULL_DOWN))
> +		pup &= ~(1 << off);
> +	else if (pull == S3C_GPIO_PULL_NONE)
> +		pup |= (1 << off);
> +	else
> +		return -EINVAL;
> 
> 	...
> But, according to your patch, maybe not called "else if" and "else".
> Because you only used "S3C_GPIO_PULL_UP" and "S3C_GPIO_PULL_DOWN" as
> argument from caller function.
> 
> So should be separate it as Ben's original code.
> 
>> @@ -300,17 +301,46 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
>>  	return 0;
>>  }
>>
>> -s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
>> -				     unsigned int off)
>> +static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
>> +				     unsigned int off, s3c_gpio_pull_t
> updown)
> 
> s3c_gpio_getpull_1updown(...)?
> And...why need 3rd argument, "updown"

Same reason as above.

> 
> +static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
> +					  unsigned int off)
> 
>>  {
>>  	void __iomem *reg = chip->base + 0x08;
>>  	u32 pup = __raw_readl(reg);
>>
>>  	pup &= (1 << off);
>> -	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
>> +	return pup ? S3C_GPIO_PULL_NONE : updown;
> 
> Is this right?
> Hmm...No...
Why do you think it is wrong? As far as I can see it is correct.

> 
>> +}
>> +#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
>> +
>> +#ifdef CONFIG_S3C_GPIO_PULL_UP
>> +s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
>> +				     unsigned int off)
>> +{
>> +	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
>> +}
>> +
>> +int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
>> +			 unsigned int off, s3c_gpio_pull_t pull)
>> +{
>> +	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
>>  }
>>  #endif /* CONFIG_S3C_GPIO_PULL_UP */
>>
>> +#ifdef CONFIG_S3C_GPIO_PULL_DOWN
>> +s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
>> +				     unsigned int off)
>> +{
>> +	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
>> +}
>> +
>> +int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
>> +			 unsigned int off, s3c_gpio_pull_t pull)
>> +{
>> +	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
>> +}
>> +#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
>> +
>> +
> 
> No need 2 empty lines.
> 
>>  #ifdef CONFIG_S5P_GPIO_DRVSTR
>>  s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
>>  {
>> diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
>> b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
>> index 8fd65d8..0d2c570 100644
>> --- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
>> +++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
>> @@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct
>> s3c_gpio_chip *chip,
>>  					    unsigned int off);
>>
>>  /**
>> + * s3c_gpio_getpull_1down() - Get configuration for choice of down or
> none
>> + * @chip: The gpio chip that the GPIO pin belongs to
>> + * @off: The offset to the pin to get the configuration of.
>> + *
>> + * This helper function reads the state of the pull-down resistor for the
>> + * given GPIO in the same case as s3c_gpio_setpull_1down.
>> +*/
>> +extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
>> +					    unsigned int off);
>> +
> 
> Need to add "extern int s3c_gpio_setpull_1down(...);"

It is already present in the current code.

> 
>> +/**
>>   * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
>>   * @chip: The gpio chip that is being configured.
>>   * @off: The offset for the GPIO being configured.
>> --
> 
> Please make sure your code has no problem again before submitting.
> However, your pointing out is right. Should be fixed this...
> 
> Thanks.
> 
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
> 

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Tuesday 30 November 2010 13:53:43 Lars-Peter Clausen wrote:

> > Hmm...how about s3c_gpio_setpull_1updown(...)?
> > And actually, not used 3rd argument, "pull" now.
> > I prefer follwoing.
> 
> You need the 4th arguemnt, because the s3c2440 only supports pullups and
> the s3c2442 only supports pulldowns. So you want to return -EINVAL if
> somebody tries to set a pullup on a s3c2442 based board.
> Your proposed solution would return 0 and set a pulldown instead.

Well, at least it allows single-binary kernel for s3c24xx to exist. I think 
it's OK, as setting pull{up,down} bit for any non S3C_GPIO_PULL_NONE arg 
preserves semantics for all SoCs (s3c2410/s3c2440/s3c2442) by cost of not 
handling errors. Anyway, who wants to call cfgpull with S3C_GPIO_PULL_DOWN on 
s3c2410/s3c2440 or with S3C_GPIO_PULL_UP on s3c2442?
 
Regards
Vasily

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Lars-Peter Clausen]

On 11/30/2010 01:01 PM, Vasily Khoruzhick wrote:
> On Tuesday 30 November 2010 13:53:43 Lars-Peter Clausen wrote:
> 
>>> Hmm...how about s3c_gpio_setpull_1updown(...)?
>>> And actually, not used 3rd argument, "pull" now.
>>> I prefer follwoing.
>>
>> You need the 4th arguemnt, because the s3c2440 only supports pullups and
>> the s3c2442 only supports pulldowns. So you want to return -EINVAL if
>> somebody tries to set a pullup on a s3c2442 based board.
>> Your proposed solution would return 0 and set a pulldown instead.
> 
> Well, at least it allows single-binary kernel for s3c24xx to exist. I think 
> it's OK, as setting pull{up,down} bit for any non S3C_GPIO_PULL_NONE arg 
> preserves semantics for all SoCs (s3c2410/s3c2440/s3c2442) by cost of not 
> handling errors. Anyway, who wants to call cfgpull with S3C_GPIO_PULL_DOWN on 
> s3c2410/s3c2440 or with S3C_GPIO_PULL_UP on s3c2442?
>  
> Regards
> Vasily

Hi

While this might work for setting the pullup, what to you want to return in get_pull?

The reason why s3c24xx_gpiocfg_default needs to have {get,set}_pull set at compile
time is that the board init code is called before the cpu init code. Which is in my
opinion a bit odd and should be fixed instead.
If it is not fixed for whatever reason we could fallback to using some sort of
"cpu_is_s3c2442() ? S3C_GPIO_PULL_UP : S3C_GPIO_PULL_DOWN"

- Lars

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Tuesday 30 November 2010 15:12:37 Lars-Peter Clausen wrote:

> Hi
> 
> While this might work for setting the pullup, what to you want to return in
> get_pull?

Some custom value like S3C_GPIO_PULL_ENABLED?
 
> The reason why s3c24xx_gpiocfg_default needs to have {get,set}_pull set at
> compile time is that the board init code is called before the cpu init
> code. Which is in my opinion a bit odd and should be fixed instead.

That's because cpu init code is arch_initcall. Kernel calls mdesc-
>init_machine before any arch_initcall function, not sure if it can be fixed 
without massive rework of existing code.

> If it is not fixed for whatever reason we could fallback to using some sort
> of "cpu_is_s3c2442() ? S3C_GPIO_PULL_UP : S3C_GPIO_PULL_DOWN"

AFAIK, Ben does not like runtime CPUtype checks

Regards
Vasily

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Lars-Peter Clausen]

On 11/30/2010 03:33 PM, Vasily Khoruzhick wrote:
> On Tuesday 30 November 2010 15:12:37 Lars-Peter Clausen wrote:
> 
>> Hi
>>
>> While this might work for setting the pullup, what to you want to return in
>> get_pull?
> 
> Some custom value like S3C_GPIO_PULL_ENABLED?
Well, or we could use a custom setter and getter functions for configuring the
pull-ups/downs.
>  
>> The reason why s3c24xx_gpiocfg_default needs to have {get,set}_pull set at
>> compile time is that the board init code is called before the cpu init
>> code. Which is in my opinion a bit odd and should be fixed instead.
> 
> That's because cpu init code is arch_initcall. Kernel calls mdesc-
>> init_machine before any arch_initcall function, not sure if it can be fixed 
> without massive rework of existing code.

Both the cpu init code and the machine init code are run at arch_initcall.

> 
>> If it is not fixed for whatever reason we could fallback to using some sort
>> of "cpu_is_s3c2442() ? S3C_GPIO_PULL_UP : S3C_GPIO_PULL_DOWN"
> 
> AFAIK, Ben does not like runtime CPUtype checks
> 
I prefer it over broken code. And you would only need it until the cpu init functions
have been run.

You would add a wrapper like:
s3c24xx_set_pull(...)
{
	if (cpu_is_s3c2442())
		s3c_gpio_setpull_1down(...)
	else
		s3c_gpio_setpull_1up(...)
}

And then set s3c24xx_gpiocfg_default.{set,get}_pull to those at compile time. And
once the cpu init code runs replace them with either s3c_gpio_setpull_1down or
s3c_gpio_setpull_1up depending on the cpu.

- Lars

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Tuesday 30 November 2010 16:53:09 Lars-Peter Clausen wrote:

> Both the cpu init code and the machine init code are run at arch_initcall.

Yeah, you're right.

[PATCH v4] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when only CONFIG_CPU_S3C2442 is selected:

arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c244{0,2}_map_io function to the new pulldown helper functions.

Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
v4: remove default pull callbacks, set them in per-soc map_io function instead.
 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2440.c                    |   11 ++++-
 arch/arm/mach-s3c2440/s3c2442.c                    |   14 ++++++
 arch/arm/plat-s3c24xx/cpu.c                        |    8 ++--
 arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
 arch/arm/plat-s3c24xx/include/plat/s3c244x.h       |    7 +++-
 arch/arm/plat-samsung/gpio-config.c                |   45 ++++++++++++++++----
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 8 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
 config CPU_S3C2442
 	bool
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2440.c b/arch/arm/mach-s3c2440/s3c2440.c
index d50f3ae..f7663f7 100644
--- a/arch/arm/mach-s3c2440/s3c2440.c
+++ b/arch/arm/mach-s3c2440/s3c2440.c
@@ -46,9 +46,6 @@ int __init s3c2440_init(void)
 {
 	printk("S3C2440: Initialising architecture\n");
 
-	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
-	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
-
 	/* change irq for watchdog */
 
 	s3c_device_wdt.resource[1].start = IRQ_S3C2440_WDT;
@@ -58,3 +55,11 @@ int __init s3c2440_init(void)
 
 	return sysdev_register(&s3c2440_sysdev);
 }
+
+void __init s3c2440_map_io(void)
+{
+	s3c244x_map_io();
+
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
+}
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..ecf8135 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -43,6 +44,11 @@
 
 #include <plat/clock.h>
 #include <plat/cpu.h>
+#include <plat/s3c244x.h>
+
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
 
 /* S3C2442 extended clock support */
 
@@ -163,3 +169,11 @@ int __init s3c2442_init(void)
 
 	return sysdev_register(&s3c2442_sysdev);
 }
+
+void __init s3c2442_map_io(void)
+{
+	s3c244x_map_io();
+
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
+}
diff --git a/arch/arm/plat-s3c24xx/cpu.c b/arch/arm/plat-s3c24xx/cpu.c
index 76d0858..4a10c0f 100644
--- a/arch/arm/plat-s3c24xx/cpu.c
+++ b/arch/arm/plat-s3c24xx/cpu.c
@@ -88,7 +88,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440000,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2440_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2440_init,
@@ -97,7 +97,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440001,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2440_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2440_init,
@@ -106,7 +106,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440aaa,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2442_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2442_init,
@@ -115,7 +115,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440aab,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2442_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2442_init,
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 24c6f5a..243b641 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,6 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
 struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
 	.set_config	= s3c_gpio_setcfg_s3c24xx,
 	.get_config	= s3c_gpio_getcfg_s3c24xx,
-	.set_pull	= s3c_gpio_setpull_1up,
-	.get_pull	= s3c_gpio_getpull_1up,
 };
 
 struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
index 307248d..89e8d0a 100644
--- a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
+++ b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
@@ -21,17 +21,22 @@ extern void s3c244x_init_clocks(int xtal);
 #else
 #define s3c244x_init_clocks NULL
 #define s3c244x_init_uarts NULL
-#define s3c244x_map_io NULL
 #endif
 
 #ifdef CONFIG_CPU_S3C2440
 extern  int s3c2440_init(void);
+
+extern void s3c2440_map_io(void);
 #else
 #define s3c2440_init NULL
+#define s3c2440_map_io NULL
 #endif
 
 #ifdef CONFIG_CPU_S3C2442
 extern  int s3c2442_init(void);
+
+extern void s3c2442_map_io(void);
 #else
 #define s3c2442_init NULL
+#define s3c2442_map_io NULL
 #endif
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index b732b77..b39e05b 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,16 +280,15 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull,
+			 s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
-	pup = __raw_readl(reg);
-
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pup == updown)
 		pup &= ~(1 << off);
 	else if (pup == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
@@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 8fd65d8..0d2c570 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.7.3.2

[PATCH v4] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Lars-Peter Clausen]

On 11/30/2010 08:46 PM, Vasily Khoruzhick wrote:
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> error when only CONFIG_CPU_S3C2442 is selected:
> 
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
> 
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
> to take an additional parameter deciding whether the pin has a pullup or pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> 
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
> in the s3c244{0,2}_map_io function to the new pulldown helper functions.
> 
> Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> v2: adapt patch for 2.6.37-rc1
> v3: restore default pull callbacks, add default pull callbacks for s3c2442
> v4: remove default pull callbacks, set them in per-soc map_io function instead.
>  arch/arm/mach-s3c2440/Kconfig                      |    1 +
>  arch/arm/mach-s3c2440/s3c2440.c                    |   11 ++++-
>  arch/arm/mach-s3c2440/s3c2442.c                    |   14 ++++++
>  arch/arm/plat-s3c24xx/cpu.c                        |    8 ++--
>  arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
>  arch/arm/plat-s3c24xx/include/plat/s3c244x.h       |    7 +++-
>  arch/arm/plat-samsung/gpio-config.c                |   45 ++++++++++++++++----
>  .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
>  8 files changed, 80 insertions(+), 19 deletions(-)
> 
> ...
>
> +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull,
> +			 s3c_gpio_pull_t updown)
>  {
>  	void __iomem *reg = chip->base + 0x08;
>  	u32 pup = __raw_readl(reg);
>  
> -	pup = __raw_readl(reg);
> -
> -	if (pup == S3C_GPIO_PULL_UP)
> +	if (pup == updown)
This should be pull == updown, otherwise looks fine to me

>  		pup &= ~(1 << off);
>  	else if (pup == S3C_GPIO_PULL_NONE)
>  		pup |= (1 << off);
> @@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
>  	return 0;
>  }

- Lars

[PATCH v4] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Tuesday 30 November 2010 21:59:01 Lars-Peter Clausen wrote:
> On 11/30/2010 08:46 PM, Vasily Khoruzhick wrote:
> > Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
> > structure are initalized via s3c_gpio_{get,set}pull_1up. This results in
> > a linker error when only CONFIG_CPU_S3C2442 is selected:
> > 
> > arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> > `s3c_gpio_getpull_1up'
> > arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> > `s3c_gpio_setpull_1up'
> > 
> > The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> > The method of controlling them is the same though.
> > So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
> > functions to take an additional parameter deciding whether the pin has a
> > pullup or pulldown. The s3c_gpio_{get,set}pull_1{down,up} functions then
> > wrap that functions passing either S3C_GPIO_PULL_UP or
> > S3C_GPIO_PULL_DOWN.
> > 
> > Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
> > fields in the s3c244{0,2}_map_io function to the new pulldown helper
> > functions.
> > 
> > Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
> > 
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> > v2: adapt patch for 2.6.37-rc1
> > v3: restore default pull callbacks, add default pull callbacks for
> > s3c2442 v4: remove default pull callbacks, set them in per-soc map_io
> > function instead.
> > 
> >  arch/arm/mach-s3c2440/Kconfig                      |    1 +
> >  arch/arm/mach-s3c2440/s3c2440.c                    |   11 ++++-
> >  arch/arm/mach-s3c2440/s3c2442.c                    |   14 ++++++
> >  arch/arm/plat-s3c24xx/cpu.c                        |    8 ++--
> >  arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
> >  arch/arm/plat-s3c24xx/include/plat/s3c244x.h       |    7 +++-
> >  arch/arm/plat-samsung/gpio-config.c                |   45
> >  ++++++++++++++++---- .../plat-samsung/include/plat/gpio-cfg-helpers.h  
> >  |   11 +++++ 8 files changed, 80 insertions(+), 19 deletions(-)
> > 
> > ...
> > 
> > +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> > +			 unsigned int off, s3c_gpio_pull_t pull,
> > +			 s3c_gpio_pull_t updown)
> > 
> >  {
> >  
> >  	void __iomem *reg = chip->base + 0x08;
> >  	u32 pup = __raw_readl(reg);
> > 
> > -	pup = __raw_readl(reg);
> > -
> > -	if (pup == S3C_GPIO_PULL_UP)
> > +	if (pup == updown)
> 
> This should be pull == updown, otherwise looks fine to me

You're right. Btw, can you test this patch on your s3c24xx-machine?

> >  		pup &= ~(1 << off);
> >  	
> >  	else if (pup == S3C_GPIO_PULL_NONE)
> >  	
> >  		pup |= (1 << off);
> > 
> > @@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip
> > *chip,
> > 
> >  	return 0;
> >  
> >  }
> 
> - Lars

Regards
Vasily

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when only CONFIG_CPU_S3C2442 is selected:

arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c244{0,2}_map_io function to the new pulldown helper functions.

Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
v4: remove default pull callbacks, set them in per-soc map_io function instead.
v5: fix obvious mistake in setpull_1
 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2440.c                    |   11 ++++-
 arch/arm/mach-s3c2440/s3c2442.c                    |   14 ++++++
 arch/arm/plat-s3c24xx/cpu.c                        |    8 ++--
 arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
 arch/arm/plat-s3c24xx/include/plat/s3c244x.h       |    7 +++-
 arch/arm/plat-samsung/gpio-config.c                |   45 ++++++++++++++++----
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 8 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
 config CPU_S3C2442
 	bool
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2440.c b/arch/arm/mach-s3c2440/s3c2440.c
index d50f3ae..f7663f7 100644
--- a/arch/arm/mach-s3c2440/s3c2440.c
+++ b/arch/arm/mach-s3c2440/s3c2440.c
@@ -46,9 +46,6 @@ int __init s3c2440_init(void)
 {
 	printk("S3C2440: Initialising architecture\n");
 
-	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
-	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
-
 	/* change irq for watchdog */
 
 	s3c_device_wdt.resource[1].start = IRQ_S3C2440_WDT;
@@ -58,3 +55,11 @@ int __init s3c2440_init(void)
 
 	return sysdev_register(&s3c2440_sysdev);
 }
+
+void __init s3c2440_map_io(void)
+{
+	s3c244x_map_io();
+
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
+}
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..ecf8135 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -43,6 +44,11 @@
 
 #include <plat/clock.h>
 #include <plat/cpu.h>
+#include <plat/s3c244x.h>
+
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
 
 /* S3C2442 extended clock support */
 
@@ -163,3 +169,11 @@ int __init s3c2442_init(void)
 
 	return sysdev_register(&s3c2442_sysdev);
 }
+
+void __init s3c2442_map_io(void)
+{
+	s3c244x_map_io();
+
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
+}
diff --git a/arch/arm/plat-s3c24xx/cpu.c b/arch/arm/plat-s3c24xx/cpu.c
index 76d0858..4a10c0f 100644
--- a/arch/arm/plat-s3c24xx/cpu.c
+++ b/arch/arm/plat-s3c24xx/cpu.c
@@ -88,7 +88,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440000,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2440_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2440_init,
@@ -97,7 +97,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440001,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2440_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2440_init,
@@ -106,7 +106,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440aaa,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2442_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2442_init,
@@ -115,7 +115,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440aab,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2442_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2442_init,
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 24c6f5a..243b641 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,6 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
 struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
 	.set_config	= s3c_gpio_setcfg_s3c24xx,
 	.get_config	= s3c_gpio_getcfg_s3c24xx,
-	.set_pull	= s3c_gpio_setpull_1up,
-	.get_pull	= s3c_gpio_getpull_1up,
 };
 
 struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
index 307248d..89e8d0a 100644
--- a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
+++ b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
@@ -21,17 +21,22 @@ extern void s3c244x_init_clocks(int xtal);
 #else
 #define s3c244x_init_clocks NULL
 #define s3c244x_init_uarts NULL
-#define s3c244x_map_io NULL
 #endif
 
 #ifdef CONFIG_CPU_S3C2440
 extern  int s3c2440_init(void);
+
+extern void s3c2440_map_io(void);
 #else
 #define s3c2440_init NULL
+#define s3c2440_map_io NULL
 #endif
 
 #ifdef CONFIG_CPU_S3C2442
 extern  int s3c2442_init(void);
+
+extern void s3c2442_map_io(void);
 #else
 #define s3c2442_init NULL
+#define s3c2442_map_io NULL
 #endif
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index b732b77..304ce7e 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,16 +280,15 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull,
+			 s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
-	pup = __raw_readl(reg);
-
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pull == updown)
 		pup &= ~(1 << off);
 	else if (pup == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
@@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 8fd65d8..0d2c570 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.7.3.2

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Ben Dooks]

On 30/11/10 20:12, Vasily Khoruzhick wrote:
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> error when only CONFIG_CPU_S3C2442 is selected:
> 
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
> 
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
> to take an additional parameter deciding whether the pin has a pullup or pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> 
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
> in the s3c244{0,2}_map_io function to the new pulldown helper functions.
> 
> Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Ok, I think this is the best solution. I'll apply it tomorrow
if there's nothing else I can think of.

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Kukjin Kim]

Ben Dooks wrote:
> 
> On 30/11/10 20:12, Vasily Khoruzhick wrote:
> > Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
> structure
> > are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> > error when only CONFIG_CPU_S3C2442 is selected:
> >
> > arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> > `s3c_gpio_getpull_1up'
> > arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> > `s3c_gpio_setpull_1up'
> >
> > The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> > The method of controlling them is the same though.
> > So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
> functions
> > to take an additional parameter deciding whether the pin has a pullup or
> pulldown.
> > The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions
> passing
> > either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> >
> > Furthermore this patch sets up the
s3c24xx_gpiocfg_default.{get,set}_pull
> fields
> > in the s3c244{0,2}_map_io function to the new pulldown helper functions.
> >
> > Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
> 
> Ok, I think this is the best solution. I'll apply it tomorrow
> if there's nothing else I can think of.
> 
Hi Ben,

I applied this, but if you will apply this, I will drop this in my tree for
avoiding conflict.

Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks

[Kukjin Kim]

Vasily Khoruzhick wrote:
> 
> Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default
> structure
> are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
> error when only CONFIG_CPU_S3C2442 is selected:
> 
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
> `s3c_gpio_getpull_1up'
> arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
> `s3c_gpio_setpull_1up'
> 
> The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
> The method of controlling them is the same though.
> So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper
> functions
> to take an additional parameter deciding whether the pin has a pullup or
> pulldown.
> The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions
> passing
> either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.
> 
> Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull
> fields
> in the s3c244{0,2}_map_io function to the new pulldown helper functions.
> 
> Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> v2: adapt patch for 2.6.37-rc1
> v3: restore default pull callbacks, add default pull callbacks for s3c2442
> v4: remove default pull callbacks, set them in per-soc map_io function
> instead.
> v5: fix obvious mistake in setpull_1
>  arch/arm/mach-s3c2440/Kconfig                      |    1 +
>  arch/arm/mach-s3c2440/s3c2440.c                    |   11 ++++-
>  arch/arm/mach-s3c2440/s3c2442.c                    |   14 ++++++
>  arch/arm/plat-s3c24xx/cpu.c                        |    8 ++--
>  arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
>  arch/arm/plat-s3c24xx/include/plat/s3c244x.h       |    7 +++-
>  arch/arm/plat-samsung/gpio-config.c                |   45
++++++++++++++++--
> --
>  .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
>  8 files changed, 80 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
> index ff024a6..478fae0 100644
> --- a/arch/arm/mach-s3c2440/Kconfig
> +++ b/arch/arm/mach-s3c2440/Kconfig
> @@ -18,6 +18,7 @@ config CPU_S3C2440
>  config CPU_S3C2442
>  	bool
>  	select CPU_ARM920T
> +	select S3C_GPIO_PULL_DOWN
>  	select S3C2410_CLOCK
>  	select S3C2410_GPIO
>  	select S3C2410_PM if PM
> diff --git a/arch/arm/mach-s3c2440/s3c2440.c b/arch/arm/mach-
> s3c2440/s3c2440.c
> index d50f3ae..f7663f7 100644
> --- a/arch/arm/mach-s3c2440/s3c2440.c
> +++ b/arch/arm/mach-s3c2440/s3c2440.c
> @@ -46,9 +46,6 @@ int __init s3c2440_init(void)
>  {
>  	printk("S3C2440: Initialising architecture\n");
> 
> -	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
> -	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
> -
>  	/* change irq for watchdog */
> 
>  	s3c_device_wdt.resource[1].start = IRQ_S3C2440_WDT;
> @@ -58,3 +55,11 @@ int __init s3c2440_init(void)
> 
>  	return sysdev_register(&s3c2440_sysdev);
>  }
> +
> +void __init s3c2440_map_io(void)
> +{
> +	s3c244x_map_io();
> +
> +	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
> +	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
> +}

How about following...

+void __init s3c2440_map_io(void)
+{
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
+
+	s3c244x_map_io();
+}


> diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-
> s3c2440/s3c2442.c
> index 188ad1e..ecf8135 100644
> --- a/arch/arm/mach-s3c2440/s3c2442.c
> +++ b/arch/arm/mach-s3c2440/s3c2442.c
> @@ -32,6 +32,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/ioport.h>
>  #include <linux/mutex.h>
> +#include <linux/gpio.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> 
> @@ -43,6 +44,11 @@
> 
>  #include <plat/clock.h>
>  #include <plat/cpu.h>
> +#include <plat/s3c244x.h>
> +

No need this empty line.

> +#include <plat/gpio-core.h>
> +#include <plat/gpio-cfg.h>
> +#include <plat/gpio-cfg-helpers.h>
> 
>  /* S3C2442 extended clock support */
> 
> @@ -163,3 +169,11 @@ int __init s3c2442_init(void)
> 
>  	return sysdev_register(&s3c2442_sysdev);
>  }
> +
> +void __init s3c2442_map_io(void)
> +{
> +	s3c244x_map_io();
> +
> +	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
> +	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
> +}

Same as above.

> diff --git a/arch/arm/plat-s3c24xx/cpu.c b/arch/arm/plat-s3c24xx/cpu.c
> index 76d0858..4a10c0f 100644
> --- a/arch/arm/plat-s3c24xx/cpu.c
> +++ b/arch/arm/plat-s3c24xx/cpu.c
> @@ -88,7 +88,7 @@ static struct cpu_table cpu_ids[] __initdata = {
>  	{
>  		.idcode		= 0x32440000,
>  		.idmask		= 0xffffffff,
> -		.map_io		= s3c244x_map_io,
> +		.map_io		= s3c2440_map_io,
>  		.init_clocks	= s3c244x_init_clocks,
>  		.init_uarts	= s3c244x_init_uarts,
>  		.init		= s3c2440_init,
> @@ -97,7 +97,7 @@ static struct cpu_table cpu_ids[] __initdata = {
>  	{
>  		.idcode		= 0x32440001,
>  		.idmask		= 0xffffffff,
> -		.map_io		= s3c244x_map_io,
> +		.map_io		= s3c2440_map_io,
>  		.init_clocks	= s3c244x_init_clocks,
>  		.init_uarts	= s3c244x_init_uarts,
>  		.init		= s3c2440_init,
> @@ -106,7 +106,7 @@ static struct cpu_table cpu_ids[] __initdata = {
>  	{
>  		.idcode		= 0x32440aaa,
>  		.idmask		= 0xffffffff,
> -		.map_io		= s3c244x_map_io,
> +		.map_io		= s3c2442_map_io,
>  		.init_clocks	= s3c244x_init_clocks,
>  		.init_uarts	= s3c244x_init_uarts,
>  		.init		= s3c2442_init,
> @@ -115,7 +115,7 @@ static struct cpu_table cpu_ids[] __initdata = {
>  	{
>  		.idcode		= 0x32440aab,
>  		.idmask		= 0xffffffff,
> -		.map_io		= s3c244x_map_io,
> +		.map_io		= s3c2442_map_io,
>  		.init_clocks	= s3c244x_init_clocks,
>  		.init_uarts	= s3c244x_init_uarts,
>  		.init		= s3c2442_init,
> diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-
> s3c24xx/gpiolib.c
> index 24c6f5a..243b641 100644
> --- a/arch/arm/plat-s3c24xx/gpiolib.c
> +++ b/arch/arm/plat-s3c24xx/gpiolib.c
> @@ -82,8 +82,6 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
>  struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
>  	.set_config	= s3c_gpio_setcfg_s3c24xx,
>  	.get_config	= s3c_gpio_getcfg_s3c24xx,
> -	.set_pull	= s3c_gpio_setpull_1up,
> -	.get_pull	= s3c_gpio_getpull_1up,

Yeah, however, in my opinion, need to add following during gpiolib_init().

@@ -222,6 +222,11 @@ static __init int s3c24xx_gpiolib_init(void)
                if (!chip->config)
                        chip->config = &s3c24xx_gpiocfg_default;

+               if (!chip->config->set_pull)
+                       chip->config->set_pull = s3c_gpio_setpull_1up;
+               if (!chip->config->get_pull)
+                       chip->config->get_pull = s3c_gpio_getpull_1up;
+
                s3c_gpiolib_add(chip);
        }


>  };
> 
>  struct s3c_gpio_chip s3c24xx_gpios[] = {
> diff --git a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h b/arch/arm/plat-
> s3c24xx/include/plat/s3c244x.h
> index 307248d..89e8d0a 100644
> --- a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
> +++ b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
> @@ -21,17 +21,22 @@ extern void s3c244x_init_clocks(int xtal);
>  #else
>  #define s3c244x_init_clocks NULL
>  #define s3c244x_init_uarts NULL
> -#define s3c244x_map_io NULL
>  #endif
> 
>  #ifdef CONFIG_CPU_S3C2440
>  extern  int s3c2440_init(void);
> +
> +extern void s3c2440_map_io(void);
>  #else
>  #define s3c2440_init NULL
> +#define s3c2440_map_io NULL
>  #endif
> 
>  #ifdef CONFIG_CPU_S3C2442
>  extern  int s3c2442_init(void);
> +
> +extern void s3c2442_map_io(void);
>  #else
>  #define s3c2442_init NULL
> +#define s3c2442_map_io NULL
>  #endif
> diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-
> samsung/gpio-config.c
> index b732b77..304ce7e 100644
> --- a/arch/arm/plat-samsung/gpio-config.c
> +++ b/arch/arm/plat-samsung/gpio-config.c
> @@ -280,16 +280,15 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct
> s3c_gpio_chip *chip,
>  }
>  #endif
> 
> -#ifdef CONFIG_S3C_GPIO_PULL_UP
> -int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
> -			 unsigned int off, s3c_gpio_pull_t pull)
> +#if defined(CONFIG_S3C_GPIO_PULL_UP) ||
defined(CONFIG_S3C_GPIO_PULL_DOWN)
> +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull,
> +			 s3c_gpio_pull_t updown)
>  {
>  	void __iomem *reg = chip->base + 0x08;
>  	u32 pup = __raw_readl(reg);
> 
> -	pup = __raw_readl(reg);
> -
> -	if (pup == S3C_GPIO_PULL_UP)
> +	if (pull == updown)
>  		pup &= ~(1 << off);
>  	else if (pup == S3C_GPIO_PULL_NONE)

pull

>  		pup |= (1 << off);
> @@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
>  	return 0;
>  }
> 
> -s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
> -				     unsigned int off)
> +static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
> +				     unsigned int off, s3c_gpio_pull_t
updown)
>  {
>  	void __iomem *reg = chip->base + 0x08;
>  	u32 pup = __raw_readl(reg);
> 
>  	pup &= (1 << off);
> -	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
> +	return pup ? S3C_GPIO_PULL_NONE : updown;
> +}
> +#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
> +
> +#ifdef CONFIG_S3C_GPIO_PULL_UP
> +s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
> +				     unsigned int off)
> +{
> +	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
> +}
> +
> +int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull)
> +{
> +	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
>  }
>  #endif /* CONFIG_S3C_GPIO_PULL_UP */
> 
> +#ifdef CONFIG_S3C_GPIO_PULL_DOWN
> +s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
> +				     unsigned int off)
> +{
> +	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
> +}
> +
> +int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
> +			 unsigned int off, s3c_gpio_pull_t pull)
> +{
> +	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
> +}
> +#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
> +

How about to separate functions between PULL_UP and PULL_DOWN like
following...

#ifdef CONFIG_S3C_GPIO_PULL_UP
s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
                                     unsigned int off)
{
        void __iomem *reg = chip->base + 0x08;
        u32 pup = __raw_readl(reg);

        pup &= (1 << off);
        return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
}

int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
                         unsigned int off, s3c_gpio_pull_t pull)
{
        void __iomem *reg = chip->base + 0x08;
        u32 pup = __raw_readl(reg);

        if (pull == S3C_GPIO_PULL_UP)
                pup &= ~(1 << off);
        else if (pull == S3C_GPIO_PULL_NONE)
                pup |= (1 << off);
        else
                return -EINVAL;

        __raw_writel(pup, reg);

        return 0;
}
#endif /* CONFIG_S3C_GPIO_PULL_UP */

#ifdef CONFIG_S3C_GPIO_PULL_DOWN
s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
                                       unsigned int off)
{
        void __iomem *reg = chip->base + 0x08;
        u32 pdown = __raw_readl(reg);

        pdown &= (1 << off);
        return pdown ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_DOWN;
}

int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
                           unsigned int off, s3c_gpio_pull_t pull)
{
        void __iomem *reg = chip->base + 0x08;
        u32 pdown = __raw_readl(reg);

        if (pull == S3C_GPIO_PULL_DOWN)
                pdown &= ~(1 << off);
        else if (pull == S3C_GPIO_PULL_NONE)
                pdown |= (1 << off);
        else
                return -EINVAL;

        __raw_writel(pdown, reg);

        return 0;
}
#endif /* CONFIG_S3C_GPIO_PULL_DOWN */


>  #ifdef CONFIG_S5P_GPIO_DRVSTR
>  s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
>  {
> diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> index 8fd65d8..0d2c570 100644
> --- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> +++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
> @@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct
> s3c_gpio_chip *chip,
>  					    unsigned int off);
> 
>  /**
> + * s3c_gpio_getpull_1down() - Get configuration for choice of down or
none
> + * @chip: The gpio chip that the GPIO pin belongs to
> + * @off: The offset to the pin to get the configuration of.
> + *
> + * This helper function reads the state of the pull-down resistor for the
> + * given GPIO in the same case as s3c_gpio_setpull_1down.
> +*/
> +extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
> +					    unsigned int off);
> +
> +/**
>   * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
>   * @chip: The gpio chip that is being configured.
>   * @off: The offset for the GPIO being configured.
> --


Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Wednesday 01 December 2010 09:26:31 Kukjin Kim wrote:

> How about following...
> 
> +void __init s3c2440_map_io(void)
> +{
> +	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
> +	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
> +
> +	s3c244x_map_io();
> +}

What's difference except order?
 
> Yeah, however, in my opinion, need to add following during gpiolib_init().
> 
> @@ -222,6 +222,11 @@ static __init int s3c24xx_gpiolib_init(void)
>                 if (!chip->config)
>                         chip->config = &s3c24xx_gpiocfg_default;
> 
> +               if (!chip->config->set_pull)
> +                       chip->config->set_pull = s3c_gpio_setpull_1up;
> +               if (!chip->config->get_pull)
> +                       chip->config->get_pull = s3c_gpio_getpull_1up;
> +
>                 s3c_gpiolib_add(chip);
>         }

Pull callbacks are soc-specific, s3c24xx_gpiolib_init is common for all socs, 
so we can't do like this.

Btw, I've sent v6 version of patch, it still with empty line issues, should I 
resend v7 version or maintainers can fixup it at their side? :)

Regards
Vasily

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks

[Kukjin Kim]

Vasily Khoruzhick wrote:
> 
> On Wednesday 01 December 2010 09:26:31 Kukjin Kim wrote:
> 
> > How about following...
> >
> > +void __init s3c2440_map_io(void)
> > +{
> > +	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
> > +	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
> > +
> > +	s3c244x_map_io();
> > +}
> 
> What's difference except order?
> 
Just same ordering is used in other Samsung S3C SoCs.
Now actually doesn't matter it.

> > Yeah, however, in my opinion, need to add following during
gpiolib_init().
> >
> > @@ -222,6 +222,11 @@ static __init int s3c24xx_gpiolib_init(void)
> >                 if (!chip->config)
> >                         chip->config = &s3c24xx_gpiocfg_default;
> >
> > +               if (!chip->config->set_pull)
> > +                       chip->config->set_pull = s3c_gpio_setpull_1up;
> > +               if (!chip->config->get_pull)
> > +                       chip->config->get_pull = s3c_gpio_getpull_1up;
> > +
> >                 s3c_gpiolib_add(chip);
> >         }
> 
> Pull callbacks are soc-specific, s3c24xx_gpiolib_init is common for all
socs,
> so we can't do like this.
> 
Yeah, right...it depends on each SoCs. But, if there is no setup it in
map_io(), there is no functions of set_pull() and get_pull(). As you know,
used s3c_gpio_{set,get}pull_1up as default before your patch...

Hmm...however, I will think about its necessity again.

> Btw, I've sent v6 version of patch, it still with empty line issues,
should I
> resend v7 version or maintainers can fixup it at their side? :)
> 
Ok..I will do it and will apply.
Then, will be sent to Linus during 37-rc.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v5] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Wednesday 01 December 2010 14:05:00 Kukjin Kim wrote:
> Yeah, right...it depends on each SoCs. But, if there is no setup it in
> map_io(), there is no functions of set_pull() and get_pull(). As you know,
> used s3c_gpio_{set,get}pull_1up as default before your patch...
> 
> Hmm...however, I will think about its necessity again.

In this case it will fail if SoC does not support pull up, so does it matter 
how it will fail?
 
> Ok..I will do it and will apply.
> Then, will be sent to Linus during 37-rc.

Thanks

Regards
Vasily

[PATCH v4] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks

[Kukjin Kim]

Lars-Peter Clausen wrote:
> 
> > +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> > +			 unsigned int off, s3c_gpio_pull_t pull,
> > +			 s3c_gpio_pull_t updown)
> >  {
> >  	void __iomem *reg = chip->base + 0x08;
> >  	u32 pup = __raw_readl(reg);
> >
> > -	pup = __raw_readl(reg);
> > -
> > -	if (pup == S3C_GPIO_PULL_UP)
> > +	if (pup == updown)
> This should be pull == updown, otherwise looks fine to me
> 
Yeah, I meant this.

> >  		pup &= ~(1 << off);
> >  	else if (pup == S3C_GPIO_PULL_NONE)

pull == S3C_GPIO_PULL_NONE?

> >  		pup |= (1 << off);

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v4] ARM: s3c244x: Fix mess with gpio {set, get}_pull callbacks

[Vasily Khoruzhick]

On Wednesday 01 December 2010 07:28:17 Kukjin Kim wrote:
> > >  	else if (pup == S3C_GPIO_PULL_NONE)
> 
> pull == S3C_GPIO_PULL_NONE?

Yep, thanks

[PATCH v6] ARM: s3c244x: Fix mess with gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

Currently the {set,get}_pull callbacks of the s3c24xx_gpiocfg_default structure
are initalized via s3c_gpio_{get,set}pull_1up. This results in a linker
error when only CONFIG_CPU_S3C2442 is selected:

arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f4): undefined reference to
`s3c_gpio_getpull_1up'
arch/arm/plat-s3c24xx/built-in.o:(.data+0x13f8): undefined reference to
`s3c_gpio_setpull_1up'

The s3c2442 has pulldowns instead of pullups compared to the s3c2440.
The method of controlling them is the same though.
So this patch modifies the existing s3c_gpio_{get,set}pull_1up helper functions
to take an additional parameter deciding whether the pin has a pullup or pulldown.
The s3c_gpio_{get,set}pull_1{down,up} functions then wrap that functions passing
either S3C_GPIO_PULL_UP or S3C_GPIO_PULL_DOWN.

Furthermore this patch sets up the s3c24xx_gpiocfg_default.{get,set}_pull fields
in the s3c244{0,2}_map_io function to the new pulldown helper functions.

Based on patch from "Lars-Peter Clausen" <lars@metafoo.de>

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
v2: adapt patch for 2.6.37-rc1
v3: restore default pull callbacks, add default pull callbacks for s3c2442
v4: remove default pull callbacks, set them in per-soc map_io function instead.
v5: fix obvious mistake in setpull_1
v6: fix another obvious mistake in setpull_1
 arch/arm/mach-s3c2440/Kconfig                      |    1 +
 arch/arm/mach-s3c2440/s3c2440.c                    |   11 +++-
 arch/arm/mach-s3c2440/s3c2442.c                    |   14 ++++++
 arch/arm/plat-s3c24xx/cpu.c                        |    8 ++--
 arch/arm/plat-s3c24xx/gpiolib.c                    |    2 -
 arch/arm/plat-s3c24xx/include/plat/s3c244x.h       |    7 +++-
 arch/arm/plat-samsung/gpio-config.c                |   47 +++++++++++++++----
 .../plat-samsung/include/plat/gpio-cfg-helpers.h   |   11 +++++
 8 files changed, 81 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-s3c2440/Kconfig b/arch/arm/mach-s3c2440/Kconfig
index ff024a6..478fae0 100644
--- a/arch/arm/mach-s3c2440/Kconfig
+++ b/arch/arm/mach-s3c2440/Kconfig
@@ -18,6 +18,7 @@ config CPU_S3C2440
 config CPU_S3C2442
 	bool
 	select CPU_ARM920T
+	select S3C_GPIO_PULL_DOWN
 	select S3C2410_CLOCK
 	select S3C2410_GPIO
 	select S3C2410_PM if PM
diff --git a/arch/arm/mach-s3c2440/s3c2440.c b/arch/arm/mach-s3c2440/s3c2440.c
index d50f3ae..f7663f7 100644
--- a/arch/arm/mach-s3c2440/s3c2440.c
+++ b/arch/arm/mach-s3c2440/s3c2440.c
@@ -46,9 +46,6 @@ int __init s3c2440_init(void)
 {
 	printk("S3C2440: Initialising architecture\n");
 
-	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
-	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
-
 	/* change irq for watchdog */
 
 	s3c_device_wdt.resource[1].start = IRQ_S3C2440_WDT;
@@ -58,3 +55,11 @@ int __init s3c2440_init(void)
 
 	return sysdev_register(&s3c2440_sysdev);
 }
+
+void __init s3c2440_map_io(void)
+{
+	s3c244x_map_io();
+
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1up;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1up;
+}
diff --git a/arch/arm/mach-s3c2440/s3c2442.c b/arch/arm/mach-s3c2440/s3c2442.c
index 188ad1e..ecf8135 100644
--- a/arch/arm/mach-s3c2440/s3c2442.c
+++ b/arch/arm/mach-s3c2440/s3c2442.c
@@ -32,6 +32,7 @@
 #include <linux/interrupt.h>
 #include <linux/ioport.h>
 #include <linux/mutex.h>
+#include <linux/gpio.h>
 #include <linux/clk.h>
 #include <linux/io.h>
 
@@ -43,6 +44,11 @@
 
 #include <plat/clock.h>
 #include <plat/cpu.h>
+#include <plat/s3c244x.h>
+
+#include <plat/gpio-core.h>
+#include <plat/gpio-cfg.h>
+#include <plat/gpio-cfg-helpers.h>
 
 /* S3C2442 extended clock support */
 
@@ -163,3 +169,11 @@ int __init s3c2442_init(void)
 
 	return sysdev_register(&s3c2442_sysdev);
 }
+
+void __init s3c2442_map_io(void)
+{
+	s3c244x_map_io();
+
+	s3c24xx_gpiocfg_default.set_pull = s3c_gpio_setpull_1down;
+	s3c24xx_gpiocfg_default.get_pull = s3c_gpio_getpull_1down;
+}
diff --git a/arch/arm/plat-s3c24xx/cpu.c b/arch/arm/plat-s3c24xx/cpu.c
index 76d0858..4a10c0f 100644
--- a/arch/arm/plat-s3c24xx/cpu.c
+++ b/arch/arm/plat-s3c24xx/cpu.c
@@ -88,7 +88,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440000,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2440_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2440_init,
@@ -97,7 +97,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440001,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2440_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2440_init,
@@ -106,7 +106,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440aaa,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2442_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2442_init,
@@ -115,7 +115,7 @@ static struct cpu_table cpu_ids[] __initdata = {
 	{
 		.idcode		= 0x32440aab,
 		.idmask		= 0xffffffff,
-		.map_io		= s3c244x_map_io,
+		.map_io		= s3c2442_map_io,
 		.init_clocks	= s3c244x_init_clocks,
 		.init_uarts	= s3c244x_init_uarts,
 		.init		= s3c2442_init,
diff --git a/arch/arm/plat-s3c24xx/gpiolib.c b/arch/arm/plat-s3c24xx/gpiolib.c
index 24c6f5a..243b641 100644
--- a/arch/arm/plat-s3c24xx/gpiolib.c
+++ b/arch/arm/plat-s3c24xx/gpiolib.c
@@ -82,8 +82,6 @@ static struct s3c_gpio_cfg s3c24xx_gpiocfg_banka = {
 struct s3c_gpio_cfg s3c24xx_gpiocfg_default = {
 	.set_config	= s3c_gpio_setcfg_s3c24xx,
 	.get_config	= s3c_gpio_getcfg_s3c24xx,
-	.set_pull	= s3c_gpio_setpull_1up,
-	.get_pull	= s3c_gpio_getpull_1up,
 };
 
 struct s3c_gpio_chip s3c24xx_gpios[] = {
diff --git a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
index 307248d..89e8d0a 100644
--- a/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
+++ b/arch/arm/plat-s3c24xx/include/plat/s3c244x.h
@@ -21,17 +21,22 @@ extern void s3c244x_init_clocks(int xtal);
 #else
 #define s3c244x_init_clocks NULL
 #define s3c244x_init_uarts NULL
-#define s3c244x_map_io NULL
 #endif
 
 #ifdef CONFIG_CPU_S3C2440
 extern  int s3c2440_init(void);
+
+extern void s3c2440_map_io(void);
 #else
 #define s3c2440_init NULL
+#define s3c2440_map_io NULL
 #endif
 
 #ifdef CONFIG_CPU_S3C2442
 extern  int s3c2442_init(void);
+
+extern void s3c2442_map_io(void);
 #else
 #define s3c2442_init NULL
+#define s3c2442_map_io NULL
 #endif
diff --git a/arch/arm/plat-samsung/gpio-config.c b/arch/arm/plat-samsung/gpio-config.c
index b732b77..0aa32f2 100644
--- a/arch/arm/plat-samsung/gpio-config.c
+++ b/arch/arm/plat-samsung/gpio-config.c
@@ -280,18 +280,17 @@ s3c_gpio_pull_t s3c_gpio_getpull_updown(struct s3c_gpio_chip *chip,
 }
 #endif
 
-#ifdef CONFIG_S3C_GPIO_PULL_UP
-int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
-			 unsigned int off, s3c_gpio_pull_t pull)
+#if defined(CONFIG_S3C_GPIO_PULL_UP) || defined(CONFIG_S3C_GPIO_PULL_DOWN)
+static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull,
+			 s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
-	pup = __raw_readl(reg);
-
-	if (pup == S3C_GPIO_PULL_UP)
+	if (pull == updown)
 		pup &= ~(1 << off);
-	else if (pup == S3C_GPIO_PULL_NONE)
+	else if (pull == S3C_GPIO_PULL_NONE)
 		pup |= (1 << off);
 	else
 		return -EINVAL;
@@ -300,17 +299,45 @@ int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
 	return 0;
 }
 
-s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
-				     unsigned int off)
+static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
+				     unsigned int off, s3c_gpio_pull_t updown)
 {
 	void __iomem *reg = chip->base + 0x08;
 	u32 pup = __raw_readl(reg);
 
 	pup &= (1 << off);
-	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
+	return pup ? S3C_GPIO_PULL_NONE : updown;
+}
+#endif /* CONFIG_S3C_GPIO_PULL_UP || CONFIG_S3C_GPIO_PULL_DOWN */
+
+#ifdef CONFIG_S3C_GPIO_PULL_UP
+s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_UP);
+}
+
+int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_UP);
 }
 #endif /* CONFIG_S3C_GPIO_PULL_UP */
 
+#ifdef CONFIG_S3C_GPIO_PULL_DOWN
+s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+				     unsigned int off)
+{
+	return s3c_gpio_getpull_1(chip, off, S3C_GPIO_PULL_DOWN);
+}
+
+int s3c_gpio_setpull_1down(struct s3c_gpio_chip *chip,
+			 unsigned int off, s3c_gpio_pull_t pull)
+{
+	return s3c_gpio_setpull_1(chip, off, pull, S3C_GPIO_PULL_DOWN);
+}
+#endif /* CONFIG_S3C_GPIO_PULL_DOWN */
+
 #ifdef CONFIG_S5P_GPIO_DRVSTR
 s5p_gpio_drvstr_t s5p_gpio_get_drvstr(unsigned int pin)
 {
diff --git a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
index 8fd65d8..0d2c570 100644
--- a/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
+++ b/arch/arm/plat-samsung/include/plat/gpio-cfg-helpers.h
@@ -210,6 +210,17 @@ extern s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
 					    unsigned int off);
 
 /**
+ * s3c_gpio_getpull_1down() - Get configuration for choice of down or none
+ * @chip: The gpio chip that the GPIO pin belongs to
+ * @off: The offset to the pin to get the configuration of.
+ *
+ * This helper function reads the state of the pull-down resistor for the
+ * given GPIO in the same case as s3c_gpio_setpull_1down.
+*/
+extern s3c_gpio_pull_t s3c_gpio_getpull_1down(struct s3c_gpio_chip *chip,
+					    unsigned int off);
+
+/**
  * s3c_gpio_setpull_s3c2443() - Pull configuration for s3c2443.
  * @chip: The gpio chip that is being configured.
  * @off: The offset for the GPIO being configured.
-- 
1.7.3.2

[PATCH RESEND v3] ARM: s3c2442: Setup gpio {set, get}_pull callbacks

[Kukjin Kim]

Lars-Peter Clausen wrote:
> 
> >> -#ifdef CONFIG_S3C_GPIO_PULL_UP
> >> -int s3c_gpio_setpull_1up(struct s3c_gpio_chip *chip,
> >> -			 unsigned int off, s3c_gpio_pull_t pull)
> >> +#if defined(CONFIG_S3C_GPIO_PULL_UP) ||
> > defined(CONFIG_S3C_GPIO_PULL_DOWN)
> >> +static int s3c_gpio_setpull_1(struct s3c_gpio_chip *chip,
> >> +			 unsigned int off, s3c_gpio_pull_t pull,
> >> +			 s3c_gpio_pull_t updown)
> >
> > Hmm...how about s3c_gpio_setpull_1updown(...)?
> > And actually, not used 3rd argument, "pull" now.
> > I prefer follwoing.
> >
> 
> You need the 4th arguemnt, because the s3c2440 only supports pullups and the
> s3c2442
> only supports pulldowns. So you want to return -EINVAL if somebody tries to
> set a
> pullup on a s3c2442 based board.
> Your proposed solution would return 0 and set a pulldown instead.
> 

Hmm...please read following which is from previous my comment.
---
> > But, according to your patch, maybe not called "else if" and "else".
> > Because you only used "S3C_GPIO_PULL_UP" and "S3C_GPIO_PULL_DOWN" as
> > argument from caller function.
> >
> > So should be separate it as Ben's original code.
---
In addition, the reason is that it is not used 3rd argument "pull" in s3c_gpio_setpull_1().
My question is simple. Why do you think that we need useless argument :-(
Do we _really_ need two s3c_gpuo_pull_t in argument here?

(snip)

> >> -s3c_gpio_pull_t s3c_gpio_getpull_1up(struct s3c_gpio_chip *chip,
> >> -				     unsigned int off)
> >> +static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
> >> +				     unsigned int off, s3c_gpio_pull_t
> > updown)
> >
> > s3c_gpio_getpull_1updown(...)?
> > And...why need 3rd argument, "updown"
> 
> Same reason as above.
> 
> >
> > +static s3c_gpio_pull_t s3c_gpio_getpull_1(struct s3c_gpio_chip *chip,
> > +					  unsigned int off)
> >
> >>  {
> >>  	void __iomem *reg = chip->base + 0x08;
> >>  	u32 pup = __raw_readl(reg);
> >>
> >>  	pup &= (1 << off);
> >> -	return pup ? S3C_GPIO_PULL_NONE : S3C_GPIO_PULL_UP;
> >> +	return pup ? S3C_GPIO_PULL_NONE : updown;
> >
> > Is this right?
> > Hmm...No...
> Why do you think it is wrong? As far as I can see it is correct.
> 
I miss-read. It's ok.

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

[PATCH v3] ARM: s3c2442: Setup gpio {set,get}_pull callbacks

[Vasily Khoruzhick]

On Monday 29 November 2010 12:15:03 Kukjin Kim wrote:

> Hi,
> 
> Actually, I didn't get your patch through my e-mail.
> Maybe you missed my e-mail in Cc at that time and I missed your patch in
> mailing list. Could you please re-send it so that can get the patch via
> e-mail?

Ok, will do in few minutes

> Now, thanks for adding me, will review...

Thanks

> I think, we have some time to fix it for 37, so don't worry about broken
> 37-stable.

Ok

Regards
Vasily