gpiokeys: first take at evdev support
ClosedPublic
Actions

Authored by avg on Aug 4 2020, 6:27 AM.

Details

Reviewers

ganbold
gonzo
manu
wulf

Commits

rS364145: gpiokeys: add evdev support

Summary

First take at evdev support.
Only linux,code is supported. No reverse mapping for freebsd,code yet.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

avg created this revision.Aug 4 2020, 6:27 AM

Herald added a subscriber: imp. · View Herald TranscriptAug 4 2020, 6:27 AM

avg requested review of this revision.Aug 4 2020, 6:27 AM

Harbormaster completed remote builds in B32739: Diff 75340.Aug 4 2020, 6:27 AM

I'm not too familiar with evdev but will that create just one /dev/input/ for each gpio-keys compatible node or one per child node ?

Good point!
The current code creates a device per each button, that is, a child node.

I tested this on an a board with only a single button, so it did not make any difference for me.

In D25940#574828, @avg wrote:

Good point!
The current code creates a device per each button, that is, a child node.

I tested this on an a board with only a single button, so it did not make any difference for me.

Ok.
I don't know what linux does but I think that we should have one device per gpio-keys node, will all button registered as a key there.
https://svnweb.freebsd.org/base/head/sys/gnu/dts/arm64/rockchip/rk3399-pinebook-pro.dts?revision=361848&view=markup#l56
I think that's this is the only way we should create two devices, having two nodes.
So the driver should evdev_alloc once and register the keys.

wulf added inline comments.Aug 5 2020, 12:18 AM

sys/dev/gpio/gpiokeys.c
189 ↗	(On Diff #75340)	evdev_rcpt_mask global variable should be checked here for EVDEV_RCPT_HW_KBD bit set. Or events will be duplicated if kbdmux evdev is enabled.
237 ↗	(On Diff #75340)	As driver has internal support for autorepeats, it is good to map evdev typematics ioctl to kbd ones. See evdev_set_methods() usage in e.g. kbdmux.c
281 ↗	(On Diff #75340)	INPUT_PROP_DIRECT is not applicable here. It means that device reported coordinates can be directly mapped to display surface. I.e. device is touchscreen.
346 ↗	(On Diff #75340)	only evdev_support_key() should be left here. Other parts gpiokeys_evdev_register_key() belongs gpiokeys_attach() to not create evdev foreach button as @manu has stated.
413 ↗	(On Diff #75340)	evdev_free() must be used in gpiokeys_detach(). evdev_unregister() destroys cdev only.

Thank you all for the reviews and suggestions.

sys/dev/gpio/gpiokeys.c
189 ↗	(On Diff #75340)	Will do!
237 ↗	(On Diff #75340)	I am very new to keyboards and evdev, so I am not sure what that does and how to do it properly in gpiokeys. I'll probably leave that for a future improvement. By the way, it seems that most / real keyboards have global autorepeat settings. But for gpio-keys it's possible to specify autorepeat settings for each individual "key" (input). Not sure if that would work well with evdev. Also, I am confused by the fact that gpiokeys now has some per-key autorepeat handling (based on parameters in FDT) and also supports keyboard ioctl-s that set kb_delay1 / kb_delay2 variables. Maybe @gonzo can help to understand how that works.
281 ↗	(On Diff #75340)	Okay.
346 ↗	(On Diff #75340)	Yes, I got it.
413 ↗	(On Diff #75340)	Okay.

address some of the comments

Harbormaster completed remote builds in B32800: Diff 75482.Aug 6 2020, 11:13 AM

LGTM from evdev point of view

sys/dev/gpio/gpiokeys.c
463 ↗	(On Diff #75482)	Typically, Linux set sysfs path as phys property and we set result of device_get_nameunit(dev) to expose newbus device backing the evdev character device to userland.
237 ↗	(On Diff #75340)	Indeed, autorepeat support looks incomplete. I see no visible connection between kb_delayN and repeat_delay variables.