linux-kernel - Re: [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b681ed9d-a31a-e5cc-04ba-6f38a5cc745b@gmail.com>
Date:   Fri, 1 Nov 2019 19:55:11 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Chris Packham <chris.packham@...iedtelesis.co.nz>,
        linus.walleij@...aro.org, rjui@...adcom.com, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com
Cc:     linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/2] pinctrl: bcm: nsp: use gpiolib infrastructure for
 interrupts



On 10/31/2019 6:56 PM, Chris Packham wrote:
> Use more of the gpiolib infrastructure for handling interrupts. The
> root interrupt still needs to be handled manually as it is shared with
> other peripherals on the SoC.
> 
> This will allow multiple instances of this driver to be supported and
> will clean up gracefully on failure thanks to the device managed APIs.
> 
> Signed-off-by: Chris Packham <chris.packham@...iedtelesis.co.nz>
> ---

Just a couple of comments below:

[snip]

> +		irqc->name = dev_name(dev);

The irq_chip used to be named "gpio-a" now it most likely will contain
the address.unit-name notation from Device Tree, since this is visible
in /proc/interrupts one might consider this to be an ABI breakage.

> +		irqc->irq_ack = nsp_gpio_irq_ack;
> +		irqc->irq_mask = nsp_gpio_irq_mask;
> +		irqc->irq_unmask = nsp_gpio_irq_unmask;
> +		irqc->irq_set_type = nsp_gpio_irq_set_type;
>  
> -			irq_set_chip_and_handler(irq, &nsp_gpio_irq_chip,
> -						 handle_simple_irq);
> -			irq_set_chip_data(irq, chip);
> -		}
> +		val = readl(chip->base + NSP_CHIP_A_INT_MASK);
> +		val = val | NSP_CHIP_A_GPIO_INT_BIT;
> +		writel(val, (chip->base + NSP_CHIP_A_INT_MASK));
>  
>  		/* Install ISR for this GPIO controller. */
> -		ret = devm_request_irq(&pdev->dev, irq, nsp_gpio_irq_handler,
> -				       IRQF_SHARED, "gpio-a", chip);
> +		ret = devm_request_irq(dev, irq, nsp_gpio_irq_handler,
> +				       IRQF_SHARED, "gpio-a", &chip->gc);
>  		if (ret) {
>  			dev_err(&pdev->dev, "Unable to request IRQ%d: %d\n",
>  				irq, ret);
> -			goto err_rm_gpiochip;
> +			return ret;
>  		}
>  
> -		val = readl(chip->base + NSP_CHIP_A_INT_MASK);
> -		val = val | NSP_CHIP_A_GPIO_INT_BIT;
> -		writel(val, (chip->base + NSP_CHIP_A_INT_MASK));
> +		girq = &chip->gc.irq;
> +		girq->chip = irqc;
> +		/* This will let us handle the parent IRQ in the driver */
> +		girq->parent_handler = NULL;
> +		girq->num_parents = 0;
> +		girq->parents = NULL;
> +		girq->default_type = IRQ_TYPE_NONE;
> +		girq->handler = handle_simple_irq;

It might be worth creating a helper that can be called to initialize all
relevant members to the values that indicate: let me manage the
interrupt. This would make us more future proof with respect to
assumptions being made in gpiolib as well as if new fields are added in
the future. This would be a separate patch obviously.

Other than that:

Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
-- 
Florian