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

2010-09-29 Thread Jean Delvare 
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,
-- 
Jean Delvare
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2010-09-28 Thread Ben Dooks 
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.lik...@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?

-- 
Ben

Q:  What's a light-year?
A:  One-third less calories than a regular year.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2010-09-28 Thread Ben Dooks 
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

Q:  What's a light-year?
A:  One-third less calories than a regular year.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

2010-09-24 Thread Grant Likely 
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.lik...@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
+
 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