bcm5974: add Magic Trackpad 2 (USB only for now) support
ClosedPublic
Actions

Authored by val_packett.cool on Mar 3 2022, 11:32 PM.

Details

Reviewers

Group Reviewers

Contributor Reviews (src)
desktop

Commits

rGf560d0ead126: bcm5974(4): add Magic Trackpad 2 (USB only) support
rGef8397c28e98: bcm5974(4): add Magic Trackpad 2 (USB only) support

Summary

The MT2 uses a compact report format, but otherwise is similar in many ways to the internal trackpads, it even uses the same mode switching commands. (With an extra byte via bluetooth for some reason :/ but exactly the same via USB) I'm not sure if handling it in the same driver is the correct way to go, but it was easy to add here.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

val_packett.cool created this revision.Mar 3 2022, 11:32 PM

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptMar 3 2022, 11:32 PM

val_packett.cool requested review of this revision.Mar 3 2022, 11:32 PM

Harbormaster completed remote builds in B44644: Diff 103470.Mar 3 2022, 11:32 PM

val_packett.cool edited the summary of this revision. (Show Details)Mar 3 2022, 11:35 PM

val_packett.cool added reviewers: wulf, Contributor Reviews (src), desktop.

wulf added inline comments.Mar 4 2022, 10:42 AM

sys/dev/hid/bcm5974.c
192	AFAIK bitfields are not endian-safe. Replace this block with single uint32_t and use shifts with bitmasks
199	ditto
380	Size member should be 0 for pressure
781	No need to set EVDEV_FLAG_MT_TRACK flag while initializing evdev if device already exports good tracking id
788	Just do not declare ABS_MT_WIDTH_MAJOR/ABS_MT_WIDTH_MAJOR events during initialization instead of setting them to 0 here.

Endian convert the initial 32 bit field, use given IDs, do not report width major/minor

Harbormaster completed remote builds in B44655: Diff 103492.Mar 4 2022, 3:30 PM

val_packett.cool marked 2 inline comments as done.Mar 4 2022, 3:32 PM

val_packett.cool added inline comments.

sys/dev/hid/bcm5974.c
192	Reading a signed 13 bit number with shifts and bitmasks sounds complicated, I used a union to read from an endian-converted uint32_t.
199	The second bitfield piece is one byte.
380	Just like wellspring9 this is truly force sensitive so resolution should be non-zero.

wulf added inline comments.Mar 4 2022, 10:07 PM

sys/dev/hid/bcm5974.c
199	The second bitfield piece is one byte. On BE archs not bytes are stored in opposite order as compared to LE, but structure members. See e.g. https://github.com/freebsd/freebsd-src/blob/main/sys/dev/usb/usbdi.h#L273 . So shifts/masks should be used here too. Alternatively, usbdi.h trick with reversing of member order can be used. It looks better than shifts/masks but just now I have no access BE arch to test it especially for members which crosses byte boundaries.