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

["Matías Martínez" <hello@matias.me>]

> If you need userspace coordination anyway, including the multiple
> timing hacks I had to implement, the question of having to route this
> through the kernel arises.

Fair question. What tipped it for me:

- The RGB wants to be a LED class device to be usable by the existing
  stacks (InputPlumber and friends consume /sys/class/leds, and with
  hw_pattern in v2 the breathing mode fits an accepted ABI). There is
  no hidraw equivalent short of every stack reimplementing the
  checksummed vendor protocol.

- The protocol and timing part (reply matching, retries, the eject
  handshake polling) now lives in one place. What remains in userspace
  is policy: when to cut controller power and what UX to wrap around
  it. That split also lets the module/eject controls be exposed as
  narrowly-scoped sysfs attributes instead of handing out the whole
  vendor interface through hidraw permissions.

- It complements ayaneo-ec, which already exposes attach state and
  controller power on the kernel side, so both halves of the flow sit
  at the same layer.

Working on this also flushed out a teardown bug that v2 fixes: a
brightness write racing a driver unbind could queue LED work that ran
after the transport was gone and the driver data freed. Reproducible
memory corruption under a write loop, and the window is reachable in
normal use, since the controller power-cycles on resume and on module
eject while userspace may be poking the LED.

> Overwriting joystick sensitivity is a bit problematic. Can you see if
> dropping those four bytes still allows RGB to go through? This might
> be preferable.

Confirmed on hardware: with bytes 22/23/37/38 left zero the firmware
still acks the config command, and RGB (solid and breathing) and eject
all work. v2 no longer writes them.

> Consider implementing the pulsing mode it offers, there should be an
> accepted ABI for it somewhere...

Done in v2 through the hw_pattern trigger ABI (pattern_set /
pattern_clear, same two-step shape as the sc27xx breathing pattern):
"0 <t> <brightness> <t>" selects the firmware's fixed-period breathing
at the current colour. Tested on the device, with an ABI document
added.

(v2 crossed your second mail in flight, so two things are still open
there:)

> Almost forgot. Magic value.
[...]
> Magic value. You need to justify those.

Right. Both are empirical firmware timings inherited from the
Handheld Daemon implementation (its reset sequence sleeps 0.5s
between the reset and the config restore, and it polls at a similar
cadence during eject); both are validated on hardware. Queued for v3
as named constants (AYA3_RESET_SETTLE_MS, AYA3_EJECT_POLL_MS/POLLS)
with a comment stating exactly that. I'll hold v3 briefly in case
more comes out of the v2 review.

Thanks for the review!

Matías