[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 at 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 at 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