Re: [PATCH RESEND 2/7] clk: spacemit: define struct k1_ccu_data

Haylen Chu
2025-03-24T11:53:40Z

On Fri, Mar 21, 2025 at 10:18:25AM -0500, Alex Elder wrote:
> Define a new structure type to be used for describing the OF match data.
> Rather than using the array of spacemit_ccu_clk structures for match
> data, we use this structure instead.
>
> Move the definition of the spacemit_ccu_clk structure closer to the top
> of the source file, and add the new structure definition below it.
>
> Shorten the name of spacemit_ccu_register() to be k1_ccu_register().

I've read your conversation about moving parts of the patch into the
clock series, I'm of course willing to :)

> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++---------
> 1 file changed, 43 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index 44db48ae71313..f7367271396a0 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
> @@ -129,6 +129,15 @@
> #define APMU_EMAC0_CLK_RES_CTRL 0x3e4
> #define APMU_EMAC1_CLK_RES_CTRL 0x3ec
>
> +struct spacemit_ccu_clk {
> + int id;
> + struct clk_hw *hw;
> +};
> +
> +struct k1_ccu_data {
> + struct spacemit_ccu_clk *clk; /* array with sentinel */
> +};

This is something like what I've dropped in v5 of the clock series so I
doubt whether it should be added back in clock series again, as at that
point there's no reason for an extra structure: Alex, is it okay for you
to keep the change in reset series?

...

> +static int k1_ccu_register(struct device *dev, struct regmap *regmap,
> + struct regmap *lock_regmap,
> + struct spacemit_ccu_clk *clks)
> {
> const struct spacemit_ccu_clk *clk;
> int i, ret, max_id = 0;
> @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev,
>
> clk_data->num = max_id + 1;
>
> - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> + if (ret)
> + dev_err(dev, "error %d adding clock hardware provider\n", ret);

This error message definitely should go in the clock series.

> + return ret;
> }

> static int k1_ccu_probe(struct platform_device *pdev)
> {
> struct regmap *base_regmap, *lock_regmap = NULL;
> struct device *dev = &pdev->dev;
> + const struct k1_ccu_data *data;
> int ret;
>
> + data = of_device_get_match_data(dev);
> + if (!data)
> + return -EINVAL;

Looking through the reset series, I don't see a reason that
of_device_get_match_data() could return NULL. This is also something
you've asked me to drop in v4 of the clock series, so I guess it isn't
necessary.

> base_regmap = device_node_to_regmap(dev->of_node);
> if (IS_ERR(base_regmap))
> return dev_err_probe(dev, PTR_ERR(base_regmap),
> @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev)
> "failed to get lock regmap\n");
> }
>
> - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
> - of_device_get_match_data(dev));
> + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk);
> if (ret)
> return dev_err_probe(dev, ret, "failed to register clocks\n");

For using ARRAY_SIZE() to simplify runtime code, it's mostly okay since
binding IDs are continuous 0-based integers. But I split the handling of
TWSI8 into another patch, which creates a hole in the range and breaks
the assumption. Do you think the TWSI8 commit should be merged back in
the clock driver one?

Best regards,
Haylen Chu