fixes for twl6040-vibra

PATCH New H. Nikolaus Schaller 1 participant 5 messages Apr 21, 2016

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

H. Nikolaus Schaller

author

2016-04-21 11:43 UTC #0 lore raw

v2 cover fixes for twl6040-vibra

Changes V2:
* remove 2 patches already accepted
* keep DT memory management patch until we know if there is a better solution in the library
* suggestions by Dmitry Torokhov:
* add cancel_work to make removal of mutex safe
* replace "unregister handler" patch by "do not reparent" patch
* new: add fix for atomic schedule bug

V1: 2016-04-18 21:55:44: There are some small bugs in the twl6040-vibra driver and differences to how twl4030 vibra works.

This patch series addresses them.

Dmitry Torokhov (1):
Input: twl6040-vibra - do not reparent to grandparent

H. Nikolaus Schaller (3):
input: twl6040-vibra: fix DT node memory management
input: twl6040-vibra: remove mutex
input: twl6040-vibra: fix atomic schedule panic

 drivers/input/misc/twl6040-vibra.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

Signature · 1 line

2.7.3

H. Nikolaus Schaller

author

2016-04-21 11:43 UTC #1 lore raw

PATCH v2 1/4 input: twl6040-vibra: fix DT node memory management

commit e7ec014a47e4 ("Input: twl6040-vibra - update for device tree support")

made the separate vibra DT node to a subnode of the twl6040.

It now calls of_find_node_by_name() to locate the "vibra" subnode. This function has a side effect to call of_node_put on() for the twl6040 parent node passed in as a parameter. This causes trouble later on.

Solution: we must call of_node_get() before of_find_node_by_name()

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 1 +
 1 file changed, 1 insertion(+)

drivers/input/misc/twl6040-vibra.c +1 -0

index 0c853c2..ad15498 100644
@@ -257,6 +257,7 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
 	int vddvibr_uV = 0;
 	int error;
 
+	of_node_get(twl6040_core_dev->of_node);
 	twl6040_core_node = of_find_node_by_name(twl6040_core_dev->of_node,
 						 "vibra");
 	if (!twl6040_core_node) {

Signature · 1 line

2.7.3

H. Nikolaus Schaller

author

2016-04-21 11:43 UTC #2 lore raw

PATCH v2 2/4 input: twl6040-vibra: remove mutex

In reply to H. Nikolaus Schaller

The mutex does not seem to be needed. twl4030-vibra doesn't use one either.

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

drivers/input/misc/twl6040-vibra.c +2 ~~-13~~

index ad15498..f7e0d17 100644
@@ -46,7 +46,7 @@ struct vibra_info {
 	struct device *dev;
 	struct input_dev *input_dev;
 	struct work_struct play_work;
-	struct mutex mutex;
+
 	int irq;
 
 	bool enabled;

@@ -182,8 +182,6 @@ static void vibra_play_work(struct work_struct *work)
 	struct vibra_info *info = container_of(work,
 				struct vibra_info, play_work);
 
-	mutex_lock(&info->mutex);
-
 	if (info->weak_speed || info->strong_speed) {
 		if (!info->enabled)
 			twl6040_vibra_enable(info);

@@ -192,7 +190,6 @@ static void vibra_play_work(struct work_struct *work)
 	} else if (info->enabled)
 		twl6040_vibra_disable(info);
 
-	mutex_unlock(&info->mutex);
 }
 
 static int vibra_play(struct input_dev *input, void *data,

@@ -223,12 +220,8 @@ static void twl6040_vibra_close(struct input_dev *input)
 
 	cancel_work_sync(&info->play_work);
 
-	mutex_lock(&info->mutex);
-
 	if (info->enabled)
 		twl6040_vibra_disable(info);
-
-	mutex_unlock(&info->mutex);
 }
 
 static int __maybe_unused twl6040_vibra_suspend(struct device *dev)

@@ -236,13 +229,11 @@ static int __maybe_unused twl6040_vibra_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct vibra_info *info = platform_get_drvdata(pdev);
 
-	mutex_lock(&info->mutex);
+	cancel_work_sync(&info->play_work);
 
 	if (info->enabled)
 		twl6040_vibra_disable(info);
 
-	mutex_unlock(&info->mutex);
-
 	return 0;
 }

@@ -301,8 +292,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	mutex_init(&info->mutex);
-
 	error = devm_request_threaded_irq(&pdev->dev, info->irq, NULL,
 					  twl6040_vib_irq_handler,
 					  IRQF_ONESHOT,

Signature · 1 line

2.7.3

H. Nikolaus Schaller

author

2016-04-21 11:44 UTC #3 lore raw

PATCH v2 3/4 Input: twl6040-vibra - do not reparent to grandparent

In reply to H. Nikolaus Schaller

From: Dmitry Torokhov <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 1 -
 1 file changed, 1 deletion(-)

drivers/input/misc/twl6040-vibra.c +0 -1

index f7e0d17..f4384b7 100644
@@ -347,7 +347,6 @@ static int twl6040_vibra_probe(struct platform_device *pdev)
 
 	info->input_dev->name = "twl6040:vibrator";
 	info->input_dev->id.version = 1;
-	info->input_dev->dev.parent = pdev->dev.parent;
 	info->input_dev->close = twl6040_vibra_close;
 	__set_bit(FF_RUMBLE, info->input_dev->ffbit);

Signature · 1 line

2.7.3

H. Nikolaus Schaller

author

2016-04-21 11:43 UTC #4 lore raw

PATCH v2 4/4 input: twl6040-vibra: fix atomic schedule panic

In reply to H. Nikolaus Schaller

commit c6f39257c952 ("mfd: twl6040: Use regmap for register cache")

did remove the private cache for the vibra control registers and replaced access within twl6040_get_vibralr_status() by calls to regmap. This is ok, as long as twl6040_get_vibralr_status() uses already cached values or is not called from interrupt context. But we call this in vibra_play() for checking that the vibrator is not configured for audio mode.

The result is a "BUG: scheduling while atomic" if the first use of the twl6040 is a vibra effect, because the first fetch is by reading the twl6040 registers through (blocking) i2c and not from the cache.

As soon as the regmap has cached the status, further calls are fine.

The solution is to move the condition to the work() function which runs in context that can block.

The original code returns -EBUSY, but the return value of ->play() functions is ignored anyways. Hence, we do not loose functionality by not returning an error but just reporting the issue to INFO loglevel.

Tested-on: Pyra (omap5) prototype

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/input/misc/twl6040-vibra.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/input/misc/twl6040-vibra.c +8 -8

index f4384b7..5690eb7 100644
@@ -181,6 +181,14 @@ static void vibra_play_work(struct work_struct *work)
 {
 	struct vibra_info *info = container_of(work,
 				struct vibra_info, play_work);
+	int ret;
+
+	/* Do not allow effect, while the routing is set to use audio */
+	ret = twl6040_get_vibralr_status(info->twl6040);
+	if (ret & TWL6040_VIBSEL) {
+		dev_info(info->dev, "Vibra is configured for audio\n");
+		return;
+	}
 
 	if (info->weak_speed || info->strong_speed) {
 		if (!info->enabled)

@@ -196,14 +204,6 @@ static int vibra_play(struct input_dev *input, void *data,
 		      struct ff_effect *effect)
 {
 	struct vibra_info *info = input_get_drvdata(input);
-	int ret;
-
-	/* Do not allow effect, while the routing is set to use audio */
-	ret = twl6040_get_vibralr_status(info->twl6040);
-	if (ret & TWL6040_VIBSEL) {
-		dev_info(&input->dev, "Vibra is configured for audio\n");
-		return -EBUSY;
-	}
 
 	info->weak_speed = effect->u.rumble.weak_magnitude;
 	info->strong_speed = effect->u.rumble.strong_magnitude;

Signature · 1 line

2.7.3