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

[Stefan Wahren]

Hi Marc,

Am 10.06.22 um 10:51 schrieb 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.
i will send a seperate fix for this.
>
>>
>>  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
>
> Thanks,
>
>         M.