Patchwork [8/9] of/i2c: Resolve interrupt references at probe time

login
register
mail settings
Submitter Thierry Reding
Date Sept. 16, 2013, 8:32 a.m.
Message ID <1379320326-13241-9-git-send-email-treding@nvidia.com>
Download mbox | patch
Permalink /patch/275150/
State Changes Requested
Headers show

Comments

Thierry Reding - Sept. 16, 2013, 8:32 a.m.
Instead of resolving interrupt references at device creation time, delay
resolution until probe time. At device creation time, there is nothing
that can be done if an interrupt parent isn't ready yet, and the device
will end up with an invalid interrupt number (0).

If the interrupt reference is resolved at probe time, the device's probe
can be deferred, so that it's interrupt resolution can be retried after
more devices (possibly including its interrupt parent) have been probed.

However, individual drivers shouldn't be required to do that themselves,
over and over again, so this commit implements this functionality within
the I2C core.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/i2c/i2c-core.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
Wolfram Sang - Sept. 23, 2013, 7:34 a.m.
On Mon, Sep 16, 2013 at 10:32:05AM +0200, Thierry Reding wrote:
> Instead of resolving interrupt references at device creation time, delay
> resolution until probe time. At device creation time, there is nothing
> that can be done if an interrupt parent isn't ready yet, and the device
> will end up with an invalid interrupt number (0).
> 
> If the interrupt reference is resolved at probe time, the device's probe
> can be deferred, so that it's interrupt resolution can be retried after
> more devices (possibly including its interrupt parent) have been probed.
> 
> However, individual drivers shouldn't be required to do that themselves,
> over and over again, so this commit implements this functionality within
> the I2C core.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/i2c/i2c-core.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 29d3f04..163a1e8 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -236,6 +236,21 @@ int i2c_recover_bus(struct i2c_adapter *adap)
>  	return adap->bus_recovery_info->recover_bus(adap);
>  }
>  
> +static int of_i2c_probe(struct i2c_client *client)

This function should be in the CONFIG_OF block.

> +{
> +	struct device_node *np = client->dev.of_node;
> +	int ret;
> +
> +	/* skip if the device node specifies no interrupts */
> +	if (of_get_property(np, "interrupts", NULL)) {
> +		ret = of_irq_get(client->dev.of_node, 0, &client->irq);

of_irq_get really sounds as it does refcounting. +1 to change the name.

Thanks,

   Wolfram
Thierry Reding - Sept. 23, 2013, 8:02 a.m.
On Mon, Sep 23, 2013 at 09:34:51AM +0200, Wolfram Sang wrote:
> On Mon, Sep 16, 2013 at 10:32:05AM +0200, Thierry Reding wrote:
> > Instead of resolving interrupt references at device creation time, delay
> > resolution until probe time. At device creation time, there is nothing
> > that can be done if an interrupt parent isn't ready yet, and the device
> > will end up with an invalid interrupt number (0).
> > 
> > If the interrupt reference is resolved at probe time, the device's probe
> > can be deferred, so that it's interrupt resolution can be retried after
> > more devices (possibly including its interrupt parent) have been probed.
> > 
> > However, individual drivers shouldn't be required to do that themselves,
> > over and over again, so this commit implements this functionality within
> > the I2C core.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/i2c/i2c-core.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index 29d3f04..163a1e8 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -236,6 +236,21 @@ int i2c_recover_bus(struct i2c_adapter *adap)
> >  	return adap->bus_recovery_info->recover_bus(adap);
> >  }
> >  
> > +static int of_i2c_probe(struct i2c_client *client)
> 
> This function should be in the CONFIG_OF block.

I'll argue the other way. I think that the whole CONFIG_OF block should
be remove, or at least be confined to public functions. The reason that
I've chosen to do it this way is that the i2c_device_probe() uses an
IS_ENABLED(CONFIG_OF), which will evaluate to 0 if OF isn't selected
which in turn will cause the compiler to throw away of_i2c_probe()
because it is static and never used.

The big advantage of this is that we get compile coverage for these
functions independent of what configuration is built. The same could
probably be done for the whole CONFIG_ACPI block. In fact I volunteer to
provide that patch if I've convinced you that this is actually better
than sprinking the code with the #ifdef blocks that make it harder than
necessary to verify that the code builds in all configurations.

> > +{
> > +	struct device_node *np = client->dev.of_node;
> > +	int ret;
> > +
> > +	/* skip if the device node specifies no interrupts */
> > +	if (of_get_property(np, "interrupts", NULL)) {
> > +		ret = of_irq_get(client->dev.of_node, 0, &client->irq);
> 
> of_irq_get really sounds as it does refcounting. +1 to change the name.

Done in v2.

Thanks,
Thierry
Wolfram Sang - Sept. 23, 2013, 8:35 a.m.
> > This function should be in the CONFIG_OF block.
> 
> I'll argue the other way. I think that the whole CONFIG_OF block should
> be remove, or at least be confined to public functions. The reason that
> I've chosen to do it this way is that the i2c_device_probe() uses an
> IS_ENABLED(CONFIG_OF), which will evaluate to 0 if OF isn't selected
> which in turn will cause the compiler to throw away of_i2c_probe()
> because it is static and never used.
> 
> The big advantage of this is that we get compile coverage for these
> functions independent of what configuration is built. The same could
> probably be done for the whole CONFIG_ACPI block. In fact I volunteer to
> provide that patch if I've convinced you that this is actually better
> than sprinking the code with the #ifdef blocks that make it harder than
> necessary to verify that the code builds in all configurations.

Yes, you (easily) did :) Still, I'd like the of_i2c_probe function
near the other of_* functions.

Thanks,

   Wolfram

Patch

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 29d3f04..163a1e8 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -236,6 +236,21 @@  int i2c_recover_bus(struct i2c_adapter *adap)
 	return adap->bus_recovery_info->recover_bus(adap);
 }
 
+static int of_i2c_probe(struct i2c_client *client)
+{
+	struct device_node *np = client->dev.of_node;
+	int ret;
+
+	/* skip if the device node specifies no interrupts */
+	if (of_get_property(np, "interrupts", NULL)) {
+		ret = of_irq_get(client->dev.of_node, 0, &client->irq);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -254,6 +269,12 @@  static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+		status = of_i2c_probe(client);
+		if (status)
+			return status;
+	}
+
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
 	if (status) {
 		client->driver = NULL;
@@ -1002,7 +1023,6 @@  static void of_i2c_register_devices(struct i2c_adapter *adap)
 			continue;
 		}
 
-		info.irq = irq_of_parse_and_map(node, 0);
 		info.of_node = of_node_get(node);
 		info.archdata = &dev_ad;
 
@@ -1016,7 +1036,6 @@  static void of_i2c_register_devices(struct i2c_adapter *adap)
 			dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
 				node->full_name);
 			of_node_put(node);
-			irq_dispose_mapping(info.irq);
 			continue;
 		}
 	}