Messages in this thread Patch in this message | | | From | Segher Boessenkool <> | Subject | Re: [PATCH] Open Firmware device tree virtual filesystem | Date | Mon, 1 Jan 2007 05:21:48 +0100 |
| |
Some comments, mostly coding style:
> - 0xb0 - 0x13f Free. Add more parameters here if you really need them. > + 0xb0 16 bytes Open Firmware information (magic, version, callback, > idt)
Is there an OF ISA binding for x86 somewhere? And don't point me to the source code, I'd like to see an actual reference doc ;-)
> +// printk(KERN_WARNING "CALLOFW: %s\n", name);
Please remove disabled code. And don't use // comments at all please.
> + if (call_firmware == NULL) > + return (-1);
No parentheses around return value.
> + argarray[0] = (int)name;
This would warn on 64-bit systems, better write it as (u32)(u64)name (and make sure that "name" is somewhere in the low 4GB of memory). This doesn't handle 64-bit client interface either btw.
> + while (numargs--) { > + argarray[argnum++] = va_arg(ap, int); > + }
No braces around single statements.
> +#undef MAXARGS
Why this #undef? That's nasty style, and this is at the end of file anyway.
> +#if 0
Better use a normal comment.
> +Here are call templates for all the standard OFW client services.
You missed "instance-to-interposed-path" (standard, although not required).
> + * By Mitch Bradley (wmb@firmworks.com), with assistance from David > Kahn. > + * Most of the basic virtual file system structure was taken from a > + * "promfs" example written by Arnd Bergmann. + *
My mailer messed up this line, that means you have trailing spaces here :-)
> + + if (root == 0) {
Same here.
> +++ b/include/asm-i386/callofw.h > @@ -0,0 +1,22 @@ > +#ifndef _I386_PROM_H > +#define _I386_PROM_H Better make the #define correspond to the file name.
Segher
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |