[PATCH v4 4/5] clk: samsung: add Exynos ACPM clock driver

Tudor Ambarus posted 5 patches 5 months ago
There is a newer version of this series
[PATCH v4 4/5] clk: samsung: add Exynos ACPM clock driver
Posted by Tudor Ambarus 5 months ago
Add the Exynos ACPM clock driver. It provides support for clocks that
are controlled by firmware that implements the ACPM interface.

Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
 drivers/clk/samsung/Kconfig    |  10 +++
 drivers/clk/samsung/Makefile   |   1 +
 drivers/clk/samsung/clk-acpm.c | 184 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+)

diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
index 76a494e95027af26272e30876a87ac293bd56dfa..70a8b82a0136b4d0213d8ff95e029c52436e5c7f 100644
--- a/drivers/clk/samsung/Kconfig
+++ b/drivers/clk/samsung/Kconfig
@@ -95,6 +95,16 @@ config EXYNOS_CLKOUT
 	  status of the certains clocks from SoC, but it could also be tied to
 	  other devices as an input clock.
 
+config EXYNOS_ACPM_CLK
+	tristate "Clock driver controlled via ACPM interface"
+	depends on EXYNOS_ACPM_PROTOCOL || (COMPILE_TEST && !EXYNOS_ACPM_PROTOCOL)
+	help
+	  This driver provides support for clocks that are controlled by
+	  firmware that implements the ACPM interface.
+
+	  This driver uses the ACPM interface to interact with the firmware
+	  providing all the clock controlls.
+
 config TESLA_FSD_COMMON_CLK
 	bool "Tesla FSD clock controller support" if COMPILE_TEST
 	depends on COMMON_CLK_SAMSUNG
diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
index b77fe288e4bb484c68d1ff497acc0b83d132ea03..04b63436b12f6f5169575d74f54b105e97bbb052 100644
--- a/drivers/clk/samsung/Makefile
+++ b/drivers/clk/samsung/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos990.o
 obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynosautov9.o
 obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynosautov920.o
 obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-gs101.o
+obj-$(CONFIG_EXYNOS_ACPM_CLK)		+= clk-acpm.o
 obj-$(CONFIG_S3C64XX_COMMON_CLK)	+= clk-s3c64xx.o
 obj-$(CONFIG_S5PV210_COMMON_CLK)	+= clk-s5pv210.o clk-s5pv210-audss.o
 obj-$(CONFIG_TESLA_FSD_COMMON_CLK)	+= clk-fsd.o
diff --git a/drivers/clk/samsung/clk-acpm.c b/drivers/clk/samsung/clk-acpm.c
new file mode 100644
index 0000000000000000000000000000000000000000..8566495265ee3e06dbf370f9e424d5540f5c7457
--- /dev/null
+++ b/drivers/clk/samsung/clk-acpm.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Samsung Exynos ACPM protocol based clock driver.
+ *
+ * Copyright 2025 Linaro Ltd.
+ */
+
+#include <linux/array_size.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/firmware/samsung/exynos-acpm-protocol.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+
+struct acpm_clk {
+	u32 id;
+	struct clk_hw hw;
+	unsigned int mbox_chan_id;
+	const struct acpm_handle *handle;
+};
+
+struct acpm_clk_variant {
+	const char *name;
+};
+
+struct acpm_clk_driver_data {
+	const struct acpm_clk_variant *clks;
+	unsigned int nr_clks;
+	unsigned int mbox_chan_id;
+};
+
+#define to_acpm_clk(clk) container_of(clk, struct acpm_clk, hw)
+
+#define ACPM_CLK(cname)					\
+	{						\
+		.name		= cname,		\
+	}
+
+static const struct acpm_clk_variant gs101_acpm_clks[] = {
+	ACPM_CLK("mif"),
+	ACPM_CLK("int"),
+	ACPM_CLK("cpucl0"),
+	ACPM_CLK("cpucl1"),
+	ACPM_CLK("cpucl2"),
+	ACPM_CLK("g3d"),
+	ACPM_CLK("g3dl2"),
+	ACPM_CLK("tpu"),
+	ACPM_CLK("intcam"),
+	ACPM_CLK("tnr"),
+	ACPM_CLK("cam"),
+	ACPM_CLK("mfc"),
+	ACPM_CLK("disp"),
+	ACPM_CLK("b0"),
+};
+
+static const struct acpm_clk_driver_data acpm_clk_gs101 = {
+	.clks = gs101_acpm_clks,
+	.nr_clks = ARRAY_SIZE(gs101_acpm_clks),
+	.mbox_chan_id = 0,
+};
+
+static unsigned long acpm_clk_recalc_rate(struct clk_hw *hw,
+					  unsigned long parent_rate)
+{
+	struct acpm_clk *clk = to_acpm_clk(hw);
+
+	return clk->handle->ops.dvfs_ops.get_rate(clk->handle,
+					clk->mbox_chan_id, clk->id, 0);
+}
+
+static int acpm_clk_determine_rate(struct clk_hw *hw,
+				   struct clk_rate_request *req)
+{
+	/*
+	 * We can't figure out what rate it will be, so just return the
+	 * rate back to the caller. acpm_clk_recalc_rate() will be called
+	 * after the rate is set and we'll know what rate the clock is
+	 * running at then.
+	 */
+	return 0;
+}
+
+static int acpm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+			     unsigned long parent_rate)
+{
+	struct acpm_clk *clk = to_acpm_clk(hw);
+
+	return clk->handle->ops.dvfs_ops.set_rate(clk->handle,
+					clk->mbox_chan_id, clk->id, rate);
+}
+
+static const struct clk_ops acpm_clk_ops = {
+	.recalc_rate = acpm_clk_recalc_rate,
+	.determine_rate = acpm_clk_determine_rate,
+	.set_rate = acpm_clk_set_rate,
+};
+
+static int acpm_clk_ops_init(struct device *dev, struct acpm_clk *aclk,
+			     const char *name)
+{
+	struct clk_init_data init = {};
+
+	init.name = name;
+	init.ops = &acpm_clk_ops;
+	aclk->hw.init = &init;
+
+	return devm_clk_hw_register(dev, &aclk->hw);
+}
+
+static int acpm_clk_probe(struct platform_device *pdev)
+{
+	const struct acpm_handle *acpm_handle;
+	struct clk_hw_onecell_data *clk_data;
+	struct clk_hw **hws;
+	struct device *dev = &pdev->dev;
+	struct acpm_clk *aclks;
+	unsigned int mbox_chan_id;
+	int i, err, count;
+
+	acpm_handle = devm_acpm_get_by_node(dev, dev->parent->of_node);
+	if (IS_ERR(acpm_handle))
+		return dev_err_probe(dev, PTR_ERR(acpm_handle),
+				     "Failed to get acpm handle.\n");
+
+	count = acpm_clk_gs101.nr_clks;
+	mbox_chan_id = acpm_clk_gs101.mbox_chan_id;
+
+	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
+				GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+
+	clk_data->num = count;
+	hws = clk_data->hws;
+
+	aclks = devm_kcalloc(dev, count, sizeof(*aclks), GFP_KERNEL);
+	if (!aclks)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		struct acpm_clk *aclk = &aclks[i];
+
+		/*
+		 * The code assumes the clock IDs start from zero,
+		 * are sequential and do not have gaps.
+		 */
+		aclk->id = i;
+		aclk->handle = acpm_handle;
+		aclk->mbox_chan_id = mbox_chan_id;
+
+		hws[i] = &aclk->hw;
+
+		err = acpm_clk_ops_init(dev, aclk,
+					acpm_clk_gs101.clks[i].name);
+		if (err)
+			return dev_err_probe(dev, err,
+					     "Failed to register clock.\n");
+	}
+
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					   clk_data);
+}
+
+static const struct platform_device_id acpm_clk_id[] = {
+	{ "gs101-acpm-clk" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, acpm_clk_id);
+
+static struct platform_driver acpm_clk_driver = {
+	.driver	= {
+		.name = "acpm-clocks",
+	},
+	.probe = acpm_clk_probe,
+	.id_table = acpm_clk_id,
+};
+module_platform_driver(acpm_clk_driver);
+
+MODULE_AUTHOR("Tudor Ambarus <tudor.ambarus@linaro.org>");
+MODULE_DESCRIPTION("Samsung Exynos ACPM clock driver");
+MODULE_LICENSE("GPL");

-- 
2.51.0.355.g5224444f11-goog
Re: [PATCH v4 4/5] clk: samsung: add Exynos ACPM clock driver
Posted by Stephen Boyd 4 months, 3 weeks ago
Quoting Tudor Ambarus (2025-09-08 06:12:45)
> diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
> index 76a494e95027af26272e30876a87ac293bd56dfa..70a8b82a0136b4d0213d8ff95e029c52436e5c7f 100644
> --- a/drivers/clk/samsung/Kconfig
> +++ b/drivers/clk/samsung/Kconfig
> @@ -95,6 +95,16 @@ config EXYNOS_CLKOUT
>           status of the certains clocks from SoC, but it could also be tied to
>           other devices as an input clock.
>  
> +config EXYNOS_ACPM_CLK
> +       tristate "Clock driver controlled via ACPM interface"
> +       depends on EXYNOS_ACPM_PROTOCOL || (COMPILE_TEST && !EXYNOS_ACPM_PROTOCOL)

Why is COMPILE_TEST limited to !EXYNOS_ACPM_PROTOCOL?

> +       help
> +         This driver provides support for clocks that are controlled by
> +         firmware that implements the ACPM interface.
> +
> +         This driver uses the ACPM interface to interact with the firmware
> +         providing all the clock controlls.
> +
>  config TESLA_FSD_COMMON_CLK
>         bool "Tesla FSD clock controller support" if COMPILE_TEST
>         depends on COMMON_CLK_SAMSUNG
> diff --git a/drivers/clk/samsung/clk-acpm.c b/drivers/clk/samsung/clk-acpm.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..8566495265ee3e06dbf370f9e424d5540f5c7457
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-acpm.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Samsung Exynos ACPM protocol based clock driver.
> + *
> + * Copyright 2025 Linaro Ltd.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/firmware/samsung/exynos-acpm-protocol.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

Is this include used?

> +#include <linux/platform_device.h>
> +#include <linux/types.h>

Are you avoiding kernel.h? If so, please include container_of.h and
device/devres.h to avoid implicit includes.

> +
> +struct acpm_clk {
> +       u32 id;
> +       struct clk_hw hw;
> +       unsigned int mbox_chan_id;
> +       const struct acpm_handle *handle;
> +};
> +
> +struct acpm_clk_variant {
> +       const char *name;
> +};
> +
> +struct acpm_clk_driver_data {
> +       const struct acpm_clk_variant *clks;
> +       unsigned int nr_clks;
> +       unsigned int mbox_chan_id;
> +};
> +
> +#define to_acpm_clk(clk) container_of(clk, struct acpm_clk, hw)
> +
> +#define ACPM_CLK(cname)                                        \
> +       {                                               \
> +               .name           = cname,                \
> +       }
> +
> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
> +       ACPM_CLK("mif"),
> +       ACPM_CLK("int"),
> +       ACPM_CLK("cpucl0"),
> +       ACPM_CLK("cpucl1"),
> +       ACPM_CLK("cpucl2"),
> +       ACPM_CLK("g3d"),
> +       ACPM_CLK("g3dl2"),
> +       ACPM_CLK("tpu"),
> +       ACPM_CLK("intcam"),
> +       ACPM_CLK("tnr"),
> +       ACPM_CLK("cam"),
> +       ACPM_CLK("mfc"),
> +       ACPM_CLK("disp"),
> +       ACPM_CLK("b0"),
> +};
> +
> +static const struct acpm_clk_driver_data acpm_clk_gs101 = {
> +       .clks = gs101_acpm_clks,
> +       .nr_clks = ARRAY_SIZE(gs101_acpm_clks),
> +       .mbox_chan_id = 0,
> +};
> +
> +static unsigned long acpm_clk_recalc_rate(struct clk_hw *hw,
> +                                         unsigned long parent_rate)
> +{
> +       struct acpm_clk *clk = to_acpm_clk(hw);
> +
> +       return clk->handle->ops.dvfs_ops.get_rate(clk->handle,
> +                                       clk->mbox_chan_id, clk->id, 0);
> +}
> +
> +static int acpm_clk_determine_rate(struct clk_hw *hw,
> +                                  struct clk_rate_request *req)
> +{
> +       /*
> +        * We can't figure out what rate it will be, so just return the
> +        * rate back to the caller. acpm_clk_recalc_rate() will be called
> +        * after the rate is set and we'll know what rate the clock is
> +        * running at then.
> +        */
> +       return 0;
> +}
> +
> +static int acpm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +                            unsigned long parent_rate)
> +{
> +       struct acpm_clk *clk = to_acpm_clk(hw);
> +
> +       return clk->handle->ops.dvfs_ops.set_rate(clk->handle,
> +                                       clk->mbox_chan_id, clk->id, rate);
> +}
> +
> +static const struct clk_ops acpm_clk_ops = {
> +       .recalc_rate = acpm_clk_recalc_rate,
> +       .determine_rate = acpm_clk_determine_rate,
> +       .set_rate = acpm_clk_set_rate,
> +};
> +
> +static int acpm_clk_ops_init(struct device *dev, struct acpm_clk *aclk,

Maybe acpm_clk_register() is a more appropriate name.

> +                            const char *name)
> +{
> +       struct clk_init_data init = {};
> +
> +       init.name = name;
> +       init.ops = &acpm_clk_ops;
> +       aclk->hw.init = &init;
> +
> +       return devm_clk_hw_register(dev, &aclk->hw);
> +}
> +
> +static int acpm_clk_probe(struct platform_device *pdev)
> +{
> +       const struct acpm_handle *acpm_handle;
> +       struct clk_hw_onecell_data *clk_data;
> +       struct clk_hw **hws;
> +       struct device *dev = &pdev->dev;
> +       struct acpm_clk *aclks;
> +       unsigned int mbox_chan_id;
> +       int i, err, count;
> +
> +       acpm_handle = devm_acpm_get_by_node(dev, dev->parent->of_node);
> +       if (IS_ERR(acpm_handle))
> +               return dev_err_probe(dev, PTR_ERR(acpm_handle),
> +                                    "Failed to get acpm handle.\n");

Remove the period please. Most error messages don't have proper
punctuation.

> +
> +       count = acpm_clk_gs101.nr_clks;
> +       mbox_chan_id = acpm_clk_gs101.mbox_chan_id;
> +
> +       clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
> +                               GFP_KERNEL);
> +       if (!clk_data)
> +               return -ENOMEM;
> +
> +       clk_data->num = count;
> +       hws = clk_data->hws;
> +
> +       aclks = devm_kcalloc(dev, count, sizeof(*aclks), GFP_KERNEL);
> +       if (!aclks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < count; i++) {
> +               struct acpm_clk *aclk = &aclks[i];
> +
> +               /*
> +                * The code assumes the clock IDs start from zero,
> +                * are sequential and do not have gaps.
> +                */
> +               aclk->id = i;
> +               aclk->handle = acpm_handle;
> +               aclk->mbox_chan_id = mbox_chan_id;
> +
> +               hws[i] = &aclk->hw;
> +
> +               err = acpm_clk_ops_init(dev, aclk,
> +                                       acpm_clk_gs101.clks[i].name);
> +               if (err)
> +                       return dev_err_probe(dev, err,
> +                                            "Failed to register clock.\n");
> +       }
> +
> +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> +                                          clk_data);
> +}
> +
> +static const struct platform_device_id acpm_clk_id[] = {
> +       { "gs101-acpm-clk" },
> +       {},

Please drop comma here so that nothing can come after.
Re: [PATCH v4 4/5] clk: samsung: add Exynos ACPM clock driver
Posted by Tudor Ambarus 4 months, 3 weeks ago
Hi, Stephen,

On 9/21/25 9:37 PM, Stephen Boyd wrote:
> Quoting Tudor Ambarus (2025-09-08 06:12:45)
>> diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
>> index 76a494e95027af26272e30876a87ac293bd56dfa..70a8b82a0136b4d0213d8ff95e029c52436e5c7f 100644
>> --- a/drivers/clk/samsung/Kconfig
>> +++ b/drivers/clk/samsung/Kconfig
>> @@ -95,6 +95,16 @@ config EXYNOS_CLKOUT
>>           status of the certains clocks from SoC, but it could also be tied to
>>           other devices as an input clock.
>>  
>> +config EXYNOS_ACPM_CLK
>> +       tristate "Clock driver controlled via ACPM interface"
>> +       depends on EXYNOS_ACPM_PROTOCOL || (COMPILE_TEST && !EXYNOS_ACPM_PROTOCOL)
> 
> Why is COMPILE_TEST limited to !EXYNOS_ACPM_PROTOCOL?


otherwise on randconfigs where COMPILE_TEST=y and EXYNOS_ACPM_PROTOCOL=n I get:
ERROR: modpost: "devm_acpm_get_by_node" [drivers/clk/samsung/clk-acpm.ko] undefined!

> 
>> +       help
>> +         This driver provides support for clocks that are controlled by
>> +         firmware that implements the ACPM interface.
>> +
>> +         This driver uses the ACPM interface to interact with the firmware
>> +         providing all the clock controlls.
>> +
>>  config TESLA_FSD_COMMON_CLK
>>         bool "Tesla FSD clock controller support" if COMPILE_TEST
>>         depends on COMMON_CLK_SAMSUNG
>> diff --git a/drivers/clk/samsung/clk-acpm.c b/drivers/clk/samsung/clk-acpm.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..8566495265ee3e06dbf370f9e424d5540f5c7457
>> --- /dev/null
>> +++ b/drivers/clk/samsung/clk-acpm.c
>> @@ -0,0 +1,184 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Samsung Exynos ACPM protocol based clock driver.
>> + *
>> + * Copyright 2025 Linaro Ltd.
>> + */
>> +
>> +#include <linux/array_size.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/firmware/samsung/exynos-acpm-protocol.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
> 
> Is this include used?

ah, it's no longer used, I'll drop it, thanks!
> 
>> +#include <linux/platform_device.h>
>> +#include <linux/types.h>
> 
> Are you avoiding kernel.h? If so, please include container_of.h and

I tend to include just what's needed, yes. kernel.h has some things that
are not yet needed in this driver.

> device/devres.h to avoid implicit includes.

Will add the two and recheck whether I rely on implicit includes somewhere else.

>> +
>> +struct acpm_clk {
>> +       u32 id;
>> +       struct clk_hw hw;
>> +       unsigned int mbox_chan_id;
>> +       const struct acpm_handle *handle;
>> +};
>> +
>> +struct acpm_clk_variant {
>> +       const char *name;
>> +};
>> +
>> +struct acpm_clk_driver_data {
>> +       const struct acpm_clk_variant *clks;
>> +       unsigned int nr_clks;
>> +       unsigned int mbox_chan_id;
>> +};
>> +
>> +#define to_acpm_clk(clk) container_of(clk, struct acpm_clk, hw)
>> +
>> +#define ACPM_CLK(cname)                                        \
>> +       {                                               \
>> +               .name           = cname,                \
>> +       }
>> +
>> +static const struct acpm_clk_variant gs101_acpm_clks[] = {
>> +       ACPM_CLK("mif"),
>> +       ACPM_CLK("int"),
>> +       ACPM_CLK("cpucl0"),
>> +       ACPM_CLK("cpucl1"),
>> +       ACPM_CLK("cpucl2"),
>> +       ACPM_CLK("g3d"),
>> +       ACPM_CLK("g3dl2"),
>> +       ACPM_CLK("tpu"),
>> +       ACPM_CLK("intcam"),
>> +       ACPM_CLK("tnr"),
>> +       ACPM_CLK("cam"),
>> +       ACPM_CLK("mfc"),
>> +       ACPM_CLK("disp"),
>> +       ACPM_CLK("b0"),
>> +};
>> +
>> +static const struct acpm_clk_driver_data acpm_clk_gs101 = {
>> +       .clks = gs101_acpm_clks,
>> +       .nr_clks = ARRAY_SIZE(gs101_acpm_clks),
>> +       .mbox_chan_id = 0,
>> +};
>> +
>> +static unsigned long acpm_clk_recalc_rate(struct clk_hw *hw,
>> +                                         unsigned long parent_rate)
>> +{
>> +       struct acpm_clk *clk = to_acpm_clk(hw);
>> +
>> +       return clk->handle->ops.dvfs_ops.get_rate(clk->handle,
>> +                                       clk->mbox_chan_id, clk->id, 0);
>> +}
>> +
>> +static int acpm_clk_determine_rate(struct clk_hw *hw,
>> +                                  struct clk_rate_request *req)
>> +{
>> +       /*
>> +        * We can't figure out what rate it will be, so just return the
>> +        * rate back to the caller. acpm_clk_recalc_rate() will be called
>> +        * after the rate is set and we'll know what rate the clock is
>> +        * running at then.
>> +        */
>> +       return 0;
>> +}
>> +
>> +static int acpm_clk_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                            unsigned long parent_rate)
>> +{
>> +       struct acpm_clk *clk = to_acpm_clk(hw);
>> +
>> +       return clk->handle->ops.dvfs_ops.set_rate(clk->handle,
>> +                                       clk->mbox_chan_id, clk->id, rate);
>> +}
>> +
>> +static const struct clk_ops acpm_clk_ops = {
>> +       .recalc_rate = acpm_clk_recalc_rate,
>> +       .determine_rate = acpm_clk_determine_rate,
>> +       .set_rate = acpm_clk_set_rate,
>> +};
>> +
>> +static int acpm_clk_ops_init(struct device *dev, struct acpm_clk *aclk,
> 
> Maybe acpm_clk_register() is a more appropriate name.

okay, will update.

> 
>> +                            const char *name)
>> +{
>> +       struct clk_init_data init = {};
>> +
>> +       init.name = name;
>> +       init.ops = &acpm_clk_ops;
>> +       aclk->hw.init = &init;
>> +
>> +       return devm_clk_hw_register(dev, &aclk->hw);
>> +}
>> +
>> +static int acpm_clk_probe(struct platform_device *pdev)
>> +{
>> +       const struct acpm_handle *acpm_handle;
>> +       struct clk_hw_onecell_data *clk_data;
>> +       struct clk_hw **hws;
>> +       struct device *dev = &pdev->dev;
>> +       struct acpm_clk *aclks;
>> +       unsigned int mbox_chan_id;
>> +       int i, err, count;
>> +
>> +       acpm_handle = devm_acpm_get_by_node(dev, dev->parent->of_node);
>> +       if (IS_ERR(acpm_handle))
>> +               return dev_err_probe(dev, PTR_ERR(acpm_handle),
>> +                                    "Failed to get acpm handle.\n");
> 
> Remove the period please. Most error messages don't have proper
> punctuation.

okay, will update.

> 
>> +
>> +       count = acpm_clk_gs101.nr_clks;
>> +       mbox_chan_id = acpm_clk_gs101.mbox_chan_id;
>> +
>> +       clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
>> +                               GFP_KERNEL);
>> +       if (!clk_data)
>> +               return -ENOMEM;
>> +
>> +       clk_data->num = count;
>> +       hws = clk_data->hws;
>> +
>> +       aclks = devm_kcalloc(dev, count, sizeof(*aclks), GFP_KERNEL);
>> +       if (!aclks)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               struct acpm_clk *aclk = &aclks[i];
>> +
>> +               /*
>> +                * The code assumes the clock IDs start from zero,
>> +                * are sequential and do not have gaps.
>> +                */
>> +               aclk->id = i;
>> +               aclk->handle = acpm_handle;
>> +               aclk->mbox_chan_id = mbox_chan_id;
>> +
>> +               hws[i] = &aclk->hw;
>> +
>> +               err = acpm_clk_ops_init(dev, aclk,
>> +                                       acpm_clk_gs101.clks[i].name);
>> +               if (err)
>> +                       return dev_err_probe(dev, err,
>> +                                            "Failed to register clock.\n");
>> +       }
>> +
>> +       return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>> +                                          clk_data);
>> +}
>> +
>> +static const struct platform_device_id acpm_clk_id[] = {
>> +       { "gs101-acpm-clk" },
>> +       {},
> 
> Please drop comma here so that nothing can come after.

okay. Thanks for the review!
Cheers,
ta