[PATCH 0/5] Add of_ functions for device_link_add()

[PATCH 0/5] Add of_ functions for device_link_add()

[Benjamin Gaignard]

It could happen that we need to control suspend/resume ordering between
devices without obvious consumer/supplier link. For example when touchscreens
and DSI panels share the same reset line, in this case we need to be sure 
of pm_runtime operations ordering between those two devices to correctly
perform reset.
DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
I2C client and I2C bus or regulator client and regulator provider) so we need
to describe this in device-tree.

This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
helpers to find and parse 'links-add' property in a device-tree node.
It allows to create and remove links between consumer and suppliers from 
device-tree data so consumers will be suspend before their suppliers and resume 
after them.

Benjamin Gaignard (3):
  of/device: Add of_ functions for device_link_{add,remove}
  Input: edt-ft5x06: Document links-add property
  Input: goodix: Document links-add property

Yannick Fertré (2):
  input: edt-ft5x06 - Call devm_of_device_links_add() to create links
  input: goodix - Call devm_of_device_links_add() to create links

 .../bindings/input/touchscreen/edt-ft5x06.txt      |   2 +
 .../bindings/input/touchscreen/goodix.txt          |   2 +
 drivers/input/touchscreen/edt-ft5x06.c             |   2 +
 drivers/input/touchscreen/goodix.c                 |   3 +
 drivers/of/device.c                                | 103 +++++++++++++++++++++
 include/linux/of_device.h                          |  20 ++++
 6 files changed, 132 insertions(+)

-- 
2.15.0

[PATCH 1/5] of/device: Add of_ functions for device_link_{add,remove}

[Benjamin Gaignard]

Allows to create and remove links between consumer and suppliers from 
device-tree data. Use 'links-add' property from consumer node to setup
a link with a list of suppliers.
Consumers will be suspend before their suppliers and resume after them.

Add devm_of_device_links_add() to automatically remove the links
when the device is unbound from the bus.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/of/device.c       | 103 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_device.h |  20 +++++++++
 2 files changed, 123 insertions(+)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 3717f2a20d0d..011ba9bf7642 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -336,3 +336,106 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
+
+/**
+ * of_device_links_add - Create links between consumer and suppliers from
+ * device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_device_links_add(struct device *consumer)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+	int i = 0;
+
+	np = of_parse_phandle(consumer->of_node, "links-add", i++);
+	while (np) {
+		pdev = of_find_device_by_node(np);
+		of_node_put(np);
+		if (!pdev)
+			return -EINVAL;
+
+		device_link_add(consumer, &pdev->dev, DL_FLAG_STATELESS);
+		platform_device_put(pdev);
+
+		np = of_parse_phandle(consumer->of_node, "links-add", i++);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_links_add);
+
+/**
+ * of_device_links_remove - Remove links between consumer and suppliers from
+ * device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ */
+int of_device_links_remove(struct device *consumer)
+{
+	struct device_node *np;
+	struct platform_device *pdev;
+	int i = 0;
+
+	np = of_parse_phandle(consumer->of_node, "links-add", i++);
+	while (np) {
+		pdev = of_find_device_by_node(np);
+		of_node_put(np);
+		if (!pdev)
+			return -EINVAL;
+
+		device_link_remove(consumer, &pdev->dev);
+		platform_device_put(pdev);
+
+		np = of_parse_phandle(consumer->of_node, "links-add", i++);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_device_links_remove);
+
+static void devm_of_device_links_remove(struct device *dev, void *res)
+{
+	of_device_links_remove(*(struct device **)res);
+}
+
+/**
+ * devm_of_device_links_add - Create links between consumer and suppliers
+ * from device tree data
+ *
+ * @consumer: consumer device
+ *
+ * Returns 0 on success, < 0 on failure.
+ *
+ * Similar to of_device_links_add(), but will automatically call
+ * of_device_links_remove() when the device is unbound from the bus.
+ */
+int devm_of_device_links_add(struct device *consumer)
+{
+		struct device **ptr;
+	int ret;
+
+	if (!consumer)
+		return -EINVAL;
+
+	ptr = devres_alloc(devm_of_device_links_remove,
+			   sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = of_device_links_add(consumer);
+	if (ret < 0) {
+		devres_free(ptr);
+	} else {
+		*ptr = consumer;
+		devres_add(consumer, ptr);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_of_device_links_add);
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 8d31e39dd564..ad01db6828e8 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -41,6 +41,11 @@ extern int of_device_request_module(struct device *dev);
 extern void of_device_uevent(struct device *dev, struct kobj_uevent_env *env);
 extern int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env);
 
+
+extern int of_device_links_add(struct device *consumer);
+extern int of_device_links_remove(struct device *consumer);
+extern int devm_of_device_links_add(struct device *consumer);
+
 static inline void of_device_node_put(struct device *dev)
 {
 	of_node_put(dev->of_node);
@@ -91,6 +96,21 @@ static inline int of_device_uevent_modalias(struct device *dev,
 	return -ENODEV;
 }
 
+static int of_device_links_add(struct device *consumer)
+{
+	return 0;
+}
+
+static int of_device_links_remove(struct device *consumer)
+{
+	return 0;
+}
+
+static int devm_of_device_links_add(struct device *consumer)
+{
+	return 0;
+}
+
 static inline void of_device_node_put(struct device *dev) { }
 
 static inline const struct of_device_id *__of_match_device(
-- 
2.15.0

Re: [PATCH 1/5] of/device: Add of_ functions for device_link_{add,remove}

[Benjamin GAIGNARD]

On 4/24/19 3:06 PM, Rafael J. Wysocki wrote:
> On 4/24/2019 12:19 PM, Benjamin Gaignard wrote:
>> Allows to create and remove links between consumer and suppliers from
>> device-tree data. Use 'links-add' property from consumer node to setup
>> a link with a list of suppliers.
>
> One immediate question about this one is why stateless links are 
> better here?
They aren't better, it is just because I wanted to keep one of_ function 
for each related device_link_* functions.
>
>> Consumers will be suspend before their suppliers and resume after them.
>>
>> Add devm_of_device_links_add() to automatically remove the links
>> when the device is unbound from the bus.
>
> And this might not be necessary even with managed links.

I guess I could use DL_FLAG_PM_RUNTIME and DL_FLAG_AUTOREMOVE_CONSUMER 
flags and just keep

of_device_links_add() but the naming will not be explicit.

>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>   drivers/of/device.c       | 103 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/of_device.h |  20 +++++++++
>>   2 files changed, 123 insertions(+)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 3717f2a20d0d..011ba9bf7642 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -336,3 +336,106 @@ int of_device_uevent_modalias(struct device 
>> *dev, struct kobj_uevent_env *env)
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>> +
>> +/**
>> + * of_device_links_add - Create links between consumer and suppliers 
>> from
>> + * device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + */
>> +int of_device_links_add(struct device *consumer)
>> +{
>> +    struct device_node *np;
>> +    struct platform_device *pdev;
>> +    int i = 0;
>> +
>> +    np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +    while (np) {
>> +        pdev = of_find_device_by_node(np);
>> +        of_node_put(np);
>> +        if (!pdev)
>> +            return -EINVAL;
>> +
>> +        device_link_add(consumer, &pdev->dev, DL_FLAG_STATELESS);
>> +        platform_device_put(pdev);
>> +
>> +        np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_links_add);
>> +
>> +/**
>> + * of_device_links_remove - Remove links between consumer and 
>> suppliers from
>> + * device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + */
>> +int of_device_links_remove(struct device *consumer)
>> +{
>> +    struct device_node *np;
>> +    struct platform_device *pdev;
>> +    int i = 0;
>> +
>> +    np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +    while (np) {
>> +        pdev = of_find_device_by_node(np);
>> +        of_node_put(np);
>> +        if (!pdev)
>> +            return -EINVAL;
>> +
>> +        device_link_remove(consumer, &pdev->dev);
>> +        platform_device_put(pdev);
>> +
>> +        np = of_parse_phandle(consumer->of_node, "links-add", i++);
>> +    }
>> +
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_device_links_remove);
>> +
>> +static void devm_of_device_links_remove(struct device *dev, void *res)
>> +{
>> +    of_device_links_remove(*(struct device **)res);
>> +}
>> +
>> +/**
>> + * devm_of_device_links_add - Create links between consumer and 
>> suppliers
>> + * from device tree data
>> + *
>> + * @consumer: consumer device
>> + *
>> + * Returns 0 on success, < 0 on failure.
>> + *
>> + * Similar to of_device_links_add(), but will automatically call
>> + * of_device_links_remove() when the device is unbound from the bus.
>> + */
>> +int devm_of_device_links_add(struct device *consumer)
>> +{
>> +        struct device **ptr;
>> +    int ret;
>> +
>> +    if (!consumer)
>> +        return -EINVAL;
>> +
>> +    ptr = devres_alloc(devm_of_device_links_remove,
>> +               sizeof(*ptr), GFP_KERNEL);
>> +    if (!ptr)
>> +        return -ENOMEM;
>> +
>> +    ret = of_device_links_add(consumer);
>> +    if (ret < 0) {
>> +        devres_free(ptr);
>> +    } else {
>> +        *ptr = consumer;
>> +        devres_add(consumer, ptr);
>> +    }
>> +
>> +    return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_device_links_add);
>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>> index 8d31e39dd564..ad01db6828e8 100644
>> --- a/include/linux/of_device.h
>> +++ b/include/linux/of_device.h
>> @@ -41,6 +41,11 @@ extern int of_device_request_module(struct device 
>> *dev);
>>   extern void of_device_uevent(struct device *dev, struct 
>> kobj_uevent_env *env);
>>   extern int of_device_uevent_modalias(struct device *dev, struct 
>> kobj_uevent_env *env);
>>   +
>> +extern int of_device_links_add(struct device *consumer);
>> +extern int of_device_links_remove(struct device *consumer);
>> +extern int devm_of_device_links_add(struct device *consumer);
>> +
>>   static inline void of_device_node_put(struct device *dev)
>>   {
>>       of_node_put(dev->of_node);
>> @@ -91,6 +96,21 @@ static inline int of_device_uevent_modalias(struct 
>> device *dev,
>>       return -ENODEV;
>>   }
>>   +static int of_device_links_add(struct device *consumer)
>> +{
>> +    return 0;
>> +}
>> +
>> +static int of_device_links_remove(struct device *consumer)
>> +{
>> +    return 0;
>> +}
>> +
>> +static int devm_of_device_links_add(struct device *consumer)
>> +{
>> +    return 0;
>> +}
>> +
>>   static inline void of_device_node_put(struct device *dev) { }
>>     static inline const struct of_device_id *__of_match_device(
>
>

[PATCH 2/5] Input: edt-ft5x06: Document links-add property

[Benjamin Gaignard]

Explain the purpose of links_add property.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
index 870b8c5cce9b..38b84d2b32e6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.txt
@@ -60,6 +60,8 @@ Optional properties:
  - touchscreen-inverted-x  : See touchscreen.txt
  - touchscreen-inverted-y  : See touchscreen.txt
  - touchscreen-swapped-x-y : See touchscreen.txt
+ - links-add		: List of suppliers handles, suppliers will be
+			  suspended after touchscreen device and resume before it.
 
 Example:
 	polytouch: edt-ft5x06@38 {
-- 
2.15.0

[PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links

[Benjamin Gaignard]

From: Yannick Fertré <yannick.fertre@st.com>

Add a call to devm_of_device_links_add() to create links with suppliers
at probe time.

Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/input/touchscreen/edt-ft5x06.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
index 702bfda7ee77..ac9f7e85efb0 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -1167,6 +1167,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, tsdata);
 
+	devm_of_device_links_add(&client->dev);
+
 	irq_flags = irq_get_trigger_type(client->irq);
 	if (irq_flags == IRQF_TRIGGER_NONE)
 		irq_flags = IRQF_TRIGGER_FALLING;
-- 
2.15.0

Re: [PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links

[Dmitry Torokhov]

Hi Benjamin,

On Wed, Apr 24, 2019 at 12:19:11PM +0200, Benjamin Gaignard wrote:
> From: Yannick Fertré <yannick.fertre@st.com>
> 
> Add a call to devm_of_device_links_add() to create links with suppliers
> at probe time.
> 
> Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> ---
>  drivers/input/touchscreen/edt-ft5x06.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
> index 702bfda7ee77..ac9f7e85efb0 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -1167,6 +1167,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, tsdata);
>  
> +	devm_of_device_links_add(&client->dev);
> +

This seems pretty generic action and I believe it should be done in
generic code, either bus (i2c, spi, etc), or in device core.

Thanks.

-- 
Dmitry

Re: [PATCH 3/5] input: edt-ft5x06 - Call devm_of_device_links_add() to create links

[Benjamin GAIGNARD]

On 4/25/19 12:52 AM, Dmitry Torokhov wrote:
> Hi Benjamin,
>
> On Wed, Apr 24, 2019 at 12:19:11PM +0200, Benjamin Gaignard wrote:
>> From: Yannick Fertré <yannick.fertre@st.com>
>>
>> Add a call to devm_of_device_links_add() to create links with suppliers
>> at probe time.
>>
>> Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
>> ---
>>   drivers/input/touchscreen/edt-ft5x06.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/input/touchscreen/edt-ft5x06.c b/drivers/input/touchscreen/edt-ft5x06.c
>> index 702bfda7ee77..ac9f7e85efb0 100644
>> --- a/drivers/input/touchscreen/edt-ft5x06.c
>> +++ b/drivers/input/touchscreen/edt-ft5x06.c
>> @@ -1167,6 +1167,8 @@ static int edt_ft5x06_ts_probe(struct i2c_client *client,
>>   
>>   	i2c_set_clientdata(client, tsdata);
>>   
>> +	devm_of_device_links_add(&client->dev);
>> +
> This seems pretty generic action and I believe it should be done in
> generic code, either bus (i2c, spi, etc), or in device core.
It is done (or could be done) in most of the buses or framework (like 
for regulator)
but I think that adding it in device core add a device-tree node parsing 
for all the
drivers while only a few of them will really need this feature. That why 
I have put the
call here.

Benjamin
>
> Thanks.
>

[PATCH 4/5] Input: goodix: Document links-add property

[Benjamin Gaignard]

Explain the purpose of links_add property.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 8cf0b4d38a7e..0447151608c6 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -24,6 +24,8 @@ Optional properties:
  - touchscreen-size-x
  - touchscreen-size-y
  - touchscreen-swapped-x-y
+ - links-add		: List of suppliers handles, suppliers will be
+			  suspended after goodix device and resume before it.
 
 The touchscreen-* properties are documented in touchscreen.txt in this
 directory.
-- 
2.15.0

[PATCH 5/5] input: goodix - Call devm_of_device_links_add() to create links

[Benjamin Gaignard]

From: Yannick Fertré <yannick.fertre@st.com>

Add a call to devm_of_device_links_add() to create links with suppliers
at probe time.

Signed-off-by: Yannick Fertré <yannick.fertre@st.com>
Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
---
 drivers/input/touchscreen/goodix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f57d82220a88..9aefbfa39319 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -30,6 +30,7 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <asm/unaligned.h>
 
 struct goodix_ts_data;
@@ -812,6 +813,8 @@ static int goodix_ts_probe(struct i2c_client *client,
 
 	ts->chip = goodix_get_chip_data(ts->id);
 
+	devm_of_device_links_add(&client->dev);
+
 	if (ts->gpiod_int && ts->gpiod_rst) {
 		/* update device config */
 		ts->cfg_name = devm_kasprintf(&client->dev, GFP_KERNEL,
-- 
2.15.0

Re: [PATCH 0/5] Add of_ functions for device_link_add()

[Rob Herring]

On Wed, Apr 24, 2019 at 5:19 AM Benjamin Gaignard
<benjamin.gaignard@st.com> wrote:
>
> It could happen that we need to control suspend/resume ordering between
> devices without obvious consumer/supplier link. For example when touchscreens
> and DSI panels share the same reset line, in this case we need to be sure
> of pm_runtime operations ordering between those two devices to correctly
> perform reset.
> DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
> I2C client and I2C bus or regulator client and regulator provider) so we need
> to describe this in device-tree.

Needing to know which touchscreen is attached to a panel could be
important to describe if you have multiple displays.

Doesn't the reset subsystem already have some support for shared
resets? Seems like it could provide clients with struct device or
device_node ptrs to other devices sharing a reset.

>
> This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
> helpers to find and parse 'links-add' property in a device-tree node.

Going to document that property somewhere? :)

I think this is too generic and coupled to Linux. It doesn't have any
information as to what is the dependency or connection nor what the
direction of the dependency is.

I'm not convinced we need to solve this generically vs. defining
something for this specific example.

Rob

Re: [PATCH 0/5] Add of_ functions for device_link_add()

[Dmitry Torokhov]

On Thu, Apr 25, 2019 at 11:08 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Apr 24, 2019 at 5:19 AM Benjamin Gaignard
> <benjamin.gaignard@st.com> wrote:
> >
> > It could happen that we need to control suspend/resume ordering between
> > devices without obvious consumer/supplier link. For example when touchscreens
> > and DSI panels share the same reset line, in this case we need to be sure
> > of pm_runtime operations ordering between those two devices to correctly
> > perform reset.
> > DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
> > I2C client and I2C bus or regulator client and regulator provider) so we need
> > to describe this in device-tree.
>
> Needing to know which touchscreen is attached to a panel could be
> important to describe if you have multiple displays.
>
> Doesn't the reset subsystem already have some support for shared
> resets? Seems like it could provide clients with struct device or
> device_node ptrs to other devices sharing a reset.
>
> >
> > This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
> > helpers to find and parse 'links-add' property in a device-tree node.
>
> Going to document that property somewhere? :)
>
> I think this is too generic and coupled to Linux. It doesn't have any
> information as to what is the dependency or connection nor what the
> direction of the dependency is.
>
> I'm not convinced we need to solve this generically vs. defining
> something for this specific example.

I am pretty sure there will be more drivers needing complex
dependencies. Doesn't ACPI allow defining relationship between devices
that goes beyond the tree structure?

Thanks.

-- 
Dmitry

Re: [PATCH 0/5] Add of_ functions for device_link_add()

[Rob Herring]

On Thu, Apr 25, 2019 at 2:24 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Apr 25, 2019 at 11:08 AM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Wed, Apr 24, 2019 at 5:19 AM Benjamin Gaignard
> > <benjamin.gaignard@st.com> wrote:
> > >
> > > It could happen that we need to control suspend/resume ordering between
> > > devices without obvious consumer/supplier link. For example when touchscreens
> > > and DSI panels share the same reset line, in this case we need to be sure
> > > of pm_runtime operations ordering between those two devices to correctly
> > > perform reset.
> > > DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
> > > I2C client and I2C bus or regulator client and regulator provider) so we need
> > > to describe this in device-tree.
> >
> > Needing to know which touchscreen is attached to a panel could be
> > important to describe if you have multiple displays.
> >
> > Doesn't the reset subsystem already have some support for shared
> > resets? Seems like it could provide clients with struct device or
> > device_node ptrs to other devices sharing a reset.
> >
> > >
> > > This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
> > > helpers to find and parse 'links-add' property in a device-tree node.
> >
> > Going to document that property somewhere? :)
> >
> > I think this is too generic and coupled to Linux. It doesn't have any
> > information as to what is the dependency or connection nor what the
> > direction of the dependency is.
> >
> > I'm not convinced we need to solve this generically vs. defining
> > something for this specific example.
>
> I am pretty sure there will be more drivers needing complex
> dependencies. Doesn't ACPI allow defining relationship between devices
> that goes beyond the tree structure?

Can't speak to ACPI, but I assume you where implying ACPI supports
this so DT should too.

Almost every binding we have is defining relationships between
devices. Interrupts, clocks, gpio, pinctrl, etc. all do this. To use a
similar example to the one here, we already define the relationship
between a display and a backlight with a 'backlight' property in the
display node. Why should touchscreen be any different than backlight?

What really concerns me here is folks just add relationships to
'links-add' which are already captured in DT (such as backlight) just
because they'll get it for free and not have to go add support to
handle each specific binding.

Rob

Re: [PATCH 0/5] Add of_ functions for device_link_add()

[Benjamin GAIGNARD]

On 4/26/19 1:02 AM, Rob Herring wrote:
> On Thu, Apr 25, 2019 at 2:24 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Thu, Apr 25, 2019 at 11:08 AM Rob Herring <robh+dt@kernel.org> wrote:
>>> On Wed, Apr 24, 2019 at 5:19 AM Benjamin Gaignard
>>> <benjamin.gaignard@st.com> wrote:
>>>> It could happen that we need to control suspend/resume ordering between
>>>> devices without obvious consumer/supplier link. For example when touchscreens
>>>> and DSI panels share the same reset line, in this case we need to be sure
>>>> of pm_runtime operations ordering between those two devices to correctly
>>>> perform reset.
>>>> DSI panel and touchscreen aren't sharing any heriachical relationship (unlike
>>>> I2C client and I2C bus or regulator client and regulator provider) so we need
>>>> to describe this in device-tree.
>>> Needing to know which touchscreen is attached to a panel could be
>>> important to describe if you have multiple displays.
>>>
>>> Doesn't the reset subsystem already have some support for shared
>>> resets? Seems like it could provide clients with struct device or
>>> device_node ptrs to other devices sharing a reset.
Sharing reset will not help here because we don't want to have two reset
occuring in the same line but only one when the both devices are resumed.
That why we to force suspend/resume ordering and so add a link between the
devices.
>>>
>>>> This series introduce of_device_links_{add,remove} and devm_of_device_links_add()
>>>> helpers to find and parse 'links-add' property in a device-tree node.
>>> Going to document that property somewhere? :)
I have put a description of this property in each device bindings but if 
you can tell me
where to put it, I will follow your advice.
>>>
>>> I think this is too generic and coupled to Linux. It doesn't have any
>>> information as to what is the dependency or connection nor what the
>>> direction of the dependency is.
Could something like 'link-suppliers' or 'pm-runtime-dependencies'
are more explicit property name to describe direction, goal, and connection
between consumer and supplier ?
>>>
>>> I'm not convinced we need to solve this generically vs. defining
>>> something for this specific example.
>> I am pretty sure there will be more drivers needing complex
>> dependencies. Doesn't ACPI allow defining relationship between devices
>> that goes beyond the tree structure?
> Can't speak to ACPI, but I assume you where implying ACPI supports
> this so DT should too.
>
> Almost every binding we have is defining relationships between
> devices. Interrupts, clocks, gpio, pinctrl, etc. all do this. To use a
> similar example to the one here, we already define the relationship
> between a display and a backlight with a 'backlight' property in the
> display node. Why should touchscreen be any different than backlight?
It is different because it is only about suspend/resume ordering.
There is no need for a panel to knows about touchscreen unlike
than backlight that it could drive.
I have the same need to order suspend/resume operations between
GPU and display to be sure that GPU is suspend before display and resume
after it.

>
> What really concerns me here is folks just add relationships to
> 'links-add' which are already captured in DT (such as backlight) just
> because they'll get it for free and not have to go add support to
> handle each specific binding.
It won't be for free since I don't want to put it in device core so each 
driver
will have to add a call to a function to enable it and will have to 
explain why doing it.

Benjamin

>
> Rob