Re: [PATCH v11 01/18] HID: asus: add support for ROG Ally handhelds

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

[-- Attachment #1: Type: text/plain, Size: 3932 bytes --]

On Fri, 30 Oct 2026, Denis Benato wrote:

> 
> On 10/26/26 13:38, Ilpo Järvinen wrote:
> > On Sun, 25 Oct 2026, Denis Benato wrote:
> >
> >> Implement the core functionality for ASUS handhelds:
> >> - extend functionality of hid-asus if driver is enabled
> >> - initialise the device to a "ready" state
> >> - implement Ally X input mapping
> >> - emit a single proper event for AC button long press
> >> - reject probes from a different controller and from duplicated
> >>   interfaces of the same one, which could evict the shared state
> >>
> >> Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
> >> Signed-off-by: Denis Benato <denis.benato@linux.dev>
> >> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
> >> Signed-off-by: Luke Jones <luke@ljones.dev>
> >> Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
> > Shouldn't you be using some Co-developed-by tags too?

> >> +/**
> >> + * ally_gamepad_check_ready() - Wait for the gamepad MCU to report ready
> >> + * @ally: ally handheld structure
> >> + * @hdev: HID device
> >> + *
> >> + * This should be called before any remapping attempts, and on driver
> >> + * init/resume, after the asus handshake has been performed on the
> >> + * configuration endpoint.
> >> + *
> >> + * Return: 0 on success, negative errno on failure
> >> + */
> >> +static int ally_gamepad_check_ready(struct ally_handheld *ally, struct hid_device *hdev)
> >> +{
> >> +	u8 payload[] = { 0x00 };
> > = {} is enough to initialize.
> >
> >> +	int ret;
> >> +
> >> +	for (int i = 0; i < HID_ALLY_READY_MAX_TRIES; i++) {
> >> +		u8 *buf __free(kfree) = ally_alloc_cmd(CMD_CHECK_READY, payload, sizeof(payload));
> >> +		if (!buf)
> >> +			return -ENOMEM;
> >> +
> >> +		ret = ally_gamepad_send_receive_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
> > ally_gamepad_send_packet()'s buf is const so why you need to alloc it over 
> > and over again?
> >
> > Arguably, I'm not sure if this retry loop is expected to execute many 
> > times.
> 
> no the used buffer is u8 *buf (non const) and it gets overwritten by the method
> with the MCU response.

Ah, yes the buf arg seems not const now that I double check. I must have 
accidently searched for some other function name which inputed const buf 
(e.g. old function name in the copy-paste buffer for some reason). I'm 
sorry about that.

> Since I am not sure if while not ready it can write other bytes I have opted to
> free it and reallocate it with correct data sice it is what gets sent and also
> what is returned.
> 
> Also yes, that cycle has to run many times to await for the MCU to come back:
> it has caused quite a few troubles in the past.
> 
> >> +		if (ret < 0) {
> >> +			hid_dbg(hdev, "ROG Ally check %d/%d failed: %d\n", i,
> >> +				HID_ALLY_READY_MAX_TRIES, ret);
> >> +			continue;
> >> +		}
> >> +
> >> +		if (buf[2] == CMD_CHECK_READY)
> >> +			return 0;
> >> +
> >> +		usleep_range(1000, 2000);
> >> +	}
> >> +
> >> +	hid_err(hdev, "ROG Ally never responded with a ready\n");
> >> +	return -ENODEV;
> >> +}


> >> +	byte = in_report->buttons[0];
> >> +	input_report_key(input, BTN_A, byte & BIT(0));
> >> +	input_report_key(input, BTN_B, byte & BIT(1));
> >> +	input_report_key(input, BTN_X, byte & BIT(2));
> >> +	input_report_key(input, BTN_Y, byte & BIT(3));
> >> +	input_report_key(input, BTN_TL, byte & BIT(4));
> >> +	input_report_key(input, BTN_TR, byte & BIT(5));
> >> +	input_report_key(input, BTN_SELECT, byte & BIT(6));
> >> +	input_report_key(input, BTN_START, byte & BIT(7));
> >> +
> >> +	byte = in_report->buttons[1];
> >> +	input_report_key(input, BTN_THUMBL, byte & BIT(0));
> >> +	input_report_key(input, BTN_THUMBR, byte & BIT(1));
> >> +	input_report_key(input, BTN_MODE, byte & BIT(2));

BTW, while cutting text I noticed these BIT()s should be named with 
defines.



-- 
 i.