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

[Antheas Kapenekakis]

On Fri, 18 Sept 2026 at 00:23, Matías Martínez <hello@xxxxxxxxx> wrote:
>
> 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.

As I said, none of this is your responsibility.

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

If there is an existing ABI, you shouldnt add a new sysfs doc file.

https://github.com/torvalds/linux/blob/b5a051f6b840d48f159166ef073d3021989bfb50/Documentation/ABI/testing/sysfs-driver-hid-lenovo-go#L1-L13

There is an existing class of device in the same space with a new ABI
that does exactly what this device needs. I am really not sure why you
were guided on a LED PWM interface by the author of that ABI.

You need to modify that sysfs file in a separate patch to make it
generic by removing go.

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

I am also fine either way. I'd prefer what is simpler.

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

All occurances.

Thanks,
Antheas

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