On Sun, Jun 10, 2018 at 07:02:20PM +1000, NeilBrown wrote:
> On Sat, Jun 09 2018, Sergio Paracuellos wrote:
>
> > Instead of create a custom irq_domain for this chip, use
> > 'gpiochip_set_chained_irqchip' from GPIOLIB_IRQCHIP. It
> > is ok to call this function several times. You only have to
> > mark the line with 'IRQF_SHARED' and then loop over the
> > three banks until you find a hit. There were some problems
> > with removing an irqchip like that but this driver is a bool
> > so it might work just fine. After this changes the functions
> > 'mediatek_gpio_to_irq' and 'to_mediatek_gpio' are not needed
> > anymore and also the 'gpio_irq_domain' field from the state
> > container. Instead of use the custom irq domain in the irq
> > handler use the associated domain from the gpio_chip in
> > 'irq_find_mapping' function. Function 'mediatek_gpio_bank_probe'
> > has been moved a it to the botton to have all the irq related
> > functions together and avoid some forward declarations to resolve
> > some symbols along the code.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
> > ---
> > drivers/staging/mt7621-gpio/Kconfig | 2 +-
> > drivers/staging/mt7621-gpio/gpio-mt7621.c | 137 +++++++++++++-----------------
> > 2 files changed, 58 insertions(+), 81 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-gpio/Kconfig b/drivers/staging/mt7621-gpio/Kconfig
> > index 835998a..b6a921a 100644
> > --- a/drivers/staging/mt7621-gpio/Kconfig
> > +++ b/drivers/staging/mt7621-gpio/Kconfig
> > @@ -2,6 +2,6 @@ config GPIO_MT7621
> > bool "Mediatek GPIO Support"
> > depends on SOC_MT7620 || SOC_MT7621 || COMPILE_TEST
> > select GPIO_GENERIC
> > - select ARCH_REQUIRE_GPIOLIB
> > + select GPIOLIB_IRQCHIP
> > help
> > Say yes here to support the Mediatek SoC GPIO device
> > diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > index 80e618f..5d082c8 100644
> > --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> > @@ -8,7 +8,6 @@
> > #include <linux/gpio/driver.h>
> > #include <linux/interrupt.h>
> > #include <linux/io.h>
> > -#include <linux/irqdomain.h>
> > #include <linux/module.h>
> > #include <linux/of_irq.h>
> > #include <linux/platform_device.h>
> > @@ -54,23 +53,15 @@ struct mtk_gc {
> > * @dev: device instance
> > * @gpio_membase: memory base address
> > * @gpio_irq: irq number from the device tree
> > - * @gpio_irq_domain: irq domain for this chip
> > * @gc_map: array of the gpio chips
> > */
> > struct mtk_data {
> > struct device *dev;
> > void __iomem *gpio_membase;
> > int gpio_irq;
> > - struct irq_domain *gpio_irq_domain;
> > struct mtk_gc gc_map[MTK_BANK_CNT];
> > };
> >
> > -static inline struct mtk_gc *
> > -to_mediatek_gpio(struct gpio_chip *chip)
> > -{
> > - return container_of(chip, struct mtk_gc, chip);
> > -}
> > -
> > static inline void
> > mtk_gpio_w32(struct mtk_gc *rg, u32 offset, u32 val)
> > {
> > @@ -89,63 +80,6 @@ mtk_gpio_r32(struct mtk_gc *rg, u32 offset)
> > return gc->read_reg(gpio_data->gpio_membase + offset);
> > }
> >
> > -static int
> > -mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
> > -{
> > - struct mtk_data *gpio_data = gpiochip_get_data(chip);
> > - struct mtk_gc *rg = to_mediatek_gpio(chip);
> > -
> > - return irq_create_mapping(gpio_data->gpio_irq_domain,
> > - pin + (rg->bank * MTK_BANK_WIDTH));
> > -}
> > -
> > -static int
> > -mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> > -{
> > - struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> > - const __be32 *id = of_get_property(bank, "reg", NULL);
> > - struct mtk_gc *rg;
> > - int ret;
> > -
> > - if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> > - return -EINVAL;
> > -
> > - rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> > - memset(rg, 0, sizeof(*rg));
> > -
> > - spin_lock_init(&rg->lock);
> > - rg->bank = be32_to_cpu(*id);
> > -
> > - ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> > - gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> > - gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> > - gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> > - gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > - gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > - 0);
> > - if (ret) {
> > - dev_err(&pdev->dev, "bgpio_init() failed\n");
> > - return ret;
> > - }
> > -
> > - if (gpio_data->gpio_irq_domain)
> > - rg->chip.to_irq = mediatek_gpio_to_irq;
> > -
> > - ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> > - if (ret < 0) {
> > - dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> > - rg->chip.ngpio, ret);
> > - return ret;
> > - }
> > -
> > - /* set polarity to low for all gpios */
> > - mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> > -
> > - dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> > -
> > - return 0;
> > -}
> > -
> > static void
> > mediatek_gpio_irq_handler(struct irq_desc *desc)
> > {
> > @@ -163,7 +97,7 @@ mediatek_gpio_irq_handler(struct irq_desc *desc)
> > pending = mtk_gpio_r32(rg, GPIO_REG_STAT(rg->bank));
> >
> > for_each_set_bit(bit, &pending, MTK_BANK_WIDTH) {
> > - u32 map = irq_find_mapping(gpio_data->gpio_irq_domain,
> > + u32 map = irq_find_mapping(rg->chip.irq.domain,
> > (MTK_BANK_WIDTH * i) + bit);
>
> I think you need more changes to mediatek_gpio_irq_handler.
> irq_desk_get_handler_data() now returns a 'struct gpio_chip' I think.
> However this was the part that doesn't work for me yet.
> Does this get called once for each bank when an interrupt occurs?
> Maybe.
> Anyway, something else is wrong here.
Interrupt line must be marked as IRQF_SHARED to make this working
properly.
>
> Once you've clean up the rest, I'll try again and see if I can work out
> what is happening.
>
> Thanks,
> NeilBrown
Best regards,
Sergio Paracuellos
>
>
> >
> > generic_handle_irq(map);
> > @@ -308,6 +242,62 @@ static const struct irq_domain_ops irq_domain_ops = {
> > };
> >
> > static int
> > +mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
> > +{
> > + struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
> > + const __be32 *id = of_get_property(bank, "reg", NULL);
> > + struct mtk_gc *rg;
> > + int ret;
> > +
> > + if (!id || be32_to_cpu(*id) >= MTK_BANK_CNT)
> > + return -EINVAL;
> > +
> > + rg = &gpio_data->gc_map[be32_to_cpu(*id)];
> > + memset(rg, 0, sizeof(*rg));
> > +
> > + spin_lock_init(&rg->lock);
> > + rg->bank = be32_to_cpu(*id);
> > +
> > + ret = bgpio_init(&rg->chip, &pdev->dev, 4,
> > + gpio_data->gpio_membase + GPIO_REG_DATA(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_DSET(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_DCLR(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > + gpio_data->gpio_membase + GPIO_REG_CTRL(rg->bank),
> > + 0);
> > + if (ret) {
> > + dev_err(&pdev->dev, "bgpio_init() failed\n");
> > + return ret;
> > + }
> > +
> > + ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> > + rg->chip.ngpio, ret);
> > + return ret;
> > + }
> > +
> > + ret = gpiochip_irqchip_add(&rg->chip, &mediatek_gpio_irq_chip,
> > + 0, handle_simple_irq, IRQ_TYPE_NONE);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to add gpiochip_irqchip\n");
> > + return ret;
> > + }
> > +
> > + gpiochip_set_chained_irqchip(&rg->chip, &mediatek_gpio_irq_chip,
> > + gpio_data->gpio_irq,
> > + mediatek_gpio_irq_handler);
> > +
> > + /* set polarity to low for all gpios */
> > + mtk_gpio_w32(rg, GPIO_REG_POL(rg->bank), 0);
> > +
> > + dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio);
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static int
> > mediatek_gpio_probe(struct platform_device *pdev)
> > {
> > struct device_node *bank, *np = pdev->dev.of_node;
> > @@ -323,14 +313,6 @@ mediatek_gpio_probe(struct platform_device *pdev)
> > return PTR_ERR(gpio_data->gpio_membase);
> >
> > gpio_data->gpio_irq = irq_of_parse_and_map(np, 0);
> > - if (gpio_data->gpio_irq) {
> > - gpio_data->gpio_irq_domain = irq_domain_add_linear(np,
> > - MTK_BANK_CNT * MTK_BANK_WIDTH,
> > - &irq_domain_ops, gpio_data);
>
> This is the only place that irq_domain_ops was used - currently it is
> unused.
> If we really don't needed it, it should be removed.
>
> > - if (!gpio_data->gpio_irq_domain)
> > - dev_err(&pdev->dev, "irq_domain_add_linear failed\n");
> > - }
> > -
> > gpio_data->dev = &pdev->dev;
> > platform_set_drvdata(pdev, gpio_data);
> >
> > @@ -338,11 +320,6 @@ mediatek_gpio_probe(struct platform_device *pdev)
> > if (of_device_is_compatible(bank, "mediatek,mt7621-gpio-bank"))
> > mediatek_gpio_bank_probe(pdev, bank);
> >
> > - if (gpio_data->gpio_irq_domain)
> > - irq_set_chained_handler_and_data(gpio_data->gpio_irq,
> > - mediatek_gpio_irq_handler,
> > - gpio_data);
> > -
> > return 0;
> > }
> >
> > --
> > 2.7.4
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
