On Mon, Mar 24, 2008 at 05:43:15PM -0400, Peter Jones wrote:
> This mail contains a patch which merges the IMAC mode code into the efifb
> driver and removes the imacfb driver entirely. There are also a couple of
> minor bug fixes. Any comments before I start bothering the upstream
> maintainers with it?
<snip>
> + switch (model) {
> + case M_I17:
> + width = 1440;
> + height = 900;
> + linelength = 1472 * 4;
> + base = 0x80010000;
> + break;
> + case M_I20:
> + width = 1680;
> + height = 1050;
> + linelength = 1728 * 4;
> + base = 0x80010000;
> + break;
> + case M_MINI:
> + width = 1024;
> + height = 768;
> + linelength = 2048 * 4;
> + base = 0x80000000;
> + break;
> + case M_MACBOOK:
> + width = 1280;
> + height = 800;
> + linelength = 2048 * 4;
> + base = 0x80000000;
> + break;
> + }
Oh wow. This is... ultragroady. Couldn't we do something slightly
cleaner, like instead of setting the dmi private data to a int flag, set
it to a pointer to a struct efifb_params or something? This has the
potential to become a switch-of-doom...
Might be nice to put the Intel Mac specific code in its own source file
too?
> +
> + if (!screen_info.lfb_depth)
> + screen_info.lfb_depth = 32;
> + if (!screen_info.pages)
> + screen_info.pages = 1;
> + if (base && !screen_info.lfb_base)
> + screen_info.lfb_base = base;
> +
> + if (manual_width)
> + width = manual_width;
> + if (width && !screen_info.lfb_width)
> + screen_info.lfb_width = width;
> + if (manual_height)
> + height = manual_height;
> + if (height && !screen_info.lfb_height)
> + screen_info.lfb_height = height;
> +
> + /* just assume they're all unset if any are */
> + if (!screen_info.blue_size) {
> + screen_info.blue_size = 8;
> + screen_info.blue_pos = 0;
> + screen_info.green_size = 8;
> + screen_info.green_pos = 8;
> + screen_info.red_size = 8;
> + screen_info.red_pos = 16;
> + screen_info.rsvd_size = 8;
> + screen_info.rsvd_pos = 24;
> + }
> +
> + if (linelength && !screen_info.lfb_linelength)
> + screen_info.lfb_linelength = linelength;
>
And possibly slurp this out of line into a seperate setup_fb_from_param
function or something?
It looks kind of groady for something that isn't the "common case"... I
hope.
Awesome patch though, always good to see more "-" than "+" :)
cheers, Kyle
_______________________________________________
Fedora-kernel-list mailing list
Fedora-kernel-list@redhat.com
https://www.redhat.com/mailman/listinfo/fedora-kernel-list
