i2c: Add devtree-aware iic support for PPC4xx
PATCH 2/2 i2c: Add devtree-aware iic support for PPC4xx
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).
--- 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(-)
drivers/i2c/busses/Kconfig +1 -1
@@ -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.
drivers/i2c/busses/i2c-ibm_iic.c +208 -1
236 lines not on this page · raw message
drivers/i2c/busses/i2c-ibm_iic.h +2 -0
@@ -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 */
Signature · 1 line
1.5.3.4
On Mon, Oct 15, 2007 at 03:29:11PM +0200, Stefan Roese wrote:
<snip>
Quoted 4 lines
drivers/i2c/busses/i2c-ibm_iic.c +4
-0+#ifdef CONFIG_PPC_MERGE +static int device_idx =-1 ; +#endif +
<snip>
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 776
Quoted 2 lines
drivers/i2c/busses/i2c-ibm_iic.c +2
-0+ 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.
Signature · 1 line
Eugene
Quoted 18 lines · Eugene Surovegin
On Mon, Oct 15, 2007 at 03:29:11PM +0200, Stefan Roese wrote:
<snip>
Quoted 4 lines
drivers/i2c/busses/i2c-ibm_iic.c +4
-0+#ifdef CONFIG_PPC_MERGE +static int device_idx =-1 ; +#endif +<snip>
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 776
Quoted 2 lines
drivers/i2c/busses/i2c-ibm_iic.c +2
-0+ 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?
Quoted 5 lines
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_deffor 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.
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Quoted 8 lines · Grant Likely
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
Quoted 4 lines · Scott Wood
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.
Signature · 1 line
Eugene
Quoted 9 lines · Eugene Surovegin
Quoted 4 lines · Scott Wood
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?
Quoted 4 lines
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
Quoted 3 lines
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.
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Quoted 7 lines · Eugene Surovegin
Quoted 4 lines · Scott Wood
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.
Quoted 2 lines
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.
Quoted 3 lines
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
Quoted 5 lines · Scott Wood
Quoted 4 lines · Grant Likely
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):
Quoted 7 lines
Quoted 4 lines
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.
Quoted 5 lines
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.
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Quoted 10 lines · Grant Likely
Quoted 9 lines · Scott Wood
Quoted 8 lines · Grant Likely
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"; };
Quoted 5 lines
Quoted 2 lines
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.
Quoted 4 lines
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. :-)
Quoted 10 lines
Quoted 4 lines
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
Quoted 17 lines · Scott Wood
Quoted 10 lines · Grant Likely
Quoted 9 lines · Scott Wood
Quoted 8 lines · Grant Likely
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"; };Quoted 5 lines
Quoted 2 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 3 lines
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.
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Quoted 7 lines · Grant Likely
Quoted 3 lines · Scott Wood
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.
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
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?
Quoted 3 lines
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
Quoted 6 lines · Scott Wood
Quoted 5 lines · Grant Likely
Quoted 3 lines · Scott Wood
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.
Quoted 7 lines
Quoted 4 lines
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).
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
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).
Quoted 10 lines
Quoted 4 lines
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.
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Quoted 9 lines · Grant Likely
Quoted 3 lines · Scott Wood
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.
Quoted 5 lines
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
Quoted 20 lines · Grant Likely
Quoted 5 lines · Scott Wood
Quoted 4 lines · Grant Likely
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):
Quoted 7 lines
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 14 lines
Quoted 4 lines
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.
Signature · 4 lines
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
Quoted 8 lines · David Gibson
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.
Quoted 6 lines
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! :-)
Quoted 11 lines
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.
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Quoted 7 lines · Grant Likely
Quoted 6 lines · David Gibson
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.)
Quoted 5 lines
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.
Signature · 1 line
Jean Delvare
Quoted 9 lines · Jean Delvare
Quoted 7 lines · Grant Likely
Quoted 6 lines · David Gibson
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.
Quoted 3 lines
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.
Quoted 10 lines
Quoted 5 lines
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
_atall_. Don't usei2c_add_numbered_adapter, usei2c_add_adapterand let i2c-core choose an appropriate a bus number.
Signature · 4 lines
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
Quoted 7 lines · Stefan Roese
This patch reworks existing ibm-iic driver to support
of_platform_deviceand enables it to talk to device tree directly. The "old" OCP interface forarch/ppcis still supported via #ifdef's and shall be removed whenarch/ppcis gone in a few months.This is done to enable I2C support for the PPC4xx platforms now being moved from
arch/ppc(ocp) toarch/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.
Quoted 286 lines
Signed-off-by Stefan Roese <[email protected]>--- 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(-)drivers/i2c/busses/Kconfig +1
-1@@ -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.drivers/i2c/busses/i2c-ibm_iic.c +208
-1@@ -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);drivers/i2c/busses/i2c-ibm_iic.h +2
-1@@ -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
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Signature · 4 lines
Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [email protected] (403) 399-0195
Please, take a look at my comments below
Quoted 79 lines · Stefan Roese
This patch reworks existing ibm-iic driver to support
of_platform_deviceand enables it to talk to device tree directly. The "old" OCP interface forarch/ppcis still supported via #ifdef's and shall be removed whenarch/ppcis gone in a few months.This is done to enable I2C support for the PPC4xx platforms now being moved from
arch/ppc(ocp) toarch/powerpc(of).Signed-off-by Stefan Roese <[email protected]>--- 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(-)drivers/i2c/busses/Kconfig +1
-1@@ -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.drivers/i2c/busses/i2c-ibm_iic.c +25
-1@@ -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;
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 698
Quoted 10 lines
drivers/i2c/busses/i2c-ibm_iic.c +10
-0+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.
Quoted 15 lines
drivers/i2c/busses/i2c-ibm_iic.c +15
-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 ) {
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 723
Quoted 10 lines
drivers/i2c/busses/i2c-ibm_iic.c +10
-0+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)
Quoted 32 lines
drivers/i2c/busses/i2c-ibm_iic.c +32
-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 ]
Quoted 12 lines
drivers/i2c/busses/i2c-ibm_iic.c +12
-0+ 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 ;
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 777
Quoted 10 lines
drivers/i2c/busses/i2c-ibm_iic.c +10
-0+ 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.
Quoted 5 lines
drivers/i2c/busses/i2c-ibm_iic.c +5
-0+printk (KERN_CRIT "ibm-iic(%s): failed to register i2c adapter\n" , + dev->np->full_name); + goto fail; + } +
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 792
Quoted 10 lines
drivers/i2c/busses/i2c-ibm_iic.c +10
-0+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);
Quoted 24 lines
drivers/i2c/busses/i2c-ibm_iic.c +24
-0+ } + +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 ... */
drivers/i2c/busses/i2c-ibm_iic.c static inline u8 iic_clckdiv(unsigned int opb) · line 826
Quoted 10 lines
drivers/i2c/busses/i2c-ibm_iic.c +10
-0+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);.......
Quoted 60 lines
drivers/i2c/busses/i2c-ibm_iic.c +45
-0+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);drivers/i2c/busses/i2c-ibm_iic.h +0
-0
drivers/i2c/busses/i2c-ibm_iic.h struct ibm_iic_private { · line 57
Quoted 8 lines
drivers/i2c/busses/i2c-ibm_iic.h +2
-0@@ -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.