Hi Sergei, Thanks for reviewing the patch.
On Wed, Aug 08, 2012 at 21:44:55, Sergei Shtylyov wrote: > Hello. > > On 08-08-2012 19:35, Manjunathappa, Prakash wrote: > > > LCD controller on am335x supports 24bpp raster configuration in addition > > to ones on da850. LCDC also supports 24bpp in unpacked format having > > ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha > > component of the data. > > > Signed-off-by: Manjunathappa, Prakash <[email protected]> > > Cc: Anatolij Gustschin <[email protected]> > [...] > > > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c > > index 7ae9d53..1abcfa9 100644 > > --- a/drivers/video/da8xx-fb.c > > +++ b/drivers/video/da8xx-fb.c > [...] > > @@ -499,6 +501,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par > > *par, u32 width, u32 height, > > { > > u32 reg; > > > > + if ((bpp > 16) && (lcd_revision == LCD_VERSION_1)) > > Parens around operands of && not necessary. > I agree it is not necessary, isn't it more readable? > > @@ -542,6 +547,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par > > *par, u32 width, u32 height, > > reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8); > > if (raster_order) > > reg |= LCD_RASTER_ORDER; > > + > > + if (bpp == 24) > > + reg |= LCD_V2_TFT_24BPP_MODE; > > + else if (bpp == 32) > > + reg |= (LCD_V2_TFT_24BPP_MODE | LCD_V2_TFT_24BPP_UNPACK); > > + > > This asks to be a *switch* statement. > I will move it to *switch* statement below. > > lcdc_write(reg, LCD_RASTER_CTRL_REG); > > > > switch (bpp) { > > @@ -549,6 +560,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par > > *par, u32 width, u32 height, > > case 2: > > case 4: > > case 16: > > + case 24: > > + case 32: > > par->palette_sz = 16 * 2; > > break; > > > > @@ -578,13 +591,36 @@ static int fb_setcolreg(unsigned regno, unsigned red, > > unsigned green, > > if (info->fix.visual == FB_VISUAL_DIRECTCOLOR) > > return 1; > > > > - if (info->var.bits_per_pixel == 4) { > > - if (regno > 15) > > - return 1; > > + if ((info->var.bits_per_pixel > 16) && (lcd_revision == LCD_VERSION_1)) > > Parens around operands of && not necessary. > Same as above, can I retain it for purpose of readability? > > + switch (info->fix.visual) { > > + case FB_VISUAL_TRUECOLOR: > > + red = CNVT_TOHW(red, info->var.red.length); > > + green = CNVT_TOHW(green, info->var.green.length); > > + blue = CNVT_TOHW(blue, info->var.blue.length); > > + break; > > + case FB_VISUAL_PSEUDOCOLOR: > > + if (info->var.bits_per_pixel == 4) { > > + if (regno > 15) > > + return -EINVAL; > > + > > + if (info->var.grayscale) { > > + pal = regno; > > + } else { > > + red >>= 4; > > + green >>= 8; > > + blue >>= 12; > > + > > + pal = (red & 0x0f00); > > + pal |= (green & 0x00f0); > > + pal |= (blue & 0x000f); > > Parens not needed. > Will remove parens around. > > + } > > + if (regno == 0) > > + pal |= 0x2000; > > + palette[regno] = pal; > > + > > + } else if (info->var.bits_per_pixel == 8) { > > This asks to be a *switch* statement. > Agreed, I will replace with *switch* statement. Thanks, Prakash > > @@ -842,6 +877,9 @@ static int fb_check_var(struct fb_var_screeninfo *var, > > { > > int err = 0; [...] _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
