Message ID | 20210406043011.1780229-3-sjg@chromium.org |
---|---|
State | Changes Requested |
Delegated to: | Jagannadha Sutradharudu Teki |
Headers | show |
Series | sf: Add documentation and an 'sf mmap' command | expand |
On 06/04/21 04:30PM, Simon Glass wrote: > Update this code to use IS_ENABLED() instead. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > 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] = { > "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 */ Won't this cause all the test related functions to always be compiled into the binary? Then what's the point of having it behind a config at all? > > 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")) > 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 > +#endif /* CONFIG_SYS_LONGHELP */ > + ; > + > +U_BOOT_CMD( > + sf, 5, 1, do_spi_flash, > + "SPI flash sub-system", long_help > ); > -- > 2.31.0.208.g409f899ff0-goog >
On 06.04.21 12:22, Pratyush Yadav wrote: > On 06/04/21 04:30PM, Simon Glass wrote: >> Update this code to use IS_ENABLED() instead. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> 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] = { >> "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 */ > > Won't this cause all the test related functions to always be compiled > into the binary? Then what's the point of having it behind a config at > all? We compile all functions into separate sections. The linker eliminates unused functions. Look for -ffunction-sections in files arch/<arch>/config.mk. Best regards Heinrich > >> >> 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")) >> 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 >> +#endif /* CONFIG_SYS_LONGHELP */ >> + ; >> + >> +U_BOOT_CMD( >> + sf, 5, 1, do_spi_flash, >> + "SPI flash sub-system", long_help >> ); >> -- >> 2.31.0.208.g409f899ff0-goog >> >
On 06/04/21 01:32PM, Heinrich Schuchardt wrote: > On 06.04.21 12:22, Pratyush Yadav wrote: > > On 06/04/21 04:30PM, Simon Glass wrote: > >> Update this code to use IS_ENABLED() instead. > >> > >> Signed-off-by: Simon Glass <sjg@chromium.org> > >> --- > >> > >> 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] = { > >> "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 */ > > > > Won't this cause all the test related functions to always be compiled > > into the binary? Then what's the point of having it behind a config at > > all? > > We compile all functions into separate sections. The linker eliminates > unused functions. Look for -ffunction-sections in files > arch/<arch>/config.mk. Ah, clever! This would rely on the compiler eliminating the dead if branch but the build uses -O2 or -Os, both of which should eliminate dead code. > > Best regards > > Heinrich > > > > >> > >> 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")) > >> 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 > >> +#endif /* CONFIG_SYS_LONGHELP */ > >> + ; > >> + > >> +U_BOOT_CMD( > >> + sf, 5, 1, do_spi_flash, > >> + "SPI flash sub-system", long_help > >> ); > >> -- > >> 2.31.0.208.g409f899ff0-goog > >> > > >
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] = { "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")) 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 +#endif /* CONFIG_SYS_LONGHELP */ + ; + +U_BOOT_CMD( + sf, 5, 1, do_spi_flash, + "SPI flash sub-system", long_help );
Update this code to use IS_ENABLED() instead. Signed-off-by: Simon Glass <sjg@chromium.org> --- cmd/sf.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)