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

[Matías Martínez]

On Thu, 17 Sept 2026 at 23:48, Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote:

Hi Antheas,

Thanks for going through v3, and for all the help along the way. This
driver exists because of your protocol work in Handheld Daemon and
your reviews of every revision here.

> After you submitted your patch series to the lore, why didn't the
> review take place here and take place downstream?

Sequencing rather than intent. The OGC kernel folks wanted the driver
submitted and tested in their tree first, and their maintainer's
review landed on the already-merged downstream PR after v1 and v2
were on the list. I folded it into v3 rather than ignoring it. Agreed
that from here on review belongs on the list.

> 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).

Understood, I carried it over from his downstream review round.
Denis, if you are still fine with the driver as of v3, could you
confirm your Reviewed-by here? Otherwise I will drop the tag in v4.

> 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?

The delta_t point is fair: hw_pattern accepts timing values this
firmware cannot honor, and the sc27xx precedent does use them. For
context, hw_pattern was my reading of your v1 suggestion to find an
accepted ABI for the pulsing mode, and the downstream review
preferred the generic trigger ABI over adding a custom effect
attribute, so the two reviews point in opposite directions here.
hid-lenovo-go-s documents its effect/effect_index attributes in its
own ABI file, so a doc file exists on that path too, though shared
with a second driver it could become a common one. I am happy to
switch to effect/effect_index mirroring hid-lenovo-go-s (values
monocolor and breathe) if that is the preferred direction. Jiri,
Benjamin, a preference either way would settle this.

> 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.

The packed structs came from the downstream review, mirroring
hid-lenovo-go; they replaced a set of byte-offset defines and the
static_asserts pin the wire sizes. I would mildly prefer to keep
them, but I am fine either way if the maintainers have a preference.

> If you set vibration, you need to expose it to userspace. Otherwise
> this driver degrades functionality over userspace implementations.

Agreed. v4 adds an attribute for the vibration level (the firmware
takes three) plus an index listing the options, instead of writing a
hardcoded default.

> Prefer removing semicolons; they have a particular smell ;)

Will reword the comment.

> 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.

Agreed, v4 drops the probe-time status check and with it the
hid_device_io_start call. Every sysfs read already talks to the
device live, so the check adds nothing, and without it the driver
generates no traffic unless userspace asks, which also keeps it inert
next to userspace implementations like yours.

> 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.

Will do. v4 waits for Jiri and Benjamin, and goes out as a fresh
thread.

Thanks again,
Matías