| Message ID | 1379320326-13241-5-git-send-email-treding@nvidia.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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
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
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
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
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
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(-)