Matthias Weisser wrote:
Anatolij Gustschin agust@denx.de schrieb am 01.07.2009 um 15:40:
Dear Matthias,
Thanks for this patch! Please see some comments/issues below which
we
should resolve before committing the driver.
First of all thanks for your detailed comments. I fix them all before resending the patch. Please see some additional comments at some points.
if(0 == bpp){if (bpp == 0) {
The spaces are OK but why turn around the comparison? I always use this style to prevent from unwanted assignments. Well, GCC warns about assignments in ifs and as u-boot is GCC only I can do that.
it is a matter of personal preference. "if (0 == bpp) {" is OK for me, too.
We also should use macros for register offsets, I think. Can we coordinate efforts for fixing this in mb862xx driver also?
As the graphic controller in the jade soc is only a slight evolution of the coral graphic chip (additional display output/video input) I think
we may even merge the two drivers into one with some #ifdefs to deal with the differences.
When I started to implement the driver I decided against that idea because I don't have any hardware here to test the "non-jade" case.
if you want to use the existing driver, it shouldn't be too complicated. To use the mb862xx driver you can implement "board_video_init()" in your board code in which you initialize both display controllers, set "GraphicDevice mb862xx" structure fields winSizeX, etc. and return the video RAM base. Then additionally implement "board_get_regs()" in your board code, e.g.
#include <mb862xx.h>
static const gdc_regs init_regs [] = { {0, 0} }
const gdc_regs *board_get_regs (void) { return init_regs; }
and define CONFIG_VIDEO_MB862xx in the board config file.
see e.g. the appropriate code in "board/lwmon5/lwmon5.c". It does the display controller initialization differently and therefore my suggestion to do the display controller init in custom "board_video_init()".
We have to fix some register access macros in the mb862xx driver for JADE and it should work. I can try to provide a patch for this fix in mb862xx.c.
But a common include file with the register offsets seems to be a good idea to me.
I'will prepare a list of register offset defines for "include/mb862xx.h" and send a patch so that you can use them in your code.
Thanks, Anatolij