Re: [PATCH v3] HID: ayaneo: Add AYANEO 3 detachable controller driver

[Derek J. Clark]

On September 18, 2026 12:52:38 AM PDT, Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote:
>On Fri, 18 Sept 2026 at 02:42, Derek J. Clark <derekjohn.clark@xxxxxxxxx> wrote:
>>
>> On September 17, 2026 2:48:11 PM PDT, Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote:
>> >On Thu, 17 Sept 2026 at 18:07, Matías Martínez <hello@xxxxxxxxx> wrote:
>> >>
>> >> From: Matías Martínez <hello@xxxxxxxxx>
>> >
>> >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 ("<device name>: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 <hello@xxxxxxxxx>
>> >> Reviewed-by: Denis Benato <denis.benato@xxxxxxxxx>
>> >> ---
>> >> 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).
>> >
>> >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.
>
>If Denis reviewed it prior to V1 this is ok. It still does not explain
>why you reviewed the PR after V1. I checked. Both V1 and V2 were
>posted on Aug 24. Your feedback was on the 28th

Antheas,

I already told you I wasn't aware that it had been posted the LKML when I reviewed it on GitHub. I'm not going to engage with this line of conversation further.

>What you did is a disservice to Matias and to us. Matias wants to see
>his device work under Bazzite. You set the arbitrary requirement of
>kernel drivers being needed for hid writes and that those drivers
>_need_ to be on the mailing list before you merge them. So, his device
>broke in a recent update. Then, Matias spends his personal time to
>develop such a kernel driver and posts it to the mailing list.
>
>Instead of getting thorough reviews here and support to show this
>driver has traction, you conduct your review on a misc repo after
>getting your arbitrary ML requirement met. This is a disservice to
>Matias. And we see a driver without any traction and conduct
>duplicative reviews. This is a disservice to us.

>> Without your feedback necessary we've already implemented better control policies to avoid this in the future.
>
>Explain how you will avoid this situation in the future

No.

Derek

>Antheas
>
>> 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/<led>/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 <hello@xxxxxxxxx>
>> >> +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 <t> <brightness> <t>"
>> >> +
>> >> +               Both delta_t values are accepted but ignored, as the
>> >> +               period is not configurable. <brightness> 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/<dev>/module_left
>> >> +What:          /sys/bus/hid/drivers/hid-ayaneo/<dev>/module_right
>> >> +Date:          August 2026
>> >> +KernelVersion: 7.3
>> >> +Contact:       Matías Martínez <hello@xxxxxxxxx>
>> >> +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/<dev>/eject
>> >> +Date:          August 2026
>> >> +KernelVersion: 7.3
>> >> +Contact:       Matías Martínez <hello@xxxxxxxxx>
>> >> +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/<dev>/reset
>> >> +Date:          August 2026
>> >> +KernelVersion: 7.3
>> >> +Contact:       Matías Martínez <hello@xxxxxxxxx>
>> >> +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 <hello@xxxxxxxxx>
>> >> +L:     linux-input@xxxxxxxxxxxxxxx
>> >> +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 <lkml@xxxxxxxxxxx>
>> >>  L:     platform-driver-x86@xxxxxxxxxxxxxxx
>> >> 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 <hello@xxxxxxxxx>
>> >> + */
>> >> +
>> >> +#include <linux/build_bug.h>
>> >> +#include <linux/cleanup.h>
>> >> +#include <linux/delay.h>
>> >> +#include <linux/dmi.h>
>> >> +#include <linux/hid.h>
>> >> +#include <linux/led-class-multicolor.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/mutex.h>
>> >> +#include <linux/sysfs.h>
>> >> +#include <linux/unaligned.h>
>> >> +#include <linux/workqueue.h>
>> >> +
>> >> +#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 <t> <brightness> <t>"; 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 <hello@xxxxxxxxx>");
>> >> +MODULE_DESCRIPTION("AYANEO 3 detachable controller driver");
>> >> +MODULE_LICENSE("GPL");
>> >> --
>> >> 2.54.0 (Apple Git-157)
>> >>
>> >>
>> >
>>
>>
>