On Thu, 25 Jan 2007 01:24:01 +0100
Segher Boessenkool <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
> > Acked-by: Heiko J Schick <[EMAIL PROTECTED]>
>
> > #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
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Openipmi-developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
