Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Christian Krafft]

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 supporti=
ng
devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt.
Only ipmi-kcs could be tested.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>


Index: linux-2.6.19/drivers/char/ipmi/ipmi_si_intf.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- linux-2.6.19.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.19/drivers/char/ipmi/ipmi_si_intf.c
@@ -9,6 +9,7 @@
  *         source@mvista.com
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <krafft@de.ibm.com>
  *
  *  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,12 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
=20
+/* only exists on powerpc */
+#ifdef CONFIG_PPC_OF
+#include <asm/of_device.h>
+#include <asm/of_platform.h>
+#endif
+
 #define PFX "ipmi_si: "
=20
 /* Measure times between events in the driver. */
@@ -1888,6 +1895,96 @@ static struct pci_driver ipmi_pci_driver
 #endif /* CONFIG_PCI */
=20
=20
+#ifdef CONFIG_PPC_OF
+static int 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 =3D dev->node;
+	int ret;
+
+	dev_info(&dev->dev, PFX "probing via device tree\n");
+
+	ret =3D of_address_to_resource(np, 0, &resource);
+	if (ret) {
+		dev_warn(&dev->dev, PFX "invalid address from OF\n");
+		return ret;
+	}
+
+	regsize =3D get_property(np, "reg-size", NULL);
+	regspacing =3D get_property(np, "reg-spacing", NULL);
+	regshift =3D get_property(np, "reg-shift", NULL);
+
+	info =3D 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		=3D (enum si_type) match->data;
+	info->addr_source	=3D "device-tree";
+	info->io_setup		=3D mem_setup;
+	info->irq_setup		=3D std_irq_setup;
+
+	info->io.addr_type	=3D IPMI_MEM_ADDR_SPACE;
+	info->io.addr_data	=3D resource.start;
+
+	if (!regsize)
+		info->io.regsize =3D DEFAULT_REGSPACING;
+	else
+		info->io.regsize =3D *regsize;
+
+	if (!regspacing)
+		info->io.regspacing =3D DEFAULT_REGSPACING;
+	else
+		info->io.regspacing =3D *regspacing;
+
+	if (!regshift)
+		info->io.regshift =3D 0;
+	else
+		info->io.regshift =3D *regshift;
+
+	info->irq		=3D irq_of_parse_and_map(dev->node, 0);
+	info->dev		=3D &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 =3D (void*) info;
+
+	return try_smi_init(info);
+}
+
+static int 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[] =3D
+{
+	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D (void*)SI_KCS =
},
+	{ .type =3D "ipmi", .compatible =3D "ipmi-smic", .data =3D (void*)SI_SMIC=
 },
+	{ .type =3D "ipmi", .compatible =3D "ipmi-bt",   .data =3D (void*)SI_BT },
+	{},
+};
+
+static struct of_platform_driver ipmi_of_platform_driver =3D
+{
+	.name		=3D "ipmi",
+	.match_table	=3D ipmi_match,
+	.probe		=3D ipmi_of_probe,
+	.remove		=3D __devexit_p(ipmi_of_remove),
+};
+#endif /* CONFIG_PPC_OF */
+
+
 static int try_get_dev_id(struct smi_info *smi_info)
 {
 	unsigned char         msg[2];
@@ -2490,6 +2587,10 @@ static __devinit int init_ipmi_si(void)
 	pci_module_init(&ipmi_pci_driver);
 #endif
=20
+#ifdef CONFIG_PPC_OF
+	of_register_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {
@@ -2587,6 +2688,10 @@ static __exit void cleanup_ipmi_si(void)
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
=20
+#ifdef CONFIG_PPC_OF
+	of_unregister_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	mutex_lock(&smi_infos_lock);
 	list_for_each_entry_safe(e, tmp_e, &smi_infos, link)
 		cleanup_one_si(e);



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

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

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Segher Boessenkool]

> 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'd only include that one then.  But the code is trivial,
the risk is minimal, why not.


> +/* only exists on powerpc */
> +#ifdef CONFIG_PPC_OF
> +#include <asm/of_device.h>
> +#include <asm/of_platform.h>
> +#endif

Comment is inexact -- just kill it.

> +static int ipmi_of_probe(struct of_device *dev,
> +			 const struct of_device_id *match)

Shouldn't this (and everything else) be some kind of __init?

> +	regsize = get_property(np, "reg-size", NULL);
> +	regspacing = get_property(np, "reg-spacing", NULL);
> +	regshift = get_property(np, "reg-shift", NULL);

You should check whether you get exactly 4 bytes back or not.

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

That lands you a compiler warning (cast from pointer to shorter
integer) on 64-bit.

> +	if (!regsize)
> +		info->io.regsize = DEFAULT_REGSPACING;
> +	else
> +		info->io.regsize = *regsize;

info->io_regsize = regsize ? *regsize : DEFAULT_REGSIZE;

[Please note that fixes a copy/paste bug, too].

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

Please all addresses/sizes/spacings in hexadecimal?  And
you might want to output regshift, too.

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

So fix that :-)

> +	return 0;
> +}

> +static struct of_device_id ipmi_match[] =
> +{
> +	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data = (void*) 
> SI_KCS },
> +	{ .type = "ipmi", .compatible = "ipmi-smic", .data = (void*) 
> SI_SMIC },
> +	{ .type = "ipmi", .compatible = "ipmi-bt",   .data = (void*)SI_BT },

That cast-to-pointer is what gives you that warning when
casting back.  Is there no better solution?


All pretty minor, but please fix.  Looks like you're almost
there :-)


Segher

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Arnd Bergmann]

On Monday 18 December 2006 22:52, Segher Boessenkool wrote:

> > +static int ipmi_of_probe(struct of_device *dev,
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 const =
struct of_device_id *match)
>=20
> Shouldn't this (and everything else) be some kind of __init?

It should be __devinit.

> > +=A0=A0=A0=A0=A0regsize =3D get_property(np, "reg-size", NULL);
> > +=A0=A0=A0=A0=A0regspacing =3D get_property(np, "reg-spacing", NULL);
> > +=A0=A0=A0=A0=A0regshift =3D get_property(np, "reg-shift", NULL);
>=20
> You should check whether you get exactly 4 bytes back or not.
>
> > +=A0=A0=A0=A0=A0if (!regsize)
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0info->io.regsize =3D DEFAULT_RE=
GSPACING;
> > +=A0=A0=A0=A0=A0else
> > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0info->io.regsize =3D *regsize;
>=20
> info->io_regsize =3D regsize ? *regsize : DEFAULT_REGSIZE;
>=20
> [Please note that fixes a copy/paste bug, too].

These two changes are contradictory. It's either

    regsize =3D get_property(np, "reg-size", NULL);
    info->io_regsize =3D regsize ? *regsize : DEFAULT_REGSIZE;

or

    regsize =3D get_property(np, "reg-size", &proplen);
    info->io_regsize =3D (proplen =3D=3D 4) ? *regsize : DEFAULT_REGSIZE;

> > +static int ipmi_of_remove(struct of_device *dev)
> > +{
> > +=A0=A0=A0=A0=A0/* should call
> > +=A0=A0=A0=A0=A0 * cleanup_one_si(dev->dev.driver_data); */
>=20
> So fix that :-)

That should be a separate patch that fixes the same thing for
pci ipmi devices as well. It needs to move code around for this
(or introduce forward declarations), and the effect is no
different, so it's really just a cleanup.

> > +static struct of_device_id ipmi_match[] =3D
> > +{
> > +=A0=A0=A0=A0=A0{ .type =3D "ipmi", .compatible =3D "ipmi-kcs", =A0.dat=
a =3D (void*)=20
> > SI_KCS },
> > +=A0=A0=A0=A0=A0{ .type =3D "ipmi", .compatible =3D "ipmi-smic", .data =
=3D (void*)=20
> > SI_SMIC },
> > +=A0=A0=A0=A0=A0{ .type =3D "ipmi", .compatible =3D "ipmi-bt", =A0 .dat=
a =3D (void*)SI_BT },
>=20
> That cast-to-pointer is what gives you that warning when
> casting back. =A0Is there no better solution?

The one alternative might be something more complicated like:

static const enum si_type __devinitdata of_ipmi_dev_info[] =3D {
	[SI_KCS] SI_KCS,
	[SI_SMIC] SI_SMIC,
	[SI_BT] SI_BT,
};

static const struct of_device_id of_ipmi_match[] =3D {
	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D &of_ipmi_dev_in=
fo[SI_KCS] },
	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D &of_ipmi_dev_in=
fo[SI_SMIC] },
	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D &of_ipmi_dev_in=
fo[SI_BT] },
};

Not sure if that's worth it.

	Arnd <><

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Segher Boessenkool]

>>> +     regsize = get_property(np, "reg-size", NULL);
>>> +     regspacing = get_property(np, "reg-spacing", NULL);
>>> +     regshift = get_property(np, "reg-shift", NULL);
>>
>> You should check whether you get exactly 4 bytes back or not.
>>
>>> +     if (!regsize)
>>> +             info->io.regsize = DEFAULT_REGSPACING;
>>> +     else
>>> +             info->io.regsize = *regsize;
>>
>> info->io_regsize = regsize ? *regsize : DEFAULT_REGSIZE;
>>
>> [Please note that fixes a copy/paste bug, too].
>
> These two changes are contradictory.

No they're not.  If you don't get exactly 4 bytes back, you have
to fail the device, since you don't know how to handle it.  If
on the other hand you don't have the property at all, you use
the default value.

>>> +static int ipmi_of_remove(struct of_device *dev)
>>> +{
>>> +     /* should call
>>> +      * cleanup_one_si(dev->dev.driver_data); */
>>
>> So fix that :-)
>
> That should be a separate patch that fixes the same thing for
> pci ipmi devices as well. It needs to move code around for this
> (or introduce forward declarations), and the effect is no
> different, so it's really just a cleanup.

Oh, this is broken in the existing stuff as well?  Never mind then.

>>> +static struct of_device_id ipmi_match[] =
>>> +{
>>> +     { .type = "ipmi", .compatible = "ipmi-kcs",  .data = (void*)
>>> SI_KCS },
>>> +     { .type = "ipmi", .compatible = "ipmi-smic", .data = (void*)
>>> SI_SMIC },
>>> +     { .type = "ipmi", .compatible = "ipmi-bt",   .data = (void*) 
>>> SI_BT },
>>
>> That cast-to-pointer is what gives you that warning when
>> casting back.  Is there no better solution?
>
> The one alternative might be something more complicated like:
>
> static const enum si_type __devinitdata of_ipmi_dev_info[] = {
> 	[SI_KCS] SI_KCS,
> 	[SI_SMIC] SI_SMIC,
> 	[SI_BT] SI_BT,
> };
>
> static const struct of_device_id of_ipmi_match[] = {
> 	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data =  
> &of_ipmi_dev_info[SI_KCS] },
> 	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data =  
> &of_ipmi_dev_info[SI_SMIC] },
> 	{ .type = "ipmi", .compatible = "ipmi-kcs",  .data =  
> &of_ipmi_dev_info[SI_BT] },
> };
>
> Not sure if that's worth it.

Yeah, I don't think so either.  Maybe we should have some macro's
though that hide the casts (and make them right!), this stuff is
(ab)used a lot in Linux (and everyone gets it wrong always).

Any way around, the cast in this driver needs fixing.


Segher

Re: [Openipmi-developer] [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Corey Minyard]

Segher Boessenkool wrote:
>>>> +static int ipmi_of_remove(struct of_device *dev)
>>>> +{
>>>> +     /* should call
>>>> +      * cleanup_one_si(dev->dev.driver_data); */
>>>>         
>>> So fix that :-)
>>>       
>> That should be a separate patch that fixes the same thing for
>> pci ipmi devices as well. It needs to move code around for this
>> (or introduce forward declarations), and the effect is no
>> different, so it's really just a cleanup.
>>     
>
> Oh, this is broken in the existing stuff as well?  Never mind then.
>   
The forward declaration (and changes required for this to work) is in 
Linus' current tree, but was not released in 2.6.19.  So a later patch 
should be fine.

-Corey

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Christian Krafft]

On Mon, 18 Dec 2006 22:52:07 +0100
Segher Boessenkool <segher@kernel.crashing.org> wrote:


>=20
> info->io_regsize =3D regsize ? *regsize : DEFAULT_REGSIZE;
>=20
> [Please note that fixes a copy/paste bug, too].

There is no DEFAULT_REGSIZE, all the code is using DEFAULT_REGSPACING as th=
e default size.
It looks like the code assumes that the registers are located next to each =
other.
If thats not good, DEFAULT_REGSIZE should be introduced and used in all oth=
er probe functions as well.
That would be a seperate issue .

>=20
>=20
> Segher
>=20


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

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

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Segher Boessenkool]

>> info->io_regsize = regsize ? *regsize : DEFAULT_REGSIZE;
>>
>> [Please note that fixes a copy/paste bug, too].
>
> There is no DEFAULT_REGSIZE, all the code is using  
> DEFAULT_REGSPACING as the default size.
> It looks like the code assumes that the registers are located next  
> to each other.

It would be more logical to only use REGSIZE then, heh.

> If thats not good, DEFAULT_REGSIZE should be introduced and used in  
> all other probe functions as well.
> That would be a seperate issue .

You could start the cleanup by doing

#define DEFAULT_REGSIZE DEFAULT_REGSPACING

and using REGSIZE in the new code.  Or replace s/REGSPACING/REGSIZE/
throughout.  Or something.

Not your fault though, just leave it as-is if you don't feel
like fixing others' mess :-)


Segher

Re: [patch 0/1] ipmi: update: add autosensing of ipmi devices on powerpc using of device tree

[Corey Minyard]

Segher Boessenkool wrote:
>>> info->io_regsize = regsize ? *regsize : DEFAULT_REGSIZE;
>>>
>>> [Please note that fixes a copy/paste bug, too].
>>
>> There is no DEFAULT_REGSIZE, all the code is using DEFAULT_REGSPACING 
>> as the default size.
>> It looks like the code assumes that the registers are located next to 
>> each other.
>
> It would be more logical to only use REGSIZE then, heh.
>
>> If thats not good, DEFAULT_REGSIZE should be introduced and used in 
>> all other probe functions as well.
>> That would be a seperate issue .
>
> You could start the cleanup by doing
>
> #define DEFAULT_REGSIZE DEFAULT_REGSPACING
>
> and using REGSIZE in the new code.  Or replace s/REGSPACING/REGSIZE/
> throughout.  Or something.
>
> Not your fault though, just leave it as-is if you don't feel
> like fixing others' mess :-)
Doing this is fine with me, it needs to be done.

-Corey

Re: [patch 0/1] updated version

[Christian Krafft]

I'm about to send an updated version,

I added=20
length check for the properties,
DEFAULT_REGSIZE,
simplified the comparison,

I did not fix the compiler warning.


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

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

Re: [patch 0/1] updated version

[Arnd Bergmann]

On Wednesday 20 December 2006 15:45, Christian Krafft wrote:
> I'm about to send an updated version,
> 
> I added 
> length check for the properties,
> DEFAULT_REGSIZE,
> simplified the comparison,
> 
> I did not fix the compiler warning.
> 

You should really fix the warning before sending the patch, ideally
you also check that you don't introduce new warnings found by 'sparse'.

Is the warning just about the cast from void* to enum? If so, you
need to replace that with a cast to unsigned long, like

enum { A, B, } e;
void *p;

p = (void *)(unsigned long)A;
e = (unsigned long)p;

	Arnd <><

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

[Christian Krafft]

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 supporti=
ng
devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt.
Only ipmi-kcs could be tested.

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

Index: linux/drivers/char/ipmi/ipmi_si_intf.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- linux.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux/drivers/char/ipmi/ipmi_si_intf.c
@@ -9,6 +9,7 @@
  *         source@mvista.com
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <krafft@de.ibm.com>
  *
  *  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 <linux/string.h>
 #include <linux/ctype.h>
=20
+#ifdef CONFIG_PPC_OF
+#include <asm/of_device.h>
+#include <asm/of_platform.h>
+#endif
+
 #define PFX "ipmi_si: "
=20
 /* 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 */
=20
 #define DEFAULT_REGSPACING	1
+#define DEFAULT_REGSIZE		DEFAULT_REGSPACING
=20
 static int           si_trydefaults =3D 1;
 static char          *si_type[SI_MAX_PARMS];
@@ -2174,6 +2181,100 @@ static struct pci_driver ipmi_pci_driver
 #endif /* CONFIG_PCI */
=20
=20
+#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 =3D dev->node;
+	int ret;
+	int proplen;
+
+	dev_info(&dev->dev, PFX "probing via device tree\n");
+
+	ret =3D of_address_to_resource(np, 0, &resource);
+	if (ret) {
+		dev_warn(&dev->dev, PFX "invalid address from OF\n");
+		return ret;
+	}
+
+	regsize =3D get_property(np, "reg-size", &proplen);
+	if (regsize && proplen!=3D4) {
+		dev_warn(&dev->dev, PFX "invalid regsize from OF\n");
+		return -EINVAL;
+	}
+
+	regspacing =3D get_property(np, "reg-spacing", &proplen);
+	if (regspacing && proplen!=3D4) {
+		dev_warn(&dev->dev, PFX "invalid regspacing from OF\n");
+		return -EINVAL;
+	}
+
+	regshift =3D get_property(np, "reg-shift", &proplen);
+	if (regshift && proplen!=3D4) {
+		dev_warn(&dev->dev, PFX "invalid regshift from OF\n");
+		return -EINVAL;
+	}
+
+	info =3D 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		=3D (enum si_type) match->data;
+	info->addr_source	=3D "device-tree";
+	info->io_setup		=3D mem_setup;
+	info->irq_setup		=3D std_irq_setup;
+
+	info->io.addr_type	=3D IPMI_MEM_ADDR_SPACE;
+	info->io.addr_data	=3D resource.start;
+
+	info->io.regsize	=3D regsize ? *regsize : DEFAULT_REGSIZE;
+	info->io.regspacing	=3D regspacing ? *regspacing : DEFAULT_REGSPACING;
+	info->io.regshift	=3D regshift ? *regshift : 0;
+
+	info->irq		=3D irq_of_parse_and_map(dev->node, 0);
+	info->dev		=3D &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 =3D (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[] =3D
+{
+	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D (void *)(unsig=
ned long) SI_KCS },
+	{ .type =3D "ipmi", .compatible =3D "ipmi-smic", .data =3D (void *)(unsig=
ned long) SI_SMIC },
+	{ .type =3D "ipmi", .compatible =3D "ipmi-bt",   .data =3D (void *)(unsig=
ned long) SI_BT },
+	{},
+};
+
+static struct of_platform_driver ipmi_of_platform_driver =3D
+{
+	.name		=3D "ipmi",
+	.match_table	=3D ipmi_match,
+	.probe		=3D ipmi_of_probe,
+	.remove		=3D __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
=20
+#ifdef CONFIG_PPC_OF
+	of_register_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {
@@ -2895,6 +3000,10 @@ static __exit void cleanup_ipmi_si(void)
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
=20
+#ifdef CONFIG_PPC_OF
+	of_unregister_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	mutex_lock(&smi_infos_lock);
 	list_for_each_entry_safe(e, tmp_e, &smi_infos, link)
 		cleanup_one_si(e);

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

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

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

[Benjamin Herrenschmidt]

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

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

[Segher Boessenkool]

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

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

[Christian Krafft]

Hi,

I just recognized, that the forward declaration is in the tree, so I'll sen=
d an updated version with the cleanup call.

Tschuss,
ck

On Thu, 25 Jan 2007 01:29:48 +0100
Segher Boessenkool <segher@kernel.crashing.org> 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.
>=20
> 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.
>=20
> > Though since you don't
> > have the choice in having a module_exit or not, you should really
> > implement one that works :-)
>=20
> Genau.
>=20
>=20
> Segher
>=20


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

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

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

[Benjamin Herrenschmidt]

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.

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

[Segher Boessenkool]

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

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

[Arnd Bergmann]

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

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

[Segher Boessenkool]

> 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@de.ibm.com>
> Acked-by: Heiko J Schick <schihei@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

Re: [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@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

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

[Christian Krafft]

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 supporti=
ng
devices with compatible settings ipmi-kcs, ipmi-smic and ipmi-bt.
Only ipmi-kcs could be tested.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>
Acked-by: Heiko J Schick <schihei@de.ibm.com>
Signed-off-by: Corey Minyard <minyard@acm.org>

Index: linux/drivers/char/ipmi/ipmi_si_intf.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- linux.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux/drivers/char/ipmi/ipmi_si_intf.c
@@ -9,6 +9,7 @@
  *         source@mvista.com
  *
  * Copyright 2002 MontaVista Software Inc.
+ * Copyright 2006 IBM Corp., Christian Krafft <krafft@de.ibm.com>
  *
  *  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 <linux/string.h>
 #include <linux/ctype.h>
=20
+#ifdef CONFIG_PPC_OF
+#include <asm/of_device.h>
+#include <asm/of_platform.h>
+#endif
+
 #define PFX "ipmi_si: "
=20
 /* 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 */
=20
 #define DEFAULT_REGSPACING	1
+#define DEFAULT_REGSIZE		1
=20
 static int           si_trydefaults =3D 1;
 static char          *si_type[SI_MAX_PARMS];
@@ -2174,6 +2181,99 @@ static struct pci_driver ipmi_pci_driver
 #endif /* CONFIG_PCI */
=20
=20
+#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 =3D dev->node;
+	int ret;
+	int proplen;
+
+	dev_info(&dev->dev, PFX "probing via device tree\n");
+
+	ret =3D of_address_to_resource(np, 0, &resource);
+	if (ret) {
+		dev_warn(&dev->dev, PFX "invalid address from OF\n");
+		return ret;
+	}
+
+	regsize =3D get_property(np, "reg-size", &proplen);
+	if (regsize && proplen !=3D 4) {
+		dev_warn(&dev->dev, PFX "invalid regsize from OF\n");
+		return -EINVAL;
+	}
+
+	regspacing =3D get_property(np, "reg-spacing", &proplen);
+	if (regspacing && proplen !=3D 4) {
+		dev_warn(&dev->dev, PFX "invalid regspacing from OF\n");
+		return -EINVAL;
+	}
+
+	regshift =3D get_property(np, "reg-shift", &proplen);
+	if (regshift && proplen !=3D 4) {
+		dev_warn(&dev->dev, PFX "invalid regshift from OF\n");
+		return -EINVAL;
+	}
+
+	info =3D 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		=3D (enum si_type) match->data;
+	info->addr_source	=3D "device-tree";
+	info->io_setup		=3D mem_setup;
+	info->irq_setup		=3D std_irq_setup;
+
+	info->io.addr_type	=3D IPMI_MEM_ADDR_SPACE;
+	info->io.addr_data	=3D resource.start;
+
+	info->io.regsize	=3D regsize ? *regsize : DEFAULT_REGSIZE;
+	info->io.regspacing	=3D regspacing ? *regspacing : DEFAULT_REGSPACING;
+	info->io.regshift	=3D regshift ? *regshift : 0;
+
+	info->irq		=3D irq_of_parse_and_map(dev->node, 0);
+	info->dev		=3D &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 =3D (void*) info;
+
+	return try_smi_init(info);
+}
+
+static int __devexit ipmi_of_remove(struct of_device *dev)
+{
+	cleanup_one_si(dev->dev.driver_data);
+	return 0;
+}
+
+static struct of_device_id ipmi_match[] =3D
+{
+	{ .type =3D "ipmi", .compatible =3D "ipmi-kcs",  .data =3D (void *)(unsig=
ned long) SI_KCS },
+	{ .type =3D "ipmi", .compatible =3D "ipmi-smic", .data =3D (void *)(unsig=
ned long) SI_SMIC },
+	{ .type =3D "ipmi", .compatible =3D "ipmi-bt",   .data =3D (void *)(unsig=
ned long) SI_BT },
+	{},
+};
+
+static struct of_platform_driver ipmi_of_platform_driver =3D
+{
+	.name		=3D "ipmi",
+	.match_table	=3D ipmi_match,
+	.probe		=3D ipmi_of_probe,
+	.remove		=3D __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 +2898,10 @@ static __devinit int init_ipmi_si(void)
 	}
 #endif
=20
+#ifdef CONFIG_PPC_OF
+	of_register_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {
@@ -2895,6 +2999,10 @@ static __exit void cleanup_ipmi_si(void)
 	pci_unregister_driver(&ipmi_pci_driver);
 #endif
=20
+#ifdef CONFIG_PPC_OF
+	of_unregister_platform_driver(&ipmi_of_platform_driver);
+#endif
+
 	mutex_lock(&smi_infos_lock);
 	list_for_each_entry_safe(e, tmp_e, &smi_infos, link)
 		cleanup_one_si(e);

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

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

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

[Arnd Bergmann]

On Thursday 25 January 2007 04:34, 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 <krafft@de.ibm.com>
> Acked-by: Heiko J Schick <schihei@de.ibm.com>
> Signed-off-by: Corey Minyard <minyard@acm.org>

Acked-by: Arnd Bergmann <arnd.bergmann@de.ibm.com>

Of course, the subject line is wrong, but I guess Corey can
fix that up to come out as 'ipmi: add support for of_device probing'
or something like that.

	Arnd <><

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

[Corey Minyard]

Sorry, I've been traveling and haven't been able to follow this until now.

This should probably wait for 2.6.20, so I will hold it until the merge 
window
opens and send it on with a proper subject.

Thanks for noticing that the cleanup code now handles dynamic removal of
devices even when the driver is not being removed.

As for putting this in another file, that might be a good idea.  It also 
might
be a good idea to put the PCI, SMBIOS, and ACPI versions in their own
files, too.  I'll try to get that for the 2.6.20 merge window.

-Corey

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

[Christoph Hellwig]

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 <krafft@de.ibm.com>
> Acked-by: Heiko J Schick <schihei@de.ibm.com>
> 
> 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 @@
>   *         source@mvista.com
>   *
>   * Copyright 2002 MontaVista Software Inc.
> + * Copyright 2006 IBM Corp., Christian Krafft <krafft@de.ibm.com>
>   *
>   *  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 <linux/string.h>
>  #include <linux/ctype.h>
>  
> +#ifdef CONFIG_PPC_OF
> +#include <asm/of_device.h>
> +#include <asm/of_platform.h>
> +#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? :-)

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

[Arnd Bergmann]

On Friday 26 January 2007 03:54, Christoph Hellwig wrote:
> > =A0
> > +#ifdef CONFIG_PPC_OF
> > +#include <asm/of_device.h>
> > +#include <asm/of_platform.h>
> > +#endif
>=20
> Adding this OF-specific code to the generic file doesn't look exactly
> nice. =A0Is 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)
> > +{
> > +=A0=A0=A0=A0=A0/* should call
> > +=A0=A0=A0=A0=A0 * cleanup_one_si(dev->dev.driver_data); */
>=20
> Wo why doesn't it do that currently?=20

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

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

[Christian Krafft]

On Fri, 26 Jan 2007 05:23:02 +0100
Arnd Bergmann <arnd@arndb.de> wrote:


> > > +static int __devexit ipmi_of_remove(struct of_device *dev)
> > > +{
> > > +=A0=A0=A0=A0=A0/* should call
> > > +=A0=A0=A0=A0=A0 * cleanup_one_si(dev->dev.driver_data); */
> >=20
> > Wo why doesn't it do that currently?=20

This has been fixed already by the updated version sent on 25/01/07, 14:34 =
EST.

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

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