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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |