Re: [PATCH 2/2] irqchip: add support for Sigma Designs SMP86xx interrupt controller

["Måns Rullgård" <mans@mansr.com>]

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