Re: [PATCH] clk: scmi: Fix children encountered before parents case

[Cristian Marussi <cristian.marussi@arm.com>]

On Wed, Jun 04, 2025 at 01:00:30PM +0200, Sascha Hauer wrote:
> When it comes to clocks with parents the SCMI clk driver assumes that
> parents are always initialized before their children which might not
> always be the case.
> 

Hi,

> During initialization of the parent_data array we have:
> 
> 	sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
> 
> hws[sclk->info->parents[i]] will not yet be initialized when children
> are encountered before their possible parents. Solve this by allocating
> all struct scmi_clk as an array first and populating all hws[] upfront.
> 

Yes indeed, good catch.
Thanks for this.

Just one minor nitpick down below.

> Fixes: 65a8a3dd3b95f ("clk: scmi: Add support for clock {set,get}_parent")
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/clk/clk-scmi.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index 15510c2ff21c0335f5cb30677343bd4ef59c0738..f258ad7dda73e3c50c3ce567a8e22b3d2ec9836b 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -404,6 +404,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  	const struct scmi_handle *handle = sdev->handle;
>  	struct scmi_protocol_handle *ph;
>  	const struct clk_ops *scmi_clk_ops_db[SCMI_MAX_CLK_OPS] = {};
> +	struct scmi_clk *sclks;
>  
>  	if (!handle)
>  		return -ENODEV;
> @@ -430,18 +431,24 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  	transport_is_atomic = handle->is_transport_atomic(handle,
>  							  &atomic_threshold_us);
>  
> +	sclks = devm_kcalloc(dev, count, sizeof(*sclks), GFP_KERNEL);
> +	if (!sclks)
> +		return -ENOMEM;
> +
>  	for (idx = 0; idx < count; idx++) {
> -		struct scmi_clk *sclk;
> -		const struct clk_ops *scmi_ops;
> +		struct scmi_clk *sclk = &sclks[idx];

...do we really need this intermediate local variable in this initializarion loop ?
...doesnt feel like giving more readability (even though the compiler will probably
kill it anyway...)
  
> -		sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL);
> -		if (!sclk)
> -			return -ENOMEM;
> +		hws[idx] = &sclk->hw;

....cant we just

  	for (idx = 0; idx < count; idx++)
		hws[idx] = &sclks[idx].hw;


Other than this, LGTM.
Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian