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

Andy Shevchenko
2023-09-11T22:47:11Z

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