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

[Christian Krafft <krafft@de.ibm.com>]

On Thu, 25 Jan 2007 01:24:01 +0100
Segher Boessenkool <segher@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 =20
> > supporting
> > devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt.
> > Only ipmi-kcs could be tested.
>=20
> 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.=20
So if you have another argument than the few bytes baggage, I'll remove it.

>=20
> > Signed-off-by: Christian Krafft <krafft@de.ibm.com>
> > Acked-by: Heiko J Schick <schihei@de.ibm.com>
>=20
> >  #define DEFAULT_REGSPACING	1
> > +#define DEFAULT_REGSIZE		DEFAULT_REGSPACING
>=20
> Just #define it as 1 I'd say.  Esp. for KCS interfaces, it can't
> ever be anything else there.

fixed

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

fixed

>=20
> > +	info->si_type		=3D (enum si_type) match->data;
>=20
> 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.

>=20
> > +static int __devexit ipmi_of_remove(struct of_device *dev)
> > +{
> > +	/* should call
> > +	 * cleanup_one_si(dev->dev.driver_data); */
> > +	return 0;
> > +}
>=20
> 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,=20
so the cleanup code can be called there.

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

Why ?

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

You just wanna confuse me, right ?

>=20
> Cheers,
>=20
>=20
> Segher
>=20


See my next mail for patch.

--=20
Mit freundlichen Gr=FCssen,
kind regards,

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