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

[Hugo Villeneuve]

Hi Dmitry,

On Wed, 25 Feb 2026 09:12:57 -0800
Dmitry Torokhov <dmitry.torokhov at 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 at 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 at 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