Re: [PATCH 2/4] fastboot: blk: add block device flashing configuration

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Dmitrii,

Thank you for the patch and sorry for the review delay.

On mer., mars 06, 2024 at 18:59, Dmitrii Merkurev <dimorinny@google.com> wrote:

> Signed-off-by: Dmitrii Merkurev <dimorinny@google.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> ---
>  drivers/fastboot/Kconfig | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig
> index 5e5855a76c..460c5e98d7 100644
> --- a/drivers/fastboot/Kconfig
> +++ b/drivers/fastboot/Kconfig
> @@ -87,7 +87,7 @@ config FASTBOOT_USB_DEV
>  config FASTBOOT_FLASH
>  	bool "Enable FASTBOOT FLASH command"
>  	default y if ARCH_SUNXI || ARCH_ROCKCHIP
> -	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS)
> +	depends on MMC || (MTD_RAW_NAND && CMD_MTDPARTS) || BLK
>  	select IMAGE_SPARSE
>  	help
>  	  The fastboot protocol includes a "flash" command for writing
> @@ -109,12 +109,16 @@ choice
>  
>  config FASTBOOT_FLASH_MMC
>  	bool "FASTBOOT on MMC"
> -	depends on MMC
> +	depends on MMC && BLK
>  
>  config FASTBOOT_FLASH_NAND
>  	bool "FASTBOOT on NAND"
>  	depends on MTD_RAW_NAND && CMD_MTDPARTS
>  
> +config FASTBOOT_FLASH_BLOCK
> +	bool "FASTBOOT on block device"
> +	depends on BLK
> +

If we just apply this patch, then this KConfig option is unused. Can we
please squash this into patch 3/4?

>  endchoice
>  
>  config FASTBOOT_FLASH_MMC_DEV
> @@ -189,6 +193,18 @@ config FASTBOOT_MMC_USER_NAME
>  	  defined here.
>  	  The default target name for erasing EMMC_USER is "mmc0".
>  
> +config FASTBOOT_FLASH_BLOCK_INTERFACE_NAME
> +	string "Define FASTBOOT block interface name"
> +	depends on FASTBOOT_FLASH_BLOCK
> +	help
> +	  "Fastboot block interface name (mmc, virtio, etc)"

There is a finite list of supported options for this. Can we please
document all of them so that users know what is valid and what not?
If so, we can drop the "etc" part in the help string.

> +
> +config FASTBOOT_FLASH_BLOCK_DEVICE_ID
> +	int "Define FASTBOOT block device id"
> +	depends on FASTBOOT_FLASH_BLOCK
> +	help
> +	  "Fastboot block device id"

When FASTBOOT_FLASH_BLOCK=="mmc", how is FASTBOOT_FLASH_BLOCK_DEVICE_ID
different from FASTBOOT_FLASH_MMC_DEV?

If they are similar, are we sure this symbol really needed?

> +
>  config FASTBOOT_GPT_NAME
>  	string "Target name for updating GPT"
>  	depends on FASTBOOT_FLASH_MMC && EFI_PARTITION
> -- 
> 2.44.0.278.ge034bb2e1d-goog