| Message ID | 20100924221313.28357.61419.stgit@angua (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Sep 24, 2010 at 04:14:53PM -0600, Grant Likely wrote: > Commit 959e85f7, "i2c: add OF-style registration and binding" caused a > module dependency loop where of_i2c.c calls functions in i2c-core, and > i2c-core calls of_i2c_register_devices() in of_i2c. This means that > when i2c support is built as a module when CONFIG_OF is set, then > neither i2c_core nor of_i2c are able to be loaded. > > This patch fixes the problem by moving the of_i2c_register_devices() > function into the body of i2c_core and renaming it to > i2c_scan_of_devices (of_i2c_register_devices is analogous to the > existing i2c_scan_static_board_info function and so should be named > similarly). This function isn't called by any code outside of > i2c_core, and it must always be present when CONFIG_OF is selected, so > it makes sense to locate it there. When CONFIG_OF is not selected, > of_i2c_register_devices() becomes a no-op. I sort of go with this one.
On Fri, Sep 24, 2010 at 04:14:53PM -0600, Grant Likely wrote: > Commit 959e85f7, "i2c: add OF-style registration and binding" caused a > module dependency loop where of_i2c.c calls functions in i2c-core, and > i2c-core calls of_i2c_register_devices() in of_i2c. This means that > when i2c support is built as a module when CONFIG_OF is set, then > neither i2c_core nor of_i2c are able to be loaded. > > This patch fixes the problem by moving the of_i2c_register_devices() > function into the body of i2c_core and renaming it to > i2c_scan_of_devices (of_i2c_register_devices is analogous to the > existing i2c_scan_static_board_info function and so should be named > similarly). This function isn't called by any code outside of > i2c_core, and it must always be present when CONFIG_OF is selected, so > it makes sense to locate it there. When CONFIG_OF is not selected, > of_i2c_register_devices() becomes a no-op. > > Signed-off-by: Grant Likely <grant.likely@secretlab.ca> > --- > drivers/i2c/i2c-core.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++-- > drivers/of/of_i2c.c | 57 --------------------------------------------- > include/linux/of_i2c.h | 7 ------ > 3 files changed, 59 insertions(+), 66 deletions(-) > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c > index 6649176..64a261b 100644 > --- a/drivers/i2c/i2c-core.c > +++ b/drivers/i2c/i2c-core.c > @@ -32,8 +32,8 @@ > #include <linux/init.h> > #include <linux/idr.h> > #include <linux/mutex.h> > -#include <linux/of_i2c.h> > #include <linux/of_device.h> > +#include <linux/of_irq.h> > #include <linux/completion.h> > #include <linux/hardirq.h> > #include <linux/irqflags.h> > @@ -818,6 +818,63 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter) > up_read(&__i2c_board_lock); > } > > +#ifdef CONFIG_OF > +void i2c_scan_of_devices(struct i2c_adapter *adap) > +{ > + void *result; > + struct device_node *node; > + > + /* Only register child devices if the adapter has a node pointer set */ > + if (!adap->dev.of_node) > + return; > + > + for_each_child_of_node(adap->dev.of_node, node) { > + struct i2c_board_info info = {}; > + struct dev_archdata dev_ad = {}; > + const __be32 *addr; > + int len; > + > + dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name); > + if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) { > + dev_err(&adap->dev, "of_i2c: modalias failure on %s\n", > + node->full_name); > + continue; > + } > + > + addr = of_get_property(node, "reg", &len); > + if (!addr || (len < sizeof(int))) { > + dev_err(&adap->dev, "of_i2c: invalid reg on %s\n", > + node->full_name); > + continue; > + } > + > + info.addr = be32_to_cpup(addr); > + if (info.addr > (1 << 10) - 1) { > + dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n", > + info.addr, node->full_name); > + continue; > + } > + > + info.irq = irq_of_parse_and_map(node, 0); > + info.of_node = of_node_get(node); > + info.archdata = &dev_ad; > + > + request_module("%s", info.type); > + > + result = i2c_new_device(adap, &info); > + if (result == NULL) { > + dev_err(&adap->dev, "of_i2c: Failure registering %s\n", > + node->full_name); > + of_node_put(node); > + irq_dispose_mapping(info.irq); > + continue; > + } > + } > +} > +#else > +static inline void i2c_scan_of_devices(struct i2c_adapter *adap) { } > +#endif Is there any advantage to just making the definition in the header file a static inline and removing the #else part of this?
On Wed, Sep 29, 2010 at 8:21 AM, Ben Dooks <ben-i2c@fluff.org> wrote: > On Fri, Sep 24, 2010 at 04:14:53PM -0600, Grant Likely wrote: >> Commit 959e85f7, "i2c: add OF-style registration and binding" caused a >> module dependency loop where of_i2c.c calls functions in i2c-core, and >> i2c-core calls of_i2c_register_devices() in of_i2c. This means that >> when i2c support is built as a module when CONFIG_OF is set, then >> neither i2c_core nor of_i2c are able to be loaded. >> >> This patch fixes the problem by moving the of_i2c_register_devices() >> function into the body of i2c_core and renaming it to >> i2c_scan_of_devices (of_i2c_register_devices is analogous to the >> existing i2c_scan_static_board_info function and so should be named >> similarly). This function isn't called by any code outside of >> i2c_core, and it must always be present when CONFIG_OF is selected, so >> it makes sense to locate it there. When CONFIG_OF is not selected, >> of_i2c_register_devices() becomes a no-op. >> >> Signed-off-by: Grant Likely <grant.likely@secretlab.ca> >> --- >> drivers/i2c/i2c-core.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++-- >> drivers/of/of_i2c.c | 57 --------------------------------------------- >> include/linux/of_i2c.h | 7 ------ >> 3 files changed, 59 insertions(+), 66 deletions(-) >> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c >> index 6649176..64a261b 100644 >> --- a/drivers/i2c/i2c-core.c >> +++ b/drivers/i2c/i2c-core.c >> @@ -32,8 +32,8 @@ >> #include <linux/init.h> >> #include <linux/idr.h> >> #include <linux/mutex.h> >> -#include <linux/of_i2c.h> >> #include <linux/of_device.h> >> +#include <linux/of_irq.h> >> #include <linux/completion.h> >> #include <linux/hardirq.h> >> #include <linux/irqflags.h> >> @@ -818,6 +818,63 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter) >> up_read(&__i2c_board_lock); >> } >> >> +#ifdef CONFIG_OF >> +void i2c_scan_of_devices(struct i2c_adapter *adap) >> +{ >> + void *result; >> + struct device_node *node; >> + >> + /* Only register child devices if the adapter has a node pointer set */ >> + if (!adap->dev.of_node) >> + return; >> + >> + for_each_child_of_node(adap->dev.of_node, node) { >> + struct i2c_board_info info = {}; >> + struct dev_archdata dev_ad = {}; >> + const __be32 *addr; >> + int len; >> + >> + dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name); >> + if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) { >> + dev_err(&adap->dev, "of_i2c: modalias failure on %s\n", >> + node->full_name); >> + continue; >> + } >> + >> + addr = of_get_property(node, "reg", &len); >> + if (!addr || (len < sizeof(int))) { >> + dev_err(&adap->dev, "of_i2c: invalid reg on %s\n", >> + node->full_name); >> + continue; >> + } >> + >> + info.addr = be32_to_cpup(addr); >> + if (info.addr > (1 << 10) - 1) { >> + dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n", >> + info.addr, node->full_name); >> + continue; >> + } >> + >> + info.irq = irq_of_parse_and_map(node, 0); >> + info.of_node = of_node_get(node); >> + info.archdata = &dev_ad; >> + >> + request_module("%s", info.type); >> + >> + result = i2c_new_device(adap, &info); >> + if (result == NULL) { >> + dev_err(&adap->dev, "of_i2c: Failure registering %s\n", >> + node->full_name); >> + of_node_put(node); >> + irq_dispose_mapping(info.irq); >> + continue; >> + } >> + } >> +} >> +#else >> +static inline void i2c_scan_of_devices(struct i2c_adapter *adap) { } >> +#endif > > Is there any advantage to just making the definition in the header > file a static inline and removing the #else part of this? I'm not sure what you mean. This particular patch makes i2c_scan_of_devices() completely internal to i2c-core.c so that there is no need to expose it in the header file at all. g. > > -- > Ben > > Q: What's a light-year? > A: One-third less calories than a regular year. > >
On Wed, 29 Sep 2010 00:20:54 +0100, Ben Dooks wrote: > On Fri, Sep 24, 2010 at 04:14:53PM -0600, Grant Likely wrote: > > Commit 959e85f7, "i2c: add OF-style registration and binding" caused a > > module dependency loop where of_i2c.c calls functions in i2c-core, and > > i2c-core calls of_i2c_register_devices() in of_i2c. This means that > > when i2c support is built as a module when CONFIG_OF is set, then > > neither i2c_core nor of_i2c are able to be loaded. > > > > This patch fixes the problem by moving the of_i2c_register_devices() > > function into the body of i2c_core and renaming it to > > i2c_scan_of_devices (of_i2c_register_devices is analogous to the > > existing i2c_scan_static_board_info function and so should be named > > similarly). This function isn't called by any code outside of > > i2c_core, and it must always be present when CONFIG_OF is selected, so > > it makes sense to locate it there. When CONFIG_OF is not selected, > > of_i2c_register_devices() becomes a no-op. > > I sort of go with this one. Actually I would prefer option #2, even though I understand it won't make Grant too happy. Having a large chunk of OF-specific code in i2c-core, leaving of_i2c.c almost empty, doesn't seem right. I took a look at what other relevant subsystems do. SPI is boolean so it doesn't have the issue. MDIO is tristate, the registration function is in of_mdio.c and individual drivers call it. And there are a lot more of these (9) than i2c drivers (3). So I would let individual drivers call of_i2c_register_devices(), as it used to be until 2.6.35. 2 extra functions calls doesn't seem a high price to pay to keep the code logically separated. This also make things consistent, with all OF registration functions living under drivers/of. Thanks,
On Wed, Sep 29, 2010 at 11:42 PM, Jean Delvare <khali@linux-fr.org> wrote: > On Wed, 29 Sep 2010 00:20:54 +0100, Ben Dooks wrote: >> On Fri, Sep 24, 2010 at 04:14:53PM -0600, Grant Likely wrote: >> > Commit 959e85f7, "i2c: add OF-style registration and binding" caused a >> > module dependency loop where of_i2c.c calls functions in i2c-core, and >> > i2c-core calls of_i2c_register_devices() in of_i2c. This means that >> > when i2c support is built as a module when CONFIG_OF is set, then >> > neither i2c_core nor of_i2c are able to be loaded. >> > >> > This patch fixes the problem by moving the of_i2c_register_devices() >> > function into the body of i2c_core and renaming it to >> > i2c_scan_of_devices (of_i2c_register_devices is analogous to the >> > existing i2c_scan_static_board_info function and so should be named >> > similarly). This function isn't called by any code outside of >> > i2c_core, and it must always be present when CONFIG_OF is selected, so >> > it makes sense to locate it there. When CONFIG_OF is not selected, >> > of_i2c_register_devices() becomes a no-op. >> >> I sort of go with this one. > > Actually I would prefer option #2, even though I understand it won't > make Grant too happy. Having a large chunk of OF-specific code in > i2c-core, leaving of_i2c.c almost empty, doesn't seem right. I'm fine with this. In the grand scheme, it ends up being an unimportant point. > I took a look at what other relevant subsystems do. SPI is boolean so it > doesn't have the issue. MDIO is tristate, the registration function is > in of_mdio.c and individual drivers call it. And there are a lot more > of these (9) than i2c drivers (3). > > So I would let individual drivers call of_i2c_register_devices(), as it > used to be until 2.6.35. 2 extra functions calls doesn't seem a high > price to pay to keep the code logically separated. This also make > things consistent, with all OF registration functions living under > drivers/of. This is actually historical and somewhat in transition. Now that all the OF core code is cleaned up and generalized, I'm looking at the bus specific hooks and deciding what would be best to do about them. I'm likely to move the SPI support into drivers/spi, and I'll probably post a patch to do the same for drivers/net/phy and for the platform bus. The reason being that the data extraction code is far more bus specific than it is OF-specific. I will however back off from putting the registration hook directly into the shared bus registration functions for the time being. It is a minor issue, and it does make a certain amount of sense for the individual drivers to control the bus population. Proof is in the patches anyway and we can debate it after I actually post something concrete. g.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index 6649176..64a261b 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -32,8 +32,8 @@ #include <linux/init.h> #include <linux/idr.h> #include <linux/mutex.h> -#include <linux/of_i2c.h> #include <linux/of_device.h> +#include <linux/of_irq.h> #include <linux/completion.h> #include <linux/hardirq.h> #include <linux/irqflags.h> @@ -818,6 +818,63 @@ static void i2c_scan_static_board_info(struct i2c_adapter *adapter) up_read(&__i2c_board_lock); } +#ifdef CONFIG_OF +void i2c_scan_of_devices(struct i2c_adapter *adap) +{ + void *result; + struct device_node *node; + + /* Only register child devices if the adapter has a node pointer set */ + if (!adap->dev.of_node) + return; + + for_each_child_of_node(adap->dev.of_node, node) { + struct i2c_board_info info = {}; + struct dev_archdata dev_ad = {}; + const __be32 *addr; + int len; + + dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name); + if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) { + dev_err(&adap->dev, "of_i2c: modalias failure on %s\n", + node->full_name); + continue; + } + + addr = of_get_property(node, "reg", &len); + if (!addr || (len < sizeof(int))) { + dev_err(&adap->dev, "of_i2c: invalid reg on %s\n", + node->full_name); + continue; + } + + info.addr = be32_to_cpup(addr); + if (info.addr > (1 << 10) - 1) { + dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n", + info.addr, node->full_name); + continue; + } + + info.irq = irq_of_parse_and_map(node, 0); + info.of_node = of_node_get(node); + info.archdata = &dev_ad; + + request_module("%s", info.type); + + result = i2c_new_device(adap, &info); + if (result == NULL) { + dev_err(&adap->dev, "of_i2c: Failure registering %s\n", + node->full_name); + of_node_put(node); + irq_dispose_mapping(info.irq); + continue; + } + } +} +#else +static inline void i2c_scan_of_devices(struct i2c_adapter *adap) { } +#endif + static int i2c_do_add_adapter(struct i2c_driver *driver, struct i2c_adapter *adap) { @@ -877,7 +934,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap) i2c_scan_static_board_info(adap); /* Register devices from the device tree */ - of_i2c_register_devices(adap); + i2c_scan_of_devices(adap); /* Notify drivers */ mutex_lock(&core_lock); diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c index 0a694de..e0c3841 100644 --- a/drivers/of/of_i2c.c +++ b/drivers/of/of_i2c.c @@ -17,63 +17,6 @@ #include <linux/of_irq.h> #include <linux/module.h> -void of_i2c_register_devices(struct i2c_adapter *adap) -{ - void *result; - struct device_node *node; - - /* Only register child devices if the adapter has a node pointer set */ - if (!adap->dev.of_node) - return; - - dev_dbg(&adap->dev, "of_i2c: walking child nodes\n"); - - for_each_child_of_node(adap->dev.of_node, node) { - struct i2c_board_info info = {}; - struct dev_archdata dev_ad = {}; - const __be32 *addr; - int len; - - dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name); - - if (of_modalias_node(node, info.type, sizeof(info.type)) < 0) { - dev_err(&adap->dev, "of_i2c: modalias failure on %s\n", - node->full_name); - continue; - } - - addr = of_get_property(node, "reg", &len); - if (!addr || (len < sizeof(int))) { - dev_err(&adap->dev, "of_i2c: invalid reg on %s\n", - node->full_name); - continue; - } - - info.addr = be32_to_cpup(addr); - if (info.addr > (1 << 10) - 1) { - dev_err(&adap->dev, "of_i2c: invalid addr=%x on %s\n", - info.addr, node->full_name); - continue; - } - - info.irq = irq_of_parse_and_map(node, 0); - info.of_node = of_node_get(node); - info.archdata = &dev_ad; - - request_module("%s", info.type); - - result = i2c_new_device(adap, &info); - if (result == NULL) { - dev_err(&adap->dev, "of_i2c: Failure registering %s\n", - node->full_name); - of_node_put(node); - irq_dispose_mapping(info.irq); - continue; - } - } -} -EXPORT_SYMBOL(of_i2c_register_devices); - static int of_dev_node_match(struct device *dev, void *data) { return dev->of_node == data; diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h index 0efe8d4..1998cf8 100644 --- a/include/linux/of_i2c.h +++ b/include/linux/of_i2c.h @@ -15,16 +15,9 @@ #if defined(CONFIG_OF_I2C) || defined(CONFIG_OF_I2C_MODULE) #include <linux/i2c.h> -extern void of_i2c_register_devices(struct i2c_adapter *adap); - /* must call put_device() when done with returned i2c_client device */ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node); -#else -static inline void of_i2c_register_devices(struct i2c_adapter *adap) -{ - return; -} #endif /* CONFIG_OF_I2C */ #endif /* __LINUX_OF_I2C_H */
Commit 959e85f7, "i2c: add OF-style registration and binding" caused a module dependency loop where of_i2c.c calls functions in i2c-core, and i2c-core calls of_i2c_register_devices() in of_i2c. This means that when i2c support is built as a module when CONFIG_OF is set, then neither i2c_core nor of_i2c are able to be loaded. This patch fixes the problem by moving the of_i2c_register_devices() function into the body of i2c_core and renaming it to i2c_scan_of_devices (of_i2c_register_devices is analogous to the existing i2c_scan_static_board_info function and so should be named similarly). This function isn't called by any code outside of i2c_core, and it must always be present when CONFIG_OF is selected, so it makes sense to locate it there. When CONFIG_OF is not selected, of_i2c_register_devices() becomes a no-op. Signed-off-by: Grant Likely <grant.likely@secretlab.ca> --- drivers/i2c/i2c-core.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++-- drivers/of/of_i2c.c | 57 --------------------------------------------- include/linux/of_i2c.h | 7 ------ 3 files changed, 59 insertions(+), 66 deletions(-)