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

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

[Hugo Villeneuve]

Hi Dmitry,

On Fri, 13 Mar 2026 11:22:39 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Mar 12, 2026 at 02:00:58PM -0400, 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.
> 
> LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

I was just wondering if this will go into v7.1, as I am not seing the
patch series in your input/next tree/branch for the moment? Let me know
if you need me to rebase it on v7.0.

-- 
Hugo Villeneuve

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

[Dmitry Torokhov]

Hi Hugo,

On Thu, Mar 12, 2026 at 02:00:58PM -0400, Hugo Villeneuve wrote:
> +
> +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);

This needs input_sync() as otherwise userspace is free to only recognize
the last MSC_SCAN event.

> +	}
> +
> +	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) {

This does not work if debouncing is disabled (debounce threshold is 0).

> +		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 code;
> +
> +	code = 0;
> +	for (unsigned int oline = 0; oline < keypad->nlines; oline++) {
> +		DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> +		int err;
> +
> +		/* 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. */
> +		err = gpiod_get_array_value_cansleep(keypad->line_gpios->ndescs,
> +						     keypad->line_gpios->desc,
> +						     keypad->line_gpios->info, values);
> +		if (err)
> +			return;

We need to deactivate the line on error too.

> +
> +		for (unsigned int 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)
> +{
> +	char **pin_names;
> +	char label[32];
> +
> +	snprintf(label, sizeof(label), "%s-pin", pdev->name);
> +
> +	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;
> +
> +	pin_names = devm_kasprintf_strarray(&pdev->dev, label, keypad->nlines);
> +	if (IS_ERR(pin_names))
> +		return PTR_ERR(pin_names);
> +
> +	for (unsigned int i = 0; i < keypad->line_gpios->ndescs; i++)
> +		gpiod_set_consumer_name(keypad->line_gpios->desc[i], pin_names[i]);
> +
> +	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);

Not all of these are required properties. If they are missing the driver
will operate on garbage values.

> +
> +	keypad->current_code = -1;
> +	keypad->debounce_code = -1;
> +	keypad->debounce_threshold = DIV_ROUND_UP(debounce_interval_ms, poll_interval_ms);

This will bomb if poll interval is 0.

> +
> +	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");

Missing "return".

> +
> +	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");

Missing "return".

I fixed it up and applied, please take a look in my 'next' branch and
tell me if I messed up.

Thanks.

-- 
Dmitry

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

[Hugo Villeneuve]

Hi Dmitry,

On Sun, 19 Apr 2026 21:47:40 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> Hi Hugo,
> 
> On Thu, Mar 12, 2026 at 02:00:58PM -0400, Hugo Villeneuve wrote:
> > +
> > +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);
> 
> This needs input_sync() as otherwise userspace is free to only recognize
> the last MSC_SCAN event.

Ok, now I get it, my code would have been be working only if it was an
if/else.

> 
> > +	}
> > +
> > +	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) {
> 
> This does not work if debouncing is disabled (debounce threshold is 0).

Yes.


> 
> > +		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 code;
> > +
> > +	code = 0;
> > +	for (unsigned int oline = 0; oline < keypad->nlines; oline++) {
> > +		DECLARE_BITMAP(values, MATRIX_MAX_ROWS);
> > +		int err;
> > +
> > +		/* 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. */
> > +		err = gpiod_get_array_value_cansleep(keypad->line_gpios->ndescs,
> > +						     keypad->line_gpios->desc,
> > +						     keypad->line_gpios->info, values);
> > +		if (err)
> > +			return;
> 
> We need to deactivate the line on error too.

Yer, good catch.

> 
> > +
> > +		for (unsigned int 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)
> > +{
> > +	char **pin_names;
> > +	char label[32];
> > +
> > +	snprintf(label, sizeof(label), "%s-pin", pdev->name);
> > +
> > +	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;
> > +
> > +	pin_names = devm_kasprintf_strarray(&pdev->dev, label, keypad->nlines);
> > +	if (IS_ERR(pin_names))
> > +		return PTR_ERR(pin_names);
> > +
> > +	for (unsigned int i = 0; i < keypad->line_gpios->ndescs; i++)
> > +		gpiod_set_consumer_name(keypad->line_gpios->desc[i], pin_names[i]);
> > +
> > +	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);
> 
> Not all of these are required properties. If they are missing the driver
> will operate on garbage values.

Yes.


> 
> > +
> > +	keypad->current_code = -1;
> > +	keypad->debounce_code = -1;
> > +	keypad->debounce_threshold = DIV_ROUND_UP(debounce_interval_ms, poll_interval_ms);
> 
> This will bomb if poll interval is 0.

Yes.

> 
> > +
> > +	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");
> 
> Missing "return".
> 
> > +
> > +	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");
> 
> Missing "return".

Ok for both.

> I fixed it up and applied, please take a look in my 'next' branch and
> tell me if I messed up.

Thank you for the review and the fixes.

I tested it on the real hardware and all is good.

So I imagine that it can still go into 7.1 since it is a new driver
and not a modification of an existing one?


-- 
Hugo Villeneuve

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

[Dmitry Torokhov]

On Mon, Apr 20, 2026 at 11:01:59AM -0400, Hugo Villeneuve wrote:
> I tested it on the real hardware and all is good.

Thank you for testing the changes.

> 
> So I imagine that it can still go into 7.1 since it is a new driver
> and not a modification of an existing one?

Yes, since this is a new driver I will include it in 7.1 pull request.

Thanks.

-- 
Dmitry

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

[Hugo Villeneuve]

On Mon, 20 Apr 2026 10:16:02 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Mon, Apr 20, 2026 at 11:01:59AM -0400, Hugo Villeneuve wrote:
> > I tested it on the real hardware and all is good.
> 
> Thank you for testing the changes.
> 
> > 
> > So I imagine that it can still go into 7.1 since it is a new driver
> > and not a modification of an existing one?
> 
> Yes, since this is a new driver I will include it in 7.1 pull request.
> 
> Thanks.

Great, thank you.

-- 
Hugo Villeneuve