Re: [PATCH v2 2/2] Input: atmel_mxt_ts: Allow per-board config via DT compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The OF device node reference for the root node is leaked when falling back to the generic configuration loader.
- [Medium] Mismatch between the constructed firmware filename and the filename documented in the commit message.
--

commit e3439c3fb5cf6c8c740fae5a27659ea3cb9e7436
Author: Hendrik Noack <hendrik-noack@gmx.de>

Input: atmel_mxt_ts: Allow per-board config via DT compatible

This commit modifies the Atmel maXTouch driver to load a board-specific
configuration file based on the device tree compatible string. If the specific
configuration file is not found, it falls back to the original generic
configuration behavior.

> Extend the driver to support per-board config files keyed by the device
> tree compatible string. For DT-based systems, the driver now looks up the
> primary compatible of the board and searches for a config file named:
> 
>   atmel/maxtouch,<compatible>.cfg

[Severity: Medium]
The documentation in the commit message uses a comma before the compatible
string, but the code implementation constructs the filename using a dot.

Is it intended to use a dot here, and if so, could the commit message
be updated to match the code?

> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 07690c3bff06e..a9e9f151d0424 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c

[ ... ]

> @@ -2233,6 +2235,64 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)

[ ... ]

> +static int mxt_invoke_config_loader(struct mxt_data *data, bool device_specific)
> +{
> +	struct device_node *root;
> +	char *board_type = NULL;
> +	char *fw_name;
> +	void (*cb)(const struct firmware *fw, void *context);
> +	int error;
> +
> +	root = of_find_node_by_path("/");
> +	if (device_specific && root) {
> +		const char *tmp;
> +
> +		if (!of_property_read_string_index(root, "compatible", 0, &tmp))
> +			board_type = kstrdup(tmp, GFP_KERNEL);
> +
> +		of_node_put(root);
> +	}

[Severity: Medium]
Does this leak the root device node reference when device_specific is false?

The call to of_find_node_by_path() unconditionally acquires a reference,
but of_node_put() is only called inside the if block. The fallback
configuration path triggered from mxt_board_config_cb() passes false
for device_specific, which would bypass the release.

> +
> +	if (board_type) {
> +		/* get rid of '/' in the compatible string to be able to find the FW */
> +		strreplace(board_type, '/', '-');
> +
> +		fw_name = kasprintf(GFP_KERNEL, "%s/%s.%s%s", MXT_CFG_FOLDER, MXT_CFG_NAME,
> +				    board_type, MXT_CFG_EXTENSION);

[Severity: Medium]
Does this format string construct the filename with a dot instead of the comma
described in the commit message?

This format string expands to atmel/maxtouch.<compatible>.cfg which might lead
to unexpected firmware loading failures for users following the naming scheme
documented in the commit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523152332.86864-1-hendrik-noack@gmx.de?part=2