Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On Thu, Dec 15, 2022 at 10:15:50AM +0100, Patrick Delaunay wrote: > Much of the fastboot code predates the introduction of Kconfig and > has quite a few #ifdefs in it which is unnecessary now that we can use > IS_ENABLED() et al. > > Signed-off-by: Patrick Delaunay > Reviewed-by: Mattijs Korpershoek > Reviewed-by: Sean Anderson > Tested-by: Mattijs Korpershoek # on vim3l Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On Thu, Jan 05, 2023 at 12:37:58PM +0100, Patrick DELAUNAY wrote:
> Hi Tom,
>
> On 1/3/23 21:35, Tom Rini wrote:
> > On Thu, Dec 15, 2022 at 10:15:50AM +0100, Patrick Delaunay wrote:
> > > Much of the fastboot code predates the introduction of Kconfig and
> > > has quite a few #ifdefs in it which is unnecessary now that we can use
> > > IS_ENABLED() et al.
> > >
> > > Signed-off-by: Patrick Delaunay
> > > ---
> > >
> > > cmd/fastboot.c | 35 +--
> > > drivers/fastboot/fb_command.c | 104
> > > drivers/fastboot/fb_common.c| 11 ++--
> > > drivers/fastboot/fb_getvar.c| 49 ++-
> > > drivers/usb/gadget/f_fastboot.c | 7 +--
> > > include/fastboot.h | 13
> > > net/fastboot.c | 8 +--
> > > 7 files changed, 82 insertions(+), 145 deletions(-)
> > >
> > > diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> > > index b498e4b22bb3..b94dbd548843 100644
> > > --- a/cmd/fastboot.c
> > > +++ b/cmd/fastboot.c
> > > @@ -19,8 +19,14 @@
> > > static int do_fastboot_udp(int argc, char *const argv[],
> > > uintptr_t buf_addr, size_t buf_size)
> > > {
> > > -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
> > > - int err = net_loop(FASTBOOT);
> > > + int err;
> > > +
> > > + if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
> > > + pr_err("Fastboot UDP not enabled\n");
> > > + return CMD_RET_FAILURE;
> > > + }
> > > +
> > > + err = net_loop(FASTBOOT);
> > > if (err < 0) {
> > > printf("fastboot udp error: %d\n", err);
> > > @@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const
> > > argv[],
> > > }
> > > return CMD_RET_SUCCESS;
> > > -#else
> > > - pr_err("Fastboot UDP not enabled\n");
> > > - return CMD_RET_FAILURE;
> > > -#endif
> > > }
> > This probably needs to become an if (CONFIG_IS_ENABLED(...)) { ... }
> > else { ... } in order to remain size-neutral.
>
>
> Are you sure ?
>
>
> {
> if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
>
> return CMD_RET_FAILURE;
> }
>
>
>
> return CMD_RET_SUCCESS;
> }
>
>
> For me, it is exactly the same size after compiler/linker than :
>
>
> {
> if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
>
> return CMD_RET_FAILURE;
> } else {
>
> return CMD_RET_SUCCESS;
>
> }
>
> }
>
>
> if UDP_FUNCTION_FASTBOOTis activated or not
>
> or I forget something during the Christmas break.
If you've size-tested and it's the same, OK. I'm just worried about
strings not being discarded since that's sometimes an unexpected side
effect / bug.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
Hi Tom,
On 1/3/23 21:35, Tom Rini wrote:
On Thu, Dec 15, 2022 at 10:15:50AM +0100, Patrick Delaunay wrote:
Much of the fastboot code predates the introduction of Kconfig and
has quite a few #ifdefs in it which is unnecessary now that we can use
IS_ENABLED() et al.
Signed-off-by: Patrick Delaunay
---
cmd/fastboot.c | 35 +--
drivers/fastboot/fb_command.c | 104
drivers/fastboot/fb_common.c| 11 ++--
drivers/fastboot/fb_getvar.c| 49 ++-
drivers/usb/gadget/f_fastboot.c | 7 +--
include/fastboot.h | 13
net/fastboot.c | 8 +--
7 files changed, 82 insertions(+), 145 deletions(-)
diff --git a/cmd/fastboot.c b/cmd/fastboot.c
index b498e4b22bb3..b94dbd548843 100644
--- a/cmd/fastboot.c
+++ b/cmd/fastboot.c
@@ -19,8 +19,14 @@
static int do_fastboot_udp(int argc, char *const argv[],
uintptr_t buf_addr, size_t buf_size)
{
-#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
- int err = net_loop(FASTBOOT);
+ int err;
+
+ if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
+ pr_err("Fastboot UDP not enabled\n");
+ return CMD_RET_FAILURE;
+ }
+
+ err = net_loop(FASTBOOT);
if (err < 0) {
printf("fastboot udp error: %d\n", err);
@@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
}
return CMD_RET_SUCCESS;
-#else
- pr_err("Fastboot UDP not enabled\n");
- return CMD_RET_FAILURE;
-#endif
}
This probably needs to become an if (CONFIG_IS_ENABLED(...)) { ... }
else { ... } in order to remain size-neutral.
Are you sure ?
{
if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
return CMD_RET_FAILURE;
}
return CMD_RET_SUCCESS;
}
For me, it is exactly the same size after compiler/linker than :
{
if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
return CMD_RET_FAILURE;
} else {
return CMD_RET_SUCCESS;
}
}
if UDP_FUNCTION_FASTBOOTis activated or not
or I forget something during the Christmas break.
Patrick
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On Thu, Dec 15, 2022 at 10:15:50AM +0100, Patrick Delaunay wrote:
> Much of the fastboot code predates the introduction of Kconfig and
> has quite a few #ifdefs in it which is unnecessary now that we can use
> IS_ENABLED() et al.
>
> Signed-off-by: Patrick Delaunay
> ---
>
> cmd/fastboot.c | 35 +--
> drivers/fastboot/fb_command.c | 104
> drivers/fastboot/fb_common.c| 11 ++--
> drivers/fastboot/fb_getvar.c| 49 ++-
> drivers/usb/gadget/f_fastboot.c | 7 +--
> include/fastboot.h | 13
> net/fastboot.c | 8 +--
> 7 files changed, 82 insertions(+), 145 deletions(-)
>
> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> index b498e4b22bb3..b94dbd548843 100644
> --- a/cmd/fastboot.c
> +++ b/cmd/fastboot.c
> @@ -19,8 +19,14 @@
> static int do_fastboot_udp(int argc, char *const argv[],
> uintptr_t buf_addr, size_t buf_size)
> {
> -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
> - int err = net_loop(FASTBOOT);
> + int err;
> +
> + if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
> + pr_err("Fastboot UDP not enabled\n");
> + return CMD_RET_FAILURE;
> + }
> +
> + err = net_loop(FASTBOOT);
>
> if (err < 0) {
> printf("fastboot udp error: %d\n", err);
> @@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
> }
>
> return CMD_RET_SUCCESS;
> -#else
> - pr_err("Fastboot UDP not enabled\n");
> - return CMD_RET_FAILURE;
> -#endif
> }
This probably needs to become an if (CONFIG_IS_ENABLED(...)) { ... }
else { ... } in order to remain size-neutral.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
Hi,
On 12/15/22 16:40, Sean Anderson wrote:
On 12/15/22 04:15, Patrick Delaunay wrote:
Much of the fastboot code predates the introduction of Kconfig and
has quite a few #ifdefs in it which is unnecessary now that we can use
IS_ENABLED() et al.
Signed-off-by: Patrick Delaunay
---
cmd/fastboot.c | 35 +--
drivers/fastboot/fb_command.c | 104
drivers/fastboot/fb_common.c | 11 ++--
drivers/fastboot/fb_getvar.c | 49 ++-
drivers/usb/gadget/f_fastboot.c | 7 +--
include/fastboot.h | 13
net/fastboot.c | 8 +--
7 files changed, 82 insertions(+), 145 deletions(-)
diff --git a/cmd/fastboot.c b/cmd/fastboot.c
index b498e4b22bb3..b94dbd548843 100644
--- a/cmd/fastboot.c
+++ b/cmd/fastboot.c
@@ -19,8 +19,14 @@
static int do_fastboot_udp(int argc, char *const argv[],
uintptr_t buf_addr, size_t buf_size)
{
-#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
- int err = net_loop(FASTBOOT);
+ int err;
+
+ if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
+ pr_err("Fastboot UDP not enabled\n");
+ return CMD_RET_FAILURE;
+ }
+
+ err = net_loop(FASTBOOT);
if (err < 0) {
printf("fastboot udp error: %d\n", err);
@@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const
argv[],
}
return CMD_RET_SUCCESS;
-#else
- pr_err("Fastboot UDP not enabled\n");
- return CMD_RET_FAILURE;
-#endif
}
static int do_fastboot_usb(int argc, char *const argv[],
uintptr_t buf_addr, size_t buf_size)
{
-#if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)
int controller_index;
char *usb_controller;
char *endp;
int ret;
+ if (!CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)) {
+ pr_err("Fastboot USB not enabled\n");
+ return CMD_RET_FAILURE;
+ }
+
if (argc < 2)
return CMD_RET_USAGE;
@@ -88,10 +94,6 @@ exit:
g_dnl_clear_detach();
return ret;
-#else
- pr_err("Fastboot USB not enabled\n");
- return CMD_RET_FAILURE;
-#endif
}
static int do_fastboot(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -148,17 +150,12 @@ NXTARG:
return do_fastboot_usb(argc, argv, buf_addr, buf_size);
}
-#ifdef CONFIG_SYS_LONGHELP
-static char fastboot_help_text[] =
+U_BOOT_CMD(
+ fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
+ "run as a fastboot usb or udp device",
"[-l addr] [-s size] usb | udp\n"
"\taddr - address of buffer used during data transfers ("
__stringify(CONFIG_FASTBOOT_BUF_ADDR) ")\n"
"\tsize - size of buffer used during data transfers ("
__stringify(CONFIG_FASTBOOT_BUF_SIZE) ")"
- ;
-#endif
-
-U_BOOT_CMD(
- fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
- "run as a fastboot usb or udp device", fastboot_help_text
);
diff --git a/drivers/fastboot/fb_command.c
b/drivers/fastboot/fb_command.c
index bdfdf262c8a3..f0fd605854da 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -31,27 +31,16 @@ static u32 fastboot_bytes_expected;
static void okay(char *, char *);
static void getvar(char *, char *);
static void download(char *, char *);
-#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
static void flash(char *, char *);
static void erase(char *, char *);
-#endif
static void reboot_bootloader(char *, char *);
static void reboot_fastbootd(char *, char *);
static void reboot_recovery(char *, char *);
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
static void oem_format(char *, char *);
-#endif
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_PARTCONF)
static void oem_partconf(char *, char *);
-#endif
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_BOOTBUS)
static void oem_bootbus(char *, char *);
-#endif
-
-#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT)
static void run_ucmd(char *, char *);
static void run_acmd(char *, char *);
-#endif
static const struct {
const char *command;
@@ -65,16 +54,14 @@ static const struct {
.command = "download",
.dispatch = download
},
-#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
[FASTBOOT_COMMAND_FLASH] = {
.command = "flash",
- .dispatch = flash
+ .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (flash), (NULL))
},
[FASTBOOT_COMMAND_ERASE] = {
.command = "erase",
- .dispatch = erase
+ .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (erase), (NULL))
},
-#endif
[FASTBOOT_COMMAND_BOOT] = {
.command = "boot",
.dispatch = okay
@@ -103,34 +90,26 @@ static const struct {
.command = "set_active",
.dispatch = okay
},
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
[FASTBOOT_COMMAND_OEM_FORMAT] = {
.command = "oem format",
- .dispatch = oem_format,
+ .dispatch = CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT,
(oem_format), (NULL))
},
-#endif
-#if CONFIG_IS_ENABLE
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On Thu, Dec 15, 2022 at 14:45, Mattijs Korpershoek
wrote:
> On Thu, Dec 15, 2022 at 10:15, Patrick Delaunay
> wrote:
>
>> Much of the fastboot code predates the introduction of Kconfig and
>> has quite a few #ifdefs in it which is unnecessary now that we can use
>> IS_ENABLED() et al.
>>
>> Signed-off-by: Patrick Delaunay
>
> Hi Patrick,
>
> Thank you for this, it's a nice improvement!
>
> I will test this on vim3l tomorrow, but I've already reviewed it:
>
> Reviewed-by: Mattijs Korpershoek
Did a bootloader reflash to MMC following commands from [1]
Tested-by: Mattijs Korpershoek # on vim3l
[1] https://source.android.com/docs/setup/create/devices#vim3-fastboot
>
>> ---
>>
>> cmd/fastboot.c | 35 +--
>> drivers/fastboot/fb_command.c | 104
>> drivers/fastboot/fb_common.c| 11 ++--
>> drivers/fastboot/fb_getvar.c| 49 ++-
>> drivers/usb/gadget/f_fastboot.c | 7 +--
>> include/fastboot.h | 13
>> net/fastboot.c | 8 +--
>> 7 files changed, 82 insertions(+), 145 deletions(-)
>>
>> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
>> index b498e4b22bb3..b94dbd548843 100644
>> --- a/cmd/fastboot.c
>> +++ b/cmd/fastboot.c
>> @@ -19,8 +19,14 @@
>> static int do_fastboot_udp(int argc, char *const argv[],
>> uintptr_t buf_addr, size_t buf_size)
>> {
>> -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
>> -int err = net_loop(FASTBOOT);
>> +int err;
>> +
>> +if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
>> +pr_err("Fastboot UDP not enabled\n");
>> +return CMD_RET_FAILURE;
>> +}
>> +
>> +err = net_loop(FASTBOOT);
>>
>> if (err < 0) {
>> printf("fastboot udp error: %d\n", err);
>> @@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
>> }
>>
>> return CMD_RET_SUCCESS;
>> -#else
>> -pr_err("Fastboot UDP not enabled\n");
>> -return CMD_RET_FAILURE;
>> -#endif
>> }
>>
>> static int do_fastboot_usb(int argc, char *const argv[],
>> uintptr_t buf_addr, size_t buf_size)
>> {
>> -#if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)
>> int controller_index;
>> char *usb_controller;
>> char *endp;
>> int ret;
>>
>> +if (!CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)) {
>> +pr_err("Fastboot USB not enabled\n");
>> +return CMD_RET_FAILURE;
>> +}
>> +
>> if (argc < 2)
>> return CMD_RET_USAGE;
>>
>> @@ -88,10 +94,6 @@ exit:
>> g_dnl_clear_detach();
>>
>> return ret;
>> -#else
>> -pr_err("Fastboot USB not enabled\n");
>> -return CMD_RET_FAILURE;
>> -#endif
>> }
>>
>> static int do_fastboot(struct cmd_tbl *cmdtp, int flag, int argc,
>> @@ -148,17 +150,12 @@ NXTARG:
>> return do_fastboot_usb(argc, argv, buf_addr, buf_size);
>> }
>>
>> -#ifdef CONFIG_SYS_LONGHELP
>> -static char fastboot_help_text[] =
>> +U_BOOT_CMD(
>> +fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
>> +"run as a fastboot usb or udp device",
>> "[-l addr] [-s size] usb | udp\n"
>> "\taddr - address of buffer used during data transfers ("
>> __stringify(CONFIG_FASTBOOT_BUF_ADDR) ")\n"
>> "\tsize - size of buffer used during data transfers ("
>> __stringify(CONFIG_FASTBOOT_BUF_SIZE) ")"
>> -;
>> -#endif
>> -
>> -U_BOOT_CMD(
>> -fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
>> -"run as a fastboot usb or udp device", fastboot_help_text
>> );
>> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
>> index bdfdf262c8a3..f0fd605854da 100644
>> --- a/drivers/fastboot/fb_command.c
>> +++ b/drivers/fastboot/fb_command.c
>> @@ -31,27 +31,16 @@ static u32 fastboot_bytes_expected;
>> static void okay(char *, char *);
>> static void getvar(char *, char *);
>> static void download(char *, char *);
>> -#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>> static void flash(char *, char *);
>> static void erase(char *, char *);
>> -#endif
>> static void reboot_bootloader(char *, char *);
>> static void reboot_fastbootd(char *, char *);
>> static void reboot_recovery(char *, char *);
>> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
>> static void oem_format(char *, char *);
>> -#endif
>> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_PARTCONF)
>> static void oem_partconf(char *, char *);
>> -#endif
>> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_BOOTBUS)
>> static void oem_bootbus(char *, char *);
>> -#endif
>> -
>> -#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT)
>> static void run_ucmd(char *, char *);
>> static void run_acmd(char *, char *);
>> -#endif
>>
>> static const struct {
>> const char *command;
>> @@ -65,16 +54,14 @@ static const struct {
>> .command = "download",
>> .dispatch = download
>> },
>> -#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
>> [FASTBOOT_COMMAND_FLASH] = {
>> .command = "flash
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On 12/15/22 04:15, Patrick Delaunay wrote:
> Much of the fastboot code predates the introduction of Kconfig and
> has quite a few #ifdefs in it which is unnecessary now that we can use
> IS_ENABLED() et al.
>
> Signed-off-by: Patrick Delaunay
> ---
>
> cmd/fastboot.c | 35 +--
> drivers/fastboot/fb_command.c | 104
> drivers/fastboot/fb_common.c| 11 ++--
> drivers/fastboot/fb_getvar.c| 49 ++-
> drivers/usb/gadget/f_fastboot.c | 7 +--
> include/fastboot.h | 13
> net/fastboot.c | 8 +--
> 7 files changed, 82 insertions(+), 145 deletions(-)
>
> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> index b498e4b22bb3..b94dbd548843 100644
> --- a/cmd/fastboot.c
> +++ b/cmd/fastboot.c
> @@ -19,8 +19,14 @@
> static int do_fastboot_udp(int argc, char *const argv[],
> uintptr_t buf_addr, size_t buf_size)
> {
> -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
> - int err = net_loop(FASTBOOT);
> + int err;
> +
> + if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
> + pr_err("Fastboot UDP not enabled\n");
> + return CMD_RET_FAILURE;
> + }
> +
> + err = net_loop(FASTBOOT);
>
> if (err < 0) {
> printf("fastboot udp error: %d\n", err);
> @@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
> }
>
> return CMD_RET_SUCCESS;
> -#else
> - pr_err("Fastboot UDP not enabled\n");
> - return CMD_RET_FAILURE;
> -#endif
> }
>
> static int do_fastboot_usb(int argc, char *const argv[],
> uintptr_t buf_addr, size_t buf_size)
> {
> -#if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)
> int controller_index;
> char *usb_controller;
> char *endp;
> int ret;
>
> + if (!CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)) {
> + pr_err("Fastboot USB not enabled\n");
> + return CMD_RET_FAILURE;
> + }
> +
> if (argc < 2)
> return CMD_RET_USAGE;
>
> @@ -88,10 +94,6 @@ exit:
> g_dnl_clear_detach();
>
> return ret;
> -#else
> - pr_err("Fastboot USB not enabled\n");
> - return CMD_RET_FAILURE;
> -#endif
> }
>
> static int do_fastboot(struct cmd_tbl *cmdtp, int flag, int argc,
> @@ -148,17 +150,12 @@ NXTARG:
> return do_fastboot_usb(argc, argv, buf_addr, buf_size);
> }
>
> -#ifdef CONFIG_SYS_LONGHELP
> -static char fastboot_help_text[] =
> +U_BOOT_CMD(
> + fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
> + "run as a fastboot usb or udp device",
> "[-l addr] [-s size] usb | udp\n"
> "\taddr - address of buffer used during data transfers ("
> __stringify(CONFIG_FASTBOOT_BUF_ADDR) ")\n"
> "\tsize - size of buffer used during data transfers ("
> __stringify(CONFIG_FASTBOOT_BUF_SIZE) ")"
> - ;
> -#endif
> -
> -U_BOOT_CMD(
> - fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
> - "run as a fastboot usb or udp device", fastboot_help_text
> );
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index bdfdf262c8a3..f0fd605854da 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -31,27 +31,16 @@ static u32 fastboot_bytes_expected;
> static void okay(char *, char *);
> static void getvar(char *, char *);
> static void download(char *, char *);
> -#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
> static void flash(char *, char *);
> static void erase(char *, char *);
> -#endif
> static void reboot_bootloader(char *, char *);
> static void reboot_fastbootd(char *, char *);
> static void reboot_recovery(char *, char *);
> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
> static void oem_format(char *, char *);
> -#endif
> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_PARTCONF)
> static void oem_partconf(char *, char *);
> -#endif
> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_BOOTBUS)
> static void oem_bootbus(char *, char *);
> -#endif
> -
> -#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT)
> static void run_ucmd(char *, char *);
> static void run_acmd(char *, char *);
> -#endif
>
> static const struct {
> const char *command;
> @@ -65,16 +54,14 @@ static const struct {
> .command = "download",
> .dispatch = download
> },
> -#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
> [FASTBOOT_COMMAND_FLASH] = {
> .command = "flash",
> - .dispatch = flash
> + .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (flash), (NULL))
> },
> [FASTBOOT_COMMAND_ERASE] = {
> .command = "erase",
> - .dispatch = erase
> + .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (erase), (NULL))
> },
> -#endif
> [FASTBOOT_COMMAND_BOOT] = {
> .command = "boot",
> .dispatch = okay
> @@ -103,34 +90,26 @@ static const struct {
> .command = "set_acti
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On 12/15/22 09:30, Marek Vasut wrote:
> On 12/15/22 10:15, Patrick Delaunay wrote:
>> Much of the fastboot code predates the introduction of Kconfig and
>> has quite a few #ifdefs in it which is unnecessary now that we can use
>> IS_ENABLED() et al.
>>
>> Signed-off-by: Patrick Delaunay
>> ---
>>
>> cmd/fastboot.c | 35 +--
>> drivers/fastboot/fb_command.c | 104
>> drivers/fastboot/fb_common.c | 11 ++--
>> drivers/fastboot/fb_getvar.c | 49 ++-
>> drivers/usb/gadget/f_fastboot.c | 7 +--
>> include/fastboot.h | 13
>> net/fastboot.c | 8 +--
>> 7 files changed, 82 insertions(+), 145 deletions(-)
>>
>> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
>> index b498e4b22bb3..b94dbd548843 100644
>> --- a/cmd/fastboot.c
>> +++ b/cmd/fastboot.c
>> @@ -19,8 +19,14 @@
>> static int do_fastboot_udp(int argc, char *const argv[],
>> uintptr_t buf_addr, size_t buf_size)
>> {
>> -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
>
> Unrelated to this, don't we need to define Kconfig entries for 'config
> SPL_UDP_FUNCTION_FASTBOOT' and 'config TPL_UDP_FUNCTION_FASTBOOT' and the
> other macros tested in fastboot, so it would be correctly configurable in SPL
> ?
Is fastboot even supported in SPL? This should probably be IS_ENABLED.
--Sean
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On 12/15/22 10:15, Patrick Delaunay wrote:
Much of the fastboot code predates the introduction of Kconfig and
has quite a few #ifdefs in it which is unnecessary now that we can use
IS_ENABLED() et al.
Signed-off-by: Patrick Delaunay
---
cmd/fastboot.c | 35 +--
drivers/fastboot/fb_command.c | 104
drivers/fastboot/fb_common.c| 11 ++--
drivers/fastboot/fb_getvar.c| 49 ++-
drivers/usb/gadget/f_fastboot.c | 7 +--
include/fastboot.h | 13
net/fastboot.c | 8 +--
7 files changed, 82 insertions(+), 145 deletions(-)
diff --git a/cmd/fastboot.c b/cmd/fastboot.c
index b498e4b22bb3..b94dbd548843 100644
--- a/cmd/fastboot.c
+++ b/cmd/fastboot.c
@@ -19,8 +19,14 @@
static int do_fastboot_udp(int argc, char *const argv[],
uintptr_t buf_addr, size_t buf_size)
{
-#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
Unrelated to this, don't we need to define Kconfig entries for 'config
SPL_UDP_FUNCTION_FASTBOOT' and 'config TPL_UDP_FUNCTION_FASTBOOT' and
the other macros tested in fastboot, so it would be correctly
configurable in SPL ?
Re: [PATCH] fastboot: remove #ifdef CONFIG when it is possible
On Thu, Dec 15, 2022 at 10:15, Patrick Delaunay
wrote:
> Much of the fastboot code predates the introduction of Kconfig and
> has quite a few #ifdefs in it which is unnecessary now that we can use
> IS_ENABLED() et al.
>
> Signed-off-by: Patrick Delaunay
Hi Patrick,
Thank you for this, it's a nice improvement!
I will test this on vim3l tomorrow, but I've already reviewed it:
Reviewed-by: Mattijs Korpershoek
> ---
>
> cmd/fastboot.c | 35 +--
> drivers/fastboot/fb_command.c | 104
> drivers/fastboot/fb_common.c| 11 ++--
> drivers/fastboot/fb_getvar.c| 49 ++-
> drivers/usb/gadget/f_fastboot.c | 7 +--
> include/fastboot.h | 13
> net/fastboot.c | 8 +--
> 7 files changed, 82 insertions(+), 145 deletions(-)
>
> diff --git a/cmd/fastboot.c b/cmd/fastboot.c
> index b498e4b22bb3..b94dbd548843 100644
> --- a/cmd/fastboot.c
> +++ b/cmd/fastboot.c
> @@ -19,8 +19,14 @@
> static int do_fastboot_udp(int argc, char *const argv[],
> uintptr_t buf_addr, size_t buf_size)
> {
> -#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
> - int err = net_loop(FASTBOOT);
> + int err;
> +
> + if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
> + pr_err("Fastboot UDP not enabled\n");
> + return CMD_RET_FAILURE;
> + }
> +
> + err = net_loop(FASTBOOT);
>
> if (err < 0) {
> printf("fastboot udp error: %d\n", err);
> @@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
> }
>
> return CMD_RET_SUCCESS;
> -#else
> - pr_err("Fastboot UDP not enabled\n");
> - return CMD_RET_FAILURE;
> -#endif
> }
>
> static int do_fastboot_usb(int argc, char *const argv[],
> uintptr_t buf_addr, size_t buf_size)
> {
> -#if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)
> int controller_index;
> char *usb_controller;
> char *endp;
> int ret;
>
> + if (!CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)) {
> + pr_err("Fastboot USB not enabled\n");
> + return CMD_RET_FAILURE;
> + }
> +
> if (argc < 2)
> return CMD_RET_USAGE;
>
> @@ -88,10 +94,6 @@ exit:
> g_dnl_clear_detach();
>
> return ret;
> -#else
> - pr_err("Fastboot USB not enabled\n");
> - return CMD_RET_FAILURE;
> -#endif
> }
>
> static int do_fastboot(struct cmd_tbl *cmdtp, int flag, int argc,
> @@ -148,17 +150,12 @@ NXTARG:
> return do_fastboot_usb(argc, argv, buf_addr, buf_size);
> }
>
> -#ifdef CONFIG_SYS_LONGHELP
> -static char fastboot_help_text[] =
> +U_BOOT_CMD(
> + fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
> + "run as a fastboot usb or udp device",
> "[-l addr] [-s size] usb | udp\n"
> "\taddr - address of buffer used during data transfers ("
> __stringify(CONFIG_FASTBOOT_BUF_ADDR) ")\n"
> "\tsize - size of buffer used during data transfers ("
> __stringify(CONFIG_FASTBOOT_BUF_SIZE) ")"
> - ;
> -#endif
> -
> -U_BOOT_CMD(
> - fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
> - "run as a fastboot usb or udp device", fastboot_help_text
> );
> diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
> index bdfdf262c8a3..f0fd605854da 100644
> --- a/drivers/fastboot/fb_command.c
> +++ b/drivers/fastboot/fb_command.c
> @@ -31,27 +31,16 @@ static u32 fastboot_bytes_expected;
> static void okay(char *, char *);
> static void getvar(char *, char *);
> static void download(char *, char *);
> -#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
> static void flash(char *, char *);
> static void erase(char *, char *);
> -#endif
> static void reboot_bootloader(char *, char *);
> static void reboot_fastbootd(char *, char *);
> static void reboot_recovery(char *, char *);
> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
> static void oem_format(char *, char *);
> -#endif
> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_PARTCONF)
> static void oem_partconf(char *, char *);
> -#endif
> -#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_BOOTBUS)
> static void oem_bootbus(char *, char *);
> -#endif
> -
> -#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT)
> static void run_ucmd(char *, char *);
> static void run_acmd(char *, char *);
> -#endif
>
> static const struct {
> const char *command;
> @@ -65,16 +54,14 @@ static const struct {
> .command = "download",
> .dispatch = download
> },
> -#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
> [FASTBOOT_COMMAND_FLASH] = {
> .command = "flash",
> - .dispatch = flash
> + .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (flash), (NULL))
> },
> [FASTBOOT_COMMAND_ERASE] = {
> .command = "erase",
> - .dispatch = erase
> + .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (erase), (NULL))
> },
> -#endif
> [FAS
[PATCH] fastboot: remove #ifdef CONFIG when it is possible
Much of the fastboot code predates the introduction of Kconfig and
has quite a few #ifdefs in it which is unnecessary now that we can use
IS_ENABLED() et al.
Signed-off-by: Patrick Delaunay
---
cmd/fastboot.c | 35 +--
drivers/fastboot/fb_command.c | 104
drivers/fastboot/fb_common.c| 11 ++--
drivers/fastboot/fb_getvar.c| 49 ++-
drivers/usb/gadget/f_fastboot.c | 7 +--
include/fastboot.h | 13
net/fastboot.c | 8 +--
7 files changed, 82 insertions(+), 145 deletions(-)
diff --git a/cmd/fastboot.c b/cmd/fastboot.c
index b498e4b22bb3..b94dbd548843 100644
--- a/cmd/fastboot.c
+++ b/cmd/fastboot.c
@@ -19,8 +19,14 @@
static int do_fastboot_udp(int argc, char *const argv[],
uintptr_t buf_addr, size_t buf_size)
{
-#if CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)
- int err = net_loop(FASTBOOT);
+ int err;
+
+ if (!CONFIG_IS_ENABLED(UDP_FUNCTION_FASTBOOT)) {
+ pr_err("Fastboot UDP not enabled\n");
+ return CMD_RET_FAILURE;
+ }
+
+ err = net_loop(FASTBOOT);
if (err < 0) {
printf("fastboot udp error: %d\n", err);
@@ -28,21 +34,21 @@ static int do_fastboot_udp(int argc, char *const argv[],
}
return CMD_RET_SUCCESS;
-#else
- pr_err("Fastboot UDP not enabled\n");
- return CMD_RET_FAILURE;
-#endif
}
static int do_fastboot_usb(int argc, char *const argv[],
uintptr_t buf_addr, size_t buf_size)
{
-#if CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)
int controller_index;
char *usb_controller;
char *endp;
int ret;
+ if (!CONFIG_IS_ENABLED(USB_FUNCTION_FASTBOOT)) {
+ pr_err("Fastboot USB not enabled\n");
+ return CMD_RET_FAILURE;
+ }
+
if (argc < 2)
return CMD_RET_USAGE;
@@ -88,10 +94,6 @@ exit:
g_dnl_clear_detach();
return ret;
-#else
- pr_err("Fastboot USB not enabled\n");
- return CMD_RET_FAILURE;
-#endif
}
static int do_fastboot(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -148,17 +150,12 @@ NXTARG:
return do_fastboot_usb(argc, argv, buf_addr, buf_size);
}
-#ifdef CONFIG_SYS_LONGHELP
-static char fastboot_help_text[] =
+U_BOOT_CMD(
+ fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
+ "run as a fastboot usb or udp device",
"[-l addr] [-s size] usb | udp\n"
"\taddr - address of buffer used during data transfers ("
__stringify(CONFIG_FASTBOOT_BUF_ADDR) ")\n"
"\tsize - size of buffer used during data transfers ("
__stringify(CONFIG_FASTBOOT_BUF_SIZE) ")"
- ;
-#endif
-
-U_BOOT_CMD(
- fastboot, CONFIG_SYS_MAXARGS, 1, do_fastboot,
- "run as a fastboot usb or udp device", fastboot_help_text
);
diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c
index bdfdf262c8a3..f0fd605854da 100644
--- a/drivers/fastboot/fb_command.c
+++ b/drivers/fastboot/fb_command.c
@@ -31,27 +31,16 @@ static u32 fastboot_bytes_expected;
static void okay(char *, char *);
static void getvar(char *, char *);
static void download(char *, char *);
-#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
static void flash(char *, char *);
static void erase(char *, char *);
-#endif
static void reboot_bootloader(char *, char *);
static void reboot_fastbootd(char *, char *);
static void reboot_recovery(char *, char *);
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
static void oem_format(char *, char *);
-#endif
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_PARTCONF)
static void oem_partconf(char *, char *);
-#endif
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_BOOTBUS)
static void oem_bootbus(char *, char *);
-#endif
-
-#if CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT)
static void run_ucmd(char *, char *);
static void run_acmd(char *, char *);
-#endif
static const struct {
const char *command;
@@ -65,16 +54,14 @@ static const struct {
.command = "download",
.dispatch = download
},
-#if CONFIG_IS_ENABLED(FASTBOOT_FLASH)
[FASTBOOT_COMMAND_FLASH] = {
.command = "flash",
- .dispatch = flash
+ .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (flash), (NULL))
},
[FASTBOOT_COMMAND_ERASE] = {
.command = "erase",
- .dispatch = erase
+ .dispatch = CONFIG_IS_ENABLED(FASTBOOT_FLASH, (erase), (NULL))
},
-#endif
[FASTBOOT_COMMAND_BOOT] = {
.command = "boot",
.dispatch = okay
@@ -103,34 +90,26 @@ static const struct {
.command = "set_active",
.dispatch = okay
},
-#if CONFIG_IS_ENABLED(FASTBOOT_CMD_OEM_FORMAT)
[FASTBOOT_COMMAND_OEM_FORMAT] = {
.command = "oem format",
- .dispatch = oem_format,
+
