[PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Timur Tabi]

An I2C device tree node can contain a 'cell-index' property that can be
used to enumerate the I2C devices.  If such a property exists, use it
to specify the I2C adapter number.

This feature is necessary for the Freescale PowerPC audio drivers (e.g.
on the P1022DS).  The "machine driver" needs to know the adapter number
for each I2C adapter, but it only has access to the device tree.
Previously, the I2C nodes always appeared in cell-index order, so the
dynamic numbering coincided with the cell-index property.  With commit
ab827d97 ("powerpc/85xx: Rework P1022DS device tree"), the I2C nodes are
unintentionally reversed in the device tree, and so the machine driver
guesses the wrong I2C adapter number.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/i2c/busses/i2c-mpc.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 107397a..8551c34 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -632,7 +632,19 @@ static int __devinit fsl_i2c_probe(struct platform_device *op)
 	i2c->adap.dev.parent = &op->dev;
 	i2c->adap.dev.of_node = of_node_get(op->dev.of_node);
 
-	result = i2c_add_adapter(&i2c->adap);
+	/*
+	 * If the I2C node has a "cell-index" property, use it to enumerate
+	 * the I2C adapter.
+	 */
+	prop = of_get_property(i2c->adap.dev.of_node, "cell-index", &plen);
+	if (prop && plen == sizeof(u32)) {
+		dev_dbg(i2c->dev, "using cell-index property %u", *prop);
+		i2c->adap.nr = *prop;
+		result = i2c_add_numbered_adapter(&i2c->adap);
+	} else {
+		result = i2c_add_adapter(&i2c->adap);
+	}
+
 	if (result < 0) {
 		dev_err(i2c->dev, "failed to add adapter\n");
 		goto fail_add;
-- 
1.7.3.4

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Scott Wood]

On 12/01/2011 11:33 AM, Timur Tabi wrote:
> An I2C device tree node can contain a 'cell-index' property that can be
> used to enumerate the I2C devices.  If such a property exists, use it
> to specify the I2C adapter number.

Didn't we decide a long time ago that this was a bad idea?

> This feature is necessary for the Freescale PowerPC audio drivers (e.g.
> on the P1022DS).  The "machine driver" needs to know the adapter number
> for each I2C adapter, but it only has access to the device tree.
> Previously, the I2C nodes always appeared in cell-index order, so the
> dynamic numbering coincided with the cell-index property.  With commit
> ab827d97 ("powerpc/85xx: Rework P1022DS device tree"), the I2C nodes are
> unintentionally reversed in the device tree, and so the machine driver
> guesses the wrong I2C adapter number.

What specifically do you need this number for?  What does it represent?

-Scott

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Timur Tabi]

Scott Wood wrote:
> On 12/01/2011 11:33 AM, Timur Tabi wrote:
>> An I2C device tree node can contain a 'cell-index' property that can be
>> used to enumerate the I2C devices.  If such a property exists, use it
>> to specify the I2C adapter number.
> 
> Didn't we decide a long time ago that this was a bad idea?

I don't see what's wrong with it.

>> This feature is necessary for the Freescale PowerPC audio drivers (e.g.
>> on the P1022DS).  The "machine driver" needs to know the adapter number
>> for each I2C adapter, but it only has access to the device tree.
>> Previously, the I2C nodes always appeared in cell-index order, so the
>> dynamic numbering coincided with the cell-index property.  With commit
>> ab827d97 ("powerpc/85xx: Rework P1022DS device tree"), the I2C nodes are
>> unintentionally reversed in the device tree, and so the machine driver
>> guesses the wrong I2C adapter number.
> 
> What specifically do you need this number for?  What does it represent?

Well, I thought the above paragraph explained it pretty well.

Audio drivers come in sets of four -- machine, ssi, dma, and codec.  The machine driver needs to know which ssi is connected to which codec and which DMA channel(s) it needs to use.  So I extract all this information from the device tree.  

The individual drivers, however, register only with their own subsystem.  So the codec driver, for example, doesn't know anything about device trees -- it just registers as an i2c driver.  That means that the machine driver has to guess what name the codec driver will use when it registers.  In this case, that's the i2c device name, which include the I2C adapter number (adap->nr).  That means that given a device tree node, I need to know what the actual bus number is.

An alternative approach is to create a function like this:

	struct i2c_adapter *i2c_adapter_from_node(struct device_node *np);

I could then just use adap->nr directly.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Scott Wood]

On 12/01/2011 02:54 PM, Timur Tabi wrote:
> Scott Wood wrote:
>> On 12/01/2011 11:33 AM, Timur Tabi wrote:
>>> An I2C device tree node can contain a 'cell-index' property that can be
>>> used to enumerate the I2C devices.  If such a property exists, use it
>>> to specify the I2C adapter number.
>>
>> Didn't we decide a long time ago that this was a bad idea?
> 
> I don't see what's wrong with it.

Obviously, since you're suggesting doing it. :-P

Did you search for the old discussions?

How is this going to interact with other i2c buses (e.g. on a board
FPGA) that might have a conflicting static numbering scheme?  Have you
ensured that no dynamic bus registrations (e.g. an i2c bus on a PCI
device) can happen before the static SoC i2c buses are added?

Global numberspaces of this sort are usually the wrong answer.  Look at
the mess it made with IRQ numbers, and the gymnastics we go through to
virtualize it.

>>> This feature is necessary for the Freescale PowerPC audio drivers (e.g.
>>> on the P1022DS).  The "machine driver" needs to know the adapter number
>>> for each I2C adapter, but it only has access to the device tree.
>>> Previously, the I2C nodes always appeared in cell-index order, so the
>>> dynamic numbering coincided with the cell-index property.  With commit
>>> ab827d97 ("powerpc/85xx: Rework P1022DS device tree"), the I2C nodes are
>>> unintentionally reversed in the device tree, and so the machine driver
>>> guesses the wrong I2C adapter number.
>>
>> What specifically do you need this number for?  What does it represent?
> 
> Well, I thought the above paragraph explained it pretty well.

All you said was "needs to know the adapter number", nothing about why.

> Audio drivers come in sets of four -- machine, ssi, dma, and codec.
> The machine driver needs to know which ssi is connected to which
> codec and which DMA channel(s) it needs to use.  So I extract all
> this information from the device tree.
>
> The individual drivers, however, register only with their own
> subsystem.  So the codec driver, for example, doesn't know anything
> about device trees -- it just registers as an i2c driver.  That means
> that the machine driver has to guess what name the codec driver will
> use when it registers.  In this case, that's the i2c device name,
> which include the I2C adapter number (adap->nr).  That means that
> given a device tree node, I need to know what the actual bus number
> is.
> 
> An alternative approach is to create a function like this:
> 
> 	struct i2c_adapter *i2c_adapter_from_node(struct device_node *np);
> 
> I could then just use adap->nr directly.

If there isn't a way to get a "struct device" from "struct device_node",
we should add it.  From that you should be able to look up the sysfs
name -- no need to mess around with adap->nr as an intermediary.

-Scott

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Timur Tabi]

Scott Wood wrote:

> How is this going to interact with other i2c buses (e.g. on a board
> FPGA) that might have a conflicting static numbering scheme?  Have you
> ensured that no dynamic bus registrations (e.g. an i2c bus on a PCI
> device) can happen before the static SoC i2c buses are added?

Hmm.... You have a point there.

>> An alternative approach is to create a function like this:
>>
>> 	struct i2c_adapter *i2c_adapter_from_node(struct device_node *np);
>>
>> I could then just use adap->nr directly.
> 
> If there isn't a way to get a "struct device" from "struct device_node",
> we should add it. 

How do I do that?  Scan all the struct devices until I find one where dev->of_node == np?  That seems really inefficient.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Scott Wood]

On 12/01/2011 03:46 PM, Timur Tabi wrote:
> Scott Wood wrote:
> 
>> How is this going to interact with other i2c buses (e.g. on a board
>> FPGA) that might have a conflicting static numbering scheme?  Have you
>> ensured that no dynamic bus registrations (e.g. an i2c bus on a PCI
>> device) can happen before the static SoC i2c buses are added?
> 
> Hmm.... You have a point there.
> 
>>> An alternative approach is to create a function like this:
>>>
>>> 	struct i2c_adapter *i2c_adapter_from_node(struct device_node *np);
>>>
>>> I could then just use adap->nr directly.
>>
>> If there isn't a way to get a "struct device" from "struct device_node",
>> we should add it. 
> 
> How do I do that?  Scan all the struct devices until I find one where dev->of_node == np?  That seems really inefficient.

Ideally we would have a field in struct device_node that points to
struct device.

-Scott

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Grant Likely]

On Thu, Dec 1, 2011 at 2:52 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 12/01/2011 03:46 PM, Timur Tabi wrote:
>> Scott Wood wrote:
>>
>>> How is this going to interact with other i2c buses (e.g. on a board
>>> FPGA) that might have a conflicting static numbering scheme? =A0Have yo=
u
>>> ensured that no dynamic bus registrations (e.g. an i2c bus on a PCI
>>> device) can happen before the static SoC i2c buses are added?
>>
>> Hmm.... You have a point there.
>>
>>>> An alternative approach is to create a function like this:
>>>>
>>>> =A0 =A0 struct i2c_adapter *i2c_adapter_from_node(struct device_node *=
np);
>>>>
>>>> I could then just use adap->nr directly.
>>>
>>> If there isn't a way to get a "struct device" from "struct device_node"=
,
>>> we should add it.
>>
>> How do I do that? =A0Scan all the struct devices until I find one where =
dev->of_node =3D=3D np? =A0That seems really inefficient.
>
> Ideally we would have a field in struct device_node that points to
> struct device.

No.  There are cases where multiple devices may reference the same node.

It is better to walk the list of i2c adapters and look for one that
has the matching node pointer.  It really isn't an expensive operation
to do it that way.

g.

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Timur Tabi]

Grant Likely wrote:
> It is better to walk the list of i2c adapters and look for one that
> has the matching node pointer.  It really isn't an expensive operation
> to do it that way.

That's what I was thinking.  I can't figure out how to walk the list, though.  i2c_get_adapter() takes an adapter number, but I don't think this is going to work:

unsigned int i = 0;
struct i2c_adapter adap = i2c_get_adapter(0);

while (adap) {
	if (adap->nr == nr)
		break;
	adap = i2c_get_adapter(++i);
}

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Timur Tabi]

Scott Wood wrote:
> Ideally we would have a field in struct device_node that points to
> struct device.

I don't think there's a single place in the code where the connection between the device and device_node is made.  In fact, I think dev->of_node needs to be manually initialized by the driver during the OF probe.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Scott Wood]

On 12/01/2011 03:59 PM, Timur Tabi wrote:
> Scott Wood wrote:
>> Ideally we would have a field in struct device_node that points to
>> struct device.
> 
> I don't think there's a single place in the code where the connection
> between the device and device_node is made.  In fact, I think
> dev->of_node needs to be manually initialized by the driver during
> the OF probe.

The i2c core does this.

Does of_find_i2c_device_by_node() do what you want?

-Scott

Re: [PATCH] i2c-mpc: use the cell-index property to enumerate the I2C adapters

[Timur Tabi]

Scott Wood wrote:
> 
> Does of_find_i2c_device_by_node() do what you want?

Yeah, that'll work.  I wasn't thinking about looking for an i2c device.  Thanks.

-- 
Timur Tabi
Linux kernel developer at Freescale