Re: [XenPPC] [Patch] Find serial device and platform type

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

Hi Maria, you didn't include any description with your patch, so I'm not
sure if you intend it for submission or as a request for comments.

Stylistically, there are some typos, some renaming needed, some
formatting (e.g. "if (foo) bar" all on one line), // comments, lack of
punctuation in printf messages, etc. Also, the boot_of_serial() function
is waaay too long, with structs and unions defined in the middle of
it...

Also, it seems you haven't addressed any of the comments I made about
your earlier patch.

More generally, I'm not clear on why we need constants like
PMAC_HARDWARE at all (yes, I know Jimi suggested them). Why not
something like this:

----- setup.c -----
struct platform {
        char *compat;
        int (*init)(void);
} platforms = {
        { .compat = "Power Macintosh", .init = pmac_init },
        { .compat = "Momentum,Maple", .init = maple_init },

boot_of_platform() {
        for platform in platforms {
                if (compatible(platform.compat)
                        && platform.init() == SUCCESS)
                break;
        }
}

------ pmac.c -----
pmac_init() {
        zilog_register();
        return SUCCESS;
}

----- zilog.c -----
struct zilog_port {
        whatever;
} port;

zilog_register() {
        serial_register_uart(0, zilog_driver, &port)
}

-- 
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel