Re: [PATCH v6 2/3] clk: eswin: Add eic7700 HSP clock driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Global static clock arrays are modified during probe without locking, breaking support for multiple instances.
- [Low] The Kconfig symbol `COMMON_CLK_EIC7700_HSP` lacks the `default ARCH_ESWIN` property, which is inconsistent with the base clock driver.
--

commit 5c87f865ca8ae746d80ff71fdbbce73f7be502ec
Author: Xuyang Dong <dongxuyang@eswincomputing.com>

clk: eswin: Add eic7700 HSP clock driver

Add driver for the ESWIN EIC7700 high-speed peripherals system clock
controller and register an auxiliary device for system reset controller
which is named as "hsp-reset".

> diff --git a/drivers/clk/eswin/Kconfig b/drivers/clk/eswin/Kconfig
> index 0406ec499ec9c..37857bd773356 100644
> --- a/drivers/clk/eswin/Kconfig
> +++ b/drivers/clk/eswin/Kconfig
[ ... ]
> @@ -13,3 +13,16 @@ config COMMON_CLK_EIC7700
>  	  SoC. The clock controller generates and supplies clocks to various
>  	  peripherals within the SoC.
>  	  Say yes here to support the clock controller on the EIC7700 SoC.
> +
> +config COMMON_CLK_EIC7700_HSP
> +	tristate "EIC7700 HSP Clock Driver"
> +	depends on ARCH_ESWIN || COMPILE_TEST

This isn't a bug, but should this config have default ARCH_ESWIN to match
the base clock driver? Without it, the high-speed peripheral clocks might
be unexpectedly disabled by default on ESWIN platforms.

> +	select AUXILIARY_BUS
> +	select COMMON_CLK_EIC7700
> +	select RESET_EIC7700_HSP if RESET_CONTROLLER
> +	select REGMAP_MMIO

[ ... ]
> diff --git a/drivers/clk/eswin/clk-eic7700-hsp.c b/drivers/clk/eswin/clk-eic7700-hsp.c
> new file mode 100644
> index 0000000000000..3383aa64948a6
> --- /dev/null
> +++ b/drivers/clk/eswin/clk-eic7700-hsp.c
[ ... ]
> +static struct eswin_fixed_factor_clock eic7700_hsp_factor_clks[] = {
> +	ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_CFG_DIV2, "factor_hsp_cfg_div2",
> +		     hsp_cfg, 1, 2, 0),
> +	ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_CFG_DIV4, "factor_hsp_cfg_div4",
> +		     hsp_cfg, 1, 4, 0),
> +	ESWIN_FACTOR(EIC7700_HSP_CLK_FAC_MMC_DIV10, "factor_hsp_mmc_div10",
> +		     hsp_mmc, 1, 10, 0),
> +};
[ ... ]
> +static int eic7700_hsp_clk_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct auxiliary_device *adev;
> +	struct eswin_clock_data *data;
> +	struct regmap *regmap;
> +	struct clk_hw *hw;
> +	int i, ret;
> +
> +	data = eswin_clk_init(pdev, EIC7700_HSP_NR_CLKS);
> +	if (IS_ERR(data))
> +		return dev_err_probe(dev, PTR_ERR(data),
> +				     "failed to get clk data!\n");
> +
> +	regmap = devm_regmap_init_mmio(dev, data->base,
> +				       &eic7700_hsp_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "failed to get regmap!\n");
> +
> +	ret = eswin_clk_register_fixed_factor(dev, eic7700_hsp_factor_clks,
> +					      ARRAY_SIZE(eic7700_hsp_factor_clks),
> +					      data);

Is it safe to pass the global static array eic7700_hsp_factor_clks to this
function?

Looking at eswin_clk_register_fixed_factor() in drivers/clk/eswin/clk.c, it
modifies the array during probe:

int eswin_clk_register_fixed_factor(...)
{
    ...
        clks[i].hw = *clk_hw;
        data->clk_data.hws[clks[i].id] = clk_hw;
    ...
}

If multiple instances of this driver probe concurrently or defer, could this
data race corrupt the CCF parent-child linkages? The same pattern appears
to affect eic7700_hsp_gate_clks and eic7700_hsp_mux_clks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519081431.1424-1-dongxuyang@eswincomputing.com?part=2