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

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

On 9/18/26 02:42, Derek J. Clark wrote:
> On September 17, 2026 2:48:11 PM PDT, Antheas Kapenekakis <lkml@antheas.dev> wrote:
>> On Thu, 17 Sept 2026 at 18:07, Matías Martínez <hello@matias.me> wrote:
>>> From: Matías Martínez <hello@matias.me>
>> 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@matias.me>
>>> Reviewed-by: Denis Benato <denis.benato@linux.dev>
>>> ---
>>> 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/<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@matias.me>
>>> +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@matias.me>
>>> +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@matias.me>
>>> +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@matias.me>
>>> +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@matias.me>
>>> +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 <lkml@antheas.dev>
>>>  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 <hello@matias.me>
>>> + */
>>> +
>>> +#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@matias.me>");
>>> +MODULE_DESCRIPTION("AYANEO 3 detachable controller driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.54.0 (Apple Git-157)
>>>
>>>