Re: [PATCH v7 05/13] mfd: zx297520v3: Add a clock and reset MFD driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Complex platform data structures (`mfd_cell` arrays) are passed directly through the `.data` field of the `of_device_id` match table.
- [Medium] The MFD core driver is incorrectly located in `drivers/soc/zte/` instead of `drivers/mfd/`.
- [Medium] The driver uses `PLATFORM_DEVID_NONE` instead of `PLATFORM_DEVID_AUTO` for subdevice indexing.
--

--- Patch [5]: [PATCH v7 05/13] mfd: zx297520v3: Add a clock and reset MFD driver ---
commit 040c43d18f894269b77d87091f84cd5ad6b9755d
Author: Stefan Dösinger <stefandoesinger@gmail.com>

mfd: zx297520v3: Add a clock and reset MFD driver

This driver registers child devices for the zx297520v3 clock and reset
controllers. The clk-zx297520v3 and reset-zte-zx297520v3 submitted in
the next patches will drive the respective functionalities.

> diff --git a/drivers/soc/zte/Makefile b/drivers/soc/zte/Makefile
> new file mode 100644
> index 0000000000000..090ba8aa06c1e
> --- /dev/null
> +++ b/drivers/soc/zte/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_ZTE_ZX297520V3_CRM) += zx297520v3-crm.o

[Severity: Medium]
Since this driver utilizes the devm_mfd_add_devices() API and registers
multiple child devices, should it be located in drivers/mfd/ instead of
drivers/soc/zte/?

The MFD guidelines state that placing MFD core drivers outside of drivers/mfd/
violates subsystem layering and bypasses proper subsystem maintainer review.

[ ... ]

> diff --git a/drivers/soc/zte/zx297520v3-crm.c b/drivers/soc/zte/zx297520v3-crm.c
> new file mode 100644
> index 0000000000000..8b82ccba4e90d
> --- /dev/null
> +++ b/drivers/soc/zte/zx297520v3-crm.c

[ ... ]

> +static int zx297520v3_crm_probe(struct platform_device *pdev)
> +{
> +	const struct zx297520v3_crm_data *data;
> +
> +	data = of_device_get_match_data(&pdev->dev);
> +	if (!data)
> +		return -ENODEV;
> +
> +	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_NONE, data->cells,

[Severity: Medium]
Is it intentional to use PLATFORM_DEVID_NONE here?

The MFD subsystem guidelines prefer using PLATFORM_DEVID_AUTO for subdevice
indexing to avoid sysfs device naming collisions and driver load failures.

> +				    data->num_cells, NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id of_match_zx297520v3_crm[] = {
> +	{ .compatible = "zte,zx297520v3-topcrm", .data = &zx297520v3_topcrm_data },

[Severity: High]
Does passing complex platform data structures (like the mfd_cell array inside
zx297520v3_topcrm_data) directly through the .data field of the of_device_id
match table pose a memory safety hazard and lead to initialization ordering
races?

The MFD guidelines suggest storing an integer ID or enum in the match table's
.data field, and using a switch statement in zx297520v3_crm_probe() to select
the correct static const mfd_cell array.

> +	{ .compatible = "zte,zx297520v3-matrixcrm", .data = &zx297520v3_matrixcrm_data },
> +	{ .compatible = "zte,zx297520v3-lspcrm", .data = &zx297520v3_lspcrm_data },
> +	{ }
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-zx29clk-v7-0-408411cfcf36@gmail.com?part=5

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy