[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4F7205F3.3000108@cavium.com>
Date: Tue, 27 Mar 2012 11:24:51 -0700
From: David Daney <david.daney@...ium.com>
To: Rob Herring <robherring2@...il.com>
CC: David Daney <ddaney.cavm@...il.com>, linux-mips@...ux-mips.org,
ralf@...ux-mips.org, devicetree-discuss@...ts.ozlabs.org,
Grant Likely <grant.likely@...retlab.ca>,
Rob Herring <rob.herring@...xeda.com>,
linux-kernel@...r.kernel.org
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@...ium.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists