drivers: input: Input subsystem #48342

MichaelOwen2022 · 2022-07-27T03:29:13Z

Input subsystem support api for input devices (like keyboard, touscreen, etc).

drivers/key/key_gpio.c

mbolivar-nordic

In addition to the problems here, you need to fix your commit logs.

Please watch https://www.youtube.com/watch?v=mnVNLM7ynMQ or at least read the slides for an overview of the contribution process: https://static.sched.com/hosted_files/zephyr2022/a3/HOWTO_Get_Your_Zephyr_Patches_Merged_ZDS_2022.pdf

drivers/key/CMakeLists.txt

drivers/key/Kconfig

drivers/key/key_handlers.c

dts/bindings/gpio/gpio-keys.yaml

include/zephyr/drivers/key.h

include/zephyr/dt-bindings/key/key.h

samples/basic/gpio_key/src/main.c

dcpleung

Can I suggest to rename this to keyboard instead of just key? This would make it more clear that these are keyboard related stuff.

drivers/key/key_handlers.c

str4t0m · 2022-08-22T15:53:12Z

Tagging @petejohanson as he might be interested and have some feedback.

petejohanson · 2022-08-22T16:33:26Z

I think if this is intended just for simple "send a key" uses, this will work, but for anything as complex as https://github.com/zmkfirmware/zmk/ this abstraction would not work for us.

ZMK settled on using the existing kscan driver API, and implemented both matrix and direct wire versions. There seems a lot of overlap (potentially) between this new API and kscan.
By using KSCAN + GPIO abstraction that exists, our existing matrix driver, for instance, works fine with IO expanders, shift registers, etc. This, in theory, might also be able to do that, but something to be aware of.
On the above point this seems to mix a few concerns:
- Assignment of "key values" to different hardware.
- Debouncing
- Hardware details
Debounce can actually be complicated, and should ideally not be duplicated. Eager debounce can be used to get key presses sent earlier, and debounce the release instead, etc.

Just a few thoughts, I'll think more and add more when I can.

MichaelOwen2022 · 2022-08-23T06:19:39Z

@petejohanson @dcpleung
Whether this commit has a chance to be merged?
If this commit doesn't have a chance to merge, I will close this pull request.
If I implement a new input module instand of kscan, userspace use poll mothed to get input events, like linux input subsystem. Is that can be merged?

dcpleung · 2022-08-23T16:02:10Z

My issue is simply that setting up the callback should not be done from user thread. It can be done in kernel mode.

MichaelOwen2022 · 2022-08-24T02:41:07Z

My issue is simply that setting up the callback should not be done from user thread. It can be done in kernel mode.

I mean, If I solve the callback program, is there a chance to merge? this pull request is highly similar to kscan driver.

@dcpleung

carlescufi · 2022-12-05T10:57:27Z

@MichaelOwen2022 sorry, this flew under my radar. Could you please rebase?
I've asked for a re-review from the other reviewers.

no longer working on dt bindings

Input subsystem support public API for input device. Signed-off-by: Michael Owen <[email protected]>

This driver scan gpio keyboard status, and report key event to input subsystem. Signed-off-by: Michael Owen <[email protected]>

This sample use input api to get gpio keyboard event. Signed-off-by: Michael Owen <[email protected]>

MichaelOwen2022 · 2022-12-17T09:09:36Z

@MichaelOwen2022 sorry, this flew under my radar. Could you please rebase? I've asked for a re-review from the other reviewers.

I've rebase already.

tbursztyka

Input is a way too generic name, a keyboard being one of the many input devices (and we already have kscan for this).
Hard to see how this could be used for a mouse, or else...

tbursztyka · 2022-12-23T07:39:14Z

drivers/input/input_internal.c

+
+#include "input_internal.h"
+
+int input_internal_setup(struct input_dev *dev)


From one input device stand-point: are you going to call this more than once at runtime?

If not, then all of this can be integrated into a macro with relevant static intialization for each objects (rb, mutex, sem...)

tbursztyka · 2022-12-23T07:40:23Z

include/zephyr/drivers/input.h

+ */
+__syscall int input_setup(const struct device *dev);
+
+static inline int z_impl_input_setup(const struct device *dev)


Why can't this be done at device initialization? (see previous comment)

tbursztyka · 2022-12-23T07:41:46Z

include/zephyr/drivers/input.h

+ */
+__syscall int input_event_read(const struct device *dev, struct input_event *event);
+
+static inline int z_impl_input_event_read(const struct device *dev, struct input_event *event)


as noted already, adding asynchronous input API would be a must here.

tbursztyka · 2022-12-23T07:48:49Z

drivers/input/keyboard/keyboard_gpio.c

+	KEY_EVENT_NONE = 0xff,
+};
+
+struct kbd_gpio_driver {


weird naming, the whole code in keyboard_gpio.c is the driver.
Actually I don't see the point of not including of these attributes into kbd_gpio_data

tbursztyka · 2022-12-23T07:50:24Z

drivers/input/keyboard/keyboard_gpio.c

+	struct input_dev *input = data->input;
+	int retval = 0;
+
+	retval = input_internal_attr_get(input, type, attr);


retval seems superfluous, just return from the called input_* function

tbursztyka · 2022-12-23T07:50:35Z

drivers/input/keyboard/keyboard_gpio.c

+	struct input_dev *input = data->input;
+	int retval = 0;
+
+	retval = input_internal_attr_set(input, type, attr);


tbursztyka · 2022-12-23T07:50:41Z

drivers/input/keyboard/keyboard_gpio.c

+	struct input_dev *input = data->input;
+	int retval = 0;
+
+	retval = input_internal_event_read(input, event);


tbursztyka · 2022-12-23T07:50:46Z

drivers/input/keyboard/keyboard_gpio.c

+	struct input_dev *input = data->input;
+	int retval = 0;
+
+	retval = input_internal_event_write(input, event);


tbursztyka · 2022-12-23T07:55:08Z

include/zephyr/drivers/input.h

+ */
+__syscall int input_release(const struct device *dev);
+
+static inline int z_impl_input_release(const struct device *dev)


what would be the use case for this function?

Like the setup above. It seems to bring concepts (pluggin in/out a keyboard? pm perhaps?) to the user level that should not be its role to manage. Which is not the right way.

MichaelOwen2022 · 2022-12-28T13:27:23Z

Input is a way too generic name, a keyboard being one of the many input devices (and we already have kscan for this). Hard to see how this could be used for a mouse, or else...

With all due respect, I don't think you're good enough to understand this input subsysem framework.

petejohanson · 2022-12-28T14:31:53Z

drivers/input/keyboard/keyboard_gpio.c

+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#define DT_DRV_COMPAT keyboard_gpio


I still don't know why this isn't done as a kscan driver, and this input system doesn't have a mapping to consume a kscan driver and map to input system events.

I don't think you understand enough of this input subsystem framework either.

petejohanson · 2022-12-28T14:33:01Z

drivers/input/keyboard/keyboard_gpio.c

+			continue;
+		}
+
+		err = gpio_pin_interrupt_configure_dt(gpio, GPIO_INT_EDGE_TO_ACTIVE);


Not all gpio pins support interrupts. This will have limitations and only work on certain hardware configurations.

Also, using edge to active interrupt won't allow deep sleep/wake from deep sleep on nRF52.

petejohanson · 2022-12-28T14:37:12Z

include/zephyr/drivers/input.h

+enum keyboard_value {
+	KEY_RELEASE,
+	KEY_PRESSED,
+	KEY_LONG_PRESSED,


Why are these added. As soon as you have long press/release, why not support double tap? Triple?

If the goal is a "lowest common denominator" API good enough for basic input to programs, this is probably fine.

I think maybe this is my disconnect, I come from developing featureful input devices.

If the goal of this API is to focus on the consumers of input data, then we should clearly document that, since the current system doesn't seem to be flexible/powerful enough to handle the other.

petejohanson · 2022-12-28T14:39:35Z

include/zephyr/dt-bindings/input/input.h

+
+/* keyboard code */
+
+#define KEY_CODE_RESERVED   0


The current code set may be fine for a basic sample program, but definitely won't allow a complex system that covers the full range of possible keyboard codes, other possible layouts/languages, etc.

petejohanson · 2022-12-28T14:40:01Z

include/zephyr/dt-bindings/input/input.h

+#define KEY_CODE_Y          89
+#define KEY_CODE_Z          90
+
+#define KEY_CODE_KP0        96


There's far more keypad codes than these.

MichaelOwen2022 · 2022-12-28T16:38:56Z

It's been a few months since this commit and the code review that followed was disappointing. I can't speak highly of the entire zephyr team and have no interest in committing anything on this project.

MichaelOwen2022 · 2022-12-29T02:54:24Z

It's no fun to waste time with a bunch of elementary school students。

MichaelOwen2022 requested review from MaureenHelm, carlescufi, galak and nashif as code owners July 27, 2022 03:29

zephyrbot added the area: Devicetree label Jul 27, 2022

zephyrbot requested a review from mbolivar-nordic July 27, 2022 03:29

MichaelOwen2022 force-pushed the feature branch 2 times, most recently from 22ddd28 to ed9cad3 Compare July 28, 2022 02:16

rosterloh reviewed Aug 12, 2022

View reviewed changes

drivers/key/key_gpio.c Outdated Show resolved Hide resolved

mbolivar-nordic previously requested changes Aug 18, 2022

View reviewed changes

MichaelOwen2022 force-pushed the feature branch from ed9cad3 to d5ca8a3 Compare August 22, 2022 06:54

zephyrbot added the area: Documentation label Aug 22, 2022

zephyrbot requested review from gmarull and utzig August 22, 2022 06:55

MichaelOwen2022 requested review from mbolivar-nordic and removed request for MaureenHelm, carlescufi, galak, gmarull, nashif and utzig August 22, 2022 06:55

dcpleung requested changes Aug 22, 2022

View reviewed changes

drivers/key/key_handlers.c Outdated Show resolved Hide resolved

MichaelOwen2022 force-pushed the feature branch from d5ca8a3 to fc649d9 Compare August 24, 2022 06:28

zephyrbot requested review from carlescufi and galak August 24, 2022 06:29

MichaelOwen2022 requested review from desowin and mbolivar-nordic and removed request for jfischer-no and mbolivar-nordic October 14, 2022 03:44

gmarull mentioned this pull request Dec 2, 2022

Develop an Input subsystem for Zephyr #52732

Closed

carlescufi requested review from dcpleung, jfischer-no, mbolivar-nordic, pdunaj and rosterloh December 5, 2022 10:56

mbolivar-nordic requested review from galak and removed request for mbolivar-nordic and rosterloh December 6, 2022 18:10

Michael Owen added 3 commits December 17, 2022 17:04

drivers: input: add input subsystem.

59d37d2

Input subsystem support public API for input device. Signed-off-by: Michael Owen <[email protected]>

drivers: input: keyboard: add gpio keyboard driver.

9469b99

This driver scan gpio keyboard status, and report key event to input subsystem. Signed-off-by: Michael Owen <[email protected]>

samples: keyboard: add gpio keyboard sample.

32ac382

This sample use input api to get gpio keyboard event. Signed-off-by: Michael Owen <[email protected]>

MichaelOwen2022 force-pushed the feature branch from cfd44e5 to 32ac382 Compare December 17, 2022 09:05

zephyrbot requested review from carlescufi and nashif December 17, 2022 09:05

tbursztyka requested changes Dec 23, 2022

View reviewed changes

petejohanson reviewed Dec 28, 2022

View reviewed changes

MichaelOwen2022 closed this Dec 28, 2022

o7-machinehum mentioned this pull request Jan 11, 2023

Implemented iqs5xx driver #52649

Closed

carlescufi mentioned this pull request Feb 14, 2023

[RFC][Draft][DNM] Input Subsystem proof-of-concept #54620

Closed


		#include "input_internal.h"

		int input_internal_setup(struct input_dev *dev)

drivers: input: Input subsystem #48342

drivers: input: Input subsystem #48342

Uh oh!

Conversation

MichaelOwen2022 commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mbolivar-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dcpleung left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

str4t0m commented Aug 22, 2022

Uh oh!

petejohanson commented Aug 22, 2022

Uh oh!

MichaelOwen2022 commented Aug 23, 2022

Uh oh!

dcpleung commented Aug 23, 2022

Uh oh!

MichaelOwen2022 commented Aug 24, 2022

Uh oh!

carlescufi commented Dec 5, 2022

Uh oh!

MichaelOwen2022 commented Dec 17, 2022

Uh oh!

tbursztyka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelOwen2022 commented Dec 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelOwen2022 commented Dec 28, 2022

Uh oh!

MichaelOwen2022 commented Dec 29, 2022

Uh oh!

Reviewers

Assignees

Labels

MichaelOwen2022 commented Jul 27, 2022 •

edited

Loading