drivers: input: Implement driver for ADC keys #68446

xingrz · 2024-02-02T04:00:03Z

This commit introduces a driver for ADC keys, a common circuit design where keys are connected to an ADC input via a resistor ladder.

The driver itself does not calculate each possible combination of resistor values. Instead, users are required to specify the voltage for each single key press or for combinations of key presses.

Example 1

The schematic of a common design:

An example DTS:

&adc1 {
	#address-cells = <1>;
	#size-cells = <0>;
	status = "okay";

	channel@4 {
		reg = <4>;
		zephyr,gain = "ADC_GAIN_1";
		zephyr,reference = "ADC_REF_INTERNAL";
		zephyr,acquisition-time = <ADC_ACQ_TIME_DEFAULT>;
		zephyr,resolution = <12>;
	};
};

/ {
	adc-keys {
		compatible = "adc-keys";
		io-channels = <&adc1 4>;
		sample-period-ms = <100>;
		keyup-threshold-mv = <0>;

		key_1 {
			press-thresholds-mv = <1650>;  /* K1 */
			zephyr,code = <INPUT_KEY_1>;
		};

		key_2 {
			press-thresholds-mv = <1357>;  /* K2 */
			zephyr,code = <INPUT_KEY_2>;
		};

		key_3 {
			press-thresholds-mv = <1211>;  /* K3 */
			zephyr,code = <INPUT_KEY_3>;
		};

		key_4 {
			press-thresholds-mv = <1058>;  /* K4 */
			zephyr,code = <INPUT_KEY_4>;
		};
	};
};

Example 2

Another example that supports simultaneously press both of keys:

/ {
	adc-keys {
		compatible = "adc-keys";
		io-channels = <&adc1 4>;
		sample-period-ms = <100>;
		keyup-threshold-mv = <0>;

		key_4 {
			press-thresholds-mv = <1650>,  /* K4 */
					      <2536>;  /* K4 + K5 */
			zephyr,code = <INPUT_KEY_4>;
		};

		key_5 {
			press-thresholds-mv = <2300>,  /* K5 */
					      <2536>;  /* K4 + K5 */
			zephyr,code = <INPUT_KEY_5>;
		};
	};
};

In the circuit above, for K4:

For K5:

For both K4 and K5 being pressed simultaneously:

bbilas · 2024-02-02T07:53:31Z

drivers/input/input_adc_keys.c

+	adc_keys_process(dev);
+}
+
+static void adc_keys_timer_handler(struct k_timer *timer)


Could k_work_delayable be used instead?

fabiobaltieri

Can you add an instance of this in tests/drivers/build_all/input/app.overlay so it gets built in CI?

fabiobaltieri · 2024-02-02T09:01:49Z

drivers/input/input_adc_keys.c

+	const struct device *self;
+	struct k_work_delayable dwork;
+	struct adc_sequence seq;
+	struct adc_keys_key_data *key_data;


This pointer can go in the config structure since the pointer itself does not change, save few bytes of ram. Then at that point the only thing to initialize is self, you can set that in adc_keys_init and drop the static initialization, which saves few bytes of flash as well.

Thank you for your suggestion. My understanding is that even if I assign data->self in adc_keys_init, the self pointer would still occupy some memory within struct adc_keys_data. The DEVICE_DT_INST_GET merely populates this pointer with an address at compile time. Am I correct in this understanding? It's a bit unexpected for me. Looking forward to your clarification.

Yeah that's right, but if you initialize it statically then it's going to take the space in RAM and also space in flash for the static initialization, including for the members that you are not explicitly initializing (storing a bunch of 0 essentially), while if you let it uninitialized the system will zero-initialize it automatically and you just use the flash for the instructions to set the pointer, so it's usually a good idea to leave data structures uninitalized if possible.

dts/bindings/input/adc-keys.yaml

fabiobaltieri · 2024-02-08T11:45:14Z

tests/drivers/build_all/input/app.overlay

+			io-channels = <&test_adc 0>;
+			keyup-threshold-mv = <0>;
+			button_0 {
+				press-thresholds-mv = <1500>;


maybe add an extra value and an extra button to make sure that the iteration macros are tested

(still not addressed)

drivers/input/input_adc_keys.c

fabiobaltieri · 2024-02-08T14:16:02Z

drivers/input/input_adc_keys.c

+		 * being overwritten by another threshold configuration.
+		 */
+		if (closest_mv == code_cfg->press_mv) {
+			key_data->curr_state = 1;


Suggested change

key_data->curr_state = 1;

key_data->curr_state = true;

fabiobaltieri · 2024-02-08T14:16:09Z

drivers/input/input_adc_keys.c

+		 * Reset the state so that it can be updated in the next
+		 * iteration.
+		 */
+		key_data->curr_state = 0;


Suggested change

key_data->curr_state = 0;

key_data->curr_state = false;

fabiobaltieri · 2024-02-08T14:19:13Z

drivers/input/input_adc_keys.c

+#define ADC_KEYS_KEY_DATA(node_id)                                                                 \
+	{                                                                                          \
+		.zephyr_code = DT_PROP(node_id, zephyr_code), .last_state = 0, .curr_state = 0,    \
+	}


how about moving zephyr_code in its own array? I think you could have a const uint16_t *codes for the codes, that is going to be key_cnt sized, then you only have to initialize that statically and you can leave key_data being zero initialized automatically. That will save another bit of flash and another bit of ram. :-)

Good idea! Now everything looks comfortable.

fabiobaltieri · 2024-02-09T16:21:35Z

tests/drivers/build_all/input/app.overlay

+			io-channels = <&test_adc 0>;
+			keyup-threshold-mv = <0>;
+			button_0 {
+				press-thresholds-mv = <1500>;


(still not addressed)

drivers/input/input_adc_keys.c

fabiobaltieri

Cool, just few nitpicks on the types but looks good otherwise

drivers/input/input_adc_keys.c

fabiobaltieri

Good stuff thanks for contributing this.

drivers/input/input_adc_keys.c

bbilas · 2024-02-20T10:43:06Z

drivers/input/input_adc_keys.c

+	data->self = dev;
+	k_work_init_delayable(&data->dwork, adc_keys_work_handler);
+
+#if defined(CONFIG_INPUT_LOG_LEVEL_DBG)


Suggested change

#if defined(CONFIG_INPUT_LOG_LEVEL_DBG)

if (IS_ENABLED((CONFIG_INPUT_LOG_LEVEL_DBG)) {

Why? It should be completely removed in compile time if not enabled

Linker should drop this part anyway. You can double-check that by comparing the binaries.

This seems a bit too dependent on the compiler's optimization level. Wouldn't using the preprocessor be a more recommended approach, as discussed in #53200?

It depends. In that case, the compiler will catch the errors in that for loop even though CONFIG_INPUT_LOG_LEVEL_DBG is disabled. As it's an init function it does not have much impact on the code even though the linker would not drop it.

Oh, that's enlightening. Thank you for your suggestion. Updated.

thanks for catching this @bbilas -- there's a bit in the coding style guide: https://docs.zephyrproject.org/latest/kernel/util/index.html#c.IS_ENABLED, yes it does depend on compiler optimization but that's a fairly common theme in the code base and at this point just something understood and utilized

drivers/input/input_adc_keys.c

This commit introduces a driver for ADC keys, a common circuit design where keys are connected to an ADC input via a resistor ladder. Signed-off-by: Chen Xingyu <hi@xingrz.me>

* so that it'll be built by the CI Signed-off-by: Chen Xingyu <hi@xingrz.me>

xingrz · 2024-02-26T13:50:48Z

Thanks all! I've learned a lot from this PR.

xingrz force-pushed the input-adc-keys branch 5 times, most recently from 21ee194 to 6db4b16 Compare February 2, 2024 04:43

xingrz marked this pull request as ready for review February 2, 2024 04:58

zephyrbot added area: Input Input Subsystem and Drivers area: Devicetree Binding PR modifies or adds a Device Tree binding labels Feb 2, 2024

zephyrbot requested review from fabiobaltieri, galak and gmarull February 2, 2024 04:59

zephyrbot assigned fabiobaltieri Feb 2, 2024

bbilas reviewed Feb 2, 2024

View reviewed changes

xingrz force-pushed the input-adc-keys branch from 6db4b16 to 2044fad Compare February 2, 2024 08:24

fabiobaltieri reviewed Feb 2, 2024

View reviewed changes

xingrz force-pushed the input-adc-keys branch 4 times, most recently from 1c36de9 to 32bd46c Compare February 2, 2024 16:38

fabiobaltieri reviewed Feb 6, 2024

View reviewed changes

dts/bindings/input/adc-keys.yaml Show resolved Hide resolved

fabiobaltieri reviewed Feb 8, 2024

View reviewed changes

xingrz force-pushed the input-adc-keys branch from 32bd46c to 1c3e660 Compare February 8, 2024 13:22

fabiobaltieri reviewed Feb 8, 2024

View reviewed changes

xingrz force-pushed the input-adc-keys branch from 1c3e660 to 67bdd04 Compare February 8, 2024 14:37

fabiobaltieri reviewed Feb 9, 2024

View reviewed changes

xingrz force-pushed the input-adc-keys branch from 67bdd04 to 29fa6fa Compare February 9, 2024 16:40

fabiobaltieri reviewed Feb 14, 2024

View reviewed changes

xingrz force-pushed the input-adc-keys branch 2 times, most recently from 0de60a9 to 0602460 Compare February 15, 2024 13:57

fabiobaltieri previously approved these changes Feb 16, 2024

View reviewed changes

fabiobaltieri requested a review from bbilas February 16, 2024 10:16

bbilas reviewed Feb 20, 2024

View reviewed changes

drivers/input/input_adc_keys.c Outdated Show resolved Hide resolved

bbilas reviewed Feb 20, 2024

View reviewed changes

drivers/input/input_adc_keys.c Show resolved Hide resolved

bbilas reviewed Feb 20, 2024

View reviewed changes

drivers/input/input_adc_keys.c Outdated Show resolved Hide resolved

bbilas reviewed Feb 20, 2024

View reviewed changes

drivers/input/input_adc_keys.c Outdated Show resolved Hide resolved

xingrz dismissed fabiobaltieri’s stale review via 9d4ee22 February 20, 2024 11:08

xingrz force-pushed the input-adc-keys branch from 0602460 to 9d4ee22 Compare February 20, 2024 11:08

xingrz added 2 commits February 20, 2024 19:34

drivers: input: Implement driver for ADC keys

342ec8a

This commit introduces a driver for ADC keys, a common circuit design where keys are connected to an ADC input via a resistor ladder. Signed-off-by: Chen Xingyu <hi@xingrz.me>

tests: drivers: build_all: input: Add adc-keys

3648041

* so that it'll be built by the CI Signed-off-by: Chen Xingyu <hi@xingrz.me>

xingrz force-pushed the input-adc-keys branch from 9d4ee22 to 3648041 Compare February 20, 2024 11:34

bbilas approved these changes Feb 20, 2024

View reviewed changes

xingrz requested a review from fabiobaltieri February 20, 2024 16:04

fabiobaltieri approved these changes Feb 20, 2024

View reviewed changes

fabiobaltieri added this to the v3.7.0 milestone Feb 20, 2024

fabiobaltieri merged commit eaa492a into zephyrproject-rtos:main Feb 26, 2024
18 checks passed

xingrz deleted the input-adc-keys branch February 26, 2024 13:50

kartben mentioned this pull request May 3, 2024

boards: st: add stm32c0116_dk support #71722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: input: Implement driver for ADC keys #68446

drivers: input: Implement driver for ADC keys #68446

xingrz commented Feb 2, 2024 •

edited

bbilas Feb 2, 2024 •

edited

xingrz Feb 2, 2024

fabiobaltieri left a comment

fabiobaltieri Feb 2, 2024

xingrz Feb 2, 2024

fabiobaltieri Feb 6, 2024

fabiobaltieri Feb 8, 2024

fabiobaltieri Feb 9, 2024

fabiobaltieri Feb 8, 2024

fabiobaltieri Feb 8, 2024

fabiobaltieri Feb 8, 2024

xingrz Feb 8, 2024

fabiobaltieri Feb 9, 2024

fabiobaltieri left a comment

fabiobaltieri left a comment

bbilas Feb 20, 2024 •

edited

xingrz Feb 20, 2024

bbilas Feb 20, 2024

xingrz Feb 20, 2024

bbilas Feb 20, 2024

xingrz Feb 20, 2024

fabiobaltieri Feb 20, 2024

xingrz commented Feb 26, 2024

	#if defined(CONFIG_INPUT_LOG_LEVEL_DBG)
	if (IS_ENABLED((CONFIG_INPUT_LOG_LEVEL_DBG)) {

drivers: input: Implement driver for ADC keys #68446

drivers: input: Implement driver for ADC keys #68446

Conversation

xingrz commented Feb 2, 2024 • edited

Example 1

Example 2

bbilas Feb 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiobaltieri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabiobaltieri left a comment

Choose a reason for hiding this comment

fabiobaltieri left a comment

Choose a reason for hiding this comment

bbilas Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xingrz commented Feb 26, 2024

xingrz commented Feb 2, 2024 •

edited

bbilas Feb 2, 2024 •

edited

bbilas Feb 20, 2024 •

edited