Re: [PATCH v9 06/15] clk: ambarella: add CV75 RCT clock controller

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Thu, Oct 01, 2026 at 08:40:55PM +0800, Long Zhao via B4 Relay wrote:

> Add a table-driven CCF driver for the CV75 root clock tree. Register
> the core PLL, AHB/APB fixed factors and UART0 composite clock, with
> osc supplied via clk_parent_data.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk-provider.h>
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/io.h>

> +#include <linux/math.h>
> +#include <linux/math64.h>

The latter one covers the former one.

> +#include <linux/module.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>

...

> +struct cv75_pll_hw {
> +	struct clk_hw hw;

> +	u32 ctrl;
> +	u32 frac;
> +	u32 ctrl2;

This repeats twice in different data structures. Is the semantic the same in
both cases? If so, it might make sense to have a separate data structure for
these three and embed it as required.

> +};

...

> +struct cv75_pll {
> +	const char *name;
> +	struct cv75_pll_hw *hw;
> +	unsigned long flags;

> +	u32 ctrl;
> +	u32 frac;
> +	u32 ctrl2;

^^^ See above.

> +	int id;
> +};

...

> +{
> +	struct clk_parent_data parent_data = { .fw_name = "osc" };
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cv75_plls); i++) {

	for (unsigned int i = 0; i < ARRAY_SIZE(cv75_plls); i++) {

Same approach in the below code where similar cases appear.

> +		const struct cv75_pll *desc = &cv75_plls[i];
> +		struct cv75_pll_hw *pll = desc->hw;
> +		struct clk_init_data init = {};
> +		int ret;
> +
> +		pll->ctrl = desc->ctrl;
> +		pll->frac = desc->frac;
> +		pll->ctrl2 = desc->ctrl2;
> +
> +		init.name = desc->name;
> +		init.ops = &cv75_pll_ops;
> +		init.parent_data = &parent_data;
> +		init.num_parents = 1;
> +		init.flags = desc->flags;
> +		pll->hw.init = &init;
> +
> +		ret = devm_clk_hw_register(dev, &pll->hw);
> +		if (ret)
> +			return ret;
> +
> +		if (desc->id >= 0)
> +			data->hws[desc->id] = &pll->hw;
> +	}
> +
> +	return 0;
> +}

...

> +static int cv75_rct_probe(struct platform_device *pdev)
> +{
> +	struct clk_hw_onecell_data *data;
> +	struct device *dev = &pdev->dev;
> +	int ret;

> +	spin_lock_init(&cv75_ccu.lock);

Can it be done later, after memory allocation?

> +	cv75_ccu.base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(cv75_ccu.base))
> +		return PTR_ERR(cv75_ccu.base);
> +
> +	data = devm_kzalloc(dev, struct_size(data, hws, CV75_NUM_CLKS), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +	data->num = CV75_NUM_CLKS;
> +
> +	ret = cv75_register_plls(dev, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register plls\n");
> +
> +	ret = cv75_register_fixed_factors(dev, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register fixed factors\n");
> +
> +	ret = cv75_register_composites(dev, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register composites\n");
> +
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add clock provider\n");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko