From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
Add support for GPIO-based charlieplex keypad, allowing to control
N^2-N keys using N GPIO lines.
Reuse matrix keypad keymap to simplify, even if there is no concept
of rows and columns in this type of keyboard.
Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
---
MAINTAINERS | 7 +
drivers/input/keyboard/Kconfig | 14 ++
drivers/input/keyboard/Makefile | 1 +
drivers/input/keyboard/charlieplex_keypad.c | 213 ++++++++++++++++++++
4 files changed, 235 insertions(+)
create mode 100644 drivers/input/keyboard/charlieplex_keypad.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ed6d11a7746..0b2d71f32b40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5765,6 +5765,13 @@ S: Maintained
F: Documentation/hwmon/powerz.rst
F: drivers/hwmon/powerz.c
+CHARLIEPLEX KEYPAD DRIVER
+M: Hugo Villeneuve <hvilleneuve@dimonoff.com>
+S: Supported
+W: http://www.mosaic-industries.com/embedded-systems/microcontroller-projects/electronic-circuits/matrix-keypad-scan-decode
+F: Documentation/devicetree/bindings/input/gpio-charlieplex-keypad.yaml
+F: drivers/input/keyboard/charlieplex_keypad.c
+
CHECKPATCH
M: Andy Whitcroft <apw@canonical.com>
M: Joe Perches <joe@perches.com>
diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 2ff4fef322c2..ae54b4b7e2d8 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -289,6 +289,20 @@ config KEYBOARD_MATRIX
To compile this driver as a module, choose M here: the
module will be called matrix_keypad.
+config KEYBOARD_CHARLIEPLEX
+ tristate "GPIO driven chearlieplex keypad support"
+ depends on GPIOLIB || COMPILE_TEST
+ select INPUT_MATRIXKMAP
+ help
+ Enable support for GPIO driven charlieplex keypad. A charlieplex
+ keypad allows to use fewer GPIO lines to interface to key switches.
+ For example, an N lines charlieplex keypad can be used to interface
+ to N^2-N different key switches. However, this type of keypad
+ cannot detect more than one key press at a time.
+
+ To compile this driver as a module, choose M here: the
+ module will be called charlieplex_keypad.
+
config KEYBOARD_HIL_OLD
tristate "HP HIL keyboard support (simple driver)"
depends on GSC || HP300
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 2d906e14f3e2..40b5cf5d374d 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_KEYBOARD_LOCOMO) += locomokbd.o
obj-$(CONFIG_KEYBOARD_LPC32XX) += lpc32xx-keys.o
obj-$(CONFIG_KEYBOARD_MAPLE) += maple_keyb.o
obj-$(CONFIG_KEYBOARD_MATRIX) += matrix_keypad.o
+obj-$(CONFIG_KEYBOARD_CHARLIEPLEX) += charlieplex_keypad.o
obj-$(CONFIG_KEYBOARD_MAX7359) += max7359_keypad.o
obj-$(CONFIG_KEYBOARD_MAX7360) += max7360-keypad.o
obj-$(CONFIG_KEYBOARD_MPR121) += mpr121_touchkey.o
diff --git a/drivers/input/keyboard/charlieplex_keypad.c b/drivers/input/keyboard/charlieplex_keypad.c
new file mode 100644
index 000000000000..fae0aeb1d167
--- /dev/null
+++ b/drivers/input/keyboard/charlieplex_keypad.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * GPIO driven charlieplex keypad driver
+ *
+ * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com>
+ *
+ * Based on matrix_keyboard.c
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/types.h>
+
+struct charlieplex_keypad {
+ struct input_dev *input_dev;
+ struct gpio_descs *line_gpios;
+ unsigned int nlines;
+ unsigned int settling_time_us;
+ unsigned int debounce_threshold;
+ unsigned int debounce_count;
+ int debounce_code;
+ int current_code;
+};
+
+static void charlieplex_keypad_report_key(struct input_dev *input)
+{
+ struct charlieplex_keypad *keypad = input_get_drvdata(input);
+ const unsigned short *keycodes = input->keycode;
+
+ if (keypad->current_code > 0) {
+ input_event(input, EV_MSC, MSC_SCAN, keypad->current_code);
+ input_report_key(input, keycodes[keypad->current_code], 0);
+ }
+
+ if (keypad->debounce_code) {
+ input_event(input, EV_MSC, MSC_SCAN, keypad->debounce_code);
+ input_report_key(input, keycodes[keypad->debounce_code], 1);
+ }
+
+ input_sync(input);
+ keypad->current_code = keypad->debounce_code;
+}
+
+static void charlieplex_keypad_check_switch_change(struct input_dev *input,
+ int code)
+{
+ struct charlieplex_keypad *keypad = input_get_drvdata(input);
+
+ if (code != keypad->debounce_code) {
+ keypad->debounce_count = 0;
+ keypad->debounce_code = code;
+ } else if (keypad->debounce_count < keypad->debounce_threshold) {
+ keypad->debounce_count++;
+
+ if (keypad->debounce_count >= keypad->debounce_threshold &&
+ keypad->debounce_code != keypad->current_code)
+ charlieplex_keypad_report_key(input);
+ }
+}
+
+static void charlieplex_keypad_poll(struct input_dev *input)
+{
+ struct charlieplex_keypad *keypad = input_get_drvdata(input);
+ int oline;
+ int code;
+
+ for (code = 0, oline = 0; oline < keypad->nlines; oline++) {
+ DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
+ int iline;
+ int rc;
+
+ /* Activate only one line as output at a time. */
+ gpiod_direction_output(keypad->line_gpios->desc[oline], 1);
+
+ if (keypad->settling_time_us)
+ fsleep(keypad->settling_time_us);
+
+ /* Read input on all other lines. */
+ rc = gpiod_get_array_value_cansleep(keypad->line_gpios->ndescs,
+ keypad->line_gpios->desc,
+ keypad->line_gpios->info, values);
+ if (rc)
+ return;
+
+ for (iline = 0; iline < keypad->nlines; iline++) {
+ if (iline == oline)
+ continue; /* Do not read active output line. */
+
+ /* Check if GPIO is asserted. */
+ if (test_bit(iline, values)) {
+ code = MATRIX_SCAN_CODE(oline, iline,
+ get_count_order(keypad->nlines));
+ /*
+ * Exit loop immediately since we cannot detect
+ * more than one key press at a time.
+ */
+ break;
+ }
+ }
+
+ gpiod_direction_input(keypad->line_gpios->desc[oline]);
+
+ if (code)
+ break;
+ }
+
+ charlieplex_keypad_check_switch_change(input, code);
+}
+
+static int charlieplex_keypad_init_gpio(struct platform_device *pdev,
+ struct charlieplex_keypad *keypad)
+{
+ int i;
+
+ keypad->line_gpios = devm_gpiod_get_array(&pdev->dev, "line", GPIOD_IN);
+ if (IS_ERR(keypad->line_gpios))
+ return PTR_ERR(keypad->line_gpios);
+
+ keypad->nlines = keypad->line_gpios->ndescs;
+
+ if (keypad->nlines > MATRIX_MAX_ROWS)
+ return -EINVAL;
+
+ for (i = 0; i < keypad->nlines; i++)
+ gpiod_set_consumer_name(keypad->line_gpios->desc[i], "charlieplex_kbd_line");
+
+ return 0;
+}
+
+static int charlieplex_keypad_probe(struct platform_device *pdev)
+{
+ struct charlieplex_keypad *keypad;
+ unsigned int debounce_interval_ms;
+ unsigned int poll_interval_ms;
+ struct input_dev *input_dev;
+ int err;
+
+ keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
+ if (!keypad)
+ return -ENOMEM;
+
+ input_dev = devm_input_allocate_device(&pdev->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ keypad->input_dev = input_dev;
+
+ device_property_read_u32(&pdev->dev, "poll-interval", &poll_interval_ms);
+ device_property_read_u32(&pdev->dev, "debounce-delay-ms", &debounce_interval_ms);
+ device_property_read_u32(&pdev->dev, "settling-time-us", &keypad->settling_time_us);
+
+ keypad->current_code = -1;
+ keypad->debounce_code = -1;
+ keypad->debounce_threshold = DIV_ROUND_UP(debounce_interval_ms, poll_interval_ms);
+
+ err = charlieplex_keypad_init_gpio(pdev, keypad);
+ if (err)
+ return err;
+
+ input_dev->name = pdev->name;
+ input_dev->id.bustype = BUS_HOST;
+
+ err = matrix_keypad_build_keymap(NULL, NULL, keypad->nlines,
+ keypad->nlines, NULL, input_dev);
+ if (err)
+ dev_err_probe(&pdev->dev, -ENOMEM, "failed to build keymap\n");
+
+ if (device_property_read_bool(&pdev->dev, "autorepeat"))
+ __set_bit(EV_REP, input_dev->evbit);
+
+ input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+
+ err = input_setup_polling(input_dev, charlieplex_keypad_poll);
+ if (err)
+ dev_err_probe(&pdev->dev, err, "unable to set up polling\n");
+
+ input_set_poll_interval(input_dev, poll_interval_ms);
+
+ input_set_drvdata(input_dev, keypad);
+
+ err = input_register_device(keypad->input_dev);
+ if (err)
+ return err;
+
+ platform_set_drvdata(pdev, keypad);
+
+ return 0;
+}
+
+static const struct of_device_id charlieplex_keypad_dt_match[] = {
+ { .compatible = "gpio-charlieplex-keypad" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, charlieplex_keypad_dt_match);
+
+static struct platform_driver charlieplex_keypad_driver = {
+ .probe = charlieplex_keypad_probe,
+ .driver = {
+ .name = "charlieplex-keypad",
+ .of_match_table = of_match_ptr(charlieplex_keypad_dt_match),
+ },
+};
+module_platform_driver(charlieplex_keypad_driver);
+
+MODULE_AUTHOR("Hugo Villeneuve <hvilleneuve@dimonoff.com>");
+MODULE_DESCRIPTION("GPIO driven charlieplex keypad driver");
+MODULE_LICENSE("GPL");
--
2.47.3On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote:
> Add support for GPIO-based charlieplex keypad, allowing to control
> N^2-N keys using N GPIO lines.
>
> Reuse matrix keypad keymap to simplify, even if there is no concept
> of rows and columns in this type of keyboard.
...
> +/*
> + * GPIO driven charlieplex keypad driver
> + *
> + * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com>
> + *
> + * Based on matrix_keyboard.c
A single space after asterisk is enough.
> + */
...
+ bitops.h
> +#include <linux/delay.h>
+ dev_printk.h
+ device/devres.h
+ err.h
> +#include <linux/gpio/consumer.h>
> +#include <linux/input.h>
> +#include <linux/input/matrix_keypad.h>
+ math.h
> +#include <linux/module.h>
> +#include <linux/of.h>
Is this in use? Or you wanted mod_devicetable.h for OF ID table?
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/types.h>
...
> + for (code = 0, oline = 0; oline < keypad->nlines; oline++) {
> + DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> + int iline;
> + int rc;
I think Dmitry prefers 'error' name for this kind of variables.
> + /* Activate only one line as output at a time. */
> + gpiod_direction_output(keypad->line_gpios->desc[oline], 1);
> +
> + if (keypad->settling_time_us)
> + fsleep(keypad->settling_time_us);
> +
> + /* Read input on all other lines. */
> + rc = gpiod_get_array_value_cansleep(keypad->line_gpios->ndescs,
> + keypad->line_gpios->desc,
> + keypad->line_gpios->info, values);
> + if (rc)
> + return;
> +
> + for (iline = 0; iline < keypad->nlines; iline++) {
> + if (iline == oline)
> + continue; /* Do not read active output line. */
> +
> + /* Check if GPIO is asserted. */
> + if (test_bit(iline, values)) {
> + code = MATRIX_SCAN_CODE(oline, iline,
> + get_count_order(keypad->nlines));
> + /*
> + * Exit loop immediately since we cannot detect
> + * more than one key press at a time.
> + */
> + break;
> + }
> + }
> +
> + gpiod_direction_input(keypad->line_gpios->desc[oline]);
> +
> + if (code)
> + break;
> + }
...
> +static int charlieplex_keypad_init_gpio(struct platform_device *pdev,
> + struct charlieplex_keypad *keypad)
> +{
> + int i;
Why signed? But see below as well.
> + keypad->line_gpios = devm_gpiod_get_array(&pdev->dev, "line", GPIOD_IN);
> + if (IS_ERR(keypad->line_gpios))
> + return PTR_ERR(keypad->line_gpios);
> +
> + keypad->nlines = keypad->line_gpios->ndescs;
> +
> + if (keypad->nlines > MATRIX_MAX_ROWS)
> + return -EINVAL;
> + for (i = 0; i < keypad->nlines; i++)
iterator is local to the loop, hence
for (unsigned int i = 0; i < keypad->nlines; i++)
> + gpiod_set_consumer_name(keypad->line_gpios->desc[i], "charlieplex_kbd_line");
> +
> + return 0;
> +}
...
> +static int charlieplex_keypad_probe(struct platform_device *pdev)
> +{
> + struct charlieplex_keypad *keypad;
> + unsigned int debounce_interval_ms;
> + unsigned int poll_interval_ms;
> + struct input_dev *input_dev;
> + int err;
The naming is even inconsistent between the functions...
> + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> + if (!keypad)
> + return -ENOMEM;
> +
> + input_dev = devm_input_allocate_device(&pdev->dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + keypad->input_dev = input_dev;
> +
> + device_property_read_u32(&pdev->dev, "poll-interval", &poll_interval_ms);
> + device_property_read_u32(&pdev->dev, "debounce-delay-ms", &debounce_interval_ms);
> + device_property_read_u32(&pdev->dev, "settling-time-us", &keypad->settling_time_us);
> +
> + keypad->current_code = -1;
> + keypad->debounce_code = -1;
> + keypad->debounce_threshold = DIV_ROUND_UP(debounce_interval_ms, poll_interval_ms);
> +
> + err = charlieplex_keypad_init_gpio(pdev, keypad);
> + if (err)
> + return err;
> +
> + input_dev->name = pdev->name;
> + input_dev->id.bustype = BUS_HOST;
> +
> + err = matrix_keypad_build_keymap(NULL, NULL, keypad->nlines,
> + keypad->nlines, NULL, input_dev);
> + if (err)
> + dev_err_probe(&pdev->dev, -ENOMEM, "failed to build keymap\n");
> +
> + if (device_property_read_bool(&pdev->dev, "autorepeat"))
> + __set_bit(EV_REP, input_dev->evbit);
> +
> + input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +
> + err = input_setup_polling(input_dev, charlieplex_keypad_poll);
> + if (err)
> + dev_err_probe(&pdev->dev, err, "unable to set up polling\n");
> +
> + input_set_poll_interval(input_dev, poll_interval_ms);
> +
> + input_set_drvdata(input_dev, keypad);
> +
> + err = input_register_device(keypad->input_dev);
> + if (err)
> + return err;
> + platform_set_drvdata(pdev, keypad);
Is this needed?
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Hi Andy,
thank you for the review.
On Wed, 25 Feb 2026 18:12:13 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote:
>
> > Add support for GPIO-based charlieplex keypad, allowing to control
> > N^2-N keys using N GPIO lines.
> >
> > Reuse matrix keypad keymap to simplify, even if there is no concept
> > of rows and columns in this type of keyboard.
>
> ...
>
> > +/*
> > + * GPIO driven charlieplex keypad driver
> > + *
> > + * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > + *
> > + * Based on matrix_keyboard.c
>
> A single space after asterisk is enough.
Ok, leftover from copy/paste from matrix_keyboard.c :)
>
> > + */
>
> ...
>
> + bitops.h
>
> > +#include <linux/delay.h>
>
> + dev_printk.h
> + device/devres.h
> + err.h
>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/input.h>
> > +#include <linux/input/matrix_keypad.h>
>
> + math.h
Ok.
>
> > +#include <linux/module.h>
>
> > +#include <linux/of.h>
>
> Is this in use? Or you wanted mod_devicetable.h for OF ID table?
I need only OF ID table, so will replace with mod_devicetable.h.
>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/types.h>
>
> ...
>
> > + for (code = 0, oline = 0; oline < keypad->nlines; oline++) {
> > + DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> > + int iline;
>
> > + int rc;
>
> I think Dmitry prefers 'error' name for this kind of variables.
I hate using "error", can be so misleading :)
I would prefer to use "rc" everywhere, but if Dmitry chimes in and
specifies "error" or "err", then so it will be.
>
> > + /* Activate only one line as output at a time. */
> > + gpiod_direction_output(keypad->line_gpios->desc[oline], 1);
> > +
> > + if (keypad->settling_time_us)
> > + fsleep(keypad->settling_time_us);
> > +
> > + /* Read input on all other lines. */
> > + rc = gpiod_get_array_value_cansleep(keypad->line_gpios->ndescs,
> > + keypad->line_gpios->desc,
> > + keypad->line_gpios->info, values);
> > + if (rc)
> > + return;
> > +
> > + for (iline = 0; iline < keypad->nlines; iline++) {
> > + if (iline == oline)
> > + continue; /* Do not read active output line. */
> > +
> > + /* Check if GPIO is asserted. */
> > + if (test_bit(iline, values)) {
> > + code = MATRIX_SCAN_CODE(oline, iline,
> > + get_count_order(keypad->nlines));
> > + /*
> > + * Exit loop immediately since we cannot detect
> > + * more than one key press at a time.
> > + */
> > + break;
> > + }
> > + }
> > +
> > + gpiod_direction_input(keypad->line_gpios->desc[oline]);
> > +
> > + if (code)
> > + break;
> > + }
>
> ...
>
> > +static int charlieplex_keypad_init_gpio(struct platform_device *pdev,
> > + struct charlieplex_keypad *keypad)
> > +{
> > + int i;
>
> Why signed? But see below as well.
Will switch to unsigned.
>
> > + keypad->line_gpios = devm_gpiod_get_array(&pdev->dev, "line", GPIOD_IN);
> > + if (IS_ERR(keypad->line_gpios))
> > + return PTR_ERR(keypad->line_gpios);
> > +
> > + keypad->nlines = keypad->line_gpios->ndescs;
> > +
> > + if (keypad->nlines > MATRIX_MAX_ROWS)
> > + return -EINVAL;
>
> > + for (i = 0; i < keypad->nlines; i++)
>
> iterator is local to the loop, hence
>
> for (unsigned int i = 0; i < keypad->nlines; i++)
Ok
>
> > + gpiod_set_consumer_name(keypad->line_gpios->desc[i], "charlieplex_kbd_line");
> > +
> > + return 0;
> > +}
>
> ...
>
> > +static int charlieplex_keypad_probe(struct platform_device *pdev)
> > +{
> > + struct charlieplex_keypad *keypad;
> > + unsigned int debounce_interval_ms;
> > + unsigned int poll_interval_ms;
> > + struct input_dev *input_dev;
>
> > + int err;
>
> The naming is even inconsistent between the functions...
Agreed, will fix as stated above.
>
> > + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> > + if (!keypad)
> > + return -ENOMEM;
> > +
> > + input_dev = devm_input_allocate_device(&pdev->dev);
> > + if (!input_dev)
> > + return -ENOMEM;
> > +
> > + keypad->input_dev = input_dev;
> > +
> > + device_property_read_u32(&pdev->dev, "poll-interval", &poll_interval_ms);
> > + device_property_read_u32(&pdev->dev, "debounce-delay-ms", &debounce_interval_ms);
> > + device_property_read_u32(&pdev->dev, "settling-time-us", &keypad->settling_time_us);
> > +
> > + keypad->current_code = -1;
> > + keypad->debounce_code = -1;
> > + keypad->debounce_threshold = DIV_ROUND_UP(debounce_interval_ms, poll_interval_ms);
> > +
> > + err = charlieplex_keypad_init_gpio(pdev, keypad);
> > + if (err)
> > + return err;
> > +
> > + input_dev->name = pdev->name;
> > + input_dev->id.bustype = BUS_HOST;
> > +
> > + err = matrix_keypad_build_keymap(NULL, NULL, keypad->nlines,
> > + keypad->nlines, NULL, input_dev);
> > + if (err)
> > + dev_err_probe(&pdev->dev, -ENOMEM, "failed to build keymap\n");
> > +
> > + if (device_property_read_bool(&pdev->dev, "autorepeat"))
> > + __set_bit(EV_REP, input_dev->evbit);
> > +
> > + input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > +
> > + err = input_setup_polling(input_dev, charlieplex_keypad_poll);
> > + if (err)
> > + dev_err_probe(&pdev->dev, err, "unable to set up polling\n");
> > +
> > + input_set_poll_interval(input_dev, poll_interval_ms);
> > +
> > + input_set_drvdata(input_dev, keypad);
> > +
> > + err = input_register_device(keypad->input_dev);
> > + if (err)
> > + return err;
>
> > + platform_set_drvdata(pdev, keypad);
>
> Is this needed?
No, will remove it, and replace last lines with:
return input_register_device(keypad->input_dev);
>
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>
Hugo Villeneuve
Hi Hugo,
On Wed, Feb 25, 2026 at 11:41:55AM -0500, Hugo Villeneuve wrote:
> Hi Andy,
> thank you for the review.
>
> On Wed, 25 Feb 2026 18:12:13 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>
> > On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote:
> >
> > > Add support for GPIO-based charlieplex keypad, allowing to control
> > > N^2-N keys using N GPIO lines.
> > >
> > > Reuse matrix keypad keymap to simplify, even if there is no concept
> > > of rows and columns in this type of keyboard.
> >
> > ...
> >
> > > +/*
> > > + * GPIO driven charlieplex keypad driver
> > > + *
> > > + * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > + *
> > > + * Based on matrix_keyboard.c
> >
> > A single space after asterisk is enough.
>
> Ok, leftover from copy/paste from matrix_keyboard.c :)
>
> >
> > > + */
> >
> > ...
> >
> > + bitops.h
> >
> > > +#include <linux/delay.h>
> >
> > + dev_printk.h
> > + device/devres.h
> > + err.h
> >
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/matrix_keypad.h>
> >
> > + math.h
>
> Ok.
>
>
> >
> > > +#include <linux/module.h>
> >
> > > +#include <linux/of.h>
> >
> > Is this in use? Or you wanted mod_devicetable.h for OF ID table?
>
> I need only OF ID table, so will replace with mod_devicetable.h.
>
>
> >
> > > +#include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > +#include <linux/types.h>
> >
> > ...
> >
> > > + for (code = 0, oline = 0; oline < keypad->nlines; oline++) {
> > > + DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> > > + int iline;
> >
> > > + int rc;
> >
> > I think Dmitry prefers 'error' name for this kind of variables.
>
> I hate using "error", can be so misleading :)
>
> I would prefer to use "rc" everywhere, but if Dmitry chimes in and
> specifies "error" or "err", then so it will be.
Yes, I prefer err or error for variables that carry error code or 0.
This allows to write
error = action(...);
if (error) {
// handle it
}
which is very clear IMO.
> > > +
> > > + err = input_register_device(keypad->input_dev);
> > > + if (err)
> > > + return err;
> >
> > > + platform_set_drvdata(pdev, keypad);
> >
> > Is this needed?
>
> No, will remove it, and replace last lines with:
>
> return input_register_device(keypad->input_dev);
Please use
err = input_register_device(...);
if (err)
return err;
return 0;
It clearly differentiates error an normal paths, shows that function
returns 0 and not anything else on success, and allows to reorder
or add additional actions easily.
Thanks.
--
Dmitry
Hi Dmitry,
On Wed, 25 Feb 2026 09:12:57 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Hugo,
>
> On Wed, Feb 25, 2026 at 11:41:55AM -0500, Hugo Villeneuve wrote:
> > Hi Andy,
> > thank you for the review.
> >
> > On Wed, 25 Feb 2026 18:12:13 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >
> > > On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote:
> > >
> > > > Add support for GPIO-based charlieplex keypad, allowing to control
> > > > N^2-N keys using N GPIO lines.
> > > >
> > > > Reuse matrix keypad keymap to simplify, even if there is no concept
> > > > of rows and columns in this type of keyboard.
> > >
> > > ...
> > >
> > > > +/*
> > > > + * GPIO driven charlieplex keypad driver
> > > > + *
> > > > + * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > > + *
> > > > + * Based on matrix_keyboard.c
> > >
> > > A single space after asterisk is enough.
> >
> > Ok, leftover from copy/paste from matrix_keyboard.c :)
> >
> > >
> > > > + */
> > >
> > > ...
> > >
> > > + bitops.h
> > >
> > > > +#include <linux/delay.h>
> > >
> > > + dev_printk.h
> > > + device/devres.h
> > > + err.h
> > >
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/input/matrix_keypad.h>
> > >
> > > + math.h
> >
> > Ok.
> >
> >
> > >
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of.h>
> > >
> > > Is this in use? Or you wanted mod_devicetable.h for OF ID table?
> >
> > I need only OF ID table, so will replace with mod_devicetable.h.
> >
> >
> > >
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/property.h>
> > > > +#include <linux/types.h>
> > >
> > > ...
> > >
> > > > + for (code = 0, oline = 0; oline < keypad->nlines; oline++) {
> > > > + DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> > > > + int iline;
> > >
> > > > + int rc;
> > >
> > > I think Dmitry prefers 'error' name for this kind of variables.
> >
> > I hate using "error", can be so misleading :)
> >
> > I would prefer to use "rc" everywhere, but if Dmitry chimes in and
> > specifies "error" or "err", then so it will be.
>
> Yes, I prefer err or error for variables that carry error code or 0.
> This allows to write
>
> error = action(...);
> if (error) {
> // handle it
> }
>
> which is very clear IMO.
Ok, will fix it globally to "err" everywhere in V4.
>
>
> > > > +
> > > > + err = input_register_device(keypad->input_dev);
> > > > + if (err)
> > > > + return err;
> > >
> > > > + platform_set_drvdata(pdev, keypad);
> > >
> > > Is this needed?
> >
> > No, will remove it, and replace last lines with:
> >
> > return input_register_device(keypad->input_dev);
>
> Please use
>
> err = input_register_device(...);
> if (err)
> return err;
>
> return 0;
>
> It clearly differentiates error an normal paths, shows that function
> returns 0 and not anything else on success, and allows to reorder
> or add additional actions easily.
Ok.
Thank you,
Hugo
On Wed, 25 Feb 2026 11:41:55 -0500
Hugo Villeneuve <hugo@hugovil.com> wrote:
> Hi Andy,
> thank you for the review.
>
> On Wed, 25 Feb 2026 18:12:13 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>
> > On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote:
> >
> > > Add support for GPIO-based charlieplex keypad, allowing to control
> > > N^2-N keys using N GPIO lines.
> > >
> > > Reuse matrix keypad keymap to simplify, even if there is no concept
> > > of rows and columns in this type of keyboard.
> >
> > ...
> >
> > > +/*
> > > + * GPIO driven charlieplex keypad driver
> > > + *
> > > + * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com>
> > > + *
> > > + * Based on matrix_keyboard.c
> >
> > A single space after asterisk is enough.
>
> Ok, leftover from copy/paste from matrix_keyboard.c :)
>
> >
> > > + */
> >
> > ...
> >
> > + bitops.h
> >
> > > +#include <linux/delay.h>
> >
> > + dev_printk.h
> > + device/devres.h
> > + err.h
> >
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/matrix_keypad.h>
> >
> > + math.h
>
> Ok.
>
>
> >
> > > +#include <linux/module.h>
> >
> > > +#include <linux/of.h>
> >
> > Is this in use? Or you wanted mod_devicetable.h for OF ID table?
>
> I need only OF ID table, so will replace with mod_devicetable.h.
Hi Andy,
finally I need <linux/of.h> for of_match_ptr()...
But I will keep <mod_devicetable.h> ...
> >
> > > +#include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > +#include <linux/types.h>
> >
> > ...
> >
> > > + for (code = 0, oline = 0; oline < keypad->nlines; oline++) {
> > > + DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> > > + int iline;
> >
> > > + int rc;
> >
> > I think Dmitry prefers 'error' name for this kind of variables.
>
> I hate using "error", can be so misleading :)
>
> I would prefer to use "rc" everywhere, but if Dmitry chimes in and
> specifies "error" or "err", then so it will be.
>
>
> >
> > > + /* Activate only one line as output at a time. */
> > > + gpiod_direction_output(keypad->line_gpios->desc[oline], 1);
> > > +
> > > + if (keypad->settling_time_us)
> > > + fsleep(keypad->settling_time_us);
> > > +
> > > + /* Read input on all other lines. */
> > > + rc = gpiod_get_array_value_cansleep(keypad->line_gpios->ndescs,
> > > + keypad->line_gpios->desc,
> > > + keypad->line_gpios->info, values);
> > > + if (rc)
> > > + return;
> > > +
> > > + for (iline = 0; iline < keypad->nlines; iline++) {
> > > + if (iline == oline)
> > > + continue; /* Do not read active output line. */
> > > +
> > > + /* Check if GPIO is asserted. */
> > > + if (test_bit(iline, values)) {
> > > + code = MATRIX_SCAN_CODE(oline, iline,
> > > + get_count_order(keypad->nlines));
> > > + /*
> > > + * Exit loop immediately since we cannot detect
> > > + * more than one key press at a time.
> > > + */
> > > + break;
> > > + }
> > > + }
> > > +
> > > + gpiod_direction_input(keypad->line_gpios->desc[oline]);
> > > +
> > > + if (code)
> > > + break;
> > > + }
> >
> > ...
> >
> > > +static int charlieplex_keypad_init_gpio(struct platform_device *pdev,
> > > + struct charlieplex_keypad *keypad)
> > > +{
> > > + int i;
> >
> > Why signed? But see below as well.
>
> Will switch to unsigned.
>
>
> >
> > > + keypad->line_gpios = devm_gpiod_get_array(&pdev->dev, "line", GPIOD_IN);
> > > + if (IS_ERR(keypad->line_gpios))
> > > + return PTR_ERR(keypad->line_gpios);
> > > +
> > > + keypad->nlines = keypad->line_gpios->ndescs;
> > > +
> > > + if (keypad->nlines > MATRIX_MAX_ROWS)
> > > + return -EINVAL;
> >
> > > + for (i = 0; i < keypad->nlines; i++)
> >
> > iterator is local to the loop, hence
> >
> > for (unsigned int i = 0; i < keypad->nlines; i++)
>
> Ok
>
>
> >
> > > + gpiod_set_consumer_name(keypad->line_gpios->desc[i], "charlieplex_kbd_line");
> > > +
> > > + return 0;
> > > +}
> >
> > ...
> >
> > > +static int charlieplex_keypad_probe(struct platform_device *pdev)
> > > +{
> > > + struct charlieplex_keypad *keypad;
> > > + unsigned int debounce_interval_ms;
> > > + unsigned int poll_interval_ms;
> > > + struct input_dev *input_dev;
> >
> > > + int err;
> >
> > The naming is even inconsistent between the functions...
>
> Agreed, will fix as stated above.
>
>
> >
> > > + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> > > + if (!keypad)
> > > + return -ENOMEM;
> > > +
> > > + input_dev = devm_input_allocate_device(&pdev->dev);
> > > + if (!input_dev)
> > > + return -ENOMEM;
> > > +
> > > + keypad->input_dev = input_dev;
> > > +
> > > + device_property_read_u32(&pdev->dev, "poll-interval", &poll_interval_ms);
> > > + device_property_read_u32(&pdev->dev, "debounce-delay-ms", &debounce_interval_ms);
> > > + device_property_read_u32(&pdev->dev, "settling-time-us", &keypad->settling_time_us);
> > > +
> > > + keypad->current_code = -1;
> > > + keypad->debounce_code = -1;
> > > + keypad->debounce_threshold = DIV_ROUND_UP(debounce_interval_ms, poll_interval_ms);
> > > +
> > > + err = charlieplex_keypad_init_gpio(pdev, keypad);
> > > + if (err)
> > > + return err;
> > > +
> > > + input_dev->name = pdev->name;
> > > + input_dev->id.bustype = BUS_HOST;
> > > +
> > > + err = matrix_keypad_build_keymap(NULL, NULL, keypad->nlines,
> > > + keypad->nlines, NULL, input_dev);
> > > + if (err)
> > > + dev_err_probe(&pdev->dev, -ENOMEM, "failed to build keymap\n");
> > > +
> > > + if (device_property_read_bool(&pdev->dev, "autorepeat"))
> > > + __set_bit(EV_REP, input_dev->evbit);
> > > +
> > > + input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> > > +
> > > + err = input_setup_polling(input_dev, charlieplex_keypad_poll);
> > > + if (err)
> > > + dev_err_probe(&pdev->dev, err, "unable to set up polling\n");
> > > +
> > > + input_set_poll_interval(input_dev, poll_interval_ms);
> > > +
> > > + input_set_drvdata(input_dev, keypad);
> > > +
> > > + err = input_register_device(keypad->input_dev);
> > > + if (err)
> > > + return err;
> >
> > > + platform_set_drvdata(pdev, keypad);
> >
> > Is this needed?
>
> No, will remove it, and replace last lines with:
>
> return input_register_device(keypad->input_dev);
>
>
> >
> > > + return 0;
> > > +}
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
> >
>
> Hugo Villeneuve
--
Hugo Villeneuve
On Wed, Feb 25, 2026 at 11:56:52AM -0500, Hugo Villeneuve wrote: > On Wed, 25 Feb 2026 11:41:55 -0500 > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > Hi Andy, > > thank you for the review. > > > > On Wed, 25 Feb 2026 18:12:13 +0200 > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > > > On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote: > > > > > > > Add support for GPIO-based charlieplex keypad, allowing to control > > > > N^2-N keys using N GPIO lines. > > > > > > > > Reuse matrix keypad keymap to simplify, even if there is no concept > > > > of rows and columns in this type of keyboard. > > > > > > ... > > > > > > > +/* > > > > + * GPIO driven charlieplex keypad driver > > > > + * > > > > + * Copyright (c) 2025 Hugo Villeneuve <hvilleneuve@dimonoff.com> > > > > + * > > > > + * Based on matrix_keyboard.c > > > > > > A single space after asterisk is enough. > > > > Ok, leftover from copy/paste from matrix_keyboard.c :) > > > > > > > > > + */ > > > > > > ... > > > > > > + bitops.h > > > > > > > +#include <linux/delay.h> > > > > > > + dev_printk.h > > > + device/devres.h > > > + err.h > > > > > > > +#include <linux/gpio/consumer.h> > > > > +#include <linux/input.h> > > > > +#include <linux/input/matrix_keypad.h> > > > > > > + math.h > > > > Ok. > > > > > > > > > > > +#include <linux/module.h> > > > > > > > +#include <linux/of.h> > > > > > > Is this in use? Or you wanted mod_devicetable.h for OF ID table? > > > > I need only OF ID table, so will replace with mod_devicetable.h. > > Hi Andy, > finally I need <linux/of.h> for of_match_ptr()... > > But I will keep <mod_devicetable.h> ... Do we need the dependency on OF? We may include of match pointer unconditionally and the driver could be used on ACPI systems with PRP0001 HID. Thanks. -- Dmitry
On Wed, Feb 25, 2026 at 09:14:16AM -0800, Dmitry Torokhov wrote: > On Wed, Feb 25, 2026 at 11:56:52AM -0500, Hugo Villeneuve wrote: > > On Wed, 25 Feb 2026 11:41:55 -0500 > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > On Wed, 25 Feb 2026 18:12:13 +0200 > > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > > On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote: ... > > > > > +#include <linux/of.h> > > > > > > > > Is this in use? Or you wanted mod_devicetable.h for OF ID table? > > > > > > I need only OF ID table, so will replace with mod_devicetable.h. > > > > Hi Andy, > > finally I need <linux/of.h> for of_match_ptr()... > > > > But I will keep <mod_devicetable.h> ... > > Do we need the dependency on OF? We may include of match pointer > unconditionally and the driver could be used on ACPI systems with > PRP0001 HID. Not only that. of_match_ptr() or ACPI_PTR() shouldn't be used in a new code (there are, of course, _rare_ corner cases, which this one is not one of). -- With Best Regards, Andy Shevchenko
On Wed, 25 Feb 2026 19:21:36 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Wed, Feb 25, 2026 at 09:14:16AM -0800, Dmitry Torokhov wrote: > > On Wed, Feb 25, 2026 at 11:56:52AM -0500, Hugo Villeneuve wrote: > > > On Wed, 25 Feb 2026 11:41:55 -0500 > > > Hugo Villeneuve <hugo@hugovil.com> wrote: > > > > On Wed, 25 Feb 2026 18:12:13 +0200 > > > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > > > On Wed, Feb 25, 2026 at 10:54:01AM -0500, Hugo Villeneuve wrote: > > ... > > > > > > > +#include <linux/of.h> > > > > > > > > > > Is this in use? Or you wanted mod_devicetable.h for OF ID table? > > > > > > > > I need only OF ID table, so will replace with mod_devicetable.h. > > > > > > Hi Andy, > > > finally I need <linux/of.h> for of_match_ptr()... > > > > > > But I will keep <mod_devicetable.h> ... > > > > Do we need the dependency on OF? We may include of match pointer > > unconditionally and the driver could be used on ACPI systems with > > PRP0001 HID. > > Not only that. of_match_ptr() or ACPI_PTR() shouldn't be used in a new code > (there are, of course, _rare_ corner cases, which this one is not one of). Ok, will remove of_match_ptr() and also include <linux/of.h> Hugo Villeneuve
© 2016 - 2026 Red Hat, Inc.