[PATCH v2 2/2] fastboot: add default fastboot_set_reboot_flag implementation

[Roman Stratiienko]

Hi Roman,

чт, 4 июн. 2020 г. в 10:56, Roman Kovalivskyi
<roman.kovalivskyi at globallogic.com>:
>
> Default implementation of fastboot_set_reboot_flag function that depends
> on "bcb" commands could be used in general case if there are no need to
> make any platform-specific implementation, otherwise it could be
> disabled via Kconfig option FASTBOOT_USE_BCB_SET_REBOOT_FLAG.
>
> Please note that FASTBOOT_USE_BCB_SET_REBOOT_FLAG is mutually exclusive
> with some platforms which already have their own implementation of this
> function.
>
> Signed-off-by: Roman Kovalivskyi <roman.kovalivskyi at globallogic.com>
> ---
>  drivers/fastboot/Kconfig     |  9 +++++++++
>  drivers/fastboot/fb_common.c | 36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+)
>
> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> index d4436dfc9173..bcb43bc5d556 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -165,6 +165,15 @@ config FASTBOOT_CMD_OEM_FORMAT
>           relies on the env variable partitions to contain the list of
>           partitions as required by the gpt command.
>
> +config FASTBOOT_USE_BCB_SET_REBOOT_FLAG
> +       bool "Enable default fastboot_set_reboot_flag implementation"
> +       depends on CMD_BCB && !ARCH_MESON && !ARCH_ROCKCHIP && !TARGET_KC1 && \
> +               !TARGET_SNIPER && !TARGET_AM57XX_EVM && !TARGET_DRA7XX_EVM
> +       default 1
> +       help
> +               Add default implementation of fastboot_set_reboot_flag that uses
> +               "bcb" commands.
> +
>  endif # FASTBOOT
>
>  endmenu
> diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c
> index 012a6288c187..77fe22e88f3d 100644
> --- a/drivers/fastboot/fb_common.c
> +++ b/drivers/fastboot/fb_common.c
> @@ -88,6 +88,41 @@ void fastboot_okay(const char *reason, char *response)
>   * which sets whatever flag your board specific Android bootloader flow
>   * requires in order to re-enter the bootloader.
>   */

Consider creating an array :
const char *bcb_boot_reason_commands = {"bootonce-bootloader",
"boot-fastboot", "boot-recovery");

> +#if CONFIG_IS_ENABLED(FASTBOOT_USE_BCB_SET_REBOOT_FLAG)
> +int fastboot_set_reboot_flag(int reason)
> +{
> +       char cmd[32];
> +

Check the reason range is in between 0 to FASTBOOT_REBOOT_REASON_COUNT-1.

> +       snprintf(cmd, sizeof(cmd), "bcb load %d misc",
> +                CONFIG_FASTBOOT_FLASH_MMC_DEV);
> +
> +       if (run_command(cmd, 0))
> +               return -ENODEV;
> +
> +       switch (reason) {
> +       case FASTBOOT_REBOOT_BOOTLOADER:
> +               snprintf(cmd, sizeof(cmd),
> +                        "bcb set command bootonce-bootloader");
> +               break;
> +       case FASTBOOT_REBOOT_FASTBOOTD:
> +               snprintf(cmd, sizeof(cmd), "bcb set command boot-fastboot");
> +               break;
> +       case FASTBOOT_REBOOT_RECOVERY:
> +               snprintf(cmd, sizeof(cmd), "bcb set command boot-recovery");
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

Instead of doing switch-case, consider using:
snprintf(cmd, sizeof(cmd), "bcb set command %s",
bcb_boot_reason_commands[reason]);

> +
> +       if (run_command(cmd, 0))
> +               return -ENOEXEC;
> +
> +       if (run_command("bcb store", 0))
> +               return -EIO;
> +
> +       return 0;
> +}
> +#else
>  int __weak fastboot_set_reboot_flag(int reason)
>  {
>         if (reason != FASTBOOT_REBOOT_BOOTLOADER)
> @@ -95,6 +130,7 @@ int __weak fastboot_set_reboot_flag(int reason)
>
>         return -ENOSYS;
>  }
> +#endif
>
>  /**
>   * fastboot_get_progress_callback() - Return progress callback
> --
> 2.17.1
>

Why not move this logic into a separate file?

Regards,
Roman Stratiienko