Re: [PATCH] IPMI: driver model and sysfs support

[Yani Ioannou <yani.ioannou@gmail.com>]

On 8/30/05, Corey Minyard <minyard@acm.org> 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