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

[Stefan Wahren <stefan.wahren@xxxxxxxx>]

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@xxxxxxxx>
> > ---
> >
> > 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.