linux-kernel - Re: [PATCH REBASE] irqchip: Add TB10x interrupt controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.02.1305271414270.4220@ionos>
Date:	Mon, 27 May 2013 14:26:36 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Christian Ruppert <christian.ruppert@...lis.com>
cc:	Vineet Gupta <Vineet.Gupta1@...opsys.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Pierrick Hascoet <pierrick.hascoet@...lis.com>
Subject: Re: [PATCH REBASE] irqchip: Add TB10x interrupt controller driver

On Tue, 7 May 2013, Christian Ruppert wrote:
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include "../../drivers/irqchip/irqchip.h"

#include "irqchip.h" perhaps?

> +#define AB_IRQCTL_INT_ENABLE   (0x00)

What's the purpose of the parens? Decrease readability?

> +static void tb10x_irq_unmask(struct irq_data *data)
> +{
> +	struct tb10x_ictl *ictl = irq_data_get_irq_chip_data(data);
> +	unsigned int hwirq = irqd_to_hwirq(data);
> +
> +	ab_irqctl_writereg(ictl, AB_IRQCTL_INT_ENABLE,
> +		ab_irqctl_readreg(ictl, AB_IRQCTL_INT_ENABLE) | (1 << hwirq));

Another implementation of the generic interrupt chip functionality.

> +static int tb10x_irq_map(struct irq_domain *d, unsigned int irq,
> +				irq_hw_number_t hw)
> +{
> +	irq_set_chip_data(irq, d->host_data);
> +	irq_set_chip_and_handler(irq, &irq_tb10x_chip, handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static int tb10x_irq_xlate(struct irq_domain *d, struct device_node *node,
> +			const u32 *intspec, unsigned int intsize,
> +			unsigned long *out_hwirq, unsigned int *out_type)
> +{
> +	static const unsigned int tb10x_xlate_types[] = {
> +		IRQ_TYPE_EDGE_RISING,
> +		IRQ_TYPE_LEVEL_LOW,
> +		IRQ_TYPE_LEVEL_HIGH,
> +		IRQ_TYPE_EDGE_FALLING,

Why do you need to introduce another format for specifying the
interrupt type instead of using the IRQ_TYPE values and
irq_domain_xlate_twocell()?

> +static void tb10x_irq_cascade(unsigned int irq, struct irq_desc *desc)
> +{
> +	struct tb10x_ictl *ictl = irq_desc_get_handler_data(desc);
> +
> +	generic_handle_irq(irq_find_mapping(ictl->domain, irq));
> +}
> +
> +static int __init of_tb10x_init_irq(struct device_node *ictl,
> +					struct device_node *parent)
> +{
> +	int i;
> +	int ret;
> +	int nrirqs = of_irq_count(ictl);

One line for all those ints please

> +	ic->domain = irq_domain_add_tree(ictl, &irq_tb10x_domain_ops, ic);

Why do you want a tree, if you require that the interrupts are mapped
1:1 to the hardware interrupt numbers? This doesn't make any sense at
all and makes the above cascade handler a pointless waste of cpu cycles.

Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/