Re: Boot Console on LCD (fbcon) second try...

On Mon, 01 Mar 2004 12:07:43 +0100
Pattrick Hueper <pattyh_at_gmx.net> wrote:

> Any Comments?
Ok, here goes the comments.

First of all, please use real tabs characters. It looks to me like you
use a broken editor with tab width of 4 characters; this is absolutely
wrong. Look at Documentation/CodingStyle.txt as a ultimate reference.

Next observation is that you must remove explicitly calling
mq1100_fb_init() since this function is automatically called by kernel
at module init time (no matter if it's statically or dynamically
linked). This is simply wrong, since it's marked as __init and goes away
as soon as the module gets unloaded. Also it removes the need for the
initialization lock.

Further, this change:

/* if we dont have a backlight module yet, we try to find it once again */
   if (!info->bm) {
       info->bm=backlight_module_get(backlight_module_find(info->bm_name));
   }

added to the LCD notifier routine is partially correct, but not quite. It will
work if backlight module is registered *before* LCD module, but won't work if
the backlight module is registered after that. Thus I'll modify a little the
logic so that backlight module is queried every time it is needed (this
happens very seldom), if it has not been already found.

Moving debug_func() to the beginning of function is also wrong, as
debug_func() is basically a wrapper around printk(), and calling printk before
declaring variables is a syntax error in C.

Further, the driver_register variable and associated logic should be removed.
The mq1100fb_init function should be called only by the kernel and never
automatically, and kernel tracks itself this issue.

Please fix these issues and re-submit the patch. The core logic of it looks
okay to me.

--
Greetings,
   Andrew