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:

quoted

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

quoted

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

quoted

+}
+
 /*** 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