Re: [PATCH v2 4/5] firmware: exynos-acpm: register ACPM clocks dev

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/08/2025 14:42, Tudor Ambarus wrote:
> +
> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
> +	ACPM_CLK(CLK_ACPM_DVFS_MIF, "mif"),
> +	ACPM_CLK(CLK_ACPM_DVFS_INT, "int"),
> +	ACPM_CLK(CLK_ACPM_DVFS_CPUCL0, "cpucl0"),
> +	ACPM_CLK(CLK_ACPM_DVFS_CPUCL1, "cpucl1"),
> +	ACPM_CLK(CLK_ACPM_DVFS_CPUCL2, "cpucl2"),
> +	ACPM_CLK(CLK_ACPM_DVFS_G3D, "g3d"),
> +	ACPM_CLK(CLK_ACPM_DVFS_G3DL2, "g3dl2"),
> +	ACPM_CLK(CLK_ACPM_DVFS_TPU, "tpu"),
> +	ACPM_CLK(CLK_ACPM_DVFS_INTCAM, "intcam"),
> +	ACPM_CLK(CLK_ACPM_DVFS_TNR, "tnr"),
> +	ACPM_CLK(CLK_ACPM_DVFS_CAM, "cam"),
> +	ACPM_CLK(CLK_ACPM_DVFS_MFC, "mfc"),
> +	ACPM_CLK(CLK_ACPM_DVFS_DISP, "disp"),
> +	ACPM_CLK(CLK_ACPM_DVFS_BO, "b0"),
> +};

I don't understand why clocks are defined in the firmware driver, not in
the clock driver.

This creates dependency of this patch on the clock patch, so basically
there is no way I will take it in one cycle.

> +
>  /**
>   * acpm_get_saved_rx() - get the response if it was already saved.
>   * @achan:	ACPM channel info.
> @@ -606,6 +636,7 @@ static void acpm_setup_ops(struct acpm_info *acpm)
>  
>  static int acpm_probe(struct platform_device *pdev)
>  {
> +	const struct acpm_clk_platform_data *acpm_clk_pdata;
>  	const struct acpm_match_data *match_data;
>  	struct device *dev = &pdev->dev;
>  	struct device_node *shmem;
> @@ -647,7 +678,30 @@ static int acpm_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, acpm);
>  
> -	return devm_of_platform_populate(dev);
> +	acpm_clk_pdata = match_data->acpm_clk_pdata;
> +	acpm->clk_pdev = platform_device_register_data(dev, "acpm-clocks",
> +						       PLATFORM_DEVID_NONE,
> +						       acpm_clk_pdata,
> +						       sizeof(*acpm_clk_pdata));
> +	if (IS_ERR(acpm->clk_pdev))
> +		return dev_err_probe(dev, PTR_ERR(acpm->clk_pdev),
> +				     "Failed to register ACPM clocks device.\n");
> +
> +	ret = devm_of_platform_populate(dev);
> +	if (ret) {
> +		platform_device_unregister(acpm->clk_pdev);

I think this should stick to devm-interfaces everywhere, not mix them,
to have exactly expected cleanup sequence. Now your remove() first
unregisters and then de-populates, which is different order than it was
done in probe(). Use devm-action handler for device unregistering.

Best regards,
Krzysztof