Re: [PATCH] irqchip: Renesas INTC External IRQ pin driver

[Kuninori Morimoto]

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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/