Hi Nikhil,
Thanks for the patch.
On 10/04/23 12:58, Nikhil M Jain wrote:
> Remove #ifdef in header file to avoid multiple definitions while
multiple definitions doesnt seem to be right phrase as that will give
compilation error anyway.
> compilation. To improve code readability use if() rather than if's and
> '#ifdef's'.
Avoid using preprocessor compilation directives and instead use simple logical
expressions for better readability since compiler will anyway optimize
out the respective code block if condition is not satisfied.
>
> Signed-off-by: Nikhil M Jain <n-ja...@ti.com>
> ---
> V7(patch introduced):
> - Replace #ifdef and #if with if's.
>
> common/bmp.c | 12 +++---------
> common/splash.c | 12 ++++++------
> include/splash.h | 12 ------------
> 3 files changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/common/bmp.c b/common/bmp.c
> index 51766b3c21..62b2d07d23 100644
> --- a/common/bmp.c
> +++ b/common/bmp.c
> @@ -33,7 +33,7 @@
> * Returns NULL if decompression failed, or if the decompressed data
> * didn't contain a valid BMP signature.
or decompression is not enabled in Kconfig.
> */
> -#if CONFIG_IS_ENABLED(VIDEO_BMP_GZIP)
> +
nitpick, blank line.
> struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> void **alloc_addr)
> {
> @@ -41,6 +41,8 @@ struct bmp_image *gunzip_bmp(unsigned long addr, unsigned
> long *lenp,
> unsigned long len;
> struct bmp_image *bmp;
>
> + if (!CONFIG_IS_ENABLED(VIDEO_BMP_GZIP))
> + return NULL;
> /*
> * Decompress bmp image
> */
> @@ -77,14 +79,6 @@ struct bmp_image *gunzip_bmp(unsigned long addr, unsigned
> long *lenp,
> *alloc_addr = dst;
> return bmp;
> }
> -#else
> -struct bmp_image *gunzip_bmp(unsigned long addr, unsigned long *lenp,
> - void **alloc_addr)
> -{
> - return NULL;
> -}
> -#endif
> -
>
> #ifdef CONFIG_NEEDS_MANUAL_RELOC
Don't you want to remove this too and switch to if ?
> void bmp_reloc(void) {
> diff --git a/common/splash.c b/common/splash.c
> index a4e68b7042..4c38a155d0 100644
> --- a/common/splash.c
> +++ b/common/splash.c
> @@ -79,7 +79,7 @@ static int splash_video_logo_load(void)
> }
>
> memcpy((void *)bmp_load_addr, bmp_logo_bitmap,
> - ARRAY_SIZE(bmp_logo_bitmap));
> + ARRAY_SIZE(bmp_logo_bitmap));
Is this intended? Does checkpatch work fine with that?
>
> return 0;
> }
> @@ -96,11 +96,12 @@ __weak int splash_screen_prepare(void)
> return splash_video_logo_load();
> }
>
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN_ALIGN)
> void splash_get_pos(int *x, int *y)
> {
> char *s = env_get("splashpos");
>
> + if (!CONFIG_IS_ENABLED(SPLASH_SCREEN_ALIGN))
> + return;
> if (!s)
> return;
You can combine above both in a single statement using ||
>
> @@ -117,7 +118,6 @@ void splash_get_pos(int *x, int *y)
> *y = simple_strtol(s + 1, NULL, 0);
> }
> }
> -#endif /* CONFIG_SPLASH_SCREEN_ALIGN */
>
> #if CONFIG_IS_ENABLED(VIDEO) && !CONFIG_IS_ENABLED(HIDE_LOGO_VERSION)
>
Don't you intend to remove above too and switch to if for all #ifdefs ?
> @@ -159,13 +159,14 @@ void splash_display_banner(void)
> * Common function to show a splash image if env("splashimage") is set.
> * For additional details please refer to doc/README.splashprepare.
> */
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN) && CONFIG_IS_ENABLED(BMP)
> +
> int splash_display(void)
> {
> ulong addr;
> char *s;
> int x = 0, y = 0, ret;
> -
> + if (!(CONFIG_IS_ENABLED(SPLASH_SCREEN) && CONFIG_IS_ENABLED(BMP)))
> + return -ENOSYS;
I think BMP check should come under bmp specific code latter in the function.
Regards
Devarsh
> s = env_get("splashimage");
> if (!s)
> return -EINVAL;
> @@ -189,4 +190,3 @@ int splash_display(void)
> end:
> return ret;
> }
> -#endif
> diff --git a/include/splash.h b/include/splash.h
> index aa0c14024e..8f15adf9dd 100644
> --- a/include/splash.h
> +++ b/include/splash.h
> @@ -61,21 +61,9 @@ static inline int splash_source_load(struct
> splash_location *locations,
>
> int splash_screen_prepare(void);
>
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN_ALIGN)
> void splash_get_pos(int *x, int *y);
> -#else
> -static inline void splash_get_pos(int *x, int *y) { }
> -#endif
>
> -#if CONFIG_IS_ENABLED(SPLASH_SCREEN) && CONFIG_IS_ENABLED(BMP)
> int splash_display(void);
> -#else
> -static inline int splash_display(void)
> -{
> - return -ENOSYS;
> -}
> -#endif
> -
> #define BMP_ALIGN_CENTER 0x7FFF
>
> #endif
> \ No newline at end of file
