On 8/30/05, Corey Minyard <[EMAIL PROTECTED]> wrote: > This is very good. I believe the structure is correct, but I'm not a > sysfs expert. > > There are a few things we need to deal with, though. > > * There are some significant changes to versioning in the > driver that are in the mm tree right now (you can pull them from > > http://www.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.13-rc6/2.6.13-rc6-mm2/broken-out)
Gah, ok :), I'll try to apply with the -mm changes. > * There are some coding style problems. You have places like: > +static void ipmi_bmc_unregister(struct ipmi_si_device *si, > + struct ipmi_bmc_device *bmc){ > The '{' for functions and structures needs to be on it's own line. > Also, several: > + if(bmc->guid_present){ > that need spaces after the 'if' and before the '{'. > The patch also adds some trailing spaces to empty lines, the > following is a telltale sign: > - > + > There was also one place where you added unneeded braces to > a single statement and another where you deleted the empty line > between two functions. > These are all standard kernel coding style rules. Yes, I should have caught those, thanks. > * I'd prefer to store the product id, device id, and manufacturer id > decoded. This makes it easier to handle (no need to use "memcmp" > to compare) and print. The printing of the product id, for instance, > will be rather unnatural in product_id_show(). I was under the impression that they were just OEM strings, and not necessarily ASCII strings? I'll have a look at the specs again. > * guids are not printable strings (see guid_show()). guid_show, sounds useful! Thanks, Yani - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/