[PATCH 0/5] drm/bridge: Allow using fwnode API to get the next bridge

[PATCH 0/5] drm/bridge: Allow using fwnode API to get the next bridge

[Sui Jingfeng]

Make it possible to use drm-bridge drivers on non-DT based systems.

Sui Jingfeng (5):
  drm/bridge: Add drm_bridge_find_by_fwnode() helper
  drm/bridge: simple-bridge: Extend match support for non-DT based
    systems
  drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode
    API
  drm/bridge: display-connector: Extend match support for non-DT based
    systems
  drm-bridge: display-connector: Switch to use fwnode API

 drivers/gpu/drm/bridge/display-connector.c | 46 +++++++++----
 drivers/gpu/drm/bridge/simple-bridge.c     | 75 +++++++++++++++++++---
 drivers/gpu/drm/drm_bridge.c               | 33 ++++++++++
 include/drm/drm_bridge.h                   |  4 ++
 4 files changed, 139 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH 1/5] drm/bridge: Add drm_bridge_find_by_fwnode() helper

[Sui Jingfeng]

Because ACPI based systems only has the fwnode associated, the of_node
member of struct device is NULL. To order to move things forward, we add
drm_bridge_find_by_fwnode() to extend the support.

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

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cee3188adf3d..ffd969adc2fb 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -1347,6 +1347,39 @@ 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 associated fwnode
+ *
+ * @fwnode: fwnode for which to find the matching drm_bridge
+ *
+ * This function looks up a drm_bridge based on its associated fwnode.
+ *
+ * RETURNS:
+ * A reference to the drm_bridge control structure if found, NULL on failure.
+ */
+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(drm_bridge_find_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 e39da5807ba7..fe3d5f4bf37f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -720,6 +720,8 @@ struct drm_bridge {
 	struct list_head chain_node;
 	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
+	/** @fwnode: associated fwnode supplied by platform firmware */
+	struct fwnode_handle *fwnode;
 	/** @list: to keep track of all added bridges */
 	struct list_head list;
 	/**
@@ -796,6 +798,8 @@ 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);
+
 /**
  * drm_bridge_get_next_bridge() - Get the next bridge in the chain
  * @bridge: bridge object
-- 
2.25.1

[PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems

[Sui Jingfeng]

Which is intended to be used on non-DT environment, where the simple-bridge
platform device is created by either the display controller driver side or
platform firmware subsystem. To avoid duplication and to keep consistent,
we choose to reuse the OF match tables. Because the potentional user may
not has a of_node attached, nor a ACPI match id. If this is the case,
a software node string property can be provide to fill the niche.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/simple-bridge.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index cbe8e778d7c7..595f672745b9 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -166,6 +166,24 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
 	.disable	= simple_bridge_disable,
 };
 
+static const void *simple_bridge_get_match_data(const struct device *dev)
+{
+	const struct of_device_id *matches = dev->driver->of_match_table;
+
+	/* Try to get the match data by software node */
+	while (matches) {
+		if (!matches->compatible[0])
+			break;
+
+		if (device_is_compatible(dev, matches->compatible))
+			return matches->data;
+
+		matches++;
+	}
+
+	return NULL;
+}
+
 static int simple_bridge_probe(struct platform_device *pdev)
 {
 	struct simple_bridge *sbridge;
@@ -176,7 +194,10 @@ static int simple_bridge_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, sbridge);
 
-	sbridge->info = of_device_get_match_data(&pdev->dev);
+	if (pdev->dev.of_node)
+		sbridge->info = of_device_get_match_data(&pdev->dev);
+	else
+		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
 
 	/* Get the next bridge in the pipeline. */
 	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
@@ -309,3 +330,4 @@ module_platform_driver(simple_bridge_driver);
 MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
 MODULE_DESCRIPTION("Simple DRM bridge driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:simple-bridge");
-- 
2.25.1

[PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API

[Sui Jingfeng]

Which make it possible to use this driver on non-DT based systems,
meanwhile, made no functional changes for DT based systems.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/simple-bridge.c | 51 ++++++++++++++++++++++----
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index 595f672745b9..cfea5a67cc5b 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const struct device *dev)
 	return NULL;
 }
 
+static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
+						   struct drm_bridge **next_bridge)
+{
+	struct drm_bridge *bridge;
+	struct fwnode_handle *ep;
+	struct fwnode_handle *remote;
+
+	ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
+	if (!ep) {
+		dev_err(dev, "The endpoint is unconnected\n");
+		return -EINVAL;
+	}
+
+	remote = fwnode_graph_get_remote_port_parent(ep);
+	fwnode_handle_put(ep);
+	if (!remote) {
+		dev_err(dev, "No valid remote node\n");
+		return -ENODEV;
+	}
+
+	bridge = drm_bridge_find_by_fwnode(remote);
+	fwnode_handle_put(remote);
+
+	if (!bridge) {
+		dev_warn(dev, "Next bridge not found, deferring probe\n");
+		return -EPROBE_DEFER;
+	}
+
+	*next_bridge = bridge;
+
+	return 0;
+}
+
 static int simple_bridge_probe(struct platform_device *pdev)
 {
 	struct simple_bridge *sbridge;
@@ -199,14 +232,17 @@ static int simple_bridge_probe(struct platform_device *pdev)
 	else
 		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
 
-	/* Get the next bridge in the pipeline. */
-	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
-	if (!remote)
-		return -EINVAL;
-
-	sbridge->next_bridge = of_drm_find_bridge(remote);
-	of_node_put(remote);
+	if (pdev->dev.of_node) {
+		/* Get the next bridge in the pipeline. */
+		remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
+		if (!remote)
+			return -EINVAL;
 
+		sbridge->next_bridge = of_drm_find_bridge(remote);
+		of_node_put(remote);
+	} else {
+		simple_bridge_get_next_bridge_by_fwnode(&pdev->dev, &sbridge->next_bridge);
+	}
 	if (!sbridge->next_bridge) {
 		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
@@ -231,6 +267,7 @@ static int simple_bridge_probe(struct platform_device *pdev)
 	/* Register the bridge. */
 	sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
 	sbridge->bridge.of_node = pdev->dev.of_node;
+	sbridge->bridge.fwnode = pdev->dev.fwnode;
 	sbridge->bridge.timings = sbridge->info->timings;
 
 	drm_bridge_add(&sbridge->bridge);
-- 
2.25.1

[PATCH 4/5] drm/bridge: display-connector: Extend match support for non-DT based systems

[Sui Jingfeng]

On which case the driver is not probed by OF, Instead, a fwnode is
associated to the platform device before this driver is probed. The newly
added code is intended to be used on non-DT environment. It is assumed
that there is a string fwnode property associated with the platform device,
the name of the string property is compatible, the value of the string
property is used to get platform match data.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/display-connector.c | 24 +++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index 08bd5695ddae..eb7e194e7735 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -202,6 +202,24 @@ static int display_connector_get_supply(struct platform_device *pdev,
 	return PTR_ERR_OR_ZERO(conn->supply);
 }
 
+static const void *display_connector_get_match_data(const struct device *dev)
+{
+	const struct of_device_id *matches = dev->driver->of_match_table;
+
+	/* Try to get the match data by software node */
+	while (matches) {
+		if (!matches->compatible[0])
+			break;
+
+		if (device_is_compatible(dev, matches->compatible))
+			return matches->data;
+
+		matches++;
+	}
+
+	return NULL;
+}
+
 static int display_connector_probe(struct platform_device *pdev)
 {
 	struct display_connector *conn;
@@ -215,7 +233,10 @@ static int display_connector_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, conn);
 
-	type = (uintptr_t)of_device_get_match_data(&pdev->dev);
+	if (pdev->dev.of_node)
+		type = (uintptr_t)of_device_get_match_data(&pdev->dev);
+	else
+		type = (uintptr_t)display_connector_get_match_data(&pdev->dev);
 
 	/* Get the exact connector type. */
 	switch (type) {
@@ -434,3 +455,4 @@ module_platform_driver(display_connector_driver);
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
 MODULE_DESCRIPTION("Display connector driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:display-connector");
-- 
2.25.1

[PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API

[Sui Jingfeng]

From: Sui Jingfeng <suijingfeng@loongson.cn>

Because API has wider coverage, it can be used on non-DT systems as well.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/bridge/display-connector.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index eb7e194e7735..2c3e54a458e8 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -243,8 +243,8 @@ static int display_connector_probe(struct platform_device *pdev)
 	case DRM_MODE_CONNECTOR_DVII: {
 		bool analog, digital;
 
-		analog = of_property_read_bool(pdev->dev.of_node, "analog");
-		digital = of_property_read_bool(pdev->dev.of_node, "digital");
+		analog = fwnode_property_present(pdev->dev.fwnode, "analog");
+		digital = fwnode_property_present(pdev->dev.fwnode, "digital");
 		if (analog && !digital) {
 			conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
 		} else if (!analog && digital) {
@@ -261,8 +261,8 @@ static int display_connector_probe(struct platform_device *pdev)
 	case DRM_MODE_CONNECTOR_HDMIA: {
 		const char *hdmi_type;
 
-		ret = of_property_read_string(pdev->dev.of_node, "type",
-					      &hdmi_type);
+		ret = fwnode_property_read_string(pdev->dev.fwnode, "type",
+						  &hdmi_type);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "HDMI connector with no type\n");
 			return -EINVAL;
@@ -292,7 +292,7 @@ static int display_connector_probe(struct platform_device *pdev)
 	conn->bridge.interlace_allowed = true;
 
 	/* Get the optional connector label. */
-	of_property_read_string(pdev->dev.of_node, "label", &label);
+	fwnode_property_read_string(pdev->dev.fwnode, "label", &label);
 
 	/*
 	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
@@ -330,12 +330,13 @@ static int display_connector_probe(struct platform_device *pdev)
 	if (type == DRM_MODE_CONNECTOR_DVII ||
 	    type == DRM_MODE_CONNECTOR_HDMIA ||
 	    type == DRM_MODE_CONNECTOR_VGA) {
-		struct device_node *phandle;
+		struct fwnode_handle *fwnode;
 
-		phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
-		if (phandle) {
-			conn->bridge.ddc = of_get_i2c_adapter_by_node(phandle);
-			of_node_put(phandle);
+		fwnode = fwnode_find_reference(pdev->dev.fwnode, "ddc-i2c-bus", 0);
+		if (!IS_ERR_OR_NULL(fwnode)) {
+			dev_info(&pdev->dev, "has I2C bus property\n");
+			conn->bridge.ddc = i2c_get_adapter_by_fwnode(fwnode);
+			fwnode_handle_put(fwnode);
 			if (!conn->bridge.ddc)
 				return -EPROBE_DEFER;
 		} else {
@@ -380,6 +381,7 @@ static int display_connector_probe(struct platform_device *pdev)
 
 	conn->bridge.funcs = &display_connector_bridge_funcs;
 	conn->bridge.of_node = pdev->dev.of_node;
+	conn->bridge.fwnode = pdev->dev.fwnode;
 
 	if (conn->bridge.ddc)
 		conn->bridge.ops |= DRM_BRIDGE_OP_EDID
-- 
2.25.1

Re: [PATCH 1/5] drm/bridge: Add drm_bridge_find_by_fwnode() helper

[Laurent Pinchart]

Hi Sui,

Thank you for the patch.

On Tue, Jan 23, 2024 at 12:32:16AM +0800, Sui Jingfeng wrote:
> Because ACPI based systems only has the fwnode associated, the of_node
> member of struct device is NULL. To order to move things forward, we add
> drm_bridge_find_by_fwnode() to extend the support.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>

Could we switch completely to fwnode, instead of maintaining the fwnode
and OF options side-by-side ?

> ---
>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  4 ++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cee3188adf3d..ffd969adc2fb 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -1347,6 +1347,39 @@ 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 associated fwnode
> + *
> + * @fwnode: fwnode for which to find the matching drm_bridge
> + *
> + * This function looks up a drm_bridge based on its associated fwnode.
> + *
> + * RETURNS:
> + * A reference to the drm_bridge control structure if found, NULL on failure.
> + */
> +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(drm_bridge_find_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 e39da5807ba7..fe3d5f4bf37f 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -720,6 +720,8 @@ struct drm_bridge {
>  	struct list_head chain_node;
>  	/** @of_node: device node pointer to the bridge */
>  	struct device_node *of_node;
> +	/** @fwnode: associated fwnode supplied by platform firmware */
> +	struct fwnode_handle *fwnode;
>  	/** @list: to keep track of all added bridges */
>  	struct list_head list;
>  	/**
> @@ -796,6 +798,8 @@ 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);
> +
>  /**
>   * drm_bridge_get_next_bridge() - Get the next bridge in the chain
>   * @bridge: bridge object

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 12:32:18AM +0800, Sui Jingfeng wrote:
> Which make it possible to use this driver on non-DT based systems,
> meanwhile, made no functional changes for DT based systems.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 51 ++++++++++++++++++++++----
>  1 file changed, 44 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index 595f672745b9..cfea5a67cc5b 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const struct device *dev)
>  	return NULL;
>  }
>  
> +static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
> +						   struct drm_bridge **next_bridge)
> +{
> +	struct drm_bridge *bridge;
> +	struct fwnode_handle *ep;
> +	struct fwnode_handle *remote;
> +
> +	ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
> +	if (!ep) {
> +		dev_err(dev, "The endpoint is unconnected\n");
> +		return -EINVAL;
> +	}
> +
> +	remote = fwnode_graph_get_remote_port_parent(ep);
> +	fwnode_handle_put(ep);
> +	if (!remote) {
> +		dev_err(dev, "No valid remote node\n");
> +		return -ENODEV;
> +	}
> +
> +	bridge = drm_bridge_find_by_fwnode(remote);
> +	fwnode_handle_put(remote);
> +
> +	if (!bridge) {
> +		dev_warn(dev, "Next bridge not found, deferring probe\n");
> +		return -EPROBE_DEFER;
> +	}
> +
> +	*next_bridge = bridge;
> +
> +	return 0;
> +}
> +

Hmmmm yes, this convinces me further that we should switch to fwnode,
not implement fwnode and OF side-by-side.

>  static int simple_bridge_probe(struct platform_device *pdev)
>  {
>  	struct simple_bridge *sbridge;
> @@ -199,14 +232,17 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  	else
>  		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
>  
> -	/* Get the next bridge in the pipeline. */
> -	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> -	if (!remote)
> -		return -EINVAL;
> -
> -	sbridge->next_bridge = of_drm_find_bridge(remote);
> -	of_node_put(remote);
> +	if (pdev->dev.of_node) {
> +		/* Get the next bridge in the pipeline. */
> +		remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> +		if (!remote)
> +			return -EINVAL;
>  
> +		sbridge->next_bridge = of_drm_find_bridge(remote);
> +		of_node_put(remote);
> +	} else {
> +		simple_bridge_get_next_bridge_by_fwnode(&pdev->dev, &sbridge->next_bridge);
> +	}
>  	if (!sbridge->next_bridge) {
>  		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
>  		return -EPROBE_DEFER;
> @@ -231,6 +267,7 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  	/* Register the bridge. */
>  	sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
>  	sbridge->bridge.of_node = pdev->dev.of_node;
> +	sbridge->bridge.fwnode = pdev->dev.fwnode;
>  	sbridge->bridge.timings = sbridge->info->timings;
>  
>  	drm_bridge_add(&sbridge->bridge);
> -- 
> 2.25.1
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 12:32:20AM +0800, Sui Jingfeng wrote:
> From: Sui Jingfeng <suijingfeng@loongson.cn>
> 
> Because API has wider coverage, it can be used on non-DT systems as well.
> 
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/bridge/display-connector.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index eb7e194e7735..2c3e54a458e8 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -243,8 +243,8 @@ static int display_connector_probe(struct platform_device *pdev)
>  	case DRM_MODE_CONNECTOR_DVII: {
>  		bool analog, digital;
>  
> -		analog = of_property_read_bool(pdev->dev.of_node, "analog");
> -		digital = of_property_read_bool(pdev->dev.of_node, "digital");
> +		analog = fwnode_property_present(pdev->dev.fwnode, "analog");
> +		digital = fwnode_property_present(pdev->dev.fwnode, "digital");
>  		if (analog && !digital) {
>  			conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
>  		} else if (!analog && digital) {
> @@ -261,8 +261,8 @@ static int display_connector_probe(struct platform_device *pdev)
>  	case DRM_MODE_CONNECTOR_HDMIA: {
>  		const char *hdmi_type;
>  
> -		ret = of_property_read_string(pdev->dev.of_node, "type",
> -					      &hdmi_type);
> +		ret = fwnode_property_read_string(pdev->dev.fwnode, "type",
> +						  &hdmi_type);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "HDMI connector with no type\n");
>  			return -EINVAL;
> @@ -292,7 +292,7 @@ static int display_connector_probe(struct platform_device *pdev)
>  	conn->bridge.interlace_allowed = true;
>  
>  	/* Get the optional connector label. */
> -	of_property_read_string(pdev->dev.of_node, "label", &label);
> +	fwnode_property_read_string(pdev->dev.fwnode, "label", &label);
>  
>  	/*
>  	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
> @@ -330,12 +330,13 @@ static int display_connector_probe(struct platform_device *pdev)
>  	if (type == DRM_MODE_CONNECTOR_DVII ||
>  	    type == DRM_MODE_CONNECTOR_HDMIA ||
>  	    type == DRM_MODE_CONNECTOR_VGA) {
> -		struct device_node *phandle;
> +		struct fwnode_handle *fwnode;
>  
> -		phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
> -		if (phandle) {
> -			conn->bridge.ddc = of_get_i2c_adapter_by_node(phandle);
> -			of_node_put(phandle);
> +		fwnode = fwnode_find_reference(pdev->dev.fwnode, "ddc-i2c-bus", 0);
> +		if (!IS_ERR_OR_NULL(fwnode)) {
> +			dev_info(&pdev->dev, "has I2C bus property\n");

This looks like a debugging leftover.

> +			conn->bridge.ddc = i2c_get_adapter_by_fwnode(fwnode);
> +			fwnode_handle_put(fwnode);
>  			if (!conn->bridge.ddc)
>  				return -EPROBE_DEFER;
>  		} else {
> @@ -380,6 +381,7 @@ static int display_connector_probe(struct platform_device *pdev)
>  
>  	conn->bridge.funcs = &display_connector_bridge_funcs;
>  	conn->bridge.of_node = pdev->dev.of_node;
> +	conn->bridge.fwnode = pdev->dev.fwnode;

This goes in the right direction. Let's address the other drivers and
drop the OF-based calls in the same series :-)

>  
>  	if (conn->bridge.ddc)
>  		conn->bridge.ops |= DRM_BRIDGE_OP_EDID

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote:
> Which is intended to be used on non-DT environment, where the simple-bridge
> platform device is created by either the display controller driver side or
> platform firmware subsystem.

Could you give an example of a platform where you intend to use this ?

> To avoid duplication and to keep consistent,
> we choose to reuse the OF match tables. Because the potentional user may
> not has a of_node attached, nor a ACPI match id. If this is the case,
> a software node string property can be provide to fill the niche.

Shouldn't non-DT, non-ACPI platforms use swnodes ?

> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index cbe8e778d7c7..595f672745b9 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -166,6 +166,24 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
>  	.disable	= simple_bridge_disable,
>  };
>  
> +static const void *simple_bridge_get_match_data(const struct device *dev)
> +{
> +	const struct of_device_id *matches = dev->driver->of_match_table;
> +
> +	/* Try to get the match data by software node */
> +	while (matches) {
> +		if (!matches->compatible[0])
> +			break;
> +
> +		if (device_is_compatible(dev, matches->compatible))
> +			return matches->data;
> +
> +		matches++;
> +	}
> +
> +	return NULL;
> +}
> +
>  static int simple_bridge_probe(struct platform_device *pdev)
>  {
>  	struct simple_bridge *sbridge;
> @@ -176,7 +194,10 @@ static int simple_bridge_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, sbridge);
>  
> -	sbridge->info = of_device_get_match_data(&pdev->dev);
> +	if (pdev->dev.of_node)
> +		sbridge->info = of_device_get_match_data(&pdev->dev);
> +	else
> +		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
>  
>  	/* Get the next bridge in the pipeline. */
>  	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> @@ -309,3 +330,4 @@ module_platform_driver(simple_bridge_driver);
>  MODULE_AUTHOR("Maxime Ripard <maxime.ripard@free-electrons.com>");
>  MODULE_DESCRIPTION("Simple DRM bridge driver");
>  MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:simple-bridge");

This is an unrelated change.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] drm/bridge: Add drm_bridge_find_by_fwnode() helper

[Sui Jingfeng]

Hi,


Thanks a lot for your review :-)


On 2024/1/23 09:17, Laurent Pinchart wrote:
> Hi Sui,
>
> Thank you for the patch.
>
> On Tue, Jan 23, 2024 at 12:32:16AM +0800, Sui Jingfeng wrote:
>> Because ACPI based systems only has the fwnode associated, the of_node
>> member of struct device is NULL. To order to move things forward, we add
>> drm_bridge_find_by_fwnode() to extend the support.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> Could we switch completely to fwnode, instead of maintaining the fwnode
> and OF options side-by-side ?


The side-by-side approach allow us to migrate smoothly,
the main consideration is that the OF approach has been
works very well, it is flexible and very successful in
the embedded world.

It seems that the fwnode API could NOT replace the OF
options completely. For example, the'of_device_id' and 'of_match_table' related things are always there, there
are large well-established helpers and subroutines and
already formed as a standard. Some part of it may suffer
from backward compatibility problems.

So I want to leave some space to other programmers.
Maybe there are other programmers who feel that using
OF alone is enough for a specific problem(domain).

Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems

[Sui Jingfeng]

Hi,


On 2024/1/23 09:21, Laurent Pinchart wrote:
> On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote:
>> Which is intended to be used on non-DT environment, where the simple-bridge
>> platform device is created by either the display controller driver side or
>> platform firmware subsystem.
> Could you give an example of a platform where you intend to use this ?


For example:

1) USB based display adapter, such as FL2000DX[1] which use
    the it66121 HDMI transmitter to convert the RGB888 to HDMI.


2) Simple 2D PCIe display controller, such as SM750(EMPV-1201)
    which using sii9022 HDMI transmitter to convert the RGB888
    to HDMI.


3) Some FPGA PCIe Board (sil9136)

4) Be able to run unit test of drm bridges on X86.
  
[1] https://github.com/FrescoLogic/FL2000

Re: [PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API

[Sui Jingfeng]

Hi,


On 2024/1/23 09:18, Laurent Pinchart wrote:
> On Tue, Jan 23, 2024 at 12:32:18AM +0800, Sui Jingfeng wrote:
>> Which make it possible to use this driver on non-DT based systems,
>> meanwhile, made no functional changes for DT based systems.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/bridge/simple-bridge.c | 51 ++++++++++++++++++++++----
>>   1 file changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
>> index 595f672745b9..cfea5a67cc5b 100644
>> --- a/drivers/gpu/drm/bridge/simple-bridge.c
>> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
>> @@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const struct device *dev)
>>   	return NULL;
>>   }
>>   
>> +static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
>> +						   struct drm_bridge **next_bridge)
>> +{
>> +	struct drm_bridge *bridge;
>> +	struct fwnode_handle *ep;
>> +	struct fwnode_handle *remote;
>> +
>> +	ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
>> +	if (!ep) {
>> +		dev_err(dev, "The endpoint is unconnected\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	remote = fwnode_graph_get_remote_port_parent(ep);
>> +	fwnode_handle_put(ep);
>> +	if (!remote) {
>> +		dev_err(dev, "No valid remote node\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	bridge = drm_bridge_find_by_fwnode(remote);
>> +	fwnode_handle_put(remote);
>> +
>> +	if (!bridge) {
>> +		dev_warn(dev, "Next bridge not found, deferring probe\n");
>> +		return -EPROBE_DEFER;
>> +	}
>> +
>> +	*next_bridge = bridge;
>> +
>> +	return 0;
>> +}
>> +
> Hmmmm yes, this convinces me further that we should switch to fwnode,
> not implement fwnode and OF side-by-side.
>

OK, I'm agree with you.


But this means that I have to make the drm_bridge_find_by_fwnode() function works
on both DT systems and non-DT systems. This is also means that we will no longer
need to call of_drm_find_bridge() function anymore. This will eventually lead to
completely remove of_drm_find_bridge()?


As far as I can see, if I follow you suggestion, drm/bridge subsystem will
encountering a *big* refactor. My 'side-by-side' approach allows co-exist.
It is not really meant to purge OF. I feel it is a little bit of aggressive.

hello Maxime, are you watching this? what do you think?


>>   static int simple_bridge_probe(struct platform_device *pdev)
>>   {
>>   	struct simple_bridge *sbridge;
>> @@ -199,14 +232,17 @@ static int simple_bridge_probe(struct platform_device *pdev)
>>   	else
>>   		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
>>   
>> -	/* Get the next bridge in the pipeline. */
>> -	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
>> -	if (!remote)
>> -		return -EINVAL;
>> -
>> -	sbridge->next_bridge = of_drm_find_bridge(remote);
>> -	of_node_put(remote);
>> +	if (pdev->dev.of_node) {
>> +		/* Get the next bridge in the pipeline. */
>> +		remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
>> +		if (!remote)
>> +			return -EINVAL;
>>   
>> +		sbridge->next_bridge = of_drm_find_bridge(remote);
>> +		of_node_put(remote);
>> +	} else {
>> +		simple_bridge_get_next_bridge_by_fwnode(&pdev->dev, &sbridge->next_bridge);
>> +	}
>>   	if (!sbridge->next_bridge) {
>>   		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
>>   		return -EPROBE_DEFER;
>> @@ -231,6 +267,7 @@ static int simple_bridge_probe(struct platform_device *pdev)
>>   	/* Register the bridge. */
>>   	sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
>>   	sbridge->bridge.of_node = pdev->dev.of_node;
>> +	sbridge->bridge.fwnode = pdev->dev.fwnode;
>>   	sbridge->bridge.timings = sbridge->info->timings;
>>   
>>   	drm_bridge_add(&sbridge->bridge);
>> -- 
>> 2.25.1
>>

Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems

[Sui Jingfeng]

Hi,


On 2024/1/23 09:21, Laurent Pinchart wrote:
>>   static int simple_bridge_probe(struct platform_device *pdev)
>>   {
>>   	struct simple_bridge *sbridge;
>> @@ -176,7 +194,10 @@ static int simple_bridge_probe(struct platform_device *pdev)
>>   		return -ENOMEM;
>>   	platform_set_drvdata(pdev, sbridge);
>>   
>> -	sbridge->info = of_device_get_match_data(&pdev->dev);
>> +	if (pdev->dev.of_node)
>> +		sbridge->info = of_device_get_match_data(&pdev->dev);
>> +	else
>> +		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
>>   
>>   	/* Get the next bridge in the pipeline. */
>>   	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
>> @@ -309,3 +330,4 @@ module_platform_driver(simple_bridge_driver);
>>   MODULE_AUTHOR("Maxime Ripard<maxime.ripard@free-electrons.com>");
>>   MODULE_DESCRIPTION("Simple DRM bridge driver");
>>   MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:simple-bridge");
> This is an unrelated change.


Otherwise, this driver will not be probed when compiled as module on non-DT environment.

Re: [PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API

[Sui Jingfeng]

Hi,


On 2024/1/23 09:20, Laurent Pinchart wrote:
> On Tue, Jan 23, 2024 at 12:32:20AM +0800, Sui Jingfeng wrote:
>> From: Sui Jingfeng<suijingfeng@loongson.cn>
>>
>> Because API has wider coverage, it can be used on non-DT systems as well.
>>
>> Signed-off-by: Sui Jingfeng<suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/bridge/display-connector.c | 22 ++++++++++++----------
>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
>> index eb7e194e7735..2c3e54a458e8 100644
>> --- a/drivers/gpu/drm/bridge/display-connector.c
>> +++ b/drivers/gpu/drm/bridge/display-connector.c
>> @@ -243,8 +243,8 @@ static int display_connector_probe(struct platform_device *pdev)
>>   	case DRM_MODE_CONNECTOR_DVII: {
>>   		bool analog, digital;
>>   
>> -		analog = of_property_read_bool(pdev->dev.of_node, "analog");
>> -		digital = of_property_read_bool(pdev->dev.of_node, "digital");
>> +		analog = fwnode_property_present(pdev->dev.fwnode, "analog");
>> +		digital = fwnode_property_present(pdev->dev.fwnode, "digital");
>>   		if (analog && !digital) {
>>   			conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
>>   		} else if (!analog && digital) {
>> @@ -261,8 +261,8 @@ static int display_connector_probe(struct platform_device *pdev)
>>   	case DRM_MODE_CONNECTOR_HDMIA: {
>>   		const char *hdmi_type;
>>   
>> -		ret = of_property_read_string(pdev->dev.of_node, "type",
>> -					      &hdmi_type);
>> +		ret = fwnode_property_read_string(pdev->dev.fwnode, "type",
>> +						  &hdmi_type);
>>   		if (ret < 0) {
>>   			dev_err(&pdev->dev, "HDMI connector with no type\n");
>>   			return -EINVAL;
>> @@ -292,7 +292,7 @@ static int display_connector_probe(struct platform_device *pdev)
>>   	conn->bridge.interlace_allowed = true;
>>   
>>   	/* Get the optional connector label. */
>> -	of_property_read_string(pdev->dev.of_node, "label", &label);
>> +	fwnode_property_read_string(pdev->dev.fwnode, "label", &label);
>>   
>>   	/*
>>   	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
>> @@ -330,12 +330,13 @@ static int display_connector_probe(struct platform_device *pdev)
>>   	if (type == DRM_MODE_CONNECTOR_DVII ||
>>   	    type == DRM_MODE_CONNECTOR_HDMIA ||
>>   	    type == DRM_MODE_CONNECTOR_VGA) {
>> -		struct device_node *phandle;
>> +		struct fwnode_handle *fwnode;
>>   
>> -		phandle = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
>> -		if (phandle) {
>> -			conn->bridge.ddc = of_get_i2c_adapter_by_node(phandle);
>> -			of_node_put(phandle);
>> +		fwnode = fwnode_find_reference(pdev->dev.fwnode, "ddc-i2c-bus", 0);
>> +		if (!IS_ERR_OR_NULL(fwnode)) {
>> +			dev_info(&pdev->dev, "has I2C bus property\n");
> This looks like a debugging leftover.


Yes, thanks a lot for reviewing.
I will pick up suggestions and go back to improve.

Re: [PATCH 1/5] drm/bridge: Add drm_bridge_find_by_fwnode() helper

[Laurent Pinchart]

Hi Sui,

On Tue, Jan 23, 2024 at 04:01:28PM +0800, Sui Jingfeng wrote:
> On 2024/1/23 09:17, Laurent Pinchart wrote:
> > On Tue, Jan 23, 2024 at 12:32:16AM +0800, Sui Jingfeng wrote:
> >> Because ACPI based systems only has the fwnode associated, the of_node
> >> member of struct device is NULL. To order to move things forward, we add
> >> drm_bridge_find_by_fwnode() to extend the support.
> >>
> >> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> >
> > Could we switch completely to fwnode, instead of maintaining the fwnode
> > and OF options side-by-side ?
> 
> The side-by-side approach allow us to migrate smoothly,

But it increases the maintenance burden for the duration of the
migration. I fear the migration would span years, with nobody really
taking active care of it, and the OF and non-OF API will have a risk to
diverge.

> the main consideration is that the OF approach has been
> works very well, it is flexible and very successful in
> the embedded world.

fwnode is a superset of OF, so I don't expect issues switching from OF
to fwnode. For the non-OF, non-fwnode users, that's possibly a different
question.

> It seems that the fwnode API could NOT replace the OF
> options completely. For example, the'of_device_id' and 'of_match_table' related things are always there, there

Yes, and that's not a problem. OF drivers still use of_device_id and
of_match_table, even if they use the fwnode API. No issue there.

> are large well-established helpers and subroutines and
> already formed as a standard. Some part of it may suffer
> from backward compatibility problems.

fwnode has been designed to offer the same API as OF for drivers. If
something is missing, it can be raised with the maintainers.

> So I want to leave some space to other programmers.
> Maybe there are other programmers who feel that using
> OF alone is enough for a specific problem(domain).

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems

[Laurent Pinchart]

On Tue, Jan 23, 2024 at 04:20:04PM +0800, Sui Jingfeng wrote:
> On 2024/1/23 09:21, Laurent Pinchart wrote:
> > On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote:
> >> Which is intended to be used on non-DT environment, where the simple-bridge
> >> platform device is created by either the display controller driver side or
> >> platform firmware subsystem.
> >
> > Could you give an example of a platform where you intend to use this ?
> 
> For example:
> 
> 1) USB based display adapter, such as FL2000DX[1] which use
>     the it66121 HDMI transmitter to convert the RGB888 to HDMI.
> 
> 2) Simple 2D PCIe display controller, such as SM750(EMPV-1201)
>     which using sii9022 HDMI transmitter to convert the RGB888
>     to HDMI.
> 
> 3) Some FPGA PCIe Board (sil9136)
> 
> 4) Be able to run unit test of drm bridges on X86.

Thank you, those are useful examples. It would be nice to capture at
least some of them (first instance the first two) to the commit message.

> [1] https://github.com/FrescoLogic/FL2000

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API

[Laurent Pinchart]

Hello Sui,

On Tue, Jan 23, 2024 at 08:18:22PM +0800, Sui Jingfeng wrote:
> On 2024/1/23 09:18, Laurent Pinchart wrote:
> > On Tue, Jan 23, 2024 at 12:32:18AM +0800, Sui Jingfeng wrote:
> >> Which make it possible to use this driver on non-DT based systems,
> >> meanwhile, made no functional changes for DT based systems.
> >>
> >> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> >> ---
> >>   drivers/gpu/drm/bridge/simple-bridge.c | 51 ++++++++++++++++++++++----
> >>   1 file changed, 44 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> >> index 595f672745b9..cfea5a67cc5b 100644
> >> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> >> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> >> @@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const struct device *dev)
> >>   	return NULL;
> >>   }
> >>   
> >> +static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
> >> +						   struct drm_bridge **next_bridge)
> >> +{
> >> +	struct drm_bridge *bridge;
> >> +	struct fwnode_handle *ep;
> >> +	struct fwnode_handle *remote;
> >> +
> >> +	ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
> >> +	if (!ep) {
> >> +		dev_err(dev, "The endpoint is unconnected\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	remote = fwnode_graph_get_remote_port_parent(ep);
> >> +	fwnode_handle_put(ep);
> >> +	if (!remote) {
> >> +		dev_err(dev, "No valid remote node\n");
> >> +		return -ENODEV;
> >> +	}
> >> +
> >> +	bridge = drm_bridge_find_by_fwnode(remote);
> >> +	fwnode_handle_put(remote);
> >> +
> >> +	if (!bridge) {
> >> +		dev_warn(dev, "Next bridge not found, deferring probe\n");
> >> +		return -EPROBE_DEFER;
> >> +	}
> >> +
> >> +	*next_bridge = bridge;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >
> > Hmmmm yes, this convinces me further that we should switch to fwnode,
> > not implement fwnode and OF side-by-side.
> 
> OK, I'm agree with you.
> 
> But this means that I have to make the drm_bridge_find_by_fwnode() function works
> on both DT systems and non-DT systems. This is also means that we will no longer
> need to call of_drm_find_bridge() function anymore. This will eventually lead to
> completely remove of_drm_find_bridge()?

It would be replaced by fwnode_drm_find_bridge(). Although, if we need
to rename the function, I think it would be best to make have a drm_
prefix, maybe drm_bridge_find-by_fwnode() or something similar.

> As far as I can see, if I follow you suggestion, drm/bridge subsystem will
> encountering a *big* refactor. My 'side-by-side' approach allows co-exist.
> It is not really meant to purge OF. I feel it is a little bit of aggressive.
> 
> hello Maxime, are you watching this? what do you think?
> 
> >>   static int simple_bridge_probe(struct platform_device *pdev)
> >>   {
> >>   	struct simple_bridge *sbridge;
> >> @@ -199,14 +232,17 @@ static int simple_bridge_probe(struct platform_device *pdev)
> >>   	else
> >>   		sbridge->info = simple_bridge_get_match_data(&pdev->dev);
> >>   
> >> -	/* Get the next bridge in the pipeline. */
> >> -	remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> >> -	if (!remote)
> >> -		return -EINVAL;
> >> -
> >> -	sbridge->next_bridge = of_drm_find_bridge(remote);
> >> -	of_node_put(remote);
> >> +	if (pdev->dev.of_node) {
> >> +		/* Get the next bridge in the pipeline. */
> >> +		remote = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> >> +		if (!remote)
> >> +			return -EINVAL;
> >>   
> >> +		sbridge->next_bridge = of_drm_find_bridge(remote);
> >> +		of_node_put(remote);
> >> +	} else {
> >> +		simple_bridge_get_next_bridge_by_fwnode(&pdev->dev, &sbridge->next_bridge);
> >> +	}
> >>   	if (!sbridge->next_bridge) {
> >>   		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
> >>   		return -EPROBE_DEFER;
> >> @@ -231,6 +267,7 @@ static int simple_bridge_probe(struct platform_device *pdev)
> >>   	/* Register the bridge. */
> >>   	sbridge->bridge.funcs = &simple_bridge_bridge_funcs;
> >>   	sbridge->bridge.of_node = pdev->dev.of_node;
> >> +	sbridge->bridge.fwnode = pdev->dev.fwnode;
> >>   	sbridge->bridge.timings = sbridge->info->timings;
> >>   
> >>   	drm_bridge_add(&sbridge->bridge);

-- 
Regards,

Laurent Pinchart

Re: Re: [PATCH 3/5] drm/bridge: simple-bridge: Allow acquiring the next bridge with fwnode API

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2837 bytes --]

On Tue, Jan 23, 2024 at 08:18:22PM +0800, Sui Jingfeng wrote:
> Hi,
> 
> 
> On 2024/1/23 09:18, Laurent Pinchart wrote:
> > On Tue, Jan 23, 2024 at 12:32:18AM +0800, Sui Jingfeng wrote:
> > > Which make it possible to use this driver on non-DT based systems,
> > > meanwhile, made no functional changes for DT based systems.
> > > 
> > > Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> > > ---
> > >   drivers/gpu/drm/bridge/simple-bridge.c | 51 ++++++++++++++++++++++----
> > >   1 file changed, 44 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> > > index 595f672745b9..cfea5a67cc5b 100644
> > > --- a/drivers/gpu/drm/bridge/simple-bridge.c
> > > +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> > > @@ -184,6 +184,39 @@ static const void *simple_bridge_get_match_data(const struct device *dev)
> > >   	return NULL;
> > >   }
> > > +static int simple_bridge_get_next_bridge_by_fwnode(struct device *dev,
> > > +						   struct drm_bridge **next_bridge)
> > > +{
> > > +	struct drm_bridge *bridge;
> > > +	struct fwnode_handle *ep;
> > > +	struct fwnode_handle *remote;
> > > +
> > > +	ep = fwnode_graph_get_endpoint_by_id(dev->fwnode, 1, 0, 0);
> > > +	if (!ep) {
> > > +		dev_err(dev, "The endpoint is unconnected\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	remote = fwnode_graph_get_remote_port_parent(ep);
> > > +	fwnode_handle_put(ep);
> > > +	if (!remote) {
> > > +		dev_err(dev, "No valid remote node\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	bridge = drm_bridge_find_by_fwnode(remote);
> > > +	fwnode_handle_put(remote);
> > > +
> > > +	if (!bridge) {
> > > +		dev_warn(dev, "Next bridge not found, deferring probe\n");
> > > +		return -EPROBE_DEFER;
> > > +	}
> > > +
> > > +	*next_bridge = bridge;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > Hmmmm yes, this convinces me further that we should switch to fwnode,
> > not implement fwnode and OF side-by-side.
> > 
> 
> OK, I'm agree with you.
> 
> 
> But this means that I have to make the drm_bridge_find_by_fwnode() function works
> on both DT systems and non-DT systems. This is also means that we will no longer
> need to call of_drm_find_bridge() function anymore. This will eventually lead to
> completely remove of_drm_find_bridge()?
> 
> 
> As far as I can see, if I follow you suggestion, drm/bridge subsystem will
> encountering a *big* refactor. My 'side-by-side' approach allows co-exist.
> It is not really meant to purge OF. I feel it is a little bit of aggressive.
> 
> hello Maxime, are you watching this? what do you think?

It's indeed going to be a pretty big refactoring, but I agree with
Laurent that we don't want to maintain both side by side.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API

[Andy Shevchenko]

On Tue, Jan 23, 2024 at 03:20:26AM +0200, Laurent Pinchart wrote:
> On Tue, Jan 23, 2024 at 12:32:20AM +0800, Sui Jingfeng wrote:

...

> >  	conn->bridge.of_node = pdev->dev.of_node;
> > +	conn->bridge.fwnode = pdev->dev.fwnode;
> 
> This goes in the right direction. Let's address the other drivers and
> drop the OF-based calls in the same series :-)

+1. BUT, please use device_set_node() instead of both lines.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems

[Andy Shevchenko]

On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote:
> Which is intended to be used on non-DT environment, where the simple-bridge
> platform device is created by either the display controller driver side or
> platform firmware subsystem. To avoid duplication and to keep consistent,
> we choose to reuse the OF match tables. Because the potentional user may
> not has a of_node attached, nor a ACPI match id. If this is the case,
> a software node string property can be provide to fill the niche.

...

> -	sbridge->info = of_device_get_match_data(&pdev->dev);
> +	if (pdev->dev.of_node)
> +		sbridge->info = of_device_get_match_data(&pdev->dev);
> +	else
> +		sbridge->info = simple_bridge_get_match_data(&pdev->dev);

This is wrong. Just use device_get_match_data() instead of of_ counter part.
The rest, if required, has to be addressed elsewhere.

So, formal NAK for the changes like above.

-- 
With Best Regards,
Andy Shevchenko