Re: [PATCH 04/15] x86/dtb: add irq domain abstraction

[Grant Likely]

On Fri, Dec 17, 2010 at 04:33:42PM +0100, Sebastian Andrzej Siewior wrote:
> The here introduced irq_domain abstraction represents a generic irq
> controller. It is a subset of powerpc's irq_host which is going to be
> renamed to irq_domain and then become generic. This implementation will
> be removed once it is generic.
> 
> The xlate callback is resposible to parse irq informations like irq type
> and number and returns the hardware irq number which is reported by the
> hardware as active.
> 
> Cc: devicetree-discuss@xxxxxxxxxxxxxxxx
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> Tested-by: Dirk Brandewie <dirk.brandewie@xxxxxxxxx>
> ---
>  arch/x86/include/asm/irq_controller.h |   12 ++++++++
>  arch/x86/include/asm/prom.h           |    2 +
>  arch/x86/kernel/prom.c                |   47 ++++++++++++++++++++++++++++++++-
>  3 files changed, 60 insertions(+), 1 deletions(-)
>  create mode 100644 arch/x86/include/asm/irq_controller.h
> 
> diff --git a/arch/x86/include/asm/irq_controller.h b/arch/x86/include/asm/irq_controller.h
> new file mode 100644
> index 0000000..423bbbd
> --- /dev/null
> +++ b/arch/x86/include/asm/irq_controller.h
> @@ -0,0 +1,12 @@
> +#ifndef __IRQ_CONTROLLER__
> +#define __IRQ_CONTROLLER__
> +
> +struct irq_domain {
> +	int (*xlate)(struct irq_domain *h, const u32 *intspec, u32 intsize,
> +			u32 *out_hwirq, u32 *out_type);
> +	void *priv;
> +	struct device_node *controller;
> +	struct list_head l;
> +};

True, it's not a full implementation, but I'd rather the same
structure be used for both powerpc and x86.

Thomas, this only touches x86, so I have no objection to you merging
it, but I'd like to see a follow up patch to merge the x86 and powerpc
irq_host/irq_domain structures.

However, there is a defect below that must be fixed first....

>  unsigned int irq_create_of_mapping(struct device_node *controller,
>  		const u32 *intspec, unsigned int intsize)
>  {
> -	return intspec[0];
> +	struct irq_domain *ih;
> +	u32 virq;
> +	u32 type;
> +	int ret;
>  
> +	ih = get_ih_from_node(controller);
> +	if (!ih)
> +		return -ENODEV;

Return value is an unsigned int.  0 or NO_IRQ is the correct thing to
return if an IRQ cannot be mapped.

> +	ret = ih->xlate(ih, intspec, intsize, &virq, &type);
> +	if (ret)
> +		return ret;

Ditto here.  xlate is also supposed to return a virq number, not an
error code, so the failure condition must be "if (!ret)" or
"if (reg == NO_IRQ)".  The former is preferred since we're trying to
eliminate references to NO_IRQ, not add more of them.

> +	if (type == IRQ_TYPE_NONE)
> +		return virq;
> +	/* set the mask if it is different from current */
> +	if (type == (irq_to_desc(virq)->status & IRQF_TRIGGER_MASK))
> +		set_irq_type(virq, type);
> +	return virq;
>  }
>  EXPORT_SYMBOL_GPL(irq_create_of_mapping);
>  
> -- 
> 1.7.3.2
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@xxxxxxxxxxxxxxxx
> https://lists.ozlabs.org/listinfo/devicetree-discuss
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/