Re: [PATCH 2/2] mfd: pcf50633 - use threaded interrupts

[Samuel Ortiz]

Hi Dmitry,

On Mon, Nov 09, 2009 at 01:25:21AM -0800, Dmitry Torokhov wrote:
> Switch to using theraded IRQs instead of implementing custom solution
> with regular inettrup dandler and a custom workqueue.
Same as the first patch, this one contains both a fairly intrusibe change
along with comment and whitespace cleanups. I'd appreciate if you could remove
those cleanup parts from this patch.

Also, Balaji could you please give this patch a try before I apply it ?

Cheers,
Samuel.


> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxx>
> ---
> 
>  drivers/mfd/pcf50633-core.c       |   87 +++++++++++--------------------------
>  include/linux/mfd/pcf50633/core.h |    6 +--
>  2 files changed, 29 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
> index a3d0226..4aeefa2 100644
> --- a/drivers/mfd/pcf50633-core.c
> +++ b/drivers/mfd/pcf50633-core.c
> @@ -126,7 +126,6 @@ int pcf50633_reg_set_bit_mask(struct pcf50633 *pcf, u8 reg, u8 mask, u8 val)
>  
>  out:
>  	mutex_unlock(&pcf->lock);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(pcf50633_reg_set_bit_mask);
> @@ -324,13 +323,13 @@ static void pcf50633_irq_call_handler(struct pcf50633 *pcf, int irq)
>  /* Maximum amount of time ONKEY is held before emergency action is taken */
>  #define PCF50633_ONKEY1S_TIMEOUT 8
>  
> -static void pcf50633_irq_worker(struct work_struct *work)
> +static irqreturn_t pcf50633_irq(int irq, void *data)
>  {
> -	struct pcf50633 *pcf;
> +	struct pcf50633 *pcf = data;
>  	int ret, i, j;
>  	u8 pcf_int[5], chgstat;
>  
> -	pcf = container_of(work, struct pcf50633, irq_work);
> +	dev_dbg(pcf->dev, "pcf50633_irq\n");
>  
>  	/* Read the 5 INT regs in one transaction */
>  	ret = pcf50633_read_block(pcf, PCF50633_REG_INT1,
> @@ -345,8 +344,10 @@ static void pcf50633_irq_worker(struct work_struct *work)
>  		goto out;
>  	}
>  
> -	/* We immediately read the usb and adapter status. We thus make sure
> -	 * only of USBINS/USBREM IRQ handlers are called */
> +	/*
> +	 * We immediately read the usb and adapter status. We thus make sure
> +	 * only of USBINS/USBREM IRQ handlers are called
> +	 */
>  	if (pcf_int[0] & (PCF50633_INT1_USBINS | PCF50633_INT1_USBREM)) {
>  		chgstat = pcf50633_reg_read(pcf, PCF50633_REG_MBCS2);
>  		if (chgstat & (0x3 << 4))
> @@ -364,15 +365,17 @@ static void pcf50633_irq_worker(struct work_struct *work)
>  			pcf_int[0] &= ~(1 << PCF50633_INT1_ADPINS);
>  	}
>  
> -	dev_dbg(pcf->dev, "INT1=0x%02x INT2=0x%02x INT3=0x%02x "
> -			"INT4=0x%02x INT5=0x%02x\n", pcf_int[0],
> -			pcf_int[1], pcf_int[2], pcf_int[3], pcf_int[4]);
> +	dev_dbg(pcf->dev,
> +		"INT1=0x%02x INT2=0x%02x INT3=0x%02x INT4=0x%02x INT5=0x%02x\n",
> +		pcf_int[0], pcf_int[1], pcf_int[2], pcf_int[3], pcf_int[4]);
>  
> -	/* Some revisions of the chip don't have a 8s standby mode on
> -	 * ONKEY1S press. We try to manually do it in such cases. */
> +	/*
> +	 * Some revisions of the chip don't have a 8s standby mode on
> +	 * ONKEY1S press. We try to manually do it in such cases.
> +	 */
>  	if ((pcf_int[0] & PCF50633_INT1_SECOND) && pcf->onkey1s_held) {
> -		dev_info(pcf->dev, "ONKEY1S held for %d secs\n",
> -							pcf->onkey1s_held);
> +		dev_info(pcf->dev,
> +			 "ONKEY1S held for %d secs\n", pcf->onkey1s_held);
>  		if (pcf->onkey1s_held++ == PCF50633_ONKEY1S_TIMEOUT)
>  			if (pcf->pdata->force_shutdown)
>  				pcf->pdata->force_shutdown(pcf);
> @@ -409,8 +412,8 @@ static void pcf50633_irq_worker(struct work_struct *work)
>  	}
>  
>  	/* Have we just resumed ? */
> -	if (pcf->is_suspended) {
> -		pcf->is_suspended = 0;
> +	if (unlikely(pcf->is_suspended)) {
> +		pcf->is_suspended = false;
>  
>  		/* Set the resume reason filtering out non resumers */
>  		for (i = 0; i < ARRAY_SIZE(pcf_int); i++)
> @@ -432,20 +435,6 @@ static void pcf50633_irq_worker(struct work_struct *work)
>  	}
>  
>  out:
> -	put_device(pcf->dev);
> -	enable_irq(pcf->irq);
> -}
> -
> -static irqreturn_t pcf50633_irq(int irq, void *data)
> -{
> -	struct pcf50633 *pcf = data;
> -
> -	dev_dbg(pcf->dev, "pcf50633_irq\n");
> -
> -	get_device(pcf->dev);
> -	disable_irq_nosync(pcf->irq);
> -	queue_work(pcf->work_queue, &pcf->irq_work);
> -
>  	return IRQ_HANDLED;
>  }
>  
> @@ -538,13 +527,9 @@ static int pcf50633_suspend(struct device *dev, pm_message_t state)
>  
>  	pcf = dev_get_drvdata(dev);
>  
> -	/* Make sure our interrupt handlers are not called
> -	 * henceforth */
> +	/* Make sure our interrupt handlers are not called henceforth. */
>  	disable_irq(pcf->irq);
>  
> -	/* Make sure that any running IRQ worker has quit */
> -	cancel_work_sync(&pcf->irq_work);
> -
>  	/* Save the masks */
>  	ret = pcf50633_read_block(pcf, PCF50633_REG_INT1M,
>  				ARRAY_SIZE(pcf->suspend_irq_masks),
> @@ -565,7 +550,7 @@ static int pcf50633_suspend(struct device *dev, pm_message_t state)
>  		goto out;
>  	}
>  
> -	pcf->is_suspended = 1;
> +	pcf->is_suspended = true;
>  
>  out:
>  	return ret;
> @@ -573,11 +558,9 @@ out:
>  
>  static int pcf50633_resume(struct device *dev)
>  {
> -	struct pcf50633 *pcf;
> +	struct pcf50633 *pcf = dev_get_drvdata(dev);
>  	int ret;
>  
> -	pcf = dev_get_drvdata(dev);
> -
>  	/* Write the saved mask registers */
>  	ret = pcf50633_write_block(pcf, PCF50633_REG_INT1M,
>  				ARRAY_SIZE(pcf->suspend_irq_masks),
> @@ -585,16 +568,11 @@ static int pcf50633_resume(struct device *dev)
>  	if (ret < 0)
>  		dev_err(pcf->dev, "Error restoring saved suspend masks\n");
>  
> -	/* Restore regulators' state */
> -
> -
> -	get_device(pcf->dev);
> -
>  	/*
>  	 * Clear any pending interrupts and set resume reason if any.
> -	 * This will leave with enable_irq()
>  	 */
> -	pcf50633_irq_worker(&pcf->irq_work);
> +	pcf50633_irq(pcf->irq, pcf);
> +	enable_irq(pcf->irq);
>  
>  	return 0;
>  }
> @@ -628,21 +606,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	pcf->i2c_client = client;
>  	pcf->irq = client->irq;
>  
> -	pcf->work_queue = create_singlethread_workqueue("pcf50633");
> -	if (!pcf->work_queue) {
> -		dev_err(pcf->dev, "Failed to create workqueue\n");
> -		error = -ENOMEM;
> -		goto err_free_mem;
> -	}
> -
> -	INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
> -
>  	version = pcf50633_reg_read(pcf, 0);
>  	variant = pcf50633_reg_read(pcf, 1);
>  	if (version < 0 || variant < 0) {
>  		dev_err(pcf->dev, "Unable to probe pcf50633\n");
>  		error = -ENODEV;
> -		goto err_destroy_wq;
> +		goto err_free_mem;
>  	}
>  
>  	dev_info(pcf->dev, "Probed device version %d variant %d\n",
> @@ -682,8 +651,9 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  			goto err_free_pdevs;
>  	}
>  
> -	error = request_irq(client->irq, pcf50633_irq,
> -			    IRQF_TRIGGER_LOW, "pcf50633", pcf);
> +	error = request_threaded_irq(client->irq, NULL, pcf50633_irq,
> +				     IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +				     "pcf50633", pcf);
>  	if (error) {
>  		dev_err(pcf->dev, "Failed to request IRQ %d\n", error);
>  		goto err_free_pdevs;
> @@ -709,8 +679,6 @@ err_free_irq:
>  	free_irq(pcf->irq, pcf);
>  err_free_pdevs:
>  	pcf50633_unregister_sub_devices(pcf);
> -err_destroy_wq:
> -	destroy_workqueue(pcf->work_queue);
>  err_free_mem:
>  	kfree(pcf);
>  	return error;
> @@ -722,7 +690,6 @@ static int __devexit pcf50633_remove(struct i2c_client *client)
>  
>  
>  	free_irq(pcf->irq, pcf);
> -	destroy_workqueue(pcf->work_queue);
>  
>  	pcf50633_unregister_sub_devices(pcf);
>  
> diff --git a/include/linux/mfd/pcf50633/core.h b/include/linux/mfd/pcf50633/core.h
> index 0c21222..04990c4 100644
> --- a/include/linux/mfd/pcf50633/core.h
> +++ b/include/linux/mfd/pcf50633/core.h
> @@ -13,8 +13,8 @@
>  #ifndef __LINUX_MFD_PCF50633_CORE_H
>  #define __LINUX_MFD_PCF50633_CORE_H
>  
> +#include <linux/types.h>
>  #include <linux/i2c.h>
> -#include <linux/workqueue.h>
>  #include <linux/regulator/driver.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/power_supply.h>
> @@ -141,15 +141,13 @@ struct pcf50633 {
>  	struct pcf50633_platform_data *pdata;
>  	int irq;
>  	struct pcf50633_irq irq_handler[PCF50633_NUM_IRQ];
> -	struct work_struct irq_work;
> -	struct workqueue_struct *work_queue;
>  	struct mutex lock;
>  
>  	u8 mask_regs[5];
>  
>  	u8 suspend_irq_masks[5];
>  	u8 resume_reason[5];
> -	int is_suspended;
> +	bool is_suspended;
>  
>  	int onkey1s_held;
>  
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
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/