[PATCH 0/3] h1940 series

[PATCH 0/3] h1940 series

[Vasily Khoruzhick]

This patch series replaces h1940-specific latch-related
functions with gpiolib, fixes compilation error for h1940-bluetooth
and adds PM support for mmc driver.

[PATCH 1/3] h1940: use gpiolib for latch access

[Vasily Khoruzhick]

This patch adds gpiolib support for h1940 latch.
With this patch it's possible to use leds-gpio and uda1380
drivers (they require gpiolib support for appropriate pins).
And now it's possible to drop leds-h1940 driver.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/mach-s3c2410/h1940-bluetooth.c          |   13 +++-
 arch/arm/mach-s3c2410/include/mach/h1940-latch.h |   57 +++++-----------
 arch/arm/mach-s3c2410/mach-h1940.c               |   77 ++++++++++++++++++++--
 arch/arm/plat-s3c24xx/Kconfig                    |    1 +
 4 files changed, 100 insertions(+), 48 deletions(-)

diff --git a/arch/arm/mach-s3c2410/h1940-bluetooth.c b/arch/arm/mach-s3c2410/h1940-bluetooth.c
index 8cdeb14..8aa2f19 100644
--- a/arch/arm/mach-s3c2410/h1940-bluetooth.c
+++ b/arch/arm/mach-s3c2410/h1940-bluetooth.c
@@ -30,7 +30,7 @@ static void h1940bt_enable(int on)
 {
 	if (on) {
 		/* Power on the chip */
-		h1940_latch_control(0, H1940_LATCH_BLUETOOTH_POWER);
+		gpio_set_value(H1940_LATCH_BLUETOOTH_POWER, 1);
 		/* Reset the chip */
 		mdelay(10);
 
@@ -43,7 +43,7 @@ static void h1940bt_enable(int on)
 		mdelay(10);
 		gpio_set_value(S3C2410_GPH(1), 0);
 		mdelay(10);
-		h1940_latch_control(H1940_LATCH_BLUETOOTH_POWER, 0);
+		gpio_set_value(H1940_LATCH_BLUETOOTH_POWER, 0);
 	}
 }
 
@@ -64,7 +64,14 @@ static int __devinit h1940bt_probe(struct platform_device *pdev)
 
 	ret = gpio_request(S3C2410_GPH(1), dev_name(&pdev->dev));
 	if (ret) {
-		dev_err(&pdev->dev, "could not get GPH1\n");\
+		dev_err(&pdev->dev, "could not get GPH1\n");
+		return ret;
+	}
+
+	ret = gpio_request(H1940_LATCH_BLUETOOTH_POWER, dev_name(&pdev->dev));
+	if (ret) {
+		gpio_free(S3C2410_GPH(1));
+		dev_err(&pdev->dev, "could not get BT_POWER\n");
 		return ret;
 	}
 
diff --git a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
index d8a8327..73586f2 100644
--- a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
+++ b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
@@ -14,51 +14,30 @@
 #ifndef __ASM_ARCH_H1940_LATCH_H
 #define __ASM_ARCH_H1940_LATCH_H
 
+#include <mach/gpio.h>
 
-#ifndef __ASSEMBLY__
-#define H1940_LATCH		((void __force __iomem *)0xF8000000)
-#else
-#define H1940_LATCH		0xF8000000
-#endif
-
-#define H1940_PA_LATCH		(S3C2410_CS2)
+#define H1940_LATCH_GPIO(x)		(S3C_GPIO_END + (x))
 
 /* SD layer latch */
 
-#define H1940_LATCH_SDQ1		(1<<16)
-#define H1940_LATCH_LCD_P1		(1<<17)
-#define H1940_LATCH_LCD_P2		(1<<18)
-#define H1940_LATCH_LCD_P3		(1<<19)
-#define H1940_LATCH_MAX1698_nSHUTDOWN	(1<<20)		/* LCD backlight */
-#define H1940_LATCH_LED_RED		(1<<21)
-#define H1940_LATCH_SDQ7		(1<<22)
-#define H1940_LATCH_USB_DP		(1<<23)
+#define H1940_LATCH_SDQ1		H1940_LATCH_GPIO(0)
+#define H1940_LATCH_LCD_P1		H1940_LATCH_GPIO(1)
+#define H1940_LATCH_LCD_P2		H1940_LATCH_GPIO(2)
+#define H1940_LATCH_LCD_P3		H1940_LATCH_GPIO(3)
+#define H1940_LATCH_MAX1698_nSHUTDOWN	H1940_LATCH_GPIO(4)
+#define H1940_LATCH_LED_RED		H1940_LATCH_GPIO(5)
+#define H1940_LATCH_SDQ7		H1940_LATCH_GPIO(6)
+#define H1940_LATCH_USB_DP		H1940_LATCH_GPIO(7)
 
 /* CPU layer latch */
 
-#define H1940_LATCH_UDA_POWER		(1<<24)
-#define H1940_LATCH_AUDIO_POWER		(1<<25)
-#define H1940_LATCH_SM803_ENABLE	(1<<26)
-#define H1940_LATCH_LCD_P4		(1<<27)
-#define H1940_LATCH_CPUQ5		(1<<28)		/* untraced */
-#define H1940_LATCH_BLUETOOTH_POWER	(1<<29)		/* active high */
-#define H1940_LATCH_LED_GREEN		(1<<30)
-#define H1940_LATCH_LED_FLASH		(1<<31)
-
-/* default settings */
-
-#define H1940_LATCH_DEFAULT		\
-	H1940_LATCH_LCD_P4		| \
-	H1940_LATCH_SM803_ENABLE	| \
-	H1940_LATCH_SDQ1		| \
-	H1940_LATCH_LCD_P1		| \
-	H1940_LATCH_LCD_P2		| \
-	H1940_LATCH_LCD_P3		| \
-	H1940_LATCH_MAX1698_nSHUTDOWN   | \
-	H1940_LATCH_CPUQ5
-
-/* control functions */
-
-extern void h1940_latch_control(unsigned int clear, unsigned int set);
+#define H1940_LATCH_UDA_POWER		H1940_LATCH_GPIO(8)
+#define H1940_LATCH_AUDIO_POWER		H1940_LATCH_GPIO(9)
+#define H1940_LATCH_SM803_ENABLE	H1940_LATCH_GPIO(10)
+#define H1940_LATCH_LCD_P4		H1940_LATCH_GPIO(11)
+#define H1940_LATCH_CPUQ5		H1940_LATCH_GPIO(12)
+#define H1940_LATCH_BLUETOOTH_POWER	H1940_LATCH_GPIO(13)
+#define H1940_LATCH_LED_GREEN		H1940_LATCH_GPIO(14)
+#define H1940_LATCH_LED_FLASH		H1940_LATCH_GPIO(15)
 
 #endif /* __ASM_ARCH_H1940_LATCH_H */
diff --git a/arch/arm/mach-s3c2410/mach-h1940.c b/arch/arm/mach-s3c2410/mach-h1940.c
index 779b45b..9717790 100644
--- a/arch/arm/mach-s3c2410/mach-h1940.c
+++ b/arch/arm/mach-s3c2410/mach-h1940.c
@@ -42,6 +42,7 @@
 #include <mach/regs-gpio.h>
 #include <mach/gpio-fns.h>
 #include <mach/gpio-nrs.h>
+#include <mach/gpio.h>
 
 #include <mach/h1940.h>
 #include <mach/h1940-latch.h>
@@ -58,6 +59,16 @@
 #include <plat/mci.h>
 #include <plat/ts.h>
 
+#ifndef __ASSEMBLY__
+#define H1940_LATCH		((void __force __iomem *)0xF8000000)
+#else
+#define H1940_LATCH		0xF8000000
+#endif
+
+#define H1940_PA_LATCH		(S3C2410_CS2)
+
+#define H1940_LATCH_BIT(x)		(1 << ((x) + 16 - S3C_GPIO_END))
+
 static struct map_desc h1940_iodesc[] __initdata = {
 	[0] = {
 		.virtual	= (unsigned long)H1940_LATCH,
@@ -99,9 +110,16 @@ static struct s3c2410_uartcfg h1940_uartcfgs[] __initdata = {
 
 /* Board control latch control */
 
-static unsigned int latch_state = H1940_LATCH_DEFAULT;
+static unsigned int latch_state = H1940_LATCH_BIT(H1940_LATCH_LCD_P4) |
+	H1940_LATCH_BIT(H1940_LATCH_SM803_ENABLE)	|
+	H1940_LATCH_BIT(H1940_LATCH_SDQ1)			|
+	H1940_LATCH_BIT(H1940_LATCH_LCD_P1)			|
+	H1940_LATCH_BIT(H1940_LATCH_LCD_P2)			|
+	H1940_LATCH_BIT(H1940_LATCH_LCD_P3)			|
+	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN)   |
+	H1940_LATCH_BIT(H1940_LATCH_CPUQ5);
 
-void h1940_latch_control(unsigned int clear, unsigned int set)
+static void h1940_latch_control(unsigned int clear, unsigned int set)
 {
 	unsigned long flags;
 
@@ -115,7 +133,50 @@ void h1940_latch_control(unsigned int clear, unsigned int set)
 	local_irq_restore(flags);
 }
 
-EXPORT_SYMBOL_GPL(h1940_latch_control);
+static int h1940_gpiolib_latch_input(struct gpio_chip *chip, unsigned offset)
+{
+	return -EINVAL;
+}
+
+
+static inline int h1940_gpiolib_to_latch(int offset)
+{
+	return 1 << (offset + 16);
+}
+
+static void h1940_gpiolib_latch_set(struct gpio_chip *chip,
+					unsigned offset, int value)
+{
+	int latch_bit = h1940_gpiolib_to_latch(offset);
+
+	h1940_latch_control(value ? 0 : latch_bit,
+		value ? latch_bit : 0);
+}
+
+static int h1940_gpiolib_latch_output(struct gpio_chip *chip,
+					unsigned offset, int value)
+{
+	h1940_gpiolib_latch_set(chip, offset, value);
+
+	return 0;
+}
+
+static int h1940_gpiolib_latch_get(struct gpio_chip *chip,
+					unsigned offset)
+{
+	return (latch_state >> (offset + 16)) & 1;
+}
+
+struct gpio_chip h1940_latch_gpiochip = {
+	.base				= H1940_LATCH_GPIO(0),
+	.owner				= THIS_MODULE,
+	.label				= "H1940_LATCH",
+	.ngpio				= 16,
+	.direction_input	= h1940_gpiolib_latch_input,
+	.direction_output	= h1940_gpiolib_latch_output,
+	.set				= h1940_gpiolib_latch_set,
+	.get				= h1940_gpiolib_latch_get,
+};
 
 static void h1940_udc_pullup(enum s3c2410_udc_cmd_e cmd)
 {
@@ -124,10 +185,10 @@ static void h1940_udc_pullup(enum s3c2410_udc_cmd_e cmd)
 	switch (cmd)
 	{
 		case S3C2410_UDC_P_ENABLE :
-			h1940_latch_control(0, H1940_LATCH_USB_DP);
+			gpio_set_value(H1940_LATCH_USB_DP, 1);
 			break;
 		case S3C2410_UDC_P_DISABLE :
-			h1940_latch_control(H1940_LATCH_USB_DP, 0);
+			gpio_set_value(H1940_LATCH_USB_DP, 0);
 			break;
 		case S3C2410_UDC_P_RESET :
 			break;
@@ -302,6 +363,8 @@ static void __init h1940_map_io(void)
 	memcpy(phys_to_virt(H1940_SUSPEND_RESUMEAT), h1940_pm_return, 1024);
 #endif
 	s3c_pm_init();
+
+	WARN_ON(gpiochip_add(&h1940_latch_gpiochip));
 }
 
 static void __init h1940_init_irq(void)
@@ -334,9 +397,11 @@ static void __init h1940_init(void)
 	gpio_request(S3C2410_GPC(0), "LCD power");
 	gpio_request(S3C2410_GPC(5), "LCD power");
 	gpio_request(S3C2410_GPC(6), "LCD power");
-
 	gpio_direction_input(S3C2410_GPC(6));
 
+	gpio_request(H1940_LATCH_USB_DP, "USB pullup");
+	gpio_direction_output(H1940_LATCH_USB_DP, 0);
+
 	platform_add_devices(h1940_devices, ARRAY_SIZE(h1940_devices));
 }
 
diff --git a/arch/arm/plat-s3c24xx/Kconfig b/arch/arm/plat-s3c24xx/Kconfig
index 984bf66..5a27b1b 100644
--- a/arch/arm/plat-s3c24xx/Kconfig
+++ b/arch/arm/plat-s3c24xx/Kconfig
@@ -69,6 +69,7 @@ config S3C24XX_GPIO_EXTRA
 	int
 	default 128 if S3C24XX_GPIO_EXTRA128
 	default 64 if S3C24XX_GPIO_EXTRA64
+	default 16 if ARCH_H1940
 	default 0
 
 config S3C24XX_GPIO_EXTRA64
-- 
1.7.2

[PATCH 1/3] h1940: use gpiolib for latch access

[Marek Vasut]

Dne ?t 19. srpna 2010 17:00:01 Vasily Khoruzhick napsal(a):
> This patch adds gpiolib support for h1940 latch.
> With this patch it's possible to use leds-gpio and uda1380
> drivers (they require gpiolib support for appropriate pins).
> And now it's possible to drop leds-h1940 driver.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  arch/arm/mach-s3c2410/h1940-bluetooth.c          |   13 +++-
>  arch/arm/mach-s3c2410/include/mach/h1940-latch.h |   57 +++++-----------
>  arch/arm/mach-s3c2410/mach-h1940.c               |   77
> ++++++++++++++++++++-- arch/arm/plat-s3c24xx/Kconfig                    | 
>   1 +
>  4 files changed, 100 insertions(+), 48 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c2410/h1940-bluetooth.c
> b/arch/arm/mach-s3c2410/h1940-bluetooth.c index 8cdeb14..8aa2f19 100644
> --- a/arch/arm/mach-s3c2410/h1940-bluetooth.c
> +++ b/arch/arm/mach-s3c2410/h1940-bluetooth.c
> @@ -30,7 +30,7 @@ static void h1940bt_enable(int on)
>  {
>  	if (on) {
>  		/* Power on the chip */
> -		h1940_latch_control(0, H1940_LATCH_BLUETOOTH_POWER);
> +		gpio_set_value(H1940_LATCH_BLUETOOTH_POWER, 1);
>  		/* Reset the chip */
>  		mdelay(10);
> 
> @@ -43,7 +43,7 @@ static void h1940bt_enable(int on)
>  		mdelay(10);
>  		gpio_set_value(S3C2410_GPH(1), 0);
>  		mdelay(10);
> -		h1940_latch_control(H1940_LATCH_BLUETOOTH_POWER, 0);
> +		gpio_set_value(H1940_LATCH_BLUETOOTH_POWER, 0);
>  	}
>  }
> 
> @@ -64,7 +64,14 @@ static int __devinit h1940bt_probe(struct
> platform_device *pdev)
> 
>  	ret = gpio_request(S3C2410_GPH(1), dev_name(&pdev->dev));
>  	if (ret) {
> -		dev_err(&pdev->dev, "could not get GPH1\n");\
> +		dev_err(&pdev->dev, "could not get GPH1\n");
> +		return ret;
> +	}
> +
> +	ret = gpio_request(H1940_LATCH_BLUETOOTH_POWER, dev_name(&pdev->dev));

This should contain the name of the GPIO, not dev_name I assume.

> +	if (ret) {
> +		gpio_free(S3C2410_GPH(1));

What's this constant (the 1) here ? Maybe some sane #define wont hurt or comment 
around it.
> +		dev_err(&pdev->dev, "could not get BT_POWER\n");
>  		return ret;
>  	}
> 
> diff --git a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h index d8a8327..73586f2
> 100644
> --- a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> +++ b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> @@ -14,51 +14,30 @@
>  #ifndef __ASM_ARCH_H1940_LATCH_H
>  #define __ASM_ARCH_H1940_LATCH_H
> 
> +#include <mach/gpio.h>
> 
> -#ifndef __ASSEMBLY__
> -#define H1940_LATCH		((void __force __iomem *)0xF8000000)
> -#else
> -#define H1940_LATCH		0xF8000000
> -#endif
> -
> -#define H1940_PA_LATCH		(S3C2410_CS2)
> +#define H1940_LATCH_GPIO(x)		(S3C_GPIO_END + (x))
> 
>  /* SD layer latch */
> 
> -#define H1940_LATCH_SDQ1		(1<<16)
> -#define H1940_LATCH_LCD_P1		(1<<17)
> -#define H1940_LATCH_LCD_P2		(1<<18)
> -#define H1940_LATCH_LCD_P3		(1<<19)
> -#define H1940_LATCH_MAX1698_nSHUTDOWN	(1<<20)		/* LCD backlight 
*/
> -#define H1940_LATCH_LED_RED		(1<<21)
> -#define H1940_LATCH_SDQ7		(1<<22)
> -#define H1940_LATCH_USB_DP		(1<<23)
> +#define H1940_LATCH_SDQ1		H1940_LATCH_GPIO(0)
> +#define H1940_LATCH_LCD_P1		H1940_LATCH_GPIO(1)
> +#define H1940_LATCH_LCD_P2		H1940_LATCH_GPIO(2)
> +#define H1940_LATCH_LCD_P3		H1940_LATCH_GPIO(3)
> +#define H1940_LATCH_MAX1698_nSHUTDOWN	H1940_LATCH_GPIO(4)
> +#define H1940_LATCH_LED_RED		H1940_LATCH_GPIO(5)
> +#define H1940_LATCH_SDQ7		H1940_LATCH_GPIO(6)
> +#define H1940_LATCH_USB_DP		H1940_LATCH_GPIO(7)
> 
>  /* CPU layer latch */
> 
> -#define H1940_LATCH_UDA_POWER		(1<<24)
> -#define H1940_LATCH_AUDIO_POWER		(1<<25)
> -#define H1940_LATCH_SM803_ENABLE	(1<<26)
> -#define H1940_LATCH_LCD_P4		(1<<27)
> -#define H1940_LATCH_CPUQ5		(1<<28)		/* untraced */
> -#define H1940_LATCH_BLUETOOTH_POWER	(1<<29)		/* active high */
> -#define H1940_LATCH_LED_GREEN		(1<<30)
> -#define H1940_LATCH_LED_FLASH		(1<<31)
> -
> -/* default settings */
> -
> -#define H1940_LATCH_DEFAULT		\
> -	H1940_LATCH_LCD_P4		| \
> -	H1940_LATCH_SM803_ENABLE	| \
> -	H1940_LATCH_SDQ1		| \
> -	H1940_LATCH_LCD_P1		| \
> -	H1940_LATCH_LCD_P2		| \
> -	H1940_LATCH_LCD_P3		| \
> -	H1940_LATCH_MAX1698_nSHUTDOWN   | \
> -	H1940_LATCH_CPUQ5
> -
> -/* control functions */
> -
> -extern void h1940_latch_control(unsigned int clear, unsigned int set);
> +#define H1940_LATCH_UDA_POWER		H1940_LATCH_GPIO(8)
> +#define H1940_LATCH_AUDIO_POWER		H1940_LATCH_GPIO(9)
> +#define H1940_LATCH_SM803_ENABLE	H1940_LATCH_GPIO(10)
> +#define H1940_LATCH_LCD_P4		H1940_LATCH_GPIO(11)
> +#define H1940_LATCH_CPUQ5		H1940_LATCH_GPIO(12)
> +#define H1940_LATCH_BLUETOOTH_POWER	H1940_LATCH_GPIO(13)
> +#define H1940_LATCH_LED_GREEN		H1940_LATCH_GPIO(14)
> +#define H1940_LATCH_LED_FLASH		H1940_LATCH_GPIO(15)
> 
>  #endif /* __ASM_ARCH_H1940_LATCH_H */
> diff --git a/arch/arm/mach-s3c2410/mach-h1940.c
> b/arch/arm/mach-s3c2410/mach-h1940.c index 779b45b..9717790 100644
> --- a/arch/arm/mach-s3c2410/mach-h1940.c
> +++ b/arch/arm/mach-s3c2410/mach-h1940.c
> @@ -42,6 +42,7 @@
>  #include <mach/regs-gpio.h>
>  #include <mach/gpio-fns.h>
>  #include <mach/gpio-nrs.h>
> +#include <mach/gpio.h>
> 
>  #include <mach/h1940.h>
>  #include <mach/h1940-latch.h>
> @@ -58,6 +59,16 @@
>  #include <plat/mci.h>
>  #include <plat/ts.h>
> 
> +#ifndef __ASSEMBLY__
> +#define H1940_LATCH		((void __force __iomem *)0xF8000000)
> +#else
> +#define H1940_LATCH		0xF8000000
> +#endif

Is the __ASSEMBLY__ really needed ? You can establish mapping when the kernel 
boots (looks like you're doing that already) but then use __raw_readX 
__raw_writeX to access that space instead of this stuff above.

Possibly like this:

#define	H1940_VA_LATCH 0xf8000000

some_fn()
{
...
	val = __raw_readl(H1940_VA_LATCH + offset);
...
}

Cheers
> +
> +#define H1940_PA_LATCH		(S3C2410_CS2)
> +
> +#define H1940_LATCH_BIT(x)		(1 << ((x) + 16 - S3C_GPIO_END))
> +
>  static struct map_desc h1940_iodesc[] __initdata = {
>  	[0] = {
>  		.virtual	= (unsigned long)H1940_LATCH,
> @@ -99,9 +110,16 @@ static struct s3c2410_uartcfg h1940_uartcfgs[]
> __initdata = {
> 
>  /* Board control latch control */
> 
> -static unsigned int latch_state = H1940_LATCH_DEFAULT;
> +static unsigned int latch_state = H1940_LATCH_BIT(H1940_LATCH_LCD_P4) |
> +	H1940_LATCH_BIT(H1940_LATCH_SM803_ENABLE)	|
> +	H1940_LATCH_BIT(H1940_LATCH_SDQ1)			|
> +	H1940_LATCH_BIT(H1940_LATCH_LCD_P1)			|
> +	H1940_LATCH_BIT(H1940_LATCH_LCD_P2)			|
> +	H1940_LATCH_BIT(H1940_LATCH_LCD_P3)			|
> +	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN)   |
> +	H1940_LATCH_BIT(H1940_LATCH_CPUQ5);
> 
> -void h1940_latch_control(unsigned int clear, unsigned int set)
> +static void h1940_latch_control(unsigned int clear, unsigned int set)
>  {
>  	unsigned long flags;
> 
> @@ -115,7 +133,50 @@ void h1940_latch_control(unsigned int clear, unsigned
> int set) local_irq_restore(flags);
>  }
> 
> -EXPORT_SYMBOL_GPL(h1940_latch_control);
> +static int h1940_gpiolib_latch_input(struct gpio_chip *chip, unsigned
> offset) +{
> +	return -EINVAL;
> +}
> +
> +
> +static inline int h1940_gpiolib_to_latch(int offset)
> +{
> +	return 1 << (offset + 16);
> +}
> +
> +static void h1940_gpiolib_latch_set(struct gpio_chip *chip,
> +					unsigned offset, int value)
> +{
> +	int latch_bit = h1940_gpiolib_to_latch(offset);
> +
> +	h1940_latch_control(value ? 0 : latch_bit,
> +		value ? latch_bit : 0);
> +}
> +
> +static int h1940_gpiolib_latch_output(struct gpio_chip *chip,
> +					unsigned offset, int value)
> +{
> +	h1940_gpiolib_latch_set(chip, offset, value);
> +
> +	return 0;
> +}
> +
> +static int h1940_gpiolib_latch_get(struct gpio_chip *chip,
> +					unsigned offset)
> +{
> +	return (latch_state >> (offset + 16)) & 1;
> +}
> +
> +struct gpio_chip h1940_latch_gpiochip = {
> +	.base				= H1940_LATCH_GPIO(0),
> +	.owner				= THIS_MODULE,
> +	.label				= "H1940_LATCH",
> +	.ngpio				= 16,
> +	.direction_input	= h1940_gpiolib_latch_input,
> +	.direction_output	= h1940_gpiolib_latch_output,
> +	.set				= h1940_gpiolib_latch_set,
> +	.get				= h1940_gpiolib_latch_get,
> +};
> 
>  static void h1940_udc_pullup(enum s3c2410_udc_cmd_e cmd)
>  {
> @@ -124,10 +185,10 @@ static void h1940_udc_pullup(enum s3c2410_udc_cmd_e
> cmd) switch (cmd)
>  	{
>  		case S3C2410_UDC_P_ENABLE :
> -			h1940_latch_control(0, H1940_LATCH_USB_DP);
> +			gpio_set_value(H1940_LATCH_USB_DP, 1);
>  			break;
>  		case S3C2410_UDC_P_DISABLE :
> -			h1940_latch_control(H1940_LATCH_USB_DP, 0);
> +			gpio_set_value(H1940_LATCH_USB_DP, 0);
>  			break;
>  		case S3C2410_UDC_P_RESET :
>  			break;
> @@ -302,6 +363,8 @@ static void __init h1940_map_io(void)
>  	memcpy(phys_to_virt(H1940_SUSPEND_RESUMEAT), h1940_pm_return, 1024);
>  #endif
>  	s3c_pm_init();
> +
> +	WARN_ON(gpiochip_add(&h1940_latch_gpiochip));
>  }
> 
>  static void __init h1940_init_irq(void)
> @@ -334,9 +397,11 @@ static void __init h1940_init(void)
>  	gpio_request(S3C2410_GPC(0), "LCD power");
>  	gpio_request(S3C2410_GPC(5), "LCD power");
>  	gpio_request(S3C2410_GPC(6), "LCD power");
> -
>  	gpio_direction_input(S3C2410_GPC(6));
> 
> +	gpio_request(H1940_LATCH_USB_DP, "USB pullup");
> +	gpio_direction_output(H1940_LATCH_USB_DP, 0);
> +
>  	platform_add_devices(h1940_devices, ARRAY_SIZE(h1940_devices));
>  }
> 
> diff --git a/arch/arm/plat-s3c24xx/Kconfig b/arch/arm/plat-s3c24xx/Kconfig
> index 984bf66..5a27b1b 100644
> --- a/arch/arm/plat-s3c24xx/Kconfig
> +++ b/arch/arm/plat-s3c24xx/Kconfig
> @@ -69,6 +69,7 @@ config S3C24XX_GPIO_EXTRA
>  	int
>  	default 128 if S3C24XX_GPIO_EXTRA128
>  	default 64 if S3C24XX_GPIO_EXTRA64
> +	default 16 if ARCH_H1940
>  	default 0
> 
>  config S3C24XX_GPIO_EXTRA64

[PATCH 1/3] h1940: use gpiolib for latch access

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 16:49:51 ????? Marek Vasut ???????:

> > +	ret = gpio_request(H1940_LATCH_BLUETOOTH_POWER, dev_name(&pdev-
>dev));
> 
> This should contain the name of the GPIO, not dev_name I assume.

Ok

> > +	if (ret) {
> > +		gpio_free(S3C2410_GPH(1));
> 
> What's this constant (the 1) here ? Maybe some sane #define wont hurt or
> comment around it.

GPH(1) specifies gpio bit, that's not magic constant.

> > +#ifndef __ASSEMBLY__
> > +#define H1940_LATCH		((void __force __iomem *)0xF8000000)
> > +#else
> > +#define H1940_LATCH		0xF8000000
> > +#endif
> 
> Is the __ASSEMBLY__ really needed ? You can establish mapping when the
> kernel boots (looks like you're doing that already) but then use
> __raw_readX __raw_writeX to access that space instead of this stuff above.

Uh, I just moved this code from another file, not sure if __ASSEMBLY__ is 
necessary, I'll remove it if it is not.
 
Thanks for review :)

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/f6e24406/attachment-0001.sig>

[PATCH 1/3] h1940: use gpiolib for latch access

[Marek Vasut]

Dne ?t 24. srpna 2010 16:04:00 Vasily Khoruzhick napsal(a):
> ? ????????? ?? 24 ??????? 2010 16:49:51 ????? Marek Vasut ???????:
> > > +	ret = gpio_request(H1940_LATCH_BLUETOOTH_POWER, dev_name(&pdev-
> >
> >dev));
> >
> > This should contain the name of the GPIO, not dev_name I assume.
> 
> Ok
> 
> > > +	if (ret) {
> > > +		gpio_free(S3C2410_GPH(1));
> > 
> > What's this constant (the 1) here ? Maybe some sane #define wont hurt or
> > comment around it.
> 
> GPH(1) specifies gpio bit, that's not magic constant.

Defining S3C2410_GPH_GPIONAME for each bit might be better then.

> 
> > > +#ifndef __ASSEMBLY__
> > > +#define H1940_LATCH		((void __force __iomem *)0xF8000000)
> > > +#else
> > > +#define H1940_LATCH		0xF8000000
> > > +#endif
> > 
> > Is the __ASSEMBLY__ really needed ? You can establish mapping when the
> > kernel boots (looks like you're doing that already) but then use
> > __raw_readX __raw_writeX to access that space instead of this stuff
> > above.
> 
> Uh, I just moved this code from another file, not sure if __ASSEMBLY__ is
> necessary, I'll remove it if it is not.

Well this isn't included in any assembly file. So it's not. Also, I'd be happier 
if you could fix it the way I outlined.

Check for example mach-pxa/balloon3.c and how NAND access is done there. Cheers
> 
> Thanks for review :)

You're welcome.
> 
> Regards
> Vasily

[PATCH 1/3] h1940: use gpiolib for latch access

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 17:40:21 ????? Marek Vasut ???????:

> Defining S3C2410_GPH_GPIONAME for each bit might be better then.

Well, that requires massive refactoring of h1940 (and rx1950) support. And 
btw, that's common for s3c24xx-based machines to use generic s3c24xx gpio 
names, not machine specific.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/51ad203a/attachment.sig>

[PATCH 1/3] h1940: use gpiolib for latch access

[Marek Vasut]

Dne ?t 24. srpna 2010 16:50:36 Vasily Khoruzhick napsal(a):
> ? ????????? ?? 24 ??????? 2010 17:40:21 ????? Marek Vasut ???????:
> > Defining S3C2410_GPH_GPIONAME for each bit might be better then.
> 
> Well, that requires massive refactoring of h1940 (and rx1950) support. And
> btw, that's common for s3c24xx-based machines to use generic s3c24xx gpio
> names, not machine specific.

I assume I see how it works now. Ok, please put a comment around it then so 
others will understand what it does.

Using machine-specific GPIO names might help readability btw. -- the approach 
"everyone does it this way so I'll do it this way without thinking" is bad :)

Well, it's just my suggestion for improvement of the overall code quality. Ben, 
what's your opinion?

Cheers
> 
> Regards
> Vasily

[PATCH 1/3] h1940: use gpiolib for latch access

[Russell King - ARM Linux]

On Thu, Aug 19, 2010 at 06:00:01PM +0300, Vasily Khoruzhick wrote:
> diff --git a/arch/arm/mach-s3c2410/mach-h1940.c b/arch/arm/mach-s3c2410/mach-h1940.c
> index 779b45b..9717790 100644
> --- a/arch/arm/mach-s3c2410/mach-h1940.c
> +++ b/arch/arm/mach-s3c2410/mach-h1940.c
> @@ -42,6 +42,7 @@
>  #include <mach/regs-gpio.h>
>  #include <mach/gpio-fns.h>
>  #include <mach/gpio-nrs.h>
> +#include <mach/gpio.h>

linux/gpio.h ?

>  
>  #include <mach/h1940.h>
>  #include <mach/h1940-latch.h>
> @@ -58,6 +59,16 @@
>  #include <plat/mci.h>
>  #include <plat/ts.h>
>  
> +#ifndef __ASSEMBLY__
> +#define H1940_LATCH		((void __force __iomem *)0xF8000000)
> +#else
> +#define H1940_LATCH		0xF8000000
> +#endif

This file can never be built by the assembler, so the above is pointless.

> +
> +#define H1940_PA_LATCH		(S3C2410_CS2)

Parents not required.

Rest looks fine to me.

[PATCH 2/3] h1940: fix h1940-bluetooth compilation

[Vasily Khoruzhick]

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/mach-s3c2410/h1940-bluetooth.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-s3c2410/h1940-bluetooth.c b/arch/arm/mach-s3c2410/h1940-bluetooth.c
index 8aa2f19..6b86a72 100644
--- a/arch/arm/mach-s3c2410/h1940-bluetooth.c
+++ b/arch/arm/mach-s3c2410/h1940-bluetooth.c
@@ -77,13 +77,13 @@ static int __devinit h1940bt_probe(struct platform_device *pdev)
 
 	/* Configures BT serial port GPIOs */
 	s3c_gpio_cfgpin(S3C2410_GPH(0), S3C2410_GPH0_nCTS0);
-	s3c_gpio_cfgpull(S3C2410_GPH(0), S3C_GPIO_PULL_NONE);
+	s3c_gpio_setpull(S3C2410_GPH(0), S3C_GPIO_PULL_NONE);
 	s3c_gpio_cfgpin(S3C2410_GPH(1), S3C2410_GPIO_OUTPUT);
-	s3c_gpio_cfgpull(S3C2410_GPH(1), S3C_GPIO_PULL_NONE);
+	s3c_gpio_setpull(S3C2410_GPH(1), S3C_GPIO_PULL_NONE);
 	s3c_gpio_cfgpin(S3C2410_GPH(2), S3C2410_GPH2_TXD0);
-	s3c_gpio_cfgpull(S3C2410_GPH(2), S3C_GPIO_PULL_NONE);
+	s3c_gpio_setpull(S3C2410_GPH(2), S3C_GPIO_PULL_NONE);
 	s3c_gpio_cfgpin(S3C2410_GPH(3), S3C2410_GPH3_RXD0);
-	s3c_gpio_cfgpull(S3C2410_GPH(3), S3C_GPIO_PULL_NONE);
+	s3c_gpio_setpull(S3C2410_GPH(3), S3C_GPIO_PULL_NONE);
 
 
 	rfk = rfkill_alloc(DRV_NAME, &pdev->dev, RFKILL_TYPE_BLUETOOTH,
-- 
1.7.2

[PATCH 2/3] h1940: fix h1940-bluetooth compilation

[Marek Vasut]

Dne ?t 19. srpna 2010 17:00:02 Vasily Khoruzhick napsal(a):
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

Errr ... fix ... in what way? What was broken? I don't understand, please add 
more explanatory text. Thanks
Cheers
> ---
>  arch/arm/mach-s3c2410/h1940-bluetooth.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c2410/h1940-bluetooth.c
> b/arch/arm/mach-s3c2410/h1940-bluetooth.c index 8aa2f19..6b86a72 100644
> --- a/arch/arm/mach-s3c2410/h1940-bluetooth.c
> +++ b/arch/arm/mach-s3c2410/h1940-bluetooth.c
> @@ -77,13 +77,13 @@ static int __devinit h1940bt_probe(struct
> platform_device *pdev)
> 
>  	/* Configures BT serial port GPIOs */
>  	s3c_gpio_cfgpin(S3C2410_GPH(0), S3C2410_GPH0_nCTS0);
> -	s3c_gpio_cfgpull(S3C2410_GPH(0), S3C_GPIO_PULL_NONE);
> +	s3c_gpio_setpull(S3C2410_GPH(0), S3C_GPIO_PULL_NONE);
>  	s3c_gpio_cfgpin(S3C2410_GPH(1), S3C2410_GPIO_OUTPUT);
> -	s3c_gpio_cfgpull(S3C2410_GPH(1), S3C_GPIO_PULL_NONE);
> +	s3c_gpio_setpull(S3C2410_GPH(1), S3C_GPIO_PULL_NONE);
>  	s3c_gpio_cfgpin(S3C2410_GPH(2), S3C2410_GPH2_TXD0);
> -	s3c_gpio_cfgpull(S3C2410_GPH(2), S3C_GPIO_PULL_NONE);
> +	s3c_gpio_setpull(S3C2410_GPH(2), S3C_GPIO_PULL_NONE);
>  	s3c_gpio_cfgpin(S3C2410_GPH(3), S3C2410_GPH3_RXD0);
> -	s3c_gpio_cfgpull(S3C2410_GPH(3), S3C_GPIO_PULL_NONE);
> +	s3c_gpio_setpull(S3C2410_GPH(3), S3C_GPIO_PULL_NONE);
> 
> 
>  	rfk = rfkill_alloc(DRV_NAME, &pdev->dev, RFKILL_TYPE_BLUETOOTH,

[PATCH 2/3] h1940: fix h1940-bluetooth compilation

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 16:50:46 ????? Marek Vasut ???????:
> Dne ?t 19. srpna 2010 17:00:02 Vasily Khoruzhick napsal(a):
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> 
> Errr ... fix ... in what way? What was broken? I don't understand, please
> add more explanatory text. Thanks
> Cheers

It's compile fix, without this patch it just doesn't compile.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/40d09b73/attachment.sig>

[PATCH 2/3] h1940: fix h1940-bluetooth compilation

[Marek Vasut]

Dne ?t 24. srpna 2010 15:57:07 Vasily Khoruzhick napsal(a):
> ? ????????? ?? 24 ??????? 2010 16:50:46 ????? Marek Vasut ???????:
> > Dne ?t 19. srpna 2010 17:00:02 Vasily Khoruzhick napsal(a):
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > 
> > Errr ... fix ... in what way? What was broken? I don't understand, please
> > add more explanatory text. Thanks
> > Cheers
> 
> It's compile fix, without this patch it just doesn't compile.

Because the GPIO api changed or what ? Please stick this text into the next 
version of the patch.

Thanks!

[PATCH 3/3] h1940: implement mmc_power function

[Vasily Khoruzhick]

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 arch/arm/mach-s3c2410/include/mach/h1940-latch.h |    2 +-
 arch/arm/mach-s3c2410/mach-h1940.c               |   23 +++++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
index 73586f2..ef7d8cf 100644
--- a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
+++ b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
@@ -35,7 +35,7 @@
 #define H1940_LATCH_AUDIO_POWER		H1940_LATCH_GPIO(9)
 #define H1940_LATCH_SM803_ENABLE	H1940_LATCH_GPIO(10)
 #define H1940_LATCH_LCD_P4		H1940_LATCH_GPIO(11)
-#define H1940_LATCH_CPUQ5		H1940_LATCH_GPIO(12)
+#define H1940_LATCH_SD_POWER		H1940_LATCH_GPIO(12)
 #define H1940_LATCH_BLUETOOTH_POWER	H1940_LATCH_GPIO(13)
 #define H1940_LATCH_LED_GREEN		H1940_LATCH_GPIO(14)
 #define H1940_LATCH_LED_FLASH		H1940_LATCH_GPIO(15)
diff --git a/arch/arm/mach-s3c2410/mach-h1940.c b/arch/arm/mach-s3c2410/mach-h1940.c
index 9717790..c1ccc8e 100644
--- a/arch/arm/mach-s3c2410/mach-h1940.c
+++ b/arch/arm/mach-s3c2410/mach-h1940.c
@@ -116,8 +116,7 @@ static unsigned int latch_state = H1940_LATCH_BIT(H1940_LATCH_LCD_P4) |
 	H1940_LATCH_BIT(H1940_LATCH_LCD_P1)			|
 	H1940_LATCH_BIT(H1940_LATCH_LCD_P2)			|
 	H1940_LATCH_BIT(H1940_LATCH_LCD_P3)			|
-	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN)   |
-	H1940_LATCH_BIT(H1940_LATCH_CPUQ5);
+	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN);
 
 static void h1940_latch_control(unsigned int clear, unsigned int set)
 {
@@ -259,10 +258,25 @@ static struct platform_device h1940_device_bluetooth = {
 	.id               = -1,
 };
 
+static void h1940_set_mmc_power(unsigned char power_mode, unsigned short vdd)
+{
+	switch (power_mode) {
+	case MMC_POWER_OFF:
+		gpio_set_value(H1940_LATCH_SD_POWER, 0);
+		break;
+	case MMC_POWER_UP:
+	case MMC_POWER_ON:
+		gpio_set_value(H1940_LATCH_SD_POWER, 1);
+		break;
+	default:
+		break;
+	};
+}
+
 static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = {
 	.gpio_detect   = S3C2410_GPF(5),
 	.gpio_wprotect = S3C2410_GPH(8),
-	.set_power     = NULL,
+	.set_power     = h1940_set_mmc_power,
 	.ocr_avail     = MMC_VDD_32_33,
 };
 
@@ -402,6 +416,9 @@ static void __init h1940_init(void)
 	gpio_request(H1940_LATCH_USB_DP, "USB pullup");
 	gpio_direction_output(H1940_LATCH_USB_DP, 0);
 
+	gpio_request(H1940_LATCH_SD_POWER, "SD power");
+	gpio_direction_output(H1940_LATCH_SD_POWER, 0);
+
 	platform_add_devices(h1940_devices, ARRAY_SIZE(h1940_devices));
 }
 
-- 
1.7.2

[PATCH 3/3] h1940: implement mmc_power function

[Marek Vasut]

Dne ?t 19. srpna 2010 17:00:03 Vasily Khoruzhick napsal(a):
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  arch/arm/mach-s3c2410/include/mach/h1940-latch.h |    2 +-
>  arch/arm/mach-s3c2410/mach-h1940.c               |   23
> +++++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h index 73586f2..ef7d8cf
> 100644
> --- a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> +++ b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> @@ -35,7 +35,7 @@
>  #define H1940_LATCH_AUDIO_POWER		H1940_LATCH_GPIO(9)
>  #define H1940_LATCH_SM803_ENABLE	H1940_LATCH_GPIO(10)
>  #define H1940_LATCH_LCD_P4		H1940_LATCH_GPIO(11)
> -#define H1940_LATCH_CPUQ5		H1940_LATCH_GPIO(12)
> +#define H1940_LATCH_SD_POWER		H1940_LATCH_GPIO(12)
>  #define H1940_LATCH_BLUETOOTH_POWER	H1940_LATCH_GPIO(13)
>  #define H1940_LATCH_LED_GREEN		H1940_LATCH_GPIO(14)
>  #define H1940_LATCH_LED_FLASH		H1940_LATCH_GPIO(15)
> diff --git a/arch/arm/mach-s3c2410/mach-h1940.c
> b/arch/arm/mach-s3c2410/mach-h1940.c index 9717790..c1ccc8e 100644
> --- a/arch/arm/mach-s3c2410/mach-h1940.c
> +++ b/arch/arm/mach-s3c2410/mach-h1940.c
> @@ -116,8 +116,7 @@ static unsigned int latch_state =
> H1940_LATCH_BIT(H1940_LATCH_LCD_P4) |
> H1940_LATCH_BIT(H1940_LATCH_LCD_P1)			|
>  	H1940_LATCH_BIT(H1940_LATCH_LCD_P2)			|
>  	H1940_LATCH_BIT(H1940_LATCH_LCD_P3)			|
> -	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN)   |
> -	H1940_LATCH_BIT(H1940_LATCH_CPUQ5);
> +	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN);
> 
>  static void h1940_latch_control(unsigned int clear, unsigned int set)
>  {
> @@ -259,10 +258,25 @@ static struct platform_device h1940_device_bluetooth
> = { .id               = -1,
>  };
> 
> +static void h1940_set_mmc_power(unsigned char power_mode, unsigned short
> vdd) +{
> +	switch (power_mode) {
> +	case MMC_POWER_OFF:
> +		gpio_set_value(H1940_LATCH_SD_POWER, 0);
> +		break;
> +	case MMC_POWER_UP:
> +	case MMC_POWER_ON:
> +		gpio_set_value(H1940_LATCH_SD_POWER, 1);
> +		break;
> +	default:
> +		break;
> +	};
> +}
> +
>  static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = {
>  	.gpio_detect   = S3C2410_GPF(5),
>  	.gpio_wprotect = S3C2410_GPH(8),
> -	.set_power     = NULL,
> +	.set_power     = h1940_set_mmc_power,
>  	.ocr_avail     = MMC_VDD_32_33,
>  };
> 

Can't you implement gpio_power into the s3c24xx mmc driver ? Then you can fix 
mach-n30 too.

> @@ -402,6 +416,9 @@ static void __init h1940_init(void)
>  	gpio_request(H1940_LATCH_USB_DP, "USB pullup");
>  	gpio_direction_output(H1940_LATCH_USB_DP, 0);
> 
> +	gpio_request(H1940_LATCH_SD_POWER, "SD power");
> +	gpio_direction_output(H1940_LATCH_SD_POWER, 0);

Please handle possible return values here !
> +
>  	platform_add_devices(h1940_devices, ARRAY_SIZE(h1940_devices));
>  }

Cheers

[PATCH 3/3] h1940: implement mmc_power function

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 16:52:42 ????? Marek Vasut ???????:
> Dne ?t 19. srpna 2010 17:00:03 Vasily Khoruzhick napsal(a):
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> > 
> >  arch/arm/mach-s3c2410/include/mach/h1940-latch.h |    2 +-
> >  arch/arm/mach-s3c2410/mach-h1940.c               |   23
> > 
> > +++++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> > b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h index 73586f2..ef7d8cf
> > 100644
> > --- a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> > +++ b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> > @@ -35,7 +35,7 @@
> > 
> >  #define H1940_LATCH_AUDIO_POWER		H1940_LATCH_GPIO(9)
> >  #define H1940_LATCH_SM803_ENABLE	H1940_LATCH_GPIO(10)
> >  #define H1940_LATCH_LCD_P4		H1940_LATCH_GPIO(11)
> > 
> > -#define H1940_LATCH_CPUQ5		H1940_LATCH_GPIO(12)
> > +#define H1940_LATCH_SD_POWER		H1940_LATCH_GPIO(12)
> > 
> >  #define H1940_LATCH_BLUETOOTH_POWER	H1940_LATCH_GPIO(13)
> >  #define H1940_LATCH_LED_GREEN		H1940_LATCH_GPIO(14)
> >  #define H1940_LATCH_LED_FLASH		H1940_LATCH_GPIO(15)
> > 
> > diff --git a/arch/arm/mach-s3c2410/mach-h1940.c
> > b/arch/arm/mach-s3c2410/mach-h1940.c index 9717790..c1ccc8e 100644
> > --- a/arch/arm/mach-s3c2410/mach-h1940.c
> > +++ b/arch/arm/mach-s3c2410/mach-h1940.c
> > @@ -116,8 +116,7 @@ static unsigned int latch_state =
> > H1940_LATCH_BIT(H1940_LATCH_LCD_P4) |
> > H1940_LATCH_BIT(H1940_LATCH_LCD_P1)			|
> > 
> >  	H1940_LATCH_BIT(H1940_LATCH_LCD_P2)			|
> >  	H1940_LATCH_BIT(H1940_LATCH_LCD_P3)			|
> > 
> > -	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN)   |
> > -	H1940_LATCH_BIT(H1940_LATCH_CPUQ5);
> > +	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN);
> > 
> >  static void h1940_latch_control(unsigned int clear, unsigned int set)
> >  {
> > 
> > @@ -259,10 +258,25 @@ static struct platform_device
> > h1940_device_bluetooth = { .id               = -1,
> > 
> >  };
> > 
> > +static void h1940_set_mmc_power(unsigned char power_mode, unsigned short
> > vdd) +{
> > +	switch (power_mode) {
> > +	case MMC_POWER_OFF:
> > +		gpio_set_value(H1940_LATCH_SD_POWER, 0);
> > +		break;
> > +	case MMC_POWER_UP:
> > +	case MMC_POWER_ON:
> > +		gpio_set_value(H1940_LATCH_SD_POWER, 1);
> > +		break;
> > +	default:
> > +		break;
> > +	};
> > +}
> > +
> > 
> >  static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = {
> >  
> >  	.gpio_detect   = S3C2410_GPF(5),
> >  	.gpio_wprotect = S3C2410_GPH(8),
> > 
> > -	.set_power     = NULL,
> > +	.set_power     = h1940_set_mmc_power,
> > 
> >  	.ocr_avail     = MMC_VDD_32_33,
> >  
> >  };
> 
> Can't you implement gpio_power into the s3c24xx mmc driver ? Then you can
> fix mach-n30 too.

Well, I'm happy with callback. However, maybe it worth to replace set_power 
callback with regulator.

> > @@ -402,6 +416,9 @@ static void __init h1940_init(void)
> > 
> >  	gpio_request(H1940_LATCH_USB_DP, "USB pullup");
> >  	gpio_direction_output(H1940_LATCH_USB_DP, 0);
> > 
> > +	gpio_request(H1940_LATCH_SD_POWER, "SD power");
> > +	gpio_direction_output(H1940_LATCH_SD_POWER, 0);
> 
> Please handle possible return values here !

Ok, I suppose WARN_ON will be enought?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/80915fef/attachment.sig>

[PATCH 3/3] h1940: implement mmc_power function

[Marek Vasut]

Dne ?t 24. srpna 2010 16:00:26 Vasily Khoruzhick napsal(a):
> ? ????????? ?? 24 ??????? 2010 16:52:42 ????? Marek Vasut ???????:
> > Dne ?t 19. srpna 2010 17:00:03 Vasily Khoruzhick napsal(a):
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > ---
> > > 
> > >  arch/arm/mach-s3c2410/include/mach/h1940-latch.h |    2 +-
> > >  arch/arm/mach-s3c2410/mach-h1940.c               |   23
> > > 
> > > +++++++++++++++++++-- 2 files changed, 21 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> > > b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h index
> > > 73586f2..ef7d8cf 100644
> > > --- a/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> > > +++ b/arch/arm/mach-s3c2410/include/mach/h1940-latch.h
> > > @@ -35,7 +35,7 @@
> > > 
> > >  #define H1940_LATCH_AUDIO_POWER		H1940_LATCH_GPIO(9)
> > >  #define H1940_LATCH_SM803_ENABLE	H1940_LATCH_GPIO(10)
> > >  #define H1940_LATCH_LCD_P4		H1940_LATCH_GPIO(11)
> > > 
> > > -#define H1940_LATCH_CPUQ5		H1940_LATCH_GPIO(12)
> > > +#define H1940_LATCH_SD_POWER		H1940_LATCH_GPIO(12)
> > > 
> > >  #define H1940_LATCH_BLUETOOTH_POWER	H1940_LATCH_GPIO(13)
> > >  #define H1940_LATCH_LED_GREEN		H1940_LATCH_GPIO(14)
> > >  #define H1940_LATCH_LED_FLASH		H1940_LATCH_GPIO(15)
> > > 
> > > diff --git a/arch/arm/mach-s3c2410/mach-h1940.c
> > > b/arch/arm/mach-s3c2410/mach-h1940.c index 9717790..c1ccc8e 100644
> > > --- a/arch/arm/mach-s3c2410/mach-h1940.c
> > > +++ b/arch/arm/mach-s3c2410/mach-h1940.c
> > > @@ -116,8 +116,7 @@ static unsigned int latch_state =
> > > H1940_LATCH_BIT(H1940_LATCH_LCD_P4) |
> > > H1940_LATCH_BIT(H1940_LATCH_LCD_P1)			|
> > > 
> > >  	H1940_LATCH_BIT(H1940_LATCH_LCD_P2)			|
> > >  	H1940_LATCH_BIT(H1940_LATCH_LCD_P3)			|
> > > 
> > > -	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN)   |
> > > -	H1940_LATCH_BIT(H1940_LATCH_CPUQ5);
> > > +	H1940_LATCH_BIT(H1940_LATCH_MAX1698_nSHUTDOWN);
> > > 
> > >  static void h1940_latch_control(unsigned int clear, unsigned int set)
> > >  {
> > > 
> > > @@ -259,10 +258,25 @@ static struct platform_device
> > > h1940_device_bluetooth = { .id               = -1,
> > > 
> > >  };
> > > 
> > > +static void h1940_set_mmc_power(unsigned char power_mode, unsigned
> > > short vdd) +{
> > > +	switch (power_mode) {
> > > +	case MMC_POWER_OFF:
> > > +		gpio_set_value(H1940_LATCH_SD_POWER, 0);
> > > +		break;
> > > +	case MMC_POWER_UP:
> > > +	case MMC_POWER_ON:
> > > +		gpio_set_value(H1940_LATCH_SD_POWER, 1);
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	};
> > > +}
> > > +
> > > 
> > >  static struct s3c24xx_mci_pdata h1940_mmc_cfg __initdata = {
> > >  
> > >  	.gpio_detect   = S3C2410_GPF(5),
> > >  	.gpio_wprotect = S3C2410_GPH(8),
> > > 
> > > -	.set_power     = NULL,
> > > +	.set_power     = h1940_set_mmc_power,
> > > 
> > >  	.ocr_avail     = MMC_VDD_32_33,
> > >  
> > >  };
> > 
> > Can't you implement gpio_power into the s3c24xx mmc driver ? Then you can
> > fix mach-n30 too.
> 
> Well, I'm happy with callback. However, maybe it worth to replace set_power
> callback with regulator.
> 
> > > @@ -402,6 +416,9 @@ static void __init h1940_init(void)
> > > 
> > >  	gpio_request(H1940_LATCH_USB_DP, "USB pullup");
> > >  	gpio_direction_output(H1940_LATCH_USB_DP, 0);
> > > 
> > > +	gpio_request(H1940_LATCH_SD_POWER, "SD power");
> > > +	gpio_direction_output(H1940_LATCH_SD_POWER, 0);
> > 
> > Please handle possible return values here !
> 
> Ok, I suppose WARN_ON will be enought?

pr_err() and don't probe the device for which you couldn't request GPIO.

Cheers

[PATCH 3/3] h1940: implement mmc_power function

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 17:01:12 ????? Marek Vasut ???????:

> pr_err() and don't probe the device for which you couldn't request GPIO.

Well, handling those errors in machine init functions doesn't make sense. PDA 
is not usable without basic hardware.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/828fe421/attachment.sig>

[PATCH 3/3] h1940: implement mmc_power function

[Marek Vasut]

Dne ?t 24. srpna 2010 16:21:01 Vasily Khoruzhick napsal(a):
> ? ????????? ?? 24 ??????? 2010 17:01:12 ????? Marek Vasut ???????:
> > pr_err() and don't probe the device for which you couldn't request GPIO.
> 
> Well, handling those errors in machine init functions doesn't make sense.
> PDA is not usable without basic hardware.
> 
You can use PDA without LCD for example (there are such people, trust me). So, 
this comment really isn't too valid. You can also use PDA without MMC support, 
you still have onboard flash, etc. etc.

Cheers

> Regards
> Vasily

[PATCH 3/3] h1940: implement mmc_power function

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 17:41:57 ????? Marek Vasut ???????:
> Dne ?t 24. srpna 2010 16:21:01 Vasily Khoruzhick napsal(a):
> > ? ????????? ?? 24 ??????? 2010 17:01:12 ????? Marek Vasut ???????:
> > > pr_err() and don't probe the device for which you couldn't request
> > > GPIO.
> > 
> > Well, handling those errors in machine init functions doesn't make sense.
> > PDA is not usable without basic hardware.
> 
> You can use PDA without LCD for example (there are such people, trust me).
> So, this comment really isn't too valid. You can also use PDA without MMC
> support, you still have onboard flash, etc. etc.

But error handling here just increases code size, and gpio_request in init 
function should always succeed. It means something's going really wrong 
(kernel bug or misconfiguration) if gpio_request fails. Btw, Ben suggested to 
use WARN_ON in machine init, look through init functions of qt2410, n30, 
rx1950, vr1000, etc.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/f1a44531/attachment.sig>

[PATCH 3/3] h1940: implement mmc_power function

[Marek Vasut]

Dne ?t 24. srpna 2010 16:56:06 Vasily Khoruzhick napsal(a):
> ? ????????? ?? 24 ??????? 2010 17:41:57 ????? Marek Vasut ???????:
> > Dne ?t 24. srpna 2010 16:21:01 Vasily Khoruzhick napsal(a):
> > > ? ????????? ?? 24 ??????? 2010 17:01:12 ????? Marek Vasut ???????:
> > > > pr_err() and don't probe the device for which you couldn't request
> > > > GPIO.
> > > 
> > > Well, handling those errors in machine init functions doesn't make
> > > sense. PDA is not usable without basic hardware.
> > 
> > You can use PDA without LCD for example (there are such people, trust
> > me). So, this comment really isn't too valid. You can also use PDA
> > without MMC support, you still have onboard flash, etc. etc.
> 
> But error handling here just increases code size, and gpio_request in init
> function should always succeed. It means something's going really wrong
> (kernel bug or misconfiguration) if gpio_request fails. Btw, Ben suggested
> to use WARN_ON in machine init, look through init functions of qt2410,
> n30, rx1950, vr1000, etc.

See my previous response. It'd increase code size, but if you mark that code 
__init, it'll be freed after that function finishes so it doesn't matter.

If something wrong goes on, you can still debug it without for example MMC card 
(though if you continued probing the MMC driver, it might cause additional 
unwanted noise and make the debugging harder). That's the whole point.

Cheers
> 
> Regards
> Vasily

[PATCH 3/3] h1940: implement mmc_power function

[Vasily Khoruzhick]

? ????????? ?? 24 ??????? 2010 18:07:31 ????? Marek Vasut ???????:
> See my previous response. It'd increase code size, but if you mark that
> code __init, it'll be freed after that function finishes so it doesn't
> matter.
> 
> If something wrong goes on, you can still debug it without for example MMC
> card (though if you continued probing the MMC driver, it might cause
> additional unwanted noise and make the debugging harder). That's the whole
> point.

Ok, but I also want to hear Ben's opinion on this point.

Regards
Vasily
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/247d8297/attachment.sig>

[PATCH 0/3] h1940 series

[Vasily Khoruzhick]

? ????????? ?? 19 ??????? 2010 18:00:00 ????? Vasily Khoruzhick ???????:
> This patch series replaces h1940-specific latch-related
> functions with gpiolib, fixes compilation error for h1940-bluetooth
> and adds PM support for mmc driver.

Ping
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100824/51866d68/attachment.sig>