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
