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
