Re: [PATCH v11 08/18] HID: asus: add support to force feedback

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 26 Oct 2026, Sakari Ailus wrote:

> Hi Denis,
> 
> I tested this on Asus T100TA, it seems to work fine.
> 
> Please see my other comments below.
> 
> On Sun, Oct 25, 2026 at 11:52:21PM +0000, Denis Benato wrote:
> > Unlike ROG ally the X version and following ones uses DInput protocol
> > and the force feedback needs to be implemented as its protocol is
> > vendor-specific, therefore add support for FF_RUMBLE with magnitude
> > scaling on a work-queue based approach to avoid using possibly
> > sleeping calls in atomic context.
> > 
> > Signed-off-by: Denis Benato <denis.benato@linux.dev>
> > Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
> > Signed-off-by: Luke Jones <luke@ljones.dev>
> > ---
> >  drivers/hid/Kconfig    |   1 +
> >  drivers/hid/hid-asus.c | 321 +++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 297 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 2481bc01c435..042e3bb0d817 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -189,6 +189,7 @@ config HID_ASUS
> >  	depends on USB_HID
> >  	depends on LEDS_CLASS
> >  	depends on ASUS_WMI || ASUS_WMI=n
> > +        select INPUT_FF_MEMLESS
> 
> Tabs, please.
> 
> >  	select POWER_SUPPLY
> >  	help
> >  	Support for Asus notebook built-in keyboard and touchpad via i2c, and
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index e1883ee20413..a3a21a1493aa 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -244,6 +244,23 @@ struct ally_config {
> >   */
> >  static struct ally_config *ally_config;
> >  
> > +/* XInput force-feedback report (output report 0x0d, gamepad interface) */
> > +struct ff_data {
> > +	u8 enable;
> > +	u8 magnitude_left;
> > +	u8 magnitude_right;
> > +	u8 magnitude_strong;
> > +	u8 magnitude_weak;
> > +	u8 pulse_sustain_10ms;
> > +	u8 pulse_release_10ms;
> > +	u8 loop_count;
> > +} __packed;
> > +
> > +struct ff_report {
> > +	u8 report_id;
> > +	struct ff_data ff;
> > +} __packed;
> > +
> >  struct ally_handheld {
> >  	/* All read/write to IN interfaces must lock */
> >  	struct mutex intf_mutex;
> > @@ -269,6 +286,13 @@ struct ally_handheld {
> >  	bool ally_x_probe_in_progress;
> >  	bool keyboard_probe_in_progress;
> >  
> > +	struct ff_report ff_packet;
> > +	struct work_struct ff_work;
> > +	/* Serializes ff_packet and update_ff between play_effect and ff_work */
> > +	spinlock_t ff_lock;
> > +	bool ff_work_initialized;
> > +	bool update_ff;
> 
> Arranging these with the rest of the bools would save some memory.
> 
> > +
> >  	struct hid_device *keyboard_hdev;
> >  	struct input_dev *keyboard_input;
> >  
> > @@ -384,10 +408,15 @@ enum ally_command_codes {
> >  	CMD_SET_ANTI_DEADZONE           = 0x18,
> >  };
> >  
> > +/* XInput rumble magnitudes use the hardware's 0..100 intensity range. */
> > +#define ALLY_FF_MAX_INTENSITY 100
> > +
> >  static const u8 ALLY_FORCE_FEEDBACK_OFF[] = {
> >  	0x0D, 0x0F, 0x00, 0x00, 0x00, 0x00, 0xFF, 0x00, 0xEB
> >  };
> >  
> > +static_assert(sizeof(struct ff_report) == sizeof(ALLY_FORCE_FEEDBACK_OFF));
> > +
> >  /*
> >   * The ROG Ally device presents multiple USB interfaces (keyboard, mouse, gamepad,
> >   * and custom configuration interface) that bind to the same module. Since only
> > @@ -395,6 +424,7 @@ static const u8 ALLY_FORCE_FEEDBACK_OFF[] = {
> >   * ally_handheld structure to share state across these separate HID interfaces.
> >   */
> >  static void ally_resume_work_fn(struct work_struct *work);
> > +static void ally_x_ff_work_fn(struct work_struct *work);
> >  
> >  /*
> >   * Changes to ally_drvdata must lock: the raw_event callbacks, which may
> > @@ -410,6 +440,13 @@ static struct ally_handheld ally_drvdata = {
> >  	 */
> >  	.resume_work = __DELAYED_WORK_INITIALIZER(ally_drvdata.resume_work,
> >  						  ally_resume_work_fn, 0),
> > +	/*
> > +	 * Initialised statically for the same reason as resume_work: remove()
> > +	 * drains the work whichever of the interfaces probed or failed to
> > +	 * probe, so it must be safe to cancel unconditionally.
> > +	 */
> > +	.ff_work = __WORK_INITIALIZER(ally_drvdata.ff_work, ally_x_ff_work_fn),
> > +	.ff_lock = __SPIN_LOCK_UNLOCKED(ally_drvdata.ff_lock),
> >  };
> >  
> >  /*
> > @@ -2425,7 +2462,8 @@ static void ally_config_remove(struct hid_device *hdev, struct ally_config *cfg)
> >  	 * create them, so a device without those capabilities does not
> >  	 * trigger a "not found" warning.
> >  	 */
> > -	if (cfg->user_cal_support || cfg->anti_deadzone_support) {
> > +	if (cfg->user_cal_support || cfg->anti_deadzone_support ||
> > +	    cfg->resp_curve_support) {
> >  		for (i = 0; i < ARRAY_SIZE(ally_cal_attr_groups); i++)
> >  			sysfs_remove_group(&hdev->dev.kobj,
> >  					   ally_cal_attr_groups[i]);
> > @@ -2611,6 +2649,106 @@ static void ally_x_input_close(struct input_dev *dev)
> >  	hid_hw_close(input_get_drvdata(dev));
> >  }
> >  
> > +/**
> > + * ally_x_send_ff_report() - Send a force-feedback report to the gamepad
> > + * @hdev: HID device
> > + * @buf: buffer containing the report to send
> > + * @len: length of the report
> > + *
> > + * The gamepad interface consumes force-feedback packets as output reports,
> > + * unlike the config interface which expects feature reports: a rumble packet
> > + * sent as HID_REQ_SET_REPORT would be rejected by the hardware.
> > + *
> > + * The caller must hold ally->intf_mutex, so that a send cannot race with
> > + * the gamepad interface being unbound and its transport being stopped.
> > + *
> > + * Return: count of data transferred, negative if error
> > + */
> > +static int ally_x_send_ff_report(struct hid_device *hdev,
> > +				 const u8 *buf, size_t len)
> > +{
> > +	u8 *dmabuf __free(kfree) = kmemdup(buf, len, GFP_KERNEL);
> 
> It's not very nice to assign variables and then check for errors. I'd thus
> separate the declaration and the assignment.

I actually thought the same thing first.

But then stopped and realized, it's mid-function declaration of a cleanup 
variable exactly following the guidelines (the long comment in cleanup.h).

So this function doesn't really have variable declaration block at all, 
making the mid-function variable declaration look like one. I understand 
it looks confusing when encountering this for the first time, it was for 
me as well.

> > +	if (!dmabuf)
> > +		return -ENOMEM;
> > +
> > +	return hid_hw_output_report(hdev, dmabuf, len);
> > +}

-- 
 i.