Currently, the various display bridge drivers rely on OF infrastructures
to works very well, yet there are platforms and/or devices absence of 'OF'
support. Such as virtual display drivers, USB display apapters and ACPI
based systems etc.

Add fwnode based helpers to fill the niche, this allows part of the display
bridge drivers to work across systems. As the fwnode API has wider coverage
than DT counterpart and the fwnode graphs are compatible with the OF graph,
so the provided helpers can be used on all systems in theory. Assumed that
the system has valid fwnode graphs established before drm bridge drivers
are probed, and there has fwnode assigned to involved drm bridge instance.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/drm_bridge.c | 74 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     | 16 ++++++++
 2 files changed, 90 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 28abe9aa99ca..fc1a314140e7 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1368,6 +1368,80 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 EXPORT_SYMBOL(of_drm_find_bridge);
 #endif
 
+/**
+ * drm_bridge_find_by_fwnode - Find the bridge corresponding to the fwnode
+ *
+ * @fwnode: fwnode for which to find the matching drm_bridge
+ *
+ * This function looks up a drm_bridge in the global bridge list, based on
+ * its associated fwnode. Drivers who want to use this function should has
+ * fwnode handle assigned to the fwnode member of the struct drm_bridge
+ * instance.
+ *
+ * Returns:
+ *  * A reference to the requested drm_bridge object on success, or
+ *  * %NULL otherwise (object does not exist or the driver of requested
+ *    bridge not probed yet).
+ */
+struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode)
+{
+	struct drm_bridge *ret = NULL;
+	struct drm_bridge *bridge;
+
+	if (!fwnode)
+		return NULL;
+
+	mutex_lock(&bridge_lock);
+
+	list_for_each_entry(bridge, &bridge_list, list) {
+		if (bridge->fwnode == fwnode) {
+			ret = bridge;
+			break;
+		}
+	}
+
+	mutex_unlock(&bridge_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drm_bridge_find_by_fwnode);
+
+/**
+ * drm_bridge_find_next_bridge_by_fwnode - Find the next bridge by fwnode
+ * @fwnode: fwnode pointer to the current device.
+ * @port: identifier of the port node of the next bridge is connected.
+ *
+ * This function find the next bridge at the current node, it assumed that
+ * there has valid fwnode graph established.
+ *
+ * Returns:
+ *  * A reference to the requested drm_bridge object on success, or
+ *  * -%EINVAL or -%ENODEV if the fwnode graph or OF graph is not complete, or
+ *  * %NULL if object does not exist or the next bridge is not probed yet.
+ */
+struct drm_bridge *
+drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port)
+{
+	struct drm_bridge *bridge;
+	struct fwnode_handle *ep;
+	struct fwnode_handle *remote;
+
+	ep = fwnode_graph_get_endpoint_by_id(fwnode, port, 0, 0);
+	if (!ep)
+		return ERR_PTR(-EINVAL);
+
+	remote = fwnode_graph_get_remote_port_parent(ep);
+	fwnode_handle_put(ep);
+	if (!remote)
+		return ERR_PTR(-ENODEV);
+
+	bridge = drm_bridge_find_by_fwnode(remote);
+	fwnode_handle_put(remote);
+
+	return bridge;
+}
+EXPORT_SYMBOL_GPL(drm_bridge_find_next_bridge_by_fwnode);
+
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
 MODULE_DESCRIPTION("DRM bridge infrastructure");
 MODULE_LICENSE("GPL and additional rights");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..a3f5d12a308c 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -26,6 +26,7 @@
 #include <linux/ctype.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_encoder.h>
@@ -721,6 +722,8 @@ struct drm_bridge {
 	struct list_head chain_node;
 	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
+	/** @fwnode: fwnode pointer to the bridge */
+	struct fwnode_handle *fwnode;
 	/** @list: to keep track of all added bridges */
 	struct list_head list;
 	/**
@@ -788,6 +791,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous,
 		      enum drm_bridge_attach_flags flags);
 
+static inline void
+drm_bridge_set_node(struct drm_bridge *bridge, struct fwnode_handle *fwnode)
+{
+	bridge->fwnode = fwnode;
+	bridge->of_node = to_of_node(fwnode);
+}
+
 #ifdef CONFIG_OF
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 #else
@@ -797,6 +807,12 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 }
 #endif
 
+struct drm_bridge *
+drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode);
+
+struct drm_bridge *
+drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port);
+
 /**
  * drm_bridge_get_next_bridge() - Get the next bridge in the chain
  * @bridge: bridge object
-- 
2.34.1

On Tue, Apr 23, 2024 at 03:18:55AM +0800, Sui Jingfeng wrote:
> Currently, the various display bridge drivers rely on OF infrastructures
> to works very well, yet there are platforms and/or devices absence of 'OF'
> support. Such as virtual display drivers, USB display apapters and ACPI
> based systems etc.
> 
> Add fwnode based helpers to fill the niche, this allows part of the display
> bridge drivers to work across systems. As the fwnode API has wider coverage
> than DT counterpart and the fwnode graphs are compatible with the OF graph,
> so the provided helpers can be used on all systems in theory. Assumed that
> the system has valid fwnode graphs established before drm bridge drivers
> are probed, and there has fwnode assigned to involved drm bridge instance.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/drm_bridge.c | 74 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     | 16 ++++++++
>  2 files changed, 90 insertions(+)
> 

[skipped]

> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4baca0d9107b..a3f5d12a308c 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -26,6 +26,7 @@
>  #include <linux/ctype.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_encoder.h>
> @@ -721,6 +722,8 @@ struct drm_bridge {
>  	struct list_head chain_node;
>  	/** @of_node: device node pointer to the bridge */
>  	struct device_node *of_node;
> +	/** @fwnode: fwnode pointer to the bridge */
> +	struct fwnode_handle *fwnode;

My comment is still the same: plese replace of_node with fwnode. It is
more intrusive, however it will lower the possible confusion if the
driver sets both of_node and fwnode. Also it will remove the necessity
for helpers like drm_bridge_set_node().

>  	/** @list: to keep track of all added bridges */
>  	struct list_head list;
>  	/**
> @@ -788,6 +791,13 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous,
>  		      enum drm_bridge_attach_flags flags);
>  
> +static inline void
> +drm_bridge_set_node(struct drm_bridge *bridge, struct fwnode_handle *fwnode)
> +{
> +	bridge->fwnode = fwnode;
> +	bridge->of_node = to_of_node(fwnode);
> +}
> +
>  #ifdef CONFIG_OF
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  #else
> @@ -797,6 +807,12 @@ static inline struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  }
>  #endif
>  
> +struct drm_bridge *
> +drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode);
> +
> +struct drm_bridge *
> +drm_bridge_find_next_bridge_by_fwnode(struct fwnode_handle *fwnode, u32 port);
> +
>  /**
>   * drm_bridge_get_next_bridge() - Get the next bridge in the chain
>   * @bridge: bridge object
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Hi,


On 2024/4/23 03:51, Dmitry Baryshkov wrote:
> On Tue, Apr 23, 2024 at 03:18:55AM +0800, Sui Jingfeng wrote:
>> Currently, the various display bridge drivers rely on OF infrastructures
>> to works very well, yet there are platforms and/or devices absence of 'OF'
>> support. Such as virtual display drivers, USB display apapters and ACPI
>> based systems etc.
>>
>> Add fwnode based helpers to fill the niche, this allows part of the display
>> bridge drivers to work across systems. As the fwnode API has wider coverage
>> than DT counterpart and the fwnode graphs are compatible with the OF graph,
>> so the provided helpers can be used on all systems in theory. Assumed that
>> the system has valid fwnode graphs established before drm bridge drivers
>> are probed, and there has fwnode assigned to involved drm bridge instance.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/drm_bridge.c | 74 ++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_bridge.h     | 16 ++++++++
>>   2 files changed, 90 insertions(+)
>>
> [skipped]
>
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 4baca0d9107b..a3f5d12a308c 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -26,6 +26,7 @@
>>   #include <linux/ctype.h>
>>   #include <linux/list.h>
>>   #include <linux/mutex.h>
>> +#include <linux/of.h>
>>   
>>   #include <drm/drm_atomic.h>
>>   #include <drm/drm_encoder.h>
>> @@ -721,6 +722,8 @@ struct drm_bridge {
>>   	struct list_head chain_node;
>>   	/** @of_node: device node pointer to the bridge */
>>   	struct device_node *of_node;
>> +	/** @fwnode: fwnode pointer to the bridge */
>> +	struct fwnode_handle *fwnode;
> My comment is still the same: plese replace of_node with fwnode.

s/plese/please


Unless you can guarantee that *all* maintainers agree(welcome) with
the code changes involved by your proposal. Otherwise I'm going to
respect the domain specific maintainers to keep the code base as it
is.

I need the agreement of all other maintainers involved before I
could take any further action. I'm asking because I need to make sure
that such changes is what *everybody* wanted. As I have to respect
to respective maintainers(such as Daniel, Thomas, Maxime, Laurent
and all other maintainers of the drm miscellaneous).


>   It is more intrusive,

It is not only intrusive, but also annoying.

> however it will lower the possible confusion if the
> driver sets both of_node and fwnode.

The of_node and the fwnode can point to different thing, the potential
reason it the situation of them is not symmetrical.

  - On non-DT environment the of_node can point to NULL.
  - The reverse is also true, that is on DT environment the fwnode can also point to NULL
    if specific subsystem is not going to use it.
  - And USB display adapter can be using at any arch in theory, it can use both of them, or
    one of themm or neither of them.
  

This is a extremely flexible design, it's toward to future and also works with legacy.
So what's the confusion is?


> Also it will remove the necessity for helpers like drm_bridge_set_node().


Thedrm_bridge_set_node() is just a mimic to the device_set_node(), the 
struct device contains both of_node and fwnode as its data members.
I didn't see anyone complains about it, am I fail to understand something?


Or, let's put it straightforward, I'm going to follow your idea
if you could remove the of_node data member from the struct device.
Do you have the ability?


-- 
Best regards,
Sui