Re: [PATCH 7/8] staging: mt7621-gpio: avoid custom irq_domain for gpio

[NeilBrown <neil@brown.name>]

[-- Attachment #1.1: Type: text/plain, Size: 8763 bytes --]

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@gmail.com>
> ---
>  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.

Once you've clean up the rest, I'll try again and see if I can work out
what is happening.

Thanks,
NeilBrown


>  
>  			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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 169 bytes --]

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel