On Sat, 17 Sep 2022 14:19:07 +0100, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote: > > This makes the Nomadik GPIO irqchip immutable. > > Tested on the Samsung Galaxy SIII mini GT-I8190. > > Cc: Marc Zyngier <maz@xxxxxxxxxx> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx> > --- > drivers/pinctrl/nomadik/pinctrl-nomadik.c | 51 +++++++++++------------ > 1 file changed, 24 insertions(+), 27 deletions(-) > > diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c > index f5014d09d81a..1ee3b45dd6bc 100644 > --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c > +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c > @@ -244,7 +244,6 @@ enum nmk_gpio_slpm { > > struct nmk_gpio_chip { > struct gpio_chip chip; > - struct irq_chip irqchip; > void __iomem *addr; > struct clk *clk; > unsigned int bank; > @@ -675,15 +674,11 @@ static void __nmk_gpio_set_wake(struct nmk_gpio_chip *nmk_chip, > __nmk_gpio_irq_modify(nmk_chip, offset, WAKE, on); > } > > -static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable) > +static void nmk_gpio_irq_maskunmask(struct nmk_gpio_chip *nmk_chip, > + struct irq_data *d, bool enable) > { > - struct nmk_gpio_chip *nmk_chip; > unsigned long flags; > > - nmk_chip = irq_data_get_irq_chip_data(d); > - if (!nmk_chip) > - return -EINVAL; > - > clk_enable(nmk_chip->clk); > spin_lock_irqsave(&nmk_gpio_slpm_lock, flags); > spin_lock(&nmk_chip->lock); > @@ -696,18 +691,22 @@ static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable) > spin_unlock(&nmk_chip->lock); > spin_unlock_irqrestore(&nmk_gpio_slpm_lock, flags); > clk_disable(nmk_chip->clk); > - > - return 0; > } > > static void nmk_gpio_irq_mask(struct irq_data *d) > { > - nmk_gpio_irq_maskunmask(d, false); > + struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d); As far as I can tell, the per-chip data for a GPIO chip points to the gpio_chip, and not the driver-specific view that contains the generic structure. Here you're lucky that they are the same address as the gpio_chip is the first one. This is the same bug as the one fixed by d1e972ace423 ("gpio: tegra186: Fix chip_data type confusion"). > + > + nmk_gpio_irq_maskunmask(nmk_chip, d, false); > + gpiochip_disable_irq(&nmk_chip->chip, irqd_to_hwirq(d)); > } > > static void nmk_gpio_irq_unmask(struct irq_data *d) > { > - nmk_gpio_irq_maskunmask(d, true); > + struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d); > + > + gpiochip_enable_irq(&nmk_chip->chip, irqd_to_hwirq(d)); > + nmk_gpio_irq_maskunmask(nmk_chip, d, true); > } > > static int nmk_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > @@ -1078,13 +1077,25 @@ static struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np, > return nmk_chip; > } > > +static const struct irq_chip nmk_irq_chip = { > + .name = "nomadik-gpio", Maybe not, see below. > + .irq_ack = nmk_gpio_irq_ack, > + .irq_mask = nmk_gpio_irq_mask, > + .irq_unmask = nmk_gpio_irq_unmask, > + .irq_set_type = nmk_gpio_irq_set_type, > + .irq_set_wake = nmk_gpio_irq_set_wake, > + .irq_startup = nmk_gpio_irq_startup, > + .irq_shutdown = nmk_gpio_irq_shutdown, > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > +}; > + > static int nmk_gpio_probe(struct platform_device *dev) > { > struct device_node *np = dev->dev.of_node; > struct nmk_gpio_chip *nmk_chip; > struct gpio_chip *chip; > struct gpio_irq_chip *girq; > - struct irq_chip *irqchip; > bool supports_sleepmode; > int irq; > int ret; > @@ -1125,22 +1136,8 @@ static int nmk_gpio_probe(struct platform_device *dev) > chip->can_sleep = false; > chip->owner = THIS_MODULE; > > - irqchip = &nmk_chip->irqchip; > - irqchip->irq_ack = nmk_gpio_irq_ack; > - irqchip->irq_mask = nmk_gpio_irq_mask; > - irqchip->irq_unmask = nmk_gpio_irq_unmask; > - irqchip->irq_set_type = nmk_gpio_irq_set_type; > - irqchip->irq_set_wake = nmk_gpio_irq_set_wake; > - irqchip->irq_startup = nmk_gpio_irq_startup; > - irqchip->irq_shutdown = nmk_gpio_irq_shutdown; > - irqchip->flags = IRQCHIP_MASK_ON_SUSPEND; > - irqchip->name = kasprintf(GFP_KERNEL, "nmk%u-%u-%u", > - dev->id, > - chip->base, > - chip->base + chip->ngpio - 1); > - Dropping this is unfortunately a userspace visible change. I'm 99% sure nobody will ever care, but we've been burned by the remaining 1%, and I'd rather you keep it using the irq_print_chip() callback with something like: static void nmk_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p) { struct gpio_chip *chip = irq_data_get_irq_chip_data(d); struct nmk_gpio_chip *nmk_chip = container_of(chip, struct nmk_gpio_chip, chip); seq_printf(p, "nmk%u-%u-%u", nmk_chip->bank, chip->base, chip->base + chip->ngpio - 1); } Thanks, M. -- Without deviation from the norm, progress is not possible.