On Thu, 12 Apr 2012 10:48:55 +0200, Alessandro Rubini <[email protected]> wrote:
> 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]>
> ---
[...]
> +/* 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++)
> + gsta_set_config(chip, i, gpio_pdata->pinconfig[i]);
> +
> + /* 384 was used in previous code: be compatible for other drivers */
> + err = irq_alloc_descs(-1, 384, GSTA_NR_GPIO, NUMA_NO_NODE);
> + if (err < 0) {
> + dev_warn(&dev->dev, "sta2x11 gpio: Can't get irq base (%i)\n",
> + -err);
> + return err;
> + }
> + chip->irq_base = err;
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.
To convert, use irq_domain_add_linear() and then irq_data->hwirq gets
populated with the gpio number automatically for you and
irq_find_mapping takes care of the hwirq->irq reverse lookup.
Hello Grant. Sorry for the delay.
>> + /* 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.
> 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.
thanks
/alessandro
On Mon, May 14, 2012 at 09:25:34AM +0200, Alessandro Rubini wrote:
> >> + /* 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.
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:
>> 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:
> 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).
thanks
/alessandro
On Mon, May 14, 2012 at 11:51:11AM +0200, Alessandro Rubini wrote:
> Me:
> >> 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:
> > 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?
> 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.
So the irq number is unknown at compile time, and can't be written in
the platform data.
Actually, the problem is there in any case if you plug two chips in
the same computer -- this is extremely unlikely because the sta2x11 is
currently only used as a motherboard chipset. The thing must be
fixed in any case, because it's a pci device, but still using 384
allows me to use platform data for the current drivers that I'm running
until they are ported to upstream quality standards.
/alessandro
On Mon, May 14, 2012 at 12:06:45PM +0200, Alessandro Rubini wrote:
> > 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.
But this is because you're not using platform data! If you use platform
data to tell all the drivers including the gpio driver which ranges to
use then this problem does not exist.
Hi Alessandro,
On Mon, May 14, 2012 at 09:25:34AM +0200, Alessandro Rubini wrote:
> Hello Grant. Sorry for the delay.
>
> >> + /* 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.
>
> > 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.
That is fine with me at least.
Cheers,
Samuel.
--
Intel Open Source Technology Centre
http://oss.intel.com/