Re: [PATCH v3 06/10] clk: support init->parent_data

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jun 23, 2025 at 03:57:51PM +0200, Steffen Trumtrar wrote:
> Linux clk driver supports matching parents via init->parent_data instead
> of init->parent_names.
> 
> Add support for this feature to barebox, too.
> Linux however supports setting a "name" and "fw_name" for a parent
> clock. "fw_name" is a local name for the provider registering the clock
> and "name" is a globally unique parent name. For the time being, only
> support the globally unique "name" field.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/clk/clk.c | 48 ++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 9b21d0650b6c6b510d909b15324eecce8540aaef..6b57fbffb3726d3a4147a88edf7648306e1f8b56 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -483,11 +483,28 @@ int bclk_register(struct clk *clk)
>  	return ret;
>  }
>  
> +static int clk_cpy_name(const char **dst_p, const char *src, bool must_exist)
> +{
> +	const char *dst;
> +
> +	if (!src) {
> +		if (must_exist)
> +			return -EINVAL;
> +		return 0;
> +	}
> +
> +	*dst_p = dst = xstrdup(src);
> +	if (!dst)
> +		return -ENOMEM;

xstrdup never fails.

> +
> +	return 0;
> +}
> +
>  struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  {
>  	struct clk *clk;
>  	const struct clk_init_data *init = hw->init;
> -	char **parent_names = NULL;
> +	const char **parent_names = NULL;
>  	int i, ret;
>  
>  	if (!hw->init)
> @@ -503,14 +520,29 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  
>  	clk->parents = xzalloc(sizeof(struct clk *) * clk->num_parents);
>  
> -	if (init->parent_names) {
> +	if (init->parent_names || init->parent_data) {
>  		parent_names = xzalloc(init->num_parents * sizeof(char *));
>  
> -		for (i = 0; i < init->num_parents; i++)
> -			parent_names[i] = xstrdup(init->parent_names[i]);
> -
> -		clk->parent_names = (const char *const*)parent_names;
> -
> +		for (i = 0; i < init->num_parents; i++) {
> +			if (init->parent_names) {
> +				ret = clk_cpy_name(&parent_names[i],
> +						   init->parent_names[i], true);

ret is never checked.

> +			} else if (init->parent_data) {
> +				/* Linux copies fw_name and if successful also name.
> +				 * As fw_name is not handled in barebox, just copy the
> +				 * name field and fallback to hw->clk.name if it doesn't
> +				 * exist.
> +				 */
> +				ret = clk_cpy_name(&parent_names[i],
> +						   init->parent_data[i].name,
> +						   true);
> +				if (ret)
> +					ret = clk_cpy_name(&parent_names[i],
> +							   init->parent_data[i].hw->clk.name,
> +							   false);

also here.

> +			}
> +		}
> +		clk->parent_names = (const char *const *)parent_names;
>  	} else {
>  		for (i = 0; i < init->num_parents; i++)
>  			clk->parents[i] = clk_hw_to_clk(init->parent_hws[i]);
> @@ -522,7 +554,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
>  	if (ret) {
>  		if (parent_names) {
>  			for (i = 0; i < init->num_parents; i++)
> -				free(parent_names[i]);
> +				free(&parent_names[i]);

This change looks wrong. You want to free the string, not the address of
the pointer to the string.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |