[PATCH RFC] pinctrl: bcm2835: Make the irqchip immutable

[Marc Zyngier]

On 2022-06-07 11:33, Stefan Wahren wrote:
> Commit 6c846d026d49 ("gpio: Don't fiddle with irqchips marked as
> immutable") added a warning to indicate if the gpiolib is altering the
> internals of irqchips. The bcm2835 pinctrl is also affected by this
> warning.
> 
> Fix this by making the irqchip in the bcm2835 pinctrl driver immutable.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren at i2se.com>
> ---
> 
> Hi, not sure about this change because irq_mask/unmask also uses the
> enable/disable callbacks.

Yeah, that's totally broken.

If you have both enable and mask, they *must* be doing something
different (the names are pretty unambiguous...). If they do the
same thing, then enable/disable has to go.

> 
>  drivers/pinctrl/bcm/pinctrl-bcm2835.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> index dad453054776..f754f7ed9eb9 100644
> --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c
> @@ -516,6 +516,8 @@ static void bcm2835_gpio_irq_enable(struct irq_data 
> *data)
>  	unsigned bank = GPIO_REG_OFFSET(gpio);
>  	unsigned long flags;
> 
> +	gpiochip_enable_irq(chip, gpio);
> +
>  	raw_spin_lock_irqsave(&pc->irq_lock[bank], flags);
>  	set_bit(offset, &pc->enabled_irq_map[bank]);
>  	bcm2835_gpio_irq_config(pc, gpio, true);
> @@ -537,6 +539,8 @@ static void bcm2835_gpio_irq_disable(struct 
> irq_data *data)
>  	bcm2835_gpio_set_bit(pc, GPEDS0, gpio);
>  	clear_bit(offset, &pc->enabled_irq_map[bank]);
>  	raw_spin_unlock_irqrestore(&pc->irq_lock[bank], flags);
> +
> +	gpiochip_disable_irq(chip, gpio);
>  }
> 
>  static int __bcm2835_gpio_irq_set_type_disabled(struct bcm2835_pinctrl 
> *pc,
> @@ -693,7 +697,7 @@ static int bcm2835_gpio_irq_set_wake(struct
> irq_data *data, unsigned int on)
>  	return ret;
>  }
> 
> -static struct irq_chip bcm2835_gpio_irq_chip = {
> +static const struct irq_chip bcm2835_gpio_irq_chip = {
>  	.name = MODULE_NAME,
>  	.irq_enable = bcm2835_gpio_irq_enable,
>  	.irq_disable = bcm2835_gpio_irq_disable,
> @@ -702,7 +706,7 @@ static struct irq_chip bcm2835_gpio_irq_chip = {
>  	.irq_mask = bcm2835_gpio_irq_disable,
>  	.irq_unmask = bcm2835_gpio_irq_enable,
>  	.irq_set_wake = bcm2835_gpio_irq_set_wake,
> -	.flags = IRQCHIP_MASK_ON_SUSPEND,
> +	.flags = (IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE),

You are missing the callbacks into the resource management
code (GPIOCHIP_IRQ_RESOURCE_HELPERS).

Thanks,

         M.
-- 
Who you jivin' with that Cosmik Debris?