| Message ID | 1322760781-31226-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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
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.
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
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.
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
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? 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. 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.
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); }
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.
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
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.
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;
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(-)