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

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

On Fri, May 08, 2026 at 08:30:19AM +0300, Svyatoslav Ryhel wrote:
> чт, 7 трав. 2026 р. о 22:26 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
> >
> > Hi Svyatoslav,
> >
> > On Thu, May 07, 2026 at 04:39:48PM +0300, Svyatoslav Ryhel wrote:
> > > 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 | 540 +++++++++++++++++++++++++++++++++++
> > >  3 files changed, 553 insertions(+)
> > >  create mode 100644 drivers/input/misc/isa1200.c
> > >
> > > diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> > > index 94a753fcb64f..52f192104ee2 100644
> > > --- a/drivers/input/misc/Kconfig
> > > +++ b/drivers/input/misc/Kconfig
> > > @@ -852,6 +852,18 @@ config INPUT_IQS7222
> > >         To compile this driver as a module, choose M here: the
> > >         module will be called iqs7222.
> > >
> > > +config INPUT_ISA1200_HAPTIC
> > > +     tristate "Imagis ISA1200 haptic feedback unit"
> > > +     depends on I2C
> > > +     select INPUT_FF_MEMLESS
> > > +     select REGMAP_I2C
> > > +     help
> > > +       Say Y to enable support for the Imagis ISA1200 haptic
> > > +       feedback unit.
> > > +
> > > +       To compile this driver as a module, choose M here: the
> > > +       module will be called isa1200.
> > > +
> > >  config INPUT_CMA3000
> > >       tristate "VTI CMA3000 Tri-axis accelerometer"
> > >       help
> > > diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> > > index 415fc4e2918b..d62bf2e9d85f 100644
> > > --- a/drivers/input/misc/Makefile
> > > +++ b/drivers/input/misc/Makefile
> > > @@ -49,6 +49,7 @@ obj-$(CONFIG_INPUT_IMS_PCU)         += ims-pcu.o
> > >  obj-$(CONFIG_INPUT_IQS269A)          += iqs269a.o
> > >  obj-$(CONFIG_INPUT_IQS626A)          += iqs626a.o
> > >  obj-$(CONFIG_INPUT_IQS7222)          += iqs7222.o
> > > +obj-$(CONFIG_INPUT_ISA1200_HAPTIC)   += isa1200.o
> > >  obj-$(CONFIG_INPUT_KEYSPAN_REMOTE)   += keyspan_remote.o
> > >  obj-$(CONFIG_INPUT_KXTJ9)            += kxtj9.o
> > >  obj-$(CONFIG_INPUT_M68K_BEEP)                += m68kspkr.o
> > > diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
> > > new file mode 100644
> > > index 000000000000..f8dba8a95c7d
> > > --- /dev/null
> > > +++ b/drivers/input/misc/isa1200.c
> > > @@ -0,0 +1,540 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +
> > > +#include <linux/array_size.h>
> > > +#include <linux/bitmap.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/devm-helpers.h>
> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/input.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/property.h>
> > > +#include <linux/pwm.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/units.h>
> > > +
> > > +/*
> > > + * System control (LDO regulator)
> > > + *
> > > + * LDO voltage to register mapping is linear, but it is split in two parts:
> > > + * 2.3V - 3.0V map to 0x08 - 0x0f; 3.1V - 3.8V map to 0x00 - 0x7
> > > + */
> > > +
> > > +#define ISA1200_SCTRL                        0x00
> > > +#define ISA1200_LDO_VOLTAGE_BASE     0x08
> > > +#define ISA1200_LDO_VOLTAGE_STEP     100000
> > > +#define ISA1200_LDO_VOLTAGE_2V3              23
> > > +#define ISA1200_LDO_VOLTAGE_3V1              31
> > > +#define ISA1200_LDO_VOLTAGE_MIN              2300000
> > > +#define ISA1200_LDO_VOLTAGE_MAX              3800000
> > > +
> > > +/*
> > > + * The output frequency is calculated with this formula:
> > > + *
> > > + *                 base clock frequency
> > > + * fout = -----------------------------------------
> > > + *        (128 - PWM_FREQ) * 2 * PLLDIV * PWM_PERIOD
> > > + *
> > > + * The base clock frequency is the clock frequency provided on the
> > > + * clock input to the chip, divided by the value in HCTRL0
> > > + *
> > > + * PWM_FREQ is configured in register HCTRL4, it is common to set this
> > > + * to 0 to get only two variables to calculate.
> > > + *
> > > + * PLLDIV is configured in register HCTRL3 (bits 7..4, so 0..15)
> > > + * PWM_PERIOD is configured in register HCTRL6
> > > + * Further the duty cycle can be configured in HCTRL5
> > > + */
> > > +
> > > +/*
> > > + * HCTRL0 configures clock or PWM input and selects the divider for
> > > + * the clock input.
> > > + */
> > > +#define ISA1200_HCTRL0                       0x30
> > > +#define ISA1200_HCTRL0_HAP_ENABLE    BIT(7)
> > > +#define ISA1200_HCTRL0_PWM_GEN_MODE  BIT(4)
> > > +#define ISA1200_HCTRL0_PWM_INPUT_MODE        BIT(3)
> > > +#define ISA1200_HCTRL0_CLKDIV_128    128
> > > +
> > > +/*
> > > + * HCTRL1 configures the motor type and clock sourse
> > > + */
> > > +#define ISA1200_HCTRL1                       0x31
> > > +#define ISA1200_HCTRL1_EXT_CLOCK     BIT(7)
> > > +#define ISA1200_HCTRL1_DAC_INVERT    BIT(6)
> > > +#define ISA1200_HCTRL1_MODE(n)               (((n) & 1) << 5)
> >
> > I wonder if this should simply be BIT(5) and you conditionally use it in
> > the code. The macro is not really usable to disable the setting...
> >
> 
> That was the initial idea but mode is not boolean it is an enum and
> macro fits better to handle enum. Code does not enable/disable this
> field, this field is configured with every start call.

OK.

> 
> > > +
> > > +/* HCTRL2 controls software reset of the chip */
> > > +#define ISA1200_HCTRL2                       0x32
> > > +#define ISA1200_HCTRL2_SW_RESET              BIT(0)
> > > +
> > > +/*
> > > + * HCTRL3 controls the PLL divisor
> > > + *
> > > + * Bits [0,1] are always set to 1 (we don't know what they are
> > > + * used for) and bit 4 and upward control the PLL divisor.
> > > + */
> > > +#define ISA1200_HCTRL3                       0x33
> > > +#define ISA1200_HCTRL3_DEFAULT               0x03
> > > +#define ISA1200_HCTRL3_PLLDIV(n)     (((n) & 0xf) << 4)
> > > +
> > > +/* HCTRL4 controls the PWM frequency of external channel */
> > > +#define ISA1200_HCTRL4                       0x34
> > > +
> > > +/* HCTRL5 controls the PWM high duty cycle of internal channel */
> > > +#define ISA1200_HCTRL5                       0x35
> > > +
> > > +/* HCTRL6 controls the PWM period of internal channel */
> > > +#define ISA1200_HCTRL6                       0x36
> > > +#define ISA1200_HCTRL6_PERIOD_SCALE  100
> > > +
> > > +/* The use for these registers is unknown but they exist */
> > > +#define ISA1200_HCTRL7                       0x37
> > > +#define ISA1200_HCTRL8                       0x38
> > > +#define ISA1200_HCTRL9                       0x39
> > > +#define ISA1200_HCTRLA                       0x3a
> > > +#define ISA1200_HCTRLB                       0x3b
> > > +#define ISA1200_HCTRLC                       0x3c
> > > +#define ISA1200_HCTRLD                       0x3d
> > > +
> > > +#define ISA1200_EN_PINS_MAX          2
> > > +
> > > +struct isa1200_config {
> > > +     u32 ldo_voltage;
> > > +     u32 mode;
> > > +     u32 clkdiv;
> > > +     u32 plldiv;
> > > +     u32 freq;
> > > +     u32 period;
> > > +     u32 duty;
> > > +};
> > > +
> > > +struct isa1200 {
> > > +     struct input_dev *input;
> > > +     struct regmap *map;
> > > +
> > > +     struct clk *clk;
> > > +     struct pwm_device *pwm;
> > > +     struct gpio_descs *enable_gpios;
> > > +
> > > +     struct work_struct play_work;
> > > +     struct isa1200_config config;
> > > +
> > > +     int level;
> > > +     bool clk_on;
> >
> > I think you need not only clk_on, but general "active" flag that you
> > would set at the end of isa1200_start().
> >
> 
> Acknowledged.
> 
> > > +};
> > > +
> > > +static const struct regmap_config isa1200_regmap_config = {
> > > +     .reg_bits = 8,
> > > +     .val_bits = 8,
> > > +     .max_register = ISA1200_HCTRLD,
> > > +};
> > > +
> > > +static void isa1200_start(struct isa1200 *isa)
> > > +{
> > > +     struct isa1200_config *config = &isa->config;
> > > +     struct pwm_state state;
> > > +     u8 hctrl0 = 0, hctrl1 = 0;
> > > +     DECLARE_BITMAP(values, ISA1200_EN_PINS_MAX);
> > > +     int ret;
> >
> > Please use "error" or "err" for all variables that only hold error codes
> > (or 0) instead of a real value that is used for something.
> >
> 
> Not real value, but return value. Why I cannot use ret aka return
> value? It is much more versatile since it can hold any function return
> value including errors.

This is my preference for input. I do not want versatility, I want the
opposite: if I see error I do not need to consider whether it holds
something of value besides an error code. And if I see "ret" or "retval"
I know that caller might be interested the value. And also

	if (error) {
		// handle error
	}

looks neat.

> 
> > > +
> > > +     if (!isa->clk_on) {
> > > +             ret = clk_prepare_enable(isa->clk);
> >
> > This return 0 on success so
> >
> >                 if (error)
> >                         return;
> >
> 
> No, code is correct. If clock enable fails, further function execution
> should stop since regmap operations on unconfigured device is not
> desirable. Since this function is void using general "active" flag as
> you suggested to indicate that start reached end would be beneficial.

I am simply saying that you do not need to check if value is negative,
checking that it is non-zero is sufficient.

Thanks.

-- 
Dmitry