Hi, On Mon, 9 Apr 2018 13:42:28 +0200 Maxime Ripard <[email protected]> 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 <[email protected]> > > --- > > 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 -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
