| Message ID | 1528276734-1483-2-git-send-email-j-keerthy@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: davinci: Move fetching IRQ to beginning of probe | expand |
On 06/06/2018 04:18 AM, Keerthy wrote: > Currently the driver assumes that the interrupts are continuous > and does platform_get_irq only once and assumes the rest are continuous, > instead call platform_get_irq for all the interrupts and store them > in an array for later use. > > Signed-off-by: Keerthy <j-keerthy@ti.com> > --- > > Tested for GPIO Interrupts on da850-lcdk and keystone-k2g-evm boards. > > drivers/gpio/gpio-davinci.c | 49 ++++++++++++++++++++++++++++++--------------- > 1 file changed, 33 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c > index 861f35b..375578c 100644 > --- a/drivers/gpio/gpio-davinci.c > +++ b/drivers/gpio/gpio-davinci.c > @@ -42,6 +42,7 @@ struct davinci_gpio_regs { > > #define BINTEN 0x8 /* GPIO Interrupt Per-Bank Enable Register */ > #define MAX_LABEL_SIZE 20 > +#define MAX_INT_PER_BANK 32 > > static void __iomem *gpio_base; > static unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0}; > @@ -55,7 +56,7 @@ static inline struct davinci_gpio_regs __iomem *irq2regs(struct irq_data *d) > return g; > } > > -static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq); > +static int davinci_gpio_irq_setup(struct platform_device *pdev, int *bank_irq); > > /*--------------------------------------------------------------------------*/ > > @@ -168,7 +169,8 @@ static int davinci_gpio_probe(struct platform_device *pdev) > { > static int ctrl_num, bank_base; > int gpio, bank, ret = 0; > - unsigned ngpio, nbank, bank_irq; > + unsigned int ngpio, nbank, nirq; > + int bank_irq[MAX_INT_PER_BANK], i; > struct davinci_gpio_controller *chips; > struct davinci_gpio_platform_data *pdata; > struct device *dev = &pdev->dev; > @@ -197,6 +199,16 @@ static int davinci_gpio_probe(struct platform_device *pdev) > if (WARN_ON(ARCH_NR_GPIOS < ngpio)) > ngpio = ARCH_NR_GPIOS; > > + /* > + * If there are unbanked interrupts then the number of > + * interrupts is equal to number of gpios else all are banked so > + * number of interrupts is equal to number of banks(each with 16 gpios) > + */ > + if (pdata->gpio_unbanked) > + nirq = pdata->gpio_unbanked; > + else > + nirq = DIV_ROUND_UP(ngpio, 16); > + > nbank = DIV_ROUND_UP(ngpio, 32); > chips = devm_kzalloc(dev, > nbank * sizeof(struct davinci_gpio_controller), > @@ -209,10 +221,13 @@ static int davinci_gpio_probe(struct platform_device *pdev) > if (IS_ERR(gpio_base)) > return PTR_ERR(gpio_base); > > - bank_irq = platform_get_irq(pdev, 0); > - if (bank_irq < 0) { > - dev_dbg(dev, "IRQ not populated\n"); > - return bank_irq; > + for (i = 0; i < nirq; i++) { > + bank_irq[i] = platform_get_irq(pdev, i); > + if (bank_irq[i] < 0) { > + dev_info(dev, "IRQ not populated, err = %d\n", > + bank_irq[i]); > + return bank_irq[i]; > + } > } > > snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++); > @@ -458,7 +473,7 @@ static struct irq_chip *keystone_gpio_get_irq_chip(unsigned int irq) > * (dm6446) can be set appropriately for GPIOV33 pins. > */ > > -static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) > +static int davinci_gpio_irq_setup(struct platform_device *pdev, int *bank_irq) > { > unsigned gpio, bank; > int irq; > @@ -492,6 +507,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) > dev_err(dev, "Error %ld getting gpio clock\n", PTR_ERR(clk)); > return PTR_ERR(clk); > } > + > ret = clk_prepare_enable(clk); > if (ret) > return ret; > @@ -531,12 +547,12 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) > if (pdata->gpio_unbanked) { > /* pass "bank 0" GPIO IRQs to AINTC */ > chips->chip.to_irq = gpio_to_irq_unbanked; > - chips->base_irq = bank_irq; > + chips->base_irq = bank_irq[0]; i think, you need to update gpio_to_irq_unbanked() also, which probably would require to save array of irqs. and gpio_irq_type_unbanked() [...] >
On Wed, Jun 6, 2018 at 11:18 AM, Keerthy <j-keerthy@ti.com> wrote: > Currently the driver assumes that the interrupts are continuous > and does platform_get_irq only once and assumes the rest are continuous, > instead call platform_get_irq for all the interrupts and store them > in an array for later use. > > Signed-off-by: Keerthy <j-keerthy@ti.com> Hm! Thierry has recently submitted patches to make it easier for chips with banked IRQs to use the GPIOLIB_IRQCHIP. Please look at the code in gpio/gpio-tegra186.c and see if you can use his approach. As you can see this chip is using gpiochip_irq_map/unmap in its domain, and manipulates struct gpio_irq_chip directly. I think the idea is to make it possible to use GPIOLIB_IRQCHIP for banked IRQs but the infrastructure is not yet inside the gpiolib so it is a bit taped on the side right now. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 14 June 2018 01:59 PM, Linus Walleij wrote: > On Wed, Jun 6, 2018 at 11:18 AM, Keerthy <j-keerthy@ti.com> wrote: > >> Currently the driver assumes that the interrupts are continuous >> and does platform_get_irq only once and assumes the rest are continuous, >> instead call platform_get_irq for all the interrupts and store them >> in an array for later use. >> >> Signed-off-by: Keerthy <j-keerthy@ti.com> > > Hm! Thierry has recently submitted patches to make it easier for chips with > banked IRQs to use the GPIOLIB_IRQCHIP. > > Please look at the code in gpio/gpio-tegra186.c and see if you can use > his approach. > > As you can see this chip is using gpiochip_irq_map/unmap in its > domain, and manipulates struct gpio_irq_chip directly. > > I think the idea is to make it possible to use GPIOLIB_IRQCHIP > for banked IRQs but the infrastructure is not yet inside the gpiolib > so it is a bit taped on the side right now. Okay. I will take a look at that. The key issue that this patch addresses is that currently driver only calls platform_get_irq once and assumes the rest are continuous which is wrong hence the key issue addressed with this patch is to call platform_get_irq for each interrupt. The version 4 is here: https://patchwork.kernel.org/patch/10461537/ I will try to follow up with generic gpiochip_irq_map/unmap once i get that working. Hope that is okay? > > Yours, > Linus Walleij > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 14, 2018 at 10:51 AM, Keerthy <j-keerthy@ti.com> wrote: >> I think the idea is to make it possible to use GPIOLIB_IRQCHIP >> for banked IRQs but the infrastructure is not yet inside the gpiolib >> so it is a bit taped on the side right now. > > Okay. I will take a look at that. The key issue that this patch > addresses is that currently driver only calls platform_get_irq once and > assumes the rest are continuous which is wrong hence the key issue > addressed with this patch is to call platform_get_irq for each interrupt. > > The version 4 is here: > > https://patchwork.kernel.org/patch/10461537/ > > I will try to follow up with generic gpiochip_irq_map/unmap once i get > that working. Hope that is okay? Oh that's fine, I'll merge it. Just wanted you to have a look at Thierry's stuff so we can share more code, you're a core developer so I wanted some smart people to look at this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/14/2018 4:48 PM, Linus Walleij wrote: > On Thu, Jun 14, 2018 at 10:51 AM, Keerthy <j-keerthy@ti.com> wrote: > >>> I think the idea is to make it possible to use GPIOLIB_IRQCHIP >>> for banked IRQs but the infrastructure is not yet inside the gpiolib >>> so it is a bit taped on the side right now. >> >> Okay. I will take a look at that. The key issue that this patch >> addresses is that currently driver only calls platform_get_irq once and >> assumes the rest are continuous which is wrong hence the key issue >> addressed with this patch is to call platform_get_irq for each interrupt. >> >> The version 4 is here: >> >> https://patchwork.kernel.org/patch/10461537/ >> >> I will try to follow up with generic gpiochip_irq_map/unmap once i get >> that working. Hope that is okay? > > Oh that's fine, I'll merge it. Thanks! > > Just wanted you to have a look at Thierry's stuff so we can share > more code, you're a core developer so I wanted some smart people > to look at this. Sure Linus! > > Yours, > Linus Walleij > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c index 861f35b..375578c 100644 --- a/drivers/gpio/gpio-davinci.c +++ b/drivers/gpio/gpio-davinci.c @@ -42,6 +42,7 @@ struct davinci_gpio_regs { #define BINTEN 0x8 /* GPIO Interrupt Per-Bank Enable Register */ #define MAX_LABEL_SIZE 20 +#define MAX_INT_PER_BANK 32 static void __iomem *gpio_base; static unsigned int offset_array[5] = {0x10, 0x38, 0x60, 0x88, 0xb0}; @@ -55,7 +56,7 @@ static inline struct davinci_gpio_regs __iomem *irq2regs(struct irq_data *d) return g; } -static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq); +static int davinci_gpio_irq_setup(struct platform_device *pdev, int *bank_irq); /*--------------------------------------------------------------------------*/ @@ -168,7 +169,8 @@ static int davinci_gpio_probe(struct platform_device *pdev) { static int ctrl_num, bank_base; int gpio, bank, ret = 0; - unsigned ngpio, nbank, bank_irq; + unsigned int ngpio, nbank, nirq; + int bank_irq[MAX_INT_PER_BANK], i; struct davinci_gpio_controller *chips; struct davinci_gpio_platform_data *pdata; struct device *dev = &pdev->dev; @@ -197,6 +199,16 @@ static int davinci_gpio_probe(struct platform_device *pdev) if (WARN_ON(ARCH_NR_GPIOS < ngpio)) ngpio = ARCH_NR_GPIOS; + /* + * If there are unbanked interrupts then the number of + * interrupts is equal to number of gpios else all are banked so + * number of interrupts is equal to number of banks(each with 16 gpios) + */ + if (pdata->gpio_unbanked) + nirq = pdata->gpio_unbanked; + else + nirq = DIV_ROUND_UP(ngpio, 16); + nbank = DIV_ROUND_UP(ngpio, 32); chips = devm_kzalloc(dev, nbank * sizeof(struct davinci_gpio_controller), @@ -209,10 +221,13 @@ static int davinci_gpio_probe(struct platform_device *pdev) if (IS_ERR(gpio_base)) return PTR_ERR(gpio_base); - bank_irq = platform_get_irq(pdev, 0); - if (bank_irq < 0) { - dev_dbg(dev, "IRQ not populated\n"); - return bank_irq; + for (i = 0; i < nirq; i++) { + bank_irq[i] = platform_get_irq(pdev, i); + if (bank_irq[i] < 0) { + dev_info(dev, "IRQ not populated, err = %d\n", + bank_irq[i]); + return bank_irq[i]; + } } snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++); @@ -458,7 +473,7 @@ static struct irq_chip *keystone_gpio_get_irq_chip(unsigned int irq) * (dm6446) can be set appropriately for GPIOV33 pins. */ -static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) +static int davinci_gpio_irq_setup(struct platform_device *pdev, int *bank_irq) { unsigned gpio, bank; int irq; @@ -492,6 +507,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) dev_err(dev, "Error %ld getting gpio clock\n", PTR_ERR(clk)); return PTR_ERR(clk); } + ret = clk_prepare_enable(clk); if (ret) return ret; @@ -531,12 +547,12 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) if (pdata->gpio_unbanked) { /* pass "bank 0" GPIO IRQs to AINTC */ chips->chip.to_irq = gpio_to_irq_unbanked; - chips->base_irq = bank_irq; + chips->base_irq = bank_irq[0]; chips->gpio_unbanked = pdata->gpio_unbanked; binten = GENMASK(pdata->gpio_unbanked / 16, 0); /* AINTC handles mask/unmask; GPIO handles triggering */ - irq = bank_irq; + irq = bank_irq[0]; irq_chip = gpio_get_irq_chip(irq); irq_chip->name = "GPIO-AINTC"; irq_chip->irq_set_type = gpio_irq_type_unbanked; @@ -547,10 +563,11 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) writel_relaxed(~0, &g->set_rising); /* set the direct IRQs up to use that irqchip */ - for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) { - irq_set_chip(irq, irq_chip); - irq_set_handler_data(irq, chips); - irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH); + for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++) { + irq_set_chip(bank_irq[gpio], irq_chip); + irq_set_handler_data(bank_irq[gpio], chips); + irq_set_status_flags(bank_irq[gpio], + IRQ_TYPE_EDGE_BOTH); } goto done; @@ -560,7 +577,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) * Or, AINTC can handle IRQs for banks of 16 GPIO IRQs, which we * then chain through our own handler. */ - for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) { + for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 16) { /* disabled by default, enabled only as needed * There are register sets for 32 GPIOs. 2 banks of 16 * GPIOs are covered by each set of registers hence divide by 2 @@ -587,8 +604,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev, int bank_irq) irqdata->bank_num = bank; irqdata->chip = chips; - irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler, - irqdata); + irq_set_chained_handler_and_data(bank_irq[bank], + gpio_irq_handler, irqdata); binten |= BIT(bank); }
Currently the driver assumes that the interrupts are continuous and does platform_get_irq only once and assumes the rest are continuous, instead call platform_get_irq for all the interrupts and store them in an array for later use. Signed-off-by: Keerthy <j-keerthy@ti.com> --- Tested for GPIO Interrupts on da850-lcdk and keystone-k2g-evm boards. drivers/gpio/gpio-davinci.c | 49 ++++++++++++++++++++++++++++++--------------- 1 file changed, 33 insertions(+), 16 deletions(-)