Re: [PATCH v6 10/12] input: keyboard: Add support for MAX7360 keypad

[Dmitry Torokhov]

Hi Mathieu,

On Wed, Apr 09, 2025 at 04:55:57PM +0200, Mathieu Dubois-Briand wrote:
> +struct max7360_keypad {
> +	struct input_dev *input;
> +	unsigned int rows;
> +	unsigned int cols;
> +	unsigned int debounce_ms;
> +	int irq;
> +	struct regmap *regmap;
> +	unsigned short keycodes[MAX7360_MAX_KEY_ROWS * MAX7360_MAX_KEY_COLS];
> +};
> +
> +static irqreturn_t max7360_keypad_irq(int irq, void *data)
> +{
> +	struct max7360_keypad *max7360_keypad = data;
> +	unsigned int val;
> +	unsigned int row, col;
> +	unsigned int release;
> +	unsigned int code;
> +	int ret;

int error;

> +
> +	do {
> +		ret = regmap_read(max7360_keypad->regmap, MAX7360_REG_KEYFIFO, &val);
> +		if (ret) {
> +			dev_err(&max7360_keypad->input->dev, "Failed to read max7360 FIFO");

This will return name pf the input device, whereas logging name of the
platform device (representing the hardware device) would be much more
interesting. You can either use max7360_keypad->input->dev.parent, or,
better yet, add *dev pointer to struct max7360_keypad.

> +			return IRQ_NONE;
> +		}
> +
> +		/* FIFO overflow: ignore it and get next event. */
> +		if (val == MAX7360_FIFO_OVERFLOW)
> +			dev_warn(&max7360_keypad->input->dev, "max7360 FIFO overflow");
> +	} while (val == MAX7360_FIFO_OVERFLOW);
> +
> +	if (val == MAX7360_FIFO_EMPTY) {
> +		dev_dbg(&max7360_keypad->input->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(&max7360_keypad->input->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);
> +	int ret;

"int error" for variables holding error codes or 0. Also elsewhere in
the driver.

> +
> +	/* Somebody is using the device: get out of sleep. */
> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG,
> +				MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP);
> +	if (ret)
> +		dev_err(&max7360_keypad->input->dev, "Failed to write max7360 configuration\n");

Log error code?

Explicit error return please.
		retrun error;
	}
> +
> +	return ret;

	return 0;

> +}
> +
> +static void max7360_keypad_close(struct input_dev *pdev)
> +{
> +	struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev);
> +	int ret;
> +
> +	/* Nobody is using the device anymore: go to sleep. */
> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, MAX7360_CFG_SLEEP, 0);
> +	if (ret)
> +		dev_err(&max7360_keypad->input->dev,
> +			"Failed to write max7360 configuration\n");

Log error code?

> +}
> +
> +static int max7360_keypad_hw_init(struct max7360_keypad *max7360_keypad)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	val = max7360_keypad->debounce_ms - MAX7360_DEBOUNCE_MIN;
> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_DEBOUNCE,
> +				MAX7360_DEBOUNCE,
> +				FIELD_PREP(MAX7360_DEBOUNCE, val));
> +	if (ret) {
> +		return dev_err_probe(&max7360_keypad->input->dev, ret,
> +			"Failed to write max7360 debounce configuration\n");
> +	}

No need for braces with single line statements.

> +
> +	ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_INTERRUPT,
> +				MAX7360_INTERRUPT_TIME_MASK,
> +				FIELD_PREP(MAX7360_INTERRUPT_TIME_MASK, 1));
> +	if (ret) {
> +		return dev_err_probe(&max7360_keypad->input->dev, ret,
> +			"Failed to write max7360 keypad interrupt configuration\n");
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_keypad_build_keymap(struct max7360_keypad *max7360_keypad)
> +{
> +	struct input_dev *input_dev = max7360_keypad->input;
> +	struct device *dev = input_dev->dev.parent->parent;
> +	struct matrix_keymap_data keymap_data;
> +	const char *propname = "linux,keymap";
> +	unsigned int max_keys;
> +	int size;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	max_keys = max7360_keypad->cols * max7360_keypad->rows;
> +	if (size > max_keys) {
> +		dev_err(dev, "%s size overflow (%d vs max %u)\n", propname, size, max_keys);
> +		return -EINVAL;
> +	}
> +
> +	u32 *keys __free(kfree) = kmalloc_array(size, sizeof(*keys), GFP_KERNEL);
> +	if (!keys)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u32_array(dev, propname, keys, size);
> +	if (ret) {
> +		dev_err(dev, "failed to read %s property: %d\n", propname, ret);
> +		return ret;
> +	}
> +
> +	keymap_data.keymap = keys;
> +	keymap_data.keymap_size = size;
> +	ret = matrix_keypad_build_keymap(&keymap_data, NULL, max7360_keypad->rows, max7360_keypad->cols,
> +					 max7360_keypad->keycodes, max7360_keypad->input);

What if it fails? Error handling please.

Also, it looks like you are repeating what matrix_keypad_build_keymap()
is already doing. If you pass NULL as keymap data, won't it do the right
thing?

> +
> +	return 0;
> +}
> +
> +static int max7360_keypad_parse_fw(struct device *dev,
> +				   struct max7360_keypad *max7360_keypad,
> +				   bool *autorepeat)
> +{
> +	int ret;
> +
> +	ret = matrix_keypad_parse_properties(dev->parent, &max7360_keypad->rows,
> +					     &max7360_keypad->cols);
> +	if (ret)
> +		return ret;
> +
> +	if (!max7360_keypad->rows || !max7360_keypad->cols ||
> +	    max7360_keypad->rows > MAX7360_MAX_KEY_ROWS ||
> +	    max7360_keypad->cols > MAX7360_MAX_KEY_COLS) {
> +		dev_err(dev, "Invalid number of columns or rows (%ux%u)\n",
> +			max7360_keypad->cols, max7360_keypad->rows);
> +		return -EINVAL;
> +	}
> +
> +	*autorepeat = device_property_read_bool(dev->parent, "autorepeat");
> +
> +	max7360_keypad->debounce_ms = MAX7360_DEBOUNCE_MIN;
> +	ret = device_property_read_u32(dev->parent, "keypad-debounce-delay-ms",
> +				       &max7360_keypad->debounce_ms);
> +	if (ret == -EINVAL) {
> +		dev_info(dev, "Using default keypad-debounce-delay-ms: %u\n",
> +			 max7360_keypad->debounce_ms);
> +	} else if (ret < 0) {
> +		dev_err(dev, "Failed to read keypad-debounce-delay-ms property\n");
> +		return ret;
> +	}
> +
> +	if (!in_range(max7360_keypad->debounce_ms, MAX7360_DEBOUNCE_MIN,
> +		      MAX7360_DEBOUNCE_MAX - MAX7360_DEBOUNCE_MIN)) {
> +		dev_err(dev, "Invalid keypad-debounce-delay-ms: %u, should be between %u and %u.\n",
> +			max7360_keypad->debounce_ms, MAX7360_DEBOUNCE_MIN, MAX7360_DEBOUNCE_MAX);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int max7360_keypad_probe(struct platform_device *pdev)
> +{
> +	struct max7360_keypad *max7360_keypad;
> +	struct device *dev = &pdev->dev;
> +	struct input_dev *input;
> +	struct regmap *regmap;
> +	bool autorepeat;
> +	int ret;
> +	int irq;
> +
> +	regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!regmap)
> +		dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n");

		return dev_err_probe(...) ?

Thanks.

-- 
Dmitry