[PATCH 0/2] Make gpiolib work with static device properties

[PATCH 0/2] Make gpiolib work with static device properties

[Dmitry Torokhov]

Hi,

These 2 patches implement GPIOs lookup for boards still using static
properties (instead of OF or ACPI), which will allow drivers to abandon
the custom platform data structures and switch to using generic bindings
with gpiod_get_*() API working transparently.

The 2nd patch was posted in September of 2018, before Heikki did the
switch from psets to software_nodes; I finally rebased it.

Enrico, it looks like you are working with a board that uses gpio_keys
that still uses platform data, it would be great if you could try this
out as I have not.

Thanks!

Dmitry Torokhov (2):
  drivers: base: swnode: link devices to software nodes
  gpiolib: add support for fetching descriptors from static properties

 drivers/base/property.c  |   1 +
 drivers/base/swnode.c    |  35 ++++++++++++-
 drivers/gpio/gpiolib.c   | 108 ++++++++++++++++++++++++++++-----------
 include/linux/property.h |   5 ++
 4 files changed, 119 insertions(+), 30 deletions(-)

-- 
2.22.0.510.g264f2c817a-goog

[PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Dmitry Torokhov]

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;
 };
 
 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_ */
-- 
2.22.0.510.g264f2c817a-goog

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Linus Walleij]

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

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Rafael J. Wysocki]

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!

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Rafael J. Wysocki]

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.

Re: [PATCH 1/2] drivers: base: swnode: link devices to software nodes

[Dmitry Torokhov]

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.

-- 
Dmitry

[PATCH 2/2] gpiolib: add support for fetching descriptors from static properties

[Dmitry Torokhov]

Now that static device properties understand notion of child nodes, let's
teach gpiolib to tie such children and machine GPIO descriptor tables.
We will continue using a single table for entire device, but instead of
using connection ID as a lookup key in the GPIO descriptor table directly,
we will perform additional translation: fwnode_get_named_gpiod() when
dealing with property_set-backed fwnodes will try parsing string property
with name matching connection ID and use result of the lookup as the key in
the table:

static const struct property_entry dev_child1_props[] __initconst = {
	...
	PROPERTY_ENTRY_STRING("gpios",		"child-1-gpios"),
	{ }
};

static struct gpiod_lookup_table dev_gpiod_table = {
	.dev_id = "some-device",
	.table = {
		...
		GPIO_LOOKUP_IDX("B", 1, "child-1-gpios", 1, GPIO_ACTIVE_LOW),
		...
	},
};

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/gpio/gpiolib.c | 108 ++++++++++++++++++++++++++++++-----------
 1 file changed, 79 insertions(+), 29 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e013d417a936..b6574febe2b8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4307,39 +4307,44 @@ struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
 }
 EXPORT_SYMBOL(gpiod_get_from_of_node);
 
-/**
- * fwnode_get_named_gpiod - obtain a GPIO from firmware node
- * @fwnode:	handle of the firmware node
- * @propname:	name of the firmware property representing the GPIO
- * @index:	index of the GPIO to obtain for the consumer
- * @dflags:	GPIO initialization flags
- * @label:	label to attach to the requested GPIO
- *
- * This function can be used for drivers that get their configuration
- * from opaque firmware.
- *
- * The function properly finds the corresponding GPIO using whatever is the
- * underlying firmware interface and then makes sure that the GPIO
- * descriptor is requested before it is returned to the caller.
- *
- * Returns:
- * On successful request the GPIO pin is configured in accordance with
- * provided @dflags.
- *
- * In case of error an ERR_PTR() is returned.
- */
-struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label)
+static struct gpio_desc *
+software_node_get_gpiod(struct fwnode_handle *fwnode,
+			const char *propname, int index, unsigned long *flags)
+{
+	struct device *dev;
+	const char *con_id;
+
+	dev = software_node_get_linked_device(fwnode);
+	if (IS_ERR_OR_NULL(dev))
+		return ERR_PTR(-EINVAL);
+
+	if (fwnode_property_read_string(fwnode, propname, &con_id)) {
+		/*
+		 * We could not find string mapping property name to
+		 * entry in gpio lookup table. Let's see if we are
+		 * dealing with firmware node corresponding to the
+		 * device (and not a child node): for such nodes we can
+		 * try doing lookup directly with property name.
+		 */
+		if (fwnode_get_parent(fwnode))
+			return ERR_PTR(-ENOENT);
+
+		con_id = propname;
+	}
+
+	return gpiod_find(dev, con_id, index, flags);
+}
+
+static struct gpio_desc *__fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
+						  const char *propname,
+						  int index,
+						  enum gpiod_flags dflags,
+						  const char *label)
 {
 	unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT;
 	struct gpio_desc *desc = ERR_PTR(-ENODEV);
 	int ret;
 
-	if (!fwnode)
-		return ERR_PTR(-EINVAL);
-
 	if (is_of_node(fwnode)) {
 		desc = gpiod_get_from_of_node(to_of_node(fwnode),
 					      propname, index,
@@ -4355,9 +4360,13 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 		acpi_gpio_update_gpiod_flags(&dflags, &info);
 		acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
+	} else if (is_software_node(fwnode)) {
+		desc = software_node_get_gpiod(fwnode, propname, index,
+					       &lflags);
+		if (IS_ERR(desc))
+			return desc;
 	}
 
-	/* Currently only ACPI takes this path */
 	ret = gpiod_request(desc, label);
 	if (ret)
 		return ERR_PTR(ret);
@@ -4370,6 +4379,47 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 
 	return desc;
 }
+
+/**
+ * fwnode_get_named_gpiod - obtain a GPIO from firmware node
+ * @fwnode:	handle of the firmware node
+ * @propname:	name of the firmware property representing the GPIO
+ * @index:	index of the GPIO to obtain for the consumer
+ * @dflags:	GPIO initialization flags
+ * @label:	label to attach to the requested GPIO
+ *
+ * This function can be used for drivers that get their configuration
+ * from opaque firmware.
+ *
+ * The function properly finds the corresponding GPIO using whatever is the
+ * underlying firmware interface and then makes sure that the GPIO
+ * descriptor is requested before it is returned to the caller.
+ *
+ * Returns:
+ * On successful request the GPIO pin is configured in accordance with
+ * provided @dflags.
+ *
+ * In case of error an ERR_PTR() is returned.
+ */
+struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
+					 const char *propname, int index,
+					 enum gpiod_flags dflags,
+					 const char *label)
+{
+	struct gpio_desc *desc;
+
+	if (!fwnode)
+		return ERR_PTR(-EINVAL);
+
+	desc = __fwnode_get_named_gpiod(fwnode, propname, index, dflags, label);
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT &&
+	    !IS_ERR_OR_NULL(fwnode->secondary)) {
+		desc = __fwnode_get_named_gpiod(fwnode->secondary,
+						propname, index, dflags, label);
+	}
+
+	return desc;
+}
 EXPORT_SYMBOL_GPL(fwnode_get_named_gpiod);
 
 /**
-- 
2.22.0.510.g264f2c817a-goog

Re: [PATCH 2/2] gpiolib: add support for fetching descriptors from static properties

[Linus Walleij]

On Sat, Jul 13, 2019 at 9:53 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Now that static device properties understand notion of child nodes, let's
> teach gpiolib to tie such children and machine GPIO descriptor tables.
> We will continue using a single table for entire device, but instead of
> using connection ID as a lookup key in the GPIO descriptor table directly,
> we will perform additional translation: fwnode_get_named_gpiod() when
> dealing with property_set-backed fwnodes will try parsing string property
> with name matching connection ID and use result of the lookup as the key in
> the table:
>
> static const struct property_entry dev_child1_props[] __initconst = {
>         ...
>         PROPERTY_ENTRY_STRING("gpios",          "child-1-gpios"),
>         { }
> };
>
> static struct gpiod_lookup_table dev_gpiod_table = {
>         .dev_id = "some-device",
>         .table = {
>                 ...
>                 GPIO_LOOKUP_IDX("B", 1, "child-1-gpios", 1, GPIO_ACTIVE_LOW),
>                 ...
>         },
> };
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I'm pretty grateful for this since I think at one point I provoked this whole
series. :)

> +static struct gpio_desc *__fwnode_get_named_gpiod(struct fwnode_handle *fwnode,

I am allergic to __underscore_with_unclear_semantics() so I will
change this when applying to something with meaning (I even
like "inner_" better.)

Otherwise it's good to go when I get an ACK on the first patch.

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: add support for fetching descriptors from static properties

[Heikki Krogerus]

Hi Dmitry,

On Sat, Jul 13, 2019 at 12:52:59AM -0700, Dmitry Torokhov wrote:
> Now that static device properties understand notion of child nodes, let's
> teach gpiolib to tie such children and machine GPIO descriptor tables.
> We will continue using a single table for entire device, but instead of
> using connection ID as a lookup key in the GPIO descriptor table directly,
> we will perform additional translation: fwnode_get_named_gpiod() when
> dealing with property_set-backed fwnodes will try parsing string property
> with name matching connection ID and use result of the lookup as the key in
> the table:
> 
> static const struct property_entry dev_child1_props[] __initconst = {
> 	...
> 	PROPERTY_ENTRY_STRING("gpios",		"child-1-gpios"),
> 	{ }
> };
> 
> static struct gpiod_lookup_table dev_gpiod_table = {
> 	.dev_id = "some-device",
> 	.table = {
> 		...
> 		GPIO_LOOKUP_IDX("B", 1, "child-1-gpios", 1, GPIO_ACTIVE_LOW),
> 		...
> 	},
> };

We don't need struct gpiod_lookup_table anymore. We can mimic DT with
the software nodes now that we have those "reference properties". A
gpio reference with the software nodes would look something like this:

        enum {
                GPIO_CONTROLLER,
                MY_DEVICE
        };

        static const struct software_node nodes[];

        static const struct software_node_ref_args reset_gpio_ref = {
                .node = &nodes[GPIO_CONTROLLER],
                .nargs = 2,
                .args = {
                        14,                     /* line number */
                        GPIO_ACTIVE_HIGH        /* flags */
                }
        };

        static const struct software_node_reference my_refs[] = {
                { "reset-gpios", 1, &reset_gpio_ref }
        };

        /* Optionally, we could support gpiochip finding by name... */
        static const struct property_entry my_props[] = {
                PROPERTY_ENTRY_STRING("gpio-controller", "name_of_the_controller")
        };

        static const struct software_node nodes[] = {
                [GPIO_CONTROLLER]       = { "gpio_controller" },
                [MY_DEVICE]             = { "my_device", NULL, my_props, my_refs }
        };

        void my_init(void)
        {
                ...
                ret = software_node_register_nodes(nodes);
                ...
        }

In gpiolib we should now be able to access that reference with
fwnode_property_get_references_args():

        static int gpiochip_match_fwnode(struct gpio_chip *chip, void *fwnode)
        {
                /* The fwnode member needs to be added to struct gpio_chip */
                return chip->fwnode == fwnode;
        }

        static struct gpio_desc *gpiod_find(struct device *dev,
                                            const char *con_id,
                                            unsigned int idx,
                                            unsigned long flags)
        {
                struct fwnode_reference_args args;
                struct gpio_chip *chip;
                struct gpio_desc *desc;
                const char *name;
                int ret;

                ret = fwnode_property_get_refernce_args(dev_fwnode(dev), con_id,
                                                        NULL, idx, &args);
                ...

                /* Let's find the gpiochip */
                chip = gpiochip_find(args.fwnode, gpiochip_match_fwnode);
                ...

                /* Or optionally with find_chip_by_name() */
                //ret = device_property_read_string(dev, "gpio-controller", &name);
                ...
                //chip = find_chip_by_name(name);
                ...

                /* I'm assuming hwnum is the same as line number? */
                desc = gpiochip_get_desc(chip, args.args[0]);
                *flags = args.args[1];

                return desc;
        }

The above is just an example, but I'm pretty sure that something like
it (with a little bit of tuning) is all that we need.

thanks,

-- 
heikki