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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
