Patchwork [4/9] of/irq: Introduce of_irq_get()

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

Comments

Thierry Reding - Sept. 16, 2013, 8:32 a.m.
This is a version of irq_of_parse_and_map() that propagates the precise
error code instead of returning 0 for all errors. It will be used in
subsequent patches to allow further propagation of error codes.

To avoid code duplication, implement irq_of_parse_and_map() as a wrapper
around the new of_irq_get().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/of/irq.c       | 21 +++++++++++++++++----
 include/linux/of_irq.h |  3 +++
 2 files changed, 20 insertions(+), 4 deletions(-)
Rob Herring - Sept. 16, 2013, 9:24 p.m.
On 09/16/2013 03:32 AM, Thierry Reding wrote:
> This is a version of irq_of_parse_and_map() that propagates the precise
> error code instead of returning 0 for all errors. It will be used in
> subsequent patches to allow further propagation of error codes.
> 
> To avoid code duplication, implement irq_of_parse_and_map() as a wrapper
> around the new of_irq_get().

*_get typically also implies some reference counting which I don't
believe this does. I don't think having 2 functions with completely
different names doing the same thing with only a different calling
convention is good either. So I would keep the old name and the names
aligned.

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/of/irq.c       | 21 +++++++++++++++++----
>  include/linux/of_irq.h |  3 +++
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 5f44388..8225289 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -26,6 +26,20 @@
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  
> +int of_irq_get(struct device_node *dev, unsigned int index, unsigned int *virqp)
> +{
> +	struct of_irq oirq;
> +	int ret;
> +
> +	ret = of_irq_map_one(dev, index, &oirq);
> +	if (ret)
> +		return ret;
> +
> +	return __irq_create_of_mapping(oirq.controller, oirq.specifier,
> +				       oirq.size, virqp);
> +}
> +EXPORT_SYMBOL_GPL(of_irq_get);
> +
>  /**
>   * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
>   * @dev: Device node of the device whose interrupt is to be mapped
> @@ -36,13 +50,12 @@
>   */
>  unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
>  {
> -	struct of_irq oirq;
> +	unsigned int virq;
>  
> -	if (of_irq_map_one(dev, index, &oirq))
> +	if (of_irq_get(dev, index, &virq))
>  		return 0;
>  
> -	return irq_create_of_mapping(oirq.controller, oirq.specifier,
> -				     oirq.size);
> +	return virq;

This can be an inline and written more concisely:

{
	unsigned int virq;
	return (of_irq_get(dev, index, &virq) < 0) ? 0 : virq;
}

Rob


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - Sept. 17, 2013, 1:28 p.m.
On Mon, Sep 16, 2013 at 04:24:47PM -0500, Rob Herring wrote:
> On 09/16/2013 03:32 AM, Thierry Reding wrote:
> > This is a version of irq_of_parse_and_map() that propagates the precise
> > error code instead of returning 0 for all errors. It will be used in
> > subsequent patches to allow further propagation of error codes.
> > 
> > To avoid code duplication, implement irq_of_parse_and_map() as a wrapper
> > around the new of_irq_get().
> 
> *_get typically also implies some reference counting which I don't
> believe this does. I don't think having 2 functions with completely
> different names doing the same thing with only a different calling
> convention is good either. So I would keep the old name and the names
> aligned.

Okay, I'll make the new function __irq_of_parse_and_map().

> >  unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
> >  {
> > -	struct of_irq oirq;
> > +	unsigned int virq;
> >  
> > -	if (of_irq_map_one(dev, index, &oirq))
> > +	if (of_irq_get(dev, index, &virq))
> >  		return 0;
> >  
> > -	return irq_create_of_mapping(oirq.controller, oirq.specifier,
> > -				     oirq.size);
> > +	return virq;
> 
> This can be an inline and written more concisely:
> 
> {
> 	unsigned int virq;
> 	return (of_irq_get(dev, index, &virq) < 0) ? 0 : virq;
> }

I find code such as the above very hard to read. But if you insist I can
move the function into include/of/of_irq.h and make it static inline
with the implementation above.

Thierry
Linus Walleij - Sept. 23, 2013, 7:18 p.m.
On Tue, Sep 17, 2013 at 3:28 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Sep 16, 2013 at 04:24:47PM -0500, Rob Herring wrote:

>> *_get typically also implies some reference counting which I don't
>> believe this does. I don't think having 2 functions with completely
>> different names doing the same thing with only a different calling
>> convention is good either. So I would keep the old name and the names
>> aligned.
>
> Okay, I'll make the new function __irq_of_parse_and_map().

I don't know why i detest __prefixing so much but I think it's
really nasty.

Usually this is reserved for compiler- and linker related things,
like __packed;  or __init.

I would prefer irq_of_parse_and_map_strict() or something
like that.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding - Sept. 23, 2013, 8:49 p.m.
On Mon, Sep 23, 2013 at 09:18:52PM +0200, Linus Walleij wrote:
> On Tue, Sep 17, 2013 at 3:28 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 16, 2013 at 04:24:47PM -0500, Rob Herring wrote:
> 
> >> *_get typically also implies some reference counting which I don't
> >> believe this does. I don't think having 2 functions with completely
> >> different names doing the same thing with only a different calling
> >> convention is good either. So I would keep the old name and the names
> >> aligned.
> >
> > Okay, I'll make the new function __irq_of_parse_and_map().
> 
> I don't know why i detest __prefixing so much but I think it's
> really nasty.
> 
> Usually this is reserved for compiler- and linker related things,
> like __packed;  or __init.
> 
> I would prefer irq_of_parse_and_map_strict() or something
> like that.

Following from the discussion on one of the other patches, perhaps this
could also be converted to return int (instead of unsigned int), convert
all callers to cope with negative or 0 (instead of only 0) as errors and
then modify it to actually start returning negative error codes once all
users have been updated. Eventually I think we should get rid of using 0
as an error indicator altogether, but as I already mentioned it might be
difficult to do because new users can always come along and use the then
deprecated 0 return on error.

I'm somewhat worried about the amount of work that this is turning into.

Thierry

Patch

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 5f44388..8225289 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -26,6 +26,20 @@ 
 #include <linux/string.h>
 #include <linux/slab.h>
 
+int of_irq_get(struct device_node *dev, unsigned int index, unsigned int *virqp)
+{
+	struct of_irq oirq;
+	int ret;
+
+	ret = of_irq_map_one(dev, index, &oirq);
+	if (ret)
+		return ret;
+
+	return __irq_create_of_mapping(oirq.controller, oirq.specifier,
+				       oirq.size, virqp);
+}
+EXPORT_SYMBOL_GPL(of_irq_get);
+
 /**
  * irq_of_parse_and_map - Parse and map an interrupt into linux virq space
  * @dev: Device node of the device whose interrupt is to be mapped
@@ -36,13 +50,12 @@ 
  */
 unsigned int irq_of_parse_and_map(struct device_node *dev, int index)
 {
-	struct of_irq oirq;
+	unsigned int virq;
 
-	if (of_irq_map_one(dev, index, &oirq))
+	if (of_irq_get(dev, index, &virq))
 		return 0;
 
-	return irq_create_of_mapping(oirq.controller, oirq.specifier,
-				     oirq.size);
+	return virq;
 }
 EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
 
diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index c383dd1..cac15d7 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -17,6 +17,9 @@  struct of_irq;
  */
 extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
 
+extern int of_irq_get(struct device_node *node, unsigned int index,
+		      unsigned int *virqp);
+
 #if defined(CONFIG_OF_IRQ)
 /**
  * of_irq - container for device_node/irq_specifier pair for an irq controller