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 Passed

Unit

No Test Coverage

Build Status

Buildable 32739
Build 30181: arc lint + arc unit

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	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	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	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	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	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	Will do!
237	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	Okay.
346	Yes, I got it.
413	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
237	Indeed, autorepeat support looks incomplete. I see no visible connection between kb_delayN and repeat_delay variables.
489	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.