Re: [PATCH v1 2/2] Input: keyboard: Add Nuvoton MA35D1 keypad driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] Input: ma35d1_keypad - use guard(mutex) in suspend and resume
  callbacks
- [Low] Input: ma35d1_keypad - name errno variables error and return 0
  explicitly
--

commit ca9c8a019e4d03872087b6ba2c747cf4ce6faf45
Author: Feng-Yi Hsueh <fyhsueh4linux@gmail.com>

Input: keyboard: Add Nuvoton MA35D1 keypad driver

This commit adds an input driver for the Nuvoton MA35D1 hardware matrix
keypad controller, handling key press/release interrupts, input event
reporting, and clock management across open, close, suspend, and resume.

> diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
> new file mode 100644
> index 0000000000000..673ad8b9704bd
> --- /dev/null
> +++ b/drivers/input/keyboard/ma35d1_keypad.c

[ ... ]

> +/* Called with input->mutex held (open, PM resume). */
> +static int ma35d1_keypad_enable(struct ma35d1_keypad *keypad)
> +{
> +	int ret;
> +
> +	if (keypad->running)
> +		return 0;
> +
> +	ret = clk_prepare_enable(keypad->clk);
> +	if (ret)
> +		return ret;

[Severity: Low]
This isn't a bug, but in the input subsystem, variables that only hold 0
or a negative error code are conventionally named error or err rather than
ret.  The same applies to ma35d1_keypad_resume() and ma35d1_keypad_probe(),
and ma35d1_keypad_resume() should also explicitly return 0 on the success
path rather than returning the error variable directly.

[ ... ]

> +static int ma35d1_keypad_suspend(struct device *dev)
> +{
> +	struct ma35d1_keypad *keypad = dev_get_drvdata(dev);
> +	struct input_dev *input = keypad->input;
> +
> +	mutex_lock(&input->mutex);
> +	if (input_device_enabled(input))
> +		ma35d1_keypad_disable(keypad);
> +	mutex_unlock(&input->mutex);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_keypad_resume(struct device *dev)
> +{
> +	struct ma35d1_keypad *keypad = dev_get_drvdata(dev);
> +	struct input_dev *input = keypad->input;
> +	int ret = 0;
> +
> +	mutex_lock(&input->mutex);
> +	if (input_device_enabled(input))
> +		ret = ma35d1_keypad_enable(keypad);
> +	mutex_unlock(&input->mutex);
> +
> +	return ret;
> +}

[Severity: Low]
This isn't a bug, but new input drivers prefer guard(mutex)(&input->mutex)
over manual mutex_lock() and mutex_unlock() calls in
ma35d1_keypad_suspend() and ma35d1_keypad_resume().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110111419.89535-1-fyhsueh4linux@gmail.com?part=2