[PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Stefan Roese]

This patch reworks existing ibm-iic driver to support of_platform_device
and enables it to talk to device tree directly. The "old" OCP interface
for arch/ppc is still supported via #ifdef's and shall be removed when
arch/ppc is gone in a few months.

This is done to enable I2C support for the PPC4xx platforms now
being moved from arch/ppc (ocp) to arch/powerpc (of).

Signed-off-by: Stefan Roese <sr@denx.de>
---
 drivers/i2c/busses/Kconfig       |    2 +-
 drivers/i2c/busses/i2c-ibm_iic.c |  209 +++++++++++++++++++++++++++++++++++++-
 drivers/i2c/busses/i2c-ibm_iic.h |    2 +
 3 files changed, 211 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index de95c75..a47b5e6 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -241,7 +241,7 @@ config I2C_PIIX4
 
 config I2C_IBM_IIC
 	tristate "IBM PPC 4xx on-chip I2C interface"
-	depends on IBM_OCP
+	depends on 4xx
 	help
 	  Say Y here if you want to use IIC peripheral found on 
 	  embedded IBM PPC 4xx based systems. 
diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
index 956b947..78c6bf4 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.c
+++ b/drivers/i2c/busses/i2c-ibm_iic.c
@@ -39,8 +39,13 @@
 #include <asm/io.h>
 #include <linux/i2c.h>
 #include <linux/i2c-id.h>
+
+#ifdef CONFIG_PPC_MERGE
+#include <linux/of_platform.h>
+#else
 #include <asm/ocp.h>
 #include <asm/ibm4xx.h>
+#endif
 
 #include "i2c-ibm_iic.h"
 
@@ -57,6 +62,10 @@ static int iic_force_fast;
 module_param(iic_force_fast, bool, 0);
 MODULE_PARM_DESC(iic_fast_poll, "Force fast mode (400 kHz)");
 
+#ifdef CONFIG_PPC_MERGE
+static int device_idx = -1;
+#endif
+
 #define DBG_LEVEL 0
 
 #ifdef DBG
@@ -660,8 +669,205 @@ static inline u8 iic_clckdiv(unsigned int opb)
 /*
  * Register single IIC interface
  */
-static int __devinit iic_probe(struct ocp_device *ocp){
 
+#ifdef CONFIG_PPC_MERGE
+/*
+ * device-tree (of) when used from arch/powerpc
+ */
+static int __devinit iic_probe(struct of_device *ofdev,
+			       const struct of_device_id *match)
+{
+	struct ibm_iic_private* dev;
+	struct i2c_adapter* adap;
+	struct device_node *np;
+	int ret = -ENODEV;
+	int  irq, len;
+	const u32 *prop;
+	struct resource res;
+
+	np = ofdev->node;
+	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
+		printk(KERN_CRIT "ibm-iic(%s): failed to allocate device data\n",
+		       np->full_name);
+		return -ENOMEM;
+	}
+
+	dev_set_drvdata(&ofdev->dev, dev);
+
+	dev->np = np;
+	irq = irq_of_parse_and_map(np, 0);
+
+	if (of_address_to_resource(np, 0, &res)) {
+		printk(KERN_ERR "ibd-iic(%s): Can't get registers address\n",
+		       np->full_name);
+		goto fail1;
+	}
+	dev->paddr = res.start;
+
+	if (!request_mem_region(dev->paddr, sizeof(struct iic_regs), "ibm_iic")) {
+		ret = -EBUSY;
+		goto fail1;
+	}
+	dev->vaddr = ioremap(dev->paddr, sizeof(struct iic_regs));
+
+	if (dev->vaddr == NULL) {
+		printk(KERN_CRIT "ibm-iic(%s): failed to ioremap device regs\n",
+		       dev->np->full_name);
+		ret = -ENXIO;
+		goto fail2;
+	}
+
+	init_waitqueue_head(&dev->wq);
+
+	dev->irq = iic_force_poll ? -1 : (irq == NO_IRQ) ? -1 : irq;
+	if (dev->irq >= 0){
+		/* Disable interrupts until we finish initialization,
+		 * assumes level-sensitive IRQ setup...
+		 */
+		iic_interrupt_mode(dev, 0);
+		if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)) {
+			printk(KERN_ERR "ibm-iic(%s): request_irq %d failed\n",
+			       dev->np->full_name, dev->irq);
+			/* Fallback to the polling mode */
+			dev->irq = -1;
+		}
+	}
+
+	if (dev->irq < 0)
+		printk(KERN_WARNING "ibm-iic(%s): using polling mode\n",
+		       dev->np->full_name);
+
+	/* Board specific settings */
+	prop = of_get_property(np, "iic-mode", &len);
+	/* use 400kHz only if stated in dts, 100kHz otherwise */
+	dev->fast_mode = (prop && (*prop == 400));
+	/* clckdiv is the same for *all* IIC interfaces,
+	 * but I'd rather make a copy than introduce another global. --ebs
+	 */
+	/* Parent bus should have frequency filled */
+	prop = of_get_property(of_get_parent(np), "clock-frequency", &len);
+	if (prop == NULL) {
+		printk(KERN_ERR
+		       "ibm-iic(%s):no clock-frequency prop on parent bus!\n",
+		       dev->np->full_name);
+		goto fail;
+	}
+
+	dev->clckdiv = iic_clckdiv(*prop);
+	DBG("%s: clckdiv = %d\n", dev->np->full_name, dev->clckdiv);
+
+	/* Initialize IIC interface */
+	iic_dev_init(dev);
+
+	/* Register it with i2c layer */
+	adap = &dev->adap;
+	adap->dev.parent = &ofdev->dev;
+	strcpy(adap->name, "IBM IIC");
+	i2c_set_adapdata(adap, dev);
+	adap->id = I2C_HW_OCP;
+	adap->class = I2C_CLASS_HWMON;
+	adap->algo = &iic_algo;
+	adap->client_register = NULL;
+	adap->client_unregister = NULL;
+	adap->timeout = 1;
+	adap->retries = 1;
+
+	dev->idx = ++device_idx;
+	adap->nr = dev->idx;
+	if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
+		printk(KERN_CRIT"ibm-iic(%s): failed to register i2c adapter\n",
+		       dev->np->full_name);
+		goto fail;
+	}
+
+	printk(KERN_INFO "ibm-iic(%s): using %s mode\n", dev->np->full_name,
+	       dev->fast_mode ?
+	       "fast (400 kHz)" : "standard (100 kHz)");
+
+	return 0;
+
+fail:
+	if (dev->irq >= 0){
+		iic_interrupt_mode(dev, 0);
+		free_irq(dev->irq, dev);
+	}
+
+	iounmap(dev->vaddr);
+fail2:
+	release_mem_region(dev->paddr, sizeof(struct iic_regs));
+fail1:
+	dev_set_drvdata(&ofdev->dev, NULL);
+	kfree(dev);
+
+	return ret;
+}
+
+/*
+ * Cleanup initialized IIC interface
+ */
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private *dev =
+		(struct ibm_iic_private *)dev_get_drvdata(&ofdev->dev);
+	BUG_ON(dev == NULL);
+	if (i2c_del_adapter(&dev->adap)){
+		printk(KERN_CRIT "ibm-iic(%s): failed to delete i2c adapter\n",
+		       dev->np->full_name);
+		/* That's *very* bad, just shutdown IRQ ... */
+		if (dev->irq >= 0){
+			iic_interrupt_mode(dev, 0);
+			free_irq(dev->irq, dev);
+			dev->irq = -1;
+		}
+	} else {
+		if (dev->irq >= 0){
+			iic_interrupt_mode(dev, 0);
+			free_irq(dev->irq, dev);
+		}
+		iounmap(dev->vaddr);
+		release_mem_region(dev->paddr, sizeof(struct iic_regs));
+		kfree(dev);
+	}
+	return 0;
+}
+
+static struct of_device_id ibm_iic_match[] = {
+	{
+		.type = "i2c",
+		.compatible = "ibm,iic",
+	},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, ibm_iic_match);
+
+static struct of_platform_driver ibm_iic_driver = {
+	.name = "ibm-iic",
+	.match_table = ibm_iic_match,
+	.probe = iic_probe,
+	.remove = iic_remove,
+#if defined(CONFIG_PM)
+	.suspend = NULL,
+	.resume = NULL,
+#endif
+};
+
+static int __init iic_init(void)
+{
+	printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
+	return of_register_platform_driver(&ibm_iic_driver);
+}
+
+static void __exit iic_exit(void)
+{
+	of_unregister_platform_driver(&ibm_iic_driver);
+}
+#else
+/*
+ * OCP when used from arch/ppc
+ */
+static int __devinit iic_probe(struct ocp_device *ocp)
+{
 	struct ibm_iic_private* dev;
 	struct i2c_adapter* adap;
 	struct ocp_func_iic_data* iic_data = ocp->def->additions;
@@ -828,6 +1034,7 @@ static void __exit iic_exit(void)
 {
 	ocp_unregister_driver(&ibm_iic_driver);
 }
+#endif /* CONFIG_PPC_MERGE */
 
 module_init(iic_init);
 module_exit(iic_exit);
diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
index fdaa482..c15b091 100644
--- a/drivers/i2c/busses/i2c-ibm_iic.h
+++ b/drivers/i2c/busses/i2c-ibm_iic.h
@@ -50,6 +50,8 @@ struct ibm_iic_private {
 	int irq;
 	int fast_mode;
 	u8  clckdiv;
+	struct device_node *np;
+	phys_addr_t paddr;
 };
 
 /* IICx_CNTL register */
-- 
1.5.3.4

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Eugene Surovegin]

On Mon, Oct 15, 2007 at 03:29:11PM +0200, Stefan Roese wrote:

<snip>

> +#ifdef CONFIG_PPC_MERGE
> +static int device_idx = -1;
> +#endif
> +

<snip>

> +	dev->idx = ++device_idx;
> +	adap->nr = dev->idx;

Hmm, this doesn't look right. That mighty powerpc device everybody 
was so excited about for the last years doesn't provide a device 
instance number/index?

I think this approach is wrong, because I want i2c bus numbers for the 
on-chip i2c to be fixed. This code makes it dependent on the order 
devices were described in the device tree; how do you handle a 
situation when only the second i2c adapter is connected? For OCP I 
would just remove ocp_def for the IIC0.

-- 
Eugene

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, Eugene Surovegin <ebs@ebshome.net> wrote:
> On Mon, Oct 15, 2007 at 03:29:11PM +0200, Stefan Roese wrote:
>
> <snip>
>
> > +#ifdef CONFIG_PPC_MERGE
> > +static int device_idx = -1;
> > +#endif
> > +
>
> <snip>
>
> > +     dev->idx = ++device_idx;
> > +     adap->nr = dev->idx;
>
> Hmm, this doesn't look right. That mighty powerpc device everybody
> was so excited about for the last years doesn't provide a device
> instance number/index?
>
> I think this approach is wrong, because I want i2c bus numbers for the
> on-chip i2c to be fixed. This code makes it dependent on the order
> devices were described in the device tree; how do you handle a
> situation when only the second i2c adapter is connected? For OCP I
> would just remove ocp_def for the IIC0.

Segher is recommending that we use an aliases node as per the open
firmware example for this.  I think in this case it would look
something like this (but I'm not the expert):

aliases {
     IIC0 = "/path/to/bus/iic@0x2000";
     IIC1 = "/path/to/bus/iic@0x2000";
};

Which seems to make sense to me.  And it keeps it easy to have
multiple iic bus types sharing the same IIC bus number space (each
device does not try to maintain it's own little 'next index' value).

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Scott Wood]

On Mon, Oct 15, 2007 at 10:57:48AM -0600, Grant Likely wrote:
> Segher is recommending that we use an aliases node as per the open
> firmware example for this.  I think in this case it would look
> something like this (but I'm not the expert):
> 
> aliases {
>      IIC0 = "/path/to/bus/iic@0x2000";
>      IIC1 = "/path/to/bus/iic@0x2000";
> };

I think this is overly complicated; something like linux,i2c-index in the
i2c adapter node would be simpler.

Though, I don't see what the problem with the original approach is, as long
as the numbers are chosen in the same way when registering i2c clients based
on the children of the adapter node.  There's no concept in the hardware
itself of a bus number.

-Scott

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Eugene Surovegin]

On Mon, Oct 15, 2007 at 01:53:40PM -0500, Scott Wood wrote:
> Though, I don't see what the problem with the original approach is, as long
> as the numbers are chosen in the same way when registering i2c clients based
> on the children of the adapter node.  There's no concept in the hardware
> itself of a bus number.

Huh? As far as I can tell, there is. Also, I want messages from the 
kernel mention something I can map to the real hw, e.g. fixed IIC 
device index, not some random number.

This already works with the current OCP code, so if you want change 
it to a "superior" technology, please, make sure it provides the same 
functionality as trivial OCP one.

I find it rather puzzling that instead people are trying to make 
this a non-issue as soon as it cannot be implemented easily with 
their new and shiny infrastructure.

-- 
Eugene

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, Eugene Surovegin <ebs@ebshome.net> wrote:
> On Mon, Oct 15, 2007 at 01:53:40PM -0500, Scott Wood wrote:
> > Though, I don't see what the problem with the original approach is, as long
> > as the numbers are chosen in the same way when registering i2c clients based
> > on the children of the adapter node.  There's no concept in the hardware
> > itself of a bus number.
>
> Huh? As far as I can tell, there is. Also, I want messages from the
> kernel mention something I can map to the real hw, e.g. fixed IIC
> device index, not some random number.

Yes, in the same way that there may be more than one on-chip serial or
ethernet controllers.  However, it does not necessarily follow that
the *logical* bus number will match the way on chip devices are
numbered.

Example: Suppose you have a board with 2 chips which each include 2
i2c controllers.  Each chip numbers them 1 & 2.  So, which chip gets
1-2 and which one gets 3-4?

>
> This already works with the current OCP code, so if you want change
> it to a "superior" technology, please, make sure it provides the same
> functionality as trivial OCP one.

agreed

> I find it rather puzzling that instead people are trying to make
> this a non-issue as soon as it cannot be implemented easily with
> their new and shiny infrastructure.

No, it is a real problem; and not just for i2c.  We need a solution for it.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Scott Wood]

Eugene Surovegin wrote:
> On Mon, Oct 15, 2007 at 01:53:40PM -0500, Scott Wood wrote:
>> Though, I don't see what the problem with the original approach is,
>> as long as the numbers are chosen in the same way when registering
>> i2c clients based on the children of the adapter node.  There's no
>> concept in the hardware itself of a bus number.
> 
> Huh? As far as I can tell, there is.

Where?  Hardware != Documentation.

> Also, I want messages from the kernel mention something I can map to
> the real hw, e.g. fixed IIC device index, not some random number.

The OF node path should have a unit address that accomplishes that.

> I find it rather puzzling that instead people are trying to make this
> a non-issue as soon as it cannot be implemented easily with their new
> and shiny infrastructure.

"People" are not a monolithic entity.  I never advocated using bus numbers.

-Scott

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
> On Mon, Oct 15, 2007 at 10:57:48AM -0600, Grant Likely wrote:
> > Segher is recommending that we use an aliases node as per the open
> > firmware example for this.  I think in this case it would look
> > something like this (but I'm not the expert):
> >
> > aliases {
> >      IIC0 = "/path/to/bus/iic@0x2000";
> >      IIC1 = "/path/to/bus/iic@0x2000";
> > };
>
> I think this is overly complicated; something like linux,i2c-index in the
> i2c adapter node would be simpler.

But not perhaps better.  Enumeration is a system-wide thing.  It does
make sense to group all the device enumerations in one place.  It
eliminates two nodes trying to enumerate to the same device number and
since device numbers are something that the user will potentially want
to modify, it separates enumeration from hardware description.

As per your point below; if all the i2c devices are children of the
adapter, then yes you are right that the bus number doesn't matter to
the user.  But it *does* matter for things like serial and ethernet
ports.

>
> Though, I don't see what the problem with the original approach is, as long
> as the numbers are chosen in the same way when registering i2c clients based
> on the children of the adapter node.  There's no concept in the hardware
> itself of a bus number.

Even if you take this approach, the driver still need to know what bus
number to use (as per the i2c infrastructure).  It is not sane for an
i2c bus driver to attempt to assign the bus number itself because it
could conflict with another arbitrarily chosen bus number from another
driver.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Scott Wood]

Grant Likely wrote:
> On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
>> On Mon, Oct 15, 2007 at 10:57:48AM -0600, Grant Likely wrote:
>>> Segher is recommending that we use an aliases node as per the open
>>> firmware example for this.  I think in this case it would look
>>> something like this (but I'm not the expert):
>>>
>>> aliases {
>>>      IIC0 = "/path/to/bus/iic@0x2000";
>>>      IIC1 = "/path/to/bus/iic@0x2000";
>>> };
>> I think this is overly complicated; something like linux,i2c-index in the
>> i2c adapter node would be simpler.
> 
> But not perhaps better.  Enumeration is a system-wide thing.  It does
> make sense to group all the device enumerations in one place.

For purposes of generating a Linux bus number, having to scan all 
aliases for a matching path and turn IIC0/IIC1 into a number is a bit silly.

For associating a device node with a human readable label, I'd prefer a 
"label" property in the device node, rather than doing it backwards with 
aliases.

> As per your point below; if all the i2c devices are children of the
> adapter, then yes you are right that the bus number doesn't matter to
> the user.  But it *does* matter for things like serial and ethernet
> ports.

And a label property would be great for that. :-)

>> Though, I don't see what the problem with the original approach is, as long
>> as the numbers are chosen in the same way when registering i2c clients based
>> on the children of the adapter node.  There's no concept in the hardware
>> itself of a bus number.
> 
> Even if you take this approach, the driver still need to know what bus
> number to use (as per the i2c infrastructure).  It is not sane for an
> i2c bus driver to attempt to assign the bus number itself because it
> could conflict with another arbitrarily chosen bus number from another
> driver.

Hence why global bus numbers suck. :-)

However, statically chosen numbers should only be done for platform 
devices, not dynamic things such as PCI, so in practice I don't think 
it'd be a problem.

-Scott

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
> Grant Likely wrote:
> > On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
> >> On Mon, Oct 15, 2007 at 10:57:48AM -0600, Grant Likely wrote:
> >>> Segher is recommending that we use an aliases node as per the open
> >>> firmware example for this.  I think in this case it would look
> >>> something like this (but I'm not the expert):
> >>>
> >>> aliases {
> >>>      IIC0 = "/path/to/bus/iic@0x2000";
> >>>      IIC1 = "/path/to/bus/iic@0x2000";
> >>> };
> >> I think this is overly complicated; something like linux,i2c-index in the
> >> i2c adapter node would be simpler.
> >
> > But not perhaps better.  Enumeration is a system-wide thing.  It does
> > make sense to group all the device enumerations in one place.
>
> For purposes of generating a Linux bus number, having to scan all
> aliases for a matching path and turn IIC0/IIC1 into a number is a bit silly.
>
> For associating a device node with a human readable label, I'd prefer a
> "label" property in the device node, rather than doing it backwards with
> aliases.

Here the corresponding problem; having to scan every device node to
make sure you don't assign a number already selected by another node
(in the case where one node is assigned a number and another is not).

At some point you're going to need to reverse map from number to
device.  I'd rather scan a list of properties in aliases instead of
scanning the whole tree looking for all devices of the same type.

>
> > As per your point below; if all the i2c devices are children of the
> > adapter, then yes you are right that the bus number doesn't matter to
> > the user.  But it *does* matter for things like serial and ethernet
> > ports.
>
> And a label property would be great for that. :-)

Not really; if the user needs to renumber devices; you don't want him
fiddling around in the hardware description.  Just like the chosen
node; an aliases describes logical constructs, not physical ones.  I
don't think this is any different from the linux,stdout-path property
in chosen.

>
> >> Though, I don't see what the problem with the original approach is, as long
> >> as the numbers are chosen in the same way when registering i2c clients based
> >> on the children of the adapter node.  There's no concept in the hardware
> >> itself of a bus number.
> >
> > Even if you take this approach, the driver still need to know what bus
> > number to use (as per the i2c infrastructure).  It is not sane for an
> > i2c bus driver to attempt to assign the bus number itself because it
> > could conflict with another arbitrarily chosen bus number from another
> > driver.
>
> Hence why global bus numbers suck. :-)

I'm not going to disagree with you there; but it's unavoidable.

> However, statically chosen numbers should only be done for platform
> devices, not dynamic things such as PCI, so in practice I don't think
> it'd be a problem.

So... in the I2C case, as long as all the i2c devices are in the
device tree and the i2c layer is responsible for actually handing out
the bus numbers; then yes I agree.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Scott Wood]

Grant Likely wrote:
> On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
>> For associating a device node with a human readable label, I'd
>> prefer a "label" property in the device node, rather than doing it
>> backwards with aliases.
> 
> Here the corresponding problem; having to scan every device node to 
> make sure you don't assign a number already selected by another node 
> (in the case where one node is assigned a number and another is not).
> 
Don't Do That(tm).  If you use this mechanism, and an adapter node
doesn't have a bus number, then it doesn't get to pre-register devices,
but instead must use i2c_new_device.

>>> As per your point below; if all the i2c devices are children of
>>> the adapter, then yes you are right that the bus number doesn't
>>> matter to the user.  But it *does* matter for things like serial
>>> and ethernet ports.
>> And a label property would be great for that. :-)
> 
> Not really; if the user needs to renumber devices; you don't want him
>  fiddling around in the hardware description.

Why would the user renumber devices?

> Just like the chosen node; an aliases describes logical constructs,
> not physical ones.  I don't think this is any different from the
> linux,stdout-path property in chosen.

Well, it's somewhat different in that stdout describes a usage of the 
device, not the identity.

Still, I don't like linux,stdout-path. :-)
At the very least it should be a phandle.

-Scott

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
> Grant Likely wrote:
> > On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
> >> For associating a device node with a human readable label, I'd
> >> prefer a "label" property in the device node, rather than doing it
> >> backwards with aliases.
> >
> > Here the corresponding problem; having to scan every device node to
> > make sure you don't assign a number already selected by another node
> > (in the case where one node is assigned a number and another is not).
> >
> Don't Do That(tm).  If you use this mechanism, and an adapter node
> doesn't have a bus number, then it doesn't get to pre-register devices,
> but instead must use i2c_new_device.

Even that doesn't work.  For example if a PCI device is probed which
registers an i2c bus; there needs to be a mechanism for the i2c layer
to know that an id is already spoken for, so once again there needs to
be a mechanism to map easily from id to device (or lack thereof).

> >>> As per your point below; if all the i2c devices are children of
> >>> the adapter, then yes you are right that the bus number doesn't
> >>> matter to the user.  But it *does* matter for things like serial
> >>> and ethernet ports.
> >> And a label property would be great for that. :-)
> >
> > Not really; if the user needs to renumber devices; you don't want him
> >  fiddling around in the hardware description.
>
> Why would the user renumber devices?

Where user == system integrator or firmware engineer.  ie. boards with
no-populate options which affect the numbering; changes to match the
silkscreening on the chassis when a common board is used by multiple
systems.  It's a conceivable scenario.  (Again; this is more relevant
to eth and serial devices than i2c).

>
> > Just like the chosen node; an aliases describes logical constructs,
> > not physical ones.  I don't think this is any different from the
> > linux,stdout-path property in chosen.
>
> Well, it's somewhat different in that stdout describes a usage of the
> device, not the identity.
>
> Still, I don't like linux,stdout-path. :-)
> At the very least it should be a phandle.

I'm cool with it being a phandle.  (insert obvious objection someone
will make about that not being OF compatible)  :-)

Perhaps aliases should look like (which can be generated from the OF
path form when the device tree if flattened):
aliases {
     linux,eth0 = <phandle1>;
     linux,eth1 = <phandle2>;
}

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Scott Wood]

Grant Likely wrote:
> On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
>> Don't Do That(tm).  If you use this mechanism, and an adapter node
>> doesn't have a bus number, then it doesn't get to pre-register devices,
>> but instead must use i2c_new_device.
> 
> Even that doesn't work.  For example if a PCI device is probed which
> registers an i2c bus; there needs to be a mechanism for the i2c layer
> to know that an id is already spoken for, so once again there needs to
> be a mechanism to map easily from id to device (or lack thereof).

As long as all statically-assigned buses have their devices passed to 
i2c_register_board_info by platform code before the PCI device is 
probed, the i2c layer will hand out bus numbers that don't conflict.

> Where user == system integrator or firmware engineer.  ie. boards with
> no-populate options which affect the numbering; changes to match the
> silkscreening on the chassis when a common board is used by multiple
> systems.  It's a conceivable scenario.  (Again; this is more relevant
> to eth and serial devices than i2c).

Sure, but I guess I don't see the problem with such a person editing the 
label property.  The label property also gives more freedom in terms of 
which characters can be used in the description.

Aliases could still be used when there's a higher level abstraction 
related to purpose, not identification.

-Scott

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[David Gibson]

On Mon, Oct 15, 2007 at 01:13:14PM -0600, Grant Likely wrote:
> On 10/15/07, Scott Wood <scottwood@freescale.com> wrote:
> > On Mon, Oct 15, 2007 at 10:57:48AM -0600, Grant Likely wrote:
> > > Segher is recommending that we use an aliases node as per the open
> > > firmware example for this.  I think in this case it would look
> > > something like this (but I'm not the expert):
> > >
> > > aliases {
> > >      IIC0 = "/path/to/bus/iic@0x2000";
> > >      IIC1 = "/path/to/bus/iic@0x2000";
> > > };
> >
> > I think this is overly complicated; something like linux,i2c-index in the
> > i2c adapter node would be simpler.
> 
> But not perhaps better.  Enumeration is a system-wide thing.  It does
> make sense to group all the device enumerations in one place.  It
> eliminates two nodes trying to enumerate to the same device number and
> since device numbers are something that the user will potentially want
> to modify, it separates enumeration from hardware description.
> 
> As per your point below; if all the i2c devices are children of the
> adapter, then yes you are right that the bus number doesn't matter to
> the user.  But it *does* matter for things like serial and ethernet
> ports.

As the inventor of "linux,network-index", please don't invent
"linux,i2c-index".  linux,network-index was and is a hack - it's
badness is limited by the fact that it's essentially local to the
bootwrapper.  It's only used in the bootwrapper, and I only really
intended it for use in bootwrappers which provide their own device
tree, so as to match the device nodes against whatever order the MAC
addresses were supplied by the firmware.

I plan to replace the linux,network-index thing with aliases
(including some dtc support to make that easy) just as soon as I get
around to it... don't hold your breath.

Using a similar property from an actual kernel driver would be much
uglier, and harder to clean up later.

Using aliases would be.. less bad, but it would still require that
the device tree always supply an alias for the iic driver to work
which is kind of nasty.

In fact I think it may be acceptle to do the idx++ thing in this
situation.  Bus numbers are ugly, but it's not the worst ugliness in
the horrible mess that is the Linux i2c subsystem.  It means that bus
numbers are theoretically unstable, but that's increasingly true of
devices of all sorts - it's up to udev to assign meaningful labels at
the user level.

> > Though, I don't see what the problem with the original approach is, as long
> > as the numbers are chosen in the same way when registering i2c clients based
> > on the children of the adapter node.  There's no concept in the hardware
> > itself of a bus number.
> 
> Even if you take this approach, the driver still need to know what bus
> number to use (as per the i2c infrastructure).  It is not sane for an
> i2c bus driver to attempt to assign the bus number itself because it
> could conflict with another arbitrarily chosen bus number from another
> driver.
> 
> Cheers,
> g.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> As the inventor of "linux,network-index", please don't invent
> "linux,i2c-index".  linux,network-index was and is a hack - it's
> badness is limited by the fact that it's essentially local to the
> bootwrapper.  It's only used in the bootwrapper, and I only really
> intended it for use in bootwrappers which provide their own device
> tree, so as to match the device nodes against whatever order the MAC
> addresses were supplied by the firmware.
>
> I plan to replace the linux,network-index thing with aliases
> (including some dtc support to make that easy) just as soon as I get
> around to it... don't hold your breath.
>
> Using a similar property from an actual kernel driver would be much
> uglier, and harder to clean up later.

This I know from first hand experience; it is Uh-gly!  :-)

>
> Using aliases would be.. less bad, but it would still require that
> the device tree always supply an alias for the iic driver to work
> which is kind of nasty.
>
> In fact I think it may be acceptle to do the idx++ thing in this
> situation.  Bus numbers are ugly, but it's not the worst ugliness in
> the horrible mess that is the Linux i2c subsystem.  It means that bus
> numbers are theoretically unstable, but that's increasingly true of
> devices of all sorts - it's up to udev to assign meaningful labels at
> the user level.

I think the real problem here comes into play when there are 2 types
of i2c busses in the system.  If they both maintain their own idx++
values; then they will conflict.  If an auto assigned bus number is
used; then it needs to be assigned by the i2c infrastructure; not by
the driver.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Jean Delvare]

On Mon, 15 Oct 2007 22:21:38 -0600, Grant Likely wrote:
> On 10/15/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> > In fact I think it may be acceptle to do the idx++ thing in this
> > situation.  Bus numbers are ugly, but it's not the worst ugliness in
> > the horrible mess that is the Linux i2c subsystem.  It means that bus
> > numbers are theoretically unstable, but that's increasingly true of
> > devices of all sorts - it's up to udev to assign meaningful labels at
> > the user level.

David, after such a rant against the Linux i2c subsystem, I sure hope
that you're going to contribute patches to make it better (whatever you
think needs to be improved, as you didn't say.)

> I think the real problem here comes into play when there are 2 types
> of i2c busses in the system.  If they both maintain their own idx++
> values; then they will conflict.  If an auto assigned bus number is
> used; then it needs to be assigned by the i2c infrastructure; not by
> the driver.

Very true. If you aren't going to define the i2c bus numbers at
platform data level, then you shouldn't be defining them _at all_.
Don't use i2c_add_numbered_adapter, use i2c_add_adapter and let
i2c-core choose an appropriate a bus number.

-- 
Jean Delvare

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[David Gibson]

On Tue, Oct 16, 2007 at 09:19:39PM +0200, Jean Delvare wrote:
> On Mon, 15 Oct 2007 22:21:38 -0600, Grant Likely wrote:
> > On 10/15/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > In fact I think it may be acceptle to do the idx++ thing in this
> > > situation.  Bus numbers are ugly, but it's not the worst ugliness in
> > > the horrible mess that is the Linux i2c subsystem.  It means that bus
> > > numbers are theoretically unstable, but that's increasingly true of
> > > devices of all sorts - it's up to udev to assign meaningful labels at
> > > the user level.
> 
> David, after such a rant against the Linux i2c subsystem, I sure hope
> that you're going to contribute patches to make it better (whatever you
> think needs to be improved, as you didn't say.)

I've frequently contemplated it.  In the unlikely event that it ever
bubbles to the top of my priorities, I might well.

> > I think the real problem here comes into play when there are 2 types
> > of i2c busses in the system.  If they both maintain their own idx++
> > values; then they will conflict.  If an auto assigned bus number is
> > used; then it needs to be assigned by the i2c infrastructure; not by
> > the driver.
> 
> Very true. If you aren't going to define the i2c bus numbers at
> platform data level, then you shouldn't be defining them _at all_.
> Don't use i2c_add_numbered_adapter, use i2c_add_adapter and let
> i2c-core choose an appropriate a bus number.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Grant Likely]

On 10/15/07, Stefan Roese <sr@denx.de> wrote:
> This patch reworks existing ibm-iic driver to support of_platform_device
> and enables it to talk to device tree directly. The "old" OCP interface
> for arch/ppc is still supported via #ifdef's and shall be removed when
> arch/ppc is gone in a few months.
>
> This is done to enable I2C support for the PPC4xx platforms now
> being moved from arch/ppc (ocp) to arch/powerpc (of).

May I suggest another approach?

Take a look at driver/video/xilinxfb.c.
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/video/xilinxfb.c;h=6ef99b2d13ca6f8a4809d5914cbab6309255b3e3;hb=287e5d6fcccfa38b953cebe307e1ddfd32363355

That driver supports both the platform bus and the of_platform bus.
(xilinxfb_of_probe and xilinxfb_platform_probe) and both functions
call a common setup routine (xilinxfb_assign; but I probably should
have named it xilinxfb_setup).

Rather than writing an entirely new probe function; I suggest
splitting the binding code from the initialization code.  The binding
code translates from the device description (be that platform bus,
of_platform bus, ocp, whatever) to the values the driver needs.  The
initialization code needs to do the same thing for both bus bindings,
and the bus binding code is only concerned with getting the
appropriate values from the device descripton.

It's a little bit more work to refactor the driver to follow this
mode, but it results in far less code which is easier to review and
understand.  Plus the device description is incomplete, you can bail
out before you start allocating and mapping memory that needs to be
unwound.

Cheers,
g.

>
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  drivers/i2c/busses/Kconfig       |    2 +-
>  drivers/i2c/busses/i2c-ibm_iic.c |  209 +++++++++++++++++++++++++++++++++++++-
>  drivers/i2c/busses/i2c-ibm_iic.h |    2 +
>  3 files changed, 211 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index de95c75..a47b5e6 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -241,7 +241,7 @@ config I2C_PIIX4
>
>  config I2C_IBM_IIC
>         tristate "IBM PPC 4xx on-chip I2C interface"
> -       depends on IBM_OCP
> +       depends on 4xx
>         help
>           Say Y here if you want to use IIC peripheral found on
>           embedded IBM PPC 4xx based systems.
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
> index 956b947..78c6bf4 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -39,8 +39,13 @@
>  #include <asm/io.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-id.h>
> +
> +#ifdef CONFIG_PPC_MERGE
> +#include <linux/of_platform.h>
> +#else
>  #include <asm/ocp.h>
>  #include <asm/ibm4xx.h>
> +#endif
>
>  #include "i2c-ibm_iic.h"
>
> @@ -57,6 +62,10 @@ static int iic_force_fast;
>  module_param(iic_force_fast, bool, 0);
>  MODULE_PARM_DESC(iic_fast_poll, "Force fast mode (400 kHz)");
>
> +#ifdef CONFIG_PPC_MERGE
> +static int device_idx = -1;
> +#endif
> +
>  #define DBG_LEVEL 0
>
>  #ifdef DBG
> @@ -660,8 +669,205 @@ static inline u8 iic_clckdiv(unsigned int opb)
>  /*
>   * Register single IIC interface
>   */
> -static int __devinit iic_probe(struct ocp_device *ocp){
>
> +#ifdef CONFIG_PPC_MERGE
> +/*
> + * device-tree (of) when used from arch/powerpc
> + */
> +static int __devinit iic_probe(struct of_device *ofdev,
> +                              const struct of_device_id *match)
> +{
> +       struct ibm_iic_private* dev;
> +       struct i2c_adapter* adap;
> +       struct device_node *np;
> +       int ret = -ENODEV;
> +       int  irq, len;
> +       const u32 *prop;
> +       struct resource res;
> +
> +       np = ofdev->node;
> +       if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
> +               printk(KERN_CRIT "ibm-iic(%s): failed to allocate device data\n",
> +                      np->full_name);
> +               return -ENOMEM;
> +       }
> +
> +       dev_set_drvdata(&ofdev->dev, dev);
> +
> +       dev->np = np;
> +       irq = irq_of_parse_and_map(np, 0);
> +
> +       if (of_address_to_resource(np, 0, &res)) {
> +               printk(KERN_ERR "ibd-iic(%s): Can't get registers address\n",
> +                      np->full_name);
> +               goto fail1;
> +       }
> +       dev->paddr = res.start;
> +
> +       if (!request_mem_region(dev->paddr, sizeof(struct iic_regs), "ibm_iic")) {
> +               ret = -EBUSY;
> +               goto fail1;
> +       }
> +       dev->vaddr = ioremap(dev->paddr, sizeof(struct iic_regs));
> +
> +       if (dev->vaddr == NULL) {
> +               printk(KERN_CRIT "ibm-iic(%s): failed to ioremap device regs\n",
> +                      dev->np->full_name);
> +               ret = -ENXIO;
> +               goto fail2;
> +       }
> +
> +       init_waitqueue_head(&dev->wq);
> +
> +       dev->irq = iic_force_poll ? -1 : (irq == NO_IRQ) ? -1 : irq;
> +       if (dev->irq >= 0){
> +               /* Disable interrupts until we finish initialization,
> +                * assumes level-sensitive IRQ setup...
> +                */
> +               iic_interrupt_mode(dev, 0);
> +               if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)) {
> +                       printk(KERN_ERR "ibm-iic(%s): request_irq %d failed\n",
> +                              dev->np->full_name, dev->irq);
> +                       /* Fallback to the polling mode */
> +                       dev->irq = -1;
> +               }
> +       }
> +
> +       if (dev->irq < 0)
> +               printk(KERN_WARNING "ibm-iic(%s): using polling mode\n",
> +                      dev->np->full_name);
> +
> +       /* Board specific settings */
> +       prop = of_get_property(np, "iic-mode", &len);
> +       /* use 400kHz only if stated in dts, 100kHz otherwise */
> +       dev->fast_mode = (prop && (*prop == 400));
> +       /* clckdiv is the same for *all* IIC interfaces,
> +        * but I'd rather make a copy than introduce another global. --ebs
> +        */
> +       /* Parent bus should have frequency filled */
> +       prop = of_get_property(of_get_parent(np), "clock-frequency", &len);
> +       if (prop == NULL) {
> +               printk(KERN_ERR
> +                      "ibm-iic(%s):no clock-frequency prop on parent bus!\n",
> +                      dev->np->full_name);
> +               goto fail;
> +       }
> +
> +       dev->clckdiv = iic_clckdiv(*prop);
> +       DBG("%s: clckdiv = %d\n", dev->np->full_name, dev->clckdiv);
> +
> +       /* Initialize IIC interface */
> +       iic_dev_init(dev);
> +
> +       /* Register it with i2c layer */
> +       adap = &dev->adap;
> +       adap->dev.parent = &ofdev->dev;
> +       strcpy(adap->name, "IBM IIC");
> +       i2c_set_adapdata(adap, dev);
> +       adap->id = I2C_HW_OCP;
> +       adap->class = I2C_CLASS_HWMON;
> +       adap->algo = &iic_algo;
> +       adap->client_register = NULL;
> +       adap->client_unregister = NULL;
> +       adap->timeout = 1;
> +       adap->retries = 1;
> +
> +       dev->idx = ++device_idx;
> +       adap->nr = dev->idx;
> +       if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
> +               printk(KERN_CRIT"ibm-iic(%s): failed to register i2c adapter\n",
> +                      dev->np->full_name);
> +               goto fail;
> +       }
> +
> +       printk(KERN_INFO "ibm-iic(%s): using %s mode\n", dev->np->full_name,
> +              dev->fast_mode ?
> +              "fast (400 kHz)" : "standard (100 kHz)");
> +
> +       return 0;
> +
> +fail:
> +       if (dev->irq >= 0){
> +               iic_interrupt_mode(dev, 0);
> +               free_irq(dev->irq, dev);
> +       }
> +
> +       iounmap(dev->vaddr);
> +fail2:
> +       release_mem_region(dev->paddr, sizeof(struct iic_regs));
> +fail1:
> +       dev_set_drvdata(&ofdev->dev, NULL);
> +       kfree(dev);
> +
> +       return ret;
> +}
> +
> +/*
> + * Cleanup initialized IIC interface
> + */
> +static int __devexit iic_remove(struct of_device *ofdev)
> +{
> +       struct ibm_iic_private *dev =
> +               (struct ibm_iic_private *)dev_get_drvdata(&ofdev->dev);
> +       BUG_ON(dev == NULL);
> +       if (i2c_del_adapter(&dev->adap)){
> +               printk(KERN_CRIT "ibm-iic(%s): failed to delete i2c adapter\n",
> +                      dev->np->full_name);
> +               /* That's *very* bad, just shutdown IRQ ... */
> +               if (dev->irq >= 0){
> +                       iic_interrupt_mode(dev, 0);
> +                       free_irq(dev->irq, dev);
> +                       dev->irq = -1;
> +               }
> +       } else {
> +               if (dev->irq >= 0){
> +                       iic_interrupt_mode(dev, 0);
> +                       free_irq(dev->irq, dev);
> +               }
> +               iounmap(dev->vaddr);
> +               release_mem_region(dev->paddr, sizeof(struct iic_regs));
> +               kfree(dev);
> +       }
> +       return 0;
> +}
> +
> +static struct of_device_id ibm_iic_match[] = {
> +       {
> +               .type = "i2c",
> +               .compatible = "ibm,iic",
> +       },
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, ibm_iic_match);
> +
> +static struct of_platform_driver ibm_iic_driver = {
> +       .name = "ibm-iic",
> +       .match_table = ibm_iic_match,
> +       .probe = iic_probe,
> +       .remove = iic_remove,
> +#if defined(CONFIG_PM)
> +       .suspend = NULL,
> +       .resume = NULL,
> +#endif
> +};
> +
> +static int __init iic_init(void)
> +{
> +       printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
> +       return of_register_platform_driver(&ibm_iic_driver);
> +}
> +
> +static void __exit iic_exit(void)
> +{
> +       of_unregister_platform_driver(&ibm_iic_driver);
> +}
> +#else
> +/*
> + * OCP when used from arch/ppc
> + */
> +static int __devinit iic_probe(struct ocp_device *ocp)
> +{
>         struct ibm_iic_private* dev;
>         struct i2c_adapter* adap;
>         struct ocp_func_iic_data* iic_data = ocp->def->additions;
> @@ -828,6 +1034,7 @@ static void __exit iic_exit(void)
>  {
>         ocp_unregister_driver(&ibm_iic_driver);
>  }
> +#endif /* CONFIG_PPC_MERGE */
>
>  module_init(iic_init);
>  module_exit(iic_exit);
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
> index fdaa482..c15b091 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.h
> +++ b/drivers/i2c/busses/i2c-ibm_iic.h
> @@ -50,6 +50,8 @@ struct ibm_iic_private {
>         int irq;
>         int fast_mode;
>         u8  clckdiv;
> +       struct device_node *np;
> +       phys_addr_t paddr;
>  };
>
>  /* IICx_CNTL register */
> --
> 1.5.3.4
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH 2/2] i2c: Add devtree-aware iic support for PPC4xx

[Valentine Barshak]

Please, take a look at my comments below

Stefan Roese wrote:
> This patch reworks existing ibm-iic driver to support of_platform_device
> and enables it to talk to device tree directly. The "old" OCP interface
> for arch/ppc is still supported via #ifdef's and shall be removed when
> arch/ppc is gone in a few months.
> 
> This is done to enable I2C support for the PPC4xx platforms now
> being moved from arch/ppc (ocp) to arch/powerpc (of).
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  drivers/i2c/busses/Kconfig       |    2 +-
>  drivers/i2c/busses/i2c-ibm_iic.c |  209 +++++++++++++++++++++++++++++++++++++-
>  drivers/i2c/busses/i2c-ibm_iic.h |    2 +
>  3 files changed, 211 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index de95c75..a47b5e6 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -241,7 +241,7 @@ config I2C_PIIX4
>  
>  config I2C_IBM_IIC
>  	tristate "IBM PPC 4xx on-chip I2C interface"
> -	depends on IBM_OCP
> +	depends on 4xx
>  	help
>  	  Say Y here if you want to use IIC peripheral found on 
>  	  embedded IBM PPC 4xx based systems. 
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.c b/drivers/i2c/busses/i2c-ibm_iic.c
> index 956b947..78c6bf4 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.c
> +++ b/drivers/i2c/busses/i2c-ibm_iic.c
> @@ -39,8 +39,13 @@
>  #include <asm/io.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-id.h>
> +
> +#ifdef CONFIG_PPC_MERGE
> +#include <linux/of_platform.h>
> +#else
>  #include <asm/ocp.h>
>  #include <asm/ibm4xx.h>
> +#endif
>  
>  #include "i2c-ibm_iic.h"
>  
> @@ -57,6 +62,10 @@ static int iic_force_fast;
>  module_param(iic_force_fast, bool, 0);
>  MODULE_PARM_DESC(iic_fast_poll, "Force fast mode (400 kHz)");
>  
> +#ifdef CONFIG_PPC_MERGE
> +static int device_idx = -1;
> +#endif
> +
>  #define DBG_LEVEL 0
>  
>  #ifdef DBG
> @@ -660,8 +669,205 @@ static inline u8 iic_clckdiv(unsigned int opb)
>  /*
>   * Register single IIC interface
>   */
> -static int __devinit iic_probe(struct ocp_device *ocp){
>  
> +#ifdef CONFIG_PPC_MERGE
> +/*
> + * device-tree (of) when used from arch/powerpc
> + */
> +static int __devinit iic_probe(struct of_device *ofdev,
> +			       const struct of_device_id *match)
> +{
> +	struct ibm_iic_private* dev;
> +	struct i2c_adapter* adap;
> +	struct device_node *np;
> +	int ret = -ENODEV;
> +	int  irq, len;
> +	const u32 *prop;
> +	struct resource res;
> +
> +	np = ofdev->node;
> +	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {
> +		printk(KERN_CRIT "ibm-iic(%s): failed to allocate device data\n",
> +		       np->full_name);
> +		return -ENOMEM;
> +	}
> +
> +	dev_set_drvdata(&ofdev->dev, dev);
> +
> +	dev->np = np;
> +	irq = irq_of_parse_and_map(np, 0);

In case of an error irq might be NO_IRQ here (0)
This could cause problems in chacking the mode (irq or poll) later.

> +
> +	if (of_address_to_resource(np, 0, &res)) {
> +		printk(KERN_ERR "ibd-iic(%s): Can't get registers address\n",
> +		       np->full_name);
> +		goto fail1;
> +	}
> +	dev->paddr = res.start;
> +
> +	if (!request_mem_region(dev->paddr, sizeof(struct iic_regs), "ibm_iic")) {
> +		ret = -EBUSY;
> +		goto fail1;
> +	}
> +	dev->vaddr = ioremap(dev->paddr, sizeof(struct iic_regs));
> +
> +	if (dev->vaddr == NULL) {
> +		printk(KERN_CRIT "ibm-iic(%s): failed to ioremap device regs\n",
> +		       dev->np->full_name);
> +		ret = -ENXIO;
> +		goto fail2;
> +	}
> +
> +	init_waitqueue_head(&dev->wq);
> +
> +	dev->irq = iic_force_poll ? -1 : (irq == NO_IRQ) ? -1 : irq;
> +	if (dev->irq >= 0){

If irq equals NO_IRQ (irq == 0) we shouldn't assume interrupt mode here.
I'd suggest to update the driver to use (irq != NO_IRQ) checks instead 
of (irq >=0)

> +		/* Disable interrupts until we finish initialization,
> +		 * assumes level-sensitive IRQ setup...
> +		 */
> +		iic_interrupt_mode(dev, 0);
> +		if (request_irq(dev->irq, iic_handler, 0, "IBM IIC", dev)) {
> +			printk(KERN_ERR "ibm-iic(%s): request_irq %d failed\n",
> +			       dev->np->full_name, dev->irq);
> +			/* Fallback to the polling mode */
> +			dev->irq = -1;
> +		}
> +	}
> +
> +	if (dev->irq < 0)
> +		printk(KERN_WARNING "ibm-iic(%s): using polling mode\n",
> +		       dev->np->full_name);
> +
> +	/* Board specific settings */
> +	prop = of_get_property(np, "iic-mode", &len);
> +	/* use 400kHz only if stated in dts, 100kHz otherwise */
> +	dev->fast_mode = (prop && (*prop == 400));
> +	/* clckdiv is the same for *all* IIC interfaces,
> +	 * but I'd rather make a copy than introduce another global. --ebs
> +	 */
> +	/* Parent bus should have frequency filled */
> +	prop = of_get_property(of_get_parent(np), "clock-frequency", &len);
> +	if (prop == NULL) {
> +		printk(KERN_ERR
> +		       "ibm-iic(%s):no clock-frequency prop on parent bus!\n",
> +		       dev->np->full_name);
> +		goto fail;
> +	}
> +
[ snip ]
> +	dev->clckdiv = iic_clckdiv(*prop);
> +	DBG("%s: clckdiv = %d\n", dev->np->full_name, dev->clckdiv);
> +
> +	/* Initialize IIC interface */
> +	iic_dev_init(dev);
> +
> +	/* Register it with i2c layer */
> +	adap = &dev->adap;
> +	adap->dev.parent = &ofdev->dev;
> +	strcpy(adap->name, "IBM IIC");
> +	i2c_set_adapdata(adap, dev);
> +	adap->id = I2C_HW_OCP;
> +	adap->class = I2C_CLASS_HWMON;
> +	adap->algo = &iic_algo;
> +	adap->client_register = NULL;
> +	adap->client_unregister = NULL;
> +	adap->timeout = 1;
> +	adap->retries = 1;
> +
> +	dev->idx = ++device_idx;
> +	adap->nr = dev->idx;
> +	if ((ret = i2c_add_numbered_adapter(adap)) < 0) {
[ snip ]

This stuff might be extracted into the common init function.
Something like this:

static int iic_adap_add(struct ibm_iic_private *dev, unsigned int opb)
{
	struct i2c_adapter *adap;

	/* clckdiv is the same for *all* IIC interfaces,
	 * but I'd rather make a copy than introduce another global. --ebs
	 */
	dev->clckdiv = iic_clckdiv(opb);
	DBG("%d: clckdiv = %d\n", dev->idx, dev->clckdiv);

	/* Initialize IIC interface */
	iic_dev_init(dev);

	/* Register it with i2c layer */
	adap = &dev->adap;
	strcpy(adap->name, "IBM IIC");
	i2c_set_adapdata(adap, dev);
	adap->id = I2C_HW_OCP;
	adap->class = I2C_CLASS_HWMON;
	adap->algo = &iic_algo;
	adap->client_register = NULL;
	adap->client_unregister = NULL;
	adap->timeout = 1;
	adap->retries = 1;
	return i2c_add_adapter(adap);
}

Though, I really don't care, since OCP part is dying.

> +		printk(KERN_CRIT"ibm-iic(%s): failed to register i2c adapter\n",
> +		       dev->np->full_name);
> +		goto fail;
> +	}
> +
> +	printk(KERN_INFO "ibm-iic(%s): using %s mode\n", dev->np->full_name,
> +	       dev->fast_mode ?
> +	       "fast (400 kHz)" : "standard (100 kHz)");
> +
> +	return 0;
> +
> +fail:
> +	if (dev->irq >= 0){
> +		iic_interrupt_mode(dev, 0);
> +		free_irq(dev->irq, dev);

Please, add a call to irq_dispose_mapping(dev->irq);

> +	}
> +
> +	iounmap(dev->vaddr);
> +fail2:
> +	release_mem_region(dev->paddr, sizeof(struct iic_regs));
> +fail1:
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +	kfree(dev);
> +
> +	return ret;
> +}
> +
> +/*
> + * Cleanup initialized IIC interface
> + */
> +static int __devexit iic_remove(struct of_device *ofdev)
> +{
> +	struct ibm_iic_private *dev =
> +		(struct ibm_iic_private *)dev_get_drvdata(&ofdev->dev);
> +	BUG_ON(dev == NULL);
> +	if (i2c_del_adapter(&dev->adap)){
> +		printk(KERN_CRIT "ibm-iic(%s): failed to delete i2c adapter\n",
> +		       dev->np->full_name);
> +		/* That's *very* bad, just shutdown IRQ ... */
> +		if (dev->irq >= 0){
> +			iic_interrupt_mode(dev, 0);
> +			free_irq(dev->irq, dev);
> +			dev->irq = -1;
> +		}
> +	} else {
> +		if (dev->irq >= 0){
> +			iic_interrupt_mode(dev, 0);
> +			free_irq(dev->irq, dev);
> +		}

This can be reorganized to switch to poll mode, diapose irq and check 
i2c_del_adapter retval after that, instead of duplicating the free_irq 
stuff.
Also irq_dispose_mapping(dev->irq); should be added:
.........
	int retval;

	retval = i2c_del_adapter(&dev->adap);
	if (dev->irq != NO_IRQ) {
		iic_interrupt_mode(dev, 0);
		free_irq(dev->irq, dev);
		irq_dispose_mapping(dev->irq);
	}

	if (retval) {
		printk(KERN_CRIT "ibm-iic(%s): failed to delete i2c adapter :(\n",
			dev->np->full_name);
		/* That's *very* bad, just shutdown IRQ ... */
		dev->irq = NO_IRQ;
		return 0;
	}
	iounmap(dev->vaddr);
.......

> +		iounmap(dev->vaddr);
> +		release_mem_region(dev->paddr, sizeof(struct iic_regs));
> +		kfree(dev);
> +	}
> +	return 0;
> +}
> +
> +static struct of_device_id ibm_iic_match[] = {
> +	{
> +		.type = "i2c",
> +		.compatible = "ibm,iic",
> +	},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, ibm_iic_match);
> +
> +static struct of_platform_driver ibm_iic_driver = {
> +	.name = "ibm-iic",
> +	.match_table = ibm_iic_match,
> +	.probe = iic_probe,
> +	.remove = iic_remove,
> +#if defined(CONFIG_PM)
> +	.suspend = NULL,
> +	.resume = NULL,
> +#endif
> +};
> +
> +static int __init iic_init(void)
> +{
> +	printk(KERN_INFO "IBM IIC driver v" DRIVER_VERSION "\n");
> +	return of_register_platform_driver(&ibm_iic_driver);
> +}
> +
> +static void __exit iic_exit(void)
> +{
> +	of_unregister_platform_driver(&ibm_iic_driver);
> +}
> +#else
> +/*
> + * OCP when used from arch/ppc
> + */
> +static int __devinit iic_probe(struct ocp_device *ocp)
> +{
>  	struct ibm_iic_private* dev;
>  	struct i2c_adapter* adap;
>  	struct ocp_func_iic_data* iic_data = ocp->def->additions;
> @@ -828,6 +1034,7 @@ static void __exit iic_exit(void)
>  {
>  	ocp_unregister_driver(&ibm_iic_driver);
>  }
> +#endif /* CONFIG_PPC_MERGE */
>  
>  module_init(iic_init);
>  module_exit(iic_exit);
> diff --git a/drivers/i2c/busses/i2c-ibm_iic.h b/drivers/i2c/busses/i2c-ibm_iic.h
> index fdaa482..c15b091 100644
> --- a/drivers/i2c/busses/i2c-ibm_iic.h
> +++ b/drivers/i2c/busses/i2c-ibm_iic.h
> @@ -50,6 +50,8 @@ struct ibm_iic_private {
>  	int irq;
>  	int fast_mode;
>  	u8  clckdiv;
> +	struct device_node *np;
> +	phys_addr_t paddr;
>  };
>  
>  /* IICx_CNTL register */

Thanks,
Valentine.