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

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 07/24/2013 02:35 PM, Lee Jones wrote:
> 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@ti.com>
>> ---
>>   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.

ok.

>
>>   			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.

Would it be ok if twl6030_init_irq() will return 0 on success or error 
code on failure?

The change of twl6030_init_irq() will not affect twl-core in case of 
TWL6030/32 DT-boot --- not DT boot (legacy mode) seems need to be 
dropped form twl-core for TWL6030/32.

>
>>   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()?

this is a failure path and twl_irq == 0, so exit will do nothing,
and it safe to delete irq_domain here, because no child devices will be
created.

>
>>   	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);

In general, I need to add irq_domain_remove(irq_domain) here too
But, there is a big BUT!

>>   	}
>
> Ah yes, that's better.
>
>> -
>> -	free_irq(twl_irq, NULL);
>> -
>>   	return 0;
>>   }
>>
>

Unfortunately, I'm not fully understand how to dispose IRQ mapping and 
free IRQ domain/descriptors correctly.
- IRQ mapping is done when devices are being created from DT (including 
IRQ desc allocation)
- there is API irq_dispose_mapping(), but It's not been called from 
irq_domain_remove() and OF and Driver cores.
- there is no of_platform_unpopulate() yet

So, I can call irq_dispose_mapping() manually from twl6030_exit_irq, but 
child devices will still keep mapped IRQ numbers as their Resources.
:((

More over, in this case I can't use devm_*() API to request IRQ, because 
free_irq() will be called after irq_domian de-initialization.

That's why I've not added irq_domian de-initialization in 
twl6030_exit_irq() and not used devm_*() to request IRQ.


Regards,
- grygorii