Re: [PATCH v3 2/4] sf: Tidy up code to avoid #ifdef

[Pratyush Yadav <p.yadav@ti.com>]

On 01/08/21 03:12PM, Simon Glass wrote:
> Update this code to use IS_ENABLED() instead.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> (no changes since v1)
> 
>  cmd/sf.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
> 
> diff --git a/cmd/sf.c b/cmd/sf.c
> index 46346fb9d43..d4f5ecee38c 100644
> --- a/cmd/sf.c
> +++ b/cmd/sf.c
> @@ -384,7 +384,6 @@ static int do_spi_protect(int argc, char *const argv[])
>  	return ret == 0 ? 0 : 1;
>  }
>  
> -#ifdef CONFIG_CMD_SF_TEST
>  enum {
>  	STAGE_ERASE,
>  	STAGE_CHECK,
> @@ -394,7 +393,7 @@ enum {
>  	STAGE_COUNT,
>  };
>  
> -static char *stage_name[STAGE_COUNT] = {
> +static const char *stage_name[STAGE_COUNT] = {

Unrelated change. Dunno if it is worth its own separate patch though. Up 
to you.

>  	"erase",
>  	"check",
>  	"write",
> @@ -548,7 +547,6 @@ static int do_spi_flash_test(int argc, char *const argv[])
>  
>  	return 0;
>  }
> -#endif /* CONFIG_CMD_SF_TEST */
>  
>  static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
>  			char *const argv[])
> @@ -582,10 +580,8 @@ static int do_spi_flash(struct cmd_tbl *cmdtp, int flag, int argc,
>  		ret = do_spi_flash_erase(argc, argv);
>  	else if (strcmp(cmd, "protect") == 0)
>  		ret = do_spi_protect(argc, argv);
> -#ifdef CONFIG_CMD_SF_TEST
> -	else if (!strcmp(cmd, "test"))
> +	else if (IS_ENABLED(CONFIG_CMD_SF_TEST) && !strcmp(cmd, "test"))

Ok.

>  		ret = do_spi_flash_test(argc, argv);
> -#endif
>  	else
>  		ret = -1;
>  
> @@ -597,16 +593,8 @@ usage:
>  	return CMD_RET_USAGE;
>  }
>  
> -#ifdef CONFIG_CMD_SF_TEST
> -#define SF_TEST_HELP "\nsf test offset len		" \
> -		"- run a very basic destructive test"
> -#else
> -#define SF_TEST_HELP
> -#endif
> -
> -U_BOOT_CMD(
> -	sf,	5,	1,	do_spi_flash,
> -	"SPI flash sub-system",
> +#ifdef CONFIG_SYS_LONGHELP
> +static const char long_help[] =
>  	"probe [[bus:]cs] [hz] [mode]	- init flash device on given SPI bus\n"
>  	"				  and chip select\n"
>  	"sf read addr offset|partition len	- read `len' bytes starting at\n"
> @@ -622,6 +610,14 @@ U_BOOT_CMD(
>  	"					  at `addr' to flash at `offset'\n"
>  	"					  or to start of mtd `partition'\n"
>  	"sf protect lock/unlock sector len	- protect/unprotect 'len' bytes starting\n"
> -	"					  at address 'sector'\n"
> -	SF_TEST_HELP
> +	"					  at address 'sector'"
> +#ifdef CONFIG_CMD_SF_TEST
> +	"\nsf test offset len		- run a very basic destructive test"
> +#endif

Dunno if this is better or worse than before. Should be fine either way 
I guess

Other than the comment above,

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

> +#endif /* CONFIG_SYS_LONGHELP */
> +	;
> +
> +U_BOOT_CMD(
> +	sf,	5,	1,	do_spi_flash,
> +	"SPI flash sub-system", long_help
>  );
> -- 
> 2.32.0.554.ge1b32706d8-goog
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.