Patchwork [(Option,1)] of/i2c: fix module load order issue caused by of_i2c.c

login
register
mail settings
Submitter Grant Likely
Date Sept. 24, 2010, 10:14 p.m.
Message ID <20100924221313.28357.61419.stgit@angua>
Download mbox | patch
Permalink /patch/65702/
State Not Applicable
Headers show

Comments

Grant Likely - Sept. 24, 2010, 10:14 p.m.
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(-)
Ben Dooks - Sept. 28, 2010, 11:20 p.m.
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.
Ben Dooks - Sept. 28, 2010, 11:21 p.m.
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?
Grant Likely - Sept. 28, 2010, 11:26 p.m.
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.
>
>
Jean Delvare - Sept. 29, 2010, 2:42 p.m.
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,
Grant Likely - Sept. 30, 2010, 9:11 p.m.
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.

Patch

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 */