Messages in this thread | | | Date | Mon, 11 Sep 2023 23:48:13 +0300 | From | Andy Shevchenko <> | Subject | Re: [PATCH V13 1/5] of: dynamic: Add interfaces for creating device node dynamically |
| |
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
| |