On Fri, Jan 06, 2012 at 05:20:16PM +0100, Thierry Reding wrote:

* Grant Likely wrote:

quoted

Hi Thierry,

On Fri, Jan 6, 2012 at 7:28 AM, Thierry Reding
[off-list ref] wrote:

quoted

The irq_domain_add() function needs the number of interrupts in the
domain to properly initialize them. In addition the allocated domain
is now returned by the irq_domain_{add,generate}_simple() helpers.

The commit text should also include the justification for renaming
irq_domain_create_simple() -> irq_domain_add_simple()

Actually the commit only fixes up the comment. The function has always been
called irq_domain_add_simple().

For reference, this was introduced in commit 7e71330.

Hahaha.  Oops, you're right.  :-)

quoted

quoted

? ? ? ?domain = kzalloc(sizeof(*domain), GFP_KERNEL);
- ? ? ? if (!domain) {
- ? ? ? ? ? ? ? WARN_ON(1);
- ? ? ? ? ? ? ? return;
- ? ? ? }
+ ? ? ? if (!domain)
+ ? ? ? ? ? ? ? return ERR_PTR(-ENOMEM);

Don't use the ERR_PTR() pattern (it's a horrible pattern IMHO).

Returning NULL here is probably okay. Can the ERR_PTR stay in
irq_domain_generate_simple(), though? It has two error conditions and
handling both by returning NULL may not be what we want.

No.  ERR_PTR is a horrible pattern because you cannot tell by looking
at a prototype that returns a pointer whether or not the correct
failure test is "if (!ptr)" or "if (IS_ERR(ptr))".  Unless it is
absolutely critical for an error code to be returned (which isn't the
case here) I will not accept new code that uses ERR_PTR().

In this case, if irq_domain_add_simple() fails, then something is very
wrong.  I'd much rather the routine complain loudly regardless of the
error condition.

Actually, looking again at irq_domain_generate_simple() it should
probably succeed even if it cannot find a matching node since an
irq_domain does more than just device tree translation.  Although,
irq_domain_generate_simple() is a stop-gap solution that will
eventually be removed.

g.