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

Dmitry Torokhov
2025-04-09T18:40:51Z

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