[U-Boot] [PATCH] sunxi: improve SATAPWR and MACPWR

[U-Boot] [PATCH] sunxi: improve SATAPWR and MACPWR

[Mylène Josserand]

This commit adds a dependency on SATA for SATAPWR because
if we do not have SATA enabled, we will not have this pin
configured.

By default, these two configs (SATAPWR and MACPWR) are equals
to "". Because of that, they are always defined so we need to
check if the variables are not empty to perform the gpio request.
Otherwise, if SATA is enabled but SATAPWR is not configured, we will have
a mdelay of 500ms for nothing (because no pin requested).

Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
---
 arch/arm/mach-sunxi/Kconfig |  1 +
 board/sunxi/board.c         | 21 +++++++++++++--------
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index b868f0e350..7d36719700 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -911,6 +911,7 @@ endchoice
 config SATAPWR
 	string "SATA power pin"
 	default ""
+	depends on SATA
 	help
 	  Set the pins used to power the SATA. This takes a string in the
 	  format understood by sunxi_name_to_gpio, e.g. PH1 for pin 1 of
diff --git a/board/sunxi/board.c b/board/sunxi/board.c
index 322dd9e23a..5b080607c1 100644
--- a/board/sunxi/board.c
+++ b/board/sunxi/board.c
@@ -230,16 +230,21 @@ int board_init(void)
 		return ret;
 
 #ifdef CONFIG_SATAPWR
-	satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
-	gpio_request(satapwr_pin, "satapwr");
-	gpio_direction_output(satapwr_pin, 1);
-	/* Give attached sata device time to power-up to avoid link timeouts */
-	mdelay(500);
+	if (strlen(CONFIG_SATAPWR)) {
+		satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
+		gpio_request(satapwr_pin, "satapwr");
+		gpio_direction_output(satapwr_pin, 1);
+		/* Give attached sata device time to power-up to avoid link timeouts */
+		mdelay(500);
+	}
 #endif
+
 #ifdef CONFIG_MACPWR
-	macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
-	gpio_request(macpwr_pin, "macpwr");
-	gpio_direction_output(macpwr_pin, 1);
+	if (strlen(CONFIG_MACPWR)) {
+		macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
+		gpio_request(macpwr_pin, "macpwr");
+		gpio_direction_output(macpwr_pin, 1);
+	}
 #endif
 
 #ifdef CONFIG_DM_I2C
-- 
2.11.0

[U-Boot] [PATCH] sunxi: improve SATAPWR and MACPWR

[Maxime Ripard]

Hi,

On Mon, Apr 09, 2018 at 12:17:26PM +0200, Mylène Josserand wrote:
> This commit adds a dependency on SATA for SATAPWR because
> if we do not have SATA enabled, we will not have this pin
> configured.
> 
> By default, these two configs (SATAPWR and MACPWR) are equals
> to "". Because of that, they are always defined so we need to
> check if the variables are not empty to perform the gpio request.
> Otherwise, if SATA is enabled but SATAPWR is not configured, we will have
> a mdelay of 500ms for nothing (because no pin requested).

You should turn this commit log the other way around. First state what
the issue is (you have a 500ms delay all the time, even when SATA is
not present on the board), then why (because SATAPWR will be defined
all the time to "", so the ifdef condition will always be evaluated to
true), then what you're doing about it (adding a depends on and
checking for strlen).

It's also not clear why you need both, and why just adding the depends
on wouldn't be enough.

> Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> ---
>  arch/arm/mach-sunxi/Kconfig |  1 +
>  board/sunxi/board.c         | 21 +++++++++++++--------
>  2 files changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index b868f0e350..7d36719700 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -911,6 +911,7 @@ endchoice
>  config SATAPWR
>  	string "SATA power pin"
>  	default ""
> +	depends on SATA
>  	help
>  	  Set the pins used to power the SATA. This takes a string in the
>  	  format understood by sunxi_name_to_gpio, e.g. PH1 for pin 1 of
> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> index 322dd9e23a..5b080607c1 100644
> --- a/board/sunxi/board.c
> +++ b/board/sunxi/board.c
> @@ -230,16 +230,21 @@ int board_init(void)
>  		return ret;
>  
>  #ifdef CONFIG_SATAPWR
> -	satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
> -	gpio_request(satapwr_pin, "satapwr");
> -	gpio_direction_output(satapwr_pin, 1);
> -	/* Give attached sata device time to power-up to avoid link timeouts */
> -	mdelay(500);
> +	if (strlen(CONFIG_SATAPWR)) {

What about if (IS_ENABLED(CONFIG_SATAPWR) && strlen(CONFIG_SATAPWR))

> +		satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
> +		gpio_request(satapwr_pin, "satapwr");
> +		gpio_direction_output(satapwr_pin, 1);
> +		/* Give attached sata device time to power-up to avoid link timeouts */
> +		mdelay(500);
> +	}
>  #endif
> +
>  #ifdef CONFIG_MACPWR
> -	macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
> -	gpio_request(macpwr_pin, "macpwr");
> -	gpio_direction_output(macpwr_pin, 1);
> +	if (strlen(CONFIG_MACPWR)) {
> +		macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
> +		gpio_request(macpwr_pin, "macpwr");
> +		gpio_direction_output(macpwr_pin, 1);
> +	}

You should split that part in a separate commit.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180409/fd90ba38/attachment.sig>

[U-Boot] [PATCH] sunxi: improve SATAPWR and MACPWR

[Mylène Josserand]

Hi,

On Mon, 9 Apr 2018 13:42:28 +0200
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> Hi,
> 
> On Mon, Apr 09, 2018 at 12:17:26PM +0200, Mylène Josserand wrote:
> > This commit adds a dependency on SATA for SATAPWR because
> > if we do not have SATA enabled, we will not have this pin
> > configured.
> > 
> > By default, these two configs (SATAPWR and MACPWR) are equals
> > to "". Because of that, they are always defined so we need to
> > check if the variables are not empty to perform the gpio request.
> > Otherwise, if SATA is enabled but SATAPWR is not configured, we will have
> > a mdelay of 500ms for nothing (because no pin requested).  
> 
> You should turn this commit log the other way around. First state what
> the issue is (you have a 500ms delay all the time, even when SATA is
> not present on the board), then why (because SATAPWR will be defined
> all the time to "", so the ifdef condition will always be evaluated to
> true), then what you're doing about it (adding a depends on and
> checking for strlen).
> 
> It's also not clear why you need both, and why just adding the depends
> on wouldn't be enough.

Okay, thank you for the review, I will try to be more precise in
my commit log.

> 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/Kconfig |  1 +
> >  board/sunxi/board.c         | 21 +++++++++++++--------
> >  2 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index b868f0e350..7d36719700 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -911,6 +911,7 @@ endchoice
> >  config SATAPWR
> >  	string "SATA power pin"
> >  	default ""
> > +	depends on SATA
> >  	help
> >  	  Set the pins used to power the SATA. This takes a string in the
> >  	  format understood by sunxi_name_to_gpio, e.g. PH1 for pin 1 of
> > diff --git a/board/sunxi/board.c b/board/sunxi/board.c
> > index 322dd9e23a..5b080607c1 100644
> > --- a/board/sunxi/board.c
> > +++ b/board/sunxi/board.c
> > @@ -230,16 +230,21 @@ int board_init(void)
> >  		return ret;
> >  
> >  #ifdef CONFIG_SATAPWR
> > -	satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
> > -	gpio_request(satapwr_pin, "satapwr");
> > -	gpio_direction_output(satapwr_pin, 1);
> > -	/* Give attached sata device time to power-up to avoid link timeouts */
> > -	mdelay(500);
> > +	if (strlen(CONFIG_SATAPWR)) {  
> 
> What about if (IS_ENABLED(CONFIG_SATAPWR) && strlen(CONFIG_SATAPWR))

You already indicated me this (privately) and I tested it.
I finally did not use it because "IS_ENABLED" seems to work only for
boolean or tristate options. When I tested it, it fails to recognize
the configuration because it is a "string", I guess.
Here is the error I got:

error: pasting "__ARG_PLACEHOLDER_" and """" does not give a valid
preprocessing token

Moreover, in case CONFIG_SATA is not enabled, it leads also to an error
on "strlen" function because CONFIG_SATAPWR is not defined anymore.

That is why I kept the use of "#ifdef CONFIG_SATAPWR".

> 
> > +		satapwr_pin = sunxi_name_to_gpio(CONFIG_SATAPWR);
> > +		gpio_request(satapwr_pin, "satapwr");
> > +		gpio_direction_output(satapwr_pin, 1);
> > +		/* Give attached sata device time to power-up to avoid link timeouts */
> > +		mdelay(500);
> > +	}
> >  #endif
> > +
> >  #ifdef CONFIG_MACPWR
> > -	macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
> > -	gpio_request(macpwr_pin, "macpwr");
> > -	gpio_direction_output(macpwr_pin, 1);
> > +	if (strlen(CONFIG_MACPWR)) {
> > +		macpwr_pin = sunxi_name_to_gpio(CONFIG_MACPWR);
> > +		gpio_request(macpwr_pin, "macpwr");
> > +		gpio_direction_output(macpwr_pin, 1);
> > +	}  
> 
> You should split that part in a separate commit.

ack.

Best regards,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com