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
