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-jain1@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;
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