[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Fabio Estevam]

Currently framebuffer and MMC devices are registered even if their associated 
GPIO pins fail to be requested.

Change the logic so that the registration of such devices only occurs if their 
GPIO requests succeeded.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-mxs/mach-mx28evk.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index 3f86e7a..e5c66a2 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -353,7 +353,7 @@ static struct mxs_mmc_platform_data mx28evk_mmc_pdata[] __initdata = {
 
 static void __init mx28evk_init(void)
 {
-	int ret;
+	int ret, gpio_lcd_error;
 
 	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
 
@@ -378,18 +378,23 @@ static void __init mx28evk_init(void)
 	}
 
 	ret = gpio_request_one(MX28EVK_LCD_ENABLE, GPIOF_DIR_OUT, "lcd-enable");
-	if (ret)
+	if (ret) {
 		pr_warn("failed to request gpio lcd-enable: %d\n", ret);
+		gpio_lcd_error = 1;
+	}
 	else
 		gpio_set_value(MX28EVK_LCD_ENABLE, 1);
 
 	ret = gpio_request_one(MX28EVK_BL_ENABLE, GPIOF_DIR_OUT, "bl-enable");
-	if (ret)
+	if (ret) {
 		pr_warn("failed to request gpio bl-enable: %d\n", ret);
+		gpio_lcd_error = 1;
+	}
 	else
 		gpio_set_value(MX28EVK_BL_ENABLE, 1);
 
-	mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
+	if (!gpio_lcd_error)
+		mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
 
 	/* power on mmc slot by writing 0 to the gpio */
 	ret = gpio_request_one(MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW,
@@ -402,8 +407,9 @@ static void __init mx28evk_init(void)
 			       "mmc1-slot-power");
 	if (ret)
 		pr_warn("failed to request gpio mmc1-slot-power: %d\n", ret);
-	mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
-
+	else
+		mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
+
 	gpio_led_register_device(0, &mx28evk_led_data);
 }
 
-- 
1.7.1

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Shawn Guo]

On Tue, Sep 13, 2011 at 05:25:19PM -0300, Fabio Estevam wrote:
> Currently framebuffer and MMC devices are registered even if their associated 
> GPIO pins fail to be requested.
> 
> Change the logic so that the registration of such devices only occurs if their 
> GPIO requests succeeded.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
So the question is whether the gpio request failure is a show-stopper
failure for framebuffer and mmc.

-- 
Regards,
Shawn

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Fabio Estevam]

On Wed, Sep 14, 2011 at 12:18 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Sep 13, 2011 at 05:25:19PM -0300, Fabio Estevam wrote:
>> Currently framebuffer and MMC devices are registered even if their associated
>> GPIO pins fail to be requested.
>>
>> Change the logic so that the registration of such devices only occurs if their
>> GPIO requests succeeded.
>>
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
> So the question is whether the gpio request failure is a show-stopper
> failure for framebuffer and mmc.

If the GPIO that controls LCD enable or backlight enable failed it is
not very useful to register the framebuffer, do you agree?

Same for MMC.

You did the same logic for the CAN bus, so I extended it for the
framebuffer and MMC.

Regards,

Fabio Estevam

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Shawn Guo]

On Wed, Sep 14, 2011 at 12:12:40AM -0300, Fabio Estevam wrote:
> On Wed, Sep 14, 2011 at 12:18 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> > On Tue, Sep 13, 2011 at 05:25:19PM -0300, Fabio Estevam wrote:
> >> Currently framebuffer and MMC devices are registered even if their associated
> >> GPIO pins fail to be requested.
> >>
> >> Change the logic so that the registration of such devices only occurs if their
> >> GPIO requests succeeded.
> >>
> >> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> >> ---
> > So the question is whether the gpio request failure is a show-stopper
> > failure for framebuffer and mmc.
> 
> If the GPIO that controls LCD enable or backlight enable failed it is
> not very useful to register the framebuffer, do you agree?
> 
> Same for MMC.
> 
Agreed.

Regards,
Shawn

> You did the same logic for the CAN bus, so I extended it for the
> framebuffer and MMC.

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Shawn Guo]

On Tue, Sep 13, 2011 at 05:25:19PM -0300, Fabio Estevam wrote:
> Currently framebuffer and MMC devices are registered even if their associated 
> GPIO pins fail to be requested.
> 
> Change the logic so that the registration of such devices only occurs if their 
> GPIO requests succeeded.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-mxs/mach-mx28evk.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index 3f86e7a..e5c66a2 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -353,7 +353,7 @@ static struct mxs_mmc_platform_data mx28evk_mmc_pdata[] __initdata = {
>  
>  static void __init mx28evk_init(void)
>  {
> -	int ret;
> +	int ret, gpio_lcd_error;

Shouldn't gpio_lcd_error be initialized as 0?

>  
>  	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
>  
> @@ -378,18 +378,23 @@ static void __init mx28evk_init(void)
>  	}
>  
>  	ret = gpio_request_one(MX28EVK_LCD_ENABLE, GPIOF_DIR_OUT, "lcd-enable");
> -	if (ret)
> +	if (ret) {
>  		pr_warn("failed to request gpio lcd-enable: %d\n", ret);
> +		gpio_lcd_error = 1;
> +	}
>  	else
>  		gpio_set_value(MX28EVK_LCD_ENABLE, 1);

Per Documentation/CodingStyle, this should be like the following.

if (condition) {
        do_this();
        do_that();
} else {
        otherwise();
}

>  
>  	ret = gpio_request_one(MX28EVK_BL_ENABLE, GPIOF_DIR_OUT, "bl-enable");
> -	if (ret)
> +	if (ret) {
>  		pr_warn("failed to request gpio bl-enable: %d\n", ret);
> +		gpio_lcd_error = 1;
> +	}
>  	else
>  		gpio_set_value(MX28EVK_BL_ENABLE, 1);

Ditto

Otherwise, Acked-by: Shawn Guo <shawn.guo@linaro.org>

Regards,
Shawn

>  
> -	mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
> +	if (!gpio_lcd_error)
> +		mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
>  
>  	/* power on mmc slot by writing 0 to the gpio */
>  	ret = gpio_request_one(MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW,
> @@ -402,8 +407,9 @@ static void __init mx28evk_init(void)
>  			       "mmc1-slot-power");
>  	if (ret)
>  		pr_warn("failed to request gpio mmc1-slot-power: %d\n", ret);
> -	mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
> -
> +	else
> +		mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
> +
>  	gpio_led_register_device(0, &mx28evk_led_data);
>  }
>  
> -- 
> 1.7.1

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Uwe Kleine-König]

Hello,

On Tue, Sep 13, 2011 at 05:25:19PM -0300, Fabio Estevam wrote:
> Currently framebuffer and MMC devices are registered even if their associated 
> GPIO pins fail to be requested.
Is this a real or only theoretic problem? (For me (or later Sascha) to
judge if it should go in before 3.2.)

> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index 3f86e7a..e5c66a2 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -353,7 +353,7 @@ static struct mxs_mmc_platform_data mx28evk_mmc_pdata[] __initdata = {
>  
>  static void __init mx28evk_init(void)
>  {
> -	int ret;
> +	int ret, gpio_lcd_error;
>  
>  	mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
>  
> @@ -378,18 +378,23 @@ static void __init mx28evk_init(void)
>  	}
>  
>  	ret = gpio_request_one(MX28EVK_LCD_ENABLE, GPIOF_DIR_OUT, "lcd-enable");
> -	if (ret)
> +	if (ret) {
>  		pr_warn("failed to request gpio lcd-enable: %d\n", ret);
> +		gpio_lcd_error = 1;
> +	}
>  	else
>  		gpio_set_value(MX28EVK_LCD_ENABLE, 1);
>  
>  	ret = gpio_request_one(MX28EVK_BL_ENABLE, GPIOF_DIR_OUT, "bl-enable");
> -	if (ret)
> +	if (ret) {
>  		pr_warn("failed to request gpio bl-enable: %d\n", ret);
> +		gpio_lcd_error = 1;
> +	}
>  	else
>  		gpio_set_value(MX28EVK_BL_ENABLE, 1);
If it's not important which gpio failed, you can do:

	ret = gpio_request_array(...)
	if (ret)
		pr_warn("failed to request gpio for lcd\n");
	else
		mx28_add_mxsfb(&mx28evk_mxsfb_pdata);

(Apart from the different message this differs in semantic when the 2nd
request fails. With my suggestion the first gpio is freed then which
seems cleaner.) I don't care much though.
		
> -	mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
> +	if (!gpio_lcd_error)
> +		mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
>  
>  	/* power on mmc slot by writing 0 to the gpio */
>  	ret = gpio_request_one(MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW,
> @@ -402,8 +407,9 @@ static void __init mx28evk_init(void)
>  			       "mmc1-slot-power");
>  	if (ret)
>  		pr_warn("failed to request gpio mmc1-slot-power: %d\n", ret);
> -	mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
> -
> +	else
> +		mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
> +
>  	gpio_led_register_device(0, &mx28evk_led_data);
>  }

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: mach-mxs/mx28evk: Only register devices if their GPIO requests succeeded

[Fabio Estevam]

2011/9/14 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> Hello,
>
> On Tue, Sep 13, 2011 at 05:25:19PM -0300, Fabio Estevam wrote:
>> Currently framebuffer and MMC devices are registered even if their associated
>> GPIO pins fail to be requested.
> Is this a real or only theoretic problem? (For me (or later Sascha) to
> judge if it should go in before 3.2.)

I haven't seen this to happen, but it is the safe thing to do here.

I think this can wait 3.2 cycle.

>
>> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
>> index 3f86e7a..e5c66a2 100644
>> --- a/arch/arm/mach-mxs/mach-mx28evk.c
>> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
>> @@ -353,7 +353,7 @@ static struct mxs_mmc_platform_data mx28evk_mmc_pdata[] __initdata = {
>>
>> ?static void __init mx28evk_init(void)
>> ?{
>> - ? ? int ret;
>> + ? ? int ret, gpio_lcd_error;
>>
>> ? ? ? mxs_iomux_setup_multiple_pads(mx28evk_pads, ARRAY_SIZE(mx28evk_pads));
>>
>> @@ -378,18 +378,23 @@ static void __init mx28evk_init(void)
>> ? ? ? }
>>
>> ? ? ? ret = gpio_request_one(MX28EVK_LCD_ENABLE, GPIOF_DIR_OUT, "lcd-enable");
>> - ? ? if (ret)
>> + ? ? if (ret) {
>> ? ? ? ? ? ? ? pr_warn("failed to request gpio lcd-enable: %d\n", ret);
>> + ? ? ? ? ? ? gpio_lcd_error = 1;
>> + ? ? }
>> ? ? ? else
>> ? ? ? ? ? ? ? gpio_set_value(MX28EVK_LCD_ENABLE, 1);
>>
>> ? ? ? ret = gpio_request_one(MX28EVK_BL_ENABLE, GPIOF_DIR_OUT, "bl-enable");
>> - ? ? if (ret)
>> + ? ? if (ret) {
>> ? ? ? ? ? ? ? pr_warn("failed to request gpio bl-enable: %d\n", ret);
>> + ? ? ? ? ? ? gpio_lcd_error = 1;
>> + ? ? }
>> ? ? ? else
>> ? ? ? ? ? ? ? gpio_set_value(MX28EVK_BL_ENABLE, 1);
> If it's not important which gpio failed, you can do:
>
> ? ? ? ?ret = gpio_request_array(...)
> ? ? ? ?if (ret)
> ? ? ? ? ? ? ? ?pr_warn("failed to request gpio for lcd\n");
> ? ? ? ?else
> ? ? ? ? ? ? ? ?mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
>
> (Apart from the different message this differs in semantic when the 2nd
> request fails. With my suggestion the first gpio is freed then which
> seems cleaner.) I don't care much though.

Ok, looks better. I did like you suggested in v2.

Regards,

Fabio Estevam