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

[Matías Martínez]

On Fri, 25 Sept 2026 at 12:01, Antheas Kapenekakis <lkml@xxxxxxxxxxx> wrote:
> >   Antheas suggested generalizing the lenovo-go file instead, but that
> >   left it internally inconsistent (its other entries on the same LED
> >   keep the go: prefix) and placed this driver's ABI outside its
> >   MAINTAINERS entry, so the lenovo-go-s model won.
>
> That is fine. You should have asked for me to clarify. This driver
> does not need to own that sysfs entry.

Got it, plan for v5 below, under your ABI comment.

> > - Default the subled intensities to full so brightness writes produce
> >   light before userspace configures colours.
>
> You mean the opposite? Writes to colors produce full brightness if
> brightness is not written? Because the other way is not a problem. You
> can skip led writes if a color has not been provided.

No: the multicolor class computes each channel as
brightness * intensity / max_brightness (led_mc_calc_color_components),
and the subled intensities start at zero. Without this change a plain
"brightness" write reports the LED as on while emitting nothing until
multi_intensity is also written. With intensities defaulting to full, a
bare brightness write produces white at that brightness, and a
multi_intensity write scales it as usual. Nothing lights up
spontaneously: brightness starts at 0 and the driver sends no command
until userspace writes something.

Skipping the device write when no colour was provided keeps that
surprising state: brightness-only users (desktop sliders, triggers)
would write brightness and see nothing. hid-lenovo-go-s also starts
from non-zero subled intensities; it can read the current colour back
from the firmware, which this device cannot (the protocol has no
config read-back), so a fixed default is the only option, and white
seemed the least surprising one.

> > +What:          /sys/class/leds/<dev>:rgb:joystick_rings/effect
> [...]
> > +What:          /sys/class/leds/<dev>:rgb:joystick_rings/effect_index
>
> You cannot add a new sysfs entry for an existing ABI. You must correct
> the original one or move the led registrations to their own unique
> file and correct them.

Noted for v5. I will make it a two-patch series again: patch 1 adds
Documentation/ABI/testing/sysfs-class-led-effect describing
/sys/class/leds/<led>/effect and effect_index generically (the shared
value names — monocolor, breathe, chroma, rainbow — defined once,
effect_index listing what each device supports), and removes those
entries from sysfs-driver-hid-lenovo-go and -go-s, which currently
duplicate each other. Patch 2 then adds nothing LED-related to this
driver's ABI file. Should the other duplicated LED entries (enabled,
enabled_index) move in the same patch, or only effect/effect_index?

> Too much detail in comments. Obvious observations need not be listed.
> This applies to most comments in the current driver. E.g., the generic
> vid/pid note may remain.

Will definitely trim for v5.

> Review the use of reset_resume. It is only used for buggy devices.
> Doesn't resume work? Is it needed in your usage? does the device not
> restore colors after sleep?

Ordinary resume is fine and the driver does not hook it. reset_resume
is a different path: usbhid's hid_reset_resume only runs when the
device was reset while suspended (post hid_post_reset), i.e. the
firmware state is gone no matter how well normal resume behaves.

Two uses here. On the transparent interfaces it mirrors hid-generic,
which registers reset_resume itself (hid_generic_reset_resume ->
hidinput_reset_resume); since this driver takes over interfaces
hid-generic would otherwise drive, dropping it would regress keyboard
LED-state restoration after a reset. On the vendor interface it
reapplies the cached configuration for the same reason, and only if
userspace had successfully applied one ("configured"), so an untouched
device stays untouched.

> I think these are all my comments. Given you are still doing changes
> for stability's shake, this will need to be tested for a bit longer.

Agreed. The v4 driver is now soaking on the distribution kernel this
device ships with, and v5 will wait for that plus any maintainer
comments.

Thanks for the review and for all of your help,
Matías