From: =?iso-8859-1?Q?M=E5ns_Rullg=E5rd?= <mans@mansr.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, Jason Cooper <jason@lakedaemon.net>,
        Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller
References: <1447958026-3015-1-git-send-email-mans@mansr.com>
	<1447958026-3015-3-git-send-email-mans@mansr.com>
	<564EF235.7030207@arm.com>
Date: Fri, 20 Nov 2015 12:00:28 +0000
In-Reply-To: <564EF235.7030207@arm.com> (Marc Zyngier's message of "Fri, 20
	Nov 2015 10:13:09 +0000")
Message-ID: <yw1xfv00ev8j.fsf@unicorn.mansr.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3082
Lines: 98

Marc Zyngier <marc.zyngier@arm.com> writes:

>> +static void tangox_dispatch_irqs(struct irq_domain *dom, unsigned int status,
>> +				 int base)
>> +{
>> +	unsigned int hwirq;
>> +	unsigned int virq;
>> +
>> +	while (status) {
>> +		hwirq = __ffs(status);
>> +		virq = irq_find_mapping(dom, base + hwirq);
>
> You may want to check virq in case you get interrupts from unexpected
> sources (unlikely, but still).

Sure, never hurts to be safe.

>> +		generic_handle_irq(virq);
>> +		status &= ~BIT(hwirq);
>> +	}
>> +}

[...]

>> +static int __init tangox_irq_init(void __iomem *base, struct device_node *node)
>> +{
>> +	struct tangox_irq_chip *chip;
>> +	struct irq_domain *dom;
>> +	const char *name;
>> +	u32 ctl;
>> +	int irq;
>> +	int err;
>> +	int i;
>> +
>> +	irq = irq_of_parse_and_map(node, 0);
>> +	if (!irq)
>> +		panic("%s: failed to get IRQ", node->name);
>> +
>> +	if (of_property_read_u32(node, "sigma,reg-offset", &ctl))
>> +		panic("%s: failed to get reg base", node->name);
>
> My DT foo is a bit crap, but I'm sure there is ways to express ranges
> inside a region that do not require to have vendor-specific properties.
> Mark?

I wasn't happy about that either.  The usual way is to use a "ranges"
property in the parent node and "reg" in the child node.  That makes it
easy to obtain a mapping of the child range using of_iomap() or
whatever.  The problem is that that's not what I need here.  The type
and ack registers are common while the enable/disable registers are per
sub-block, and the generic irqchip structs use a single base address and
offsets for the various registers, so I need the offset from the common
base to the start of the per-block registers, not the actual full
address.  I could use of_address_to_resource() and subtract one from the
other, I suppose.

>> +
>> +	if (of_property_read_string(node, "label", &name))
>> +		name = node->name;
>
> Do you really need this cosmetic thing? node->name should be enough for
> everybody, and the "label" has nothing to do with the HW description.

No, it's not needed.  I'll get rid of it.

>> +
>> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
>> +	chip->ctl = ctl;
>> +	chip->base = base;
>> +
>> +	dom = irq_domain_add_linear(node, 64, &irq_generic_chip_ops, chip);
>> +	if (!dom)
>> +		panic("%s: failed to create irqdomain", node->name);
>> +
>> +	err = irq_alloc_domain_generic_chips(dom, 32, 2, name, handle_level_irq,
>> +					     0, 0, 0);
>> +	if (err)
>> +		panic("%s: failed to allocate irqchip", node->name);
>> +
>> +	tangox_irq_domain_init(dom);
>> +
>> +	for (i = 0; i < 64; i++)
>> +		irq_create_mapping(dom, i);
>
> /me puzzled. What's that for? You really should never need something
> like this.

I had some reason for doing when I first wrote this code (MIPS, no DT),
but it's not needed now.

-- 
M?ns Rullg?rd
mans@mansr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/