Re: [PATCH v7 03/10] clk: realtek: Introduce a common probe()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect error checking for `devm_auxiliary_device_create()` causes silent failure of reset controller registration.
- [High] Missing Kconfig dependency on `MFD_SYSCON` causes systematic probe failure.
- [Medium] `RTK_CLK_COMMON` selects `RESET_RTK_COMMON` without inheriting its dependency on `RESET_CONTROLLER`, leading to unmet dependency warnings and disabled drivers.
--

commit f56275b75755d51046b72d7894740ba1aaeaf03b
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Introduce a common probe()

This commit adds rtk_clk_probe() to register clock hardware and the clock
provider for Realtek SoCs. It also registers an auxiliary device for the reset
controller if the "#reset-cells" property is present.

> diff --git a/drivers/clk/realtek/Kconfig b/drivers/clk/realtek/Kconfig
> new file mode 100644
> index 0000000000000..4e042e41f30da
> --- /dev/null
> +++ b/drivers/clk/realtek/Kconfig

[ ... ]

> +config RTK_CLK_COMMON
> +	tristate "Realtek Clock Common"
> +	select RESET_RTK_COMMON

Will selecting RESET_RTK_COMMON without inheriting its dependency on
RESET_CONTROLLER cause unmet dependency warnings?

In Kconfig, RESET_RTK_COMMON is implicitly dependent on RESET_CONTROLLER.
Selecting it without depends on RESET_CONTROLLER forces the symbol on even
when its dependencies are not met.

Additionally, because drivers/reset/ is only compiled when
CONFIG_RESET_CONTROLLER is enabled, the selected reset driver will silently
not be built if it is disabled.

Could depends on RESET_CONTROLLER be added here?

Should this config also add select MFD_SYSCON?

The driver unconditionally relies on device_node_to_regmap(), which is
provided by the MFD_SYSCON subsystem. If CONFIG_MFD_SYSCON is disabled,
the stub implementation unconditionally returns ERR_PTR(-ENOTSUPP).

Because RTK_CLK_COMMON does not select MFD_SYSCON and ARCH_REALTEK does not
select it either, the clock driver can be built without MFD_SYSCON, leading
to a systematic probe failure.

> +	select AUXILIARY_BUS
> +	help
> +	  Common helper code shared by Realtek clock controller drivers.

[ ... ]

> diff --git a/drivers/clk/realtek/common.c b/drivers/clk/realtek/common.c
> new file mode 100644
> index 0000000000000..56f6ba1af6ef4
> --- /dev/null
> +++ b/drivers/clk/realtek/common.c

[ ... ]

> +static int rtk_reset_controller_register(struct device *dev, const char *aux_name,
> +					 struct regmap *map)
> +{
> +	struct auxiliary_device *adev;
> +
> +	if (!of_property_present(dev->of_node, "#reset-cells"))
> +		return 0;
> +
> +	adev = devm_auxiliary_device_create(dev, aux_name, (void *)map);
> +
> +	if (IS_ERR(adev))
> +		return PTR_ERR(adev);

Is devm_auxiliary_device_create() returning NULL on failure instead of an
error pointer?

If it returns NULL, the IS_ERR(adev) check will be bypassed and the function
will silently return 0 on failure, leaving the required reset controller
unregistered.

Could this be updated to check for !adev and return -ENOMEM?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508111641.3192177-1-eleanor.lin@realtek.com?part=3