Message-ID: <alpine.DEB.2.11.1607191649410.3596@nanos>
Date:	Tue, 19 Jul 2016 18:49:28 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Sebastian Frias <sf84@...oste.net>
cc:	Marc Zyngier <marc.zyngier@....com>,
	Jason Cooper <jason@...edaemon.net>, Mason <slash.tmp@...e.fr>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2] irqchip: add support for SMP irq router

On Tue, 19 Jul 2016, Sebastian Frias wrote:
> +
> +#define DBGERR(__format, ...)	panic("[%s:%d] %s(): " __format,	\
> +				      __FILE__, __LINE__,		\
> +				      __func__, ##__VA_ARGS__)

Please get rid of this macro mess. Use the functions (panic, ...)
directly. There is no value for this file, line, func crappola. Think about
proper texts which can be grepped for.

> +
> +#define DBGWARN(__format, ...)	pr_err("[%s:%d] %s(): " __format,	\

Very consistent. WARN == err ....

> +				       __FILE__, __LINE__,		\
> +				       __func__, ##__VA_ARGS__)
> +
> +
> +#if 0

Don't even think about posting code with "#if 0" or such in it. 

> +#define DBGLOG(__format, ...)   pr_info("[%s:%d] %s(): " __format,	\
> +					__FILE__, __LINE__,		\
> +					__func__, ##__VA_ARGS__)
> +#else
> +#define DBGLOG(__format, ...) do {} while (0)
> +#endif

pr_debug() is there for a reason.

> +#if 0
> +#define SHORT_OR_FULL_NAME full_name
> +#else
> +#define SHORT_OR_FULL_NAME name
> +#endif

Use one and be done with it. Really.

> +
> +#define NODE_NAME(__node__) (__node__ ? __node__->SHORT_OR_FULL_NAME :	\
> +			     "<no-node>")

Sigh. pr_xxx("%s", NULL) prints <null>. That's really sufficient for your
debug/error handling.

> +
> +#define BITMASK_VECTOR_SIZE(__count__) (__count__ / 32)
> +#define IRQ_TO_OFFSET(__hwirq__) (__hwirq__ * 4)
> +
> +struct tango_irqrouter;
> +
> +/*
> + * Maintains the mapping between a Linux virq and a hwirq
> + * on the parent controller.
> + * It is used by tango_irqdomain_map() or tango_irqdomain_hierarchy_alloc()
> + * to setup the route between input IRQ and output IRQ
> + */
> +struct tango_irqrouter_output {
> +	struct tango_irqrouter *context;
> +
> +	u32 domain_id;
> +
> +	u32 hwirq;
> +	u32 hwirq_level;
> +	u32 virq;
> +
> +	int shared_count;
> +	int *shared_irqs;
> +};
> +
> +/*
> + * Context for the driver
> + */
> +struct tango_irqrouter {
> +	raw_spinlock_t lock;
> +	struct device_node *node;
> +	void __iomem *base;

Plase align struct members proper

	raw_spinlock_t		lock;
	struct device_node	*node;
	void __iomem		*base;

> +
> +	int input_count;
> +	u32 irq_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +	u32 irq_invert_mask[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];

You only ever use BITMASK_VECTOR_SIZE(ROUTER_INPUTS). Why can't you simply
do:

#define ROUTER_INPUTs_MASK_SIZE	(ROUTER__INPUTS / 32) ????

> +static inline u32 tango_readl(struct tango_irqrouter *irqrouter,
> +			      int reg)
> +{
> +	u32 val = readl_relaxed(irqrouter->base + reg);
> +	/*DBGLOG("r[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
> +	return val;

Please get rid of this debug nonsense.

> +}
> +
> +
> +static inline void tango_writel(struct tango_irqrouter *irqrouter,
> +				int reg,
> +				u32 val)
> +{
> +	/*DBGLOG("w[0x%08x + 0x%08x = 0x%08x] = 0x%08x\n",
> +	  irqrouter->base, reg, irqrouter->base + reg, val);*/
> +	writel_relaxed(val, irqrouter->base + reg);
> +}
> +
> +
> +static inline void tango_set_swirq_enable(struct tango_irqrouter *irqrouter,
> +					  int swirq,
> +					  bool enable)
> +{
> +	u32 offset = SWIRQ_ENABLE;

If you name that varible 'reg' then it becomes obvious what it does.

> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 swirq_bit_index = swirq % SWIRQ_COUNT;
> +
> +#if 1
> +	DBGLOG("%smask swirq(in) %d : current regvalue 0x%x\n",
> +	       enable ? "un":"",
> +	       swirq, value);
> +#endif
> +
> +	if (enable) {
> +		/* unmask swirq */

The whole code lacks comments, but here you document the obvious ....

> +		irqrouter->swirq_mask |= (1 << swirq_bit_index);
> +		value |= (1 << swirq_bit_index);
> +	} else {
> +		/* mask swirq */
> +		irqrouter->swirq_mask &= ~(1 << swirq_bit_index);
> +		value &= ~(1 << swirq_bit_index);
> +	}
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_hwirq_enable(struct tango_irqrouter *irqrouter,
> +					  int hwirq,
> +					  bool enable)
> +{
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +	u32 *enable_mask = &(irqrouter->irq_mask[hwirq_reg_index]);
> +
> +#if 1
> +	DBGLOG("%smask hwirq(in) %d : current regvalue 0x%x\n",
> +	       enable ? "un":"",
> +	       hwirq, value);
> +#endif
> +
> +	if (enable) {
> +		/* unmask irq */
> +		*enable_mask |= (1 << hwirq_bit_index);
> +		value |= IRQ_ROUTER_ENABLE_MASK;
> +	} else {
> +		/* mask irq */
> +		*enable_mask &= ~(1 << hwirq_bit_index);
> +		value &= ~(IRQ_ROUTER_ENABLE_MASK);
> +	}
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_swirq_inversion(struct tango_irqrouter *irqrouter,
> +					     int swirq,
> +					     bool invert)
> +{
> +
> +	DBGLOG("swirq(in) %d %s inverted\n", swirq, invert ? "":"not");
> +
> +	if (invert)
> +		DBGERR("SW IRQs cannot be inverted!\n");

Groan.

> +}
> +
> +
> +static inline void tango_set_hwirq_inversion(struct tango_irqrouter *irqrouter,
> +					     int hwirq,
> +					     bool invert)
> +{
> +	u32 offset = IRQ_TO_OFFSET(hwirq);
> +	u32 value = tango_readl(irqrouter, offset);
> +	u32 hwirq_reg_index = hwirq / 32;
> +	u32 hwirq_bit_index = hwirq % 32;
> +	u32 *invert_mask = &(irqrouter->irq_invert_mask[hwirq_reg_index]);
> +
> +	if (invert) {
> +		*invert_mask |= (1 << hwirq_bit_index);
> +		value |= IRQ_ROUTER_INVERT_MASK;
> +	} else {
> +		*invert_mask &= ~(1 << hwirq_bit_index);
> +		value &= ~(IRQ_ROUTER_INVERT_MASK);
> +	}
> +
> +	DBGLOG("hwirq(in) %d %s inverted\n", hwirq, invert ? "":"not");
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_swirq_route(struct tango_irqrouter *irqrouter,
> +					 int swirq_in,
> +					 int irq_out)
> +{
> +	u32 swirq_reg_index = swirq_in / 4;
> +	u32 swirq_bit_index = (swirq_in % 4) * 8;
> +	u32 mask = ~(0x1f << swirq_bit_index);
> +	u32 offset = SWIRQ_MAP_GROUP0 + (swirq_reg_index * 4);
> +	u32 value = tango_readl(irqrouter, offset);
> +
> +	DBGLOG("ri %d, bi %d, mask 0x%x, offset 0x%x, val 0x%x\n",
> +	       swirq_reg_index,
> +	       swirq_bit_index,
> +	       mask,
> +	       offset,
> +	       value);
> +
> +	DBGLOG("route swirq %d => hwirq(out) %d\n", swirq_in, irq_out);
> +
> +	value &= mask;
> +
> +	if (irq_out < 0) {

This logic is utterly confusing, really. What's a negative interrupt number?

> +		tango_set_irq_enable(irqrouter,
> +				     swirq_in + irqrouter->input_count,
> +				     0);

Now you write back 'value' with the bit for this irq masked. Is that correct?

> +	} else

  } else {

  please

> +		value |= ((irq_out & 0x1f) << swirq_bit_index);
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline void tango_set_hwirq_route(struct tango_irqrouter *irqrouter,
> +					 int irq_in,
> +					 int irq_out)
> +{
> +	u32 offset = IRQ_TO_OFFSET(irq_in);
> +	u32 value;
> +
> +	DBGLOG("route hwirq(in) %d => hwirq(out) %d\n", irq_in, irq_out);
> +
> +	if (irq_out < 0) {
> +		tango_set_irq_enable(irqrouter,
> +				     irq_in,
> +				     0);

This fits in a single line.

> +		value = 0;

Please document why you are setting the value to 0 here.

> +	} else
> +		value = (irq_out & 0x1f);
> +
> +	tango_writel(irqrouter, offset, value);
> +}
> +
> +
> +static inline int tango_set_irq_enable(struct tango_irqrouter *irqrouter,
> +				       int irq,
> +				       bool enable)
> +{
> +	if (irq >= (irqrouter->input_count + irqrouter->swirq_count))
> +		return -EINVAL;

How can that happen? Paranoia programming?

> +	else if (irq >= irqrouter->input_count)

That "else" is pointless. You return above.

> +		tango_set_swirq_enable(irqrouter,
> +				       irq - irqrouter->input_count,
> +				       enable);
> +	else
> +		tango_set_hwirq_enable(irqrouter,
> +				       irq,
> +				       enable);

This can be written way simpler.

     	if (irq >= irqrouter->input_count)
	   	irq -= irqrouter->input_count;
	tango_set_hwirq_enable(irqrouter, irq, enable);
    	  
Hmm?

> +static int tango_set_irq_type(struct tango_irqrouter *irqrouter,
> +			      int hwirq_in,
> +			      u32 type,
> +			      u32 parent_type)
> +{
> +	int err;
> +
> +	if (parent_type & (type & IRQ_TYPE_SENSE_MASK))
> +		/* same polarity */
> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 0);
> +	else
> +		/* invert polarity */
> +		err = tango_set_irq_inversion(irqrouter, hwirq_in, 1);

  	bool invert = parent_type & (type & IRQ_TYPE_SENSE_MASK);

  	err = tango_set_irq_inversion(irqrouter, hwirq_in, invert);	

> +
> +	if (err < 0) {
> +		DBGWARN("Failed to setup IRQ %d polarity\n", hwirq_in);
> +		return err;
> +	}
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_EDGE_FALLING:
> +		DBGERR("Does not support edge triggers\n");

This really sucks. In fact you panic here while the code suggests that you
print a DEBUG error and continue .....

> +		break;
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		break;

You can spare that break.

> +	case IRQ_TYPE_LEVEL_LOW:
> +		break;
> +	default:
> +		DBGWARN("Invalid trigger mode 0x%x for hwirq(in) %d\n",
> +			type, hwirq_in);

So here you continue with an error code, but above for EDGE you panic. I
really don't get that logic.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +
> +static int tango_get_output_for_hwirq(struct tango_irqrouter *irqrouter,
> +				      int hwirq_in,
> +				      struct tango_irqrouter_output **out_val)
> +{
> +	struct tango_irqrouter_output *irqrouter_output;
> +	int i;
> +
> +	if (!out_val)
> +		return -EINVAL;

This is utter crap. Look at the call site:

> +	struct tango_irqrouter_output *irqrouter_output = NULL;
> +
> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> +	if (!irqrouter_output)
> +		goto handle_parent;

So out_val CANNOT be NULL. What's the purpose of your return values if you
ignore them?

> +
> +	/* Get the irqrouter_output for the hwirq */

This is another really helpful comment.

> +	for (i = 0; i < irqrouter->output_count; i++) {
> +		int j;
> +
> +		irqrouter_output = &(irqrouter->output[i]);
> +
> +		for (j = 0; j < irqrouter_output->shared_count; j++) {
> +			if (hwirq_in == irqrouter_output->shared_irqs[j])
> +				goto found_router_output;
> +		}
> +	}
> +	if (i == irqrouter->output_count) {

What other reason would be to get here than this?

> +		DBGWARN("Couldn't find hwirq mapping\n");
> +		return -ENODEV;
> +	}
> +
> +found_router_output:
> +
> +	*out_val = irqrouter_output;
> +	return 0;
> +}

This whole thing can be condensed to:

static struct tango_irqrouter_output *
tango_get_output_for_hwirq(struct tango_irqrouter *irqr, int hwirq_in)
{
	struct tango_irqrouter_output *rout;
	int i, j;

	/* Scan the outputs for a matching hwirq number */
	for (i = 0, rout = irqr->output; i < irqr->output_count; i++, rout++) {
		for (j = 0; j < rout->shared_count; j++) {
			if (rout->shared_irqs[j] == hwirq_in)
			   	return rout;
		}
	}
	return NULL;
}

And the call site would become:

    	struct tango_irqrouter_output *rout;

	rout = tango_get_output_for_hwirq(irqrouter, hwirq_in);
	if (!rout)
		goto handle_parent;

Hmm?

> +/* 'irqchip' handling callbacks
> + * Used for 'shared' IRQs, i.e.: IRQs that share a GIC input
> + * This driver performs the IRQ dispatch based on the flags
> + */
> +

Multiline comments have this format:
	 
/*
 * First line
 * ....
 * Last line
 */

> +static void tango_irqchip_mask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	int hwirq_in = (int)data->hwirq;

Huch? What's that silly typecast for? Why can't you use u32 for hwirq_in?

> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
> +}

> +#ifdef CONFIG_SMP
> +static int tango_irqchip_set_irq_affinity(struct irq_data *data,
> +					  const struct cpumask *mask_val,
> +					  bool force)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct irq_chip *parent_chip = irq_get_chip(irqrouter_output->virq);
> +	struct irq_data *parent_data = irq_get_irq_data(irqrouter_output->virq);
> +
> +	DBGLOG("%s:%s(0x%p)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain);
> +
> +	if (parent_chip && parent_chip->irq_set_affinity)
> +		return parent_chip->irq_set_affinity(parent_data,
> +						     mask_val,
> +						     force);
> +	else
> +		return -EINVAL;

You really can spare identation levels by writing it a bit more clever:

    	if (!parent || !parent->irq_set_affinity)
	   	 return -EINVAL;

	return parent->irq_set_affinity(parent, mask, force);

You might notice that I shortened the variable names and it still is entirely
clear what the code does.

Aside of that mask_val is a clear misnomer.

> +}
> +#endif
> +
> +
> +static inline u32 tango_dispatch_irqs(struct irq_domain *domain,
> +				      struct irq_desc *desc,
> +				      u32 status,
> +				      int base)
> +{
> +	u32 hwirq;
> +	u32 virq;

Please put same types onto a single line

> +
> +	while (status) {
> +		hwirq = __ffs(status);
> +		virq = irq_find_mapping(domain, base + hwirq);
> +		if (unlikely(!virq))
> +			handle_bad_irq(desc);
> +		else
> +			generic_handle_irq(virq);
> +
> +		status &= ~BIT(hwirq);
> +	}
> +
> +	return status;

Huch? status is guaranteed to be 0 here. So what's the point of this return
value.

> +}
> +
> +static void tango_irqdomain_handle_cascade_irq(struct irq_desc *desc)
> +{
> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	struct irq_chip *host_chip = irq_desc_get_chip(desc);
> +	u32 i, status;
> +	u32 swirq_status, irq_status[BITMASK_VECTOR_SIZE(ROUTER_INPUTS)];
> +
> +#if 0
> +	DBGLOG("%s:%s(0x%p): irqrouter_output 0x%p, hwirq(out) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
> +	       irqrouter_output, irqrouter_output->hwirq);
> +#endif
> +
> +	chained_irq_enter(host_chip, desc);
> +
> +	raw_spin_lock(&(irqrouter->lock));
> +	swirq_status = tango_readl(irqrouter, READ_SWIRQ_STATUS);
> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++)
> +		irq_status[i] = tango_readl(irqrouter,
> +					    READ_SYS_IRQ_GROUP0 + i*4);

  i * 4		please

> +	raw_spin_unlock(&(irqrouter->lock));
> +
> +	/* HW irqs */

And that comment tells us what?

> +	for (i = 0; i < BITMASK_VECTOR_SIZE(ROUTER_INPUTS); i++) {
> +#if 0
> +		DBGLOG("%d: 0x%08x (en 0x%08x, inv 0x%08x)\n",
> +		       i,
> +		       irq_status[i],
> +		       irqrouter->irq_mask[0],
> +		       irqrouter->irq_invert_mask[0]);
> +#endif
> +
> +#define HANDLE_INVERTED_LINES(__irqstatus__, __x__) ((((~__irqstatus__) & irqrouter->irq_invert_mask[__x__]) & irqrouter->irq_mask[__x__]) | __irqstatus__)
> +#define HANDLE_EN_AND_INV_MASKS(__irqstatus__, __y__) (HANDLE_INVERTED_LINES(__irqstatus__, __y__) & irqrouter->irq_mask[__y__])

What the hell does this unparseable macro mess? What's so wrong with a proper
inline function?

> +
> +		irq_status[i] = HANDLE_EN_AND_INV_MASKS(irq_status[i], i);
> +		status = tango_dispatch_irqs(domain, desc, irq_status[i], i*32);
> +		if (status & irq_status[i])

Can you pretty please explain how this can happen? It can't. Because you clear
every single bit of status in tango_dispatch_irqs(). See above.

Please clean up that unholy mess of nonsensical checks and debug code.

> +			DBGERR("%s: %d unhandled IRQs (as a mask) 0x%x\n",
> +			       NODE_NAME(irq_domain_get_of_node(domain)),
> +			       i,
> +			       status & irq_status[i]);
> +	}
> +
> +	/* SW irqs */
> +	swirq_status &= irqrouter->swirq_mask;
> +	status = tango_dispatch_irqs(domain, desc, swirq_status, 128);

128 is the magic number pulled out of thin air or what?

> +	if (status & swirq_status)
> +		DBGERR("%s: Unhandled IRQs (as a mask) 0x%x\n",
> +		       NODE_NAME(irq_domain_get_of_node(domain)),
> +		       status & swirq_status);

See above.

> +
> +	chained_irq_exit(host_chip, desc);
> +}
> +
> +
> +/**
> + * tango_irqdomain_map - route a hwirq(in) to a hwirq(out).
> + * NOTE: The hwirq(out) must have been already allocated and enabled on
> + * the parent controller.

Please put notes AFTER the argument descriptions

> + * @hwirq: HW IRQ of the device requesting an IRQ
> + * if hwirq > inputs it is a SW IRQ
> + * @virq: Linux IRQ (associated to the domain) to be given to the device
> + * @domain: IRQ domain (from the domain, we get the irqrouter_output
> + * in order to know to which output we need to route hwirq to)

And if you spend a few seconds to align this proper, then it becomes suddenly
readable.

* @hwirq:	HW IRQ of the device requesting an IRQ
*		if hwirq > inputs it is a SW IRQ
* @virq:	Linux IRQ (associated to the domain) to be given to the device
* @domain:	IRQ domain (from the domain, we get the irqrouter_output
*		in order to know to which output we need to route hwirq to)

Hmm?

> + */
> +static int tango_irqdomain_map(struct irq_domain *domain,
> +			       unsigned int virq,
> +			       irq_hw_number_t hwirq)
> +{
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +
> +	DBGLOG("%s:%s(0x%p): hwirq(in) %d := virq %d, and route "
> +	       "hwirq(in) %d => hwirq(out) %d (virq %d)\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       (u32)hwirq,
> +	       virq,
> +	       (u32)hwirq,
> +	       irqrouter_output->hwirq,
> +	       irqrouter_output->virq);
> +
> +	if (hwirq >= (irqrouter->input_count + irqrouter->swirq_count))
> +		DBGERR("%s: Invalid hwirq(in) %d >= %d + %d\n",
> +		       NODE_NAME(irq_domain_get_of_node(domain)),
> +		       (u32)hwirq,
> +		       irqrouter->input_count,
> +		       irqrouter->swirq_count);
> +	else if (hwirq >= irqrouter->input_count)
> +		DBGLOG("Map swirq %ld\n", hwirq - irqrouter->input_count);
> +
> +	irq_set_chip_and_handler(virq,
> +				 &tango_irq_chip_shared_ops,
> +				 handle_level_irq);
> +	irq_set_chip_data(virq, domain);
> +	irq_set_probe(virq);
> +
> +	tango_set_irq_route(irqrouter, hwirq, irqrouter_output->hwirq);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * tango_irqdomain_translate - used to select the domain for a given
> + * irq_fwspec

It hardly selects the domain. @domain is handed in as a argument. And please
get rid of that silly 'used to'. That just adds characters and no information.

    - Select the router section for a firmware spec

Hmm?

> + * @domain: a registered domain
> + * @fwspec: an IRQ specification. This callback is used to translate the
> + * parameters given as irq_fwspec into a HW IRQ and Type values.

fwspec is a callback?

> + * @out_hwirq: 
> + * @out_type:

Missing docs.....

> + */
> +static int tango_irqdomain_translate(struct irq_domain *domain,
> +				     struct irq_fwspec *fwspec,
> +				     unsigned long *out_hwirq,
> +				     unsigned int *out_type)
> +{
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	irq_hw_number_t irq;
> +	u32 domain_id, type;
> +	int err;
> +
> +	DBGLOG("%s:%s(0x%p): argc %d for hwirq(out) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       fwspec->param_count,
> +	       irqrouter_output->hwirq);
> +
> +	err = tango_parse_fwspec(domain,
> +				 fwspec,
> +				 &domain_id,
> +				 &irq,
> +				 &type);

Please use the full 80 characters. Your patch does not become more valuable
when you inflate the line count. It's more valuable when it is easy to read
and understand.

> +	if (err < 0) {
> +		DBGWARN("Failed to parse fwspec\n");
> +		return err;
> +	}
> +
> +	switch (domain_id) {
> +	case SIGMA_HWIRQ:
> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
> +		break;
> +	case SIGMA_SWIRQ:
> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
> +		irq += irqrouter->input_count;
> +		break;
> +	default:
> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
> +		       domain_id,
> +		       irqrouter_output->domain_id);

Huch what? You only ever handle HW and SW irqs and now you happily proceed if
the id is something else. Consistent error handling is optional and can be
replaced by random debug prints, right?

> +		break;
> +	};
> +
> +	*out_hwirq = irq;
> +	*out_type  = type & IRQ_TYPE_SENSE_MASK;
> +
> +	DBGLOG("hwirq %d type 0x%x\n", (u32)*out_hwirq, (u32)*out_type);
> +
> +	return 0;
> +}
> +
> +
> +/**
> + * tango_irqdomain_select - used to select the domain for a given irq_fwspec
> + * @domain: a registered domain
> + * @fwspec: an IRQ specification. This callback should return zero if the
> + * irq_fwspec does not belong to the given domain. If it does, it should
> + * return non-zero.

Callback????

> + * In practice it will return non-zero if the irq_fwspec matches one of the
> + * IRQs shared within the given domain.
> + * @bus_token: a bus token
> + */
> +static int tango_irqdomain_select(struct irq_domain *domain,
> +				  struct irq_fwspec *fwspec,
> +				  enum irq_domain_bus_token bus_token)
> +{
> +	struct tango_irqrouter_output *irqrouter_output = domain->host_data;
> +	struct tango_irqrouter *irqrouter = irqrouter_output->context;
> +	irq_hw_number_t irq;
> +	u32 domain_id, type;
> +	int err;
> +
> +	DBGLOG("%s:%s(0x%p): argc %d, 0x%p, bus 0x%x\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)), domain->name, domain,
> +	       fwspec->param_count, fwspec->fwnode, bus_token);
> +
> +	DBGLOG("router 0x%p, output 0x%p\n", irqrouter, irqrouter_output);
> +
> +	err = tango_parse_fwspec(domain, fwspec, &domain_id, &irq, &type);
> +	if (err < 0)
> +		return 0;
> +
> +	switch (domain_id) {
> +	case SIGMA_HWIRQ:
> +		DBGLOG("Request is for SIGMA_HWIRQ\n");
> +		break;
> +	case SIGMA_SWIRQ:
> +		DBGLOG("Request is for SIGMA_SWIRQ\n");
> +		break;
> +	default:
> +		DBGLOG("Request is for domain ID 0x%x (we are 0x%x)\n",
> +		       domain_id,
> +		       irqrouter_output->domain_id);
> +		break;
> +	};
> +
> +	if (!irqrouter->implicit_groups) {
> +		int i;
> +
> +		/* Check if the requested IRQ belongs to those listed
> +		 * to be sharing the output assigned to this domain

Your using of 'domain' is utterly confusing. Please use something like
hwdomain or whatever which makes it clear that this is something else than the
irq domain. I tripped over this several times now.

> +		 */
> +		if (irqrouter_output->shared_count <= 0) {
> +			DBGLOG("Not shared IRQ line?\n");
> +			return 0;
> +		}
> +
> +		for (i = 0; i < irqrouter_output->shared_count; i++) {
> +			if (irq == irqrouter_output->shared_irqs[i]) {
> +				DBGLOG("Match: IRQ %lu\n", irq);
> +				return 1;
> +			}
> +		}
> +	} else {
> +		/* Otherwise, check if the domain_id given matches
> +		 * the one assigned to this output
> +		 */
> +		if (domain_id == irqrouter_output->domain_id) {
> +			DBGLOG("Match: Domain ID %d\n", domain_id);
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +
> +/* 'irqrouter' handling callbacks
> + * Used for 'direct' IRQs, i.e.: IRQs that are directly routed to the GIC
> + * This driver does not dispatch the IRQs, the GIC does.
> + */
> +
> +
> +static void tango_irqrouter_mask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter *irqrouter = domain->host_data;
> +	int hwirq_in = (int)data->hwirq;
> +
> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       hwirq_in);
> +
> +	tango_set_irq_enable(irqrouter, hwirq_in, 0);
> +
> +	irq_chip_mask_parent(data);
> +}
> +
> +
> +static void tango_irqrouter_unmask_irq(struct irq_data *data)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter *irqrouter = domain->host_data;
> +	int hwirq_in = (int)data->hwirq;
> +
> +	DBGLOG("%s:%s(0x%p) hwirq(in) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       hwirq_in);
> +
> +	tango_set_irq_enable(irqrouter, hwirq_in, 1);
> +
> +	irq_chip_unmask_parent(data);
> +}
> +
> +
> +static int tango_irqrouter_set_irq_type(struct irq_data *data,
> +					unsigned int type)
> +{
> +	struct irq_domain *domain = irq_data_get_irq_chip_data(data);
> +	struct tango_irqrouter_output *irqrouter_output = NULL;
> +	struct tango_irqrouter *irqrouter = domain->host_data;
> +	int hwirq_in = (int)data->hwirq;
> +	u32 parent_type;
> +
> +	DBGLOG("%s:%s(0x%p) type 0x%x for hwirq(in) %d\n",
> +	       NODE_NAME(irq_domain_get_of_node(domain)),
> +	       domain->name,
> +	       domain,
> +	       type,
> +	       hwirq_in);
> +
> +	tango_get_output_for_hwirq(irqrouter, hwirq_in, &irqrouter_output);
> +	if (!irqrouter_output)
> +		goto handle_parent;

So if there is no output, then you unconditionally set the type on the
parent. If that's correct, this needs a big fat comment at least.

> +	parent_type = (irqrouter_output->hwirq_level & IRQ_TYPE_SENSE_MASK);
> +	tango_set_irq_type(irqrouter, hwirq_in, type, parent_type);
> +
> +handle_parent:
> +	return irq_chip_set_type_parent(data, type);
> +}

I'm stopping here. You can apply the above to the rest of the code as well.

Thanks,

	tglx

Powered by blists - more mailing lists