linux-kernel - Re: [PATCH v7 09/11] input: keyboard: Add support for MAX7360 keypad

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aBSii0rHox72GM5Y@smile.fi.intel.com>
Date: Fri, 2 May 2025 13:46:35 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Kamel Bouhara <kamel.bouhara@...tlin.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Michael Walle <mwalle@...nel.org>, Mark Brown <broonie@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	linux-input@...r.kernel.org, linux-pwm@...r.kernel.org,
	Grégory Clement <gregory.clement@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v7 09/11] input: keyboard: Add support for MAX7360 keypad

On Mon, Apr 28, 2025 at 01:57:27PM +0200, Mathieu Dubois-Briand wrote:
> Add driver for Maxim Integrated MAX7360 keypad controller, providing
> support for up to 64 keys, with a matrix of 8 columns and 8 rows.

...

> +static irqreturn_t max7360_keypad_irq(int irq, void *data)
> +{
> +	struct max7360_keypad *max7360_keypad = data;
> +	struct device *dev = max7360_keypad->input->dev.parent;
> +	unsigned int val;
> +	unsigned int row, col;
> +	unsigned int release;
> +	unsigned int code;
> +	int error;
> +
> +	do {
> +		error = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
> +		if (error) {
> +			dev_err(dev, "Failed to read max7360 FIFO");
> +			return IRQ_NONE;
> +		}
> +
> +		/* FIFO overflow: ignore it and get next event. */
> +		if (val == MAX7360_FIFO_OVERFLOW)
> +			dev_warn(dev, "max7360 FIFO overflow");

If many events are missing this will flood the logs, perhaps _ratelimited() ?

> +	} while (val == MAX7360_FIFO_OVERFLOW);

regmap_read_poll_timeout() ?

> +	if (val == MAX7360_FIFO_EMPTY) {
> +		dev_dbg(dev, "Got a spurious interrupt");
> +
> +		return IRQ_NONE;
> +	}
> +
> +	row = FIELD_GET(MAX7360_FIFO_ROW, val);
> +	col = FIELD_GET(MAX7360_FIFO_COL, val);
> +	release = val & MAX7360_FIFO_RELEASE;
> +
> +	code = MATRIX_SCAN_CODE(row, col, MAX7360_ROW_SHIFT);
> +
> +	dev_dbg(dev, "key[%d:%d] %s\n", row, col, release ? "release" : "press");
> +
> +	input_event(max7360_keypad->input, EV_MSC, MSC_SCAN, code);
> +	input_report_key(max7360_keypad->input, max7360_keypad->keycodes[code], !release);
> +	input_sync(max7360_keypad->input);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int max7360_keypad_open(struct input_dev *pdev)
> +{
> +	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
> +	struct device *dev = max7360_keypad->input->dev.parent;
> +	int error;
> +
> +	/* Somebody is using the device: get out of sleep. */
> +	error = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
> +				  MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
> +	if (error) {
> +		dev_err(dev, "Failed to write max7360 configuration: %d\n", error);

> +		return error;
> +	}
> +
> +	return 0;

Just

	return error;

should work instead of those 4 lines.

> +}

...

> +	size = device_property_count_u32(dev, propname);
> +	if (size <= 0) {
> +		dev_err(dev, "missing or malformed property %s: %d\n", propname, size);
> +		return size < 0 ? size : -EINVAL;
> +	}

Wouldn't be better to split these two?

...

> +	error = matrix_keypad_build_keymap(&keymap_data, NULL,
> +					   max7360_keypad->rows, max7360_keypad->cols,
> +					   max7360_keypad->keycodes, max7360_keypad->input);
> +
> +	return error;

	return matrix_...(...);

...

> +	device_init_wakeup(dev, true);
> +	error = dev_pm_set_wake_irq(dev, irq);
> +	if (error)
> +		dev_warn(dev, "Failed to set up wakeup irq: %d\n", error);

...

> +static void max7360_keypad_remove(struct platform_device *pdev)
> +{
> +	dev_pm_clear_wake_irq(&pdev->dev);

Shouldn't

	device_init_wakeup(false);

be called here?

-- 
With Best Regards,
Andy Shevchenko