Message-ID: <20130724104945.GH26801@laptop>
Date:	Wed, 24 Jul 2013 11:49:45 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Grygorii Strashko <grygorii.strashko@...com>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	Kevin Hilman <khilman@...aro.org>,
	Graeme Gregory <gg@...mlogic.co.uk>,
	linux-omap@...r.kernel.org, Ruslan Bilovol <ruslan.bilovol@...com>,
	linux-kernel@...r.kernel.org,
	Naga Venkata Srikanth V <vnv.srikanth@...sung.com>,
	Oleg_Kosheliev <oleg.kosheliev@...com>
Subject: Re: [PATCH 1/4] mfd: twl6030-irq: migrate to IRQ threaded handler

On Tue, 23 Jul 2013, Grygorii Strashko wrote:

> From: Naga Venkata Srikanth V <vnv.srikanth@...sung.com>
> 
> 1) Removed request_irq() and replaced it with request_threaded_irq().
> 
> 2) Removed generic_handle_irq() and replaced it with
> handle_nested_irq().
>   Handling of these interrupts is nested, as we are handling an
> interrupt (for e.g rtc, mmc1) when we are still servicing TWL irq.
> 
> 3) Removed I2C read-retry logic for the case when twl_i2c_read() is
> failed inside IRQ handler - there is no sense to do that, so just report
> an error and return.
> 
> Signed-off-by: Naga Venkata Srikanth V <vnv.srikanth@...sung.com>
> Signed-off-by: Oleg_Kosheliev <oleg.kosheliev@...com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@...com>
> ---
>  drivers/mfd/twl6030-irq.c |  146 +++++++++++++++------------------------------
>  1 file changed, 49 insertions(+), 97 deletions(-)

Besides the points I mention below I like the way this patch is
going.

> diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
> index 277a8db..b6030d9 100644
> --- a/drivers/mfd/twl6030-irq.c
> +++ b/drivers/mfd/twl6030-irq.c
> @@ -90,7 +90,6 @@ static unsigned twl6030_irq_base;
>  static int twl_irq;
>  static bool twl_irq_wake_enabled;
>  
> -static struct completion irq_event;
>  static atomic_t twl6030_wakeirqs = ATOMIC_INIT(0);
>  
>  static int twl6030_irq_pm_notifier(struct notifier_block *notifier,
> @@ -131,95 +130,57 @@ static struct notifier_block twl6030_irq_pm_notifier_block = {
>  };
>  
>  /*
> - * This thread processes interrupts reported by the Primary Interrupt Handler.
> - */
> -static int twl6030_irq_thread(void *data)
> +* Threaded irq handler for the twl6030 interrupt.
> +* We query the interrupt controller in the twl6030 to determine
> +* which module is generating the interrupt request and call
> +* handle_nested_irq for that module.
> +*/
> +static irqreturn_t twl6030_irq_thread(int irq, void *data)
>  {
> -	long irq = (long)data;
> -	static unsigned i2c_errors;
> -	static const unsigned max_i2c_errors = 100;
> -	int ret;
> -
> -	while (!kthread_should_stop()) {
> -		int i;
> -		union {
> +	int i, ret;
> +	union {
>  		u8 bytes[4];
>  		u32 int_sts;
> -		} sts;
> -
> -		/* Wait for IRQ, then read PIH irq status (also blocking) */
> -		wait_for_completion_interruptible(&irq_event);
> -
> -		/* read INT_STS_A, B and C in one shot using a burst read */
> -		ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes,
> -				REG_INT_STS_A, 3);
> -		if (ret) {
> -			pr_warning("twl6030: I2C error %d reading PIH ISR\n",
> -					ret);
> -			if (++i2c_errors >= max_i2c_errors) {
> -				printk(KERN_ERR "Maximum I2C error count"
> -						" exceeded.  Terminating %s.\n",
> -						__func__);
> -				break;
> -			}
> -			complete(&irq_event);
> -			continue;
> -		}
> -
> +	} sts;
>  
> +	/* read INT_STS_A, B and C in one shot using a burst read */
> +	ret = twl_i2c_read(TWL_MODULE_PIH, sts.bytes, REG_INT_STS_A, 3);
> +	if (ret) {
> +		pr_warn("%s: I2C error %d reading PIH ISR\n", __func__, ret);

Does the user really care which function we're returning from.

Would it be better if you replace '__func__' with the device name?

> +		return IRQ_HANDLED;
> +	}
>  
> -		sts.bytes[3] = 0; /* Only 24 bits are valid*/
> +	sts.bytes[3] = 0; /* Only 24 bits are valid*/
>  
> -		/*
> -		 * Since VBUS status bit is not reliable for VBUS disconnect
> -		 * use CHARGER VBUS detection status bit instead.
> -		 */
> -		if (sts.bytes[2] & 0x10)
> -			sts.bytes[2] |= 0x08;
> +	/*
> +	 * Since VBUS status bit is not reliable for VBUS disconnect
> +	 * use CHARGER VBUS detection status bit instead.
> +	 */
> +	if (sts.bytes[2] & 0x10)
> +		sts.bytes[2] |= 0x08;
>  
> -		for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++) {
> -			local_irq_disable();
> -			if (sts.int_sts & 0x1) {
> -				int module_irq = twl6030_irq_base +
> +	for (i = 0; sts.int_sts; sts.int_sts >>= 1, i++)
> +		if (sts.int_sts & 0x1) {

I'm a little confused by this. Where does sts.int_sts come from?

> +			int module_irq = twl6030_irq_base +
>  					twl6030_interrupt_mapping[i];
> -				generic_handle_irq(module_irq);
> -
> -			}
> -		local_irq_enable();
> +			handle_nested_irq(module_irq);
> +			pr_debug("%s: PIH ISR %u, virq%u\n",
> +				 __func__, i, module_irq);
>  		}
>  
> -		/*
> -		 * NOTE:
> -		 * Simulation confirms that documentation is wrong w.r.t the
> -		 * interrupt status clear operation. A single *byte* write to
> -		 * any one of STS_A to STS_C register results in all three
> -		 * STS registers being reset. Since it does not matter which
> -		 * value is written, all three registers are cleared on a
> -		 * single byte write, so we just use 0x0 to clear.
> -		 */
> -		ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
> -		if (ret)
> -			pr_warning("twl6030: I2C error in clearing PIH ISR\n");
> -
> -		enable_irq(irq);
> -	}
> -
> -	return 0;
> -}
> +	/*
> +	 * NOTE:
> +	 * Simulation confirms that documentation is wrong w.r.t the
> +	 * interrupt status clear operation. A single *byte* write to
> +	 * any one of STS_A to STS_C register results in all three
> +	 * STS registers being reset. Since it does not matter which
> +	 * value is written, all three registers are cleared on a
> +	 * single byte write, so we just use 0x0 to clear.
> +	 */
> +	ret = twl_i2c_write_u8(TWL_MODULE_PIH, 0x00, REG_INT_STS_A);
> +	if (ret)
> +		pr_warn("twl6030: I2C error in clearing PIH ISR\n");
>  
> -/*
> - * handle_twl6030_int() is the desc->handle method for the twl6030 interrupt.
> - * This is a chained interrupt, so there is no desc->action method for it.
> - * Now we need to query the interrupt controller in the twl6030 to determine
> - * which module is generating the interrupt request.  However, we can't do i2c
> - * transactions in interrupt context, so we must defer that work to a kernel
> - * thread.  All we do here is acknowledge and mask the interrupt and wakeup
> - * the kernel thread.
> - */
> -static irqreturn_t handle_twl6030_pih(int irq, void *devid)
> -{
> -	disable_irq_nosync(irq);
> -	complete(devid);
>  	return IRQ_HANDLED;
>  }
>  
> @@ -351,7 +312,6 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  {
>  	struct			device_node *node = dev->of_node;
>  	int			nr_irqs, irq_base, irq_end;
> -	struct task_struct	*task;
>  	static struct irq_chip  twl6030_irq_chip;
>  	int			status = 0;
>  	int			i;
> @@ -396,36 +356,25 @@ int twl6030_init_irq(struct device *dev, int irq_num)
>  		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);
>  	}
>  
> -	dev_info(dev, "PIH (irq %d) chaining IRQs %d..%d\n",
> -			irq_num, irq_base, irq_end);
> +	dev_info(dev, "PIH (irq %d) nested IRQs %d..%d\n",
> +		 irq_num, irq_base, irq_end);
>  
>  	/* install an irq handler to demultiplex the TWL6030 interrupt */
> -	init_completion(&irq_event);
> -
> -	status = request_irq(irq_num, handle_twl6030_pih, 0, "TWL6030-PIH",
> -			     &irq_event);
> +	status = request_threaded_irq(irq_num, NULL, twl6030_irq_thread,
> +				      IRQF_ONESHOT, "TWL6030-PIH", NULL);
>  	if (status < 0) {
>  		dev_err(dev, "could not claim irq %d: %d\n", irq_num, status);
>  		goto fail_irq;
>  	}
>  
> -	task = kthread_run(twl6030_irq_thread, (void *)irq_num, "twl6030-irq");
> -	if (IS_ERR(task)) {
> -		dev_err(dev, "could not create irq %d thread!\n", irq_num);
> -		status = PTR_ERR(task);
> -		goto fail_kthread;
> -	}
> -
>  	twl_irq = irq_num;
>  	register_pm_notifier(&twl6030_irq_pm_notifier_block);
>  	return irq_base;
>  
> -fail_kthread:
> -	free_irq(irq_num, &irq_event);
> -
>  fail_irq:
>  	for (i = irq_base; i < irq_end; i++)
>  		irq_set_chip_and_handler(i, NULL, NULL);
> @@ -437,10 +386,13 @@ int twl6030_exit_irq(void)
>  {
>  	unregister_pm_notifier(&twl6030_irq_pm_notifier_block);
>  
> -	if (twl6030_irq_base) {
> +	if (!twl6030_irq_base) {
>  		pr_err("twl6030: can't yet clean up IRQs?\n");
>  		return -ENOSYS;
>  	}
> +
> +	free_irq(twl_irq, NULL);
> +

If request_threaded_irq() fails, isn't there a chance that
twl6030_irq_base will be allocated, but twl_irq will still be
undefined?

>  	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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists