Re: [PATCH v11 11/18] HID: asus: add support for btn remapping

[Denis Benato <denis.benato@linux.dev>]

On 10/26/26 15:03, Ilpo Järvinen wrote:
> On Sun, 25 Oct 2026, Denis Benato wrote:
>
>> Add support for buttons remapping: the ability to assign different
>> button events to each one of the physical button on the device.
>>
>> Each button exposes a remap_index sysfs attribute listing the valid
>> remap target names for the current gamepad mode: M1 and M2 can be
>> remapped to anything at any time, while other buttons may only be
>> remapped to gamepad codes in gamepad mode and keyboard/mouse/media
>> codes in desktop mode.
>>
>> When the gamepad mode is changed the firmware automatically restores
>> its internal defaults, so the software cache is refreshed to reflect
>> the hardware state.
>>
>> Signed-off-by: Luke Jones <luke@ljones.dev>
>> Signed-off-by: Denis Benato <denis.benato@linux.dev>
>> Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
>> ---
>>  drivers/hid/hid-asus.c | 1059 +++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 1037 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
>> index ed95b2cdec81..ea27495c3df6 100644
>> --- a/drivers/hid/hid-asus.c
>> +++ b/drivers/hid/hid-asus.c
>> @@ -275,9 +275,25 @@ struct ally_turbo_config {
>>  	struct ally_btn_turbo_params btn_m1;
>>  };
>>  
>> +/* Button remap attribute structure */
>> +struct button_remap_attr {
>> +	struct device_attribute dev_attr;
>> +	enum ally_button_id button_id;
>> +	bool is_macro;
>> +	/*
>> +	 * The attributes are created while hid_asus_ally_probe() is still
>> +	 * running, before asus_probe() publishes drvdata->rog_ally: carry
>> +	 * the pointer like the turbo entries do, so the callbacks work
>> +	 * from the moment the files exist.
>> +	 */
>> +	struct ally_handheld *ally;
>> +};
>> +
>> +#define to_button_remap_attr(x) container_of(x, struct button_remap_attr, dev_attr)
>> +
>>  struct ally_btn_sysfs_entry {
>>  	struct attribute_group group;
>> -	struct attribute *attrs[5]; /* turbo_period + toggle_period + ranges + NULL */
>> +	struct attribute *attrs[8]; /* turbo + ranges + remap + macro + remap_index + NULL */
>>  	/*
>>  	 * The entries are created while hid_asus_ally_probe() is still running,
>>  	 * before asus_probe() publishes drvdata->rog_ally: carrying the shared
>> @@ -289,6 +305,9 @@ struct ally_btn_sysfs_entry {
>>  	enum ally_button_id btn;
>>  	struct device_attribute attr_turbo_period;
>>  	struct device_attribute attr_toggle_period;
>> +	struct button_remap_attr *remap_attr;
>> +	struct button_remap_attr *macro_attr;
>> +	struct device_attribute attr_remap_index;
>>  };
>>  
>>  enum ally_gamepad_mode_index {
>> @@ -296,6 +315,35 @@ enum ally_gamepad_mode_index {
>>  	ALLY_GAMEPAD_MODE_KEYBOARD = 0x02,
>>  };
>>  
>> +/* Button pair indexes for mapping commands */
>> +enum btn_pair_index {
>> +	BTN_PAIR_DPAD_UPDOWN    = 0x01,
>> +	BTN_PAIR_DPAD_LEFTRIGHT = 0x02,
>> +	BTN_PAIR_STICK_LR       = 0x03,
>> +	BTN_PAIR_BUMPER_LR      = 0x04,
>> +	BTN_PAIR_AB             = 0x05,
>> +	BTN_PAIR_XY             = 0x06,
>> +	BTN_PAIR_VIEW_MENU      = 0x07,
>> +	BTN_PAIR_M1M2           = 0x08,
>> +	BTN_PAIR_TRIGGER_LR     = 0x09,
>> +};
>> +
>> +struct button_map {
>> +	const struct btn_code_map *remap;
>> +	const struct btn_code_map *macro;
>> +};
>> +
>> +struct button_pair_map {
>> +	enum btn_pair_index pair_index;
>> +	struct button_map first;
>> +	struct button_map second;
>> +};
>> +
>> +/* Store button mapping per gamepad mode */
>> +struct ally_button_mapping {
>> +	struct button_pair_map button_pairs[9]; /* 9 button pairs */
>> +};
>> +
>>  struct ally_config {
>>  	/* Must be locked if the data is being changed */
>>  	struct mutex config_mutex;
>> @@ -331,6 +379,8 @@ struct ally_config {
>>  
>>  	struct ally_turbo_config turbo;
>>  	struct ally_btn_sysfs_entry *button_entries;
>> +	/* Button mappings of the current gamepad mode */
>> +	struct ally_button_mapping button_mappings;
>>  
>>  	struct ally_joystick_resp_curve left_curve;
>>  	struct ally_joystick_resp_curve right_curve;
>> @@ -1072,6 +1122,295 @@ static int ally_set_gamepad_mode(struct ally_handheld *ally, struct hid_device *
>>  	return 0;
>>  }
>>  
>> +enum btn_map_type {
>> +	BTN_TYPE_NONE = 0,
>> +	BTN_TYPE_PAD = 0x01,
>> +	BTN_TYPE_KB = 0x02,
>> +	BTN_TYPE_MOUSE = 0x03,
>> +	BTN_TYPE_MEDIA = 0x05,
>> +};
>> +
>> +struct btn_code_map {
>> +	unsigned char type;
>> +	unsigned char value;
>> +	const char *name;
>> +};
>> +
>> +static const struct btn_code_map ally_btn_codes[] = {
>> +	{ BTN_TYPE_NONE, 0x00, "NONE" },
>> +	/* Gamepad button codes */
>> +	{ BTN_TYPE_PAD, 0x01, "BTN_SOUTH" },
>> +	{ BTN_TYPE_PAD, 0x02, "BTN_EAST" },
>> +	{ BTN_TYPE_PAD, 0x03, "BTN_NORTH" },
>> +	{ BTN_TYPE_PAD, 0x04, "BTN_WEST" },
>> +	{ BTN_TYPE_PAD, 0x05, "BTN_TL" },
>> +	{ BTN_TYPE_PAD, 0x06, "BTN_TR" },
>> +	{ BTN_TYPE_PAD, 0x07, "BTN_THUMBL" },
>> +	{ BTN_TYPE_PAD, 0x08, "BTN_THUMBR" },
>> +	{ BTN_TYPE_PAD, 0x09, "ABS_HAT0Y_NEG" },
>> +	{ BTN_TYPE_PAD, 0x0A, "ABS_HAT0Y_POS" },
>> +	{ BTN_TYPE_PAD, 0x0B, "ABS_HAT0X_NEG" },
>> +	{ BTN_TYPE_PAD, 0x0C, "ABS_HAT0X_POS" },
>> +	{ BTN_TYPE_PAD, 0x0D, "ABS_Z" },
>> +	{ BTN_TYPE_PAD, 0x0E, "ABS_RZ" },
>> +	{ BTN_TYPE_PAD, 0x11, "BTN_SELECT" },
>> +	{ BTN_TYPE_PAD, 0x12, "BTN_START" },
>> +	{ BTN_TYPE_PAD, 0x13, "BTN_MODE" },
>> +
>> +	/* Keyboard button codes */
>> +	{ BTN_TYPE_KB, 0x8E, "FN_M2" },
>> +	{ BTN_TYPE_KB, 0x8F, "FN_M1" },
>> +	{ BTN_TYPE_KB, 0x76, "KEY_ESC" },
>> +	{ BTN_TYPE_KB, 0x05, "KEY_F1" },
>> +	{ BTN_TYPE_KB, 0x06, "KEY_F2" },
>> +	{ BTN_TYPE_KB, 0x04, "KEY_F3" },
>> +	{ BTN_TYPE_KB, 0x0C, "KEY_F4" },
>> +	{ BTN_TYPE_KB, 0x03, "KEY_F5" },
>> +	{ BTN_TYPE_KB, 0x0B, "KEY_F6" },
>> +	{ BTN_TYPE_KB, 0x80, "KEY_F7" },
>> +	{ BTN_TYPE_KB, 0x0A, "KEY_F8" },
>> +	{ BTN_TYPE_KB, 0x01, "KEY_F9" },
>> +	{ BTN_TYPE_KB, 0x09, "KEY_F10" },
>> +	{ BTN_TYPE_KB, 0x78, "KEY_F11" },
>> +	{ BTN_TYPE_KB, 0x07, "KEY_F12" },
>> +	{ BTN_TYPE_KB, 0x08, "KEY_F13" },
>> +	{ BTN_TYPE_KB, 0x10, "KEY_F14" },
>> +	{ BTN_TYPE_KB, 0x18, "KEY_F15" },
>> +	{ BTN_TYPE_KB, 0x0E, "KEY_GRAVE" }, // backtick
>> +	{ BTN_TYPE_KB, 0x16, "KEY_1" },
>> +	{ BTN_TYPE_KB, 0x1E, "KEY_2" },
>> +	{ BTN_TYPE_KB, 0x26, "KEY_3" },
>> +	{ BTN_TYPE_KB, 0x25, "KEY_4" },
>> +	{ BTN_TYPE_KB, 0x2E, "KEY_5" },
>> +	{ BTN_TYPE_KB, 0x36, "KEY_6" },
>> +	{ BTN_TYPE_KB, 0x3D, "KEY_7" },
>> +	{ BTN_TYPE_KB, 0x3E, "KEY_8" },
>> +	{ BTN_TYPE_KB, 0x46, "KEY_9" },
>> +	{ BTN_TYPE_KB, 0x45, "KEY_0" },
>> +	{ BTN_TYPE_KB, 0x4E, "KEY_MINUS" }, // hyphen
>> +	{ BTN_TYPE_KB, 0x55, "KEY_EQUAL" },
>> +	{ BTN_TYPE_KB, 0x66, "KEY_BACKSPACE" },
>> +	{ BTN_TYPE_KB, 0x0D, "KEY_TAB" },
>> +	{ BTN_TYPE_KB, 0x15, "KEY_Q" },
>> +	{ BTN_TYPE_KB, 0x1D, "KEY_W" },
>> +	{ BTN_TYPE_KB, 0x24, "KEY_E" },
>> +	{ BTN_TYPE_KB, 0x2D, "KEY_R" },
>> +	{ BTN_TYPE_KB, 0x2C, "KEY_T" },
>> +	{ BTN_TYPE_KB, 0x35, "KEY_Y" },
>> +	{ BTN_TYPE_KB, 0x3C, "KEY_U" },
>> +	{ BTN_TYPE_KB, 0x43, "KEY_I" },
>> +	{ BTN_TYPE_KB, 0x44, "KEY_O" },
>> +	{ BTN_TYPE_KB, 0x4D, "KEY_P" },
>> +	{ BTN_TYPE_KB, 0x54, "KEY_LEFTBRACE" },
>> +	{ BTN_TYPE_KB, 0x5B, "KEY_RIGHTBRACE" },
>> +	{ BTN_TYPE_KB, 0x5D, "KEY_BACKSLASH" },
>> +	{ BTN_TYPE_KB, 0x58, "KEY_CAPSLOCK" },
>> +	{ BTN_TYPE_KB, 0x1C, "KEY_A" },
>> +	{ BTN_TYPE_KB, 0x1B, "KEY_S" },
>> +	{ BTN_TYPE_KB, 0x23, "KEY_D" },
>> +	{ BTN_TYPE_KB, 0x2B, "KEY_F" },
>> +	{ BTN_TYPE_KB, 0x34, "KEY_G" },
>> +	{ BTN_TYPE_KB, 0x33, "KEY_H" },
>> +	{ BTN_TYPE_KB, 0x3B, "KEY_J" },
>> +	{ BTN_TYPE_KB, 0x42, "KEY_K" },
>> +	{ BTN_TYPE_KB, 0x4B, "KEY_L" },
>> +	{ BTN_TYPE_KB, 0x4C, "KEY_SEMICOLON" },
>> +	{ BTN_TYPE_KB, 0x52, "KEY_APOSTROPHE" },
>> +	{ BTN_TYPE_KB, 0x5A, "KEY_ENTER" },
>> +	{ BTN_TYPE_KB, 0x88, "KEY_LEFTSHIFT" },
>> +	{ BTN_TYPE_KB, 0x1A, "KEY_Z" },
>> +	{ BTN_TYPE_KB, 0x22, "KEY_X" },
>> +	{ BTN_TYPE_KB, 0x21, "KEY_C" },
>> +	{ BTN_TYPE_KB, 0x2A, "KEY_V" },
>> +	{ BTN_TYPE_KB, 0x32, "KEY_B" },
>> +	{ BTN_TYPE_KB, 0x31, "KEY_N" },
>> +	{ BTN_TYPE_KB, 0x3A, "KEY_M" },
>> +	{ BTN_TYPE_KB, 0x41, "KEY_COMMA" },
>> +	{ BTN_TYPE_KB, 0x49, "KEY_DOT" },
>> +	{ BTN_TYPE_KB, 0x4A, "KEY_SLASH" },
>> +	{ BTN_TYPE_KB, 0x89, "KEY_RIGHTSHIFT" },
>> +	{ BTN_TYPE_KB, 0x82, "KEY_LEFTMETA" },
>> +	{ BTN_TYPE_KB, 0x8A, "KEY_LEFTALT" },
>> +	{ BTN_TYPE_KB, 0x29, "KEY_SPACE" },
>> +	{ BTN_TYPE_KB, 0x8B, "KEY_RIGHTALT" },
>> +	{ BTN_TYPE_KB, 0x84, "KEY_COMPOSE" },
>> +	{ BTN_TYPE_KB, 0x8D, "KEY_RIGHTCTRL" },
>> +	{ BTN_TYPE_KB, 0xC3, "KEY_SYSRQ" },
>> +	{ BTN_TYPE_KB, 0x7E, "KEY_SCROLLLOCK" },
>> +	{ BTN_TYPE_KB, 0x91, "KEY_PAUSE" },
>> +	{ BTN_TYPE_KB, 0xC2, "KEY_INSERT" },
>> +	{ BTN_TYPE_KB, 0x94, "KEY_HOME" },
>> +	{ BTN_TYPE_KB, 0x96, "KEY_PAGEUP" },
>> +	{ BTN_TYPE_KB, 0xC0, "KEY_DELETE" },
>> +	{ BTN_TYPE_KB, 0x95, "KEY_END" },
>> +	{ BTN_TYPE_KB, 0x97, "KEY_PAGEDOWN" },
>> +	{ BTN_TYPE_KB, 0x98, "KEY_UP" },
>> +	{ BTN_TYPE_KB, 0x99, "KEY_DOWN" },
>> +	{ BTN_TYPE_KB, 0x9A, "KEY_LEFT" },
>> +	{ BTN_TYPE_KB, 0x9B, "KEY_RIGHT" },
>> +
>> +	/* Numpad button codes */
>> +	{ BTN_TYPE_KB, 0x77, "KEY_NUMLOCK" },
>> +	{ BTN_TYPE_KB, 0x90, "KEY_KPSLASH" },
>> +	{ BTN_TYPE_KB, 0x7C, "KEY_KPASTERISK" },
>> +	{ BTN_TYPE_KB, 0x7B, "KEY_KPMINUS" },
>> +	{ BTN_TYPE_KB, 0x70, "KEY_KP0" },
>> +	{ BTN_TYPE_KB, 0x69, "KEY_KP1" },
>> +	{ BTN_TYPE_KB, 0x72, "KEY_KP2" },
>> +	{ BTN_TYPE_KB, 0x7A, "KEY_KP3" },
>> +	{ BTN_TYPE_KB, 0x6B, "KEY_KP4" },
>> +	{ BTN_TYPE_KB, 0x73, "KEY_KP5" },
>> +	{ BTN_TYPE_KB, 0x74, "KEY_KP6" },
>> +	{ BTN_TYPE_KB, 0x6C, "KEY_KP7" },
>> +	{ BTN_TYPE_KB, 0x75, "KEY_KP8" },
>> +	{ BTN_TYPE_KB, 0x7D, "KEY_KP9" },
>> +	{ BTN_TYPE_KB, 0x79, "KEY_KPPLUS" },
>> +	{ BTN_TYPE_KB, 0x81, "KEY_KPENTER" },
>> +	{ BTN_TYPE_KB, 0x71, "KEY_KPDOT" },
>> +
>> +	/* Mouse button codes */
>> +	{ BTN_TYPE_MOUSE, 0x01, "BTN_LEFT" },
>> +	{ BTN_TYPE_MOUSE, 0x02, "BTN_RIGHT" },
>> +	{ BTN_TYPE_MOUSE, 0x03, "BTN_MIDDLE" },
>> +	{ BTN_TYPE_MOUSE, 0x04, "REL_WHEEL_HI_RES_UP" },
>> +	{ BTN_TYPE_MOUSE, 0x05, "REL_WHEEL_HI_RES_DOWN" },
>> +
>> +	/* Media button codes */
>> +	{ BTN_TYPE_MEDIA, 0x16, "MEDIA_SCREENSHOT" },
>> +	{ BTN_TYPE_MEDIA, 0x19, "MEDIA_SHOW_KEYBOARD" },
>> +	{ BTN_TYPE_MEDIA, 0x1C, "MEDIA_SHOW_DESKTOP" },
>> +	{ BTN_TYPE_MEDIA, 0x1E, "MEDIA_START_RECORDING" },
>> +	{ BTN_TYPE_MEDIA, 0x01, "MEDIA_MIC_OFF" },
>> +	{ BTN_TYPE_MEDIA, 0x02, "MEDIA_VOL_DOWN" },
>> +	{ BTN_TYPE_MEDIA, 0x03, "MEDIA_VOL_UP" },
>> +};
>> +
>> +static const size_t keymap_len = ARRAY_SIZE(ally_btn_codes);
> Why is this a variable???? Can't the users just directly use ARRAY_SIZE on 
> the array????

Not understood: you say variable, but both are const(s), isn't const
the exact opposite of variable?

I am stepping away for today, but I have cleaned up much of this
mess.

> At v11 it's odd to find this much garbage still remaining... :-(

At a certain (late) point of the development I noticed that remapping
buttons on the X applied immediately, while on my Ally 1 it had
to go through apply_all old sysfs interface, so I attempted to
unify them late on development, sorry.

>> +static const struct btn_code_map *find_button_by_name(const char *name);
> Could you reorder the code such that this is not necessary. Having the 
> code here would be more logical as well because that relates to the array.
>
>> +
>> +static void ally_set_default_gamepad_mapping(struct ally_button_mapping *map)
>> +{
>> +	int i;
>> +
>> +	/* Set all pair indexes and initialize to NONE */
>> +	for (i = 0; i < 9; i++) {
> ARRAY_SIZE()
>
>> +		map->button_pairs[i].pair_index = i + 1;
>> +		map->button_pairs[i].first.remap =
>> +			find_button_by_name("NONE");
>> +		map->button_pairs[i].first.macro =
>> +			find_button_by_name("NONE");
>> +		map->button_pairs[i].second.remap =
>> +			find_button_by_name("NONE");
>> +		map->button_pairs[i].second.macro =
>> +			find_button_by_name("NONE");
> Why does this have to search for the NONE button multiple times?
>
>> +	}
>> +
>> +	map->button_pairs[BTN_PAIR_AB - 1].first.remap =
>> +		find_button_by_name("BTN_SOUTH");
>> +	map->button_pairs[BTN_PAIR_AB - 1].second.remap =
>> +		find_button_by_name("BTN_EAST");
>> +
>> +	map->button_pairs[BTN_PAIR_XY - 1].first.remap =
>> +		find_button_by_name("BTN_NORTH");
>> +	map->button_pairs[BTN_PAIR_XY - 1].second.remap =
>> +		find_button_by_name("BTN_WEST");
>> +
>> +	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].first.remap =
>> +		find_button_by_name("BTN_TL");
>> +	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].second.remap =
>> +		find_button_by_name("BTN_TR");
>> +
>> +	map->button_pairs[BTN_PAIR_STICK_LR - 1].first.remap =
>> +		find_button_by_name("BTN_THUMBL");
>> +	map->button_pairs[BTN_PAIR_STICK_LR - 1].second.remap =
>> +		find_button_by_name("BTN_THUMBR");
>> +
>> +	map->button_pairs[BTN_PAIR_DPAD_UPDOWN - 1].first.remap =
>> +		find_button_by_name("ABS_HAT0Y_NEG");
>> +	map->button_pairs[BTN_PAIR_DPAD_UPDOWN - 1].second.remap =
>> +		find_button_by_name("ABS_HAT0Y_POS");
>> +
>> +	map->button_pairs[BTN_PAIR_DPAD_LEFTRIGHT - 1].first.remap =
>> +		find_button_by_name("ABS_HAT0X_NEG");
>> +	map->button_pairs[BTN_PAIR_DPAD_LEFTRIGHT - 1].second.remap =
>> +		find_button_by_name("ABS_HAT0X_POS");
>> +
>> +	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].first.remap =
>> +		find_button_by_name("ABS_Z");
>> +	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].second.remap =
>> +		find_button_by_name("ABS_RZ");
>> +
>> +	map->button_pairs[BTN_PAIR_VIEW_MENU - 1].first.remap =
>> +		find_button_by_name("BTN_SELECT");
>> +	map->button_pairs[BTN_PAIR_VIEW_MENU - 1].second.remap =
>> +		find_button_by_name("BTN_START");
>> +
>> +	map->button_pairs[BTN_PAIR_M1M2 - 1].first.remap =
>> +		find_button_by_name("FN_M2");
>> +	map->button_pairs[BTN_PAIR_M1M2 - 1].second.remap =
>> +		find_button_by_name("FN_M1");
>> +}
>> +
>> +static void ally_set_default_keyboard_mapping(struct ally_button_mapping *map)
>> +{
>> +	int i;
>> +
>> +	/* Set all pair indexes and initialize to NONE */
>> +	for (i = 0; i < 9; i++) {
>> +		map->button_pairs[i].pair_index = i + 1;
>> +		map->button_pairs[i].first.remap =
>> +			find_button_by_name("NONE");
>> +		map->button_pairs[i].first.macro =
>> +			find_button_by_name("NONE");
>> +		map->button_pairs[i].second.remap =
>> +			find_button_by_name("NONE");
>> +		map->button_pairs[i].second.macro =
>> +			find_button_by_name("NONE");
>> +	}
> Duplicated code with the very same problems!
>
>> +	/*
>> +	 * Desktop mode defaults: keyboard and mouse mappings.
>> +	 * Buttons not listed here (dpad, view, menu) remain NONE.
>> +	 */
>> +
>> +	/* btn_a => KEY_SPACE, btn_b => KEY_E */
>> +	map->button_pairs[BTN_PAIR_AB - 1].first.remap =
>> +		find_button_by_name("KEY_SPACE");
>> +	map->button_pairs[BTN_PAIR_AB - 1].second.remap =
>> +		find_button_by_name("KEY_E");
>> +
>> +	/* btn_x => KEY_R, btn_y => KEY_F */
>> +	map->button_pairs[BTN_PAIR_XY - 1].first.remap =
>> +		find_button_by_name("KEY_R");
>> +	map->button_pairs[BTN_PAIR_XY - 1].second.remap =
>> +		find_button_by_name("KEY_F");
>> +
>> +	/* LB => wheel up, RB => wheel down */
>> +	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].first.remap =
>> +		find_button_by_name("REL_WHEEL_HI_RES_UP");
>> +	map->button_pairs[BTN_PAIR_BUMPER_LR - 1].second.remap =
>> +		find_button_by_name("REL_WHEEL_HI_RES_DOWN");
>> +
>> +	/* left stick click => left click, right stick click => left shift */
>> +	map->button_pairs[BTN_PAIR_STICK_LR - 1].first.remap =
>> +		find_button_by_name("BTN_LEFT");
>> +	map->button_pairs[BTN_PAIR_STICK_LR - 1].second.remap =
>> +		find_button_by_name("KEY_LEFTSHIFT");
>> +
>> +	/* LT => left click, RT => right click */
>> +	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].first.remap =
>> +		find_button_by_name("BTN_LEFT");
>> +	map->button_pairs[BTN_PAIR_TRIGGER_LR - 1].second.remap =
>> +		find_button_by_name("BTN_RIGHT");
>> +
>> +	/* M2 => FN_M2, M1 => FN_M1 */
>> +	map->button_pairs[BTN_PAIR_M1M2 - 1].first.remap =
>> +		find_button_by_name("FN_M2");
>> +	map->button_pairs[BTN_PAIR_M1M2 - 1].second.remap =
>> +		find_button_by_name("FN_M1");
>> +}
>> +
>>  static ssize_t gamepad_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
>>  {
>>  	struct hid_device *hdev = to_hid_device(dev);
>> @@ -1113,7 +1452,7 @@ static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *a
>>  	struct ally_config *cfg;
>>  	u8 mode_byte;
>>  	int mode;
>> -	int ret;
>> +	int ret, i;
>>  
>>  	if (!ally)
>>  		return -ENODEV;
>> @@ -1141,6 +1480,33 @@ static ssize_t gamepad_mode_store(struct device *dev, struct device_attribute *a
>>  
>>  	cfg->gamepad_mode = mode_byte;
>>  
>> +	/*
>> +	 * When the gamepad mode changes, the hardware automatically
>> +	 * resets all button mappings to its internal defaults for the
>> +	 * new mode.
>> +	 *
>> +	 * Refresh the software cache so that subsequent
>> +	 * reads from sysfs reflect what the firmware is actually
>> +	 * using.
>> +	 */
>> +	if (mode_byte == ALLY_GAMEPAD_MODE_GAMEPAD)
>> +		ally_set_default_gamepad_mapping(&cfg->button_mappings);
>> +	else if (mode_byte == ALLY_GAMEPAD_MODE_KEYBOARD)
>> +		ally_set_default_keyboard_mapping(&cfg->button_mappings);
>> +
>> +	/*
>> +	 * The re-defaulted mappings are what the remap and macro files
>> +	 * report: wake anyone polling them about the new values.
>> +	 */
>> +	if (cfg->button_entries) {
>> +		for (i = 0; i < ALLY_BTN_MAX; i++) {
>> +			sysfs_notify(&dev->kobj, cfg->button_entries[i].group.name,
>> +				     "remap");
>> +			sysfs_notify(&dev->kobj, cfg->button_entries[i].group.name,
>> +				     "macro");
>> +		}
>> +	}
>> +
>>  	hid_dbg(hdev, "Set gamepad mode to %s\n", ally_gamepad_mode_text[mode]);
>>  
>>  	sysfs_notify(&dev->kobj, NULL, "gamepad_mode");
>> @@ -2815,6 +3181,575 @@ static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
>>  	entry->attrs[4] = NULL;
>>  }
>>  
>> +/**
>> + * find_button_by_name() - Find a button code map by its name
>> + * @name: name of the button code to look up
>> + *
>> + * Return: the matching button code map, or NULL if not found
>> + */
>> +static const struct btn_code_map *find_button_by_name(const char *name)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < keymap_len; i++) {
>> +		if (strcmp(ally_btn_codes[i].name, name) == 0)
> !strcmp()
>
> You probably should check if you've the include for strcmp(), could be 
> necessary for some other patch too, I've lost track what includes have 
> been added so far.
>
>> +			return &ally_btn_codes[i];
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * ally_remap_code_valid() - Check if a remap target is valid for a button
>> + * @btn: button to remap
>> + * @gamepad_mode: gamepad mode to validate the remap against
>> + * @code: remap target to check
>> + * @is_macro: whether the target is assigned to the macro mapping
>> + *
>> + * M1 and M2 can be remapped to anything at any time.
>> + * All other buttons may only be remapped to gamepad buttons (BTN_TYPE_PAD)
>> + * when in gamepad mode, and to keyboard/mouse/media codes when in desktop
>> + * (keyboard) mode.  BTN_TYPE_NONE (unmapped) is always allowed.
>> + * Macro mappings are executed by the MCU as input sequences on the virtual
>> + * keyboard and are accepted regardless of the current gamepad mode.
>> + *
>> + * Return: true if the remap is valid, false otherwise
>> + */
>> +static bool ally_remap_code_valid(enum ally_button_id btn, u8 gamepad_mode,
>> +				  const struct btn_code_map *code, bool is_macro)
>> +{
>> +	if (!code)
>> +		return false;
>> +
>> +	/* M1 and M2 can be remapped to everything */
>> +	if (btn == ALLY_BTN_M1 || btn == ALLY_BTN_M2)
>> +		return true;
>> +
>> +	/* NONE is always valid */
>> +	if (code->type == BTN_TYPE_NONE)
>> +		return true;
>> +
>> +	/* Macros are not restricted by the controller emulation mode */
>> +	if (is_macro)
>> +		return true;
>> +
>> +	if (gamepad_mode == ALLY_GAMEPAD_MODE_GAMEPAD)
>> +		return code->type == BTN_TYPE_PAD;
>> +
>> +	/* Desktop / keyboard mode: allow keyboard, mouse and media */
>> +	return code->type == BTN_TYPE_KB ||
>> +	       code->type == BTN_TYPE_MOUSE ||
>> +	       code->type == BTN_TYPE_MEDIA;
>> +}
>> +
>> +/**
>> + * ally_set_button_mapping() - Set the button mapping for a button pair
>> + * @hdev: HID device
>> + * @ally: ally handheld structure
>> + * @mapping: button pair mapping to send to the device
>> + *
>> + * Return: count of data transferred, negative if error
>> + */
>> +static int ally_set_button_mapping(struct hid_device *hdev, struct ally_handheld *ally,
>> +				   struct button_pair_map *mapping)
>> +{
>> +	/* The MCU mapping block is four consecutive 11-byte entries starting at
> Inconsistent multi-line comment style.
>
>> +	 * buf[5]: first remap 5-15, first macro 16-26, second remap 27-37,
>> +	 * second macro 38-48 (see hid-asus-ally __btn_pair_to_pkt, BTN_CODE_LEN).
> I didn't find __btn_pair_to_pkt from anywhere?? Nor BTN_CODE_LEN??
>
>> +	 */
>> +	u8 macro_bytes[11] = {0};
>> +	u8 btn_bytes[11] = {0};
> = {} initializes to default values which is 0 for u8.
>
>> +
>> +	if (!mapping || !mapping->first.remap || !mapping->first.macro ||
>> +	    !mapping->second.remap || !mapping->second.macro)
>> +		return -EINVAL;
>> +
>> +	u8 *buf __free(kfree) = ally_alloc_cmd(CMD_SET_MAPPING, NULL, 0);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	/* This packet is slightly different from the other
> Comment style.
>
>> +	 * as before the packet length there is an extra byte
>> +	 * which is the pair index.
>> +	 */
>> +	buf[3] = mapping->pair_index;
>> +	buf[4] = 0x2C; /* Length */
> Use named define or sizeof() (if viable).
>
>> +
>> +	/* First button mapping */
>> +	buf[5] = mapping->first.remap->type;
>> +	/* Fill in bytes 6-14 with button code */
>> +	if (mapping->first.remap->type) {
>> +		memset(btn_bytes, 0, sizeof(btn_bytes));
>> +		btn_bytes[0] = mapping->first.remap->type;
>> +
>> +		/* Value byte position depends on type: pad=1, kb=2, media=3,
> Comment style.
>
>> +		 * mouse=4 (see hid-asus-ally BTN_CODE definitions).
>> +		 */
>> +		switch (mapping->first.remap->type) {
>> +		case BTN_TYPE_NONE:
>> +			break;
>> +		case BTN_TYPE_PAD:
>> +			btn_bytes[1] = mapping->first.remap->value;
>> +			break;
>> +		case BTN_TYPE_KB:
>> +			btn_bytes[2] = mapping->first.remap->value;
>> +			break;
>> +		case BTN_TYPE_MEDIA:
>> +			btn_bytes[3] = mapping->first.remap->value;
>> +			break;
>> +		case BTN_TYPE_MOUSE:
>> +			btn_bytes[4] = mapping->first.remap->value;
>> +			break;
>> +		}
>> +		memcpy(&buf[5], btn_bytes, 11);
> Magic 11. Please name properly.
>
> I wonder though if there should be helpers for these with a few parameters 
> as these seem mostly just duplicated multiple times.
>
>> +	}
>> +
>> +	/* Macro mapping for first button if any */
>> +	buf[16] = mapping->first.macro->type;
>> +	if (mapping->first.macro->type) {
>> +		memset(macro_bytes, 0, sizeof(macro_bytes));
>> +		macro_bytes[0] = mapping->first.macro->type;
>> +
>> +		switch (mapping->first.macro->type) {
>> +		case BTN_TYPE_NONE:
>> +			break;
>> +		case BTN_TYPE_PAD:
>> +			macro_bytes[1] = mapping->first.macro->value;
>> +			break;
>> +		case BTN_TYPE_KB:
>> +			macro_bytes[2] = mapping->first.macro->value;
>> +			break;
>> +		case BTN_TYPE_MEDIA:
>> +			macro_bytes[3] = mapping->first.macro->value;
>> +			break;
>> +		case BTN_TYPE_MOUSE:
>> +			macro_bytes[4] = mapping->first.macro->value;
>> +			break;
>> +		}
>> +		memcpy(&buf[16], macro_bytes, 11);
> Magic 11.
>
>> +	}
>> +
>> +	/* Second button mapping */
>> +	buf[27] = mapping->second.remap->type;
>> +	/* Fill in bytes 28-36 with button code */
>> +	if (mapping->second.remap->type) {
>> +		memset(btn_bytes, 0, sizeof(btn_bytes));
>> +		btn_bytes[0] = mapping->second.remap->type;
>> +
>> +		switch (mapping->second.remap->type) {
>> +		case BTN_TYPE_NONE:
>> +			break;
>> +		case BTN_TYPE_PAD:
>> +			btn_bytes[1] = mapping->second.remap->value;
>> +			break;
>> +		case BTN_TYPE_KB:
>> +			btn_bytes[2] = mapping->second.remap->value;
>> +			break;
>> +		case BTN_TYPE_MEDIA:
>> +			btn_bytes[3] = mapping->second.remap->value;
>> +			break;
>> +		case BTN_TYPE_MOUSE:
>> +			btn_bytes[4] = mapping->second.remap->value;
>> +			break;
>> +		}
>> +		memcpy(&buf[27], btn_bytes, 11);
> Ditto.
>
>> +	}
>> +
>> +	/* Macro mapping for second button if any */
>> +	buf[38] = mapping->second.macro->type;
>> +	if (mapping->second.macro->type) {
>> +		memset(macro_bytes, 0, sizeof(macro_bytes));
>> +		macro_bytes[0] = mapping->second.macro->type;
>> +
>> +		switch (mapping->second.macro->type) {
>> +		case BTN_TYPE_NONE:
>> +			break;
>> +		case BTN_TYPE_PAD:
>> +			macro_bytes[1] = mapping->second.macro->value;
>> +			break;
>> +		case BTN_TYPE_KB:
>> +			macro_bytes[2] = mapping->second.macro->value;
>> +			break;
>> +		case BTN_TYPE_MEDIA:
>> +			macro_bytes[3] = mapping->second.macro->value;
>> +			break;
>> +		case BTN_TYPE_MOUSE:
>> +			macro_bytes[4] = mapping->second.macro->value;
>> +			break;
>> +		}
>> +		memcpy(&buf[38], macro_bytes, 11);
> Ditto.
>
>> +	}
>> +
>> +	return ally_gamepad_send_packet(ally, hdev, buf, ROG_ALLY_REPORT_SIZE);
>> +}
>> +
>> +/**
>> + * ally_apply_button_mappings() - Push the recorded mappings of the current
>> + * gamepad mode to the controller
>> + * @hdev: HID device
>> + * @ally: ally handheld structure
>> + * @cfg: ally config structure
>> + *
>> + * An MCU reset restores the firmware default mappings: push the recorded
>> + * ones again so that the hardware keeps matching what userspace
>> + * configured. The caller must hold cfg->config_mutex, exactly like the
>> + * sysfs store callbacks that update single pairs.
>> + */
>> +static void ally_apply_button_mappings(struct hid_device *hdev,
>> +				       struct ally_handheld *ally,
>> +				       struct ally_config *cfg)
>> +{
>> +	struct ally_button_mapping *mode_map;
>> +	int i, ret;
>> +
>> +	mode_map = &cfg->button_mappings;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(mode_map->button_pairs); i++) {
>> +		mode_map->button_pairs[i].pair_index = i + 1;
>> +
>> +		ret = ally_set_button_mapping(hdev, ally,
>> +					      &mode_map->button_pairs[i]);
>> +		if (ret < 0) {
>> +			hid_warn(hdev,
>> +				 "Failed to restore button mappings: %d\n",
>> +				 ret);
>> +			return;
>> +		}
>> +	}
>> +}
>> +
>> +/**
>> + * get_button_pair_info() - Get the pair index and position of a button
>> + * @button_id: button to look up
>> + * @pair_idx: output pointer for the pair index of the button
>> + * @is_first: output pointer for the position of the button within the pair
>> + *
>> + * Return: 0 on success, negative errno on failure
>> + */
>> +static int get_button_pair_info(enum ally_button_id button_id,
>> +				enum btn_pair_index *pair_idx,
>> +				bool *is_first)
>> +{
>> +	switch (button_id) {
>> +	case ALLY_BTN_DU:
>> +		*pair_idx = BTN_PAIR_DPAD_UPDOWN;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_DD:
>> +		*pair_idx = BTN_PAIR_DPAD_UPDOWN;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_DL:
>> +		*pair_idx = BTN_PAIR_DPAD_LEFTRIGHT;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_DR:
>> +		*pair_idx = BTN_PAIR_DPAD_LEFTRIGHT;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_J0B:
>> +		*pair_idx = BTN_PAIR_STICK_LR;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_J1B:
>> +		*pair_idx = BTN_PAIR_STICK_LR;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_LB:
>> +		*pair_idx = BTN_PAIR_BUMPER_LR;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_RB:
>> +		*pair_idx = BTN_PAIR_BUMPER_LR;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_A:
>> +		*pair_idx = BTN_PAIR_AB;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_B:
>> +		*pair_idx = BTN_PAIR_AB;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_X:
>> +		*pair_idx = BTN_PAIR_XY;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_Y:
>> +		*pair_idx = BTN_PAIR_XY;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_VIEW:
>> +		*pair_idx = BTN_PAIR_VIEW_MENU;
>> +		*is_first = true;
>> +		break;
>> +	case ALLY_BTN_MENU:
>> +		*pair_idx = BTN_PAIR_VIEW_MENU;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_M1:
>> +		*pair_idx = BTN_PAIR_M1M2;
>> +		*is_first = false;
>> +		break;
>> +	case ALLY_BTN_M2:
>> +		*pair_idx = BTN_PAIR_M1M2;
>> +		*is_first = true;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t button_remap_show(struct device *dev,
>> +				 struct device_attribute *attr, char *buf)
>> +{
>> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
>> +	struct ally_handheld *ally = btn_attr->ally;
>> +	const struct btn_code_map *code;
>> +	struct ally_config *cfg;
>> +	enum ally_button_id button_id = btn_attr->button_id;
>> +	enum btn_pair_index pair_idx;
>> +	struct button_pair_map *pair;
>> +	struct button_map *btn_map;
>> +	bool is_first;
>> +	int ret;
>> +
>> +	if (!ally)
>> +		return -ENODEV;
>> +
>> +	cfg = ally_get_config(ally);
>> +	if (!cfg)
>> +		return -ENODEV;
>> +
>> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	guard(mutex)(&cfg->config_mutex);
>> +
>> +	pair = &cfg->button_mappings.button_pairs[pair_idx - 1];
>> +	btn_map = is_first ? &pair->first : &pair->second;
>> +	code = btn_attr->is_macro ? btn_map->macro : btn_map->remap;
>> +
>> +	/*
>> +	 * The mappings are never NULL: the defaults, the mode-switch
>> +	 * re-defaults and the store rollback all guarantee it. A miss
>> +	 * means a broken default table, so fail loudly instead of
>> +	 * dereferencing.
>> +	 */
>> +	if (!code)
>> +		return -EIO;
>> +
>> +	if (code->type == BTN_TYPE_NONE)
>> +		return sysfs_emit(buf, "NONE\n");
> Wouldn't it emit "NONE" anyway with the normal return so why you need 
> special print for this separately?
>
>> +
>> +	return sysfs_emit(buf, "%s\n", code->name);
>> +}
>> +
>> +static ssize_t button_remap_store(struct device *dev,
>> +				  struct device_attribute *attr,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct hid_device *hdev = to_hid_device(dev);
>> +	struct button_remap_attr *btn_attr = to_button_remap_attr(attr);
>> +	struct ally_handheld *ally = btn_attr->ally;
>> +	enum ally_button_id button_id = btn_attr->button_id;
>> +	struct ally_button_mapping *mode_map;
>> +	const struct btn_code_map *code;
>> +	const struct btn_code_map *old_code;
>> +	enum btn_pair_index pair_idx;
>> +	struct button_pair_map *pair;
>> +	struct button_map *btn_map;
>> +	struct ally_config *cfg;
>> +	char btn_name[32];
>> +	bool is_first;
>> +	bool undo_hw;
>> +	int ret, i;
>> +
>> +	if (!ally)
>> +		return -ENODEV;
>> +
>> +	cfg = ally_get_config(ally);
>> +	if (!cfg)
>> +		return -ENODEV;
>> +
>> +	if (sscanf(buf, "%31s", btn_name) != 1)
>> +		return -EINVAL;
>> +
>> +	/* Handle "NONE" specially */
> Yeah yeah, and now you better must explain me why???? This is totally 
> useless comment and helps zero to explain why the code is making things 
> in a stupid way.
>
> Won't find_button_by_name() return the very same entry???
>
> (You can see probably I'm starting to get tired of reading this kind of 
> things at this point... ...I've read quite many lines by now.)
>
>> +	if (strcmp(btn_name, "NONE") == 0) {
> !strcmp(), but it looks totally useless check (see above).
>
>> +		code = &ally_btn_codes[0]; /* NONE entry */
>> +	} else {
>> +		code = find_button_by_name(btn_name);
>> +		if (!code)
>> +			return -EINVAL;
>> +	}
>> +
>> +	ret = get_button_pair_info(button_id, &pair_idx, &is_first);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	guard(mutex)(&cfg->config_mutex);
>> +
>> +	/*
>> +	 * Validate the remap target while holding the lock: the
>> +	 * gamepad mode can change concurrently, and the check must
>> +	 * be done against the very mode the mapping is written to.
>> +	 */
>> +	if (!ally_remap_code_valid(button_id, cfg->gamepad_mode, code,
>> +				   btn_attr->is_macro))
>> +		return -EINVAL;
>> +
>> +	/* Access the mapping for current gamepad mode */
>> +	pair = &cfg->button_mappings.button_pairs[pair_idx - 1];
>> +	btn_map = is_first ? &pair->first : &pair->second;
>> +
>> +	if (btn_attr->is_macro) {
>> +		old_code = btn_map->macro;
>> +		btn_map->macro = code;
>> +	} else {
>> +		old_code = btn_map->remap;
>> +		btn_map->remap = code;
>> +	}
>> +
>> +	/* Update pair index */
>> +	pair->pair_index = pair_idx;
>> +
>> +	/*
>> +	 * Send mapping to device with the caveat that first
>> +	 * generation devices require individual button pair updates.
>> +	 */
>> +	if (cfg->is_ally_x) {
>> +		ret = ally_set_button_mapping(hdev, ally, pair);
>> +		undo_hw = false;
>> +	} else {
>> +		mode_map = &cfg->button_mappings;
>> +		ret = 0;
> Is this required?
>
>> +		for (i = 0; i < 9; i++) {
> ARRAY_SIZE()
>
>> +			mode_map->button_pairs[i].pair_index = i + 1;
>> +			ret = ally_set_button_mapping(hdev, ally,
>> +						      &mode_map->button_pairs[i]);
>> +			if (ret < 0)
>> +				break;
> IMO, it would be better to use goto to deal with these errors (here and 
> above in the if blcok).
>
>> +		}
>> +
>> +		/*
>> +		 * Pairs are pushed one at a time: remember whether
>> +		 * the target pair reached the hardware before a
>> +		 * later pair failed, so the rollback below can undo
>> +		 * it on the device too.
>> +		 */
>> +		undo_hw = ret < 0 && i > pair_idx - 1;
> Do this above before goto.
>
>> +	}
>> +
>> +	/*
>> +	 * The hardware rejected the update: restore the previous
>> +	 * mapping so that the software cache keeps describing the
>> +	 * state the device is actually in.
>> +	 */
>> +	if (ret < 0) {
>> +		if (btn_attr->is_macro)
>> +			btn_map->macro = old_code;
>> +		else
>> +			btn_map->remap = old_code;
>> +
>> +		/*
>> +		 * The target pair was already committed before a
>> +		 * later pair failed: push the restored mapping once
>> +		 * more so the hardware matches the cache again. Best
>> +		 * effort only: the device is misbehaving already.
>> +		 */
>> +		if (undo_hw)
>> +			ally_set_button_mapping(hdev, ally, pair);
>> +
>> +		hid_err(hdev, "Failed to set button mapping: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	sysfs_notify(&dev->kobj, ally_button_names[btn_attr->button_id],
>> +		     btn_attr->is_macro ? "macro" : "remap");
> I suggest adding a local string_choices.h style str_macro_remap() for this 
> as it seems to occur a few time.
>
>> +
>> +	return count;
>> +}
>> +
>> +/**
>> + * button_remap_attr_create() - Create a button remap attribute
>> + * @ally: ally handheld structure
>> + * @button_id: button the attribute is for
>> + * @is_macro: whether the attribute controls the macro mapping
>> + *
>> + * Return: the newly created attribute, or NULL on allocation failure
>> + */
>> +static struct button_remap_attr *button_remap_attr_create(struct ally_handheld *ally,
>> +							  enum ally_button_id button_id,
>> +							  bool is_macro)
>> +{
>> +	struct button_remap_attr *attr __free(kfree) = kzalloc_obj(*attr);
>> +	if (!attr)
>> +		return NULL;
>> +
>> +	attr->ally = ally;
>> +	attr->button_id = button_id;
>> +	attr->is_macro = is_macro;
>> +	sysfs_attr_init(&attr->dev_attr.attr);
>> +	attr->dev_attr.attr.name = is_macro ? "macro" : "remap";
>> +	attr->dev_attr.attr.mode = 0644;
>> +	attr->dev_attr.show = button_remap_show;
>> +	attr->dev_attr.store = button_remap_store;
>> +
>> +	return no_free_ptr(attr);
> You don't need to use __free()/no_free_ptr() in this function because 
> only return is due to alloc fail check (attr is NULL at that point and 
> kfree(NULL) is no-op).
>
>> +}
>> +
>> +/**
>> + * btn_remap_index_show() - Show the list of valid remap targets for a button
>> + * @dev: device the attribute belongs to
>> + * @attr: attribute being read
>> + * @buf: buffer to write the list into
>> + *
>> + * M1/M2 list everything; other buttons list only the codes valid for the
>> + * active mode (gamepad buttons in gamepad mode, keyboard / mouse / media
>> + * in desktop mode).
>> + *
>> + * Return: number of characters written
>> + */
>> +static ssize_t btn_remap_index_show(struct device *dev,
>> +				    struct device_attribute *attr, char *buf)
>> +{
>> +	struct hid_device *hdev = to_hid_device(dev);
>> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>> +	struct ally_handheld *ally = drvdata->rog_ally;
>> +	struct ally_btn_sysfs_entry *entry = container_of(attr,
>> +		struct ally_btn_sysfs_entry, attr_remap_index);
>> +	struct ally_config *cfg;
>> +	enum ally_button_id btn = entry->btn;
>> +	u8 mode;
>> +	ssize_t len = 0;
>> +	int i;
>> +
>> +	if (!ally)
>> +		return -ENODEV;
>> +
>> +	cfg = ally_get_config(ally);
>> +	if (!cfg)
>> +		return -ENODEV;
>> +
>> +	scoped_guard(mutex, &cfg->config_mutex)
>> +		mode = cfg->gamepad_mode;
>> +
>> +	for (i = 0; i < keymap_len; i++) {
>> +		if (!ally_remap_code_valid(btn, mode, &ally_btn_codes[i], false))
>> +			continue;
>> +		len += sysfs_emit_at(buf, len, "%s ", ally_btn_codes[i].name);
>> +	}
>> +
>> +	if (len > 0)
>> +		buf[len - 1] = '\n';
>> +
>> +	return len;
>> +}
>> +
>>  /**
>>   * ally_create_button_attributes() - Create turbo button attributes
>>   * @hdev: HID device
>> @@ -2824,15 +3759,12 @@ static void ally_btn_turbo_init_attrs(struct ally_btn_sysfs_entry *entry)
>>   * Return: 0 on success, negative errno on failure
>>   */
>>  static int ally_create_button_attributes(struct hid_device *hdev,
>> -					 struct ally_handheld *ally,
>> -					 struct ally_config *cfg)
>> +					  struct ally_handheld *ally,
>> +					  struct ally_config *cfg)
>>  {
>>  	struct ally_btn_sysfs_entry *entries;
>>  	int i, ret;
>>  
>> -	if (!cfg->turbo_support)
>> -		return 0;
>> -
>>  	entries = kcalloc(ALLY_BTN_MAX, sizeof(*entries), GFP_KERNEL);
>>  	if (!entries)
>>  		return -ENOMEM;
>> @@ -2845,13 +3777,53 @@ static int ally_create_button_attributes(struct hid_device *hdev,
>>  		entries[i].hdev = hdev;
>>  		entries[i].btn = i;
>>  
>> -		if (!ally_btn_get_turbo_params(cfg, i)) {
>> -			hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
>> -			ret = -EINVAL;
>> +		if (cfg->turbo_support) {
>> +			if (!ally_btn_get_turbo_params(cfg, i)) {
>> +				hid_err(hdev, "Invalid button id %d for turbo attributes\n", i);
>> +				ret = -EINVAL;
>> +				goto err_cleanup;
>> +			}
>> +
>> +			ally_btn_turbo_init_attrs(&entries[i]);
>> +		}
>> +
>> +		entries[i].remap_attr = button_remap_attr_create(ally, i, false);
>> +		if (!entries[i].remap_attr) {
>> +			ret = -ENOMEM;
>>  			goto err_cleanup;
>>  		}
>>  
>> -		ally_btn_turbo_init_attrs(&entries[i]);
>> +		entries[i].macro_attr = button_remap_attr_create(ally, i, true);
>> +		if (!entries[i].macro_attr) {
>> +			ret = -ENOMEM;
>> +			goto err_cleanup;
>> +		}
>> +
>> +		/* Initialize the remap_index attribute */
>> +		sysfs_attr_init(&entries[i].attr_remap_index.attr);
>> +		entries[i].attr_remap_index.attr.name = "remap_index";
>> +		entries[i].attr_remap_index.attr.mode = 0444;
>> +		entries[i].attr_remap_index.show = btn_remap_index_show;
>> +		entries[i].attr_remap_index.store = NULL;
>> +
>> +		/* Set up attributes array based on what's supported */
>> +		if (cfg->turbo_support) {
>> +			entries[i].attrs[4] =
>> +				&entries[i].remap_attr->dev_attr.attr;
>> +			entries[i].attrs[5] =
>> +				&entries[i].macro_attr->dev_attr.attr;
>> +			entries[i].attrs[6] =
>> +				&entries[i].attr_remap_index.attr;
>> +			entries[i].attrs[7] = NULL;
>> +		} else {
>> +			entries[i].attrs[0] =
>> +				&entries[i].remap_attr->dev_attr.attr;
>> +			entries[i].attrs[1] =
>> +				&entries[i].macro_attr->dev_attr.attr;
>> +			entries[i].attrs[2] =
>> +				&entries[i].attr_remap_index.attr;
>> +			entries[i].attrs[3] = NULL;
> So only the attrs index changed?? Why all the duplication?
>
>> +		}
>>  
>>  		entries[i].group.name = ally_button_names[i];
>>  		entries[i].group.attrs = entries[i].attrs;
>> @@ -2867,12 +3839,27 @@ static int ally_create_button_attributes(struct hid_device *hdev,
>>  	return 0;
>>  
>>  err_cleanup:
>> +	/*
>> +	 * Unpublish first: the readers of button_entries (like the
>> +	 * gamepad_mode_store() cross-notification) hold config_mutex and
>> +	 * must observe either the live array or NULL, never a freed one.
>> +	 * The lock is released again before the removals, so a straggler
>> +	 * callback waiting on it can drain against the not yet freed
>> +	 * entries.
>> +	 */
>> +	scoped_guard(mutex, &cfg->config_mutex) {
>> +		cfg->button_entries = NULL;
>> +	}
>> +
>>  	while (--i >= 0)
>>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
>>  
>> -	kfree(entries);
>> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
>> +		kfree(entries[i].remap_attr);
>> +		kfree(entries[i].macro_attr);
>> +	}
>>  
>> -	cfg->button_entries = NULL;
>> +	kfree(entries);
>>  
>>  	return ret;
>>  }
>> @@ -2892,12 +3879,26 @@ static void ally_remove_button_attributes(struct hid_device *hdev, struct ally_c
>>  
>>  	entries = cfg->button_entries;
>>  
>> -	for (i = 0; i < ALLY_BTN_MAX; i++)
>> +	/*
>> +	 * Unpublish first: the readers of button_entries (like the
>> +	 * gamepad_mode_store() cross-notification) hold config_mutex and
>> +	 * must observe either the live array or NULL, never a freed one.
>> +	 * The lock is released again before the removals, so a straggler
>> +	 * callback waiting on it can drain against the not yet freed
>> +	 * entries, and sysfs_remove_group() waits for the last ones to
>> +	 * finish before the memory goes away.
>> +	 */
>> +	scoped_guard(mutex, &cfg->config_mutex) {
>> +		cfg->button_entries = NULL;
>> +	}
>> +
>> +	for (i = 0; i < ALLY_BTN_MAX; i++) {
>>  		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);
>> +		kfree(entries[i].remap_attr);
>> +		kfree(entries[i].macro_attr);
>> +	}
>>  
>>  	kfree(entries);
>> -
>> -	cfg->button_entries = NULL;
>>  }
>>  
>>  /**
>> @@ -2938,6 +3939,13 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>>  
>>  	if (!cfg->initialized) {
>>  		cfg->gamepad_mode = ALLY_GAMEPAD_MODE_GAMEPAD;
>> +		/*
>> +		 * Populate the button mappings for the default mode: a mode
>> +		 * switch re-defaults them, while on a later probe they hold
>> +		 * the values the user configured, which are what the restore
>> +		 * path re-applies to the controller.
>> +		 */
>> +		ally_set_default_gamepad_mapping(&cfg->button_mappings);
>>  		cfg->xbox_controller_enabled = true;
>>  		cfg->left_deadzone = 10;
>>  		cfg->left_outer_threshold = 90;
>> @@ -3010,12 +4018,15 @@ static struct ally_config *ally_config_create(struct hid_device *hdev, struct al
>>  		cal_created = ARRAY_SIZE(ally_cal_attr_groups);
>>  	}
>>  
>> -	if (cfg->turbo_support) {
>> -		ret = ally_create_button_attributes(hdev, ally, cfg);
>> -		if (ret < 0) {
>> -			hid_err(hdev, "Failed to create button attributes: %d\n", ret);
>> -			goto ally_config_create_sysfs_err;
>> -		}
>> +	/*
>> +	 * The button attributes carry the remap/macro attributes too, so
>> +	 * they are created for every device with button directories, not
>> +	 * only the ones with turbo support.
>> +	 */
>> +	ret = ally_create_button_attributes(hdev, ally, cfg);
>> +	if (ret < 0) {
>> +		hid_err(hdev, "Failed to create button attributes: %d\n", ret);
>> +		goto ally_config_create_sysfs_err;
>>  	}
>>  
>>  	cfg->initialized = true;
>> @@ -3610,6 +4621,10 @@ static int hid_asus_ally_init(struct hid_device *hdev, struct ally_handheld *all
>>  				 ret);
>>  	}
>>  
>> +	/* The mappings exist only if the button attributes were created. */
>> +	if (cfg->button_entries)
>> +		ally_apply_button_mappings(hdev, ally, cfg);
>> +
>>  	return 0;
>>  }
>>  
>>