Date:   Tue, 16 Oct 2018 15:57:43 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [RFC PATCH 3/5] drivers: base: Introducing software nodes to the
 firmware node framework

On Fri, Oct 12, 2018 at 02:39:32PM +0300, Heikki Krogerus wrote:
> Software node is a new struct fwnode_handle type that can be
> used to describe devices in kernel (software). It is meant
> to complement fwnodes representing real firmware nodes when
> they are incomplete (for example missing device properties)
> and to supply the primary fwnode when the firmware lacks
> hardware description for a device completely.
> 
> The software node type is really meant to replace the
> currently used "property_set" struct fwnode_handle type. The
> handling of struct property_set is glued to the generic
> device property handling code, and it is not possible to
> create a struct property_set independently from a device
> that it is bind to. struct property_set is only created when
> device properties are added to already initialized struct
> device, and control of it is only possible from the generic
> property handling code.
> 
> Software nodes are instead designed to be created
> independently from the device entries (struct device). It
> makes them much more flexible, as then the device meant to
> be bind to the node can be created at a later time, and from
> another location. It is also possible to bind multiple
> devices to a single software node if needed.
> 
> The software node implementation also includes support for
> node hierarchy, which was the main motivation for this
> commit. The node hierarchy was something that was requested
> for the struct property_set, but it did not seem reasonable
> to try to extend the property_set support for that purpose.
> struct property_set was really meant only for device
> property handling like the name suggests.
> 
> Support for struct property_set is not yet removed in this
> commit, but it will be in the following one.

> +static int property_entry_read_string_array(const struct property_entry *props,
> +					    const char *propname,
> +					    const char **strings, size_t nval)
> +{
> +	const struct property_entry *prop;
> +	const void *pointer;
> +	size_t array_len, length;
> +
> +	/* Find out the array length. */
> +	prop = property_entry_get(props, propname);
> +	if (!prop)
> +		return -EINVAL;
> +

> +	if (!prop->is_array)
> +		/* The array length for a non-array string property is 1. */
> +		array_len = 1;
> +	else
> +		/* Find the length of an array. */
> +		array_len = property_entry_count_elems_of_size(props, propname,
> +							  sizeof(const char *));

I understand where it comes from, but here we may use positive condition.

> +
> +	/* Return how many there are if strings is NULL. */
> +	if (!strings)
> +		return array_len;
> +
> +	array_len = min(nval, array_len);
> +	length = array_len * sizeof(*strings);
> +
> +	pointer = property_entry_find(props, propname, length);
> +	if (IS_ERR(pointer))
> +		return PTR_ERR(pointer);
> +
> +	memcpy(strings, pointer, length);
> +
> +	return array_len;
> +}

> +struct fwnode_handle *
> +software_node_get_next_child(const struct fwnode_handle *fwnode,
> +			     struct fwnode_handle *child)
> +{
> +	struct software_node *p = to_software_node(fwnode);
> +	struct software_node *c = to_software_node(child);
> +
> +	if (list_empty(&p->children) ||
> +	    (c && list_is_last(&c->entry, &p->children)))
> +		return NULL;
> +
> +	if (c)
> +		c = list_next_entry(c, entry);
> +	else
> +		c = list_first_entry(&p->children, struct software_node, entry);
> +	return &c->fwnode;
> +}

> +static ssize_t software_node_property_show(struct kobject *kobj,
> +					   struct kobj_attribute *attr,
> +					   char *buf)
> +{
> +	struct software_node *swnode = kobj_to_swnode(kobj);
> +	const struct property_entry *prop;
> +
> +	for (prop = swnode->properties; prop->name; prop++)
> +		if (prop->name == attr->attr.name)
> +			break;
> +
> +	if (prop->is_array)
> +		return property_array_show(prop, buf);
> +
> +	/* boolean property */
> +	if (!prop->length)
> +		return sprintf(buf, "1\n");
> +
> +	switch (prop->type) {

> +	case DEV_PROP_U8:
> +		return sprintf(buf, "%u\n", prop->value.u8_data);

I would expect same base for all numbers.

> +	case DEV_PROP_U16:
> +		return sprintf(buf, "0x%x\n", prop->value.u16_data);
> +	case DEV_PROP_U32:
> +		return sprintf(buf, "0x%x\n", prop->value.u32_data);
> +	case DEV_PROP_U64:
> +		return sprintf(buf, "0x%llx\n", prop->value.u64_data);
> +	case DEV_PROP_STRING:
> +		return sprintf(buf, "%s\n", prop->value.str);
> +	default:
> +		break;
> +	}

I just realize that we might need to export type of the node as well.
How can we distinguish string "251" from a number?

> +
> +	return -EINVAL;
> +}

> +}

> +#define NODE_NAME_MAXSIZE	11

sizeof(int) = 4 (32 bits), so, 32 * 3 / 10 ~= 10. On top are "node" and '\0'.
Thus, I would rather put 16 here. Or limit the maximum for ida_simple_get().

> +struct fwnode_handle *
> +fwnode_create_software_node(const struct property_entry *properties,
> +			    const struct fwnode_handle *parent)
> +{
> +	char node_name[NODE_NAME_MAXSIZE];
> +	struct software_node *p = NULL;
> +	struct software_node *swnode;
> +	int ret;
> +
> +	if (parent) {
> +		if (IS_ERR(parent))
> +			return ERR_CAST(parent);
> +		if (!is_software_node(parent))
> +			return ERR_PTR(-EINVAL);
> +		p = to_software_node(parent);
> +	}
> +
> +	swnode = kzalloc(sizeof(*swnode), GFP_KERNEL);
> +	if (!swnode)
> +		return ERR_PTR(-ENOMEM);
> +

> +	swnode->id = ida_simple_get(p ? &p->child_ids : &swnode_root_ids, 0, 0,
> +				    GFP_KERNEL);

> +	if (swnode->id < 0) {
> +		kfree(swnode);
> +		return ERR_PTR(swnode->id);
> +	}
> +
> +	sprintf(node_name, "node%d", swnode->id);
> +
> +	swnode->kobj.kset = swnode_kset;
> +	swnode->fwnode.ops = &software_node_ops;
> +
> +	ida_init(&swnode->child_ids);
> +	INIT_LIST_HEAD(&swnode->entry);
> +	INIT_LIST_HEAD(&swnode->children);
> +	swnode->parent = p;
> +
> +	if (p)
> +		list_add_tail(&swnode->entry, &p->children);
> +
> +	ret = kobject_init_and_add(&swnode->kobj, &software_node_type,
> +				   p ? &p->kobj : NULL, node_name);
> +	if (ret) {
> +		kobject_put(&swnode->kobj);
> +		return ERR_PTR(ret);
> +	}
> +
> +	ret = software_node_register_properties(swnode, properties);
> +	if (ret) {
> +		kobject_put(&swnode->kobj);
> +		return ERR_PTR(ret);
> +	}
> +
> +	kobject_uevent(&swnode->kobj, KOBJ_ADD);
> +	return &swnode->fwnode;
> +}


-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists