Re: [PATCH v5 2/2] Input: isa1200 - new driver for Imagis ISA1200

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Svyatoslav,

On Mon, Jun 15, 2026 at 09:19:27AM +0300, Svyatoslav Ryhel wrote:
> чт, 28 трав. 2026 р. о 08:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> >
> > вт, 12 трав. 2026 р. о 13:24 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > >
> > > From: Linus Walleij <linusw@kernel.org>
> > >
> > > The ISA1200 is a haptic feedback unit from Imagis Technology using two
> > > motors for haptic feedback in mobile phones. Used in many mobile devices
> > > c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
> > > GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
> > >
> > > The exact datasheet for the ISA1200 is not available; all data was modeled
> > > based on available downstream kernel sources for various devices and
> > > fragments of information scattered across the internet.
> > >
> > > Tested-by: Linus Walleij <linusw@kernel.org> # GT-I9070 Janice
> > > Signed-off-by: Linus Walleij <linusw@kernel.org>
> > > Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  drivers/input/misc/Kconfig   |  12 +
> > >  drivers/input/misc/Makefile  |   1 +
> > >  drivers/input/misc/isa1200.c | 524 +++++++++++++++++++++++++++++++++++
> > >  3 files changed, 537 insertions(+)
> > >  create mode 100644 drivers/input/misc/isa1200.c
> > >
> >
> > Hello Dmitry! Do I need to make any further adjustments to this driver?
> 
> Hello Dmitry! Do I need to make any further adjustments to this
> driver? This driver is hanging in LKML for some time already without
> responds from input maintainer. It is still relevant and I would like
> it to move forward.

There were valid sashiko comments on the patch regarding resetting
"level" to 0 and also potential racing conditions, as well as suggestion
to check number of gpios specified in the device tree.

Please see if the following works for you:

diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
index ff82252a08e1..c61adc4b605c 100644
--- a/drivers/input/misc/isa1200.c
+++ b/drivers/input/misc/isa1200.c
@@ -131,6 +131,7 @@ struct isa1200 {
 	struct work_struct play_work;
 	struct isa1200_config config;
 
+	bool suspended;
 	bool active;
 	int level;
 };
@@ -247,17 +248,21 @@ static void isa1200_stop(struct isa1200 *isa)
 			       isa->supplies);
 
 	isa->active = false;
-	isa->level = 0;
 }
 
 static void isa1200_play_work(struct work_struct *work)
 {
 	struct isa1200 *isa = container_of(work, struct isa1200, play_work);
-
-	if (isa->level)
-		isa1200_start(isa);
-	else
-		isa1200_stop(isa);
+	struct input_dev *input = isa->input;
+
+	scoped_guard(mutex_try, &input->mutex) {
+		if (!isa->suspended) {
+			if (isa->level)
+				isa1200_start(isa);
+			else
+				isa1200_stop(isa);
+		}
+	}
 }
 
 static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
@@ -280,7 +285,8 @@ static int isa1200_vibrator_play_effect(struct input_dev *input, void *data,
 
 	if (isa->level != level) {
 		isa->level = level;
-		schedule_work(&isa->play_work);
+		if (!READ_ONCE(isa->suspended))
+			schedule_work(&isa->play_work);
 	}
 
 	return 0;
@@ -292,6 +298,7 @@ static void isa1200_vibrator_close(struct input_dev *input)
 
 	cancel_work_sync(&isa->play_work);
 	isa1200_stop(isa);
+	isa->level = 0;
 }
 
 static int isa1200_of_probe(struct i2c_client *client)
@@ -331,6 +338,9 @@ static int isa1200_of_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(isa->enable_gpios),
 				     "failed to get enable gpios\n");
 
+	if (isa->enable_gpios && isa->enable_gpios->ndescs > ISA1200_EN_PINS_MAX)
+		return dev_err_probe(dev, -EINVAL, "too many enable gpios\n");
+
 	ldo_node = device_get_named_child_node(dev, "ldo");
 	if (!ldo_node)
 		return dev_err_probe(dev, -ENODEV,
@@ -479,9 +489,9 @@ static int isa1200_suspend(struct device *dev)
 	guard(mutex)(&isa->input->mutex);
 
 	if (input_device_enabled(isa->input)) {
+		WRITE_ONCE(isa->suspended, true);
 		cancel_work_sync(&isa->play_work);
-		if (isa->level)
-			isa1200_stop(isa);
+		isa1200_stop(isa);
 	}
 
 	return 0;
@@ -493,9 +503,11 @@ static int isa1200_resume(struct device *dev)
 
 	guard(mutex)(&isa->input->mutex);
 
-	if (input_device_enabled(isa->input))
+	if (input_device_enabled(isa->input)) {
+		WRITE_ONCE(isa->suspended, false);
 		if (isa->level)
-			isa1200_start(isa);
+			schedule_work(&isa->play_work);
+	}
 
 	return 0;
 }

-- 
Dmitry