Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
On Fri, 26 Jan 2007 05:23:02 +0100
Arnd Bergmann <[EMAIL PROTECTED]> wrote:
> > > +static int __devexit ipmi_of_remove(struct of_device *dev)
> > > +{
> > > + /* should call
> > > + * cleanup_one_si(dev->dev.driver_data); */
> >
> > Wo why doesn't it do that currently?
This has been fixed already by the updated version sent on 25/01/07, 14:34 EST.
--
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
On Friday 26 January 2007 03:54, Christoph Hellwig wrote:
> >
> > +#ifdef CONFIG_PPC_OF
> > +#include
> > +#include
> > +#endif
>
> Adding this OF-specific code to the generic file doesn't look exactly
> nice. Is it possible to separate the code out to a separate ipmi_of.c
> file?
This file already contains the same code for a number of other bus
layers, it might be possible to split out the of part, but then we
should also have separate files for ACPI, PCI and all the others.
I think that reworking the ipmi layer is outside of the scope of
what we want right now, that can be another patch if there is
a consensus that it would be a good idea.
> > +static int __devexit ipmi_of_remove(struct of_device *dev)
> > +{
> > + /* should call
> > + * cleanup_one_si(dev->dev.driver_data); */
>
> Wo why doesn't it do that currently?
This mimics the pci driver that also doesn't do it. The comment
is a hint that when it the ipmi driver gets cleaned up so
that _pci gets it right, _of should do the same. That should
also be a separate patch.
Arnd <><
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
On Thu, Jan 25, 2007 at 10:45:40AM +1100, Christian Krafft 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.
>
> Signed-off-by: Christian Krafft <[EMAIL PROTECTED]>
> Acked-by: Heiko J Schick <[EMAIL PROTECTED]>
>
> Index: linux/drivers/char/ipmi/ipmi_si_intf.c
> ===
> --- linux.orig/drivers/char/ipmi/ipmi_si_intf.c
> +++ linux/drivers/char/ipmi/ipmi_si_intf.c
> @@ -9,6 +9,7 @@
> * [EMAIL PROTECTED]
> *
> * Copyright 2002 MontaVista Software Inc.
> + * Copyright 2006 IBM Corp., Christian Krafft <[EMAIL PROTECTED]>
> *
> * This program is free software; you can redistribute it and/or modify it
> * under the terms of the GNU General Public License as published by the
> @@ -64,6 +65,11 @@
> #include
> #include
>
> +#ifdef CONFIG_PPC_OF
> +#include
> +#include
> +#endif
Adding this OF-specific code to the generic file doesn't look exactly
nice. Is it possible to separate the code out to a separate ipmi_of.c
file?
> +static int __devexit ipmi_of_remove(struct of_device *dev)
> +{
> + /* should call
> + * cleanup_one_si(dev->dev.driver_data); */
Wo why doesn't it do that currently? :-)
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
On Thursday 25 January 2007 01:45, Benjamin Herrenschmidt wrote: > If the underlying ipmi stuff cannot cleanup, then the driver should not > have a module_exit() so the module cannot be removed. It can do the cleanup, but it won't go through the driver's remove function currently, because it also tries to deal with implementations that don't have a 'struct device' abstraction and therefore all ipmi interfaces get torn down by the ipmi driver. Arnd <>< - 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 Openipmi-developer@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
+static int __devexit ipmi_of_remove(struct of_device *dev)
+{
+ /* should call
+ * cleanup_one_si(dev->dev.driver_data); */
+ return 0;
+}
>>>
>>> If your remove doesn't work, don't implement one.
>>
>> As explained before, it's the underlying thing that doesn't
>> work. Yeah someone should fix it one day. Still it's better
>> to have this comment than to not have anything at all. An
>> XXX FIXME: tag wouldn't be out of place of course.
>
> If the underlying ipmi stuff cannot cleanup, then the driver should
> not
> have a module_exit() so the module cannot be removed.
Well the "base" IPMI code has this same FIXME. Also, it seems
to be safe to just exit in this particular case )not a clean
way to go about things, of course).
Anyway, it sounds like Christian has a fix, let's see what he
comes up with :-)
Segher
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
On Thu, 2007-01-25 at 01:29 +0100, Segher Boessenkool wrote:
> >> +static int __devexit ipmi_of_remove(struct of_device *dev)
> >> +{
> >> + /* should call
> >> + * cleanup_one_si(dev->dev.driver_data); */
> >> + return 0;
> >> +}
> >
> > If your remove doesn't work, don't implement one.
>
> As explained before, it's the underlying thing that doesn't
> work. Yeah someone should fix it one day. Still it's better
> to have this comment than to not have anything at all. An
> XXX FIXME: tag wouldn't be out of place of course.
If the underlying ipmi stuff cannot cleanup, then the driver should not
have a module_exit() so the module cannot be removed.
Ben.
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
Hi,
I just recognized, that the forward declaration is in the tree, so I'll send an
updated version with the cleanup call.
Tschuss,
ck
On Thu, 25 Jan 2007 01:29:48 +0100
Segher Boessenkool <[EMAIL PROTECTED]> wrote:
> >> +static int __devexit ipmi_of_remove(struct of_device *dev)
> >> +{
> >> + /* should call
> >> + * cleanup_one_si(dev->dev.driver_data); */
> >> + return 0;
> >> +}
> >
> > If your remove doesn't work, don't implement one.
>
> As explained before, it's the underlying thing that doesn't
> work. Yeah someone should fix it one day. Still it's better
> to have this comment than to not have anything at all. An
> XXX FIXME: tag wouldn't be out of place of course.
>
> > Though since you don't
> > have the choice in having a module_exit or not, you should really
> > implement one that works :-)
>
> Genau.
>
>
> Segher
>
--
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
>> +static int __devexit ipmi_of_remove(struct of_device *dev)
>> +{
>> +/* should call
>> + * cleanup_one_si(dev->dev.driver_data); */
>> +return 0;
>> +}
>
> If your remove doesn't work, don't implement one.
As explained before, it's the underlying thing that doesn't
work. Yeah someone should fix it one day. Still it's better
to have this comment than to not have anything at all. An
XXX FIXME: tag wouldn't be out of place of course.
> Though since you don't
> have the choice in having a module_exit or not, you should really
> implement one that works :-)
Genau.
Segher
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
> 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 <[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.
> + 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
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
> +
> +static int __devexit ipmi_of_remove(struct of_device *dev)
> +{
> + /* should call
> + * cleanup_one_si(dev->dev.driver_data); */
> + return 0;
> +}
If your remove doesn't work, don't implement one. Though since you don't
have the choice in having a module_exit or not, you should really
implement one that works :-)
Ben.
-
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
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
Re: [Openipmi-developer] [patch 1/1] updated version, fixed the compiler warning
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.
Signed-off-by: Christian Krafft <[EMAIL PROTECTED]>
Acked-by: Heiko J Schick <[EMAIL PROTECTED]>
Index: linux/drivers/char/ipmi/ipmi_si_intf.c
===
--- linux.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux/drivers/char/ipmi/ipmi_si_intf.c
@@ -9,6 +9,7 @@
* [EMAIL PROTECTED]
*
* Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <[EMAIL PROTECTED]>
*
* This program is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License as published by the
@@ -64,6 +65,11 @@
#include
#include
+#ifdef CONFIG_PPC_OF
+#include
+#include
+#endif
+
#define PFX "ipmi_si: "
/* Measure times between events in the driver. */
@@ -1006,6 +1012,7 @@ static DEFINE_MUTEX(smi_infos_lock);
static int smi_num; /* Used to sequence the SMIs */
#define DEFAULT_REGSPACING 1
+#define DEFAULT_REGSIZEDEFAULT_REGSPACING
static int si_trydefaults = 1;
static char *si_type[SI_MAX_PARMS];
@@ -2174,6 +2181,100 @@ static struct pci_driver ipmi_pci_driver
#endif /* CONFIG_PCI */
+#ifdef CONFIG_PPC_OF
+static int __devinit ipmi_of_probe(struct of_device *dev,
+const struct of_device_id *match)
+{
+ struct smi_info *info;
+ struct resource resource;
+ const int *regsize, *regspacing, *regshift;
+ struct device_node *np = dev->node;
+ int ret;
+ int proplen;
+
+ dev_info(&dev->dev, PFX "probing via device tree\n");
+
+ ret = of_address_to_resource(np, 0, &resource);
+ if (ret) {
+ dev_warn(&dev->dev, PFX "invalid address from OF\n");
+ return ret;
+ }
+
+ regsize = get_property(np, "reg-size", &proplen);
+ if (regsize && proplen!=4) {
+ dev_warn(&dev->dev, PFX "invalid regsize from OF\n");
+ return -EINVAL;
+ }
+
+ regspacing = get_property(np, "reg-spacing", &proplen);
+ if (regspacing && proplen!=4) {
+ dev_warn(&dev->dev, PFX "invalid regspacing from OF\n");
+ return -EINVAL;
+ }
+
+ regshift = get_property(np, "reg-shift", &proplen);
+ if (regshift && proplen!=4) {
+ dev_warn(&dev->dev, PFX "invalid regshift from OF\n");
+ return -EINVAL;
+ }
+
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+
+ if (!info) {
+ dev_err(&dev->dev,
+ PFX "could not allocate memory for OF probe\n");
+ return -ENOMEM;
+ }
+
+ info->si_type = (enum si_type) match->data;
+ info->addr_source = "device-tree";
+ info->io_setup = mem_setup;
+ info->irq_setup = std_irq_setup;
+
+ info->io.addr_type = IPMI_MEM_ADDR_SPACE;
+ info->io.addr_data = resource.start;
+
+ info->io.regsize= regsize ? *regsize : DEFAULT_REGSIZE;
+ info->io.regspacing = regspacing ? *regspacing : DEFAULT_REGSPACING;
+ info->io.regshift = regshift ? *regshift : 0;
+
+ info->irq = irq_of_parse_and_map(dev->node, 0);
+ info->dev = &dev->dev;
+
+ dev_dbg(&dev->dev, "addr 0x%lx regsize %ld spacing %ld irq %x\n",
+ info->io.addr_data, info->io.regsize, info->io.regspacing,
+ info->irq);
+
+ dev->dev.driver_data = (void*) info;
+
+ return try_smi_init(info);
+}
+
+static int __devexit ipmi_of_remove(struct of_device *dev)
+{
+ /* should call
+* cleanup_one_si(dev->dev.driver_data); */
+ return 0;
+}
+
+static struct of_device_id ipmi_match[] =
+{
+ { .type = "ipmi", .compatible = "ipmi-kcs", .data = (void *)(unsigned
long) SI_KCS },
+ { .type = "ipmi", .compatible = "ipmi-smic", .data = (void *)(unsigned
long) SI_SMIC },
+ { .type = "ipmi", .compatible = "ipmi-bt", .data = (void *)(unsigned
long) SI_BT },
+ {},
+};
+
+static struct of_platform_driver ipmi_of_platform_driver =
+{
+ .name = "ipmi",
+ .match_table= ipmi_match,
+ .probe = ipmi_of_probe,
+ .remove = __devexit_p(ipmi_of_remove),
+};
+#endif /* CONFIG_PPC_OF */
+
+
static int try_get_dev_id(struct smi_info *smi_info)
{
unsigned char msg[2];
@@ -2798,6 +2899,10 @@ static __devinit int init_ipmi_si(void)
}
#endif
+#ifdef CONFIG_PPC_OF
+ of_register_platform_driver(&ipmi_of_platform_driver);
+#endif
+
if (si_trydefaults) {
mutex_lock(&smi_infos_l
