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 <prakash.pm@xxxxxx>
> > Cc: Anatolij Gustschin <agust@xxxxxxx>
> [...]
>
> > 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;
[...]
--
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
