On 26/01/16 07:22, Quan Nguyen wrote:

nit: missing @ before "flags".

  */
 struct xgene_gpio_sb {
 	struct gpio_chip	gc;
-	u32 *irq;
-	u32 nirq;
+	void __iomem		*regs;
+	struct irq_domain	*irq_domain;
+	u32			flags;
 };
 
-static void xgene_gpio_set_bit(struct gpio_chip *gc, void __iomem *reg, u32 gpio, int val)
+#define IRQ_START_PIN(priv)	(((priv)->flags >> 24) & 0xff)
+#define NIRQ_MAX(priv)		(((priv)->flags >> 16) & 0xff)
+#define NGPIO_MAX(priv)		(((priv)->flags >>  8) & 0xff)
+#define START_PARENT_IRQ(priv)	((priv)->flags & 0xff)
+

So flags is actually a set of fields, all 8bits, called irq_start, nirq,
ngpio, and parent_irq_base (or something along those lines).

You might as well make that explicit in your structure, as there is
hardly any point in hiding this information behind a bag of bits and a
set of obscure accessors...

This has no business whatsoever in set_type. This should be done either
when the GPIO is activated as an IRQ in the domain "activate" method, or
in the "startup" method of the irqchip.

+	if (ret) {
+		dev_err(priv->gc.parent,
+		"Unable to configure XGene GPIO standby pin %d as IRQ\n",
+				gpio);
+		return ret;
+	}
+
+	if ((gpio >= IRQ_START_PIN(priv)) &&
+			(d->hwirq < NIRQ_MAX(priv))) {

How can we end-up here if your GPIO is not part that range? This should
be guaranteed by construction.

+		xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+				gpio * 2, 1);
+		xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_INT_LVL,
+				d->hwirq, lvl_type);
+	}
+
+	/* Propagate IRQ type setting to parent */
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		return irq_chip_set_type_parent(d, IRQ_TYPE_EDGE_RISING);
+	else
+		return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
+}
+
+static void xgene_gpio_sb_irq_shutdown(struct irq_data *d)
+{
+	struct xgene_gpio_sb *priv = irq_data_get_irq_chip_data(d);
+
+	gpiochip_unlock_as_irq(&priv->gc, d->hwirq + IRQ_START_PIN(priv));
+}
+
+static struct irq_chip xgene_gpio_sb_irq_chip = {
+	.name           = "sbgpio",
+	.irq_ack        = irq_chip_ack_parent,
+	.irq_eoi	= irq_chip_eoi_parent,
+	.irq_mask       = irq_chip_mask_parent,
+	.irq_unmask     = irq_chip_unmask_parent,
+	.irq_set_type   = xgene_gpio_sb_irq_set_type,
+	.irq_shutdown   = xgene_gpio_sb_irq_shutdown,
+};
+
+static int xgene_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
 {
 	struct xgene_gpio_sb *priv = gpiochip_get_data(gc);
+	struct irq_fwspec fwspec;
+	unsigned int virq;
+
+	if ((gpio < IRQ_START_PIN(priv)) ||
+			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+		return -ENXIO;
+	if (gc->parent->of_node)
+		fwspec.fwnode = of_node_to_fwnode(gc->parent->of_node);
+	else
+		fwspec.fwnode = gc->parent->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = gpio - IRQ_START_PIN(priv);
+	fwspec.param[1] = IRQ_TYPE_NONE;
+	virq = irq_find_mapping(priv->irq_domain, gpio - IRQ_START_PIN(priv));
+	if (!virq)
+		virq =  irq_domain_alloc_irqs(priv->irq_domain, 1,
+						NUMA_NO_NODE, &fwspec);

You should not use these low-level functions directly. Use
irq_create_fwspec_mapping, which will do the right thing.

+	return virq;
+}
+
+static void xgene_gpio_sb_domain_activate(struct irq_domain *d,
+		struct irq_data *irq_data)
+{
+	struct xgene_gpio_sb *priv = d->host_data;
+	u32 gpio = irq_data->hwirq + IRQ_START_PIN(priv);
 
-	if (priv->irq[gpio])
-		return priv->irq[gpio];
+	if ((gpio < IRQ_START_PIN(priv)) ||
+			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+		return;

Again, how can this happen?

 
-	return -ENXIO;
+	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+			gpio * 2, 1);

This seems to program the interrupt to be active on a low level. Why?
Isn't that what set_type is supposed to do?

+}
+
+static void xgene_gpio_sb_domain_deactivate(struct irq_domain *d,
+		struct irq_data *irq_data)
+{
+	struct xgene_gpio_sb *priv = d->host_data;
+	u32 gpio = irq_data->hwirq + IRQ_START_PIN(priv);

It really feels like you need a hwirq_to_gpio() accessor.

+
+	if ((gpio < IRQ_START_PIN(priv)) ||
+			(gpio > NIRQ_MAX(priv) + IRQ_START_PIN(priv)))
+		return;

Why do we need this?

+
+	xgene_gpio_set_bit(&priv->gc, priv->regs + MPA_GPIO_SEL_LO,
+			gpio * 2, 0);
+}
+
+static int xgene_gpio_sb_domain_translate(struct irq_domain *d,
+		struct irq_fwspec *fwspec,
+		unsigned long *hwirq,
+		unsigned int *type)
+{
+	if (fwspec->param_count != 2)
+		return -EINVAL;
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1];
+	return 0;
+}
+
+static int xgene_gpio_sb_domain_alloc(struct irq_domain *domain,
+					unsigned int virq,
+					unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	struct irq_fwspec parent_fwspec;
+	struct xgene_gpio_sb *priv = domain->host_data;
+	irq_hw_number_t hwirq;
+	unsigned int type = IRQ_TYPE_NONE;
+	unsigned int i;
+	u32 ret;
+
+	ret = xgene_gpio_sb_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;

How can this fail here?

+
+	hwirq = fwspec->param[0];
+	if ((hwirq >= NIRQ_MAX(priv)) ||
+			(hwirq + nr_irqs > NIRQ_MAX(priv)))
+		return -EINVAL;

How can this happen?

Wait. Everything is hardcoded? So why do we have to deal with looking
into that structure if nothing is actually parametrized?

+#ifdef CONFIG_ACPI
+	else {
+		const struct acpi_device_id *acpi_id;
+
+		acpi_id = acpi_match_device(xgene_gpio_sb_acpi_match,
+				&pdev->dev);
+		if (acpi_id)
+			priv->flags = (uintptr_t)acpi_id->driver_data;
+	}
+#endif

nit: you can write this as

	if (of_id) {
		...
#ifdef ...
	} else {
		...
#endif
	}


Which preserves the Linux coding style.

+	ret = platform_get_irq(pdev, 0);
+	if (ret > 0) {
+		priv->flags &= ~0xff;
+		priv->flags |= irq_get_irq_data(ret)->hwirq & 0xff;
+		parent_domain = irq_get_irq_data(ret)->domain;
+	}

This is rather ugly. You have the interrupt-parent property. Why don't
you look it up, and do a irq_find_matching_fwnode? Also, what guarantee
do you have that the interrupts are going to be sorted in the DT? There
is no such garantee in the documentation.

So we do have duplicated information everywhere. Great.

 
-	priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
-				   GFP_KERNEL);
-	if (!priv->irq)
-		return -ENOMEM;
+	platform_set_drvdata(pdev, priv);
 
-	for (i = 0; i < priv->nirq; i++) {
-		priv->irq[default_lines[i]] = platform_get_irq(pdev, i);
-		xgene_gpio_set_bit(&priv->gc, regs + MPA_GPIO_SEL_LO,
-                                   default_lines[i] * 2, 1);
-		xgene_gpio_set_bit(&priv->gc, regs + MPA_GPIO_INT_LVL, i, 1);
-	}
+	priv->irq_domain = irq_domain_create_hierarchy(parent_domain,
+					0, NIRQ_MAX(priv),
+					of_node_to_fwnode(pdev->dev.of_node),
+					&xgene_gpio_sb_domain_ops, priv);
+	if (!priv->irq_domain)
+		return -ENODEV;
 
-	platform_set_drvdata(pdev, priv);
+	priv->gc.irqdomain = priv->irq_domain;
 
 	ret = gpiochip_add_data(&priv->gc, priv);
-	if (ret)
-		dev_err(&pdev->dev, "failed to register X-Gene GPIO Standby driver\n");
-	else
-		dev_info(&pdev->dev, "X-Gene GPIO Standby driver registered\n");
-
-	if (priv->nirq > 0) {
-		/* Register interrupt handlers for gpio signaled acpi events */
-		acpi_gpiochip_request_interrupts(&priv->gc);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register X-Gene GPIO Standby driver\n");
+		if (priv->irq_domain)

How can that not be true, given that you've tested irq_domain just above?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...