On Wed, Jul 18, 2012 at 12:19:45, Manjunathappa, Prakash wrote: > Hi Vaibhav, > > On Wed, Jul 18, 2012 at 11:41:43, Hiremath, Vaibhav wrote: > > On Wed, Jul 18, 2012 at 11:17:21, 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 <prakash.pm@xxxxxx> > > > Cc: Anatolij Gustschin <agust@xxxxxxx> > > > --- > > > Since v1: > > > Addressed Tobias's review comments on calculation of pseudopalette for > > > FB_VISUAL_TRUECOLOR type. > > > > > > drivers/video/da8xx-fb.c | 88 ++++++++++++++++++++++++++++----------------- > > > 1 files changed, 55 insertions(+), 33 deletions(-) > > > > > > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c > > > index 47118c7..3cda461 100644 > > > --- a/drivers/video/da8xx-fb.c > > > +++ b/drivers/video/da8xx-fb.c > > > @@ -153,7 +153,7 @@ struct da8xx_fb_par { > > > unsigned int dma_end; > > > struct clk *lcdc_clk; > > > int irq; > > > - unsigned short pseudo_palette[16]; > > > + u32 pseudo_palette[16]; > > > > Personally I don't like, mix of "u32" and "unsigned int" declaration. > > > > "unsigned int" is not guaranteed to be 32bit, may be "unsigned long" is > better choice. > > > > unsigned int palette_sz; > > > unsigned int pxl_clk; > > > int blank; > > > @@ -546,6 +546,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height, > > > return 0; > > > } > > > > > > + > > > +#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16) > > > > Did you run checkpatch.pl on this patch? > > > > Yes, checkpatch.pl did not complain anything on this line. I will add space > around binary operators. > You can choose not to do this change, since checkpatch is ok and other drivers also does same thing. > > > static int fb_setcolreg(unsigned regno, unsigned red, unsigned green, > > > unsigned blue, unsigned transp, > > > struct fb_info *info) > > > @@ -561,13 +563,33 @@ 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; > > > + 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); > > > > Why not add another underscore after TO? define like this => CNVT_TO_HW > > > > I referred drivers/video/skeletonfb.c, I assume it is standard. Please let me know if not. I did grep on drivers/video/ and could see lots of drivers use it (almost same definition). Ignore this comment as well, I could have recommended to move it to common file and do some cleanup, but not sure about background on this macro. Thanks, Vaibhav > > > > + break; > > > + case FB_VISUAL_PSEUDOCOLOR: > > > + if (info->var.bits_per_pixel == 4) { > > > + if (regno > 15) > > > + return 1; > > > + > > > + if (info->var.grayscale) { > > > + pal = regno; > > > + } else { > > > + red >>= 4; > > > + green >>= 8; > > > + blue >>= 12; > > > + > > > + pal = (red & 0x0f00); > > > + pal |= (green & 0x00f0); > > > + pal |= (blue & 0x000f); > > > + } > > > + if (regno == 0) > > > + pal |= 0x2000; > > > + palette[regno] = pal; > > > > > > - if (info->var.grayscale) { > > > - pal = regno; > > > - } else { > > > + } else if (info->var.bits_per_pixel == 8) { > > > red >>= 4; > > > green >>= 8; > > > blue >>= 12; > > > @@ -575,36 +597,35 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green, > > > pal = (red & 0x0f00); > > > pal |= (green & 0x00f0); > > > pal |= (blue & 0x000f); > > > - } > > > - if (regno == 0) > > > - pal |= 0x2000; > > > - palette[regno] = pal; > > > - > > > - } else if (info->var.bits_per_pixel == 8) { > > > - red >>= 4; > > > - green >>= 8; > > > - blue >>= 12; > > > - > > > - pal = (red & 0x0f00); > > > - pal |= (green & 0x00f0); > > > - pal |= (blue & 0x000f); > > > > > > - if (palette[regno] != pal) { > > > - update_hw = 1; > > > - palette[regno] = pal; > > > + if (palette[regno] != pal) { > > > + update_hw = 1; > > > + palette[regno] = pal; > > > + } > > > } > > > - } else if ((info->var.bits_per_pixel == 16) && regno < 16) { > > > - red >>= (16 - info->var.red.length); > > > - red <<= info->var.red.offset; > > > + break; > > > + } > > > > > > - green >>= (16 - info->var.green.length); > > > - green <<= info->var.green.offset; > > > + /* Truecolor has hardware independent palette */ > > > + if (info->fix.visual == FB_VISUAL_TRUECOLOR) { > > > + u32 v; > > > > > > - blue >>= (16 - info->var.blue.length); > > > - blue <<= info->var.blue.offset; > > > + if (regno > 15) > > > + return -EINVAL; > > > > > > - par->pseudo_palette[regno] = red | green | blue; > > > + v = (red << info->var.red.offset) | > > > + (green << info->var.green.offset) | > > > + (blue << info->var.blue.offset); > > > > > > + switch (info->var.bits_per_pixel) { > > > + case 16: > > > + ((u16 *) (info->pseudo_palette))[regno] = v; > > > + break; > > > + case 24: > > > + case 32: > > > + ((u32 *) (info->pseudo_palette))[regno] = v; > > > + break; > > > + } > > > if (palette[0] != 0x4000) { > > > update_hw = 1; > > > palette[0] = 0x4000; > > > @@ -617,6 +638,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green, > > > > > > return 0; > > > } > > > +#undef CNVT_TOHW > > > > > > > Is this required? > > > > Not required really, again thought it as standard since 8 out of 10 drivers does it. > > Thanks, > Prakash > -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html