Thanks for the review Thomas! On 23/04/13 16:09, Thomas Gleixner wrote: > On Tue, 23 Apr 2013, James Hogan wrote: >> +/** >> + * struct pdc_intc_priv - private pdc interrupt data. >> + * @nr_perips: Number of peripheral interrupt signals. >> + * @nr_syswakes: Number of syswake signals. >> + * @perip_irqs: List of peripheral IRQ numbers handled. >> + * @syswake_irq: Shared PDC syswake IRQ number. >> + * @domain: IRQ domain for PDC peripheral and syswake IRQs. >> + * @pdc_base: Base of PDC registers. >> + * @lock: Lock to protect the PDC syswake registers. >> + */ >> +struct pdc_intc_priv { >> + unsigned int nr_perips; >> + unsigned int nr_syswakes; >> + unsigned int *perip_irqs; >> + unsigned int syswake_irq; >> + struct irq_domain *domain; >> + void __iomem *pdc_base; >> + >> + spinlock_t lock; > > raw_spinlock_t please Okay. If I understand right, this would be because on RT, spinlock_t becomes a mutex and won't work correctly with irqs actually disabled for the irq callbacks below, is that right? >> +static void perip_irq_mask(struct irq_data *data) >> +{ >> + struct pdc_intc_priv *priv = irqd_to_priv(data); >> + unsigned int irq_route; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&priv->lock, flags); > > This is always called with interrupts disabled. Okay, I'll switch to raw_spin_lock >> + irq_route = pdc_read(priv, PDC_IRQ_ROUTE); >> + irq_route &= ~(1 << data->hwirq); > > Why not cache the mask value ? Yes, caching PDC_IRQ_ROUTE should be possible since it should only be used by this driver (hence the driver local spinlock). > >> + pdc_write(priv, PDC_IRQ_ROUTE, irq_route); > >> + spin_unlock_irqrestore(&priv->lock, flags); >> +} >> + >> +static void perip_irq_unmask(struct irq_data *data) >> +{ >> + struct pdc_intc_priv *priv = irqd_to_priv(data); >> + unsigned int irq_route; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&priv->lock, flags); >> + irq_route = pdc_read(priv, PDC_IRQ_ROUTE); >> + irq_route |= 1 << data->hwirq; >> + pdc_write(priv, PDC_IRQ_ROUTE, irq_route); > > This code is another slightly different copy of stuff which is > available in kernel/irq/generic-chip.c > > Can we please stop the code duplication and reuse existing > infrastructure? Don't tell me it does not work for you. I sent out a > patch yesterday which makes the code suitable for irq domains. I'll look into this. kernel/irq/generic-chip.c was added after this driver was written. >> +static void pdc_intc_perip_isr(unsigned int irq, struct irq_desc *desc) >> +{ >> + struct pdc_intc_priv *priv; >> + unsigned int i, irq_no; >> + >> + priv = (struct pdc_intc_priv *)irq_desc_get_handler_data(desc); >> + >> + /* find the peripheral number */ >> + for (i = 0; i < priv->nr_perips; ++i) >> + if (irq == priv->perip_irqs[i]) >> + goto found; > > Whee. Aren't these numbers linear ? Not necessarily as they're virtual irq numbers obtained via platform_get_irq(), which come individually from device tree. Even their hardware irq numbers aren't linear as they're not wired to their irqchip in the same order: > pdc: pdc@0x02006000 { > interrupt-controller; > #interrupt-cells = <3>; > > reg = <0x02006000 0x1000>; > compatible = "img,pdc-intc"; > > num-perips = <3>; > num-syswakes = <3>; > > interrupts = <18 4 /* level */>, /* Syswakes */ > <30 4 /* level */>, /* Perip 0 (RTC) */ > <29 4 /* level */>, /* Perip 1 (IR) */ > <31 4 /* level */>; /* Perip 2 (WDT) */ > }; >> +static int pdc_intc_probe(struct platform_device *pdev) >> +{ >> + struct pdc_intc_priv *priv; >> + struct device_node *node = pdev->dev.of_node; >> + struct resource *res_regs; >> + unsigned int i; >> + int irq, ret; >> + u32 val; >> + >> + if (!node) >> + return -ENOENT; >> + >> + /* Get registers */ >> + res_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (res_regs == NULL) { >> + dev_err(&pdev->dev, "cannot find registers resource\n"); >> + return -ENOENT; >> + } >> + >> + /* Allocate driver data */ >> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) { >> + dev_err(&pdev->dev, "cannot allocate device data\n"); >> + return -ENOMEM; >> + } >> + spin_lock_init(&priv->lock); >> + platform_set_drvdata(pdev, priv); >> + >> + /* Ioremap the registers */ >> + priv->pdc_base = devm_ioremap(&pdev->dev, res_regs->start, >> + res_regs->end - res_regs->start); >> + if (!priv->pdc_base) >> + return -EIO; > > Leaks priv. > >> + /* Get number of peripherals */ >> + ret = of_property_read_u32(node, "num-perips", &val); >> + if (ret) { >> + dev_err(&pdev->dev, "No num-perips node property found\n"); >> + return -EINVAL; > > Leaks priv and mapping > >> + } >> + if (val > SYS0_HWIRQ) { >> + dev_err(&pdev->dev, "num-perips (%u) out of range\n", val); >> + return -EINVAL; > > Error handling is optional, right? > >> +static int pdc_intc_remove(struct platform_device *pdev) >> +{ >> + struct pdc_intc_priv *priv = platform_get_drvdata(pdev); >> + >> + irq_domain_remove(priv->domain); > > And the rest of the resources is still there? I was under the impression devm_kzalloc and devm_ioremap took care of that in both the probe error case and the remove case: > * devm_kzalloc - Resource-managed kzalloc > * Managed kzalloc. Memory allocated with this function is > * automatically freed on driver detach. > * devm_ioremap - Managed ioremap() > * Managed ioremap(). Map is automatically unmapped on driver detach. I may have misunderstood the whole point of their existence though? Thanks James -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html