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

[Alex Elder]

On 3/24/25 6:53 AM, Haylen Chu wrote:
> 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@xxxxxxxxxxxx>
> > ---
> >   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?

That's perfectly fine with me.  It's not necessary yet, so it's
just fine for you to do things the way you did, and I'll add this
in as part of the 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.

You are correct.  I'll drop it.  I contemplated this and thought
it's useful to tell the reader it's necessary to not be null, but
you can tell it has to be by inspection.


> >   	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?

I didn't understand the reason why you separated the TWSI8 into a
separate commit.  Now I know.  The hole in the range doesn't really
matter much; you already initialize your ->hws[] array of pointers
with ERR_PTR(-ENOENT), so any holes are handled properly.

					-Alex
> Best regards,
> Haylen Chu