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

[Thomas Gleixner <tglx@linutronix.de>]

Magnus,

On Mon, 18 Feb 2013, Magnus Damm wrote:

> +static inline unsigned long intc_irqpin_read(struct intc_irqpin_priv *p,
> +					     int reg)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];

Newline between variable and code please.

> +	return i->read(i->iomem);
> +}
> +
> +static inline void intc_irqpin_write(struct intc_irqpin_priv *p,
> +				     int reg, unsigned long data)
> +{
> +	struct intc_irqpin_iomem *i = &p->iomem[reg];
> +	i->write(i->iomem, data);
> +}
> +
> +static inline unsigned long intc_irqpin_hwirq_mask(struct intc_irqpin_priv *p,
> +						   int reg, int hw_irq)
> +{
> +	return BIT((p->iomem[reg].width - 1) - hw_irq);
> +}
> +
> +static inline void intc_irqpin_irq_write_hwirq(struct intc_irqpin_priv *p,
> +					       int reg, int hw_irq)
> +{
> +	intc_irqpin_write(p, reg, intc_irqpin_hwirq_mask(p, reg, hw_irq));
> +}
> +
> +static DEFINE_RAW_SPINLOCK(intc_irqpin_lock); /* only used by slow path */

Shouldn't the lock be part of struct intc_irqpin_priv ?

> +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);
> +static void intc_irqpin_dbg(struct intc_irqpin_irq *i, char *str)
> +{
> +	dev_dbg(&i->p->pdev->dev, "%s (%d:%d:%d)\n",
> +		str, i->irq, i->hw_irq,
> +		irq_find_mapping(i->p->irq_domain, i->hw_irq));

Do you really want to do a lookup for each debug invocation?

> +}
> +
> +static void intc_irqpin_irq_enable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "enable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_CLEAR, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_disable(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int hw_irq = irqd_to_hwirq(d);
> +
> +	intc_irqpin_dbg(&p->irq[hw_irq], "disable");
> +	intc_irqpin_irq_write_hwirq(p, INTC_IRQPIN_REG_MASK, hw_irq);
> +}
> +
> +static void intc_irqpin_irq_enable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	intc_irqpin_irq_enable(d);
> +	irq_get_chip(irq)->irq_unmask(irq_get_irq_data(irq));
> +}
> +
> +static void intc_irqpin_irq_disable_force(struct irq_data *d)
> +{
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +	int irq = p->irq[irqd_to_hwirq(d)].irq;
> +
> +	irq_get_chip(irq)->irq_mask(irq_get_irq_data(irq));
> +	intc_irqpin_irq_disable(d);

Hmm. If I understand that code correctly, then the _force functions
are (un)masking another interrupt line. But this happens without
holding irq_desc[irq]->lock. Looks unsafe at least and if correct
wants a big fat comment.

> +}
> +
> +#define INTC_IRQ_SENSE_VALID 0x10
> +#define INTC_IRQ_SENSE(x) (x + INTC_IRQ_SENSE_VALID)
> +
> +static unsigned char intc_irqpin_sense[IRQ_TYPE_SENSE_MASK + 1] = {
> +	[IRQ_TYPE_EDGE_FALLING] = INTC_IRQ_SENSE(0x00),
> +	[IRQ_TYPE_EDGE_RISING] = INTC_IRQ_SENSE(0x01),
> +	[IRQ_TYPE_LEVEL_LOW] = INTC_IRQ_SENSE(0x02),
> +	[IRQ_TYPE_LEVEL_HIGH] = INTC_IRQ_SENSE(0x03),
> +	[IRQ_TYPE_EDGE_BOTH] = INTC_IRQ_SENSE(0x04),
> +};
> +
> +static int intc_irqpin_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	unsigned char value = intc_irqpin_sense[type & IRQ_TYPE_SENSE_MASK];
> +	struct intc_irqpin_priv *p = irq_data_get_irq_chip_data(d);
> +
> +	if (!(value & INTC_IRQ_SENSE_VALID))
> +		return -EINVAL;
> +
> +	return intc_irqpin_set_sense(p, irqd_to_hwirq(d),
> +				     value ^ INTC_IRQ_SENSE_VALID);
> +}
> +
> +static irqreturn_t intc_irqpin_irq_handler(int irq, void *dev_id)
> +{
> +	struct intc_irqpin_irq *i = dev_id;
> +	struct intc_irqpin_priv *p = i->p;
> +	unsigned long bit;
> +
> +	intc_irqpin_dbg(i, "demux1");
> +	bit = intc_irqpin_hwirq_mask(p, INTC_IRQPIN_REG_SOURCE, i->hw_irq);
> +
> +	if (intc_irqpin_read(p, INTC_IRQPIN_REG_SOURCE) & bit) {
> +		intc_irqpin_write(p, INTC_IRQPIN_REG_SOURCE, ~bit);
> +		intc_irqpin_dbg(i, "demux2");
> +		generic_handle_irq(irq_find_mapping(p->irq_domain, i->hw_irq));

Shouldn't you cache that mapping value somewhere ?

> +		return IRQ_HANDLED;
> +	}
> +	return IRQ_NONE;
> +}
> +
> +static int intc_irqpin_irq_domain_map(struct irq_domain *h, unsigned int virq,
> +				      irq_hw_number_t hw)
> +{
> +	struct intc_irqpin_priv *p = h->host_data;
> +
> +	intc_irqpin_dbg(&p->irq[hw], "map");
> +	irq_set_chip_data(virq, h->host_data);
> +	irq_set_chip_and_handler(virq, &p->irq_chip, handle_level_irq);
> +	set_irq_flags(virq, IRQF_VALID); /* kill me now */

What needs to be killed? -ENOPARSE

Thanks,

	tglx