[patch 1/1] updated version, fixed the compiler warning

Segher Boessenkool segher at kernel.crashing.org
Thu Jan 25 11:24:01 EST 2007


> This patch adds support for of_platform_driver to the ipmi_si module.
> When loading the module, the driver will be registered to of_platform.
> The driver will be probed for all devices with the type ipmi. It's  
> supporting
> devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt.
> Only ipmi-kcs could be tested.

I'm still saying that because of this, and because they might
never be used and as such be unnecessary baggage, you shouldn't
add SMIC and BT support.

> Signed-off-by: Christian Krafft <krafft at de.ibm.com>
> Acked-by: Heiko J Schick <schihei at de.ibm.com>

>  #define DEFAULT_REGSPACING	1
> +#define DEFAULT_REGSIZE		DEFAULT_REGSPACING

Just #define it as 1 I'd say.  Esp. for KCS interfaces, it can't
ever be anything else there.

> +	if (regsize && proplen!=4) {

Whitespace problem (a few times in this file).

> +	info->si_type		= (enum si_type) match->data;

Do you need the cast here?  Oh I suppose you do, why else
did you add it (and it teaches the world as a whole once
again that enums in C are bloody useless almost always).

> +static int __devexit ipmi_of_remove(struct of_device *dev)
> +{
> +	/* should call
> +	 * cleanup_one_si(dev->dev.driver_data); */
> +	return 0;
> +}

While I know this isn't really your problem, no one who
isn't touching the IPMI code will ever fix it, so...  nudge
nudge, wink wink.

> (void *)(unsigned long) SI_KCS

Yes I do hate enums.

> +	.name		= "ipmi",

Shouldn't this name be "ipmi-kcs" etc.?  Just asking :-)

Cheers,


Segher




More information about the Linuxppc-dev mailing list