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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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