Re: [Linux-fbdev-devel] [PATCH] Frame Buffer driver for TI DA8xx/OMAP-L1xx

[Rajashekhara, S. <sud...@ti...>]

Hi,

On Fri, Jun 19, 2009 at 15:05:52, Krzysztof Helt wrote:
> HI,
> 
> This version of the driver is much cleaner however major issues still
remains.
> 
> 1. The driver seems not able to change graphics mode (depth or resolution)
> but check_var() and set_par() are defined. The set_par() function does not
> modify any register. You should move this few settings done there to
> lcd initialization functions and just remove the check_var() and
set_par().

Though I was able to get rid of set_par() function inside the driver,
removing
check_var() function turned out to be little tough as based on
"var.activate"
flags the fb_set_var function is performing lot of things which will not be
done
if check_var() function is NULL. Hence I have retained the check_var()
function
as of now.

> 
> 2. The driver does not set pseudo palette entries but uses true color mode
> (16-bit). See other driver show to do this (I advice tdfxfb.c).
> 

DA8XX/OMAP-L1xx LCD controller expects the first 2 bytes of palette to be
0x4000
for TRUECOLOR mode and I am doing this in fb_setcolreg. For further details,
please
refer to the controller spec which can be found at
http://www.ti.com/litv/pdf/sprufm0a. 

> 3. The request_mem_region() should be accompanied by the
release_mem_region()
> (I have not noticed it in my previous review).
> 
> 4. Check error paths in the probing functions. Not all already
requested/allocated
> resources are released correctly (e.g. the in one path there is no
clk_put() called).
> 
> Minor issues are in comments below.
> 
> If any of above issues is false let me know. I am not specialist in all
types of 
> embedded lcd controllers.
> 
> Kind regards,
> Krzysztof
> 

I have taken care of all other review comments and I'll be submitting the
version 3 of
this patch shortly.

Thanks for spending time to review the patches.

Regards, Sudhakar