Re: [PATCH v2 2/2] Input: TWL4030-vibra: use dynamic workqueue

[Jari Vanhala <ext-jari.vanhala@nokia.com>]

On Tue, 2010-02-23 at 19:28 +0100, ext Dmitry Torokhov wrote:
> On Tue, Feb 23, 2010 at 04:27:59PM +0200, Jari Vanhala wrote:
> > +static void twl4030_vibra_wq_destroy(struct vibra_info *info)
> > +{
> > +	struct workqueue_struct *wq;
> > +
> > +	spin_lock(&info->lock);
> > +	wq = info->workqueue;
> > +	info->workqueue = NULL;
> > +	spin_unlock(&info->lock);
> 
> No need for this locking, input core provides needed serialiization.

Looks like so. Ok.
> > +
> > +	if (wq) {
> > +		cancel_work_sync(&info->play_work);
> > +		INIT_WORK(&info->play_work, vibra_play_work); /* cleanup */
> > +		destroy_workqueue(wq);
> > +	}
> > +
> > +	if (info->enabled)
> > +		vibra_disable(info);
> > +}
> > +
> > +static void twl4030_vibra_close(struct input_dev *input)
> > +{
> > +	struct vibra_info *info = input_get_drvdata(input);
> > +
> > +	if (!(--info->users))
> > +		twl4030_vibra_wq_destroy(info);
> 
> Your workqueue is per-device (and there is only one vibrator) so no need
> to track users - input core will not call you more ofte than needed for
> single device.

So close isn't directly connected userspace close-calls, more like
input-subsystem closing device.. That makes life easier.

> > +}
> > +
> >  /*** Module ***/
> >  #if CONFIG_PM
> >  static int twl4030_vibra_suspend(struct device *dev)
> > @@ -197,12 +241,8 @@ static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, info);
> >  
> > -	info->workqueue = create_singlethread_workqueue("vibra");
> > -	if (info->workqueue == NULL) {
> > -		dev_err(&pdev->dev, "couldn't create workqueue\n");
> > -		ret = -ENOMEM;
> > -		goto err_kzalloc;
> > -	}
> > +	info->workqueue = NULL;
> > +	info->users = 0;
> >  	INIT_WORK(&info->play_work, vibra_play_work);
> >  	spin_lock_init(&info->lock);
> >  
> > @@ -210,13 +250,15 @@ static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
> >  	if (info->input_dev == NULL) {
> >  		dev_err(&pdev->dev, "couldn't allocate input device\n");
> >  		ret = -ENOMEM;
> > -		goto err_workq;
> > +		goto err_kzalloc;
> >  	}
> >  	input_set_drvdata(info->input_dev, info);
> >  
> >  	info->input_dev->name = "twl4030:vibrator";
> >  	info->input_dev->id.version = 1;
> >  	info->input_dev->dev.parent = pdev->dev.parent;
> > +	info->input_dev->open = twl4030_vibra_open;
> > +	info->input_dev->close = twl4030_vibra_close;
> >  	set_bit(FF_RUMBLE, info->input_dev->ffbit);
> >  
> >  	ret = input_register_device(info->input_dev);
> > @@ -239,8 +281,6 @@ err_ireg:
> >  	info->input_dev = NULL;
> >  err_ialloc:
> >  	input_free_device(info->input_dev);
> > -err_workq:
> > -	destroy_workqueue(info->workqueue);
> >  err_kzalloc:
> >  	kfree(info);
> >  	return ret;
> > @@ -249,17 +289,8 @@ err_kzalloc:
> >  static int __devexit twl4030_vibra_remove(struct platform_device *pdev)
> >  {
> >  	struct vibra_info *info = platform_get_drvdata(pdev);
> > -	struct workqueue_struct *wq;
> > -
> > -	spin_lock(&info->lock);
> > -	wq = info->workqueue;
> > -	info->workqueue = NULL;
> > -	spin_unlock(&info->lock);
> >  
> > -	cancel_work_sync(&info->play_work);
> > -	destroy_workqueue(wq);
> > -	if (info->enabled)
> > -		vibra_disable(info);
> > +	twl4030_vibra_wq_destroy(info);
> 
> If device has not been opened there is no workqueue. If device has been
> opened input core will call close for it. There is no need to try to
> destroy workqueue here.

That input_unregister_device() seems to call it. Ok.

Looks like there was more in this approach than I thought. Maybe it's
better to combine these. I'm a bit busy with other things, so resending
probably takes couple days.

	++Jam