[patch 0/1] next updated version, fixed cleanup and some minors

[Christian Krafft]

On Thu, 25 Jan 2007 01:24:01 +0100
Segher Boessenkool <segher at kernel.crashing.org> wrote:

> > 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.

Well, i left it in because there is no baggage except the few bytes in the match array.
This way the driver gets loaded if there is such a device.
It looks better to me to add generic support for these devices,
instead of adding code every time a specific device becomes available.
But actually I don't care too much. 
So if you have another argument than the few bytes baggage, I'll remove it.

> 
> > 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.

fixed

> 
> > +	if (regsize && proplen!=4) {
> 
> Whitespace problem (a few times in this file).

fixed

> 
> > +	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).

yep, I also feel sorry for that.

> 
> > +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.

fixed, 2.6.20 will contain the forward declaration, 
so the cleanup code can be called there.

> 
> > (void *)(unsigned long) SI_KCS
> 
> Yes I do hate enums.

Why ?

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

You just wanna confuse me, right ?

> 
> Cheers,
> 
> 
> Segher
> 


See my next mail for patch.

-- 
Mit freundlichen Grüssen,
kind regards,

Christian Krafft
IBM Systems & Technology Group, 
Linux Kernel Development
IT Specialist