Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jon Smirl wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: One side effect is that legacy style i2c drivers won't work anymore. If you mean legacy-style client drivers, why not? i2c_new_device() doesn't work with legacy-style client drivers. No, but they should still work the old way. I'm not in favor trying to support both legacy and new style i2c drivers. I don't understand what it is that you did that would break support for legacy clients, though. It took me all of five minutes to convert an existing legacy driver to the new style. Pretty much all you need to do is delete code (about 100 lines). So I'd recommend converting the drivers we are interest in instead of trying to support both types. Sure, conversion is good, but that doesn't mean we want things to suddenly break for users. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Hi Scott, Jon, On Mon, 05 Nov 2007 14:51:51 -0600, Scott Wood wrote: Jon Smirl wrote: How about renaming the old driver file and leaving it hooked to ppc? Then it would get deleted when ppc goes away. That would let work progress on the powerpc version. Or we could have one driver that has two probe methods. I don't like forking the driver. I agree with Scott here, I don't want to fork the drivers. It is possible (and easy) to support both methods in the same module, let's just to that. See for example David Brownell's work on the lm75 driver: http://lists.lm-sensors.org/pipermail/lm-sensors/2007-September/021270.html i2c_new_device() doesn't work with legacy-style client drivers. No, but they should still work the old way. Definitely. This is not hard to do but the i2c people will have to agree. I need to change the i2c_driver structure to include the additional names. I got a fair bit of resistance from them on the topic of multiple match names for i2c clients. Really? All I said is that you were a bit late in the game because this had been discussed before. I know that David Brownell doesn't agree with you (he designed what we have now), but me, I am still open to discussing the matter, especially when more people complain about the situation every month. We might as well just use i2c_new_device() instead of messing around with bus numbers. Note that this is technically no longer platform code, so it's harder to justify claiming the static numberspace. I was allowing control of the bus number with cell-index and i2c_add_numbered_adapter(). Should I get rid of this and switch to i2c_add_adapter()? Yes. No! If you don't call i2c_add_numbered_adapter() then new-style i2c clients will never work on your i2c adapter. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jean Delvare wrote: We might as well just use i2c_new_device() instead of messing around with bus numbers. Note that this is technically no longer platform code, so it's harder to justify claiming the static numberspace. I was allowing control of the bus number with cell-index and i2c_add_numbered_adapter(). Should I get rid of this and switch to i2c_add_adapter()? Yes. No! If you don't call i2c_add_numbered_adapter() then new-style i2c clients will never work on your i2c adapter. I thought that was what i2c_new_device() was for? By handling all the device tree stuff in the driver, it acts more like an add-on adapter than a platform device. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/6/07, Jean Delvare [EMAIL PROTECTED] wrote: Hi Scott, Jon, On Mon, 05 Nov 2007 14:51:51 -0600, Scott Wood wrote: Jon Smirl wrote: How about renaming the old driver file and leaving it hooked to ppc? Then it would get deleted when ppc goes away. That would let work progress on the powerpc version. Or we could have one driver that has two probe methods. I don't like forking the driver. I agree with Scott here, I don't want to fork the drivers. It is possible (and easy) to support both methods in the same module, let's just to that. See for example David Brownell's work on the lm75 driver: http://lists.lm-sensors.org/pipermail/lm-sensors/2007-September/021270.html I agree that it is easy to make make a chip driver support both new and old style. But when I call i2c_new_device() on an old style chip driver it exits saying that it doesn't work for the old style adapters. Checks for is_newstyle_driver() are in the i2c_new_device code. That's what caused me to rewrite the rtc-pcf8563 driver for the new style. This probably related to probing, I have to pass the address in struct i2c_board_info. The old style drivers don't support having their address passed in. This may be complicated by the fact that the rtc drivers I'm working on are not probable. That's why I want to add device tree support for them. If this is going to work on an old style driver, how do I get the address to it? -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Hi Scott, On Tue, 06 Nov 2007 11:36:23 -0600, Scott Wood wrote: Jean Delvare wrote: We might as well just use i2c_new_device() instead of messing around with bus numbers. Note that this is technically no longer platform code, so it's harder to justify claiming the static numberspace. I was allowing control of the bus number with cell-index and i2c_add_numbered_adapter(). Should I get rid of this and switch to i2c_add_adapter()? Yes. No! If you don't call i2c_add_numbered_adapter() then new-style i2c clients will never work on your i2c adapter. I thought that was what i2c_new_device() was for? Sorry, I've not been completely clear. Yes, you can use i2c_new_device() on an adapter that has been added with i2c_add_adapter(). However, this requires that you have a reference to that i2c_adapter, which is usually not the case with system-wide I2C buses. Embedded platforms would rather use i2c_add_numbered_adapter(), give a list of chips to i2c_register_board_info() and let i2c-core instantiate them. i2c_new_device was primarily meant for multimedia adapters. By handling all the device tree stuff in the driver, it acts more like an add-on adapter than a platform device. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Hi Jon, On Tue, 6 Nov 2007 12:45:24 -0500, Jon Smirl wrote: On 11/6/07, Jean Delvare wrote: I agree with Scott here, I don't want to fork the drivers. It is possible (and easy) to support both methods in the same module, let's just to that. See for example David Brownell's work on the lm75 driver: http://lists.lm-sensors.org/pipermail/lm-sensors/2007-September/021270.html I agree that it is easy to make make a chip driver support both new and old style. But when I call i2c_new_device() on an old style chip driver it exits saying that it doesn't work for the old style adapters. Checks for is_newstyle_driver() are in the i2c_new_device code. That's what caused me to rewrite the rtc-pcf8563 driver for the new style. This probably related to probing, I have to pass the address in struct i2c_board_info. The old style drivers don't support having their address passed in. I know that. The trick is to register two struct i2c_driver (again see the lm75 example), one old-style, one new-style. I agree it's not very elegant, but it works. Hopefully we can get rid of the old-style one after some time, and it allows for a smooth transition. This may be complicated by the fact that the rtc drivers I'm working on are not probable. That's why I want to add device tree support for them. If this is going to work on an old style driver, how do I get the address to it? Old-style drivers probe for supported chips on all possible addresses (for the chip in question). If the chip can't be probed, then module parameters must be used. That's not terribly convenient, and new-style drivers are much preferred in this case. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/6/07, Jean Delvare [EMAIL PROTECTED] wrote: Hi Scott, On Tue, 06 Nov 2007 11:36:23 -0600, Scott Wood wrote: Jean Delvare wrote: We might as well just use i2c_new_device() instead of messing around with bus numbers. Note that this is technically no longer platform code, so it's harder to justify claiming the static numberspace. I was allowing control of the bus number with cell-index and i2c_add_numbered_adapter(). Should I get rid of this and switch to i2c_add_adapter()? Yes. No! If you don't call i2c_add_numbered_adapter() then new-style i2c clients will never work on your i2c adapter. I thought that was what i2c_new_device() was for? Sorry, I've not been completely clear. Yes, you can use i2c_new_device() on an adapter that has been added with i2c_add_adapter(). However, this requires that you have a reference to that i2c_adapter, which is usually not the case with system-wide I2C buses. Embedded platforms would rather use i2c_add_numbered_adapter(), give a list of chips to i2c_register_board_info() and let i2c-core instantiate them. i2c_new_device was primarily meant for multimedia adapters. *Some* embedded platforms would rather use i2c_add_numbered_adapter(). :-) On powerpc, and other platforms which have a device tree, we don't need to define a table of devices in the platform code because we've already got a rich structure for describing such things. The i2c busses and i2c devices are grouped together in the device tree, so when the i2c bus is probed, it should call out to common i2c device tree parsing code to instantiate all the devices described in the tree. It would be awkward to describe the i2c bus in the device tree but still have to use a static structure to describe the devices on that bus. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jean Delvare wrote: Sorry, I've not been completely clear. Yes, you can use i2c_new_device() on an adapter that has been added with i2c_add_adapter(). However, this requires that you have a reference to that i2c_adapter, which is usually not the case with system-wide I2C buses. But it is the case here, because the i2c driver knows about the device tree, and thus can pass the device tree node and the adapter struct to the enumeration function. The driver should still do i2c_add_numbered_adapter() when using the non-OF platform device binding, in which case it gets the bus number from the platform data. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jean Delvare wrote: On Mon, 05 Nov 2007 21:52:06 +, Matt Sealey wrote: Well, all i2c devices have a chip id you can probe for (...) This statement is completely incorrect. I2C devices do NOT have standard ID registers. Some devices have proprietary ID registers, some don't, it's really up to the manfacturer. All I2C slave devices have to have a 7- or 10-bit address to identify them by. They *may* not report what they ARE, but this is 9 times out of 10 a hardware design decision of soldering the chip to a board and the address is then coded into device trees or hardcoded into drivers. Whoever designed the board and has the datasheets knows the address they're supposed to be at, and the device can accept this. You simply cannot entertain an i2c bus with anonymous and unnumbered devices, every one has to have an address it responds to, however it is defined, or it just does not work. WRT cell-index this is an index of the bus on the chip (not the logical i2c bus but the physical difference between two i2c controllers) and then any i2c devices which need to be communicated with would be child nodes, their reg property reflecting their slave address, is that not correct? -- Matt Sealey [EMAIL PROTECTED] Genesi, Manager, Developer Relations ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/6/07, Grant Likely [EMAIL PROTECTED] wrote: On 11/6/07, Jean Delvare [EMAIL PROTECTED] wrote: Hi Scott, On Tue, 06 Nov 2007 11:36:23 -0600, Scott Wood wrote: Jean Delvare wrote: We might as well just use i2c_new_device() instead of messing around with bus numbers. Note that this is technically no longer platform code, so it's harder to justify claiming the static numberspace. I was allowing control of the bus number with cell-index and i2c_add_numbered_adapter(). Should I get rid of this and switch to i2c_add_adapter()? Yes. No! If you don't call i2c_add_numbered_adapter() then new-style i2c clients will never work on your i2c adapter. I thought that was what i2c_new_device() was for? Sorry, I've not been completely clear. Yes, you can use i2c_new_device() on an adapter that has been added with i2c_add_adapter(). However, this requires that you have a reference to that i2c_adapter, which is usually not the case with system-wide I2C buses. Embedded platforms would rather use i2c_add_numbered_adapter(), give a list of chips to i2c_register_board_info() and let i2c-core instantiate them. i2c_new_device was primarily meant for multimedia adapters. *Some* embedded platforms would rather use i2c_add_numbered_adapter(). :-) On powerpc, and other platforms which have a device tree, we don't Specifically; an OF style device tree. :-) g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Second pass at extending i2c core to accept strings of aliases for the
module. This version eliminate the need for separate name and type
fields when selecting a driver. PowerPC has to have a mapping from
device tree names to the i2c drivers, it makes sense to keep this
mapping inside the i2c driver.
Extend i2c-core to support lists of device tree compatible names when
matching drivers
From: Jon Smirl [EMAIL PROTECTED]
---
drivers/i2c/busses/i2c-mpc.c | 37 +++--
drivers/i2c/i2c-core.c | 35 ++-
drivers/rtc/rtc-pcf8563.c|1 +
drivers/rtc/rtc-rs5c372.c|3 ++-
include/linux/i2c.h | 13 +
5 files changed, 37 insertions(+), 52 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4ddebe4..30420ad 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -312,34 +312,6 @@ static struct i2c_adapter mpc_ops = {
.retries = 1
};
-struct i2c_driver_device {
- char*of_device;
- char*i2c_driver;
- char*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] = {
- {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
- {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
- {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
- {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
- {epson,pcf8564, rtc-pcf8563, pcf8564,},
-};
-
-static int of_find_i2c_driver(struct device_node *node, struct
i2c_board_info *info)
-{
- int i;
-
- for (i = 0; i ARRAY_SIZE(i2c_devices); i++) {
- if (!of_device_is_compatible(node, i2c_devices[i].of_device))
- continue;
- strncpy(info-driver_name, i2c_devices[i].i2c_driver,
KOBJ_NAME_LEN);
- strncpy(info-type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
- return 0;
- }
- return -ENODEV;
-}
-
static void of_register_i2c_devices(struct i2c_adapter *adap, struct
device_node *adap_node)
{
struct device_node *node = NULL;
@@ -347,11 +319,12 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
while ((node = of_get_next_child(adap_node, node))) {
struct i2c_board_info info;
const u32 *addr;
+ const char *compatible;
int len;
addr = of_get_property(node, reg, len);
if (!addr || len sizeof(int) || *addr (1 10) - 1) {
- printk(KERN_WARNING i2c-mpc.c: invalid i2c device
entry\n);
+ printk(KERN_WARNING i2c-mpc.c: invalid entry, missing
reg attribute\n);
continue;
}
@@ -359,8 +332,12 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
if (info.irq == NO_IRQ)
info.irq = -1;
- if (of_find_i2c_driver(node, info) 0)
+ compatible = of_get_property(node, compatible, len);
+ if (!compatible) {
+ printk(KERN_WARNING i2c-mpc.c: invalid entry, missing
compatible
attribute\n);
continue;
+ }
+ strncpy(info.name, compatible, sizeof(info.name));
info.platform_data = NULL;
info.addr = *addr;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d663e69..d9a70c2 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -17,7 +17,7 @@
Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */
/* - */
-/* With some changes from Kyösti Mälkki [EMAIL PROTECTED].
+/* With some changes from Kyösti MÀlkki [EMAIL PROTECTED].
All SMBus-related things are written by Frodo Looijaard [EMAIL PROTECTED]
SMBus 2.0 support by Mark Studebaker [EMAIL PROTECTED] and
Jean Delvare [EMAIL PROTECTED] */
@@ -51,6 +51,7 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
{
struct i2c_client *client = to_i2c_client(dev);
struct i2c_driver *driver = to_i2c_driver(drv);
+ char const **alias;
/* make legacy i2c drivers bypass driver model probing entirely;
* such drivers scan each i2c adapter/bus themselves.
@@ -60,8 +61,18 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
/* new style drivers use the same kind of driver matching policy
* as platform devices or SPI: compare device and driver IDs.
+* Match against arrary of alias device tree names. When a match
+* is found change the reference to point at the copy inside the
+* chip driver allowing the caller's string to be freed.
*/
- return strcmp(client-driver_name, drv-name) == 0;
+ alias = driver-aliases;
+ while (*alias) {
+
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Second pass on rework of mpc-i2c.c. This doesn't include code for
supporting both old and new style chip drivers. I'm still not a fan of
cluttering up mpc-i2c.c to support old style drivers since it so easy
to convert them to the new style. I'd rather just fix up the i2c
drivers used by chips on the PowerPC platform.
Convert i2c to of_platform_driver from platform_driver
From: Jon Smirl [EMAIL PROTECTED]
Improve error returns
---
arch/powerpc/sysdev/fsl_soc.c | 116 ---
drivers/i2c/busses/i2c-mpc.c | 178 +
2 files changed, 126 insertions(+), 168 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..6f80216 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -306,122 +306,6 @@ err:
arch_initcall(gfar_of_init);
-#ifdef CONFIG_I2C_BOARDINFO
-#include linux/i2c.h
-struct i2c_driver_device {
- char*of_device;
- char*i2c_driver;
- char*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] __initdata = {
- {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
- {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
- {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
- {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
-};
-
-static int __init of_find_i2c_driver(struct device_node *node, struct
i2c_board_info *info)
-{
- int i;
-
- for (i = 0; i ARRAY_SIZE(i2c_devices); i++) {
- if (!of_device_is_compatible(node, i2c_devices[i].of_device))
- continue;
- strncpy(info-driver_name, i2c_devices[i].i2c_driver,
KOBJ_NAME_LEN);
- strncpy(info-type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
- return 0;
- }
- return -ENODEV;
-}
-
-static void __init of_register_i2c_devices(struct device_node
*adap_node, int bus_num)
-{
- struct device_node *node = NULL;
-
- while ((node = of_get_next_child(adap_node, node))) {
- struct i2c_board_info info;
- const u32 *addr;
- int len;
-
- addr = of_get_property(node, reg, len);
- if (!addr || len sizeof(int) || *addr (1 10) - 1) {
- printk(KERN_WARNING fsl_ioc.c: invalid i2c device
entry\n);
- continue;
- }
-
- info.irq = irq_of_parse_and_map(node, 0);
- if (info.irq == NO_IRQ)
- info.irq = -1;
-
- if (of_find_i2c_driver(node, info) 0)
- continue;
-
- info.platform_data = NULL;
- info.addr = *addr;
-
- i2c_register_board_info(bus_num, info, 1);
- }
-}
-
-static int __init fsl_i2c_of_init(void)
-{
- struct device_node *np;
- unsigned int i;
- struct platform_device *i2c_dev;
- int ret;
-
- for (np = NULL, i = 0;
-(np = of_find_compatible_node(np, i2c, fsl-i2c)) != NULL;
-i++) {
- struct resource r[2];
- struct fsl_i2c_platform_data i2c_data;
- const unsigned char *flags = NULL;
-
- memset(r, 0, sizeof(r));
- memset(i2c_data, 0, sizeof(i2c_data));
-
- ret = of_address_to_resource(np, 0, r[0]);
- if (ret)
- goto err;
-
- of_irq_to_resource(np, 0, r[1]);
-
- i2c_dev = platform_device_register_simple(fsl-i2c, i, r, 2);
- if (IS_ERR(i2c_dev)) {
- ret = PTR_ERR(i2c_dev);
- goto err;
- }
-
- i2c_data.device_flags = 0;
- flags = of_get_property(np, dfsrr, NULL);
- if (flags)
- i2c_data.device_flags |= FSL_I2C_DEV_SEPARATE_DFSRR;
-
- flags = of_get_property(np, fsl5200-clocking, NULL);
- if (flags)
- i2c_data.device_flags |= FSL_I2C_DEV_CLOCK_5200;
-
- ret =
- platform_device_add_data(i2c_dev, i2c_data,
-sizeof(struct
- fsl_i2c_platform_data));
- if (ret)
- goto unreg;
-
- of_register_i2c_devices(np, i);
- }
-
- return 0;
-
-unreg:
- platform_device_unregister(i2c_dev);
-err:
- return ret;
-}
-
-arch_initcall(fsl_i2c_of_init);
-#endif
#ifdef CONFIG_PPC_83xx
static int __init mpc83xx_wdt_init(void)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d8de4ac..4ddebe4 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -17,7 +17,7 @@
#include linux/module.h
#include linux/sched.h
#include linux/init.h
-#include linux/platform_device.h
+#include linux/of_platform.h
#include asm/io.h
#include linux/fsl_devices.h
@@
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On Tue, 6 Nov 2007 11:26:14 -0700, Grant Likely wrote: On 11/6/07, Jean Delvare [EMAIL PROTECTED] wrote: Sorry, I've not been completely clear. Yes, you can use i2c_new_device() on an adapter that has been added with i2c_add_adapter(). However, this requires that you have a reference to that i2c_adapter, which is usually not the case with system-wide I2C buses. Embedded platforms would rather use i2c_add_numbered_adapter(), give a list of chips to i2c_register_board_info() and let i2c-core instantiate them. i2c_new_device was primarily meant for multimedia adapters. *Some* embedded platforms would rather use i2c_add_numbered_adapter(). :-) On powerpc, and other platforms which have a device tree, we don't need to define a table of devices in the platform code because we've already got a rich structure for describing such things. The i2c busses and i2c devices are grouped together in the device tree, so when the i2c bus is probed, it should call out to common i2c device tree parsing code to instantiate all the devices described in the tree. It would be awkward to describe the i2c bus in the device tree but still have to use a static structure to describe the devices on that bus. Ah, OK, thanks for the clarification. Then indeed using i2c_add_adapter() will work fine, agreed. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Hi Matt, On Tue, 06 Nov 2007 18:53:11 +, Matt Sealey wrote: Jean Delvare wrote: On Mon, 05 Nov 2007 21:52:06 +, Matt Sealey wrote: Well, all i2c devices have a chip id you can probe for (...) This statement is completely incorrect. I2C devices do NOT have standard ID registers. Some devices have proprietary ID registers, some don't, it's really up to the manfacturer. All I2C slave devices have to have a 7- or 10-bit address to identify them by. They *may* not report what they ARE, but this is 9 times out of 10 a hardware design decision of soldering the chip to a board and the address is then coded into device trees or hardcoded into drivers. Whoever designed the board and has the datasheets knows the address they're supposed to be at, and the device can accept this. You simply cannot entertain an i2c bus with anonymous and unnumbered devices, every one has to have an address it responds to, however it is defined, or it just does not work. Of course, but it is all about addressing, NOT identifying. WRT cell-index this is an index of the bus on the chip (not the logical i2c bus but the physical difference between two i2c controllers) and then any i2c devices which need to be communicated with would be child nodes, their reg property reflecting their slave address, is that not correct? I am not familiar with the OF tree, I can't tell, sorry. -- Jean Delvare ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On Tue, Nov 06, 2007 at 02:02:12PM -0500, Jon Smirl wrote: Second pass at extending i2c core to accept strings of aliases for the [snip] -/* With some changes from Kyösti Mälkki [EMAIL PROTECTED]. +/* With some changes from Kyösti MÀlkki [EMAIL PROTECTED]. This looks like an unrelated change of character encoding has slipped in here. -- 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On Mon, Nov 05, 2007 at 03:46:45PM -0700, Grant Likely wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: This is my first pass at reworking the Freescale i2c driver. It switches the driver from being a platform driver to an open firmware one. I've checked it out on my hardware and it is working. We may want to hold off on this until arch/ppc goes away (or at least all users of this driver in arch/ppc). How about renaming the old driver file and leaving it hooked to ppc? Then it would get deleted when ppc goes away. That would let work progress on the powerpc version. Or we could have one driver that has two probe methods. I don't like forking the driver. I agree. This driver can and should have multiple bus bindings. cell-index = 1; What is cell-index for? I was using it to control the bus number, is that the wrong attribute? It shouldn't be specified at all -- the hardware has no concept of a device number. cell-index is important. It describes the hardware, or more specifically the layout of the SoC. The SoC has 2 i2c busses which are numbered 0 and 1. This property should stay for the 5200. However, that is the only purpose of it. cell-index does *not* describe the system level bus number. cell-index should *only* be used if it's used to index into SoC-shared registers. It should *never* be used for logical bus or device numbering as it's being used here. -- 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jean Delvare wrote: Hi Matt, WRT cell-index this is an index of the bus on the chip (not the logical i2c bus but the physical difference between two i2c controllers) and then any i2c devices which need to be communicated with would be child nodes, their reg property reflecting their slave address, is that not correct? I am not familiar with the OF tree, I can't tell, sorry. Well, it's how board designers tell you what chip is at what address :) -- Matt Sealey [EMAIL PROTECTED] Genesi, Manager, Developer Relations ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jon Smirl wrote:
[EMAIL PROTECTED] {
device_type = i2c;
compatible = mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c;
cell-index = 1;
reg = 3d40 40;
interrupts = 2 10 0;
interrupt-parent = mpc5200_pic;
fsl5200-clocking;
[EMAIL PROTECTED] {
device_type = rtc;
compatible = epson,pcf8564;
reg = 51;
};
};
My only comment would be that the fsl5200-clocking property is
totally redundant.
Drivers can look at the compatible property (mpc5200b-i2c and
mpc5200-i2c) to match up what special needs the driver may need.
Even if it was just fsl-i2c, it could/should be implicit that
this device is the onboard i2c and the parent node is ostensibly
going to be marked as an MPC52xx SoC.. or it can look for the
mpc5200-cdm node. There is no reason to invent a property just
so you can do a property search when it replaces code of the
same size to do a node or compatible search..
--
Matt Sealey [EMAIL PROTECTED]
Genesi, Manager, Developer Relations
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jon Smirl wrote:
This is my first pass at reworking the Freescale i2c driver. It
switches the driver from being a platform driver to an open firmware
one. I've checked it out on my hardware and it is working.
We may want to hold off on this until arch/ppc goes away (or at least
all users of this driver in arch/ppc).
You can specific i2c devices on a bus by adding child nodes for them
in the device tree. It does not know how to autoload the i2c chip
driver modules.
[EMAIL PROTECTED] {
device_type = i2c;
compatible = mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c;
dtc supports the mpc5200b-i2c, mpc5200-i2c, fsl-i2c syntax.
cell-index = 1;
What is cell-index for?
fsl5200-clocking;
As Matt pointed out, this is redundant.
[EMAIL PROTECTED] {
device_type = rtc;
This isn't really necessary.
One side effect is that legacy style i2c drivers won't work anymore.
If you mean legacy-style client drivers, why not?
The driver contains a table for mapping device tree style names to
linux kernel ones.
Can we please put this in common code instead?
+static struct i2c_driver_device i2c_devices[] = {
+ {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
+ {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
+ {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
+ {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
+ {epson,pcf8564, rtc-pcf8563, pcf8564,},
+};
At the very least, include the entries that are already being used with
this driver in fsl_soc.c.
I'd like to get rid of this table. There are two obvious solutions,
use names in the device tree that match up with the linux device
driver names.
This was proposed and rejected a while ago. For one thing, some drivers
handle multiple chips, and we shouldn't base device tree bindings on
what groupings Linux happens to make in what driver supports what.
Or push these strings into the chip drivers and modify
i2c-core to also match on the device tree style names.
That would be ideal; it just hasn't been done yet.
A middle ground for now would be to keep one table in drivers/of or
something.
Without one of
these changes the table is going to need a mapping for every i2c
device on the market.
Nah, only one for every i2c device Linux supports. :-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..6f80216 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -306,122 +306,6 @@ err:
arch_initcall(gfar_of_init);
-#ifdef CONFIG_I2C_BOARDINFO
-#include linux/i2c.h
-struct i2c_driver_device {
- char*of_device;
- char*i2c_driver;
- char*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] __initdata = {
- {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
- {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
- {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
- {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
-};
This is obviously not based on head-of-tree -- there are several more
entries in this table.
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d8de4ac..5ceb936 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -17,7 +17,7 @@
#include linux/module.h
#include linux/sched.h
#include linux/init.h
-#include linux/platform_device.h
+#include asm/of_platform.h
Should be linux/of_platform.h
@@ -180,7 +182,7 @@ static void mpc_i2c_stop(struct mpc_i2c *i2c)
static int mpc_write(struct mpc_i2c *i2c, int target,
const u8 * data, int length, int restart)
{
- int i;
+ int i, result;
unsigned timeout = i2c-adap.timeout;
u32 flags = restart ? CCR_RSTA : 0;
@@ -192,15 +194,15 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
/* Write target byte */
writeb((target 1), i2c-base + MPC_I2C_DR);
- if (i2c_wait(i2c, timeout, 1) 0)
- return -1;
+ if ((result = i2c_wait(i2c, timeout, 1)) 0)
+ return result;
for (i = 0; i length; i++) {
/* Write data byte */
writeb(data[i], i2c-base + MPC_I2C_DR);
- if (i2c_wait(i2c, timeout, 1) 0)
- return -1;
+ if ((result = i2c_wait(i2c, timeout, 1)) 0)
+ return result;
}
return 0;
This is a separate change from the OF-ization -- at least note it in the
changelog.
+ for (i = 0; i ARRAY_SIZE(i2c_devices); i++) {
+ if (!of_device_is_compatible(node, i2c_devices[i].of_device))
+ continue;
+ strncpy(info-driver_name, i2c_devices[i].i2c_driver,
KOBJ_NAME_LEN);
+ strncpy(info-type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
+ printk(i2c driver is %s\n, info-driver_name);
Should be KERN_DEBUG, if it stays at all.
+static void of_register_i2c_devices(struct i2c_adapter *adap, struct
device_node
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jon Smirl wrote: I can change it to remove fsl5200-clocking if someone can tell me if this is only needed on the mpc5200b. Well, it appears to be needed on the mpc5200 (no 'b')... This driver also supports the MPC107/Tsi107/MPC8240/MPC8245 and MPC85xx/MPC8641. You forgot mpc83xx. :-) Do any of these other chips need fsl5200-clocking? None of them have it set in arch/powerpc/boot/dts. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Jon Smirl [EMAIL PROTECTED] wrote: This is my first pass at reworking the Freescale i2c driver. It switches the driver from being a platform driver to an open firmware one. I've checked it out on my hardware and it is working. Isn't this device core also used in the fsl coldfire socs? If so, it needs to have both platform bus and of_platform bus bindings. This is easy to do as long as you keep device probing and initialization in separate routines. See drivers/serial/uartlite.c for an example. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: I can change it to remove fsl5200-clocking if someone can tell me if this is only needed on the mpc5200b. Well, it appears to be needed on the mpc5200 (no 'b')... What is the recommend way to check for a mpc5200 or mpc5200b? This driver also supports the MPC107/Tsi107/MPC8240/MPC8245 and MPC85xx/MPC8641. You forgot mpc83xx. :-) Do any of these other chips need fsl5200-clocking? None of them have it set in arch/powerpc/boot/dts. -Scott -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Matt Sealey [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
[EMAIL PROTECTED] {
device_type = i2c;
compatible = mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c;
cell-index = 1;
reg = 3d40 40;
interrupts = 2 10 0;
interrupt-parent = mpc5200_pic;
fsl5200-clocking;
[EMAIL PROTECTED] {
device_type = rtc;
compatible = epson,pcf8564;
reg = 51;
};
};
My only comment would be that the fsl5200-clocking property is
totally redundant.
Drivers can look at the compatible property (mpc5200b-i2c and
mpc5200-i2c) to match up what special needs the driver may need.
Even if it was just fsl-i2c, it could/should be implicit that
this device is the onboard i2c and the parent node is ostensibly
going to be marked as an MPC52xx SoC.. or it can look for the
mpc5200-cdm node. There is no reason to invent a property just
so you can do a property search when it replaces code of the
same size to do a node or compatible search..
Yeah, I agree. Drop the fsl-clocking property. The hardware is
adequately described without it.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[EMAIL PROTECTED]
(403) 399-0195
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jon Smirl wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: I can change it to remove fsl5200-clocking if someone can tell me if this is only needed on the mpc5200b. Well, it appears to be needed on the mpc5200 (no 'b')... What is the recommend way to check for a mpc5200 or mpc5200b? Just check for mpc5200-i2c -- it's listed in the mpc5200b dts as well. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
This is my first pass at reworking the Freescale i2c driver. It
switches the driver from being a platform driver to an open firmware
one. I've checked it out on my hardware and it is working.
We may want to hold off on this until arch/ppc goes away (or at least
all users of this driver in arch/ppc).
How about renaming the old driver file and leaving it hooked to ppc?
Then it would get deleted when ppc goes away. That would let work
progress on the powerpc version.
I'm working on this because I'm adding codecs under powerpc that use
i2c and I want consistent device tree use.
You can specific i2c devices on a bus by adding child nodes for them
in the device tree. It does not know how to autoload the i2c chip
driver modules.
[EMAIL PROTECTED] {
device_type = i2c;
compatible = mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c;
dtc supports the mpc5200b-i2c, mpc5200-i2c, fsl-i2c syntax.
cell-index = 1;
What is cell-index for?
I was using it to control the bus number, is that the wrong attribute?
fsl5200-clocking;
As Matt pointed out, this is redundant.
[EMAIL PROTECTED] {
device_type = rtc;
This isn't really necessary.
Code doesn't access it. I copied it from a pre-existing device tree.
One side effect is that legacy style i2c drivers won't work anymore.
If you mean legacy-style client drivers, why not?
i2c_new_device() doesn't work with legacy-style client drivers.
The driver contains a table for mapping device tree style names to
linux kernel ones.
Can we please put this in common code instead?
+static struct i2c_driver_device i2c_devices[] = {
+ {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
+ {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
+ {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
+ {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
+ {epson,pcf8564, rtc-pcf8563, pcf8564,},
+};
At the very least, include the entries that are already being used with
this driver in fsl_soc.c.
This is the same table from fsl_soc.c it has been moved. The data
really belongs in the i2c drivers if I can figure out how to get it
there.
I'd like to get rid of this table. There are two obvious solutions,
use names in the device tree that match up with the linux device
driver names.
This was proposed and rejected a while ago. For one thing, some drivers
handle multiple chips, and we shouldn't base device tree bindings on
what groupings Linux happens to make in what driver supports what.
Or push these strings into the chip drivers and modify
i2c-core to also match on the device tree style names.
That would be ideal; it just hasn't been done yet.
This is not hard to do but the i2c people will have to agree. I need
to change the i2c_driver structure to include the additional names.
A middle ground for now would be to keep one table in drivers/of or
something.
Without one of
these changes the table is going to need a mapping for every i2c
device on the market.
Nah, only one for every i2c device Linux supports. :-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 1cf29c9..6f80216 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -306,122 +306,6 @@ err:
arch_initcall(gfar_of_init);
-#ifdef CONFIG_I2C_BOARDINFO
-#include linux/i2c.h
-struct i2c_driver_device {
- char*of_device;
- char*i2c_driver;
- char*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] __initdata = {
- {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
- {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
- {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
- {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
-};
This is obviously not based on head-of-tree -- there are several more
entries in this table.
It is based on 2.6.23. Head of tree hasn't been working very well for
me. I'll rebase it when I can get things working again.
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index d8de4ac..5ceb936 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -17,7 +17,7 @@
#include linux/module.h
#include linux/sched.h
#include linux/init.h
-#include linux/platform_device.h
+#include asm/of_platform.h
Should be linux/of_platform.h
changed
@@ -180,7 +182,7 @@ static void mpc_i2c_stop(struct mpc_i2c *i2c)
static int mpc_write(struct mpc_i2c *i2c, int target,
const u8 * data, int length, int restart)
{
- int i;
+ int i, result;
unsigned timeout = i2c-adap.timeout;
u32 flags = restart ? CCR_RSTA : 0;
@@ -192,15 +194,15 @@ static int mpc_write(struct mpc_i2c *i2c, int target,
/* Write target byte */
writeb((target 1), i2c-base + MPC_I2C_DR);
- if (i2c_wait(i2c, timeout, 1) 0)
-
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Jon Smirl [EMAIL PROTECTED] wrote: This is my first pass at reworking the Freescale i2c driver. It switches the driver from being a platform driver to an open firmware one. I've checked it out on my hardware and it is working. Is there any way to have the i2c chip modules match on the device tree and be automatically loaded? This is complicated by the fact that these modules are used on other platforms. If there is a way to do this for the i2c bus I can apply the same code to the audio bus as well. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Jon Smirl wrote:
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote:
Jon Smirl wrote:
This is my first pass at reworking the Freescale i2c driver. It
switches the driver from being a platform driver to an open firmware
one. I've checked it out on my hardware and it is working.
We may want to hold off on this until arch/ppc goes away (or at least
all users of this driver in arch/ppc).
How about renaming the old driver file and leaving it hooked to ppc?
Then it would get deleted when ppc goes away. That would let work
progress on the powerpc version.
Or we could have one driver that has two probe methods. I don't like
forking the driver.
I'm working on this because I'm adding codecs under powerpc that use
i2c and I want consistent device tree use.
We already support i2c clients in the device tree, via the code in
fsl_soc.c.
cell-index = 1;
What is cell-index for?
I was using it to control the bus number, is that the wrong attribute?
It shouldn't be specified at all -- the hardware has no concept of a
device number.
[EMAIL PROTECTED] {
device_type = rtc;
This isn't really necessary.
Code doesn't access it. I copied it from a pre-existing device tree.
I'm just trying to keep the damage from spreading. :-)
One side effect is that legacy style i2c drivers won't work anymore.
If you mean legacy-style client drivers, why not?
i2c_new_device() doesn't work with legacy-style client drivers.
No, but they should still work the old way.
Or push these strings into the chip drivers and modify
i2c-core to also match on the device tree style names.
That would be ideal; it just hasn't been done yet.
This is not hard to do but the i2c people will have to agree. I need
to change the i2c_driver structure to include the additional names.
I got a fair bit of resistance from them on the topic of multiple match
names for i2c clients.
Most of this code should be made generic and placed in drivers/of.
How so, it is specific to adding i2c drivers.
I meant generic with respect to the type of i2c controller, not generic
to all device types. :-)
It could be drivers/of/i2c.c, or drivers/i2c/of.c, etc.
We might as well just use i2c_new_device() instead of messing around
with bus numbers. Note that this is technically no longer platform
code, so it's harder to justify claiming the static numberspace.
I was allowing control of the bus number with cell-index and
i2c_add_numbered_adapter().
Should I get rid of this and switch to i2c_add_adapter()?
Yes.
+ i2c-base = ioremap((phys_addr_t)mem.start, MPC_I2C_REGION);
if (!i2c-base) {
printk(KERN_ERR i2c-mpc - failed to map controller\n);
Use of_iomap().
I didn't write this, how should it be done? MPC_I2C_REGION can be
eliminated by using mem.start - mem.end.
i2c-base = of_iomap(op-node, 0);
of_address_to_resource() and ioremap() are combined into one.
Let's take this opportunity to stop matching on device_type here
(including updating booting-without-of.txt).
Remove the .type line and leave .compatible?
Yes.
+static struct of_platform_driver mpc_i2c_driver = {
+ .owner = THIS_MODULE,
+ .name = DRV_NAME,
+ .match_table= mpc_i2c_of_match,
+ .probe = mpc_i2c_probe,
+ .remove = __devexit_p(mpc_i2c_remove),
+ .driver = {
+ .name = DRV_NAME,
},
};
Do we still need .name if we have .driver.name?
Yes, i2c-core is matching on mpc_i2c_driver.name, i2c-core would need
to be changed to use mpc_i2c_driver.driver.name.
How can i2c-core be matching on a field in an OF-specific struct?
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Matt Sealey wrote: Scott Wood wrote: Jon Smirl wrote: cell-index = 1; What is cell-index for? I was using it to control the bus number, is that the wrong attribute? It shouldn't be specified at all -- the hardware has no concept of a device number. Well, all i2c devices have a chip id you can probe for, I meant a controller device number (a.k.a. bus number), which (outside of documentation) is purely a Linux invention, and which is what cell-index was being used for above. as for buses I think cell-index is a holdover from the way the PSC code is organised on the MPC5200 for example - if you have multiple buses which use the same registers, for example. It's redundant on the PSC's for programming because they all use different register offsets but if you move to other devices like the GPTs, then it is then useful for debugging (it is far more interesting to say GPT1 than GPT @ offset to match the) and in general for tweaking OTHER parts of the chip (for instance the CDM - very relevant!) which use single registers to control entire swathes of units. Right, that's what cell-index is for. This is different. :-) -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: This is my first pass at reworking the Freescale i2c driver. It switches the driver from being a platform driver to an open firmware one. I've checked it out on my hardware and it is working. We may want to hold off on this until arch/ppc goes away (or at least all users of this driver in arch/ppc). How about renaming the old driver file and leaving it hooked to ppc? Then it would get deleted when ppc goes away. That would let work progress on the powerpc version. Or we could have one driver that has two probe methods. I don't like forking the driver. I agree. This driver can and should have multiple bus bindings. cell-index = 1; What is cell-index for? I was using it to control the bus number, is that the wrong attribute? It shouldn't be specified at all -- the hardware has no concept of a device number. cell-index is important. It describes the hardware, or more specifically the layout of the SoC. The SoC has 2 i2c busses which are numbered 0 and 1. This property should stay for the 5200. However, that is the only purpose of it. cell-index does *not* describe the system level bus number. I was allowing control of the bus number with cell-index and i2c_add_numbered_adapter(). Should I get rid of this and switch to i2c_add_adapter()? Yes. Yes, the purpose of cell-index is not to give an i2c bus number enumeration. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Matt Sealey wrote: Scott Wood wrote: Jon Smirl wrote: cell-index = 1; What is cell-index for? I was using it to control the bus number, is that the wrong attribute? It shouldn't be specified at all -- the hardware has no concept of a device number. Well, all i2c devices have a chip id you can probe for, I meant a controller device number (a.k.a. bus number), which (outside of documentation) is purely a Linux invention, and which is what cell-index was being used for above. as for buses I think cell-index is a holdover from the way the PSC code is organised on the MPC5200 for example - if you have multiple buses which use the same registers, for example. It's redundant on the PSC's for programming because they all use different register offsets but if you move to other devices like the GPTs, then it is then useful for debugging (it is far more interesting to say GPT1 than GPT @ offset to match the) Actually, it is not intended for this. cell-index is not intended to be able to say GPT1 instead of [EMAIL PROTECTED] (while that may be possible, it is not the intent). Nor is it a holdover from the PSC code design. It is designed to describe the internal structure of the SoC. The 5200 has 6 PSCs, and there are some chip registers which are shared between all the PSCs (for clocking, IO mode, etc). It is not sufficient to simply plop down a device tree node for each PSC because it doesn't give enough information about which bits to use in the shared regs. (For example, the port_config register) and in general for tweaking OTHER parts of the chip (for instance the CDM - very relevant!) which use single registers to control entire swathes of units. Right, that's what cell-index is for. This is different. :-) Yes, this is correct. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. [EMAIL PROTECTED] (403) 399-0195 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Grant Likely [EMAIL PROTECTED] wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: Jon Smirl wrote: This is my first pass at reworking the Freescale i2c driver. It switches the driver from being a platform driver to an open firmware one. I've checked it out on my hardware and it is working. We may want to hold off on this until arch/ppc goes away (or at least all users of this driver in arch/ppc). How about renaming the old driver file and leaving it hooked to ppc? Then it would get deleted when ppc goes away. That would let work progress on the powerpc version. Or we could have one driver that has two probe methods. I don't like forking the driver. I agree. This driver can and should have multiple bus bindings. What non-of platform uses this driver? I believe this is the last struct platform driver left for the mpc5200. Fixing this one allows the platform bus to be removed. With a device tree there isn't any reason to keep a platform bus, everything should be on the of_platform bus. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote: One side effect is that legacy style i2c drivers won't work anymore. If you mean legacy-style client drivers, why not? i2c_new_device() doesn't work with legacy-style client drivers. No, but they should still work the old way. I'm not in favor trying to support both legacy and new style i2c drivers. It took me all of five minutes to convert an existing legacy driver to the new style. Pretty much all you need to do is delete code (about 100 lines). So I'd recommend converting the drivers we are interest in instead of trying to support both types. -- Jon Smirl [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote:
+static struct of_platform_driver mpc_i2c_driver = {
+ .owner = THIS_MODULE,
+ .name = DRV_NAME,
+ .match_table= mpc_i2c_of_match,
+ .probe = mpc_i2c_probe,
+ .remove = __devexit_p(mpc_i2c_remove),
+ .driver = {
+ .name = DRV_NAME,
},
};
Do we still need .name if we have .driver.name?
This is a general question, if of_platform_driver doesn't need .name
it should be removed from the structure.
--
Jon Smirl
[EMAIL PROTECTED]
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On Mon, 5 Nov 2007 20:34:51 -0500 Jon Smirl [EMAIL PROTECTED] wrote:
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote:
+static struct of_platform_driver mpc_i2c_driver = {
+ .owner = THIS_MODULE,
+ .name = DRV_NAME,
+ .match_table= mpc_i2c_of_match,
+ .probe = mpc_i2c_probe,
+ .remove = __devexit_p(mpc_i2c_remove),
+ .driver = {
+ .name = DRV_NAME,
},
};
Do we still need .name if we have .driver.name?
This is a general question, if of_platform_driver doesn't need .name
it should be removed from the structure.
I am in the process of doing that. However there a quite a few drivers
that need to be fixed first. In the meantime,
of_register_platform_driver will copy which ever of the name and owner
fields you fill in to the others, so we can convert over time. For new
drivers (and changing), please use the name and owner fields in the
embedded device_driver struct. (this is the same as done by the platform
drivers currently ...)
--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/
pgpD6Eb0OCgP6.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
On 11/5/07, Scott Wood [EMAIL PROTECTED] wrote:
Or push these strings into the chip drivers and modify
i2c-core to also match on the device tree style names.
That would be ideal; it just hasn't been done yet.
This is not hard to do but the i2c people will have to agree. I need
to change the i2c_driver structure to include the additional names.
I got a fair bit of resistance from them on the topic of multiple match
names for i2c clients.
Here's a first pass at pushing the strings back into the i2c drivers.
If this looks reasonable it can be optimized a lot more. A more
advanced version of this code would combine the alias, name, and
driver_name fields. The existing pairing of driver_name/name could be
merged into the concept of alias names for the driver.
Extend i2c-core to support lists of device tree compatible names when
matching drivers
From: Jon Smirl [EMAIL PROTECTED]
---
drivers/i2c/busses/i2c-mpc.c | 35 ---
drivers/i2c/i2c-core.c | 17 +++--
drivers/rtc/rtc-pcf8563.c|1 +
drivers/rtc/rtc-rs5c372.c|1 +
include/linux/i2c.h | 11 +--
5 files changed, 30 insertions(+), 35 deletions(-)
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 4ddebe4..6313631 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -312,34 +312,6 @@ static struct i2c_adapter mpc_ops = {
.retries = 1
};
-struct i2c_driver_device {
- char*of_device;
- char*i2c_driver;
- char*i2c_type;
-};
-
-static struct i2c_driver_device i2c_devices[] = {
- {ricoh,rs5c372a, rtc-rs5c372, rs5c372a,},
- {ricoh,rs5c372b, rtc-rs5c372, rs5c372b,},
- {ricoh,rv5c386, rtc-rs5c372, rv5c386,},
- {ricoh,rv5c387a, rtc-rs5c372, rv5c387a,},
- {epson,pcf8564, rtc-pcf8563, pcf8564,},
-};
-
-static int of_find_i2c_driver(struct device_node *node, struct
i2c_board_info *info)
-{
- int i;
-
- for (i = 0; i ARRAY_SIZE(i2c_devices); i++) {
- if (!of_device_is_compatible(node, i2c_devices[i].of_device))
- continue;
- strncpy(info-driver_name, i2c_devices[i].i2c_driver,
KOBJ_NAME_LEN);
- strncpy(info-type, i2c_devices[i].i2c_type, I2C_NAME_SIZE);
- return 0;
- }
- return -ENODEV;
-}
-
static void of_register_i2c_devices(struct i2c_adapter *adap, struct
device_node *adap_node)
{
struct device_node *node = NULL;
@@ -347,6 +319,7 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
while ((node = of_get_next_child(adap_node, node))) {
struct i2c_board_info info;
const u32 *addr;
+ const char *compatible;
int len;
addr = of_get_property(node, reg, len);
@@ -359,9 +332,9 @@ static void of_register_i2c_devices(struct
i2c_adapter *adap, struct device_node
if (info.irq == NO_IRQ)
info.irq = -1;
- if (of_find_i2c_driver(node, info) 0)
- continue;
-
+ compatible = of_get_property(node, compatible, len);
+ strlcpy(info.driver_name, compatible, len);
+ info.type[0] = '\0';
info.platform_data = NULL;
info.addr = *addr;
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index d663e69..4128cd1 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -17,7 +17,7 @@
Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */
/* - */
-/* With some changes from Kyösti Mälkki [EMAIL PROTECTED].
+/* With some changes from Kyï¿œsti Mï¿œlkki [EMAIL PROTECTED].
All SMBus-related things are written by Frodo Looijaard [EMAIL PROTECTED]
SMBus 2.0 support by Mark Studebaker [EMAIL PROTECTED] and
Jean Delvare [EMAIL PROTECTED] */
@@ -51,6 +51,7 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
{
struct i2c_client *client = to_i2c_client(dev);
struct i2c_driver *driver = to_i2c_driver(drv);
+ char **alias;
/* make legacy i2c drivers bypass driver model probing entirely;
* such drivers scan each i2c adapter/bus themselves.
@@ -61,7 +62,19 @@ static int i2c_device_match(struct device *dev,
struct device_driver *drv)
/* new style drivers use the same kind of driver matching policy
* as platform devices or SPI: compare device and driver IDs.
*/
- return strcmp(client-driver_name, drv-name) == 0;
+ if (strcmp(client-driver_name, drv-name) == 0)
+ return true;
+
+ /* Match against alias device tree names */
+ if (!driver-alias)
+ return 0;
+ alias = driver-alias;
+ while (*alias) {
+
Re: [RFC] Rework of i2c-mpc.c - Freescale i2c driver
Hi Jon,
On Mon, 5 Nov 2007 23:25:35 -0500 Jon Smirl [EMAIL PROTECTED] wrote:
+ compatible = of_get_property(node, compatible, len);
+ strlcpy(info.driver_name, compatible, len);
Of course, you will check len against sizeof(info.driver_name), right?
-/* With some changes from Kyösti Mälkki [EMAIL PROTECTED].
+/* With some changes from Kyï¿œsti Mï¿œlkki [EMAIL PROTECTED].
Did you mean to change this?
+ char **alias;
const char ** would be nicer.
struct i2c_driver {
int id;
unsigned int class;
+
+ /* Alias name for the driver. Used to support device trees on
+ * the PowerPC architecture. Device tree names take the form of
+ * vendor,chip. For example epson,pcf8564. Alias is a list of
+ * strings terminated by a zero entry.
+ */
+ char **alias;
const char ** would be nicer;
--
Cheers,
Stephen Rothwell[EMAIL PROTECTED]
http://www.canb.auug.org.au/~sfr/
pgpXADQEA2eM8.pgp
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
