Messages in this thread Patch in this message | | | Date | Mon, 18 Feb 2013 17:04:30 -0800 (PST) | From | Kuninori Morimoto <> | Subject | Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver |
| |
Hi Magnus
Thank you for this patch. Small comment from me :)
> +struct intc_irqpin_priv { > + struct intc_irqpin_iomem iomem[INTC_IRQPIN_REG_NR]; > + struct intc_irqpin_irq irq[INTC_IRQPIN_MAX]; > + struct renesas_intc_irqpin_config config; > + unsigned int number_of_irqs; > + struct platform_device *pdev; > + struct irq_chip irq_chip; > + struct irq_domain *irq_domain; > +};
(snip)
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */ > + > +static void intc_irqpin_read_modify_write(struct intc_irqpin_priv *p, > + int reg, int shift, > + int width, int value) > +{ > + unsigned long flags; > + unsigned long tmp; > + > + raw_spin_lock_irqsave(&intc_irqpin_lock, flags); > + > + tmp = intc_irqpin_read(p, reg); > + tmp &= ~(((1 << width) - 1) << shift); > + tmp |= value << shift; > + intc_irqpin_write(p, reg, tmp); > + > + raw_spin_unlock_irqrestore(&intc_irqpin_lock, flags); > +}
It is possible to include this spin lock into priv ? This local static spin lock seems not wrong, but looks strange ?
> +static int intc_irqpin_probe(struct platform_device *pdev) > +{ > + struct renesas_intc_irqpin_config *pdata = pdev->dev.platform_data; > + struct intc_irqpin_priv *p; > + struct intc_irqpin_iomem *i; > + struct resource *io[INTC_IRQPIN_REG_NR]; > + struct resource *irq; > + struct irq_chip *irq_chip; > + void (*enable_fn)(struct irq_data *d); > + void (*disable_fn)(struct irq_data *d); > + const char *name = dev_name(&pdev->dev); > + int ret; > + int k; > + > + p = kzalloc(sizeof(*p), GFP_KERNEL);
can you use devm_kzalloc() ?
> + if (!p) { > + dev_err(&pdev->dev, "failed to allocate driver data\n"); > + ret = -ENOMEM; > + goto err0; > + } > + > + /* deal with driver instance configuration */ > + if (pdata) > + memcpy(&p->config, pdata, sizeof(*pdata)); > + if (!p->config.sense_bitfield_width) > + p->config.sense_bitfield_width = 4; /* default to 4 bits */ > + > + p->pdev = pdev; > + platform_set_drvdata(pdev, p); > + > + /* get hold of manadatory IOMEM */ > + for (k = 0; k < INTC_IRQPIN_REG_NR; k++) { > + io[k] = platform_get_resource(pdev, IORESOURCE_MEM, k); > + if (!io[k]) { > + dev_err(&pdev->dev, "not enough IOMEM resources\n"); > + ret = -EINVAL; > + goto err1; > + } > + } > + > + /* allow any number of IRQs between 1 and INTC_IRQPIN_MAX */ > + for (k = 0; k < INTC_IRQPIN_MAX; k++) { > + irq = platform_get_resource(pdev, IORESOURCE_IRQ, k); > + if (!irq) > + break; > + > + p->irq[k].hw_irq = k; > + p->irq[k].p = p; > + p->irq[k].irq = irq->start; > + } > + > + p->number_of_irqs = k; > + if (p->number_of_irqs < 1) { > + dev_err(&pdev->dev, "not enough IRQ resources\n"); > + ret = -EINVAL; > + goto err1; > + } > + > + /* ioremap IOMEM and setup read/write callbacks */ > + for (k = 0; k < INTC_IRQPIN_REG_NR; k++) { > + i = &p->iomem[k]; > + > + switch (resource_size(io[k])) { > + case 1: > + i->width = 8; > + i->read = intc_irqpin_read8; > + i->write = intc_irqpin_write8; > + break; > + case 4: > + i->width = 32; > + i->read = intc_irqpin_read32; > + i->write = intc_irqpin_write32; > + break; > + default: > + dev_err(&pdev->dev, "IOMEM size mismatch\n"); > + ret = -EINVAL; > + goto err2; > + } > + > + i->iomem = ioremap_nocache(io[k]->start, resource_size(io[k]));
devm_ioremap_nocache() or devm_request_and_ioremap()
> + if (!i->iomem) { > + dev_err(&pdev->dev, "failed to remap IOMEM\n"); > + ret = -ENXIO; > + goto err2; > + } > + } > + > + /* mask all interrupts using priority */ > + for (k = 0; k < p->number_of_irqs; k++) > + intc_irqpin_mask_unmask_prio(p, k, 1); > + > + /* use more severe masking method if requested */ > + if (p->config.control_parent) { > + enable_fn = intc_irqpin_irq_enable_force; > + disable_fn = intc_irqpin_irq_disable_force; > + } else { > + enable_fn = intc_irqpin_irq_enable; > + disable_fn = intc_irqpin_irq_disable; > + } > + > + irq_chip = &p->irq_chip; > + irq_chip->name = name; > + irq_chip->irq_mask = disable_fn; > + irq_chip->irq_unmask = enable_fn; > + irq_chip->irq_enable = enable_fn; > + irq_chip->irq_disable = disable_fn; > + irq_chip->irq_set_type = intc_irqpin_irq_set_type; > + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE; > + > + p->irq_domain = irq_domain_add_simple(pdev->dev.of_node, > + p->number_of_irqs, > + p->config.irq_base, > + &intc_irqpin_irq_domain_ops, p); > + if (!p->irq_domain) { > + ret = -ENXIO; > + dev_err(&pdev->dev, "cannot initialize irq domain\n"); > + goto err2; > + } > + > + /* request and set priority on interrupts one by one */ > + for (k = 0; k < p->number_of_irqs; k++) { > + if (request_irq(p->irq[k].irq, intc_irqpin_irq_handler,
Can you use devm_request_irq()
> + 0, name, &p->irq[k])) { > + dev_err(&pdev->dev, "failed to request low IRQ\n"); > + ret = -ENOENT; > + goto err3; > + } > + intc_irqpin_mask_unmask_prio(p, k, 0); > + } > + > + dev_info(&pdev->dev, "driving %d irqs\n", p->number_of_irqs); > + > + /* warn in case of mismatch if irq base is specified */ > + if (p->config.irq_base) { > + k = irq_find_mapping(p->irq_domain, 0); > + if (p->config.irq_base != k) > + dev_warn(&pdev->dev, "irq base mismatch (%d/%d)\n", > + p->config.irq_base, k); > + } > + > + return 0; > + > +err3: > + for (; k >= 0; k--) > + free_irq(p->irq[k - 1].irq, &p->irq[k - 1]); > + > + irq_domain_remove(p->irq_domain); > +err2: > + for (k = 0; k < INTC_IRQPIN_REG_NR; k++) > + iounmap(p->iomem[k].iomem); > +err1: > + kfree(p);
if you used devm_xxxx, you can remove kfree() / free_irq() / iounmap() here
> +err0: > + return ret; > +} > + > +static int intc_irqpin_remove(struct platform_device *pdev) > +{ > + struct intc_irqpin_priv *p = platform_get_drvdata(pdev); > + int k; > + > + for (k = 0; k < p->number_of_irqs; k++) > + free_irq(p->irq[k].irq, &p->irq[k]); > + > + irq_domain_remove(p->irq_domain); > + > + for (k = 0; k < INTC_IRQPIN_REG_NR; k++) > + iounmap(p->iomem[k].iomem); > + > + kfree(p); > + return 0; > +}
same as above
> --- /dev/null > +++ work/include/linux/platform_data/irq-renesas-intc-irqpin.h 2013-02-18 18:28:24.000000000 +0900 > @@ -0,0 +1,10 @@ > +#ifndef __IRQ_RENESAS_INTC_IRQPIN_H__ > +#define __IRQ_RENESAS_INTC_IRQPIN_H__ GPL license signage ?
> + > +struct renesas_intc_irqpin_config { > + unsigned int sense_bitfield_width; > + unsigned int irq_base; > + bool control_parent; > +}; > + > +#endif /* __IRQ_RENESAS_INTC_IRQPIN_H__ */
Best regards --- Kuninori Morimoto
| |