Re: [PATCH V13 1/5] of: dynamic: Add interfaces for creating device node dynamically

[Andy Shevchenko]

On Tue, Aug 15, 2023 at 10:19:56AM -0700, Lizhi Hou wrote:
> of_changeset_create_node() creates device node dynamically and attaches
> the newly created node to a changeset.
> 
> Expand of_changeset APIs to handle specific types of properties.
>     of_changeset_add_prop_string()
>     of_changeset_add_prop_string_array()
>     of_changeset_add_prop_u32_array()

...

> +/**
> + * of_changeset_add_prop_string - Add a string property to a changeset
> + *
> + * @ocs:	changeset pointer
> + * @np:		device node pointer
> + * @prop_name:	name of the property to be added
> + * @str:	pointer to null terminated string
> + *
> + * Create a string property and add it to a changeset.
> + *
> + * Return: 0 on success, a negative error value in case of an error.
> + */
> +int of_changeset_add_prop_string(struct of_changeset *ocs,
> +				 struct device_node *np,
> +				 const char *prop_name, const char *str)
> +{
> +	struct property prop;
> +
> +	prop.name = (char *)prop_name;

This looks not nice. You dropped const qualifier, which is a bit worrying.
Can  you fix underneath APIs/data types so we can avoid this?

> +	prop.length = strlen(str) + 1;

> +	prop.value = (void *)str;

Do you need this casting?

Okay, seems also related to the const qualifier. I would accept this in a form
of const void *, but it will probably look a bit weird. What about to have a value
to be a union?

> +	return of_changeset_add_prop_helper(ocs, np, &prop);
> +}

...

> +	prop.name = (char *)prop_name;

Same comment as per above.

...

> +	prop.length = 0;
> +	for (i = 0; i < sz; i++)
> +		prop.length += strlen(str_array[i]) + 1;
> +
> +	prop.value = kmalloc(prop.length, GFP_KERNEL);
> +	if (!prop.value)
> +		return -ENOMEM;
> +
> +	vp = prop.value;
> +	for (i = 0; i < sz; i++) {
> +		vp += snprintf(vp, (char *)prop.value + prop.length - vp, "%s",
> +			       str_array[i]) + 1;
> +	}

Is all this kinda of reinventing kasprintf()? Perhaps you can somehow utilize
kasprintf_strarray()? It might require to get a common denominator that takes
a formatting string as a parameter.

> +	ret = of_changeset_add_prop_helper(ocs, np, &prop);
> +	kfree(prop.value);

...

> +	for (i = 0; i < sz; i++)
> +		val[i] = cpu_to_be32(array[i]);

NIH cpu_to_be32_array()

...

> +	prop.name = (char *)prop_name;
> +	prop.length = sizeof(u32) * sz;
> +	prop.value = (void *)val;

Do you need this casting?

-- 
With Best Regards,
Andy Shevchenko