Messages in this thread | | | Date | Tue, 27 Mar 2012 11:24:51 -0700 | From | David Daney <> | Subject | Re: [PATCH v7 2/4] MIPS: Octeon: Setup irq_domains for interrupts. |
| |
On 03/26/2012 06:56 PM, Rob Herring wrote: > On 03/26/2012 02:31 PM, David Daney wrote: >> From: David Daney<david.daney@cavium.com> [...] >> +static bool octeon_irq_ciu_is_edge(unsigned int line, unsigned int bit) >> +{ >> + bool edge = false; >> + >> + if (line == 0) >> + switch (bit) { >> + case 48 ... 49: /* GMX DRP */ >> + case 50: /* IPD_DRP */ >> + case 52 ... 55: /* Timers */ >> + case 58: /* MPI */ >> + edge = true; >> + break; >> + default: >> + break; >> + } >> + else /* line == 1 */ >> + switch (bit) { >> + case 47: /* PTP */ >> + edge = true; >> + break; >> + default: >> + break; >> + } >> + return edge; > > Moving in the right direction, but I still don't get why this is not in > the CIU binding as a 3rd cell?
There are a several reasons, in no particular order they are:
o There is no 3rd cell. The bindings were discussed with Grant here: http://www.linux-mips.org/archives/linux-mips/2011-05/msg00355.html
o The edge/level thing cannot be changed, and the irq lines don't leave the SOC, so hard coding it is possible.
> >> +} >> + >> +struct octeon_irq_gpio_domain_data { >> + unsigned int base_hwirq; >> +}; >> + >> +static int octeon_irq_gpio_xlat(struct irq_domain *d, >> + struct device_node *node, >> + const u32 *intspec, >> + unsigned int intsize, >> + unsigned long *out_hwirq, >> + unsigned int *out_type) >> +{ >> + unsigned int type; >> + unsigned int pin; >> + unsigned int trigger; >> + bool set_edge_handler = false; >> + struct octeon_irq_gpio_domain_data *gpiod; >> + >> + if (d->of_node != node) >> + return -EINVAL; >> + >> + if (intsize< 2) >> + return -EINVAL; >> + >> + pin = intspec[0]; >> + if (pin>= 16) >> + return -EINVAL; >> + >> + trigger = intspec[1]; >> + >> + switch (trigger) { >> + case 1: >> + type = IRQ_TYPE_EDGE_RISING; >> + set_edge_handler = true; > > This is never used.
Right, it was leftover from the previous version.
> >> + break; >> + case 2: >> + type = IRQ_TYPE_EDGE_FALLING; >> + set_edge_handler = true; >> + break; >> + case 4: >> + type = IRQ_TYPE_LEVEL_HIGH; >> + break; >> + case 8: >> + type = IRQ_TYPE_LEVEL_LOW; >> + break; >> + default: >> + pr_err("Error: (%s) Invalid irq trigger specification: %x\n", >> + node->name, >> + trigger); >> + type = IRQ_TYPE_LEVEL_LOW; >> + break; >> + } >> + *out_type = type; > > Can't you get rid of the whole switch statement and just do: > > *out_type = intspec[1];
That wouldn't catch erroneous values like 6.
> [...] >> +static int octeon_irq_ciu_map(struct irq_domain *d, >> + unsigned int virq, irq_hw_number_t hw) >> +{ >> + unsigned int line = hw>> 6; >> + unsigned int bit = hw& 63; >> + >> + if (virq>= 256) >> + return -EINVAL; > > Drop this. You should not care what the virq numbers are.
I care that they don't overflow the width of octeon_irq_ciu_to_irq (a u8).
So really I want to say:
if (virq >= (1 << sizeof (octeon_irq_ciu_to_irq[0][0]))) { WARN(...); return -EINVAL; }
I need a map external to any one irq_domain. The irq handling code handles sources that come from two separate irq_domains, as well as irqs that are not part of any domain.
Thanks for looking at this again, I will re-spin the patch, David Daney
| |