[PATCH v2 04/14] spl: Remove remaining #ifdef in spl_parse_image_header()

[Simon Glass]

Hi Quentin,

On Tue, 6 Aug 2024 at 07:24, Quentin Schulz <quentin.schulz at cherry.de> wrote:
>
> Hi Simon,
>
> On 7/21/24 5:25 PM, Simon Glass wrote:
> > Define spl_set_header_raw_uboot() always so we can drop the last #ifdef
> > in this function.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > Reviewed-by: Sean Anderson <seanga2 at gmail.com>
> > ---
> >
> > Changes in v2:
> > - Avoid changing comment
> >
> >   common/spl/spl.c | 10 ++++------
> >   1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/common/spl/spl.c b/common/spl/spl.c
> > index 02567e766f1..30ff1e6f7d7 100644
> > --- a/common/spl/spl.c
> > +++ b/common/spl/spl.c
> > @@ -245,7 +245,6 @@ __weak struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size)
> >       return map_sysmem(CONFIG_TEXT_BASE + offset, 0);
> >   }
> >
> > -#ifdef CONFIG_SPL_RAW_IMAGE_SUPPORT
> >   void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
> >   {
> >       ulong u_boot_pos = spl_get_image_pos();
> > @@ -273,7 +272,6 @@ void spl_set_header_raw_uboot(struct spl_image_info *spl_image)
> >       spl_image->os = IH_OS_U_BOOT;
> >       spl_image->name = "U-Boot";
> >   }
> > -#endif
>
> I assume this doesn't add too much to the SPL in terms of size?
> Otherwise we could just define an empty function when
> CONFIG_SPL_RAW_IMAGE_SUPPORT isn't defined to avoid bloating the SPL?
>
> >
> >   __weak int spl_parse_board_header(struct spl_image_info *spl_image,
> >                                 const struct spl_boot_device *bootdev,
> > @@ -357,16 +355,16 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
> >                                   sizeof(*header)))
> >               return 0;
> >
> > -#ifdef CONFIG_SPL_RAW_IMAGE_SUPPORT
> > +     if (IS_ENABLED(CONFIG_SPL_RAW_IMAGE_SUPPORT)) {
> >               /* Signature not found - assume u-boot.bin */
> >               debug("mkimage signature not found - ih_magic = %x\n",
> > -                     header->ih_magic);
> > +                   header->ih_magic);
> >               spl_set_header_raw_uboot(spl_image);
>
> We could return 0 here.......
>
> > -#else
> > +     } else {
>
> .... to avoid here the else whose last instruction is returning -EINVAL.

Sure, but then the function would be returning -EINVAL at the end. We
try to have functions end with success, with failure handled by its
returning early. That way we can read the 'normal' flow of the
function from top to bottom.

>
> In any case,
> Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>
>
> Thanks!
> Quentin

Thanks for all your reviews.

Regards,
Simon