| Message ID | 1439504505-20314-1-git-send-email-hauke@hauke-m.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kalle Valo |
| Headers | show |
> From: Linus Walleij <linus.walleij@linaro.org> > > This switches the BCMA GPIO driver to use GPIOLIB_IRQCHIP to > handle its interrupts instead of rolling its own copy of the > irqdomain handling etc. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> Thanks, applied to wireless-drivers-next.git. Kalle Valo -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
I'm afraid it wasn't tested on BCM47XX (MIPS) :( On 14 August 2015 at 00:21, Hauke Mehrtens <hauke@hauke-m.de> wrote: > @@ -218,9 +187,8 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) > chip->set = bcma_gpio_set_value; > chip->direction_input = bcma_gpio_direction_input; > chip->direction_output = bcma_gpio_direction_output; > -#if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) > - chip->to_irq = bcma_gpio_to_irq; > -#endif > + chip->owner = THIS_MODULE; > + chip->dev = bcma_bus_get_host_dev(bus); This assigns &bus->host_pdev->dev which is NULL. > @@ -248,13 +216,13 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) > else > chip->base = -1; > > - err = bcma_gpio_irq_domain_init(cc); > + err = gpiochip_add(chip); > if (err) > return err; > > - err = gpiochip_add(chip); > + err = bcma_gpio_irq_init(cc); This results in: [ 0.157054] missing gpiochip .dev parent pointer (coming from gpiochip_irqchip_add) and [ 0.157287] bcma: bus0: Error registering GPIO driver: -22 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/18/2015 07:27 AM, Rafa? Mi?ecki wrote: > I'm afraid it wasn't tested on BCM47XX (MIPS) :( Yes, you are probably right. > On 14 August 2015 at 00:21, Hauke Mehrtens <hauke@hauke-m.de> wrote: >> @@ -218,9 +187,8 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) >> chip->set = bcma_gpio_set_value; >> chip->direction_input = bcma_gpio_direction_input; >> chip->direction_output = bcma_gpio_direction_output; >> -#if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) >> - chip->to_irq = bcma_gpio_to_irq; >> -#endif >> + chip->owner = THIS_MODULE; >> + chip->dev = bcma_bus_get_host_dev(bus); > > This assigns &bus->host_pdev->dev which is NULL. hmm, how do we fix this, as long as bcma does not have a device on mips this is a problem. Should we create a new device for mips? >> @@ -248,13 +216,13 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) >> else >> chip->base = -1; >> >> - err = bcma_gpio_irq_domain_init(cc); >> + err = gpiochip_add(chip); >> if (err) >> return err; >> >> - err = gpiochip_add(chip); >> + err = bcma_gpio_irq_init(cc); > > This results in: > [ 0.157054] missing gpiochip .dev parent pointer > (coming from gpiochip_irqchip_add) and > [ 0.157287] bcma: bus0: Error registering GPIO driver: -22 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 18, 2015 at 7:01 PM, Hauke Mehrtens <hauke@hauke-m.de> wrote: > On 12/18/2015 07:27 AM, Rafa? Mi?ecki wrote: >> I'm afraid it wasn't tested on BCM47XX (MIPS) :( > > Yes, you are probably right. > >> On 14 August 2015 at 00:21, Hauke Mehrtens <hauke@hauke-m.de> wrote: >>> @@ -218,9 +187,8 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) >>> chip->set = bcma_gpio_set_value; >>> chip->direction_input = bcma_gpio_direction_input; >>> chip->direction_output = bcma_gpio_direction_output; >>> -#if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) >>> - chip->to_irq = bcma_gpio_to_irq; >>> -#endif >>> + chip->owner = THIS_MODULE; >>> + chip->dev = bcma_bus_get_host_dev(bus); >> >> This assigns &bus->host_pdev->dev which is NULL. > > hmm, how do we fix this, as long as bcma does not have a device on mips > this is a problem. Should we create a new device for mips? Yes I think MIPS should just create a host device. That makes things simple and nice. Is it complex to do so? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/22/2015 10:01 AM, Linus Walleij wrote: > On Fri, Dec 18, 2015 at 7:01 PM, Hauke Mehrtens <hauke@hauke-m.de> wrote: >> On 12/18/2015 07:27 AM, Rafa? Mi?ecki wrote: >>> I'm afraid it wasn't tested on BCM47XX (MIPS) :( >> >> Yes, you are probably right. >> >>> On 14 August 2015 at 00:21, Hauke Mehrtens <hauke@hauke-m.de> wrote: >>>> @@ -218,9 +187,8 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) >>>> chip->set = bcma_gpio_set_value; >>>> chip->direction_input = bcma_gpio_direction_input; >>>> chip->direction_output = bcma_gpio_direction_output; >>>> -#if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) >>>> - chip->to_irq = bcma_gpio_to_irq; >>>> -#endif >>>> + chip->owner = THIS_MODULE; >>>> + chip->dev = bcma_bus_get_host_dev(bus); >>> >>> This assigns &bus->host_pdev->dev which is NULL. >> >> hmm, how do we fix this, as long as bcma does not have a device on mips >> this is a problem. Should we create a new device for mips? > > Yes I think MIPS should just create a host device. That > makes things simple and nice. Is it complex to do so? > > Yours, > Linus Walleij > Currently the documentation says this for the parent member on struct gpio_chip: * @parent: optional parent device providing the GPIOs I assume the optional is wrong here. Hauke -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 22, 2015 at 1:09 PM, Hauke Mehrtens <hauke@hauke-m.de> wrote: > On 12/22/2015 10:01 AM, Linus Walleij wrote: >> On Fri, Dec 18, 2015 at 7:01 PM, Hauke Mehrtens <hauke@hauke-m.de> wrote: >>> On 12/18/2015 07:27 AM, Rafa? Mi?ecki wrote: >>>> I'm afraid it wasn't tested on BCM47XX (MIPS) :( >>> >>> Yes, you are probably right. >>> >>>> On 14 August 2015 at 00:21, Hauke Mehrtens <hauke@hauke-m.de> wrote: >>>>> @@ -218,9 +187,8 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) >>>>> chip->set = bcma_gpio_set_value; >>>>> chip->direction_input = bcma_gpio_direction_input; >>>>> chip->direction_output = bcma_gpio_direction_output; >>>>> -#if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) >>>>> - chip->to_irq = bcma_gpio_to_irq; >>>>> -#endif >>>>> + chip->owner = THIS_MODULE; >>>>> + chip->dev = bcma_bus_get_host_dev(bus); >>>> >>>> This assigns &bus->host_pdev->dev which is NULL. >>> >>> hmm, how do we fix this, as long as bcma does not have a device on mips >>> this is a problem. Should we create a new device for mips? >> >> Yes I think MIPS should just create a host device. That >> makes things simple and nice. Is it complex to do so? >> >> Yours, >> Linus Walleij >> > Currently the documentation says this for the parent member on struct > gpio_chip: > > * @parent: optional parent device providing the GPIOs > > I assume the optional is wrong here. For just gpiochip it is optional. But if you read on: GPIO drivers providing IRQs --------------------------- (...) To use the helpers please keep the following in mind: - Make sure to assign all relevant members of the struct gpio_chip so that the irqchip can initialize. E.g. .dev and .can_sleep shall be set up properly. (...) Well it's complex. But when adding the gpio character device I am working with, I will introduce a struct device to the gpio_chip (or similar sibling gpio_device) which we can then refer, so that this actually will no longer be a requirement. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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/bcma/Kconfig b/drivers/bcma/Kconfig index be5fffb..023d448 100644 --- a/drivers/bcma/Kconfig +++ b/drivers/bcma/Kconfig @@ -92,7 +92,7 @@ config BCMA_DRIVER_GMAC_CMN config BCMA_DRIVER_GPIO bool "BCMA GPIO driver" depends on BCMA && GPIOLIB - select IRQ_DOMAIN if BCMA_HOST_SOC + select GPIOLIB_IRQCHIP if BCMA_HOST_SOC help Driver to provide access to the GPIO pins of the bcma bus. diff --git a/drivers/bcma/driver_gpio.c b/drivers/bcma/driver_gpio.c index 5f6018e..504899a7 100644 --- a/drivers/bcma/driver_gpio.c +++ b/drivers/bcma/driver_gpio.c @@ -8,10 +8,8 @@ * Licensed under the GNU/GPL. See COPYING for details. */ -#include <linux/gpio.h> -#include <linux/irq.h> +#include <linux/gpio/driver.h> #include <linux/interrupt.h> -#include <linux/irqdomain.h> #include <linux/export.h> #include <linux/bcma/bcma.h> @@ -79,19 +77,11 @@ static void bcma_gpio_free(struct gpio_chip *chip, unsigned gpio) } #if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) -static int bcma_gpio_to_irq(struct gpio_chip *chip, unsigned gpio) -{ - struct bcma_drv_cc *cc = bcma_gpio_get_cc(chip); - - if (cc->core->bus->hosttype == BCMA_HOSTTYPE_SOC) - return irq_find_mapping(cc->irq_domain, gpio); - else - return -EINVAL; -} static void bcma_gpio_irq_unmask(struct irq_data *d) { - struct bcma_drv_cc *cc = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct bcma_drv_cc *cc = bcma_gpio_get_cc(gc); int gpio = irqd_to_hwirq(d); u32 val = bcma_chipco_gpio_in(cc, BIT(gpio)); @@ -101,7 +91,8 @@ static void bcma_gpio_irq_unmask(struct irq_data *d) static void bcma_gpio_irq_mask(struct irq_data *d) { - struct bcma_drv_cc *cc = irq_data_get_irq_chip_data(d); + struct gpio_chip *gc = irq_data_get_irq_chip_data(d); + struct bcma_drv_cc *cc = bcma_gpio_get_cc(gc); int gpio = irqd_to_hwirq(d); bcma_chipco_gpio_intmask(cc, BIT(gpio), 0); @@ -116,6 +107,7 @@ static struct irq_chip bcma_gpio_irq_chip = { static irqreturn_t bcma_gpio_irq_handler(int irq, void *dev_id) { struct bcma_drv_cc *cc = dev_id; + struct gpio_chip *gc = &cc->gpio; u32 val = bcma_cc_read32(cc, BCMA_CC_GPIOIN); u32 mask = bcma_cc_read32(cc, BCMA_CC_GPIOIRQ); u32 pol = bcma_cc_read32(cc, BCMA_CC_GPIOPOL); @@ -125,81 +117,58 @@ static irqreturn_t bcma_gpio_irq_handler(int irq, void *dev_id) if (!irqs) return IRQ_NONE; - for_each_set_bit(gpio, &irqs, cc->gpio.ngpio) - generic_handle_irq(bcma_gpio_to_irq(&cc->gpio, gpio)); + for_each_set_bit(gpio, &irqs, gc->ngpio) + generic_handle_irq(irq_find_mapping(gc->irqdomain, gpio)); bcma_chipco_gpio_polarity(cc, irqs, val & irqs); return IRQ_HANDLED; } -static int bcma_gpio_irq_domain_init(struct bcma_drv_cc *cc) +static int bcma_gpio_irq_init(struct bcma_drv_cc *cc) { struct gpio_chip *chip = &cc->gpio; - int gpio, hwirq, err; + int hwirq, err; if (cc->core->bus->hosttype != BCMA_HOSTTYPE_SOC) return 0; - cc->irq_domain = irq_domain_add_linear(NULL, chip->ngpio, - &irq_domain_simple_ops, cc); - if (!cc->irq_domain) { - err = -ENODEV; - goto err_irq_domain; - } - for (gpio = 0; gpio < chip->ngpio; gpio++) { - int irq = irq_create_mapping(cc->irq_domain, gpio); - - irq_set_chip_data(irq, cc); - irq_set_chip_and_handler(irq, &bcma_gpio_irq_chip, - handle_simple_irq); - } - hwirq = bcma_core_irq(cc->core, 0); err = request_irq(hwirq, bcma_gpio_irq_handler, IRQF_SHARED, "gpio", cc); if (err) - goto err_req_irq; + return err; bcma_chipco_gpio_intmask(cc, ~0, 0); bcma_cc_set32(cc, BCMA_CC_IRQMASK, BCMA_CC_IRQ_GPIO); - return 0; - -err_req_irq: - for (gpio = 0; gpio < chip->ngpio; gpio++) { - int irq = irq_find_mapping(cc->irq_domain, gpio); - - irq_dispose_mapping(irq); + err = gpiochip_irqchip_add(chip, + &bcma_gpio_irq_chip, + 0, + handle_simple_irq, + IRQ_TYPE_NONE); + if (err) { + free_irq(hwirq, cc); + return err; } - irq_domain_remove(cc->irq_domain); -err_irq_domain: - return err; + + return 0; } -static void bcma_gpio_irq_domain_exit(struct bcma_drv_cc *cc) +static void bcma_gpio_irq_exit(struct bcma_drv_cc *cc) { - struct gpio_chip *chip = &cc->gpio; - int gpio; - if (cc->core->bus->hosttype != BCMA_HOSTTYPE_SOC) return; bcma_cc_mask32(cc, BCMA_CC_IRQMASK, ~BCMA_CC_IRQ_GPIO); free_irq(bcma_core_irq(cc->core, 0), cc); - for (gpio = 0; gpio < chip->ngpio; gpio++) { - int irq = irq_find_mapping(cc->irq_domain, gpio); - - irq_dispose_mapping(irq); - } - irq_domain_remove(cc->irq_domain); } #else -static int bcma_gpio_irq_domain_init(struct bcma_drv_cc *cc) +static int bcma_gpio_irq_init(struct bcma_drv_cc *cc) { return 0; } -static void bcma_gpio_irq_domain_exit(struct bcma_drv_cc *cc) +static void bcma_gpio_irq_exit(struct bcma_drv_cc *cc) { } #endif @@ -218,9 +187,8 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) chip->set = bcma_gpio_set_value; chip->direction_input = bcma_gpio_direction_input; chip->direction_output = bcma_gpio_direction_output; -#if IS_BUILTIN(CONFIG_BCM47XX) || IS_BUILTIN(CONFIG_ARCH_BCM_5301X) - chip->to_irq = bcma_gpio_to_irq; -#endif + chip->owner = THIS_MODULE; + chip->dev = bcma_bus_get_host_dev(bus); #if IS_BUILTIN(CONFIG_OF) if (cc->core->bus->hosttype == BCMA_HOSTTYPE_SOC) chip->of_node = cc->core->dev.of_node; @@ -248,13 +216,13 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) else chip->base = -1; - err = bcma_gpio_irq_domain_init(cc); + err = gpiochip_add(chip); if (err) return err; - err = gpiochip_add(chip); + err = bcma_gpio_irq_init(cc); if (err) { - bcma_gpio_irq_domain_exit(cc); + gpiochip_remove(chip); return err; } @@ -263,7 +231,7 @@ int bcma_gpio_init(struct bcma_drv_cc *cc) int bcma_gpio_unregister(struct bcma_drv_cc *cc) { - bcma_gpio_irq_domain_exit(cc); + bcma_gpio_irq_exit(cc); gpiochip_remove(&cc->gpio); return 0; } diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h index 6cceedf..cf03843 100644 --- a/include/linux/bcma/bcma_driver_chipcommon.h +++ b/include/linux/bcma/bcma_driver_chipcommon.h @@ -640,7 +640,6 @@ struct bcma_drv_cc { spinlock_t gpio_lock; #ifdef CONFIG_BCMA_DRIVER_GPIO struct gpio_chip gpio; - struct irq_domain *irq_domain; #endif };