Re: [PATCH v3 4/4] Input: charlieplex_keypad: add GPIO charlieplex keypad

[Hugo Villeneuve <hugo@hugovil.com>]

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