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

[Svyatoslav Ryhel]

вт, 16 черв. 2026 р. о 07:16 Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> пише:
>
> Hi Svyatoslav,
>
> On Mon, Jun 15, 2026 at 09:19:27AM +0300, Svyatoslav Ryhel wrote:
> > чт, 28 трав. 2026 р. о 08:38 Svyatoslav Ryhel <clamor95@xxxxxxxxx> пише:
> > >
> > > вт, 12 трав. 2026 р. о 13:24 Svyatoslav Ryhel <clamor95@xxxxxxxxx> пише:
> > > >
> > > > From: Linus Walleij <linusw@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx> # GT-I9070 Janice
> > > > Signed-off-by: Linus Walleij <linusw@xxxxxxxxxx>
> > > > Co-developed-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > > ---
> > > >  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

I have tested your code on my P895 and it works perfectly fine. Should
I resend with these changes or you can integrate them while picking
patchset?

Thank you for your suggestions and efforts!

Best regards,
Svyatoslav R.