Re: [PATCH 2/4] soc: toshiba: tc9564: add a new syscon driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] soc: toshiba: SOC_TOSHIBA_TC9564_SYSCON depends on hidden MFD_CORE
- [Low] soc: toshiba: MFD driver calling devm_mfd_add_devices() outside drivers/mfd/
- [Low] soc: toshiba: Kconfig help and commit message mismatch tc9564-syscon.c
--

commit 0d303444d1af16f91e36247fd63b0cb020f8be95
Author: Alex Elder <elder@riscstar.com>

soc: toshiba: tc9564: add a new syscon driver

This commit introduces a platform driver for the Toshiba TC9564 config
system controller, registering MFD child devices for the chip's clock
and reset controllers.

> diff --git a/drivers/soc/toshiba/Kconfig b/drivers/soc/toshiba/Kconfig
> new file mode 100644
> index 0000000000000..91adcbe71905c
> --- /dev/null
> +++ b/drivers/soc/toshiba/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config SOC_TOSHIBA_TC9564_SYSCON
> +	tristate "Toshiba TC9564 config system controller support"
> +	depends on TC9564_PCI
> +	depends on MFD_CORE
> +	default TC9564_PCI

[Severity: Medium]
Should SOC_TOSHIBA_TC9564_SYSCON use "select MFD_CORE" instead of
"depends on MFD_CORE"?

In drivers/mfd/Kconfig, MFD_CORE is a promptless helper symbol with
"default n":

    config MFD_CORE
        tristate
        select IRQ_DOMAIN
        default n

When a kernel configuration enables CONFIG_TC9564_PCI without any
unrelated MFD driver that selects MFD_CORE, the dependency on MFD_CORE
remains unmet and SOC_TOSHIBA_TC9564_SYSCON cannot be enabled.
Similarly, if TC9564_PCI is built-in (y) and only modular drivers select
MFD_CORE, SOC_TOSHIBA_TC9564_SYSCON is restricted to modular (m).

> +	help
> +	  This enables support for the config system controller for the
> +	  TC9564 SoC.  It implements an MMIO regmap to manage access to
> +	  two pages of I/O memory, and serves as a clock and reset
> +	  controller for IP blocks within the chip.

[Severity: Low]
Does this help text (and the commit message) accurately describe what
drivers/soc/toshiba/tc9564-syscon.c implements?

In tc9564_config_syscon_probe(), the driver only calls
devm_mfd_add_devices() to register the toshiba-tc9564-clock and
toshiba-tc9564-reset MFD cells; it does not set up an MMIO regmap (nor
does SOC_TOSHIBA_TC9564_SYSCON select MFD_SYSCON) or implement the clock
and reset controller providers itself. Could the help text and commit
description be updated to reflect that this driver instantiates the
clock and reset child devices?

[ ... ]

> diff --git a/drivers/soc/toshiba/tc9564-syscon.c b/drivers/soc/toshiba/tc9564-syscon.c
> new file mode 100644
> index 0000000000000..0d0e2422b82d0
> --- /dev/null
> +++ b/drivers/soc/toshiba/tc9564-syscon.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * Copyright (C) 2026 by RISCstar Solutions Corporation.  All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/device-id/of.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +static const struct mfd_cell tc9564_config_syscon_cells[] = {
> +	MFD_CELL_NAME("toshiba-tc9564-clock"),
> +	MFD_CELL_NAME("toshiba-tc9564-reset"),
> +};
> +
> +static int tc9564_config_syscon_probe(struct platform_device *pdev)
> +{
> +	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
> +				    tc9564_config_syscon_cells,
> +				    ARRAY_SIZE(tc9564_config_syscon_cells),
> +				    NULL, 0, NULL);
> +}

[Severity: Low]
Since tc9564_config_syscon_probe() in drivers/soc/toshiba/tc9564-syscon.c
is purely an MFD driver calling devm_mfd_add_devices() to register child
devices, should this driver be placed under drivers/mfd/ rather than
drivers/soc/toshiba/ (with an "mfd: tc9564: ..." commit subject prefix)
to follow MFD subsystem placement conventions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104004332.2141282-1-elder@riscstar.com?part=2