| Message ID | 1422530505-19054-3-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Superseded |
| Delegated to: | Anatolij Gustschin |
| Headers | show |
Hi Nikita, On 29 January 2015 at 04:21, Nikita Kiryanov <nikita@compulab.co.il> wrote: > configuration_get_cmap() is multiple platform specific functions stuffed into > one function. Split it into multiple versions, and move each version to the > appropriate driver to reduce the #ifdef complexity. > > Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> > Cc: Bo Shen <voice.shen@atmel.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Anatolij Gustschin <agust@denx.de> > --- > common/lcd.c | 19 ------------------- > drivers/video/atmel_hlcdfb.c | 13 +++++++++++++ > drivers/video/atmel_lcdfb.c | 5 +++++ > drivers/video/exynos_fb.c | 9 +++++++++ > drivers/video/mpc8xx_lcd.c | 7 +++++++ > drivers/video/pxa_lcd.c | 6 ++++++ > include/lcd.h | 9 +++++++++ > 7 files changed, 49 insertions(+), 19 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> See suggestion below. [snip] > diff --git a/include/lcd.h b/include/lcd.h > index fbba6a2..838f645 100644 > --- a/include/lcd.h > +++ b/include/lcd.h > @@ -42,13 +42,17 @@ void lcd_set_flush_dcache(int flush); > > #if defined CONFIG_MPC823 > #include <mpc823_lcd.h> > +ushort *configuration_get_cmap(void); > #elif defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \ > defined CONFIG_CPU_MONAHANS > #include <pxa_lcd.h> > +ushort *configuration_get_cmap(void); > #elif defined(CONFIG_ATMEL_LCD) || defined(CONFIG_ATMEL_HLCD) > #include <atmel_lcd.h> > +ushort *configuration_get_cmap(void); > #elif defined(CONFIG_EXYNOS_FB) > #include <exynos_lcd.h> > +ushort *configuration_get_cmap(void); > #else > typedef struct vidinfo { > ushort vl_col; /* Number of columns (i.e. 160) */ > @@ -60,6 +64,11 @@ typedef struct vidinfo { > > void *priv; /* Pointer to driver-specific data */ > } vidinfo_t; > + > +static inline ushort *configuration_get_cmap(void) > +{ > + return panel_info.cmap; > +} > #endif I'd argue for dropping the inline version and just having the same prototype in each case. Regards, Simon
Hi Simon, On 01/31/2015 02:24 AM, Simon Glass wrote: > Hi Nikita, > > On 29 January 2015 at 04:21, Nikita Kiryanov <nikita@compulab.co.il> wrote: >> configuration_get_cmap() is multiple platform specific functions stuffed into >> one function. Split it into multiple versions, and move each version to the >> appropriate driver to reduce the #ifdef complexity. >> >> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >> Cc: Bo Shen <voice.shen@atmel.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Anatolij Gustschin <agust@denx.de> >> --- >> common/lcd.c | 19 ------------------- >> drivers/video/atmel_hlcdfb.c | 13 +++++++++++++ >> drivers/video/atmel_lcdfb.c | 5 +++++ >> drivers/video/exynos_fb.c | 9 +++++++++ >> drivers/video/mpc8xx_lcd.c | 7 +++++++ >> drivers/video/pxa_lcd.c | 6 ++++++ >> include/lcd.h | 9 +++++++++ >> 7 files changed, 49 insertions(+), 19 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > See suggestion below. > > [snip] >> diff --git a/include/lcd.h b/include/lcd.h >> index fbba6a2..838f645 100644 >> --- a/include/lcd.h >> +++ b/include/lcd.h >> @@ -42,13 +42,17 @@ void lcd_set_flush_dcache(int flush); >> >> #if defined CONFIG_MPC823 >> #include <mpc823_lcd.h> >> +ushort *configuration_get_cmap(void); >> #elif defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \ >> defined CONFIG_CPU_MONAHANS >> #include <pxa_lcd.h> >> +ushort *configuration_get_cmap(void); >> #elif defined(CONFIG_ATMEL_LCD) || defined(CONFIG_ATMEL_HLCD) >> #include <atmel_lcd.h> >> +ushort *configuration_get_cmap(void); >> #elif defined(CONFIG_EXYNOS_FB) >> #include <exynos_lcd.h> >> +ushort *configuration_get_cmap(void); >> #else >> typedef struct vidinfo { >> ushort vl_col; /* Number of columns (i.e. 160) */ >> @@ -60,6 +64,11 @@ typedef struct vidinfo { >> >> void *priv; /* Pointer to driver-specific data */ >> } vidinfo_t; >> + >> +static inline ushort *configuration_get_cmap(void) >> +{ >> + return panel_info.cmap; >> +} >> #endif > > I'd argue for dropping the inline version and just having the same > prototype in each case. Are you suggesting something along the lines of: static ushort *configuration_get_cmap(void) { return panel_info.cmap; } #endif ushort *configuration_get_cmap(void); ? This is something I considered while preparing this, but I was under the impression that this isn't well defined by the standard. However, I actually have confirmation now that it is in fact defined behavior[1], so I will submit a V2 for this (even though I expect all of the above to disappear from lcd.h in the next refactor series). [1] http://stackoverflow.com/questions/28264874/non-static-function-prototype-follows-static-function-declaration > > Regards, > Simon >
Hi Nikita, On 1 February 2015 at 10:02, Nikita Kiryanov <nikita@compulab.co.il> wrote: > Hi Simon, > > > On 01/31/2015 02:24 AM, Simon Glass wrote: >> >> Hi Nikita, >> >> On 29 January 2015 at 04:21, Nikita Kiryanov <nikita@compulab.co.il> >> wrote: >>> >>> configuration_get_cmap() is multiple platform specific functions stuffed >>> into >>> one function. Split it into multiple versions, and move each version to >>> the >>> appropriate driver to reduce the #ifdef complexity. >>> >>> Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> >>> Cc: Bo Shen <voice.shen@atmel.com> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: Anatolij Gustschin <agust@denx.de> >>> --- >>> common/lcd.c | 19 ------------------- >>> drivers/video/atmel_hlcdfb.c | 13 +++++++++++++ >>> drivers/video/atmel_lcdfb.c | 5 +++++ >>> drivers/video/exynos_fb.c | 9 +++++++++ >>> drivers/video/mpc8xx_lcd.c | 7 +++++++ >>> drivers/video/pxa_lcd.c | 6 ++++++ >>> include/lcd.h | 9 +++++++++ >>> 7 files changed, 49 insertions(+), 19 deletions(-) >> >> >> Reviewed-by: Simon Glass <sjg@chromium.org> >> >> See suggestion below. >> >> [snip] >>> >>> diff --git a/include/lcd.h b/include/lcd.h >>> index fbba6a2..838f645 100644 >>> --- a/include/lcd.h >>> +++ b/include/lcd.h >>> @@ -42,13 +42,17 @@ void lcd_set_flush_dcache(int flush); >>> >>> #if defined CONFIG_MPC823 >>> #include <mpc823_lcd.h> >>> +ushort *configuration_get_cmap(void); >>> #elif defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \ >>> defined CONFIG_CPU_MONAHANS >>> #include <pxa_lcd.h> >>> +ushort *configuration_get_cmap(void); >>> #elif defined(CONFIG_ATMEL_LCD) || defined(CONFIG_ATMEL_HLCD) >>> #include <atmel_lcd.h> >>> +ushort *configuration_get_cmap(void); >>> #elif defined(CONFIG_EXYNOS_FB) >>> #include <exynos_lcd.h> >>> +ushort *configuration_get_cmap(void); >>> #else >>> typedef struct vidinfo { >>> ushort vl_col; /* Number of columns (i.e. 160) */ >>> @@ -60,6 +64,11 @@ typedef struct vidinfo { >>> >>> void *priv; /* Pointer to driver-specific data */ >>> } vidinfo_t; >>> + >>> +static inline ushort *configuration_get_cmap(void) >>> +{ >>> + return panel_info.cmap; >>> +} >>> #endif >> >> >> I'd argue for dropping the inline version and just having the same >> prototype in each case. > > > Are you suggesting something along the lines of: > > static ushort *configuration_get_cmap(void) > { > return panel_info.cmap; > } > #endif > > ushort *configuration_get_cmap(void); > > ? > > This is something I considered while preparing this, but I was under the > impression > that this isn't well defined by the standard. However, I actually have > confirmation > now that it is in fact defined behavior[1], so I will submit a V2 for this > (even though > I expect all of the above to disappear from lcd.h in the next refactor > series). > > [1] > http://stackoverflow.com/questions/28264874/non-static-function-prototype-follows-static-function-declaration OK, although if you are planning to get rid of this code in the next refactor, it's fine just as you have it in v1. Regards, Simon
diff --git a/common/lcd.c b/common/lcd.c index 1195a54..0f6c2e4 100644 --- a/common/lcd.c +++ b/common/lcd.c @@ -383,25 +383,6 @@ int lcd_getbgcolor(void) /************************************************************************/ /* ** Chipset depending Bitmap / Logo stuff... */ /************************************************************************/ -static inline ushort *configuration_get_cmap(void) -{ -#if defined CONFIG_CPU_PXA - struct pxafb_info *fbi = &panel_info.pxa; - return (ushort *)fbi->palette; -#elif defined(CONFIG_MPC823) - immap_t *immr = (immap_t *) CONFIG_SYS_IMMR; - cpm8xx_t *cp = &(immr->im_cpm); - return (ushort *)&(cp->lcd_cmap[255 * sizeof(ushort)]); -#elif defined(CONFIG_ATMEL_LCD) - return (ushort *)(panel_info.mmio + ATMEL_LCDC_LUT(0)); -#elif !defined(CONFIG_ATMEL_HLCD) && !defined(CONFIG_EXYNOS_FB) - return panel_info.cmap; -#elif defined(CONFIG_LCD_LOGO) - return bmp_logo_palette; -#else - return NULL; -#endif -} #ifdef CONFIG_LCD_LOGO void bitmap_plot(int x, int y) diff --git a/drivers/video/atmel_hlcdfb.c b/drivers/video/atmel_hlcdfb.c index 935ae42..0ce2370 100644 --- a/drivers/video/atmel_hlcdfb.c +++ b/drivers/video/atmel_hlcdfb.c @@ -13,6 +13,10 @@ #include <lcd.h> #include <atmel_hlcdc.h> +#if defined(CONFIG_LCD_LOGO) +#include <bmp_logo.h> +#endif + /* configurable parameters */ #define ATMEL_LCDC_CVAL_DEFAULT 0xc8 #define ATMEL_LCDC_DMA_BURST_LEN 8 @@ -37,6 +41,15 @@ void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue) panel_info.mmio + ATMEL_LCDC_LUT(regno)); } +ushort *configuration_get_cmap(void) +{ +#if defined(CONFIG_LCD_LOGO) + return bmp_logo_palette; +#else + return NULL; +#endif +} + void lcd_ctrl_init(void *lcdbase) { unsigned long value; diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c index 3cf008c..fa6a82c 100644 --- a/drivers/video/atmel_lcdfb.c +++ b/drivers/video/atmel_lcdfb.c @@ -29,6 +29,11 @@ #define lcdc_readl(mmio, reg) __raw_readl((mmio)+(reg)) #define lcdc_writel(mmio, reg, val) __raw_writel((val), (mmio)+(reg)) +ushort *configuration_get_cmap(void) +{ + return (ushort *)(panel_info.mmio + ATMEL_LCDC_LUT(0)); +} + void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue) { #if defined(CONFIG_ATMEL_LCD_BGR555) diff --git a/drivers/video/exynos_fb.c b/drivers/video/exynos_fb.c index be35b98..c5d7330 100644 --- a/drivers/video/exynos_fb.c +++ b/drivers/video/exynos_fb.c @@ -37,6 +37,15 @@ vidinfo_t panel_info = { }; #endif +ushort *configuration_get_cmap(void) +{ +#if defined(CONFIG_LCD_LOGO) + return bmp_logo_palette; +#else + return NULL; +#endif +} + static void exynos_lcd_init_mem(void *lcdbase, vidinfo_t *vid) { unsigned long palette_size; diff --git a/drivers/video/mpc8xx_lcd.c b/drivers/video/mpc8xx_lcd.c index add7215..9d2e5ed 100644 --- a/drivers/video/mpc8xx_lcd.c +++ b/drivers/video/mpc8xx_lcd.c @@ -357,6 +357,13 @@ lcd_setcolreg (ushort regno, ushort red, ushort green, ushort blue) /*----------------------------------------------------------------------*/ +ushort *configuration_get_cmap(void) +{ + immap_t *immr = (immap_t *)CONFIG_SYS_IMMR; + cpm8xx_t *cp = &(immr->im_cpm); + return (ushort *)&(cp->lcd_cmap[255 * sizeof(ushort)]); +} + void lcd_enable (void) { volatile immap_t *immr = (immap_t *) CONFIG_SYS_IMMR; diff --git a/drivers/video/pxa_lcd.c b/drivers/video/pxa_lcd.c index f66f615..04105d4 100644 --- a/drivers/video/pxa_lcd.c +++ b/drivers/video/pxa_lcd.c @@ -342,6 +342,12 @@ static int pxafb_init (vidinfo_t *vid); /* --------------- PXA chipset specific functions ------------------- */ /************************************************************************/ +ushort *configuration_get_cmap(void) +{ + struct pxafb_info *fbi = &panel_info.pxa; + return (ushort *)fbi->palette; +} + void lcd_ctrl_init (void *lcdbase) { pxafb_init_mem(lcdbase, &panel_info); diff --git a/include/lcd.h b/include/lcd.h index fbba6a2..838f645 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -42,13 +42,17 @@ void lcd_set_flush_dcache(int flush); #if defined CONFIG_MPC823 #include <mpc823_lcd.h> +ushort *configuration_get_cmap(void); #elif defined(CONFIG_CPU_PXA25X) || defined(CONFIG_CPU_PXA27X) || \ defined CONFIG_CPU_MONAHANS #include <pxa_lcd.h> +ushort *configuration_get_cmap(void); #elif defined(CONFIG_ATMEL_LCD) || defined(CONFIG_ATMEL_HLCD) #include <atmel_lcd.h> +ushort *configuration_get_cmap(void); #elif defined(CONFIG_EXYNOS_FB) #include <exynos_lcd.h> +ushort *configuration_get_cmap(void); #else typedef struct vidinfo { ushort vl_col; /* Number of columns (i.e. 160) */ @@ -60,6 +64,11 @@ typedef struct vidinfo { void *priv; /* Pointer to driver-specific data */ } vidinfo_t; + +static inline ushort *configuration_get_cmap(void) +{ + return panel_info.cmap; +} #endif extern vidinfo_t panel_info;
configuration_get_cmap() is multiple platform specific functions stuffed into one function. Split it into multiple versions, and move each version to the appropriate driver to reduce the #ifdef complexity. Signed-off-by: Nikita Kiryanov <nikita@compulab.co.il> Cc: Bo Shen <voice.shen@atmel.com> Cc: Simon Glass <sjg@chromium.org> Cc: Anatolij Gustschin <agust@denx.de> --- common/lcd.c | 19 ------------------- drivers/video/atmel_hlcdfb.c | 13 +++++++++++++ drivers/video/atmel_lcdfb.c | 5 +++++ drivers/video/exynos_fb.c | 9 +++++++++ drivers/video/mpc8xx_lcd.c | 7 +++++++ drivers/video/pxa_lcd.c | 6 ++++++ include/lcd.h | 9 +++++++++ 7 files changed, 49 insertions(+), 19 deletions(-)