MFD and GPIO for STA2X11

- You should not select GPIO_STA2X11 as this won't select the STA2X211 gpio driver dependencies for you. So if you build this driver with GPIOLIB=n then your build will fail.

- Why would this one depend on STA2X11 ? I see that you're calling sta2x11_get_instance() from this driver but I wonder if you could simply replace the mfd->instance pointer with one pointing to the pci_dev. Not depending on STA2X11 at all gives you the benefit of a much larger build coverage from linux-next for example.

drivers/mfd/sta2x11-mfd.c line 297

Quoted 10 lines

drivers/mfd/sta2x11-mfd.c new file +10 -0

+/* Bar 0 */
+enum bar0_cells {
+	GPIO_0 = 0,
+	GPIO_1,
+	GPIO_2,
+	GPIO_3,
+	SCTL,
+	SCR,
+	TIME,
+};

Please protect those ones with a proper namespace.

This also introduces <asm/sta2x11.h> to export a function that is in the base sta2x11 support patches. The header will increase with other prototypes and constants over time.

drivers/gpio/gpio-sta2x11.c new file +57 -0

+/*
+ * Special method: alternate functions and pullup/pulldown. This is only
+ * invoked on startup to configure gpio's according to platform data.
+ * FIXME : this functionality shall be managed (and exported to other drivers)
+ * via the pin control subsystem.
+ */
+static void gsta_set_config(struct gsta_gpio *chip, int nr, unsigned cfg)
+{
+       struct gsta_regs __iomem *regs = __regs(chip, nr);
+       unsigned long flags;
+       u32 bit = __bit(nr);
+       u32 val;
+       int err = 0;
+
+       pr_info("%s: %p %i %i\n", __func__, chip, nr, cfg);
+
+       if (cfg == PINMUX_TYPE_NONE)
+               return;
+
+       /* Alternate function or not? */
+       spin_lock_irqsave(&chip->lock, flags);
+       val = readl(&regs->afsela);
+       if (cfg == PINMUX_TYPE_FUNCTION)
+               val |= bit;
+       else
+               val &= ~bit;
+       writel(val | bit, &regs->afsela);
+       if (cfg == PINMUX_TYPE_FUNCTION) {
+               spin_unlock_irqrestore(&chip->lock, flags);
+               return;
+       }
+
+       /* not alternate function: set details */
+       switch (cfg) {
+       case PINMUX_TYPE_OUTPUT_LOW:
+               writel(bit, &regs->dirs);
+               writel(bit, &regs->datc);
+               break;
+       case PINMUX_TYPE_OUTPUT_HIGH:
+               writel(bit, &regs->dirs);
+               writel(bit, &regs->dats);
+               break;
+       case PINMUX_TYPE_INPUT:
+               writel(bit, &regs->dirc);
+               val = readl(&regs->pdis) | bit;
+               writel(val, &regs->pdis);
+               break;
+       case PINMUX_TYPE_INPUT_PULLUP:
+               writel(bit, &regs->dirc);
+               val = readl(&regs->pdis) & ~bit;
+               writel(val, &regs->pdis);
+               writel(bit, &regs->dats);
+               break;
+       case PINMUX_TYPE_INPUT_PULLDOWN:
+               writel(bit, &regs->dirc);
+               val = readl(&regs->pdis) & ~bit;
+               writel(val, &regs->pdis);

I would still prefer to use the pinctrl subsystem for this from day 1 instead of adding more custom stuff for later refactoring...

Besides, it's still marked as experimental, which on one side is not reassuring about its stability, and on the other means my whole chipset should depend on experimental as a consequence.

This introduces 128 gpio bits (for each PCI device installed) with working interrupt support.

Signed-off-by Alessandro Rubini <[email protected]>

Acked-by Giancarlo Asnaghi <[email protected]>

Cc Alan Cox <[email protected]>

This introduces 128 gpio bits (for each PCI device installed) with working interrupt support.

This introduces 128 gpio bits (for each PCI device installed) with working interrupt support.

Signed-off-by Alessandro Rubini <[email protected]>

Acked-by Giancarlo Asnaghi <[email protected]>

Cc Alan Cox <[email protected]>

---

drivers/gpio/gpio-sta2x11.c new file +32 -0

+/* The platform device used here is instantiated by the MFD device */
+static int __devinit gsta_probe(struct platform_device *dev)
+{
+	int i, err;
+	struct pci_dev *pdev;
+	struct sta2x11_gpio_pdata *gpio_pdata;
+	struct gsta_gpio *chip;
+	struct resource *res;
+
+	pdev = *(struct pci_dev **)(dev->dev.platform_data);
+	gpio_pdata = dev_get_platdata(&pdev->dev);
+
+	if (gpio_pdata == NULL)
+		dev_err(&dev->dev, "no gpio config\n");
+	pr_debug("gpio config: %p\n", gpio_pdata);
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+
+	chip = devm_kzalloc(&dev->dev, sizeof(*chip), GFP_KERNEL);
+	chip->dev = &dev->dev;
+	chip->reg_base = devm_request_and_ioremap(&dev->dev, res);
+
+	for (i = 0; i < GSTA_NR_BLOCKS; i++) {
+		chip->regs[i] = chip->reg_base + i * 4096;
+		/* disable all irqs */
+		writel(0, &chip->regs[i]->rimsc);
+		writel(0, &chip->regs[i]->fimsc);
+		writel(~0, &chip->regs[i]->ic);
+	}
+	spin_lock_init(&chip->lock);
+	gsta_gpio_setup(chip);
+	for (i = 0; i < GSTA_NR_GPIO; i++)

drivers/gpio/gpio-sta2x11.c line 390

Quoted 2 lines

drivers/gpio/gpio-sta2x11.c new file +2 -0

+	/* 384 was used in previous code: be compatible for other drivers */
+	err = irq_alloc_descs(-1, 384, GSTA_NR_GPIO, NUMA_NO_NODE);

Where does the number 384 come from? It looks like the driver only needs to allocate a range of irqs and that it doesn't actually matter what the real numbers are. Can 0 be used instead?

Actually, I'd rather see this driver switched to using irq_domain_add_linear so that irq_descs can be allocated on demand instead of all at once. That way only gpios actually used for irqs get setup.

Quoted 6 lines

drivers/gpio/gpio-sta2x11.c line 390

Quoted 2 lines

drivers/gpio/gpio-sta2x11.c new file +2 -0

+	/* 384 was used in previous code: be compatible for other drivers */
+	err = irq_alloc_descs(-1, 384, GSTA_NR_GPIO, NUMA_NO_NODE);

Where does the number 384 come from? It looks like the driver only needs to allocate a range of irqs and that it doesn't actually matter what the real numbers are. Can 0 be used instead?

The problem is that there are a number of drivers already working (but not ready to be upstreamed), and we'd better continue using them. So, the mmc driver is requesting a specific interrupt number. The code is GPL (published on sourceforge) and I'd better remain compatible.

I'm not completely clear (yet) about how to get the right interrupt number in those other drivers, but I'm willing to remove the constraint as they are cleaned up and submitted.

Quoted 3 lines

I'm not completely clear (yet) about how to get the right interrupt number in those other drivers, but I'm willing to remove the constraint as they are cleaned up and submitted.

Use platform data for both this driver and the other drivers (or device tree if you're doing that). This will hard code the magic numbers in the board files, not in the driver.

Me:

Quoted 3 lines

Quoted 3 lines

I'm not completely clear (yet) about how to get the right interrupt number in those other drivers, but I'm willing to remove the constraint as they are cleaned up and submitted.

Mark Brown:

Quoted 3 lines

Use platform data for both this driver and the other drivers (or device tree if you're doing that). This will hard code the magic numbers in the board files, not in the driver.

Yes, but it's not that easy. If the gpio driver gets unpredictable interrupt numbers associated to the pins, the other drivers must recover those numbers in some way. That's why I currently started from 384, like the prevoous gpio driver was doing: the platform data uses that knowledge in the drivers I'm still using internally (but are not submittable as-is, so I'm working on them).

If you use platform data why would you get unpredicatable numbers?

Quoted 1 line

If you use platform data why would you get unpredicatable numbers?

Because I request the irq numbers from the gpio driver (now I request a free slot starting from 384, because previous drivers had it hardwired). So if the gpio driver requests a slot starting from 0 it may get different values -- in general I can't now what it gets.

Hello Grant. Sorry for the delay.

Quoted 6 lines

drivers/gpio/gpio-sta2x11.c line 390

Quoted 2 lines

drivers/gpio/gpio-sta2x11.c new file +2 -0

+	/* 384 was used in previous code: be compatible for other drivers */
+	err = irq_alloc_descs(-1, 384, GSTA_NR_GPIO, NUMA_NO_NODE);

Where does the number 384 come from? It looks like the driver only needs to allocate a range of irqs and that it doesn't actually matter what the real numbers are. Can 0 be used instead?

The problem is that there are a number of drivers already working (but not ready to be upstreamed), and we'd better continue using them. So, the mmc driver is requesting a specific interrupt number. The code is GPL (published on sourceforge) and I'd better remain compatible.

I'm not completely clear (yet) about how to get the right interrupt number in those other drivers, but I'm willing to remove the constraint as they are cleaned up and submitted.

Quoted 4 lines

Actually, I'd rather see this driver switched to using irq_domain_add_linear so that irq_descs can be allocated on demand instead of all at once. That way only gpios actually used for irqs get setup.

If it is a request, I'll evaluate it soon. I assume an incremental patch over what Samuel has already picked up is fine.

These two patches introduce the multi-function device and GPIO support for the STA2X11 I/O Hub. Core PCI support is already in linux-next.

V1: sent on Jan 26th as RFC and got no comments back

V2: sent on Feb 16th, fixed a number of minor details, got comments from Linus Walleij and Samuel Ortiz.

V3: accepted all suggestions. I delayed this resending a while because I hoped the base STA2X11 patches to be accepted, but despite the positive comments I got on those patches, they are not in linux-next as of today