From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta0.migadu.com (out-200.mta0.migadu.com [91.218.175.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5A1083A71AD for ; Fri, 18 Sep 2026 01:38:20 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.200 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789695508; cv=none; b=jKPtje6D9c+kFCGucbK6UL/HqGCpIjKsfUBgysBsAxA18NX6YXipO5WLCFC9Uhg/MmUOLgtw31rBup+MGEqTaHuv3oyHV1N+lyEoB+j8OPR3sZsAFas1n9T0YhlAS5I9ZViYDc4p/BdNVCPlQg2urYoqVEOghjW69Q7jIr9jWq8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789695508; c=relaxed/simple; bh=nov9NZ6EhNAAdopYUIHTrZgJsPsCH+wjVGmniAJZdts=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=UyTHq34x1hAlls+gSVZ87bmA+SkmUyBQg+7f6aRbObMUfqtrJ/VXSajUXCGoLOe3VGI7LPVUOxFRHQpn+4X6oou1qmync9Z7s1WmVmMnixCRaYRD/0alzMEHJAL1LS/ZQ+fsUjyqibYK2R5+hVRdm5XDRNfkTdnDtheLqXfwrdU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=dTY5SRWs; arc=none smtp.client-ip=91.218.175.200 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="dTY5SRWs" X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=nov9NZ6EhNAAdopYUIHTrZgJsPsCH+wjVGmniAJZdts=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1789695499; v=1; x=1790300299; b=dTY5SRWsYNZjnVJ8mRja20BIyVSyas1eX9YNohpSHy0qUhWL1UZSfXBWm68bVYP5lXBLUIeB BeODhOvKrBA79t8sv0pkl3LPBSRWLIGE2a9zqaAa1+IJ9ijYTI+pmHaWjo3DZvvDk32S7rJrQwe 2KNO6rbdISaJla0ikDavYFpQ= X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id 4ec5b4c4dec98d43; Fri, 18 Sep 2026 01:38:08 +0000 X-Mizu-Trace-ID: 4ec5b4c4dec98d43 X-Migadu-Flow: FLOW_OUT Message-ID: <9cb4ccdb-89e0-4c5c-a302-6e4040d25ab6@linux.dev> Date: Fri, 18 Sep 2026 03:38:07 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] HID: ayaneo: Add AYANEO 3 detachable controller driver To: "Derek J. Clark" , Antheas Kapenekakis , =?UTF-8?B?TWF0w61hcyBNYXJ0w61uZXo=?= Cc: Jiri Kosina , Benjamin Tissoires , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Dmitry Torokhov , =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= , Armin Wolf References: <20260824223103.93947-1-hello@matias.me> <20260917160722.89391-1-hello@matias.me> Content-Language: en-US From: Denis Benato In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 9/18/26 02:42, Derek J. Clark wrote: > On September 17, 2026 2:48:11 PM PDT, Antheas Kapenekakis wrote: >> On Thu, 17 Sept 2026 at 18:07, Matías Martínez wrote: >>> From: Matías Martínez >> Hi Matias, >> >>> The AYANEO 3 handheld has a detachable controller with swappable >>> modules ("Magic Modules"). The controller exposes three USB HID >>> interfaces behind 1c4f:0002 (a generic SigmaMicro VID/PID, hence the >>> DMI gate): a gamepad, a keyboard for the extra buttons, and a vendor >>> interface accepting 65-byte commands. >>> >>> Add a driver for the vendor interface providing module identification >>> (module_left/module_right sysfs attributes), software eject of the >>> modules (eject sysfs attribute, blocking until the firmware confirms >>> the release handshake), and RGB control of the joystick rings as a >>> multicolor LED class device (":rgb:joystick_rings"; >>> userspace such as InputPlumber matches the function suffix). The >>> firmware's fixed breathing pattern is exposed through the hw_pattern >>> trigger ABI. >>> >>> This complements the ayaneo-ec platform driver, which exposes module >>> attach state and controller power. A full physical eject is performed >>> by writing to eject and then cutting power through ayaneo-ec's >>> controller_power attribute; that orchestration is deliberately left >>> to userspace. >>> >>> The protocol was reverse engineered in the Handheld Daemon project by >>> Antheas Kapenekakis. Tested on an AYANEO 3: module identification, >>> RGB solid and breathing, a full eject/reinsert/repower cycle, and >>> repeated driver unbinds under a concurrent brightness-write load. >>> Signed-off-by: Matías Martínez >>> Reviewed-by: Denis Benato >>> --- >>> Changes in v3: >>> - Use a generic ayaneo_ prefix for entry points and driver structure >>> so a future device or protocol revision slots in without churn; >>> wire-protocol constants stay AYA3_* since they are specific to >>> this firmware generation. [Derek J. Clark] >>> - Describe the wire format with packed aya3_config/aya3_resp structs, >>> static_assert their sizes against the report sizes, and name every >>> firmware vibration level in an enum instead of a lone default >>> define. [Derek J. Clark] >>> - Take the command lock with scoped_cond_guard(mutex_intr, ...) at >>> every interruptible lock site so the unlock cannot be dropped in a >>> future edit. [Derek J. Clark] >>> - Mark the LED class device LED_COLOR_ID_RGB so userspace can detect >>> the RGB interface generically. [Derek J. Clark] >>> - Name the firmware timing constants and record where the timings >>> come from and what was validated on hardware. >>> >>> No functional change relative to v2. The rework was prompted by >>> Derek J. Clark's review of the driver in the OpenGamingCollective >>> tree and retested on an AYANEO 3: module identification, RGB solid >>> and breathing via hw_pattern, rejection of malformed hw_pattern >>> writes, and repeated bind/unbind cycles, all with a clean dmesg. >>> >>> One question from that review was whether RGB sysfs writes need >>> debouncing, since Steam emits one write per slider increment during >>> a color drag. They do not: the driver registers only >>> brightness_set_blocking, so the LED core defers stores to its >>> set_brightness_work and coalesces bursts to the latest state. >>> Measured on hardware, a command+ACK round trip averages 5.3 ms >>> (3.8-8 ms over 100 samples) and 1000 back-to-back multi_intensity >>> stores return in 12 ms total, reaching the device as two to three >>> commands. >> After you submitted your patch series to the lore, why didn't the >> review take place here and take place downstream? I understand that >> prior to submitting your first kernel patch, it is natural to get some >> informal feedback, but it seems that all of the reviews of this driver >> happened outside the submission and after the submission? The rby >> Denis should not be added by you. It should be added by denis by >> replying to the mailing list and then you carry it forward on future >> revisions (for provenance). Hello Antheas, thanks for the interest but don't worry: he added my rvb because I asked him to do so. The driver was developed and tested against the at-the-time version of linux-next, as all new drivers should be as stated in the linux-next landing page. Best regards, Denis >> If you are developing a downstream kernel patch for >> OGC/Bazzite/whoever, it is perfectly fine to do downstream work and >> reviews and only submit the driver after it is ready. In fact, it >> would be very preferable for all of us for you to test your driver >> downstream, resolve all feedback and then submit it when it's ready. >> But mixing this is peculiar. This is not a comment on you, you were >> not the one reviewing your patch out of band. > Antheas, > > There is no reason to throw shade about the process. This was submitted to OGC unstable prior to v1 where Denis reviewed and tagged it prior to it being submitted. I wasn't Cc'd or informed that it was already on a v2 less than 48h later, so I submitted my feedback directly on the PR a couple days later. Had I been aware I would have obviously submitted feedback here. Without your feedback necessary we've already implemented better control policies to avoid this in the future. > > Thanks, > Derek > >>> Also suggested in that review, but held out of the patch while the >>> driver's scope is under discussion: a rumble_intensity attribute >>> (the firmware takes three vibration levels), an eject_index >>> attribute, and a notification path from hid-ayaneo to ayaneo-ec so >>> the EC driver could react to ejects. The last one concerns the >>> ayaneo-ec/pdx86 side, hence the added Cc. >>> >>> Changes in v2: >>> - Unregister the LED class device before tearing down the HID >>> transport, and flush a late-queued brightness work item that can >>> race the unregister; the work could otherwise run against freed >>> memory. Found by stress-testing rmmod under a brightness-write >>> loop; also reachable whenever the controller power-cycles (resume, >>> module eject) while userspace writes the LED. [sashiko, Denis] >>> - Abort the eject wait as soon as the transport reports a fatal >>> error instead of polling for up to 8 seconds. [sashiko] >>> - Reject report descriptors with no collections explicitly. [sashiko] >>> - Document why a late reply to a timed-out command is harmless. >>> [sashiko] >>> - Stop writing the joystick-sensitivity bytes in the config command >>> so RGB updates no longer clobber the firmware setting; verified on >>> hardware that RGB and eject work without them. [Antheas] >>> - Expose the firmware's breathing mode through the hw_pattern >>> trigger ABI, with an ABI document. [Antheas] >>> >>> .../testing/sysfs-class-led-driver-hid-ayaneo | 15 + >>> .../ABI/testing/sysfs-driver-hid-ayaneo | 36 ++ >>> MAINTAINERS | 8 + >>> drivers/hid/Kconfig | 14 + >>> drivers/hid/Makefile | 1 + >>> drivers/hid/hid-ayaneo.c | 594 ++++++++++++++++++ >>> 6 files changed, 668 insertions(+) >>> create mode 100644 Documentation/ABI/testing/sysfs-class-led-driver-hid-ayaneo >>> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-ayaneo >>> create mode 100644 drivers/hid/hid-ayaneo.c >>> >>> diff --git a/Documentation/ABI/testing/sysfs-class-led-driver-hid-ayaneo b/Documentation/ABI/testing/sysfs-class-led-driver-hid-ayaneo >>> new file mode 100644 >>> index 000000000..00f100dba >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-class-led-driver-hid-ayaneo >>> @@ -0,0 +1,15 @@ >>> +What: /sys/class/leds//hw_pattern >> Consider exploring similar ABIs in e.g., Legion Go S hid and mirroring >> their ABI. Then remove this file. This is not the only breathing >> device. Moreover, delta_t values are ignored and you are introducing >> an ABI for them? Prefer a mode setting that can be standard/breathing >> mirroring a different driver. >> >>> +Date: August 2026 >>> +KernelVersion: 7.3 >>> +Contact: Matías Martínez >>> +Description: >>> + Specify a hardware pattern for the AYANEO 3 joystick >>> + rings LED. The firmware supports a single breathing >>> + pattern, pulsing the current colour at a fixed, >>> + firmware-controlled period: >>> + >>> + "0 " >>> + >>> + Both delta_t values are accepted but ignored, as the >>> + period is not configurable. must be >>> + non-zero. Any other pattern is rejected. >>> diff --git a/Documentation/ABI/testing/sysfs-driver-hid-ayaneo b/Documentation/ABI/testing/sysfs-driver-hid-ayaneo >>> new file mode 100644 >>> index 000000000..807c4fc9d >>> --- /dev/null >>> +++ b/Documentation/ABI/testing/sysfs-driver-hid-ayaneo >>> @@ -0,0 +1,36 @@ >>> +What: /sys/bus/hid/drivers/hid-ayaneo//module_left >>> +What: /sys/bus/hid/drivers/hid-ayaneo//module_right >>> +Date: August 2026 >>> +KernelVersion: 7.3 >>> +Contact: Matías Martínez >>> +Description: >>> + Reports the type of the module currently inserted in the >>> + left/right slot of the AYANEO 3 detachable controller, as >>> + the raw identifier reported by the controller firmware in >>> + hexadecimal (e.g. "0x04"). Bits 0-5 encode the module >>> + type, bit 6 indicates the module is inserted rotated. >>> + >>> + Reading these attributes queries the controller and can >>> + take up to a second. >>> + >>> +What: /sys/bus/hid/drivers/hid-ayaneo//eject >>> +Date: August 2026 >>> +KernelVersion: 7.3 >>> +Contact: Matías Martínez >>> +Description: >>> + Write-only. Writing "left", "right" or "both" asks the >>> + controller firmware to release the corresponding >>> + module(s). The write blocks until the firmware confirms >>> + the release handshake (typically a few seconds). The >>> + module is physically released once controller power is >>> + subsequently cut through the ayaneo-ec platform driver's >>> + controller_power attribute; that final step is left to >>> + userspace. >>> + >>> +What: /sys/bus/hid/drivers/hid-ayaneo//reset >>> +Date: August 2026 >>> +KernelVersion: 7.3 >>> +Contact: Matías Martínez >>> +Description: >>> + Write-only. Writing "1" asks the controller firmware to >>> + perform a quick reset of the controller configuration. >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index 8b14f290c..3290d9957 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -4508,6 +4508,14 @@ F: Documentation/devicetree/bindings/spi/axiado,ax3000-spi.yaml >>> F: drivers/spi/spi-axiado.c >>> F: drivers/spi/spi-axiado.h >>> >>> +AYANEO 3 CONTROLLER HID DRIVER >>> +M: Matías Martínez >>> +L: linux-input@vger.kernel.org >>> +S: Maintained >>> +F: Documentation/ABI/testing/sysfs-class-led-driver-hid-ayaneo >>> +F: Documentation/ABI/testing/sysfs-driver-hid-ayaneo >>> +F: drivers/hid/hid-ayaneo.c >>> + >>> AYANEO PLATFORM EC DRIVER >>> M: Antheas Kapenekakis >>> L: platform-driver-x86@vger.kernel.org >>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >>> index 0e3a0ccd6..319eda887 100644 >>> --- a/drivers/hid/Kconfig >>> +++ b/drivers/hid/Kconfig >>> @@ -205,6 +205,20 @@ config HID_AUREAL >>> help >>> Support for Aureal Cy se W-01RN Remote Controller and other Aureal derived remotes. >>> >>> +config HID_AYANEO >>> + tristate "AYANEO 3 detachable controller support" >>> + depends on USB_HID >>> + depends on DMI >>> + depends on LEDS_CLASS_MULTICOLOR >>> + help >>> + Provides support for the detachable controller ("Magic Modules") >>> + of the AYANEO 3 handheld: module identification, software eject >>> + and RGB control of the joystick rings. Complements the ayaneo-ec >>> + platform driver, which handles module attach state and controller >>> + power. >>> + >>> + Say Y or M here if you have an AYANEO 3. >>> + >>> config HID_BELKIN >>> tristate "Belkin Flip KVM and Wireless keyboard" >>> help >>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile >>> index 79384d905..2f74f2867 100644 >>> --- a/drivers/hid/Makefile >>> +++ b/drivers/hid/Makefile >>> @@ -35,6 +35,7 @@ obj-$(CONFIG_HID_APPLETB_KBD) += hid-appletb-kbd.o >>> obj-$(CONFIG_HID_CREATIVE_SB0540) += hid-creative-sb0540.o >>> obj-$(CONFIG_HID_ASUS) += hid-asus.o >>> obj-$(CONFIG_HID_AUREAL) += hid-aureal.o >>> +obj-$(CONFIG_HID_AYANEO) += hid-ayaneo.o >>> obj-$(CONFIG_HID_BELKIN) += hid-belkin.o >>> obj-$(CONFIG_HID_BETOP_FF) += hid-betopff.o >>> obj-$(CONFIG_HID_BIGBEN_FF) += hid-bigbenff.o >>> diff --git a/drivers/hid/hid-ayaneo.c b/drivers/hid/hid-ayaneo.c >>> new file mode 100644 >>> index 000000000..3a7af5918 >>> --- /dev/null >>> +++ b/drivers/hid/hid-ayaneo.c >>> @@ -0,0 +1,594 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * HID driver for the AYANEO 3 detachable controller ("Magic Modules"). >>> + * >>> + * The AYANEO 3 controller exposes three USB HID interfaces behind >>> + * VID 0x1c4f PID 0x0002 (a generic SigmaMicro ID, hence the DMI gate): >>> + * a gamepad, a keyboard for the extra buttons, and a vendor interface >>> + * (application usage 0xff000001) accepting 65-byte commands. >>> + * >>> + * This driver binds the vendor interface and provides: >>> + * - module identification (which module type is inserted on each side) >>> + * - software eject of the left/right modules >>> + * - RGB control of the joystick rings as a multicolor LED class device >>> + * >>> + * It complements the ayaneo-ec platform driver, which exposes module >>> + * attach state and controller power. A full eject is: write to this >>> + * driver's "eject" attribute, then power the controller off through >>> + * ayaneo-ec's controller_power once the eject completes. >>> + * >>> + * The protocol was reverse engineered in the Handheld Daemon project by >>> + * Antheas Kapenekakis. >>> + * >>> + * Command format (65 bytes, unnumbered report): >>> + * [0] report id (0) >>> + * [1:3] little-endian sum of bytes 7..64 >>> + * [3] command >>> + * [4] subcommand >>> + * [5:] payload >>> + * The device replies with a 64-byte report echoing the subcommand at >>> + * byte 3. >>> + * >>> + * Copyright (C) 2026 Matías Martínez >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#define AYA3_REPORT_SIZE 65 >>> +#define AYA3_RESP_SIZE 64 >>> + >>> +/* >>> + * Empirical timings, inherited from the Handheld Daemon >>> + * implementation of this protocol and validated on hardware: the >>> + * device answers well within 300ms or not at all, needs about half >>> + * a second to settle after a reset before it accepts a new >>> + * configuration, and completes an eject handshake within a few >>> + * seconds (polled below at a rate that keeps the sysfs write >>> + * responsive). >>> + */ >>> +#define AYA3_CMD_TIMEOUT_MS 300 >>> +#define AYA3_CMD_ATTEMPTS 3 >>> +#define AYA3_RESET_SETTLE_MS 500 >>> +#define AYA3_EJECT_POLL_MS 400 >>> +#define AYA3_EJECT_POLLS 20 >>> + >>> +/* Subcommands (byte 4); byte 3 is 0x00 except for the config command */ >>> +#define AYA3_SUBCMD_CHECK 0x08 >>> +#define AYA3_CMD_CONFIG 0x21 >>> +#define AYA3_SUBCMD_CONFIG 0x09 >>> + >>> +/* Bits that stay set in the eject status byte after an eject completes */ >>> +#define AYA3_EJECT_DONE_MASK 0x11 >>> + >>> +/* Config command eject/reset field */ >>> +#define AYA3_EJECT_LEFT 0x07 >>> +#define AYA3_EJECT_RIGHT 0x70 >>> +#define AYA3_RESET 0x88 >>> + >>> +/* Config command RGB modes */ >>> +#define AYA3_RGB_SOLID 0x01 >>> +#define AYA3_RGB_PULSE 0x02 >>> +#define AYA3_RGB_OFF 0xff >>> + >>> +/* Config command vibration levels, stored in the high nibble */ >>> +enum aya3_vibration { >>> + AYA3_VIBRATION_LOW = 0x1, >>> + AYA3_VIBRATION_MEDIUM = 0x2, >>> + AYA3_VIBRATION_HIGH = 0x3, >>> + AYA3_VIBRATION_OFF = 0x4, >>> +}; >>> + >>> +struct aya3_rgb { >>> + u8 mode; >>> + u8 r; >>> + u8 g; >>> + u8 b; >>> +} __packed; >>> + >>> +/* >>> + * The 65-byte config command. The checksum is the little-endian sum of >>> + * bytes 7..64; unk* fields are sent as zero. >>> + */ >>> +struct aya3_config { >>> + u8 report_id; >>> + __le16 csum; >>> + u8 cmd; >>> + u8 subcmd; >>> + u8 unk5[3]; >>> + struct aya3_rgb right; >>> + struct aya3_rgb left; >>> + u8 unk16[4]; >>> + u8 eject; >>> + u8 unk21; >>> + u8 sensitivity[2]; >>> + u8 vibration; >>> + u8 unk25[7]; >>> + u8 magic; >>> + u8 unk33[32]; >> I am not sure of the struct naming or whether a struct is needed in >> this case. if you do not use most of the report, consider documenting >> it somewhere else and doing direct accesses to the appropriate bytes. >> >>> +} __packed; >>> +static_assert(sizeof(struct aya3_config) == AYA3_REPORT_SIZE); >>> + >>> +/* Replies echo the subcommand they answer at byte 3 */ >>> +struct aya3_resp { >>> + u8 unk0[3]; >>> + u8 subcmd; >>> + u8 unk4[15]; >>> + u8 eject_status; >>> + u8 unk20[12]; >>> + u8 module_left; >>> + u8 module_right; >>> + u8 unk34[30]; >>> +} __packed; >>> +static_assert(sizeof(struct aya3_resp) == AYA3_RESP_SIZE); >>> + >>> +struct ayaneo { >>> + struct hid_device *hdev; >>> + /* DMA-safe command buffer; guarded by lock */ >>> + u8 *xfer; >>> + /* Serializes commands and cached-config access */ >>> + struct mutex lock; >>> + struct completion resp_done; >>> + struct aya3_resp resp; >>> + u8 resp_expect; >>> + bool resp_pending; >>> + >>> + u8 rgb[3]; >>> + bool pulse; >>> + u8 vibration; >>> + >>> + struct led_classdev_mc mcled; >>> + struct mc_subled subleds[3]; >>> +}; >>> + >>> +static int ayaneo_send(struct ayaneo *aya) >>> +{ >>> + int ret; >>> + >>> + ret = hid_hw_output_report(aya->hdev, aya->xfer, AYA3_REPORT_SIZE); >>> + if (ret == -ENOSYS) >>> + ret = hid_hw_raw_request(aya->hdev, aya->xfer[0], aya->xfer, >>> + AYA3_REPORT_SIZE, HID_OUTPUT_REPORT, >>> + HID_REQ_SET_REPORT); >>> + if (ret < 0) >>> + return ret; >>> + return 0; >>> +} >>> + >>> +/** >>> + * ayaneo_cmd() - send the command in aya->xfer and wait for the reply >>> + * @aya: driver data; @aya->xfer holds the fully built 65-byte command >>> + * @resp: destination for the reply, or NULL to discard it >>> + * >>> + * The device echoes the subcommand byte of the command it is answering, >>> + * which ayaneo_raw_event() uses to match replies. Unanswered commands are >>> + * retried up to AYA3_CMD_ATTEMPTS times. >>> + * >>> + * Context: process context; the caller must hold @aya->lock, which >>> + * protects @aya->xfer and the reply state. >>> + * Return: 0 on success, -ETIMEDOUT if every attempt went unanswered, or >>> + * a negative errno if sending failed. >>> + */ >>> +static int ayaneo_cmd(struct ayaneo *aya, struct aya3_resp *resp) >>> +{ >>> + int attempt, ret; >>> + >>> + lockdep_assert_held(&aya->lock); >>> + >>> + for (attempt = 0; attempt < AYA3_CMD_ATTEMPTS; attempt++) { >>> + reinit_completion(&aya->resp_done); >>> + aya->resp_expect = aya->xfer[4]; >>> + WRITE_ONCE(aya->resp_pending, true); >>> + >>> + ret = ayaneo_send(aya); >>> + if (ret) { >>> + WRITE_ONCE(aya->resp_pending, false); >>> + return ret; >>> + } >>> + >>> + if (wait_for_completion_timeout(&aya->resp_done, >>> + msecs_to_jiffies(AYA3_CMD_TIMEOUT_MS))) { >>> + if (resp) >>> + memcpy(resp, &aya->resp, sizeof(*resp)); >>> + return 0; >>> + } >>> + } >>> + WRITE_ONCE(aya->resp_pending, false); >>> + return -ETIMEDOUT; >>> +} >>> + >>> +static void ayaneo_checksum(u8 *buf) >>> +{ >>> + u16 sum = 0; >>> + int i; >>> + >>> + for (i = 7; i < AYA3_REPORT_SIZE; i++) >>> + sum += buf[i]; >>> + put_unaligned_le16(sum, buf + 1); >>> +} >>> + >>> +static int ayaneo_check(struct ayaneo *aya, struct aya3_resp *resp) >>> +{ >>> + memset(aya->xfer, 0, AYA3_REPORT_SIZE); >>> + aya->xfer[4] = AYA3_SUBCMD_CHECK; >>> + return ayaneo_cmd(aya, resp); >>> +} >>> + >>> +/* >>> + * The config command sets everything at once: RGB for both rings, >>> + * vibration strength and the eject/reset field. The command can also >>> + * carry joystick sensitivity; those bytes are left zero so the >>> + * firmware setting is not clobbered on every RGB update. >>> + */ >>> +static int ayaneo_send_config(struct ayaneo *aya, u8 eject) >>> +{ >>> + static const struct aya3_config template = { >>> + .cmd = AYA3_CMD_CONFIG, >>> + .subcmd = AYA3_SUBCMD_CONFIG, >>> + .magic = 0x01, >>> + }; >>> + struct aya3_config *cfg = (struct aya3_config *)aya->xfer; >>> + u8 mode = AYA3_RGB_OFF; >>> + >>> + if (aya->rgb[0] || aya->rgb[1] || aya->rgb[2]) >>> + mode = aya->pulse ? AYA3_RGB_PULSE : AYA3_RGB_SOLID; >>> + >>> + *cfg = template; >>> + cfg->right.mode = mode; >>> + cfg->right.r = aya->rgb[0]; >>> + cfg->right.g = aya->rgb[1]; >>> + cfg->right.b = aya->rgb[2]; >>> + cfg->left = cfg->right; >>> + cfg->eject = eject; >>> + cfg->vibration = aya->vibration << 4; >> If you set vibration, you need to expose it to userspace. Otherwise >> this driver degrades functionality over userspace implementations. >> >>> + ayaneo_checksum(aya->xfer); >>> + >>> + return ayaneo_cmd(aya, NULL); >>> +} >>> + >>> +static int ayaneo_raw_event(struct hid_device *hdev, struct hid_report *report, >>> + u8 *data, int size) >>> +{ >>> + struct ayaneo *aya = hid_get_drvdata(hdev); >>> + const struct aya3_resp *resp = (const struct aya3_resp *)data; >>> + >>> + if (!READ_ONCE(aya->resp_pending) || size < AYA3_RESP_SIZE) >>> + return 0; >>> + /* >>> + * Replies carry no sequence number, only the subcommand echo. A >>> + * late reply to a timed-out command can thus complete a newer >>> + * command with the same subcommand; such replies are snapshots >>> + * of the same query milliseconds apart, so this is harmless. >>> + * Replies to a different subcommand are dropped here. >>> + */ >>> + if (resp->subcmd != aya->resp_expect) >>> + return 0; >>> + >>> + memcpy(&aya->resp, data, sizeof(aya->resp)); >>> + WRITE_ONCE(aya->resp_pending, false); >>> + complete(&aya->resp_done); >>> + return 0; >>> +} >>> + >>> +static ssize_t ayaneo_module_show(struct device *dev, char *buf, bool right) >>> +{ >>> + struct ayaneo *aya = dev_get_drvdata(dev); >>> + struct aya3_resp resp; >>> + int ret = 0; >>> + >>> + scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) >>> + ret = ayaneo_check(aya, &resp); >>> + if (ret) >>> + return ret; >>> + >>> + return sysfs_emit(buf, "0x%02x\n", >>> + right ? resp.module_right : resp.module_left); >>> +} >>> + >>> +static ssize_t module_left_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + return ayaneo_module_show(dev, buf, false); >>> +} >>> +static DEVICE_ATTR_RO(module_left); >>> + >>> +static ssize_t module_right_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + return ayaneo_module_show(dev, buf, true); >>> +} >>> +static DEVICE_ATTR_RO(module_right); >>> + >>> +static ssize_t eject_store(struct device *dev, struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct ayaneo *aya = dev_get_drvdata(dev); >>> + struct aya3_resp resp; >>> + u8 eject; >>> + int ret = 0, err, i; >>> + >>> + if (sysfs_streq(buf, "left")) >>> + eject = AYA3_EJECT_LEFT; >>> + else if (sysfs_streq(buf, "right")) >>> + eject = AYA3_EJECT_RIGHT; >>> + else if (sysfs_streq(buf, "both")) >>> + eject = AYA3_EJECT_LEFT | AYA3_EJECT_RIGHT; >>> + else >>> + return -EINVAL; >>> + >>> + scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) { >>> + ret = ayaneo_send_config(aya, eject); >>> + if (ret) >>> + break; >>> + >>> + /* >>> + * Wait for the firmware to report the eject as done. >>> + * Userspace must then cut power through ayaneo-ec's >>> + * controller_power for the module to be physically >>> + * released. >>> + */ >>> + ret = -ETIMEDOUT; >>> + for (i = 0; i < AYA3_EJECT_POLLS; i++) { >>> + msleep(AYA3_EJECT_POLL_MS); >>> + err = ayaneo_check(aya, &resp); >>> + if (err == -ETIMEDOUT) >>> + continue; /* busy mid-eject, keep polling */ >>> + if (err) { >>> + ret = err; >>> + break; >>> + } >>> + if (!(resp.eject_status & ~AYA3_EJECT_DONE_MASK)) { >>> + ret = 0; >>> + break; >>> + } >>> + } >>> + } >>> + return ret ? ret : count; >>> +} >>> +static DEVICE_ATTR_WO(eject); >>> + >>> +static ssize_t reset_store(struct device *dev, struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct ayaneo *aya = dev_get_drvdata(dev); >>> + bool value; >>> + int ret; >>> + >>> + ret = kstrtobool(buf, &value); >>> + if (ret) >>> + return ret; >>> + if (!value) >>> + return count; >>> + >>> + scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) { >>> + ret = ayaneo_send_config(aya, AYA3_RESET); >>> + if (!ret) { >>> + msleep(AYA3_RESET_SETTLE_MS); >>> + ret = ayaneo_send_config(aya, 0); >>> + } >>> + } >>> + return ret ? ret : count; >>> +} >>> +static DEVICE_ATTR_WO(reset); >>> + >>> +static struct attribute *ayaneo_attrs[] = { >>> + &dev_attr_module_left.attr, >>> + &dev_attr_module_right.attr, >>> + &dev_attr_eject.attr, >>> + &dev_attr_reset.attr, >>> + NULL >>> +}; >>> +ATTRIBUTE_GROUPS(ayaneo); >>> + >>> +static int ayaneo_led_set(struct led_classdev *cdev, enum led_brightness value) >>> +{ >>> + struct led_classdev_mc *mc = lcdev_to_mccdev(cdev); >>> + struct ayaneo *aya = container_of(mc, struct ayaneo, mcled); >>> + int ret = 0, i; >>> + >>> + scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) { >>> + led_mc_calc_color_components(mc, value); >>> + for (i = 0; i < 3; i++) >>> + aya->rgb[i] = min_t(unsigned int, >>> + aya->subleds[i].brightness, 255); >>> + >>> + ret = ayaneo_send_config(aya, 0); >>> + if (ret) >>> + hid_err(aya->hdev, >>> + "failed to update RGB config: %d\n", ret); >>> + } >>> + return ret; >>> +} >>> + >>> +/* >>> + * The firmware offers one fixed breathing pattern, pulsing the current >>> + * colour at a period it controls. Expose it through the hw_pattern >>> + * trigger ABI as the two-step pattern "0 "; the >>> + * delta_t values and the repeat count are accepted but not tunable >>> + * (the firmware always repeats indefinitely). >>> + */ >> Prefer removing semicolons; they have a particular smell ;) >> >>> +static int ayaneo_pattern_set(struct led_classdev *cdev, >>> + struct led_pattern *pattern, u32 len, int repeat) >>> +{ >>> + struct led_classdev_mc *mc = lcdev_to_mccdev(cdev); >>> + struct ayaneo *aya = container_of(mc, struct ayaneo, mcled); >>> + int ret = 0; >>> + >>> + if (len != 2 || pattern[0].brightness || !pattern[1].brightness) >>> + return -EINVAL; >>> + >>> + scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) { >>> + aya->pulse = true; >>> + ret = ayaneo_send_config(aya, 0); >>> + } >>> + return ret; >>> +} >>> + >>> +static int ayaneo_pattern_clear(struct led_classdev *cdev) >>> +{ >>> + struct led_classdev_mc *mc = lcdev_to_mccdev(cdev); >>> + struct ayaneo *aya = container_of(mc, struct ayaneo, mcled); >>> + int ret = 0; >>> + >>> + scoped_cond_guard(mutex_intr, return -EINTR, &aya->lock) { >>> + aya->pulse = false; >>> + ret = ayaneo_send_config(aya, 0); >>> + } >>> + return ret; >>> +} >>> + >>> +static int ayaneo_register_led(struct ayaneo *aya) >>> +{ >>> + struct led_classdev *cdev = &aya->mcled.led_cdev; >>> + >>> + aya->subleds[0].color_index = LED_COLOR_ID_RED; >>> + aya->subleds[1].color_index = LED_COLOR_ID_GREEN; >>> + aya->subleds[2].color_index = LED_COLOR_ID_BLUE; >>> + aya->mcled.subled_info = aya->subleds; >>> + aya->mcled.num_colors = 3; >>> + >>> + cdev->name = devm_kasprintf(&aya->hdev->dev, GFP_KERNEL, >>> + "%s:rgb:joystick_rings", >>> + dev_name(&aya->hdev->dev)); >>> + if (!cdev->name) >>> + return -ENOMEM; >>> + cdev->color = LED_COLOR_ID_RGB; >>> + cdev->brightness = 0; >>> + cdev->max_brightness = 255; >>> + cdev->brightness_set_blocking = ayaneo_led_set; >>> + cdev->pattern_set = ayaneo_pattern_set; >>> + cdev->pattern_clear = ayaneo_pattern_clear; >>> + >>> + /* >>> + * Not devm: the LED must be unregistered before hid_hw_stop() in >>> + * remove, or a concurrent brightness write could reach a torn >>> + * down transport. >>> + */ >>> + return led_classdev_multicolor_register(&aya->hdev->dev, >>> + &aya->mcled); >>> +} >>> + >>> +static const struct dmi_system_id ayaneo_dmi_table[] = { >>> + { >>> + .matches = { >>> + DMI_MATCH(DMI_BOARD_VENDOR, "AYANEO"), >>> + DMI_MATCH(DMI_BOARD_NAME, "AYANEO 3"), >>> + }, >>> + }, >>> + {} >>> +}; >>> + >>> +static int ayaneo_probe(struct hid_device *hdev, const struct hid_device_id *id) >>> +{ >>> + struct ayaneo *aya; >>> + int ret; >>> + >>> + /* The VID/PID is a generic SigmaMicro ID; bind on AYANEO 3 only */ >>> + if (!dmi_check_system(ayaneo_dmi_table)) >>> + return -ENODEV; >>> + >>> + if (!hid_is_usb(hdev)) >>> + return -ENODEV; >>> + >>> + ret = hid_parse(hdev); >>> + if (ret) >>> + return ret; >>> + >>> + /* Bind only the vendor interface, not the gamepad/keyboard ones */ >>> + if (!hdev->maxcollection || >>> + hdev->collection->usage != (HID_UP_MSVENDOR | 0x0001)) >>> + return -ENODEV; >>> + >>> + aya = devm_kzalloc(&hdev->dev, sizeof(*aya), GFP_KERNEL); >>> + if (!aya) >>> + return -ENOMEM; >>> + >>> + aya->xfer = devm_kzalloc(&hdev->dev, AYA3_REPORT_SIZE, GFP_KERNEL); >>> + if (!aya->xfer) >>> + return -ENOMEM; >>> + >>> + aya->hdev = hdev; >>> + aya->vibration = AYA3_VIBRATION_MEDIUM; >>> + init_completion(&aya->resp_done); >>> + ret = devm_mutex_init(&hdev->dev, &aya->lock); >>> + if (ret) >>> + return ret; >>> + hid_set_drvdata(hdev, aya); >>> + >>> + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW); >>> + if (ret) >>> + return ret; >>> + >>> + ret = hid_hw_open(hdev); >>> + if (ret) >>> + goto err_stop; >>> + >>> + /* Input reports are not delivered during probe by default */ >>> + hid_device_io_start(hdev); >>> + >>> + scoped_guard(mutex, &aya->lock) >>> + ret = ayaneo_check(aya, NULL); >>> + if (ret) >>> + hid_warn(hdev, "controller did not answer status check: %d\n", >>> + ret); >> Consider dropping the hid_device ... check block unless it is >> necessary. it seems like a premature test that can go wrong and you >> touch the device. Particularly, hid_device_io_start is a bit >> unconventional. >> >> With this check removed, this driver does not touch the device without >> userspace involvement, which is good for userspace implementations >> such as mine. >> >> I think these are all the comments I have. I'd suggest waiting a week >> before the next revision and up to two weeks for jiri/Benjamin to >> reply with some comments as I think I was the only one that reviewed >> the previous revision. >> >> Best, >> Antheas >> >>> + >>> + ret = ayaneo_register_led(aya); >>> + if (ret) >>> + goto err_close; >>> + >>> + return 0; >>> + >>> +err_close: >>> + hid_hw_close(hdev); >>> +err_stop: >>> + hid_hw_stop(hdev); >>> + return ret; >>> +} >>> + >>> +static void ayaneo_remove(struct hid_device *hdev) >>> +{ >>> + struct ayaneo *aya = hid_get_drvdata(hdev); >>> + >>> + led_classdev_multicolor_unregister(&aya->mcled); >>> + /* >>> + * A brightness store racing with the unregister can requeue >>> + * set_brightness_work after the flush inside >>> + * led_classdev_unregister() runs but before the sysfs node is >>> + * removed. Flush again now that nothing can requeue it, while >>> + * the transport is still up. >>> + */ >>> + flush_work(&aya->mcled.led_cdev.set_brightness_work); >>> + hid_hw_close(hdev); >>> + hid_hw_stop(hdev); >>> +} >>> + >>> +static const struct hid_device_id ayaneo_devices[] = { >>> + { HID_USB_DEVICE(0x1c4f, 0x0002) }, >>> + {} >>> +}; >>> +MODULE_DEVICE_TABLE(hid, ayaneo_devices); >>> + >>> +static struct hid_driver ayaneo_driver = { >>> + .name = "hid-ayaneo", >>> + .id_table = ayaneo_devices, >>> + .probe = ayaneo_probe, >>> + .remove = ayaneo_remove, >>> + .raw_event = ayaneo_raw_event, >>> + .driver = { >>> + .dev_groups = ayaneo_groups, >>> + }, >>> +}; >>> +module_hid_driver(ayaneo_driver); >>> + >>> +MODULE_AUTHOR("Matías Martínez "); >>> +MODULE_DESCRIPTION("AYANEO 3 detachable controller driver"); >>> +MODULE_LICENSE("GPL"); >>> -- >>> 2.54.0 (Apple Git-157) >>> >>>