Patchwork ARM: mx28evk: Simplify GPIO requests for mx28evk_fec_reset

login
register
mail settings
Submitter Fabio Estevam
Date Nov. 9, 2011, 7:09 p.m.
Message ID <1320865742-16768-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/124661/
State New
Headers show

Comments

Fabio Estevam - Nov. 9, 2011, 7:09 p.m.
Simplify GPIO requests inside mx28evk_fec_reset by using gpio_request_array.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/mach-mxs/mach-mx28evk.c |   29 ++++++++---------------------
 1 files changed, 8 insertions(+), 21 deletions(-)
Uwe Kleine-K├Ânig - Nov. 9, 2011, 7:13 p.m.
Hello,

On Wed, Nov 09, 2011 at 05:09:02PM -0200, Fabio Estevam wrote:
> Simplify GPIO requests inside mx28evk_fec_reset by using gpio_request_array.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-mxs/mach-mx28evk.c |   29 ++++++++---------------------
>  1 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index ac2316d..c565c33 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -219,6 +219,11 @@ static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
>  	.num_leds = ARRAY_SIZE(mx28evk_leds),
>  };
>  
> +static struct gpio mx28evk_fec_gpios[] = {
const + __initconst please.

> +	{ MX28EVK_FEC_PHY_POWER, GPIOF_OUT_INIT_LOW, "fec-power" },
> +	{ MX28EVK_FEC_PHY_RESET, GPIOF_OUT_INIT_LOW, "fec-enable" },
> +};
> +
>  /* fec */
>  static void __init mx28evk_fec_reset(void)
>  {
> @@ -231,28 +236,10 @@ static void __init mx28evk_fec_reset(void)
>  		clk_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);
> +	ret = gpio_request_array(mx28evk_fec_gpios,
> +				ARRAY_SIZE(mx28evk_fec_gpios));
>  	if (ret) {
> -		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "reset", ret);
> +		pr_err("Failed to request FEC gpios: %d\n", ret);
>  		return;
>  	}
>  
> -- 
> 1.7.1
> 
> 
>
Sascha Hauer - Nov. 10, 2011, 7:53 a.m.
Hi Fabio,

On Wed, Nov 09, 2011 at 05:09:02PM -0200, Fabio Estevam wrote:
> Simplify GPIO requests inside mx28evk_fec_reset by using gpio_request_array.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/mach-mxs/mach-mx28evk.c |   29 ++++++++---------------------
>  1 files changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
> index ac2316d..c565c33 100644
> --- a/arch/arm/mach-mxs/mach-mx28evk.c
> +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> @@ -219,6 +219,11 @@ static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
>  	.num_leds = ARRAY_SIZE(mx28evk_leds),
>  };
>  
> +static struct gpio mx28evk_fec_gpios[] = {
> +	{ MX28EVK_FEC_PHY_POWER, GPIOF_OUT_INIT_LOW, "fec-power" },
> +	{ MX28EVK_FEC_PHY_RESET, GPIOF_OUT_INIT_LOW, "fec-enable" },
> +};
> +
>  /* fec */
>  static void __init mx28evk_fec_reset(void)
>  {
> @@ -231,28 +236,10 @@ static void __init mx28evk_fec_reset(void)
>  		clk_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);
> +	ret = gpio_request_array(mx28evk_fec_gpios,
> +				ARRAY_SIZE(mx28evk_fec_gpios));

I think it makes more sense to create an array per board and not per
board function. In the mx28evk file we use gpios for the fec, we have
a gpio_request_one for the flexcan switch and a gpio_request_array for
the lcd pins. All these could be added to a single mx28evk_gpios array.
Then it would be easy to see which gpios a board uses and it would be
trivial to add additional gpios. The same applies for other boards
aswell of course.

Sascha
Dong Aisheng - Nov. 10, 2011, 8:11 a.m.
> -----Original Message-----
> From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm-
> kernel-bounces@lists.infradead.org] On Behalf Of Sascha Hauer
> Sent: Thursday, November 10, 2011 3:53 PM
> To: Estevam Fabio-R49496
> Cc: Guo Shawn-R65073; kernel@pengutronix.de; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [PATCH] ARM: mx28evk: Simplify GPIO requests for
> mx28evk_fec_reset
> 
> Hi Fabio,
> 
> On Wed, Nov 09, 2011 at 05:09:02PM -0200, Fabio Estevam wrote:
> > Simplify GPIO requests inside mx28evk_fec_reset by using
> gpio_request_array.
> >
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> >  arch/arm/mach-mxs/mach-mx28evk.c |   29 ++++++++---------------------
> >  1 files changed, 8 insertions(+), 21 deletions(-)
> >
> > diff --git a/arch/arm/mach-mxs/mach-mx28evk.c
> > b/arch/arm/mach-mxs/mach-mx28evk.c
> > index ac2316d..c565c33 100644
> > --- a/arch/arm/mach-mxs/mach-mx28evk.c
> > +++ b/arch/arm/mach-mxs/mach-mx28evk.c
> > @@ -219,6 +219,11 @@ static const struct gpio_led_platform_data
> mx28evk_led_data __initconst = {
> >  	.num_leds = ARRAY_SIZE(mx28evk_leds),  };
> >
> > +static struct gpio mx28evk_fec_gpios[] = {
> > +	{ MX28EVK_FEC_PHY_POWER, GPIOF_OUT_INIT_LOW, "fec-power" },
> > +	{ MX28EVK_FEC_PHY_RESET, GPIOF_OUT_INIT_LOW, "fec-enable" }, };
> > +
> >  /* fec */
> >  static void __init mx28evk_fec_reset(void)  { @@ -231,28 +236,10 @@
> > static void __init mx28evk_fec_reset(void)
> >  		clk_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);
> > +	ret = gpio_request_array(mx28evk_fec_gpios,
> > +				ARRAY_SIZE(mx28evk_fec_gpios));
> 
> I think it makes more sense to create an array per board and not per
> board function. In the mx28evk file we use gpios for the fec, we have a
> gpio_request_one for the flexcan switch and a gpio_request_array for the
> lcd pins. All these could be added to a single mx28evk_gpios array.
> Then it would be easy to see which gpios a board uses and it would be
> trivial to add additional gpios. The same applies for other boards aswell
> of course.
> 
One question is that if one gpio, assume in one function like fec, request fail
will cause the whole gpio array request fail.
How should we handle the error for each function?

Originally we may do like:
If (!gpio_request_array(fec_gpios))
	mxs_add_fec(0);
but after change, we do not know which function gpio request fails.

> Sascha
> 
> --
> Pengutronix e.K.                           |
> |
> Industrial Linux Solutions                 | http://www.pengutronix.de/
> |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555
> |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Regards
Dong Aisheng
Sascha Hauer - Nov. 10, 2011, 8:47 a.m.
On Thu, Nov 10, 2011 at 08:11:10AM +0000, Dong Aisheng-B29396 wrote:
> > 
> > I think it makes more sense to create an array per board and not per
> > board function. In the mx28evk file we use gpios for the fec, we have a
> > gpio_request_one for the flexcan switch and a gpio_request_array for the
> > lcd pins. All these could be added to a single mx28evk_gpios array.
> > Then it would be easy to see which gpios a board uses and it would be
> > trivial to add additional gpios. The same applies for other boards aswell
> > of course.
> > 
> One question is that if one gpio, assume in one function like fec, request fail
> will cause the whole gpio array request fail.

They don't fail. If they do it means we have a duplicate entry in the
table or otherwise screwed up the kernel. I think a single pr_err("bad
things may happen, continue and hope for the best") is enough here.

Sascha

Patch

diff --git a/arch/arm/mach-mxs/mach-mx28evk.c b/arch/arm/mach-mxs/mach-mx28evk.c
index ac2316d..c565c33 100644
--- a/arch/arm/mach-mxs/mach-mx28evk.c
+++ b/arch/arm/mach-mxs/mach-mx28evk.c
@@ -219,6 +219,11 @@  static const struct gpio_led_platform_data mx28evk_led_data __initconst = {
 	.num_leds = ARRAY_SIZE(mx28evk_leds),
 };
 
+static struct gpio mx28evk_fec_gpios[] = {
+	{ MX28EVK_FEC_PHY_POWER, GPIOF_OUT_INIT_LOW, "fec-power" },
+	{ MX28EVK_FEC_PHY_RESET, GPIOF_OUT_INIT_LOW, "fec-enable" },
+};
+
 /* fec */
 static void __init mx28evk_fec_reset(void)
 {
@@ -231,28 +236,10 @@  static void __init mx28evk_fec_reset(void)
 		clk_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);
+	ret = gpio_request_array(mx28evk_fec_gpios,
+				ARRAY_SIZE(mx28evk_fec_gpios));
 	if (ret) {
-		pr_err("Failed to drive gpio fec-phy-%s: %d\n", "reset", ret);
+		pr_err("Failed to request FEC gpios: %d\n", ret);
 		return;
 	}