[PATCH v5 2/6] misc: fs_loader: reorganize and split to FS and FW loader

[Simon Glass]

Hi Christian,

On 2026-04-03T13:51:57, Christian Marangi <ansuelsmth at gmail.com> wrote:
> misc: fs_loader: reorganize and split to FS and FW loader
>
> In preparation to the introduction of variant of the FS loader,
> reorganize and split the driver to generic fw_loader function and
> specific fs_loader function. Create a dedicated directory for the loader
> and move the internal structs and functions to a dedicated header file.
>
> This will permit to reuse all the property and logic of FS loader
> with container that are not exactly a readable filesystem.
>
> Signed-off-by: Christian Marangi <ansuelsmth at gmail.com>

> +struct device_plat {
> + struct phandle_part phandlepart;
> + char *mtdpart;
> + char *ubivol;
> +
> + int (*get_firmware)(struct udevice *dev);
> +};

This series introduces a generic firmware-loader framework with two
implementations (FS and FIP), which is a good idea. But the dispatch
mechanism uses function pointers in platform data rather than U-Boot's
driver model ops.

In U-Boot's driver model, polymorphic behaviour is handled by uclass
operations. Instead of storing function pointers in device_plat and
setting them during probe, please can you define a proper ops
structure:

    struct fw_loader_ops {
        int (*get_firmware)(struct udevice *dev);
        int (*get_size)(struct udevice *dev);
    };

Each driver would then declare its ops:

    static const struct fw_loader_ops fs_loader_ops = {
        .get_firmware = fw_get_filesystem_firmware,
        .get_size = fw_get_filesystem_firmware_size,
    };

And fw_loader.c would use dev_get_uclass_ops() to dispatch. This also
means you only need a single UCLASS_FIRMWARE_LOADER rather than
separate UCLASS_FS_FIRMWARE_LOADER and UCLASS_FIP_FIRMWARE_LOADER -
the ops handle the polymorphism, which is the whole point of having a
uclass.

See drivers/spi/spi-uclass.c or drivers/i2c/i2c-uclass.c for examples
of this pattern.

> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> @@ -612,8 +612,12 @@ config MPC83XX_SERDES
> +config FW_LOADER_LIB
> +     bool
> +
>  config FS_LOADER
>       bool "Enable loader driver for file system"
> +     select FW_LOADER_DRV

FW_LOADER_LIB is defined but never used. FW_LOADER_DRV is selected but
never defined. I suspect you meant to define FW_LOADER_DRV here
instead of FW_LOADER_LIB.

> diff --git a/drivers/misc/fw_loader/fw_loader.c b/drivers/misc/fw_loader/fw_loader.c
> @@ -0,0 +1,144 @@
> +static int _request_firmware_prepare(struct udevice *dev,
> +                                  const char *name, void *dbuf,
> +                                  size_t size, u32 offset)
> +{
> +     if (!name || name[0] == '\0')
> +             return -EINVAL;
> +
> +     struct firmware *firmwarep = dev_get_priv(dev);

Please can you move the variable declaration to the top of the
function, before the statements.

> diff --git a/drivers/misc/fw_loader/internal.h b/drivers/misc/fw_loader/internal.h
> @@ -0,0 +1,61 @@
> +/**
> + * struct firmware - A place for storing firmware and its attribute data.
> + *
> + * This holds information about a firmware and its content.
> + *
> + * @size: Size of a file
> + * @data: Buffer for file
> + * @priv: Firmware loader private fields
> + * @name: Filename
> + * @offset: Offset of reading a file
> + */
> +struct firmware {
> +     size_t size;
> +     const u8 *data;
> +     const char *name;
> +     u32 offset;
> +};

The kernel-doc mentions @priv but there is no priv member in the struct.

Regards,
Simon