Re: [PATCH] HID: pulsar: add driver for Pulsar gaming mice

[Nikolas Koesling <nikolas@koesling.info>]

Hi Leo,

thanks for the review.

> While this driver looks fine at a glance, this does seem to use the
> same protocol as the hid-kysona driver. It might be more appropriate to
> extend that driver instead of rolling a new one? The HID identifiers in
> hid-ids.h already have a vendor constant for USB_VENDOR_ID_KYSONA,
> which takes the same value as this patch's USB_VENDOR_ID_PULSAR.

Same vendor ID, same protocol. I should have caught the overlap with
hid-kysona.

I don't think simply adding Pulsar IDs to hid-kysona as-is would be sufficient.
The current driver has some issues I addressed in my patch:
 
1. No locking: raw_event writes battery state concurrently with get_property
reads
2. raw_event always returns 0, so battery/online status responses are never
consumed by the driver. They leak through to userspace as spurious input
events. hid-pulsar uses pending_event + completion to match commands to
responses and consumes responses to driver requests.
3. the protocol includes a checksum, but hid-kysona never validates it on
incoming data.
4. hid-kysona does not react in any way to the power change event messages
from the device.

> For context, I was investigating exactly this battery reporting for
> another mouse, the ATK VXE R1 SE+. It does seem to use the exact same
> protocol; in wired mode, that takes the device ID 3554:F58F, while the
> wireless dongle has a device ID of 373B:1085. I plan on sending a patch
> to add these IDs after some testing.

With three vendors sharing the same protocol, a vendor-neutral name would make
sense. Any suggestions? Something like hid-paw3395-battery (sensor),
hid-nrf52840-mouse (SoC), or is there a better identifier for this protocol?

What would you prefer: rework and rename hid-kysona in place, or rename and
extend hid-pulsar while keeping hid-kysona as-is for compatibility?

Kind regards,
Nikolas