On 03/26/2012 06:56 PM, Rob Herring wrote:

On 03/26/2012 02:31 PM, David Daney wrote:

quoted

From: David Daney<redacted>

[...]

quoted

+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.

quoted

+}
+
+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.

quoted

+		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.

[...]

quoted

+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