Page MenuHomeFreeBSD

gpiokeys: first take at evdev support
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg requested review of this revision.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.

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

LGTM from evdev point of view

sys/dev/gpio/gpiokeys.c
237 ↗(On Diff #75340)

Indeed, autorepeat support looks incomplete. I see no visible connection between kb_delayN and repeat_delay variables.

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.

This revision is now accepted and ready to land.Aug 6 2020, 10:56 PM
This revision was automatically updated to reflect the committed changes.