| Message ID | 1315945519-15178-1-git-send-email-festevam@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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
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
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
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); }
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(-)