Patchwork ARM: mx28evk: Simplify GPIO requests

login
register
mail settings
Submitter Fabio Estevam
Date Jan. 23, 2012, 2:41 p.m.
Message ID <1327329709-25545-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/137366/
State New
Headers show

Comments

Fabio Estevam - Jan. 23, 2012, 2:41 p.m.
Requesting all the GPIOs on a single array (mx28evk_gpios[]) can make the 
error handling of gpio_requests much simpler.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
This approach was suggested by Sascha Hauer:
http://patchwork.ozlabs.org/patch/124661/

 arch/arm/mach-mxs/mach-mx28evk.c |   72 ++++++++-----------------------------
 1 files changed, 16 insertions(+), 56 deletions(-)
Shawn Guo - Jan. 26, 2012, 12:19 p.m.
On Mon, Jan 23, 2012 at 12:41:49PM -0200, Fabio Estevam wrote:
> Requesting all the GPIOs on a single array (mx28evk_gpios[]) can make the 
> error handling of gpio_requests much simpler.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

Looks good.  A couple trivial comments below though.

> ---
> This approach was suggested by Sascha Hauer:
> http://patchwork.ozlabs.org/patch/124661/
> 
>  arch/arm/mach-mxs/mach-mx28evk.c |   72 ++++++++-----------------------------
>  1 files changed, 16 insertions(+), 56 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index fdb0a56..6260ec3 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -223,7 +223,6 @@ static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
>  /* fec */
>  static void __init mx28evk_fec_reset(void)
>  {
> -	int ret;
>  	struct clk *clk;
>  
>  	/* Enable fec phy clock */
> @@ -231,32 +230,6 @@ static void __init mx28evk_fec_reset(void)
>  	if (!IS_ERR(clk))
>  		clk_prepare_enable(clk);
>  
> -	/* Power up fec phy */
> -	ret = gpio_request(MX28EVK_FEC_PHY_POWER, "fec-phy-power");
> -	if (ret) {
> -		pr_err("Failed to request gpio fec-phy-%s: %d\n", "power", ret);
> -		return;
> -	}
> -
> -	ret = gpio_direction_output(MX28EVK_FEC_PHY_POWER, 0);
> -	if (ret) {
> -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "power", ret);
> -		return;
> -	}
> -
> -	/* Reset fec phy */
> -	ret = gpio_request(MX28EVK_FEC_PHY_RESET, "fec-phy-reset");
> -	if (ret) {
> -		pr_err("Failed to request gpio fec-phy-%s: %d\n", "reset", ret);
> -		return;
> -	}
> -
> -	gpio_direction_output(MX28EVK_FEC_PHY_RESET, 0);
> -	if (ret) {
> -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "reset", ret);
> -		return;
> -	}
> -
>  	mdelay(1);

This delay is supposed to be in the middle of the two calls here.  As
we are moving around the first call, can you give it a test to see if
the mdelay is still mandatory?  I would remove it if it's not necessary.

>  	gpio_set_value(MX28EVK_FEC_PHY_RESET, 1);
>  }
> @@ -417,9 +390,14 @@ static void __init mx28evk_add_regulators(void)
>  static void __init mx28evk_add_regulators(void) {}
>  #endif
>  
> -static struct gpio mx28evk_lcd_gpios[] = {
> +static const __initconst struct gpio mx28evk_gpios[] = {

Put __initconst after mx28evk_gpios[], please.

>  	{ MX28EVK_LCD_ENABLE, GPIOF_OUT_INIT_HIGH, "lcd-enable" },
>  	{ MX28EVK_BL_ENABLE, GPIOF_OUT_INIT_HIGH, "bl-enable" },
> +	{ MX28EVK_FLEXCAN_SWITCH, GPIOF_DIR_OUT, "flexcan-switch" },
> +	{ MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW, "mmc0-slot-power" },
> +	{ MX28EVK_MMC1_SLOT_POWER, GPIOF_OUT_INIT_LOW, "mmc1-slot-power" },
> +	{ MX28EVK_FEC_PHY_POWER, GPIOF_OUT_INIT_LOW, "fec-phy-power" },
> +	{ MX28EVK_FEC_PHY_RESET, GPIOF_OUT_INIT_LOW, "fec-phy-reset" },
>  };
>  
>  static const struct mxs_saif_platform_data
> @@ -447,25 +425,19 @@ static void __init mx28evk_init(void)
>  	if (mx28evk_fec_get_mac())
>  		pr_warn("%s: failed on fec mac setup\n", __func__);
>  
> +	ret = gpio_request_array(mx28evk_gpios, ARRAY_SIZE(mx28evk_gpios));
> +
Unnecessary blank line.

Regards,
Shawn

> +	if (ret)
> +		pr_err("One or more GPIOs failed to be requested: %d\n", ret);
> +
>  	mx28evk_fec_reset();
>  	mx28_add_fec(0, &mx28_fec_pdata[0]);
>  	mx28_add_fec(1, &mx28_fec_pdata[1]);
>  
> -	ret = gpio_request_one(MX28EVK_FLEXCAN_SWITCH, GPIOF_DIR_OUT,
> -				"flexcan-switch");
> -	if (ret) {
> -		pr_err("failed to request gpio flexcan-switch: %d\n", ret);
> -	} else {
> -		mx28_add_flexcan(0, &mx28evk_flexcan_pdata[0]);
> -		mx28_add_flexcan(1, &mx28evk_flexcan_pdata[1]);
> -	}
> +	mx28_add_flexcan(0, &mx28evk_flexcan_pdata[0]);
> +	mx28_add_flexcan(1, &mx28evk_flexcan_pdata[1]);
>  
> -	ret = gpio_request_array(mx28evk_lcd_gpios,
> -				 ARRAY_SIZE(mx28evk_lcd_gpios));
> -	if (ret)
> -		pr_warn("failed to request gpio pins for lcd: %d\n", ret);
> -	else
> -		mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
> +	mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
>  
>  	mxs_saif_clkmux_select(MXS_DIGCTL_SAIF_CLKMUX_EXTMSTR0);
>  	mx28_add_saif(0, &mx28evk_mxs_saif_pdata[0]);
> @@ -480,20 +452,8 @@ static void __init mx28evk_init(void)
>  	mxs_add_platform_device("mxs-sgtl5000", 0, NULL, 0,
>  			NULL, 0);
>  
> -	/* power on mmc slot by writing 0 to the gpio */
> -	ret = gpio_request_one(MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW,
> -			       "mmc0-slot-power");
> -	if (ret)
> -		pr_warn("failed to request gpio mmc0-slot-power: %d\n", ret);
> -	else
> -		mx28_add_mxs_mmc(0, &mx28evk_mmc_pdata[0]);
> -
> -	ret = gpio_request_one(MX28EVK_MMC1_SLOT_POWER, GPIOF_OUT_INIT_LOW,
> -			       "mmc1-slot-power");
> -	if (ret)
> -		pr_warn("failed to request gpio mmc1-slot-power: %d\n", ret);
> -	else
> -		mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
> +	mx28_add_mxs_mmc(0, &mx28evk_mmc_pdata[0]);
> +	mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
>  
>  	mx28_add_rtc_stmp3xxx();
>  
> -- 
> 1.7.1
Lothar Waßmann - Jan. 26, 2012, 12:49 p.m.
Hi,

Shawn Guo writes:
> On Mon, Jan 23, 2012 at 12:41:49PM -0200, Fabio Estevam wrote:
> > Requesting all the GPIOs on a single array (mx28evk_gpios[]) can make the 
> > error handling of gpio_requests much simpler.
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Looks good.  A couple trivial comments below though.
> 
> > ---
> > This approach was suggested by Sascha Hauer:
> > http://patchwork.ozlabs.org/patch/124661/
> > 
> >  arch/arm/mach-mxs/mach-mx28evk.c |   72 ++++++++-----------------------------
> >  1 files changed, 16 insertions(+), 56 deletions(-)
> > 
> > diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> > index fdb0a56..6260ec3 100644
> > --- a/arch/arm/mach-mxs/mach-mx28evk.c
> > +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> > @@ -223,7 +223,6 @@ static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
> >  /* fec */
> >  static void __init mx28evk_fec_reset(void)
> >  {
> > -	int ret;
> >  	struct clk *clk;
> >  
> >  	/* Enable fec phy clock */
> > @@ -231,32 +230,6 @@ static void __init mx28evk_fec_reset(void)
> >  	if (!IS_ERR(clk))
> >  		clk_prepare_enable(clk);
> >  
> > -	/* Power up fec phy */
> > -	ret = gpio_request(MX28EVK_FEC_PHY_POWER, "fec-phy-power");
> > -	if (ret) {
> > -		pr_err("Failed to request gpio fec-phy-%s: %d\n", "power", ret);
> > -		return;
> > -	}
> > -
> > -	ret = gpio_direction_output(MX28EVK_FEC_PHY_POWER, 0);
> > -	if (ret) {
> > -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "power", ret);
> > -		return;
> > -	}
> > -
> > -	/* Reset fec phy */
> > -	ret = gpio_request(MX28EVK_FEC_PHY_RESET, "fec-phy-reset");
> > -	if (ret) {
> > -		pr_err("Failed to request gpio fec-phy-%s: %d\n", "reset", ret);
> > -		return;
> > -	}
> > -
> > -	gpio_direction_output(MX28EVK_FEC_PHY_RESET, 0);
> > -	if (ret) {
> > -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "reset", ret);
> > -		return;
> > -	}
> > -
> >  	mdelay(1);
> 
> This delay is supposed to be in the middle of the two calls here.  As
> we are moving around the first call, can you give it a test to see if
> the mdelay is still mandatory?  I would remove it if it's not necessary.
> 
"Empirical Programming Part One"? The HW will have some timing
requirements that need to be fullfilled. That cannot be done by
'giving it a test' but by reading the appropriate datasheet and
implementing the code accordingly.


Lothar Waßmann
Shawn Guo - Jan. 26, 2012, 2:17 p.m.
On Thu, Jan 26, 2012 at 01:49:52PM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Mon, Jan 23, 2012 at 12:41:49PM -0200, Fabio Estevam wrote:
> > > Requesting all the GPIOs on a single array (mx28evk_gpios[]) can make the 
> > > error handling of gpio_requests much simpler.
> > > 
> > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > Looks good.  A couple trivial comments below though.
> > 
> > > ---
> > > This approach was suggested by Sascha Hauer:
> > > http://patchwork.ozlabs.org/patch/124661/
> > > 
> > >  arch/arm/mach-mxs/mach-mx28evk.c |   72 ++++++++-----------------------------
> > >  1 files changed, 16 insertions(+), 56 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> > > index fdb0a56..6260ec3 100644
> > > --- a/arch/arm/mach-mxs/mach-mx28evk.c
> > > +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> > > @@ -223,7 +223,6 @@ static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
> > >  /* fec */
> > >  static void __init mx28evk_fec_reset(void)
> > >  {
> > > -	int ret;
> > >  	struct clk *clk;
> > >  
> > >  	/* Enable fec phy clock */
> > > @@ -231,32 +230,6 @@ static void __init mx28evk_fec_reset(void)
> > >  	if (!IS_ERR(clk))
> > >  		clk_prepare_enable(clk);
> > >  
> > > -	/* Power up fec phy */
> > > -	ret = gpio_request(MX28EVK_FEC_PHY_POWER, "fec-phy-power");
> > > -	if (ret) {
> > > -		pr_err("Failed to request gpio fec-phy-%s: %d\n", "power", ret);
> > > -		return;
> > > -	}
> > > -
> > > -	ret = gpio_direction_output(MX28EVK_FEC_PHY_POWER, 0);
> > > -	if (ret) {
> > > -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "power", ret);
> > > -		return;
> > > -	}
> > > -
> > > -	/* Reset fec phy */
> > > -	ret = gpio_request(MX28EVK_FEC_PHY_RESET, "fec-phy-reset");
> > > -	if (ret) {
> > > -		pr_err("Failed to request gpio fec-phy-%s: %d\n", "reset", ret);
> > > -		return;
> > > -	}
> > > -
> > > -	gpio_direction_output(MX28EVK_FEC_PHY_RESET, 0);
> > > -	if (ret) {
> > > -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "reset", ret);
> > > -		return;
> > > -	}
> > > -
> > >  	mdelay(1);
> > 
> > This delay is supposed to be in the middle of the two calls here.  As
> > we are moving around the first call, can you give it a test to see if
> > the mdelay is still mandatory?  I would remove it if it's not necessary.
> > 
> "Empirical Programming Part One"? The HW will have some timing
> requirements that need to be fullfilled. That cannot be done by
> 'giving it a test' but by reading the appropriate datasheet and
> implementing the code accordingly.
> 
I was thinking that the period of code execution between
gpio_request_array() and this point may have fulfilled the timing
requirement of LAN8720 reset (100us minimum).  But you are right,
this mdelay should not be removed simply by a testing.

But the way that the patch moves thing around leaves this mdelay
a little unreadable to me.  I would like see an explicit call of
gpio_set_value(MX28EVK_FEC_PHY_RESET, 0) before the mdelay.

Patch

diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index fdb0a56..6260ec3 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -223,7 +223,6 @@  static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
 /* fec */
 static void __init mx28evk_fec_reset(void)
 {
-	int ret;
 	struct clk *clk;
 
 	/* Enable fec phy clock */
@@ -231,32 +230,6 @@  static void __init mx28evk_fec_reset(void)
 	if (!IS_ERR(clk))
 		clk_prepare_enable(clk);
 
-	/* Power up fec phy */
-	ret = gpio_request(MX28EVK_FEC_PHY_POWER, "fec-phy-power");
-	if (ret) {
-		pr_err("Failed to request gpio fec-phy-%s: %d\n", "power", ret);
-		return;
-	}
-
-	ret = gpio_direction_output(MX28EVK_FEC_PHY_POWER, 0);
-	if (ret) {
-		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "power", ret);
-		return;
-	}
-
-	/* Reset fec phy */
-	ret = gpio_request(MX28EVK_FEC_PHY_RESET, "fec-phy-reset");
-	if (ret) {
-		pr_err("Failed to request gpio fec-phy-%s: %d\n", "reset", ret);
-		return;
-	}
-
-	gpio_direction_output(MX28EVK_FEC_PHY_RESET, 0);
-	if (ret) {
-		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "reset", ret);
-		return;
-	}
-
 	mdelay(1);
 	gpio_set_value(MX28EVK_FEC_PHY_RESET, 1);
 }
@@ -417,9 +390,14 @@  static void __init mx28evk_add_regulators(void)
 static void __init mx28evk_add_regulators(void) {}
 #endif
 
-static struct gpio mx28evk_lcd_gpios[] = {
+static const __initconst struct gpio mx28evk_gpios[] = {
 	{ MX28EVK_LCD_ENABLE, GPIOF_OUT_INIT_HIGH, "lcd-enable" },
 	{ MX28EVK_BL_ENABLE, GPIOF_OUT_INIT_HIGH, "bl-enable" },
+	{ MX28EVK_FLEXCAN_SWITCH, GPIOF_DIR_OUT, "flexcan-switch" },
+	{ MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW, "mmc0-slot-power" },
+	{ MX28EVK_MMC1_SLOT_POWER, GPIOF_OUT_INIT_LOW, "mmc1-slot-power" },
+	{ MX28EVK_FEC_PHY_POWER, GPIOF_OUT_INIT_LOW, "fec-phy-power" },
+	{ MX28EVK_FEC_PHY_RESET, GPIOF_OUT_INIT_LOW, "fec-phy-reset" },
 };
 
 static const struct mxs_saif_platform_data
@@ -447,25 +425,19 @@  static void __init mx28evk_init(void)
 	if (mx28evk_fec_get_mac())
 		pr_warn("%s: failed on fec mac setup\n", __func__);
 
+	ret = gpio_request_array(mx28evk_gpios, ARRAY_SIZE(mx28evk_gpios));
+
+	if (ret)
+		pr_err("One or more GPIOs failed to be requested: %d\n", ret);
+
 	mx28evk_fec_reset();
 	mx28_add_fec(0, &mx28_fec_pdata[0]);
 	mx28_add_fec(1, &mx28_fec_pdata[1]);
 
-	ret = gpio_request_one(MX28EVK_FLEXCAN_SWITCH, GPIOF_DIR_OUT,
-				"flexcan-switch");
-	if (ret) {
-		pr_err("failed to request gpio flexcan-switch: %d\n", ret);
-	} else {
-		mx28_add_flexcan(0, &mx28evk_flexcan_pdata[0]);
-		mx28_add_flexcan(1, &mx28evk_flexcan_pdata[1]);
-	}
+	mx28_add_flexcan(0, &mx28evk_flexcan_pdata[0]);
+	mx28_add_flexcan(1, &mx28evk_flexcan_pdata[1]);
 
-	ret = gpio_request_array(mx28evk_lcd_gpios,
-				 ARRAY_SIZE(mx28evk_lcd_gpios));
-	if (ret)
-		pr_warn("failed to request gpio pins for lcd: %d\n", ret);
-	else
-		mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
+	mx28_add_mxsfb(&mx28evk_mxsfb_pdata);
 
 	mxs_saif_clkmux_select(MXS_DIGCTL_SAIF_CLKMUX_EXTMSTR0);
 	mx28_add_saif(0, &mx28evk_mxs_saif_pdata[0]);
@@ -480,20 +452,8 @@  static void __init mx28evk_init(void)
 	mxs_add_platform_device("mxs-sgtl5000", 0, NULL, 0,
 			NULL, 0);
 
-	/* power on mmc slot by writing 0 to the gpio */
-	ret = gpio_request_one(MX28EVK_MMC0_SLOT_POWER, GPIOF_OUT_INIT_LOW,
-			       "mmc0-slot-power");
-	if (ret)
-		pr_warn("failed to request gpio mmc0-slot-power: %d\n", ret);
-	else
-		mx28_add_mxs_mmc(0, &mx28evk_mmc_pdata[0]);
-
-	ret = gpio_request_one(MX28EVK_MMC1_SLOT_POWER, GPIOF_OUT_INIT_LOW,
-			       "mmc1-slot-power");
-	if (ret)
-		pr_warn("failed to request gpio mmc1-slot-power: %d\n", ret);
-	else
-		mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
+	mx28_add_mxs_mmc(0, &mx28evk_mmc_pdata[0]);
+	mx28_add_mxs_mmc(1, &mx28evk_mmc_pdata[1]);
 
 	mx28_add_rtc_stmp3xxx();