| Message ID | 20180917181603.125492-3-dmitry.torokhov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support children for legacy device properties | expand |
Hi, On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote: > Several drivers rely on having notion of sub-nodes when describing > hardware, let's allow static board-defined properties also have it. > > The board files will then attach properties to devices in the following > fashion: > > device_add_properties(&board_platform_device.dev, > main_device_props); > device_add_child_properties(&board_platform_device.dev, > dev_fwnode(&board_platform_device.dev), > child1_device_props); > device_add_child_properties(&board_platform_device.dev, > dev_fwnode(&board_platform_device.dev), > child2_device_props); > ... > platform_device_register(&board_platform_device.dev); > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++---- > include/linux/property.h | 4 ++ > 2 files changed, 102 insertions(+), 12 deletions(-) > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c > index 08ecc13080ae..63f2377aefe8 100644 > --- a/drivers/base/pset_property.c > +++ b/drivers/base/pset_property.c > @@ -18,6 +18,11 @@ struct property_set { > struct device *dev; > struct fwnode_handle fwnode; > const struct property_entry *properties; > + > + struct property_set *parent; > + /* Entry in parent->children list */ > + struct list_head child_node; > + struct list_head children; Add const char *name; and you can implement also pset_get_named_child_node(). > }; > > static const struct fwnode_operations pset_fwnode_ops; > @@ -283,10 +288,47 @@ pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode, > val, nval); > } > > +struct fwnode_handle *pset_fwnode_get_parent(const struct fwnode_handle *fwnode) > +{ > + struct property_set *pset = to_pset_node(fwnode); > + > + return pset ? &pset->parent->fwnode : NULL; > +} > + > +struct fwnode_handle * > +pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode, > + struct fwnode_handle *child) > +{ > + const struct property_set *pset = to_pset_node(fwnode); > + struct property_set *first_child; > + struct property_set *next; > + > + if (!pset) > + return NULL; > + > + if (list_empty(&pset->children)) > + return NULL; > + > + first_child = list_first_entry(&pset->children, struct property_set, > + child_node); > + > + if (child) { > + next = list_next_entry(to_pset_node(child), child_node); > + if (next == first_child) > + return NULL; > + } else { > + next = first_child; > + } > + > + return &next->fwnode; > +} > + > static const struct fwnode_operations pset_fwnode_ops = { > .property_present = pset_fwnode_property_present, > .property_read_int_array = pset_fwnode_read_int_array, > .property_read_string_array = pset_fwnode_property_read_string_array, > + .get_parent = pset_fwnode_get_parent, > + .get_next_child_node = pset_fwnode_get_next_subnode, > }; > > static void property_entry_free_data(const struct property_entry *p) > @@ -439,24 +481,31 @@ EXPORT_SYMBOL_GPL(property_entries_free); > */ > static void pset_free_set(struct property_set *pset) > { > + struct property_set *child, *next; > + > if (!pset) > return; > > + list_for_each_entry_safe(child, next, &pset->children, child_node) { > + list_del(&child->child_node); > + pset_free_set(child); > + } > + > property_entries_free(pset->properties); > kfree(pset); > } > > /** > - * pset_copy_set - copies property set > - * @pset: Property set to copy > + * pset_create_set - creates property set. > + * @src: property entries for the set. > * > - * This function takes a deep copy of the given property set and returns > - * pointer to the copy. Call device_free_property_set() to free resources > - * allocated in this function. > + * This function takes a deep copy of the given property entries and creates > + * property set. Call pset_free_set() to free resources allocated in this > + * function. > * > * Return: Pointer to the new property set or error pointer. > */ > -static struct property_set *pset_copy_set(const struct property_set *pset) > +static struct property_set *pset_create_set(const struct property_entry *src) > { > struct property_entry *properties; > struct property_set *p; > @@ -465,7 +514,11 @@ static struct property_set *pset_copy_set(const struct property_set *pset) > if (!p) > return ERR_PTR(-ENOMEM); > > - properties = property_entries_dup(pset->properties); > + INIT_LIST_HEAD(&p->child_node); > + INIT_LIST_HEAD(&p->children); > + p->fwnode.ops = &pset_fwnode_ops; > + > + properties = property_entries_dup(src); > if (IS_ERR(properties)) { > kfree(p); > return ERR_CAST(properties); > @@ -521,20 +574,53 @@ EXPORT_SYMBOL_GPL(device_remove_properties); > int device_add_properties(struct device *dev, > const struct property_entry *properties) > { > - struct property_set *p, pset; > + struct property_set *p; > > if (!properties) > return -EINVAL; > > - pset.properties = properties; > - > - p = pset_copy_set(&pset); > + p = pset_create_set(properties); > if (IS_ERR(p)) > return PTR_ERR(p); > > - p->fwnode.ops = &pset_fwnode_ops; > set_secondary_fwnode(dev, &p->fwnode); > p->dev = dev; > return 0; > } > EXPORT_SYMBOL_GPL(device_add_properties); > + > +/** > + * device_add_child_properties - Add a collection of properties to a device object. > + * @dev: Device to add properties to. @parent: missing > + * @properties: Collection of properties to add. > + * > + * Associate a collection of device properties represented by @properties as a > + * child of given @parent firmware node. The function takes a copy of > + * @properties. > + */ > +struct fwnode_handle * > +device_add_child_properties(struct device *dev, device_add_child_properties(struct device *dev, const char *child_name, > + struct fwnode_handle *parent, > + const struct property_entry *properties) > +{ > + struct property_set *p; > + struct property_set *parent_pset; > + > + if (!properties) > + return ERR_PTR(-EINVAL); > + > + parent_pset = to_pset_node(parent); > + if (!parent_pset) > + return ERR_PTR(-EINVAL); > + > + p = pset_create_set(properties); > + if (IS_ERR(p)) > + return ERR_CAST(p); > + > + p->dev = dev; p->name = kstrdup_const(child_name, GFP_KERNEL); You'll need to kfree_const(pset->name) in pset_free_set() of course. > + p->parent = parent_pset; > + list_add_tail(&p->child_node, &parent_pset->children); > + > + return &p->fwnode; > +} > +EXPORT_SYMBOL_GPL(device_add_child_properties); Br,
Hi Heikki, On Wed, Sep 19, 2018 at 06:10:26PM +0300, Heikki Krogerus wrote: > Hi, > > On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote: > > Several drivers rely on having notion of sub-nodes when describing > > hardware, let's allow static board-defined properties also have it. > > > > The board files will then attach properties to devices in the following > > fashion: > > > > device_add_properties(&board_platform_device.dev, > > main_device_props); > > device_add_child_properties(&board_platform_device.dev, > > dev_fwnode(&board_platform_device.dev), > > child1_device_props); > > device_add_child_properties(&board_platform_device.dev, > > dev_fwnode(&board_platform_device.dev), > > child2_device_props); > > ... > > platform_device_register(&board_platform_device.dev); > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++---- > > include/linux/property.h | 4 ++ > > 2 files changed, 102 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c > > index 08ecc13080ae..63f2377aefe8 100644 > > --- a/drivers/base/pset_property.c > > +++ b/drivers/base/pset_property.c > > @@ -18,6 +18,11 @@ struct property_set { > > struct device *dev; > > struct fwnode_handle fwnode; > > const struct property_entry *properties; > > + > > + struct property_set *parent; > > + /* Entry in parent->children list */ > > + struct list_head child_node; > > + struct list_head children; > > Add > > const char *name; > > and you can implement also pset_get_named_child_node(). Or char name[]; to avoid separate allocation. Alternatively, we can add it later when we need it, and add device_add_named_child_properties(). I'll leave it up to Rafael to decide. Thanks.
On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote: > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c > > > index 08ecc13080ae..63f2377aefe8 100644 > > > --- a/drivers/base/pset_property.c > > > +++ b/drivers/base/pset_property.c > > > @@ -18,6 +18,11 @@ struct property_set { > > > struct device *dev; > > > struct fwnode_handle fwnode; > > > const struct property_entry *properties; > > > + > > > + struct property_set *parent; > > > + /* Entry in parent->children list */ > > > + struct list_head child_node; > > > + struct list_head children; > > > > Add > > > > const char *name; > > > > and you can implement also pset_get_named_child_node(). > > Or > char name[]; > > to avoid separate allocation. Let's not do that, especially if you are planning on exporting this structure. If the name is coming from .rodata, there is no need to allocate anything for the name. Check kstrdup_const(). > Alternatively, we can add it later when we need it, and add > device_add_named_child_properties(). > > I'll leave it up to Rafael to decide. Fair enough. Thanks,
Hi Dmitry, On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote: > +/** > + * device_add_child_properties - Add a collection of properties to a device object. > + * @dev: Device to add properties to. In case you didn't notice my comment for this, you are missing @parent here. But why do you need both the parent and the dev? > + * @properties: Collection of properties to add. > + * > + * Associate a collection of device properties represented by @properties as a > + * child of given @parent firmware node. The function takes a copy of > + * @properties. > + */ > +struct fwnode_handle * > +device_add_child_properties(struct device *dev, > + struct fwnode_handle *parent, > + const struct property_entry *properties) > +{ > + struct property_set *p; > + struct property_set *parent_pset; > + > + if (!properties) > + return ERR_PTR(-EINVAL); > + > + parent_pset = to_pset_node(parent); For this function, the parent will in practice have to be dev_fwnode(dev), so I don't think you need @parent at all, no? There is something wrong here.. > + if (!parent_pset) > + return ERR_PTR(-EINVAL); > + > + p = pset_create_set(properties); > + if (IS_ERR(p)) > + return ERR_CAST(p); > + > + p->dev = dev; That looks wrong. I'm guessing the assumption here is that the child nodes will never be assigned to their own devices, but you can't do that. It will limit the use of the child nodes to a very small number of cases, possibly only to gpios. I think that has to be fixed. It should not be a big deal. Just expect the child nodes to be removed separately, and add ref counting to the struct property_set handling. > + p->parent = parent_pset; > + list_add_tail(&p->child_node, &parent_pset->children); > + > + return &p->fwnode; > +} > +EXPORT_SYMBOL_GPL(device_add_child_properties); The child nodes will change the purpose of the build-in property support. Originally the goal was just to support adding of build-in device properties to real firmware nodes, but things have changed quite a bit from that. These child nodes are purely tied to the build-in device property support, so we should be talking about adding pset type child nodes to pset type parent nodes in the API: fwnode_pset_add_child_node(), or something like that. I'm sorry to be a bit of pain in the butt with this. The build-in property support is a hack, it always was. A useful hack, but hack nevertheless. That means we need to be extra careful when expanding it, like here. Thanks,
On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > The child nodes will change the purpose of the build-in property > support. Originally the goal was just to support adding of build-in > device properties to real firmware nodes, but things have changed > quite a bit from that. These child nodes are purely tied to the > build-in device property support, so we should be talking about adding > pset type child nodes to pset type parent nodes in the API: > fwnode_pset_add_child_node(), or something like that. > > I'm sorry to be a bit of pain in the butt with this. The build-in > property support is a hack, it always was. A useful hack, but hack > nevertheless. That means we need to be extra careful when expanding > it, like here. I dunno how we got to here, what I tried to solve and Dmitry tries to make more general is converting old boardfiles to use fwnode, because I initially tried to just support gpio descriptors from board files. If boardfiles is what you mean with "build-in property support" I don't know, it predates both device tree and ACPI and any other hardware descriptions on the ARM architecture, from the perspective of these systems things are fine and the hardware description languages are the novelty... But I guess you know because you worked with OMAP :) So there is something I don't understand here. Yours, Linus Walleij
Hi Heikki, On Thu, Sep 20, 2018 at 04:53:48PM +0300, Heikki Krogerus wrote: > Hi Dmitry, > > On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote: > > +/** > > + * device_add_child_properties - Add a collection of properties to a device object. > > + * @dev: Device to add properties to. > > In case you didn't notice my comment for this, you are missing @parent > here. > > But why do you need both the parent and the dev? I could go by parent only and fetch dev from parent. > > > + * @properties: Collection of properties to add. > > + * > > + * Associate a collection of device properties represented by @properties as a > > + * child of given @parent firmware node. The function takes a copy of > > + * @properties. > > + */ > > +struct fwnode_handle * > > +device_add_child_properties(struct device *dev, > > + struct fwnode_handle *parent, > > + const struct property_entry *properties) > > +{ > > + struct property_set *p; > > + struct property_set *parent_pset; > > + > > + if (!properties) > > + return ERR_PTR(-EINVAL); > > + > > + parent_pset = to_pset_node(parent); > > For this function, the parent will in practice have to be > dev_fwnode(dev), so I don't think you need @parent at all, no? > > There is something wrong here.. Yes, I expect majority of the calls will use dev_fwnode(dev) as parent, but nobody stops you from doing: device_add_properties(dev, props); c1 = device_add_child_properties(dev, dev_fwnode(dev), cp1); c2 = device_add_child_properties(dev, c1, cp2); c3 = device_add_child_properties(dev, c2, cp3); ... > > > + if (!parent_pset) > > + return ERR_PTR(-EINVAL); > > + > > + p = pset_create_set(properties); > > + if (IS_ERR(p)) > > + return ERR_CAST(p); > > + > > + p->dev = dev; > > That looks wrong. > > I'm guessing the assumption here is that the child nodes will never be > assigned to their own devices, but you can't do that. It will limit > the use of the child nodes to a very small number of cases, possibly > only to gpios. If I need to assign a node to a device I'll use device_add_properties() API. device_add_child_properties() is for nodes living "below" the device. All nodes (the primary/secondary and children) would point to the owning device, just for convenience. > > I think that has to be fixed. It should not be a big deal. Just expect > the child nodes to be removed separately, and add ref counting to the > struct property_set handling. Why do we need to remove them separately and what do we need refcounting for? > > > + p->parent = parent_pset; > > + list_add_tail(&p->child_node, &parent_pset->children); > > + > > + return &p->fwnode; > > +} > > +EXPORT_SYMBOL_GPL(device_add_child_properties); > > The child nodes will change the purpose of the build-in property > support. Originally the goal was just to support adding of build-in > device properties to real firmware nodes, but things have changed > quite a bit from that. These child nodes are purely tied to the > build-in device property support, so we should be talking about adding > pset type child nodes to pset type parent nodes in the API: > fwnode_pset_add_child_node(), or something like that. OK, I can change device_add_child_properties() to fwnode_pset_add_child_node() if Rafael would prefer this name. Thanks.
On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote: > On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote: > > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c > > > > index 08ecc13080ae..63f2377aefe8 100644 > > > > --- a/drivers/base/pset_property.c > > > > +++ b/drivers/base/pset_property.c > > > > @@ -18,6 +18,11 @@ struct property_set { > > > > struct device *dev; > > > > struct fwnode_handle fwnode; > > > > const struct property_entry *properties; > > > > + > > > > + struct property_set *parent; > > > > + /* Entry in parent->children list */ > > > > + struct list_head child_node; > > > > + struct list_head children; > > > > > > Add > > > > > > const char *name; > > > > > > and you can implement also pset_get_named_child_node(). > > > > Or > > char name[]; > > > > to avoid separate allocation. > > Let's not do that, especially if you are planning on exporting this > structure. Can you please elaborate why? Not using pointer saves us 4/8 bytes + however much memory we need for bookkeeping for the extra chunk. Given that majority of pset nodes are unnamed this seems wasteful. > If the name is coming from .rodata, there is no need to > allocate anything for the name. Check kstrdup_const(). The data is most likely coming as __initconst so we do need to copy it. > > > Alternatively, we can add it later when we need it, and add > > device_add_named_child_properties(). > > > > I'll leave it up to Rafael to decide. > > Fair enough. > > > Thanks, > > -- > heikki Thanks.
On Fri, Sep 21, 2018 at 04:33:36PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote: > > On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote: > > > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c > > > > > index 08ecc13080ae..63f2377aefe8 100644 > > > > > --- a/drivers/base/pset_property.c > > > > > +++ b/drivers/base/pset_property.c > > > > > @@ -18,6 +18,11 @@ struct property_set { > > > > > struct device *dev; > > > > > struct fwnode_handle fwnode; > > > > > const struct property_entry *properties; > > > > > + > > > > > + struct property_set *parent; > > > > > + /* Entry in parent->children list */ > > > > > + struct list_head child_node; > > > > > + struct list_head children; > > > > > > > > Add > > > > > > > > const char *name; > > > > > > > > and you can implement also pset_get_named_child_node(). > > > > > > Or > > > char name[]; > > > > > > to avoid separate allocation. > > > > Let's not do that, especially if you are planning on exporting this > > structure. > > Can you please elaborate why? Not using pointer saves us 4/8 bytes + > however much memory we need for bookkeeping for the extra chunk. Given > that majority of pset nodes are unnamed this seems wasteful. > > > If the name is coming from .rodata, there is no need to > > allocate anything for the name. Check kstrdup_const(). > > The data is most likely coming as __initconst so we do need to copy it. OK, I did not consider that. Yes, it makes sense to always copy. Thanks,
Hi Linus, On Fri, Sep 21, 2018 at 08:36:53AM -0700, Linus Walleij wrote: > On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > The child nodes will change the purpose of the build-in property > > support. Originally the goal was just to support adding of build-in > > device properties to real firmware nodes, but things have changed > > quite a bit from that. These child nodes are purely tied to the > > build-in device property support, so we should be talking about adding > > pset type child nodes to pset type parent nodes in the API: > > fwnode_pset_add_child_node(), or something like that. > > > > I'm sorry to be a bit of pain in the butt with this. The build-in > > property support is a hack, it always was. A useful hack, but hack > > nevertheless. That means we need to be extra careful when expanding > > it, like here. > > I dunno how we got to here, what I tried to solve and Dmitry tries > to make more general is converting old boardfiles to use fwnode, because > I initially tried to just support gpio descriptors from board files. I understand that, but what I'm trying to say is that the solution for the child nodes is not generic enough. I'll try to explain below. > If boardfiles is what you mean with "build-in property support" I don't > know, it predates both device tree and ACPI and any other hardware > descriptions on the ARM architecture, from the perspective of these > systems things are fine and the hardware description languages > are the novelty... Rafael talked about "build-in properties" at one point, and I just started using that expression after that, but it's probable not the best term to describe this thing. But please note that we are not talking about only static information with these property sets. They can be populated dynamically as well, so this kind of extensions must consider and support that. On top of board files we need to consider things like glue and probing drivers and even buses in some cases. > But I guess you know because you worked with OMAP :) > > So there is something I don't understand here. Sorry for the poor choice of words on my behalf. I guess I'm not explaining my point properly. Let me try again. So I'm seeing a case that this solution does not seem to be able to support, but ultimately it will need to do so: with USB ports we are going to need to be able to assign a device to the child nodes that represent those ports. To be honest, normally I would not care about that, and I would simply wait for this to go into mainline, and then propose a modification to it so it can also support that other case. However, this time I feel that we should not do that. The solution for the child nodes should be implemented so that it can support all the known cases from the beginning. The reason for that is because I fear that we'll end up having a pile of ad-hoc style solutions, each solving an individual problem, and a whole lot of duplicated stuff for these property sets. In the end we will have spaghetti with meatballs that nobody is able fully understand nor handle. And I really see a strong possibility for that to happen here. Thanks,
Hi Dmitry, On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote: > > > + if (!parent_pset) > > > + return ERR_PTR(-EINVAL); > > > + > > > + p = pset_create_set(properties); > > > + if (IS_ERR(p)) > > > + return ERR_CAST(p); > > > + > > > + p->dev = dev; > > > > That looks wrong. > > > > I'm guessing the assumption here is that the child nodes will never be > > assigned to their own devices, but you can't do that. It will limit > > the use of the child nodes to a very small number of cases, possibly > > only to gpios. > > If I need to assign a node to a device I'll use device_add_properties() > API. device_add_child_properties() is for nodes living "below" the > device. device_add_properties() is not available to us before we have the actual struct device meant for the properties. If the child device is populated outside of the "boardfiles" then we have to be able to link it to the child node afterwards. But I took a closer look at the code and realized that you are not using that p->dev with the child nodes for anything, so I may be wrong about this. You are not actually linking the child nodes to that device here. > All nodes (the primary/secondary and children) would point to the owning > device, just for convenience. Since you don't use that for anything, then drop that line. It's only confusing. And besides, since we will have separate child devices for those child nodes, there is a small change that somebody needs to call device_remove_properties(child_dev). At least then that operation becomes safe, as it's a nop with the child pset nodes. > > I think that has to be fixed. It should not be a big deal. Just expect > > the child nodes to be removed separately, and add ref counting to the > > struct property_set handling. > > Why do we need to remove them separately and what do we need refcounting > for? If you could remove the nodes independently, then obviously the parent can not be removed before the children. The ref count would be there to prevent that. Though my original comment is not valid, I still think that the child nodes should be created and destroyed separately. Actually, I don't think we should be talking about child nodes in the API at all. Check below.. > > > + p->parent = parent_pset; > > > + list_add_tail(&p->child_node, &parent_pset->children); > > > + > > > + return &p->fwnode; > > > +} > > > +EXPORT_SYMBOL_GPL(device_add_child_properties); > > > > The child nodes will change the purpose of the build-in property > > support. Originally the goal was just to support adding of build-in > > device properties to real firmware nodes, but things have changed > > quite a bit from that. These child nodes are purely tied to the > > build-in device property support, so we should be talking about adding > > pset type child nodes to pset type parent nodes in the API: > > fwnode_pset_add_child_node(), or something like that. > > OK, I can change device_add_child_properties() to > fwnode_pset_add_child_node() if Rafael would prefer this name. So not even that. We should have API like this: struct fwnode_handle * fwnode_pset_create(struct fwnode_handle *parent, const char *name, const struct property_entry *props); void fwnode_pset_destroy(struct fwnode_handle *fwnode); int device_add_properties(struct device *dev, const struct fwnode_handle *pset_node); void device_remove_properties(struct device *dev); So basically we separate creation of the nodes from linking them to the devices completely. That would give this API much better structure. It would scale better, for example, it would then be possible to add child nodes from different locations if needed. Thanks,
On Mon, Sep 24, 2018 at 04:20:50PM +0300, Heikki Krogerus wrote: > Hi Dmitry, > > On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote: > > > > + if (!parent_pset) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + p = pset_create_set(properties); > > > > + if (IS_ERR(p)) > > > > + return ERR_CAST(p); > > > > + > > > > + p->dev = dev; > > > > > > That looks wrong. > > > > > > I'm guessing the assumption here is that the child nodes will never be > > > assigned to their own devices, but you can't do that. It will limit > > > the use of the child nodes to a very small number of cases, possibly > > > only to gpios. > > > > If I need to assign a node to a device I'll use device_add_properties() > > API. device_add_child_properties() is for nodes living "below" the > > device. > > device_add_properties() is not available to us before we have the > actual struct device meant for the properties. If the child device is > populated outside of the "boardfiles" then we have to be able to link > it to the child node afterwards. I think we are talking about totally different use cases and that is why we are having hard time coming to a mutually agreeable solution. Could you please describe in more detail what you would like to achieve, and preferably show how it is described now with DT and/or ACPI, so that I have a better frame of reference. Thanks.
On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote: > I think we are talking about totally different use cases and that is why > we are having hard time coming to a mutually agreeable solution. Could > you please describe in more detail what you would like to achieve, > and preferably show how it is described now with DT and/or ACPI, so that > I have a better frame of reference. Yes, of course. Sorry. USB ports are devices that usually the USB controller drivers register (or actually the USB core code). They are represented in both ACPI and DT as child nodes of the controller device node. The USB connector OF node is defined in file Documentation/devicetree/bindings/connector/usb-connector.txt In short, the controller drivers will request handle to a child node that represents a port, and only after that register the actual port device. The drivers I'm looking at currently are the USB Type-C port controller drivers and the port manager (in Greg's usb-next or linux-next): drivers/usb/typec/tcpm/tcpci.c drivers/usb/typec/tcpm/fusb302.c drivers/usb/typec/tcpm/tcpm.c The goal is simply to get rid of the platform data as usual, and ideally so that we don't need any extra code in order to support the "legacy" platforms. Thanks,
Hi Heikki, On Tue, Sep 25, 2018 at 03:19:27PM +0300, Heikki Krogerus wrote: > On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote: > > I think we are talking about totally different use cases and that is why > > we are having hard time coming to a mutually agreeable solution. Could > > you please describe in more detail what you would like to achieve, > > and preferably show how it is described now with DT and/or ACPI, so that > > I have a better frame of reference. > > Yes, of course. Sorry. > > USB ports are devices that usually the USB controller drivers register > (or actually the USB core code). They are represented in both ACPI and > DT as child nodes of the controller device node. The USB connector OF > node is defined in file > Documentation/devicetree/bindings/connector/usb-connector.txt > > In short, the controller drivers will request handle to a child node > that represents a port, and only after that register the actual port > device. > > The drivers I'm looking at currently are the USB Type-C port > controller drivers and the port manager (in Greg's usb-next or > linux-next): > > drivers/usb/typec/tcpm/tcpci.c > drivers/usb/typec/tcpm/fusb302.c > drivers/usb/typec/tcpm/tcpm.c > > The goal is simply to get rid of the platform data as usual, and > ideally so that we don't need any extra code in order to support the > "legacy" platforms. Are these actually used on any of the "legacy" platforms? I fetched linux-next today, but I do not actually see anything in drivers/usb/typec touching platform data... In the context of the connector, even before we descend to child nodes details, how do you propose implementing references between fwnodes? Especially since the other node (in case you complementing existing topology) may be ACPI or DT instance? I want to say that "You aren't gonna need it" for what you are asking here and we can actually split it apart if and when we actually want to separate creation of pset-backed device nodes and instantiating corresponding device structures. Thanks.
Hi Dmitry, Sorry for the late reply. I decided to give this a try myself, and I've now prepared something (not for the gpio stuff!). I'll do a quick internal review round, and send it to you guys as RFC after that.. On Fri, Oct 05, 2018 at 02:47:34PM -0700, Dmitry Torokhov wrote: > Hi Heikki, > > On Tue, Sep 25, 2018 at 03:19:27PM +0300, Heikki Krogerus wrote: > > On Mon, Sep 24, 2018 at 11:45:43AM -0700, Dmitry Torokhov wrote: > > > I think we are talking about totally different use cases and that is why > > > we are having hard time coming to a mutually agreeable solution. Could > > > you please describe in more detail what you would like to achieve, > > > and preferably show how it is described now with DT and/or ACPI, so that > > > I have a better frame of reference. > > > > Yes, of course. Sorry. > > > > USB ports are devices that usually the USB controller drivers register > > (or actually the USB core code). They are represented in both ACPI and > > DT as child nodes of the controller device node. The USB connector OF > > node is defined in file > > Documentation/devicetree/bindings/connector/usb-connector.txt > > > > In short, the controller drivers will request handle to a child node > > that represents a port, and only after that register the actual port > > device. > > > > The drivers I'm looking at currently are the USB Type-C port > > controller drivers and the port manager (in Greg's usb-next or > > linux-next): > > > > drivers/usb/typec/tcpm/tcpci.c > > drivers/usb/typec/tcpm/fusb302.c > > drivers/usb/typec/tcpm/tcpm.c > > > > The goal is simply to get rid of the platform data as usual, and > > ideally so that we don't need any extra code in order to support the > > "legacy" platforms. > > Are these actually used on any of the "legacy" platforms? I fetched > linux-next today, but I do not actually see anything in > drivers/usb/typec touching platform data... It's not touching any platform data, and I would like to keep it that way :-). The device for fusb302 is created for example in drivers/platform/x86/intel_cht_int33fe.c. The fusb302.c driver already tries to find that child node named "connector" (and so do the other port drivers). I'm trying to supply the driver that node here somehow so I can avoid quirks. > In the context of the connector, even before we descend to child nodes > details, how do you propose implementing references between fwnodes? > Especially since the other node (in case you complementing existing > topology) may be ACPI or DT instance? The different fwnode types (ACPI, OF, etc.) must live in parallel, i.e. you can not have something like ACPI parent node with property_set children if that's what you mean. The "secondary" member will link the different types of fwnodes together, but they will live their separate lives. I'm not planning on changing that. I'm just looking for a smooth way of describing the devices in software when the firmware is not supplying the hardware description, just like you. What I really need is separation of the fwnode registration from device registration in those cases. For that I now have a proposal, and I believe my proposal will work for you as a baseline as well. Thanks,
diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c index 08ecc13080ae..63f2377aefe8 100644 --- a/drivers/base/pset_property.c +++ b/drivers/base/pset_property.c @@ -18,6 +18,11 @@ struct property_set { struct device *dev; struct fwnode_handle fwnode; const struct property_entry *properties; + + struct property_set *parent; + /* Entry in parent->children list */ + struct list_head child_node; + struct list_head children; }; static const struct fwnode_operations pset_fwnode_ops; @@ -283,10 +288,47 @@ pset_fwnode_property_read_string_array(const struct fwnode_handle *fwnode, val, nval); } +struct fwnode_handle *pset_fwnode_get_parent(const struct fwnode_handle *fwnode) +{ + struct property_set *pset = to_pset_node(fwnode); + + return pset ? &pset->parent->fwnode : NULL; +} + +struct fwnode_handle * +pset_fwnode_get_next_subnode(const struct fwnode_handle *fwnode, + struct fwnode_handle *child) +{ + const struct property_set *pset = to_pset_node(fwnode); + struct property_set *first_child; + struct property_set *next; + + if (!pset) + return NULL; + + if (list_empty(&pset->children)) + return NULL; + + first_child = list_first_entry(&pset->children, struct property_set, + child_node); + + if (child) { + next = list_next_entry(to_pset_node(child), child_node); + if (next == first_child) + return NULL; + } else { + next = first_child; + } + + return &next->fwnode; +} + static const struct fwnode_operations pset_fwnode_ops = { .property_present = pset_fwnode_property_present, .property_read_int_array = pset_fwnode_read_int_array, .property_read_string_array = pset_fwnode_property_read_string_array, + .get_parent = pset_fwnode_get_parent, + .get_next_child_node = pset_fwnode_get_next_subnode, }; static void property_entry_free_data(const struct property_entry *p) @@ -439,24 +481,31 @@ EXPORT_SYMBOL_GPL(property_entries_free); */ static void pset_free_set(struct property_set *pset) { + struct property_set *child, *next; + if (!pset) return; + list_for_each_entry_safe(child, next, &pset->children, child_node) { + list_del(&child->child_node); + pset_free_set(child); + } + property_entries_free(pset->properties); kfree(pset); } /** - * pset_copy_set - copies property set - * @pset: Property set to copy + * pset_create_set - creates property set. + * @src: property entries for the set. * - * This function takes a deep copy of the given property set and returns - * pointer to the copy. Call device_free_property_set() to free resources - * allocated in this function. + * This function takes a deep copy of the given property entries and creates + * property set. Call pset_free_set() to free resources allocated in this + * function. * * Return: Pointer to the new property set or error pointer. */ -static struct property_set *pset_copy_set(const struct property_set *pset) +static struct property_set *pset_create_set(const struct property_entry *src) { struct property_entry *properties; struct property_set *p; @@ -465,7 +514,11 @@ static struct property_set *pset_copy_set(const struct property_set *pset) if (!p) return ERR_PTR(-ENOMEM); - properties = property_entries_dup(pset->properties); + INIT_LIST_HEAD(&p->child_node); + INIT_LIST_HEAD(&p->children); + p->fwnode.ops = &pset_fwnode_ops; + + properties = property_entries_dup(src); if (IS_ERR(properties)) { kfree(p); return ERR_CAST(properties); @@ -521,20 +574,53 @@ EXPORT_SYMBOL_GPL(device_remove_properties); int device_add_properties(struct device *dev, const struct property_entry *properties) { - struct property_set *p, pset; + struct property_set *p; if (!properties) return -EINVAL; - pset.properties = properties; - - p = pset_copy_set(&pset); + p = pset_create_set(properties); if (IS_ERR(p)) return PTR_ERR(p); - p->fwnode.ops = &pset_fwnode_ops; set_secondary_fwnode(dev, &p->fwnode); p->dev = dev; return 0; } EXPORT_SYMBOL_GPL(device_add_properties); + +/** + * device_add_child_properties - Add a collection of properties to a device object. + * @dev: Device to add properties to. + * @properties: Collection of properties to add. + * + * Associate a collection of device properties represented by @properties as a + * child of given @parent firmware node. The function takes a copy of + * @properties. + */ +struct fwnode_handle * +device_add_child_properties(struct device *dev, + struct fwnode_handle *parent, + const struct property_entry *properties) +{ + struct property_set *p; + struct property_set *parent_pset; + + if (!properties) + return ERR_PTR(-EINVAL); + + parent_pset = to_pset_node(parent); + if (!parent_pset) + return ERR_PTR(-EINVAL); + + p = pset_create_set(properties); + if (IS_ERR(p)) + return ERR_CAST(p); + + p->dev = dev; + p->parent = parent_pset; + list_add_tail(&p->child_node, &parent_pset->children); + + return &p->fwnode; +} +EXPORT_SYMBOL_GPL(device_add_child_properties); diff --git a/include/linux/property.h b/include/linux/property.h index ac8a1ebc4c1b..bb1cf4f30770 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -275,6 +275,10 @@ void property_entries_free(const struct property_entry *properties); int device_add_properties(struct device *dev, const struct property_entry *properties); +struct fwnode_handle * +device_add_child_properties(struct device *dev, + struct fwnode_handle *parent, + const struct property_entry *properties); void device_remove_properties(struct device *dev); bool device_dma_supported(struct device *dev);
Several drivers rely on having notion of sub-nodes when describing hardware, let's allow static board-defined properties also have it. The board files will then attach properties to devices in the following fashion: device_add_properties(&board_platform_device.dev, main_device_props); device_add_child_properties(&board_platform_device.dev, dev_fwnode(&board_platform_device.dev), child1_device_props); device_add_child_properties(&board_platform_device.dev, dev_fwnode(&board_platform_device.dev), child2_device_props); ... platform_device_register(&board_platform_device.dev); Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/base/pset_property.c | 110 +++++++++++++++++++++++++++++++---- include/linux/property.h | 4 ++ 2 files changed, 102 insertions(+), 12 deletions(-)