| Message ID | 20230410072843.97922-15-n-jain1@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
| Series | Add splash screen support at u-boot SPL | expand |
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; > 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
Hi Devarsh On 17/04/23 11:09, Devarsh Thakkar wrote: > 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 ? I will remove it. >> 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? >> I will do a recheck. >> 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 || >> I will combine them. >> @@ -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. > Yes, it will be better if we just keep BMP check for bmp_display function. > Regards > Devarsh Thanks, Nikhil >> 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
On 17/04/23 11:09, Devarsh Thakkar wrote: > 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 ? If this config is removed while compilation it will look for cmd/bmp which is not enabled at spl and compilation fails. >> 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 ? Removing these ifdefs' generates warning for implicit declaration. >> @@ -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
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. */ -#if CONFIG_IS_ENABLED(VIDEO_BMP_GZIP) + 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 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)); 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; @@ -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) @@ -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; 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
Remove #ifdef in header file to avoid multiple definitions while compilation. To improve code readability use if() rather than if's and '#ifdef's'. 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(-)