Re: [PATCH v2 1/2] Input: drv260x: Restore configuration after device close

[Maurizio Casciano]

Hi Dmitry,

Thank you for pointing this out in your review [8].  I reviewed the
DRV2604 Rev. C
datasheet and the regmap implementation again, and I am withdrawing this
patch.

The configuration-loss premise raised by the initial automated review
[6] was incorrect.  Section 7.4.1.3, "Operation
With EN Control", of the DRV2604 datasheet [1] explicitly states:

  "When the EN pin is logic low, the device enters the shutdown state,
  which is the lowest power state of the device.  The device registers
  are not reset."

The same section says that a complete reset to the power-up state
requires DEV_RESET in register 0x01.  Section 7.4.1.5 further describes
DEV_RESET as equivalent to power-cycling the device [1].  Therefore,
drv260x_close() deasserting the enable GPIO does not discard the
configuration established at probe, and there is no need for an open
callback to run drv260x_init() again.  The mainline close path can be
seen at [3].

This datasheet applies to the tested tablet: its firmware exposes two
present ACPI devices with HID and modalias DRV2604, at
\_SB_.PCI0.I2C1.VBR0 and \_SB_.PCI0.I2C4.VBR1.  They enumerate as
i2c-DRV2604:00 and i2c-DRV2604:01, and both are bound to the
drv260x-haptics driver.  The TI Linux support page also lists DRV2604
among the devices supported by this mainline driver [2].

Independently, Sashiko's follow-up warning [7] about the proposed
implementation is valid.  drv260x_init() calls regmap_register_patch()
for the selected actuator mode, as shown in the v7.2 source at [4].
As [5] shows, regmap_register_patch() grows map->patch with krealloc(),
copies the supplied sequence after the existing entries, and increments
map->patch_regs.  Calling
drv260x_init() on every input-device open would consequently append the
same register sequence repeatedly and retain that growing allocation
until the regmap is destroyed.  Repeated open/close cycles would thus
cause unbounded kernel-memory growth.  The relevant implementation is
quoted here and linked in full at [5]:

  p = krealloc(map->patch,
               sizeof(struct reg_sequence) *
                       (map->patch_regs + num_regs), GFP_KERNEL);
  ...
  memcpy(p + map->patch_regs, regs, num_regs * sizeof(*regs));
  map->patch_regs += num_regs;

The related concern about haptics->mode being changed by playback only
affected the proposed attempts to call drv260x_init() again during open
or resume.  Once those unnecessary reinitialization calls are removed,
the suspend/resume fix does not depend on re-running mode-specific
initialization.

I have therefore dropped v2 patch 1/2 completely, with no replacement.
The forthcoming v3 contains only the suspend/resume sequencing fix from
v2 patch 2/2.  It disables and drains the work item before suspend,
balances the work state on error paths, restores power and communication
before leaving standby, and does not call drv260x_init().

Technical references:

[1] Texas Instruments, DRV2604 Haptic Driver datasheet, Rev. C,
    sections 7.4.1.3 and 7.4.1.5:
    https://www.ti.com/lit/ds/symlink/drv2604.pdf
[2] Texas Instruments, Linux Driver for DRV260x; supported devices and
    mainline source information:
    https://www.ti.com/tool/DRV260XSW-LINUX
[3] Linux v7.2 drv260x_close(), including standby and EN deassertion:
    https://github.com/torvalds/linux/blob/v7.2/drivers/input/misc/drv260x.c#L260-L274
[4] Linux v7.2 drv260x_init(), including regmap_register_patch() calls:
    https://github.com/torvalds/linux/blob/v7.2/drivers/input/misc/drv260x.c#L315-L379
[5] Linux v7.2 regmap_register_patch() implementation:
    https://github.com/torvalds/linux/blob/v7.2/drivers/base/regmap/regmap.c#L3417-L3473
[6] Sashiko's initial review, including the configuration-loss premise:
    https://lore.kernel.org/linux-input/20260827182855.DE8E91F000E9@xxxxxxxxxxxxxxx/
[7] Sashiko's v2 follow-up identifying repeated patch registration:
    https://lore.kernel.org/linux-input/20260827233020.416711F000E9@xxxxxxxxxxxxxxx/
[8] Dmitry's review rejecting the EN-reset premise:
    https://lore.kernel.org/linux-input/apLAjoZPxp9JJTrl@xxxxxxxxxx/

Thank you, and thanks to Sashiko for identifying the memory-growth issue
in the proposed implementation.

With Best Regards,
Maurizio Casciano