Date:	Fri, 18 Sep 2015 12:15:05 +0300
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	Rob Herring <robh@...nel.org>
Cc:	Frank Rowand <frowand.list@...il.com>,
	Matt Porter <mporter@...sulko.com>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Guenter Roeck <linux@...ck-us.net>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] of: changesets: Introduce changeset helper methods

Hi Rob,

> On Sep 17, 2015, at 17:13 , Rob Herring <robh@...nel.org> wrote:
> 
> On 09/16/2015 11:11 AM, Pantelis Antoniou wrote:
>> Changesets are very powerful, but the lack of a helper API
>> makes using them cumbersome. Introduce a simple copy based
>> API that makes things considerably easier.
>> 
>> To wit, adding a property using the raw API.
>> 
>> 	struct property *prop;
>> 	prop = kzalloc(sizeof(*prop)), GFP_KERNEL);
>> 	prop->name = kstrdup("compatible");
>> 	prop->value = kstrdup("foo,bar");
>> 	prop->length = strlen(prop->value) + 1;
>> 	of_changeset_add_property(ocs, np, prop);
>> 
>> while using the helper API
>> 
>> 	of_changeset_add_property_string(ocs, np, "compatible",
>> 			"foo,bar");
> 
> How about updating the unittest to use this.
> 

OK.

>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
>> ---
>> drivers/of/dynamic.c | 251 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of.h   |  74 +++++++++++++++
>> 2 files changed, 325 insertions(+)
>> 
>> diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
>> index e452171..afa8e31 100644
>> --- a/drivers/of/dynamic.c
>> +++ b/drivers/of/dynamic.c
>> @@ -796,3 +796,254 @@ int of_changeset_action(struct of_changeset *ocs, unsigned long action,
>> 	list_add_tail(&ce->node, &ocs->entries);
>> 	return 0;
>> }
>> +
>> +/* changeset helpers */
>> +
>> +/**
>> + * of_changeset_create_device_node - Create an empty device node
>> + *
>> + * @ocs:	changeset pointer
>> + * @parent:	parent device node
>> + * @fmt:	format string for the node's full_name
>> + * @args:	argument list for the format string
>> + *
>> + * Create an empty device node, marking it as detached and allocated.
>> + *
>> + * Returns a device node on success, an error encoded pointer otherwise
>> + */
>> +struct device_node *of_changeset_create_device_nodev(
>> +	struct of_changeset *ocs, struct device_node *parent,
>> +	const char *fmt, va_list vargs)
>> +{
>> +	struct device_node *node;
>> +
>> +	node = __of_node_dupv(NULL, fmt, vargs);
>> +	if (!node)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	node->parent = parent;
>> +	return node;
>> +}
> 
> EXPORT_SYMBOL_GPL here and on others?
> 

Err, none of the others of_changeset* methods are exported right now.
Up to now it was all internal API; should I go ahead and put EXPORT_SYMBOL_GPL
to all the others as well?

>> +
>> +/**
>> + * of_changeset_create_device_node - Create an empty device node
>> + *
>> + * @ocs:	changeset pointer
>> + * @parent:	parent device node
>> + * @fmt:	Format string for the node's full_name
>> + * ...		Arguments
>> + *
>> + * Create an empty device node, marking it as detached and allocated.
>> + *
>> + * Returns a device node on success, an error encoded pointer otherwise
>> + */
>> +struct device_node *of_changeset_create_device_node(
>> +	struct of_changeset *ocs, struct device_node *parent,
>> +	const char *fmt, ...)
>> +{
>> +	va_list vargs;
>> +	struct device_node *node;
>> +
>> +	va_start(vargs, fmt);
>> +	node = of_changeset_create_device_nodev(ocs, parent, fmt, vargs);
>> +	va_end(vargs);
>> +	return node;
>> +}
>> +
>> +/**
>> + * of_changeset_add_property_copy - Create a new property copying name & value
>> + *
>> + * @ocs:	changeset pointer
>> + * @np:		device node pointer
>> + * @name:	name of the property
>> + * @value:	pointer to the value data
>> + * @length:	length of the value in bytes
>> + *
>> + * Adds a property to the changeset by making copies of the name & value
>> + * entries.
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_add_property_copy(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name, const void *value,
>> +		int length)
>> +{
>> +	struct property *prop;
>> +	char *new_name;
>> +	void *new_value;
> 
>> +	int ret;
>> +
>> +	ret = -ENOMEM;
> 
> One line
> 

OK

>> +
>> +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
>> +	if (!prop)
>> +		goto out_no_prop;
>> +
>> +	new_name = kstrdup(name, GFP_KERNEL);
>> +	if (!new_name)
>> +		goto out_no_name;
>> +
>> +	/*
>> +	 * NOTE: There is no check for zero length value.
>> +	 * In case of a boolean property, this will allocate a value
>> +	 * of zero bytes. We do this to work around the use
>> +	 * of of_get_property() calls on boolean values.
>> +	 */
>> +	new_value = kmemdup(value, length, GFP_KERNEL);
>> +	if (!new_value)
>> +		goto out_no_value;
>> +
>> +	of_property_set_flag(prop, OF_DYNAMIC);
>> +
>> +	prop->name = new_name;
>> +	prop->value = new_value;
>> +	prop->length = length;
>> +
>> +	ret = of_changeset_add_property(ocs, np, prop);
>> +	if (ret != 0)
>> +		goto out_no_add;
>> +
>> +	return 0;
>> +
>> +out_no_add:
>> +	kfree(prop->value);
>> +out_no_value:
>> +	kfree(prop->name);
>> +out_no_name:
>> +	kfree(prop);
>> +out_no_prop:
>> +	return ret;
>> +}
>> +
>> +/**
>> + * of_changeset_add_property_string - Create a new string property
>> + *
>> + * @ocs:	changeset pointer
>> + * @np:		device node pointer
>> + * @name:	name of the property
>> + * @str:	string property
>> + *
>> + * Adds a string property to the changeset by making copies of the name
>> + * and the string value.
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_add_property_string(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name, const char *str)
>> +{
>> +	return of_changeset_add_property_copy(ocs, np, name, str,
>> +			strlen(str) + 1);
>> +}
>> +
>> +/**
>> + * of_changeset_add_property_stringf - Create a new formatted string property
>> + *
>> + * @ocs:	changeset pointer
>> + * @np:		device node pointer
>> + * @name:	name of the property
>> + * @fmt:	format of string property
>> + * ...		arguments of the format string
>> + *
>> + * Adds a string property to the changeset by making copies of the name
>> + * and the formatted value.
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_add_property_stringf(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name, const char *fmt, ...)
>> +{
>> +	va_list vargs;
>> +	char *str;
>> +	int ret;
>> +
>> +	va_start(vargs, fmt);
>> +	str = kvasprintf(GFP_KERNEL, fmt, vargs);
>> +	va_end(vargs);
>> +
>> +	ret = of_changeset_add_property_string(ocs, np, name, str);
>> +
>> +	kfree(str);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * of_changeset_add_property_string_list - Create a new string list property
>> + *
>> + * @ocs:	changeset pointer
>> + * @np:		device node pointer
>> + * @name:	name of the property
>> + * @strs:	pointer to the string list
>> + * @count:	string count
>> + *
>> + * Adds a string list property to the changeset.
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_add_property_string_list(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name, const char **strs,
>> +		int count)
>> +{
>> +	int total, length, i, ret;
>> +	char *value, *s;
>> +
>> +	total = 0;
> 
> initialize in declaration.
> 
OK

>> +	for (i = 0; i < count; i++) {
>> +		length = strlen(strs[i]);
>> +		total += length + 1;
> 
> total += strlen(strs[i]) + 1;
> 
> What if strings are NULL? Error or skip? I vote for error.
> 

Yeah.

>> +	}
>> +
>> +	value = kmalloc(total, GFP_KERNEL);
>> +	if (!value)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0, s = value; i < count; i++) {
>> +		length = strlen(strs[i]);
>> +		memcpy(s, strs[i], length + 1);
> 
> strcpy perhaps?
> 

Yes

>> +		s += length + 1;
>> +	}
>> +
>> +	ret = of_changeset_add_property_copy(ocs, np, name, value, total);
>> +
>> +	kfree(value);
>> +
>> +	return ret;
>> +}
>> +
>> +/**
>> + * of_changeset_add_property_u32 - Create a new u32 property
>> + *
>> + * @ocs:	changeset pointer
>> + * @np:		device node pointer
>> + * @name:	name of the property
>> + * @val:	value in host endian format
>> + *
>> + * Adds a u32 property to the changeset.
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_add_property_u32(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name, u32 val)
>> +{
>> +	/* in place */
>> +	val = cpu_to_be32(val);
>> +	return of_changeset_add_property_copy(ocs, np, name, &val, sizeof(val));
>> +}
>> +
>> +/**
>> + * of_changeset_add_property_bool - Create a new u32 property
>> + *
>> + * @ocs:	changeset pointer
>> + * @np:		device node pointer
>> + * @name:	name of the property
>> + *
>> + * Adds a bool property to the changeset. Note that there is
>> + * no option to set the value to false, since the property
>> + * existing sets it to true.
>> + *
>> + * Returns zero on success, a negative error value otherwise.
>> + */
>> +int of_changeset_add_property_bool(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name)
>> +{
>> +	return of_changeset_add_property_copy(ocs, np, name, "", 0);
>> +}
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index b0856ed..1e6019a 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -1037,6 +1037,27 @@ static inline int of_changeset_update_property(struct of_changeset *ocs,
>> {
>> 	return of_changeset_action(ocs, OF_RECONFIG_UPDATE_PROPERTY, np, prop);
>> }
>> +
>> +struct device_node *of_changeset_create_device_nodev(
>> +	struct of_changeset *ocs, struct device_node *parent,
>> +	const char *fmt, va_list vargs);
>> +__printf(3, 4) struct device_node *of_changeset_create_device_node(
>> +	struct of_changeset *ocs, struct device_node *parent,
>> +	const char *fmt, ...);
>> +int of_changeset_add_property_copy(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name,
>> +	const void *value, int length);
>> +int of_changeset_add_property_string(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name, const char *str);
>> +__printf(4, 5) int of_changeset_add_property_stringf(struct of_changeset *ocs,
>> +		struct device_node *np, const char *name, const char *fmt, ...);
>> +int of_changeset_add_property_string_list(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name, const char **strs, int count);
>> +int of_changeset_add_property_u32(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name, u32 val);
>> +int of_changeset_add_property_bool(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name);
>> +
>> #else /* CONFIG_OF_DYNAMIC */
>> static inline int of_reconfig_notifier_register(struct notifier_block *nb)
>> {
>> @@ -1056,6 +1077,59 @@ static inline int of_reconfig_get_state_change(unsigned long action,
>> {
>> 	return -EINVAL;
>> }
>> +
>> +static inline int of_changeset_create_device_node(struct of_changeset *ocs,
>> +	struct device_node *parent, const char *fmt, ...)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +int of_changeset_add_property_copy(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name,
>> +	const void *value, int length)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +int of_changeset_add_property_string(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name, const char *str)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static inline struct device_node *of_changeset_create_device_nodev(
>> +	struct of_changeset *ocs, struct device_node *parent,
>> +	const char *fmt, va_list vargs)
>> +{
>> +	return ERR_PTR(-EINVAL);
>> +}
>> +
>> +static inline __printf(4, 5) struct device_node *
>> +	of_changeset_add_property_stringf(
>> +		struct of_changeset *ocs, struct device_node *np,
>> +		const char *name, const char *fmt, ...)
>> +{
>> +	return ERR_PTR(-EINVAL);
>> +}
>> +
>> +static inline int of_changeset_add_property_string_list(
>> +	struct of_changeset *ocs, struct device_node *np, const char *name,
>> +	const char **strs, int count)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static inline int of_changeset_add_property_u32(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name, u32 val)
>> +{
>> +	return -EINVAL;
>> +}
>> +
>> +static inline int of_changeset_add_property_bool(struct of_changeset *ocs,
>> +	struct device_node *np, const char *name)
>> +{
>> +	return -EINVAL;
>> +}
>> #endif /* CONFIG_OF_DYNAMIC */
>> 
>> /* CONFIG_OF_RESOLVE api */

OK, updated patchset over the weekend.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists