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
| ||
|
Date: Fri, 29 May 2015 17:10:33 -0700 From: Brian Norris <computersforpeace@...il.com> To: Gregory Fong <gregory.0xf0@...il.com> Cc: linux-gpio@...r.kernel.org, Alexandre Courbot <gnurou@...il.com>, bcm-kernel-feedback-list@...adcom.com, devicetree@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com>, Ian Campbell <ijc+devicetree@...lion.org.uk>, Kumar Gala <galak@...eaurora.org>, Linus Walleij <linus.walleij@...aro.org>, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>, Pawel Moll <pawel.moll@....com>, Rob Herring <robh+dt@...nel.org>, Russell King <linux@....linux.org.uk> Subject: Re: [PATCH v2 2/6] gpio: brcmstb: Add interrupt support A few small comments: On Thu, May 28, 2015 at 07:14:06PM -0700, Gregory Fong wrote: > v2: > - since imask member of bank struct was removed, just read and write from mask > reg and don't maintain a shadow ^^ this comment may be addressing what I'm going to ask about below? Not sure why this was changed, actually. > - warn on invalid IRQs > - move some irq setup to a separate function since probe is getting unwieldy > > drivers/gpio/Kconfig | 1 + > drivers/gpio/gpio-brcmstb.c | 276 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 277 insertions(+) > ... > diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c > index 7a3cb1f..b9962ff 100644 > --- a/drivers/gpio/gpio-brcmstb.c > +++ b/drivers/gpio/gpio-brcmstb.c ... > @@ -63,6 +69,231 @@ brcmstb_gpio_gc_to_priv(struct gpio_chip *gc) ... > +static void brcmstb_gpio_irq_bank_handler(int irq, > + struct brcmstb_gpio_bank *bank) > +{ > + struct brcmstb_gpio_priv *priv = bank->parent_priv; > + void __iomem *reg_base = priv->reg_base; > + unsigned long status; > + unsigned long flags; > + > + spin_lock_irqsave(&bank->bgc.lock, flags); > + while ((status = bank->bgc.read_reg(reg_base + GIO_STAT(bank->id)) & > + bank->bgc.read_reg(reg_base + GIO_MASK(bank->id)))) { In case you do run this loop multiple times (multiple interrupts in progress?), wouldn't it make sense to stash the mask exactly once, outside the loop? It's probably not a real big deal in practice, I guess. > + int bit; > + for_each_set_bit(bit, &status, 32) { > + int hwirq = bank->bgc.gc.base - > + priv->gpio_base + bit; > + int child_irq = > + irq_find_mapping(priv->irq_domain, > + hwirq); > + u32 stat = bank->bgc.read_reg(reg_base + > + GIO_STAT(bank->id)); > + if (bit >= bank->width) > + dev_warn(&priv->pdev->dev, > + "IRQ for invalid GPIO (bank=%d, offset=%d)\n", > + bank->id, bit); > + bank->bgc.write_reg(reg_base + GIO_STAT(bank->id), > + stat | BIT(bit)); > + generic_handle_irq(child_irq); > + } > + } > + spin_unlock_irqrestore(&bank->bgc.lock, flags); > +} ... > @@ -153,6 +410,16 @@ static int brcmstb_gpio_probe(struct platform_device *pdev) > priv->reg_base = reg_base; > priv->pdev = pdev; > > + if (of_find_property(np, "interrupt-controller", NULL)) { of_property_read_bool()? > + priv->parent_irq = platform_get_irq(pdev, 0); > + if (priv->parent_irq < 0) { > + dev_err(dev, "Couldn't get IRQ"); > + return -ENOENT; > + } > + } else { > + priv->parent_irq = -ENOENT; > + } > + > INIT_LIST_HEAD(&priv->bank_list); > if (brcmstb_gpio_sanity_check_banks(dev, np, res)) > return -EINVAL; Otherwise, looks OK to my inexpert eyes. Reviewed-by: Brian Norris <computersforpeace@...il.com> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists