Message ID | 1422963152-17634-3-git-send-email-nikita@compulab.co.il |
---|---|
State | Accepted |
Delegated to: | Anatolij Gustschin |
Headers | show |
Hi Nikita Kiryanov, On 02/03/2015 07:32 PM, Nikita Kiryanov 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> > Reviewed-by: Simon Glass <sjg@chromium.org> > Cc: Bo Shen <voice.shen@atmel.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Anatolij Gustschin <agust@denx.de> > --- > Changes in V2: > - Minor commit message update > - ushort *configuration_get_cmap(void) prototype is defined only once > instead of for each #ifdef CONFIG_* > > 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 | 7 +++++++ > 7 files changed, 47 insertions(+), 19 deletions(-) > > 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 I think we can include this file directly. > /* 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 > +} Here, I think no need to do the CONFIG_LCD_LOGO check, return bmp_logo_palette directly, as it will be used when use bmp command in 8 bit mode. However, no hardware to test LCDC work or not in 8 bit mode :( > 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..4ada617 100644 > --- a/include/lcd.h > +++ b/include/lcd.h > @@ -60,8 +60,15 @@ typedef struct vidinfo { > > void *priv; /* Pointer to driver-specific data */ > } vidinfo_t; > + > +static __maybe_unused ushort *configuration_get_cmap(void) > +{ > + return panel_info.cmap; > +} > #endif > > +ushort *configuration_get_cmap(void); > + > extern vidinfo_t panel_info; > > /* Video functions */ > Best Regards, Bo Shen
Hi Bo, On 02/04/2015 09:25 AM, Bo Shen wrote: > Hi Nikita Kiryanov, > > On 02/03/2015 07:32 PM, Nikita Kiryanov wrote: [..] >> #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 > > I think we can include this file directly. bmp_logo.h does not exist without CONFIG_LCD_LOGO. > >> /* 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 >> +} > > Here, I think no need to do the CONFIG_LCD_LOGO check, return bmp_logo_palette directly, as it will be used when use bmp command in 8 bit mode. However, no hardware to test LCDC work or not in 8 bit > mode :( This would make CONFIG_LCD_LOGO a prerequisite for this driver (bmp_logo_palette does not exist otherwise), and I don't think it's right to couple the two. > >> 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)) >>
Hi Nikita Kiryanov, On 02/08/2015 07:35 PM, Nikita Kiryanov wrote: > Hi Bo, > > On 02/04/2015 09:25 AM, Bo Shen wrote: >> Hi Nikita Kiryanov, >> >> On 02/03/2015 07:32 PM, Nikita Kiryanov wrote: > > [..] > >>> #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 >> >> I think we can include this file directly. > > bmp_logo.h does not exist without CONFIG_LCD_LOGO. Oh yes, my bad. >> >>> /* 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 >>> +} >> >> Here, I think no need to do the CONFIG_LCD_LOGO check, return >> bmp_logo_palette directly, as it will be used when use bmp command in >> 8 bit mode. However, no hardware to test LCDC work or not in 8 bit >> mode :( > > This would make CONFIG_LCD_LOGO a prerequisite for this driver > (bmp_logo_palette does not exist otherwise), and I don't think it's > right to couple the two. OK. let's keep this as is. >> >>> 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)) >>> > Best Regards, Bo Shen
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..4ada617 100644 --- a/include/lcd.h +++ b/include/lcd.h @@ -60,8 +60,15 @@ typedef struct vidinfo { void *priv; /* Pointer to driver-specific data */ } vidinfo_t; + +static __maybe_unused ushort *configuration_get_cmap(void) +{ + return panel_info.cmap; +} #endif +ushort *configuration_get_cmap(void); + extern vidinfo_t panel_info; /* Video functions */