Marc Zyngier <marc.zyngier@xxxxxxx> 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@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
