Re: [PATCH 3/4] mfd: twl6030-irq: convert to use linear irq_domain

[Lee Jones]

On Tue, 23 Jul 2013, Grygorii Strashko wrote:

> Since the TWL6030 PMIC is used with OMAP4 SoCs only and OMAP4 legacy
> boot is dropped there are no needs to allocate the range of IRQ
> descriptors during system boot to support TWL6030 IRQs.
> 
> Hence, convert it to use linear irq_domain and move IRQ configuration in
> .map()/.unmap() callbacks of irq_domain. So, IRQ mapping and descriptors
> allocation will be performed dynamically basing on DT configuration.
> 
> The error message will be reported in case if unmapped IRQ is received by
> TWL6030 (virq==0).
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
> ---
>  drivers/mfd/twl6030-irq.c |  114 ++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> index 790cc28..89f130b 100644
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
> @@ -138,6 +138,7 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>  static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  {
>  	int i, ret;
> +	struct irq_domain *irq_domain = (struct irq_domain *)data;
>  	union {
>  		u8 bytes[4];
>  		u32 int_sts;
> @@ -161,9 +162,14 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  
>  	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
>  		if (sts.int_sts & 0x1) {
> -			int module_irq = twl6030_irq_base +
> -					twl6030_interrupt_mapping[i];
> -			handle_nested_irq(module_irq);
> +			int module_irq =
> +				irq_find_mapping(irq_domain,
> +						 twl6030_interrupt_mapping[i]);
> +			if (module_irq)
> +				handle_nested_irq(module_irq);
> +			else
> +				pr_err("%s: Unmapped PIH ISR %u detected\n",
> +				       __func__, i);

Same here. Does the user really care which function failed?

Please consider replacing with the device name.

>  			pr_debug("%s: PIH ISR %u, virq%u\n",
>  				 __func__, i, module_irq);
>  		}
> @@ -186,19 +192,6 @@ static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  
>  /*----------------------------------------------------------------------*/
>  
> -static inline void activate_irq(int irq)
> -{
> -#ifdef CONFIG_ARM
> -	/* ARM requires an extra step to clear IRQ_NOREQUEST, which it
> -	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
> -	 */
> -	set_irq_flags(irq, IRQF_VALID);
> -#else
> -	/* same effect on other architectures */
> -	irq_set_noprobe(irq);
> -#endif
> -}
> -
>  static int twl6030_irq_set_wake(struct irq_data *d, unsigned int on)
>  {
>  	if (on)
> @@ -308,28 +301,54 @@ int twl6030_mmc_card_detect(struct device *dev, int slot)
>  }
>  EXPORT_SYMBOL(twl6030_mmc_card_detect);
>  
> +static struct irq_chip twl6030_irq_chip;
> +
> +static int twl6030_irq_map(struct irq_domain *d, unsigned int virq,
> +			      irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_data(virq, &twl6030_irq_chip);
> +	irq_set_chip_and_handler(virq,  &twl6030_irq_chip, handle_simple_irq);
> +	irq_set_nested_thread(virq, true);
> +
> +#ifdef CONFIG_ARM
> +	/*
> +	 * ARM requires an extra step to clear IRQ_NOREQUEST, which it
> +	 * sets on behalf of every irq_chip.  Also sets IRQ_NOPROBE.
> +	 */
> +	set_irq_flags(virq, IRQF_VALID);
> +#else
> +	/* same effect on other architectures */
> +	irq_set_noprobe(virq);
> +#endif
> +
> +	return 0;
> +}
> +
> +static void twl6030_irq_unmap(struct irq_domain *d, unsigned int virq)
> +{
> +#ifdef CONFIG_ARM
> +	set_irq_flags(virq, 0);
> +#endif
> +	irq_set_chip_and_handler(virq, NULL, NULL);
> +	irq_set_chip_data(virq, NULL);
> +}
> +
> +static struct irq_domain_ops twl6030_irq_domain_ops = {
> +	.map	= twl6030_irq_map,
> +	.unmap	= twl6030_irq_unmap,
> +	.xlate	= irq_domain_xlate_onetwocell,
> +};
> +
>  int twl6030_init_irq(struct device *dev, int irq_num)
>  {
>  	struct			device_node *node = dev->of_node;
> -	int			nr_irqs, irq_base, irq_end;
> -	static struct irq_chip  twl6030_irq_chip;
> +	int			nr_irqs;
>  	int			status;
> -	int			i;
>  	u8			mask[3];
> +	struct irq_domain	*irq_domain;
>  
>  	nr_irqs = TWL6030_NR_IRQS;
>  
> -	irq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
> -	if (IS_ERR_VALUE(irq_base)) {
> -		dev_err(dev, "Fail to allocate IRQ descs\n");
> -		return irq_base;
> -	}
> -
> -	irq_domain_add_legacy(node, nr_irqs, irq_base, 0,
> -			      &irq_domain_simple_ops, NULL);
> -
> -	irq_end = irq_base + nr_irqs;
> -
>  	mask[0] = 0xFF;
>  	mask[1] = 0xFF;
>  	mask[2] = 0xFF;
> @@ -346,8 +365,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  		return status;
>  	}
>  
> -	twl6030_irq_base = irq_base;
> -
>  	/*
>  	 * install an irq handler for each of the modules;
>  	 * clone dummy irq_chip since PIH can't *do* anything
> @@ -357,20 +374,18 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  	twl6030_irq_chip.irq_set_type = NULL;
>  	twl6030_irq_chip.irq_set_wake = twl6030_irq_set_wake;
>  
> -	for (i = irq_base; i < irq_end; i++) {
> -		irq_set_chip_and_handler(i, &twl6030_irq_chip,
> -					 handle_simple_irq);
> -		irq_set_chip_data(i, (void *)irq_num);
> -		irq_set_nested_thread(i, true);
> -		activate_irq(i);
> +	irq_domain = irq_domain_add_linear(node, nr_irqs,
> +					   &twl6030_irq_domain_ops, NULL);
> +	if (!irq_domain) {
> +		dev_err(dev, "Can't add irq_domain\n");
> +		return -ENOMEM;
>  	}
>  
> -	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
> -		 irq_num, irq_base, irq_end);
> +	dev_info(dev, "PIH (irq %d) nested IRQs\n", irq_num);
>  
>  	/* install an irq handler to demultiplex the TWL6030 interrupt */
>  	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
> -				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
> +				      IRQF_ONESHOT, "TWL6030-PIH", irq_domain);
>  	if (status < 0) {
>  		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>  		goto fail_irq;
> @@ -378,26 +393,19 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  
>  	twl_irq = irq_num;
>  	register_pm_notifier(&twl6030_irq_pm_notifier_block);
> -	return irq_base;
> +	return irq_num;

I think you need to change twl-core to now expect the total number of
IRQs rather than the base one now.

>  fail_irq:
> -	for (i = irq_base; i < irq_end; i++)
> -		irq_set_chip_and_handler(i, NULL, NULL);
> -
> +	irq_domain_remove(irq_domain);

Why do you kill the irqdomain here, but not in exit()?

>  	return status;
>  }
>  
>  int twl6030_exit_irq(void)
>  {
> -	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
> -
> -	if (!twl6030_irq_base) {
> -		pr_err("twl6030: can't yet clean up IRQs?\n");
> -		return -ENOSYS;
> +	if (twl_irq) {
> +		unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
> +		free_irq(twl_irq, NULL);
>  	}

Ah yes, that's better.

> -
> -	free_irq(twl_irq, NULL);
> -
>  	return 0;
>  }
>  

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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/