| Message ID | 20190713075259.243565-2-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make gpiolib work with static device properties | expand |
On Sat, Jul 13, 2019 at 9:53 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > It is helpful to know what device, if any, a software node is tied to, so > let's store a pointer to the device in software node structure. Note that > children software nodes will inherit their parent's device pointer, so we > do not have to traverse hierarchy to see what device the [sub]tree belongs > to. > > We will be using the device pointer to locate GPIO lookup tables for > devices with static properties. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> If some device core person like Rafael and/or Greg can ACK it I can apply this patch to the GPIO tree. Yours, Linus Walleij
On Monday, July 29, 2019 12:11:41 AM CEST Linus Walleij wrote: > On Sat, Jul 13, 2019 at 9:53 AM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > It is helpful to know what device, if any, a software node is tied to, so > > let's store a pointer to the device in software node structure. Note that > > children software nodes will inherit their parent's device pointer, so we > > do not have to traverse hierarchy to see what device the [sub]tree belongs > > to. > > > > We will be using the device pointer to locate GPIO lookup tables for > > devices with static properties. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > If some device core person like Rafael and/or Greg can ACK it I can > apply this patch to the GPIO tree. I don't have any concerns regarding this, so please feel free to add Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> to the commit when you apply it. Thanks!
On Sat, Jul 13, 2019 at 12:52:58AM -0700, Dmitry Torokhov wrote: > It is helpful to know what device, if any, a software node is tied to, so > let's store a pointer to the device in software node structure. Note that > children software nodes will inherit their parent's device pointer, so we > do not have to traverse hierarchy to see what device the [sub]tree belongs > to. > > We will be using the device pointer to locate GPIO lookup tables for > devices with static properties. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/base/property.c | 1 + > drivers/base/swnode.c | 35 ++++++++++++++++++++++++++++++++++- > include/linux/property.h | 5 +++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 348b37e64944..3bc93d4b35c4 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -527,6 +527,7 @@ int device_add_properties(struct device *dev, > if (IS_ERR(fwnode)) > return PTR_ERR(fwnode); > > + software_node_link_device(fwnode, dev); > set_secondary_fwnode(dev, fwnode); > return 0; > } > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > index 7fc5a18e02ad..fd12eea539b6 100644 > --- a/drivers/base/swnode.c > +++ b/drivers/base/swnode.c > @@ -24,6 +24,9 @@ struct software_node { > > /* properties */ > const struct property_entry *properties; > + > + /* device this node is associated with */ > + struct device *dev; > }; Let's not do that! The nodes can be, and in many cases are, associated with multiple devices. Every device is already linked with the software node kobject, so isn't it possible to simply walk trough those links in order to check the devices associated with the node? thanks,
On Mon, Jul 29, 2019 at 03:07:15PM +0300, Heikki Krogerus wrote: > On Sat, Jul 13, 2019 at 12:52:58AM -0700, Dmitry Torokhov wrote: > > It is helpful to know what device, if any, a software node is tied to, so > > let's store a pointer to the device in software node structure. Note that > > children software nodes will inherit their parent's device pointer, so we > > do not have to traverse hierarchy to see what device the [sub]tree belongs > > to. > > > > We will be using the device pointer to locate GPIO lookup tables for > > devices with static properties. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > drivers/base/property.c | 1 + > > drivers/base/swnode.c | 35 ++++++++++++++++++++++++++++++++++- > > include/linux/property.h | 5 +++++ > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > index 348b37e64944..3bc93d4b35c4 100644 > > --- a/drivers/base/property.c > > +++ b/drivers/base/property.c > > @@ -527,6 +527,7 @@ int device_add_properties(struct device *dev, > > if (IS_ERR(fwnode)) > > return PTR_ERR(fwnode); > > > > + software_node_link_device(fwnode, dev); > > set_secondary_fwnode(dev, fwnode); > > return 0; > > } > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > > index 7fc5a18e02ad..fd12eea539b6 100644 > > --- a/drivers/base/swnode.c > > +++ b/drivers/base/swnode.c > > @@ -24,6 +24,9 @@ struct software_node { > > > > /* properties */ > > const struct property_entry *properties; > > + > > + /* device this node is associated with */ > > + struct device *dev; > > }; > > Let's not do that! The nodes can be, and in many cases are, associated > with multiple devices. They do? Where? I see that set of properties can be shared between several devices, but when we instantiate SW node we create a new instance for device. This is also how ACPI and OF properties work; they not shared between devices (or, rather, the kernel creates distinct _and single_ devices for instance of ACPI or OF node). I do not think we want swnodes work differently from the other firmware nodes. > > Every device is already linked with the software node kobject, so > isn't it possible to simply walk trough those links in order to check > the devices associated with the node? No, we need to know the exact instance of a device, not a set of devices, because even though some properties can be shared, others can not. For example, even if 2 exactly same touch controllers in the system have "reset-gpios" property, they won't be the same GPIO for the both of them. Thanks.
On Mon, Jul 29, 2019 at 03:15:32PM +0200, Dmitry Torokhov wrote: > On Mon, Jul 29, 2019 at 03:07:15PM +0300, Heikki Krogerus wrote: > > On Sat, Jul 13, 2019 at 12:52:58AM -0700, Dmitry Torokhov wrote: > > > It is helpful to know what device, if any, a software node is tied to, so > > > let's store a pointer to the device in software node structure. Note that > > > children software nodes will inherit their parent's device pointer, so we > > > do not have to traverse hierarchy to see what device the [sub]tree belongs > > > to. > > > > > > We will be using the device pointer to locate GPIO lookup tables for > > > devices with static properties. > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > --- > > > drivers/base/property.c | 1 + > > > drivers/base/swnode.c | 35 ++++++++++++++++++++++++++++++++++- > > > include/linux/property.h | 5 +++++ > > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > > index 348b37e64944..3bc93d4b35c4 100644 > > > --- a/drivers/base/property.c > > > +++ b/drivers/base/property.c > > > @@ -527,6 +527,7 @@ int device_add_properties(struct device *dev, > > > if (IS_ERR(fwnode)) > > > return PTR_ERR(fwnode); > > > > > > + software_node_link_device(fwnode, dev); > > > set_secondary_fwnode(dev, fwnode); > > > return 0; > > > } > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > > > index 7fc5a18e02ad..fd12eea539b6 100644 > > > --- a/drivers/base/swnode.c > > > +++ b/drivers/base/swnode.c > > > @@ -24,6 +24,9 @@ struct software_node { > > > > > > /* properties */ > > > const struct property_entry *properties; > > > + > > > + /* device this node is associated with */ > > > + struct device *dev; > > > }; > > > > Let's not do that! The nodes can be, and in many cases are, associated > > with multiple devices. > > They do? Where? I see that set of properties can be shared between > several devices, but when we instantiate SW node we create a new > instance for device. This is also how ACPI and OF properties work; they > not shared between devices (or, rather, the kernel creates distinct _and > single_ devices for instance of ACPI or OF node). I do not think we want > swnodes work differently from the other firmware nodes. Having multiple devices linked to a single node is quite normal. Most multifunctional devices will share a single node. The USB port devices will share their node (if they have one) with any device that is attached to them. Etc. If you want to check how this works with ACPI, then find "physical_node" named files from sysfs. The ACPI node folders in sysfs have symlinks named "physical_node<n>" for every device they are bind to. The first one is named just "physical_node", the second "physical_node1", the third "physical_node2", and so on. > > Every device is already linked with the software node kobject, so > > isn't it possible to simply walk trough those links in order to check > > the devices associated with the node? > > No, we need to know the exact instance of a device, not a set of > devices, because even though some properties can be shared, others can > not. For example, even if 2 exactly same touch controllers in the system > have "reset-gpios" property, they won't be the same GPIO for the both of > them. I don't think I completely understand the use case you had in mind for this API, but since you planned to use it with the GPIO lookup tables, I'm going to assume it's not needed after all. Let's replace those with the references instead like I proposed in my reply to the 2/2 patch. Linking a single device with a node like that is in any case not acceptable nor possible. thanks,
On 7/30/2019 1:52 PM, Heikki Krogerus wrote: > On Mon, Jul 29, 2019 at 03:15:32PM +0200, Dmitry Torokhov wrote: >> On Mon, Jul 29, 2019 at 03:07:15PM +0300, Heikki Krogerus wrote: >>> On Sat, Jul 13, 2019 at 12:52:58AM -0700, Dmitry Torokhov wrote: >>>> It is helpful to know what device, if any, a software node is tied to, so >>>> let's store a pointer to the device in software node structure. Note that >>>> children software nodes will inherit their parent's device pointer, so we >>>> do not have to traverse hierarchy to see what device the [sub]tree belongs >>>> to. >>>> >>>> We will be using the device pointer to locate GPIO lookup tables for >>>> devices with static properties. >>>> >>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>>> --- >>>> drivers/base/property.c | 1 + >>>> drivers/base/swnode.c | 35 ++++++++++++++++++++++++++++++++++- >>>> include/linux/property.h | 5 +++++ >>>> 3 files changed, 40 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/base/property.c b/drivers/base/property.c >>>> index 348b37e64944..3bc93d4b35c4 100644 >>>> --- a/drivers/base/property.c >>>> +++ b/drivers/base/property.c >>>> @@ -527,6 +527,7 @@ int device_add_properties(struct device *dev, >>>> if (IS_ERR(fwnode)) >>>> return PTR_ERR(fwnode); >>>> >>>> + software_node_link_device(fwnode, dev); >>>> set_secondary_fwnode(dev, fwnode); >>>> return 0; >>>> } >>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c >>>> index 7fc5a18e02ad..fd12eea539b6 100644 >>>> --- a/drivers/base/swnode.c >>>> +++ b/drivers/base/swnode.c >>>> @@ -24,6 +24,9 @@ struct software_node { >>>> >>>> /* properties */ >>>> const struct property_entry *properties; >>>> + >>>> + /* device this node is associated with */ >>>> + struct device *dev; >>>> }; >>> Let's not do that! The nodes can be, and in many cases are, associated >>> with multiple devices. >> They do? Where? I see that set of properties can be shared between >> several devices, but when we instantiate SW node we create a new >> instance for device. This is also how ACPI and OF properties work; they >> not shared between devices (or, rather, the kernel creates distinct _and >> single_ devices for instance of ACPI or OF node). I do not think we want >> swnodes work differently from the other firmware nodes. > Having multiple devices linked to a single node is quite normal. Most > multifunctional devices will share a single node. The USB port devices > will share their node (if they have one) with any device that is > attached to them. Etc. > > If you want to check how this works with ACPI, then find > "physical_node" named files from sysfs. The ACPI node folders in sysfs > have symlinks named "physical_node<n>" for every device they are bind > to. The first one is named just "physical_node", the second > "physical_node1", the third "physical_node2", and so on. > >>> Every device is already linked with the software node kobject, so >>> isn't it possible to simply walk trough those links in order to check >>> the devices associated with the node? >> No, we need to know the exact instance of a device, not a set of >> devices, because even though some properties can be shared, others can >> not. For example, even if 2 exactly same touch controllers in the system >> have "reset-gpios" property, they won't be the same GPIO for the both of >> them. > I don't think I completely understand the use case you had in mind for > this API, but since you planned to use it with the GPIO lookup tables, > I'm going to assume it's not needed after all. Let's replace those > with the references instead like I proposed in my reply to the 2/2 > patch. > > Linking a single device with a node like that is in any case not > acceptable nor possible. > I think I need to withdraw my ACK here at this point.
On Tue, Jul 30, 2019 at 04:49:50PM +0200, Rafael J. Wysocki wrote: > On 7/30/2019 1:52 PM, Heikki Krogerus wrote: > > On Mon, Jul 29, 2019 at 03:15:32PM +0200, Dmitry Torokhov wrote: > > > On Mon, Jul 29, 2019 at 03:07:15PM +0300, Heikki Krogerus wrote: > > > > On Sat, Jul 13, 2019 at 12:52:58AM -0700, Dmitry Torokhov wrote: > > > > > It is helpful to know what device, if any, a software node is tied to, so > > > > > let's store a pointer to the device in software node structure. Note that > > > > > children software nodes will inherit their parent's device pointer, so we > > > > > do not have to traverse hierarchy to see what device the [sub]tree belongs > > > > > to. > > > > > > > > > > We will be using the device pointer to locate GPIO lookup tables for > > > > > devices with static properties. > > > > > > > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > > > --- > > > > > drivers/base/property.c | 1 + > > > > > drivers/base/swnode.c | 35 ++++++++++++++++++++++++++++++++++- > > > > > include/linux/property.h | 5 +++++ > > > > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/base/property.c b/drivers/base/property.c > > > > > index 348b37e64944..3bc93d4b35c4 100644 > > > > > --- a/drivers/base/property.c > > > > > +++ b/drivers/base/property.c > > > > > @@ -527,6 +527,7 @@ int device_add_properties(struct device *dev, > > > > > if (IS_ERR(fwnode)) > > > > > return PTR_ERR(fwnode); > > > > > + software_node_link_device(fwnode, dev); > > > > > set_secondary_fwnode(dev, fwnode); > > > > > return 0; > > > > > } > > > > > diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c > > > > > index 7fc5a18e02ad..fd12eea539b6 100644 > > > > > --- a/drivers/base/swnode.c > > > > > +++ b/drivers/base/swnode.c > > > > > @@ -24,6 +24,9 @@ struct software_node { > > > > > /* properties */ > > > > > const struct property_entry *properties; > > > > > + > > > > > + /* device this node is associated with */ > > > > > + struct device *dev; > > > > > }; > > > > Let's not do that! The nodes can be, and in many cases are, associated > > > > with multiple devices. > > > They do? Where? I see that set of properties can be shared between > > > several devices, but when we instantiate SW node we create a new > > > instance for device. This is also how ACPI and OF properties work; they > > > not shared between devices (or, rather, the kernel creates distinct _and > > > single_ devices for instance of ACPI or OF node). I do not think we want > > > swnodes work differently from the other firmware nodes. > > Having multiple devices linked to a single node is quite normal. Most > > multifunctional devices will share a single node. The USB port devices > > will share their node (if they have one) with any device that is > > attached to them. Etc. > > > > If you want to check how this works with ACPI, then find > > "physical_node" named files from sysfs. The ACPI node folders in sysfs > > have symlinks named "physical_node<n>" for every device they are bind > > to. The first one is named just "physical_node", the second > > "physical_node1", the third "physical_node2", and so on. > > > > > > Every device is already linked with the software node kobject, so > > > > isn't it possible to simply walk trough those links in order to check > > > > the devices associated with the node? > > > No, we need to know the exact instance of a device, not a set of > > > devices, because even though some properties can be shared, others can > > > not. For example, even if 2 exactly same touch controllers in the system > > > have "reset-gpios" property, they won't be the same GPIO for the both of > > > them. > > I don't think I completely understand the use case you had in mind for > > this API, but since you planned to use it with the GPIO lookup tables, > > I'm going to assume it's not needed after all. Let's replace those > > with the references instead like I proposed in my reply to the 2/2 > > patch. > > > > Linking a single device with a node like that is in any case not > > acceptable nor possible. > > > I think I need to withdraw my ACK here at this point. OK, fair enough, I'll see if I can make the references that Heikki mentioned work for me. Thanks.
diff --git a/drivers/base/property.c b/drivers/base/property.c index 348b37e64944..3bc93d4b35c4 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -527,6 +527,7 @@ int device_add_properties(struct device *dev, if (IS_ERR(fwnode)) return PTR_ERR(fwnode); + software_node_link_device(fwnode, dev); set_secondary_fwnode(dev, fwnode); return 0; } diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c index 7fc5a18e02ad..fd12eea539b6 100644 --- a/drivers/base/swnode.c +++ b/drivers/base/swnode.c @@ -24,6 +24,9 @@ struct software_node { /* properties */ const struct property_entry *properties; + + /* device this node is associated with */ + struct device *dev; }; static DEFINE_IDA(swnode_root_ids); @@ -607,8 +610,14 @@ fwnode_create_software_node(const struct property_entry *properties, INIT_LIST_HEAD(&swnode->children); swnode->parent = p; - if (p) + if (p) { list_add_tail(&swnode->entry, &p->children); + /* + * We want to maintain the same association as the parent node, + * so we can easily locate corresponding device. + */ + swnode->dev = p->dev; + } ret = kobject_init_and_add(&swnode->kobj, &software_node_type, p ? &p->kobj : NULL, "node%d", swnode->id); @@ -639,6 +648,30 @@ void fwnode_remove_software_node(struct fwnode_handle *fwnode) } EXPORT_SYMBOL_GPL(fwnode_remove_software_node); +int software_node_link_device(struct fwnode_handle *fwnode, struct device *dev) +{ + struct software_node *swnode = to_software_node(fwnode); + + if (!swnode) + return -EINVAL; + + swnode->dev = dev; + return 0; +} +EXPORT_SYMBOL_GPL(software_node_link_device); + +struct device * +software_node_get_linked_device(const struct fwnode_handle *fwnode) +{ + const struct software_node *swnode = to_software_node(fwnode); + + if (!swnode) + return ERR_PTR(-EINVAL); + + return swnode->dev; +} +EXPORT_SYMBOL_GPL(software_node_get_linked_device); + int software_node_notify(struct device *dev, unsigned long action) { struct fwnode_handle *fwnode = dev_fwnode(dev); diff --git a/include/linux/property.h b/include/linux/property.h index e9caa290cda5..754188cfd9db 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -338,4 +338,9 @@ fwnode_create_software_node(const struct property_entry *properties, const struct fwnode_handle *parent); void fwnode_remove_software_node(struct fwnode_handle *fwnode); +int software_node_link_device(struct fwnode_handle *fwnode, + struct device *device); +struct device * +software_node_get_linked_device(const struct fwnode_handle *fwnode); + #endif /* _LINUX_PROPERTY_H_ */
It is helpful to know what device, if any, a software node is tied to, so let's store a pointer to the device in software node structure. Note that children software nodes will inherit their parent's device pointer, so we do not have to traverse hierarchy to see what device the [sub]tree belongs to. We will be using the device pointer to locate GPIO lookup tables for devices with static properties. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/base/property.c | 1 + drivers/base/swnode.c | 35 ++++++++++++++++++++++++++++++++++- include/linux/property.h | 5 +++++ 3 files changed, 40 insertions(+), 1 deletion(-)