Messages in this thread | | | Subject | Re: new driver (hvcs) review request and sysfs questions | From | Dave Hansen <> | Date | Wed, 25 Feb 2004 08:22:33 -0800 |
| |
--- pristine/linux-2.5/drivers/char/hvcs.c ... +++ working/linux-2.5/drivers/char/hvcs.c ...
It's standard practice to create patches so that they apply with -p1. See Documentation/SubmittingPatches
Don't just indiscriminately run lindent. It makes the code look awful in some spots. Remember that it's OK to exceed 80 columns in some places, especially with things like function declarations.
in hvcs_write():
count = min(count, (int)PAGE_SIZE);
assumes that count and PAGE_SIZE are in the same units: bytes. If that's true, no need for the sizeof() here:
charbuf = kmalloc(sizeof(unsigned char *) * count, GFP_KERNEL);
hvcs_next_partner() shares code with lparcfg_write() and a bunch of the other hypervisor calls. Can you combine them with a function that just checks the return codes from hcalls? All of those switches look redundant.
hvcs_get_partner_info() is a bit long. Can that while loop go into its own function?
-- dave
- 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/
| |