Hi Tien Fong,
Thanks for the review.
Agreed on those points — addressed in v2 as below.
On 9/22/2026 12:46 pm, Tien Fong Chee wrote:
>> +__weak struct blk_desc *blk_get_by_device(struct udevice *dev)
>> +{
>> + return NULL;
>> +}
>
> Adding a weak stub next to an existing strong implementation is
> confusing and can mask missing BLK linkage in unusual configs.
>
> Drop the weak stub; include <blk.h>.
Dropped the weak blk_get_by_device() stub and kept #include <blk.h>.
>> - if (!name || name[0] == '\0')
>> - return -EINVAL;
>
> RAW mode legitimately encodes a hex offset in `name`, but FS mode still
> needs a non-empty filename for fs_read().
>
> Recommend to keep empty-name rejection for DATA_FS; only parse hex
> offsets in DATA_RAW (and still reject parse failure).
Restored empty-name rejection for DATA_FS. Hex offset parsing is limited
to DATA_RAW, with parse failure still returning -EINVAL.
>> +#ifdef CONFIG_SPI_FLASH
>> + ret = spi_flash_read_dm(...);
>> + actread = firmwarep->size;
>> +#endif
>
> Current with SPI_FLASH off, ret stays 0 and the function returns
> actread==0 as success.
>
> Recommend return -ENOSYS / -EINVAL when DATA_RAW is selected but SPI
> flash support is unavailable.
Return -ENOSYS when DATA_RAW is selected but CONFIG_SPI_FLASH is not
enabled.
>> + if (!plat->flash) {
>> + ret = spi_flash_probe_bus_cs(plat->sfconfig.bus,
>> + plat->sfconfig.cs,
>> + &plat->flash);
>
> Recommend to gate the SPI probe on plat->data_type == DATA_RAW (and/or
> successful sfconfig parse). Leave DATA_FS probe behavior unchanged.
Gated spi_flash_probe_bus_cs() on plat->data_type == DATA_RAW, so
existing MMC/UBI fs-loader nodes keep their previous probe behavior.
Will include these in the v2 series.
Best regards,
Lok
