Re: [PATCH v2 06/13] soc: synaptics: add SL2610 system controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] soc: synaptics: new MFD driver sl2610-syscon placed outside
  drivers/mfd
- [Low] soc: synaptics: MFD used for single-function AVIO and CPU clock
  blocks
- [Low] soc: synaptics: mfd_cell arrays passed via match data in
  sl2610_syscon_probe()
--

commit 13fffaa081530baa348dec7822fd2e447d70af16
Author: Jisheng Zhang <jszhang@kernel.org>

soc: synaptics: add SL2610 system controller support

This commit adds a driver for the Synaptics SL2610 system control blocks to
register their clock, reset, and pin controller child devices via MFD.

> diff --git a/drivers/soc/synaptics/Kconfig b/drivers/soc/synaptics/Kconfig
> new file mode 100644
> index 0000000000000..9f0324a1a3b7a
> --- /dev/null
> +++ b/drivers/soc/synaptics/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config SOC_SL2610_SYSCON
> +	tristate "Synaptics SL2610 system controller"
> +	depends on ARCH_BERLIN || COMPILE_TEST
> +	depends on OF && HAS_IOMEM
> +	select MFD_CORE
> +	select MFD_SYSCON

[Severity: Low]
Should this driver live under drivers/mfd/ instead of drivers/soc/synaptics/?

sl2610-syscon.c does not implement any SoC bus or SoC-specific service and
only calls devm_mfd_add_devices() to instantiate MFD child devices.  New core
MFD drivers and new calls to devm_mfd_add_devices() belong in drivers/mfd/.

> diff --git a/drivers/soc/synaptics/sl2610-syscon.c b/drivers/soc/synaptics/sl2610-syscon.c
> new file mode 100644
> index 0000000000000..c6fc2955ac0de
> --- /dev/null
> +++ b/drivers/soc/synaptics/sl2610-syscon.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 Synaptics Incorporated
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +struct sl2610_syscon_data {
> +	const struct mfd_cell *cells;
> +	unsigned int num_cells;
> +};
> +
> +static const struct mfd_cell sl2610_soc_cells[] = {
> +	MFD_CELL_NAME("sl2610-soc-clk"),
> +	MFD_CELL_NAME("sl2610-soc-reset"),
> +	MFD_CELL_NAME("sl2610-soc-pinctrl"),
> +};
> +
> +static const struct mfd_cell sl2610_mcu_cells[] = {
> +	MFD_CELL_NAME("sl2610-mcu-clk"),
> +	MFD_CELL_NAME("sl2610-mcu-reset"),
> +	MFD_CELL_NAME("sl2610-mcu-pinctrl"),
> +};
> +
> +static const struct mfd_cell sl2610_avio_cells[] = {
> +	MFD_CELL_NAME("sl2610-avio-clk"),
> +};
> +
> +static const struct mfd_cell sl2610_cpu_cells[] = {
> +	MFD_CELL_NAME("sl2610-cpu-clk"),
> +};

[Severity: Low]
Why use MFD for the single-function AVIO and CPU clock blocks?

sl2610_avio_cells and sl2610_cpu_cells each register only a single clock
child device, and per the syna,sl2610-soc-syscon binding neither block
provides reset or pinctrl functions.  MFD is intended for devices that
register multiple children across different subsystems, whereas the clock
driver can bind directly to syna,sl2610-avio-syscon and
syna,sl2610-cpu-syscon while keeping shared regmap access through the
generic syscon compatible.

> +
> +static const struct sl2610_syscon_data sl2610_soc_data = {
> +	.cells = sl2610_soc_cells,
> +	.num_cells = ARRAY_SIZE(sl2610_soc_cells),
> +};
> +
> +static const struct sl2610_syscon_data sl2610_mcu_data = {
> +	.cells = sl2610_mcu_cells,
> +	.num_cells = ARRAY_SIZE(sl2610_mcu_cells),
> +};
> +
> +static const struct sl2610_syscon_data sl2610_avio_data = {
> +	.cells = sl2610_avio_cells,
> +	.num_cells = ARRAY_SIZE(sl2610_avio_cells),
> +};
> +
> +static const struct sl2610_syscon_data sl2610_cpu_data = {
> +	.cells = sl2610_cpu_cells,
> +	.num_cells = ARRAY_SIZE(sl2610_cpu_cells),
> +};
> +
> +static int sl2610_syscon_probe(struct platform_device *pdev)
> +{
> +	const struct sl2610_syscon_data *data = device_get_match_data(&pdev->dev);
> +
> +	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
> +				    data->cells, data->num_cells, NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id sl2610_syscon_of_match[] = {
> +	{ .compatible = "syna,sl2610-soc-syscon", .data = &sl2610_soc_data },
> +	{ .compatible = "syna,sl2610-mcu-syscon", .data = &sl2610_mcu_data },
> +	{ .compatible = "syna,sl2610-avio-syscon", .data = &sl2610_avio_data },
> +	{ .compatible = "syna,sl2610-cpu-syscon", .data = &sl2610_cpu_data },
> +	{ }
> +};

[Severity: Low]
This isn't a bug in normal DT probing, but MFD drivers should not pass
mfd_cell arrays through the .data field of of_device_id tables.  In addition,
if the driver is bound without an OF match (such as via sysfs
driver_override), device_get_match_data() returns NULL and
sl2610_syscon_probe() dereferences a NULL data pointer.

Could sl2610_syscon_of_match store an enum or integer ID in .data and select
the static const mfd_cell array with a switch statement (and a default error
return) in sl2610_syscon_probe()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115102401.3520-1-jszhang@kernel.org?part=6