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

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

[Sui Jingfeng]

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.

Tested on TI BeaglePlay board.

v1 -> v2:
	 * Modify it66121 to switch togather
	 * Drop the 'side-by-side' implement
	 * Add drm_bridge_find_next_bridge_by_fwnode() helper
	 * Add drm_bridge_set_node() helper

v2 -> v3:
	 * Read kernel-doc and improve function comments (Dmitry)
	 * Drop the 'port' argument of it66121_read_bus_width() (Dmitry)
	 * drm-bridge: sii902x get nuked

v3 -> v4:
	 * drm-bridge: tfp410 get nuked
	 * Add platform module alias
	 * Rebase and improve commit message and function comments

Sui Jingfeng (9):
  drm/bridge: Allow using fwnode API to get the next bridge
  drm/bridge: simple-bridge: Use fwnode API to acquire device properties
  drm/bridge: simple-bridge: Add platform module alias
  drm-bridge: display-connector: Use fwnode API to acquire device
    properties
  drm/bridge: display-connector: Add platform module alias
  drm-bridge: sii902x: Use fwnode API to acquire device properties
  drm-bridge: it66121: Use fwnode API to acquire device properties
  drm/bridge: tfp410: Use fwnode API to acquire device properties
  drm/bridge: tfp410: Add platform module alias

 drivers/gpu/drm/bridge/display-connector.c | 25 ++++----
 drivers/gpu/drm/bridge/ite-it66121.c       | 57 ++++++++++-------
 drivers/gpu/drm/bridge/sii902x.c           | 43 ++++---------
 drivers/gpu/drm/bridge/simple-bridge.c     | 23 ++++---
 drivers/gpu/drm/bridge/ti-tfp410.c         | 42 ++++++------
 drivers/gpu/drm/drm_bridge.c               | 74 ++++++++++++++++++++++
 include/drm/drm_bridge.h                   | 16 +++++
 7 files changed, 185 insertions(+), 95 deletions(-)

-- 
2.34.1

[PATCH v4 1/9] drm/bridge: Allow using fwnode API to get the next bridge

[Sui Jingfeng]

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

Re: [PATCH v4 1/9] drm/bridge: Allow using fwnode API to get the next bridge

[Dmitry Baryshkov]

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

Re: [PATCH v4 1/9] drm/bridge: Allow using fwnode API to get the next bridge

[Sui Jingfeng]

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

[PATCH v4 2/9] drm/bridge: simple-bridge: Use fwnode API to acquire device properties

[Sui Jingfeng]

Make this driver less DT-dependent by calling the freshly created helpers,
should be no functional changes for DT based systems. But open the door for
otherwise use cases. Even though there is no user emerged yet, this still
do no harms.

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

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index 5813a2c4fc5e..3b09bdd5ad4d 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -9,7 +9,6 @@
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 
@@ -169,33 +168,32 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
 
 static int simple_bridge_probe(struct platform_device *pdev)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
 	struct simple_bridge *sbridge;
-	struct device_node *remote;
+	int ret;
 
 	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
 	if (!sbridge)
 		return -ENOMEM;
 	platform_set_drvdata(pdev, sbridge);
 
-	sbridge->info = of_device_get_match_data(&pdev->dev);
+	sbridge->info = device_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);
-
+	sbridge->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
 	if (!sbridge->next_bridge) {
 		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
+	} else if (IS_ERR(sbridge->next_bridge)) {
+		ret = PTR_ERR(sbridge->next_bridge);
+		dev_err(&pdev->dev, "Error on finding the next bridge: %d\n", ret);
+		return ret;
 	}
 
 	/* Get the regulator and GPIO resources. */
 	sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
 	if (IS_ERR(sbridge->vdd)) {
-		int ret = PTR_ERR(sbridge->vdd);
+		ret = PTR_ERR(sbridge->vdd);
 		if (ret == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
 		sbridge->vdd = NULL;
@@ -210,9 +208,9 @@ 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.timings = sbridge->info->timings;
 
+	drm_bridge_set_node(&sbridge->bridge, fwnode);
 	drm_bridge_add(&sbridge->bridge);
 
 	return 0;
-- 
2.34.1

Re: [PATCH v4 2/9] drm/bridge: simple-bridge: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:18:56AM +0800, Sui Jingfeng wrote:
> Make this driver less DT-dependent by calling the freshly created helpers,
> should be no functional changes for DT based systems. But open the door for
> otherwise use cases. Even though there is no user emerged yet, this still
> do no harms.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index 5813a2c4fc5e..3b09bdd5ad4d 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -9,7 +9,6 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -169,33 +168,32 @@ static const struct drm_bridge_funcs simple_bridge_bridge_funcs = {
>  
>  static int simple_bridge_probe(struct platform_device *pdev)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
>  	struct simple_bridge *sbridge;
> -	struct device_node *remote;
> +	int ret;
>  
>  	sbridge = devm_kzalloc(&pdev->dev, sizeof(*sbridge), GFP_KERNEL);
>  	if (!sbridge)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, sbridge);
>  
> -	sbridge->info = of_device_get_match_data(&pdev->dev);
> +	sbridge->info = device_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);
> -
> +	sbridge->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);

Can we please stick to the interface of drm_of_find_panel_or_bridge()?

Also note, the driver isn't looking for the next_bridge. It is looking
for the bridge at the fwnode remote endpoint.

>  	if (!sbridge->next_bridge) {
>  		dev_dbg(&pdev->dev, "Next bridge not found, deferring probe\n");
>  		return -EPROBE_DEFER;
> +	} else if (IS_ERR(sbridge->next_bridge)) {
> +		ret = PTR_ERR(sbridge->next_bridge);
> +		dev_err(&pdev->dev, "Error on finding the next bridge: %d\n", ret);
> +		return ret;
>  	}
>  
>  	/* Get the regulator and GPIO resources. */
>  	sbridge->vdd = devm_regulator_get_optional(&pdev->dev, "vdd");
>  	if (IS_ERR(sbridge->vdd)) {
> -		int ret = PTR_ERR(sbridge->vdd);
> +		ret = PTR_ERR(sbridge->vdd);
>  		if (ret == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
>  		sbridge->vdd = NULL;
> @@ -210,9 +208,9 @@ 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.timings = sbridge->info->timings;
>  
> +	drm_bridge_set_node(&sbridge->bridge, fwnode);

Please don't move the code. Having it in place of of_node setter
simplifies the review.

LGTM otherwise.

>  	drm_bridge_add(&sbridge->bridge);
>  
>  	return 0;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH v4 3/9] drm/bridge: simple-bridge: Add platform module alias

[Sui Jingfeng]

Otherwise when compiled as module, this driver will not be probed on
non-DT environment. This is a fundamential step to make this driver
truely OF-independent.

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

diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
index 3b09bdd5ad4d..6be271d1394b 100644
--- a/drivers/gpu/drm/bridge/simple-bridge.c
+++ b/drivers/gpu/drm/bridge/simple-bridge.c
@@ -308,3 +308,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.34.1

Re: [PATCH v4 3/9] drm/bridge: simple-bridge: Add platform module alias

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:18:57AM +0800, Sui Jingfeng wrote:
> Otherwise when compiled as module, this driver will not be probed on
> non-DT environment. This is a fundamential step to make this driver
> truely OF-independent.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/simple-bridge.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/simple-bridge.c b/drivers/gpu/drm/bridge/simple-bridge.c
> index 3b09bdd5ad4d..6be271d1394b 100644
> --- a/drivers/gpu/drm/bridge/simple-bridge.c
> +++ b/drivers/gpu/drm/bridge/simple-bridge.c
> @@ -308,3 +308,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");

I think it would be better to use MODULE_DEVICE_TABLE(platform,
simple_bridge_platform)id_table). Note, there is no such thing as
generic 'simple_bridge'. I'd expect platform device to have a
device-specific name which can be matched by the driver. This requires a
platform_device_id table.

> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH v4 4/9] drm-bridge: display-connector: Use fwnode API to acquire device properties

[Sui Jingfeng]

Make this driver less DT-dependent by using the fwnode helper functions,
should be no functional changes for DT based systems. Do the necessary
works before it can be truely DT-independent, this patch do no harms even
though there is no user yet.

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

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index ab8e00baf3f1..d80cb7bc59e6 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -204,6 +204,7 @@ static int display_connector_get_supply(struct platform_device *pdev,
 
 static int display_connector_probe(struct platform_device *pdev)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
 	struct display_connector *conn;
 	unsigned int type;
 	const char *label = NULL;
@@ -215,15 +216,15 @@ static int display_connector_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, conn);
 
-	type = (uintptr_t)of_device_get_match_data(&pdev->dev);
+	type = (uintptr_t)device_get_match_data(&pdev->dev);
 
 	/* Get the exact connector type. */
 	switch (type) {
 	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(fwnode, "analog");
+		digital = fwnode_property_present(fwnode, "digital");
 		if (analog && !digital) {
 			conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
 		} else if (!analog && digital) {
@@ -240,8 +241,7 @@ 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(fwnode, "type", &hdmi_type);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "HDMI connector with no type\n");
 			return -EINVAL;
@@ -271,7 +271,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(fwnode, "label", &label);
 
 	/*
 	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
@@ -309,12 +309,12 @@ 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 *phandle;
 
-		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);
+		phandle = fwnode_find_reference(fwnode, "ddc-i2c-bus", 0);
+		if (!IS_ERR_OR_NULL(phandle)) {
+			conn->bridge.ddc = i2c_get_adapter_by_fwnode(phandle);
+			fwnode_handle_put(phandle);
 			if (!conn->bridge.ddc)
 				return -EPROBE_DEFER;
 		} else {
@@ -358,7 +358,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;
+	drm_bridge_set_node(&conn->bridge, fwnode);
 
 	if (conn->bridge.ddc)
 		conn->bridge.ops |= DRM_BRIDGE_OP_EDID
-- 
2.34.1

Re: [PATCH v4 4/9] drm-bridge: display-connector: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:18:58AM +0800, Sui Jingfeng wrote:
> Make this driver less DT-dependent by using the fwnode helper functions,
> should be no functional changes for DT based systems. Do the necessary
> works before it can be truely DT-independent, this patch do no harms even
> though there is no user yet.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/display-connector.c | 24 +++++++++++-----------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index ab8e00baf3f1..d80cb7bc59e6 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -204,6 +204,7 @@ static int display_connector_get_supply(struct platform_device *pdev,
>  
>  static int display_connector_probe(struct platform_device *pdev)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&pdev->dev);
>  	struct display_connector *conn;
>  	unsigned int type;
>  	const char *label = NULL;
> @@ -215,15 +216,15 @@ static int display_connector_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, conn);
>  
> -	type = (uintptr_t)of_device_get_match_data(&pdev->dev);
> +	type = (uintptr_t)device_get_match_data(&pdev->dev);
>  
>  	/* Get the exact connector type. */
>  	switch (type) {
>  	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(fwnode, "analog");
> +		digital = fwnode_property_present(fwnode, "digital");
>  		if (analog && !digital) {
>  			conn->bridge.type = DRM_MODE_CONNECTOR_DVIA;
>  		} else if (!analog && digital) {
> @@ -240,8 +241,7 @@ 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(fwnode, "type", &hdmi_type);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "HDMI connector with no type\n");
>  			return -EINVAL;
> @@ -271,7 +271,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(fwnode, "label", &label);
>  
>  	/*
>  	 * Get the HPD GPIO for DVI, HDMI and DP connectors. If the GPIO can provide
> @@ -309,12 +309,12 @@ 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 *phandle;
>  
> -		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);
> +		phandle = fwnode_find_reference(fwnode, "ddc-i2c-bus", 0);
> +		if (!IS_ERR_OR_NULL(phandle)) {

s/IS_ERR_OR_NULL/IS_ERR/

LGTM otherwise.

> +			conn->bridge.ddc = i2c_get_adapter_by_fwnode(phandle);
> +			fwnode_handle_put(phandle);
>  			if (!conn->bridge.ddc)
>  				return -EPROBE_DEFER;
>  		} else {
> @@ -358,7 +358,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;
> +	drm_bridge_set_node(&conn->bridge, fwnode);
>  
>  	if (conn->bridge.ddc)
>  		conn->bridge.ops |= DRM_BRIDGE_OP_EDID
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH v4 5/9] drm/bridge: display-connector: Add platform module alias

[Sui Jingfeng]

Otherwise when compiled as module, this driver will not be probed on
non-DT environment. This is a fundamential step to make this driver
truely OF-independent.

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

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index d80cb7bc59e6..7d9b4edf4025 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -434,3 +434,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.34.1

Re: [PATCH v4 5/9] drm/bridge: display-connector: Add platform module alias

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:18:59AM +0800, Sui Jingfeng wrote:
> Otherwise when compiled as module, this driver will not be probed on
> non-DT environment. This is a fundamential step to make this driver
> truely OF-independent.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/display-connector.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index d80cb7bc59e6..7d9b4edf4025 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -434,3 +434,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");

Same comment as for one of the previous patches. Please add
platform_device_id table and corresponding MODULE_DEVICE_TABLE instead.

-- 
With best wishes
Dmitry

[PATCH v4 6/9] drm-bridge: sii902x: Use fwnode API to acquire device properties

[Sui Jingfeng]

Make this driver less DT-dependent by calling the freshly created helper
functions, which reduce boilerplate. Should be no functional changes for
DT based systems.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/sii902x.c | 43 +++++++++++---------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 8f84e98249c7..04436f318c7f 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -827,20 +827,19 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
 		.spdif = 0,
 		.max_i2s_channels = 0,
 	};
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	u8 lanes[4];
 	int num_lanes, i;
 
-	if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
+	if (!fwnode_property_present(fwnode, "#sound-dai-cells")) {
 		dev_dbg(dev, "%s: No \"#sound-dai-cells\", no audio\n",
 			__func__);
 		return 0;
 	}
 
-	num_lanes = of_property_read_variable_u8_array(dev->of_node,
-						       "sil,i2s-data-lanes",
-						       lanes, 1,
-						       ARRAY_SIZE(lanes));
-
+	num_lanes = fwnode_property_read_u8_array(fwnode,
+						  "sil,i2s-data-lanes",
+						  lanes, ARRAY_SIZE(lanes));
 	if (num_lanes == -EINVAL) {
 		dev_dbg(dev,
 			"%s: No \"sil,i2s-data-lanes\", use default <0>\n",
@@ -1097,13 +1096,13 @@ static int sii902x_init(struct sii902x *sii902x)
 		goto err_unreg_audio;
 
 	sii902x->bridge.funcs = &sii902x_bridge_funcs;
-	sii902x->bridge.of_node = dev->of_node;
 	sii902x->bridge.timings = &default_sii902x_timings;
 	sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
 
 	if (sii902x->i2c->irq > 0)
 		sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD;
 
+	drm_bridge_set_node(&sii902x->bridge, dev_fwnode(dev));
 	drm_bridge_add(&sii902x->bridge);
 
 	return 0;
@@ -1118,7 +1117,6 @@ static int sii902x_init(struct sii902x *sii902x)
 static int sii902x_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	struct device_node *endpoint;
 	struct sii902x *sii902x;
 	static const char * const supplies[] = {"iovcc", "cvcc12"};
 	int ret;
@@ -1147,27 +1145,14 @@ static int sii902x_probe(struct i2c_client *client)
 		return PTR_ERR(sii902x->reset_gpio);
 	}
 
-	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
-	if (endpoint) {
-		struct device_node *remote = of_graph_get_remote_port_parent(endpoint);
-
-		of_node_put(endpoint);
-		if (!remote) {
-			dev_err(dev, "Endpoint in port@1 unconnected\n");
-			return -ENODEV;
-		}
-
-		if (!of_device_is_available(remote)) {
-			dev_err(dev, "port@1 remote device is disabled\n");
-			of_node_put(remote);
-			return -ENODEV;
-		}
-
-		sii902x->next_bridge = of_drm_find_bridge(remote);
-		of_node_put(remote);
-		if (!sii902x->next_bridge)
-			return dev_err_probe(dev, -EPROBE_DEFER,
-					     "Failed to find remote bridge\n");
+	sii902x->next_bridge = drm_bridge_find_next_bridge_by_fwnode(dev_fwnode(dev), 1);
+	if (!sii902x->next_bridge) {
+		return dev_err_probe(dev, -EPROBE_DEFER,
+				     "Failed to find the next bridge\n");
+	} else if (IS_ERR(sii902x->next_bridge)) {
+		ret = PTR_ERR(sii902x->next_bridge);
+		dev_err(dev, "Error on find the next bridge: %d\n", ret);
+		return ret;
 	}
 
 	mutex_init(&sii902x->mutex);
-- 
2.34.1

Re: [PATCH v4 6/9] drm-bridge: sii902x: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:19:00AM +0800, Sui Jingfeng wrote:
> Make this driver less DT-dependent by calling the freshly created helper
> functions, which reduce boilerplate. Should be no functional changes for
> DT based systems.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 43 +++++++++++---------------------
>  1 file changed, 14 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 8f84e98249c7..04436f318c7f 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -827,20 +827,19 @@ static int sii902x_audio_codec_init(struct sii902x *sii902x,
>  		.spdif = 0,
>  		.max_i2s_channels = 0,
>  	};
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	u8 lanes[4];
>  	int num_lanes, i;
>  
> -	if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
> +	if (!fwnode_property_present(fwnode, "#sound-dai-cells")) {
>  		dev_dbg(dev, "%s: No \"#sound-dai-cells\", no audio\n",
>  			__func__);
>  		return 0;
>  	}
>  
> -	num_lanes = of_property_read_variable_u8_array(dev->of_node,
> -						       "sil,i2s-data-lanes",
> -						       lanes, 1,
> -						       ARRAY_SIZE(lanes));
> -
> +	num_lanes = fwnode_property_read_u8_array(fwnode,
> +						  "sil,i2s-data-lanes",
> +						  lanes, ARRAY_SIZE(lanes));

You have lost support for having less than 4 lanes. Please see the
documentation for this function.

>  	if (num_lanes == -EINVAL) {
>  		dev_dbg(dev,
>  			"%s: No \"sil,i2s-data-lanes\", use default <0>\n",
> @@ -1097,13 +1096,13 @@ static int sii902x_init(struct sii902x *sii902x)
>  		goto err_unreg_audio;
>  
>  	sii902x->bridge.funcs = &sii902x_bridge_funcs;
> -	sii902x->bridge.of_node = dev->of_node;
>  	sii902x->bridge.timings = &default_sii902x_timings;
>  	sii902x->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
>  
>  	if (sii902x->i2c->irq > 0)
>  		sii902x->bridge.ops |= DRM_BRIDGE_OP_HPD;
>  
> +	drm_bridge_set_node(&sii902x->bridge, dev_fwnode(dev));

Move back to the place of the of_node setter.

>  	drm_bridge_add(&sii902x->bridge);
>  
>  	return 0;
> @@ -1118,7 +1117,6 @@ static int sii902x_init(struct sii902x *sii902x)
>  static int sii902x_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> -	struct device_node *endpoint;
>  	struct sii902x *sii902x;
>  	static const char * const supplies[] = {"iovcc", "cvcc12"};
>  	int ret;
> @@ -1147,27 +1145,14 @@ static int sii902x_probe(struct i2c_client *client)
>  		return PTR_ERR(sii902x->reset_gpio);
>  	}
>  
> -	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 1, -1);
> -	if (endpoint) {
> -		struct device_node *remote = of_graph_get_remote_port_parent(endpoint);
> -
> -		of_node_put(endpoint);
> -		if (!remote) {
> -			dev_err(dev, "Endpoint in port@1 unconnected\n");
> -			return -ENODEV;
> -		}
> -
> -		if (!of_device_is_available(remote)) {
> -			dev_err(dev, "port@1 remote device is disabled\n");
> -			of_node_put(remote);
> -			return -ENODEV;
> -		}
> -
> -		sii902x->next_bridge = of_drm_find_bridge(remote);
> -		of_node_put(remote);
> -		if (!sii902x->next_bridge)
> -			return dev_err_probe(dev, -EPROBE_DEFER,
> -					     "Failed to find remote bridge\n");
> +	sii902x->next_bridge = drm_bridge_find_next_bridge_by_fwnode(dev_fwnode(dev), 1);
> +	if (!sii902x->next_bridge) {
> +		return dev_err_probe(dev, -EPROBE_DEFER,
> +				     "Failed to find the next bridge\n");
> +	} else if (IS_ERR(sii902x->next_bridge)) {
> +		ret = PTR_ERR(sii902x->next_bridge);
> +		dev_err(dev, "Error on find the next bridge: %d\n", ret);
> +		return ret;
>  	}
>  
>  	mutex_init(&sii902x->mutex);
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

[PATCH v4 7/9] drm-bridge: it66121: Use fwnode API to acquire device properties

[Sui Jingfeng]

Make this driver DT-independent by calling the freshly created helpers,
which reduce boilerplate and open the door for otherwise use cases. No
functional changes for DT based systems.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 57 +++++++++++++++++-----------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 925e42f46cd8..688dc1830654 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -15,7 +15,6 @@
 #include <linux/bitfield.h>
 #include <linux/property.h>
 #include <linux/regmap.h>
-#include <linux/of_graph.h>
 #include <linux/gpio/consumer.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/regulator/consumer.h>
@@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
 
 	dev_dbg(dev, "%s\n", __func__);
 
-	if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
+	if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
 		dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
 		return 0;
 	}
@@ -1503,13 +1502,36 @@ static const char * const it66121_supplies[] = {
 	"vcn33", "vcn18", "vrf12"
 };
 
+static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 *bus_width)
+{
+	struct fwnode_handle *endpoint;
+	u32 val;
+	int ret;
+
+	endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
+	if (!endpoint)
+		return -EINVAL;
+
+	ret = fwnode_property_read_u32(endpoint, "bus-width", &val);
+	fwnode_handle_put(endpoint);
+	if (ret)
+		return ret;
+
+	if (val != 12 && val != 24)
+		return -EINVAL;
+
+	*bus_width = val;
+
+	return 0;
+}
+
 static int it66121_probe(struct i2c_client *client)
 {
 	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
-	struct device_node *ep;
 	int ret;
 	struct it66121_ctx *ctx;
 	struct device *dev = &client->dev;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
 		dev_err(dev, "I2C check functionality failed.\n");
@@ -1520,29 +1542,20 @@ static int it66121_probe(struct i2c_client *client)
 	if (!ctx)
 		return -ENOMEM;
 
-	ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
-	if (!ep)
-		return -EINVAL;
-
 	ctx->dev = dev;
 	ctx->client = client;
 	ctx->info = i2c_get_match_data(client);
 
-	of_property_read_u32(ep, "bus-width", &ctx->bus_width);
-	of_node_put(ep);
-
-	if (ctx->bus_width != 12 && ctx->bus_width != 24)
-		return -EINVAL;
-
-	ep = of_graph_get_remote_node(dev->of_node, 1, -1);
-	if (!ep) {
-		dev_err(ctx->dev, "The endpoint is unconnected\n");
-		return -EINVAL;
-	}
+	ret = it66121_read_bus_width(fwnode, &ctx->bus_width);
+	if (ret)
+		return ret;
 
-	ctx->next_bridge = of_drm_find_bridge(ep);
-	of_node_put(ep);
-	if (!ctx->next_bridge) {
+	ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+	if (IS_ERR(ctx->next_bridge)) {
+		ret = PTR_ERR(ctx->next_bridge);
+		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+		return ret;
+	} else if (!ctx->next_bridge) {
 		dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
 	}
@@ -1577,8 +1590,8 @@ static int it66121_probe(struct i2c_client *client)
 		return -ENODEV;
 	}
 
+	drm_bridge_set_node(&ctx->bridge, fwnode);
 	ctx->bridge.funcs = &it66121_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
 	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
 	if (client->irq > 0) {
-- 
2.34.1

Re: [PATCH v4 7/9] drm-bridge: it66121: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:19:01AM +0800, Sui Jingfeng wrote:
> Make this driver DT-independent by calling the freshly created helpers,
> which reduce boilerplate and open the door for otherwise use cases. No
> functional changes for DT based systems.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 57 +++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 925e42f46cd8..688dc1830654 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -15,7 +15,6 @@
>  #include <linux/bitfield.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> -#include <linux/of_graph.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/regulator/consumer.h>
> @@ -1480,7 +1479,7 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev)
>  
>  	dev_dbg(dev, "%s\n", __func__);
>  
> -	if (!of_property_read_bool(dev->of_node, "#sound-dai-cells")) {
> +	if (!fwnode_property_present(dev_fwnode(dev), "#sound-dai-cells")) {
>  		dev_info(dev, "No \"#sound-dai-cells\", no audio\n");
>  		return 0;
>  	}
> @@ -1503,13 +1502,36 @@ static const char * const it66121_supplies[] = {
>  	"vcn33", "vcn18", "vrf12"
>  };
>  
> +static int it66121_read_bus_width(struct fwnode_handle *fwnode, u32 *bus_width)
> +{
> +	struct fwnode_handle *endpoint;
> +	u32 val;
> +	int ret;
> +
> +	endpoint = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> +	if (!endpoint)
> +		return -EINVAL;
> +
> +	ret = fwnode_property_read_u32(endpoint, "bus-width", &val);
> +	fwnode_handle_put(endpoint);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 12 && val != 24)
> +		return -EINVAL;
> +
> +	*bus_width = val;
> +
> +	return 0;
> +}

Ideally this should come as two patches. First patch extracts the
function, second patch changes the driver to use fwnode / property API.

> +
>  static int it66121_probe(struct i2c_client *client)
>  {
>  	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> -	struct device_node *ep;
>  	int ret;
>  	struct it66121_ctx *ctx;
>  	struct device *dev = &client->dev;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>  		dev_err(dev, "I2C check functionality failed.\n");
> @@ -1520,29 +1542,20 @@ static int it66121_probe(struct i2c_client *client)
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> -	if (!ep)
> -		return -EINVAL;
> -
>  	ctx->dev = dev;
>  	ctx->client = client;
>  	ctx->info = i2c_get_match_data(client);
>  
> -	of_property_read_u32(ep, "bus-width", &ctx->bus_width);
> -	of_node_put(ep);
> -
> -	if (ctx->bus_width != 12 && ctx->bus_width != 24)
> -		return -EINVAL;
> -
> -	ep = of_graph_get_remote_node(dev->of_node, 1, -1);
> -	if (!ep) {
> -		dev_err(ctx->dev, "The endpoint is unconnected\n");
> -		return -EINVAL;
> -	}
> +	ret = it66121_read_bus_width(fwnode, &ctx->bus_width);
> +	if (ret)
> +		return ret;
>  
> -	ctx->next_bridge = of_drm_find_bridge(ep);
> -	of_node_put(ep);
> -	if (!ctx->next_bridge) {
> +	ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
> +	if (IS_ERR(ctx->next_bridge)) {
> +		ret = PTR_ERR(ctx->next_bridge);
> +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
> +		return ret;

return dev_err_probe(dev, ret, "msg"), if your function doesn't do this.
If it does, just return ret.

> +	} else if (!ctx->next_bridge) {
>  		dev_dbg(ctx->dev, "Next bridge not found, deferring probe\n");
>  		return -EPROBE_DEFER;
>  	}
> @@ -1577,8 +1590,8 @@ static int it66121_probe(struct i2c_client *client)
>  		return -ENODEV;
>  	}
>  
> +	drm_bridge_set_node(&ctx->bridge, fwnode);
>  	ctx->bridge.funcs = &it66121_bridge_funcs;
> -	ctx->bridge.of_node = dev->of_node;
>  	ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
>  	ctx->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
>  	if (client->irq > 0) {
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 7/9] drm-bridge: it66121: Use fwnode API to acquire device properties

[Sui Jingfeng]

Hi,


On 2024/4/23 04:06, Dmitry Baryshkov wrote:
>> +
>>   static int it66121_probe(struct i2c_client *client)
>>   {
>>   	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
>> -	struct device_node *ep;
>>   	int ret;
>>   	struct it66121_ctx *ctx;
>>   	struct device *dev = &client->dev;
>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>>   
>>   	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>>   		dev_err(dev, "I2C check functionality failed.\n");
>> @@ -1520,29 +1542,20 @@ static int it66121_probe(struct i2c_client *client)
>>   	if (!ctx)
>>   		return -ENOMEM;
>>   
>> -	ep = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>> -	if (!ep)
>> -		return -EINVAL;
>> -
>>   	ctx->dev = dev;
>>   	ctx->client = client;
>>   	ctx->info = i2c_get_match_data(client);
>>   
>> -	of_property_read_u32(ep, "bus-width", &ctx->bus_width);
>> -	of_node_put(ep);
>> -
>> -	if (ctx->bus_width != 12 && ctx->bus_width != 24)
>> -		return -EINVAL;
>> -
>> -	ep = of_graph_get_remote_node(dev->of_node, 1, -1);
>> -	if (!ep) {
>> -		dev_err(ctx->dev, "The endpoint is unconnected\n");
>> -		return -EINVAL;
>> -	}
>> +	ret = it66121_read_bus_width(fwnode, &ctx->bus_width);
>> +	if (ret)
>> +		return ret;
>>   
>> -	ctx->next_bridge = of_drm_find_bridge(ep);
>> -	of_node_put(ep);
>> -	if (!ctx->next_bridge) {
>> +	ctx->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
>> +	if (IS_ERR(ctx->next_bridge)) {
>> +		ret = PTR_ERR(ctx->next_bridge);
>> +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
>> +		return ret;
> return dev_err_probe(dev, ret, "msg"), if your function doesn't do this.
> If it does, just return ret.


My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER
this is known for sure. As a//prior(priori) knowledge. Calling the dev_err_probe()
just introduce extra overhead. It is useless to use dev_err_probe() here.
  

-- 
Best regards,
Sui

[PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties

[Sui Jingfeng]

Make this driver DT-independent by calling the freshly created helpers,
which reduce boilerplate and open the door for otherwise use cases. No
functional changes for DT based systems.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++++++++++++++---------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index c7bef5c23927..58dc7492844f 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -266,8 +266,9 @@ static const struct drm_bridge_timings tfp410_default_timings = {
 
 static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
 	struct drm_bridge_timings *timings = &dvi->timings;
-	struct device_node *ep;
+	struct fwnode_handle *ep;
 	u32 pclk_sample = 0;
 	u32 bus_width = 24;
 	u32 deskew = 0;
@@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
 	 * and EDGE pins. They are specified in DT through endpoint properties
 	 * and vendor-specific properties.
 	 */
-	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
+	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
 	if (!ep)
 		return -EINVAL;
 
 	/* Get the sampling edge from the endpoint. */
-	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
-	of_property_read_u32(ep, "bus-width", &bus_width);
-	of_node_put(ep);
+	fwnode_property_read_u32(ep, "pclk-sample", &pclk_sample);
+	fwnode_property_read_u32(ep, "bus-width", &bus_width);
+	fwnode_handle_put(ep);
 
 	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
 
@@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
 	}
 
 	/* Get the setup and hold time from vendor-specific properties. */
-	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
+	fwnode_property_read_u32(fwnode, "ti,deskew", &deskew);
 	if (deskew > 7)
 		return -EINVAL;
 
@@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
 
 static int tfp410_init(struct device *dev, bool i2c)
 {
-	struct device_node *node;
+	struct fwnode_handle *fwnode = dev_fwnode(dev);
 	struct tfp410 *dvi;
 	int ret;
 
-	if (!dev->of_node) {
-		dev_err(dev, "device-tree data is missing\n");
+	if (!fwnode) {
+		dev_err(dev, "firmware data is missing\n");
 		return -ENXIO;
 	}
 
@@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
 	dvi->dev = dev;
 	dev_set_drvdata(dev, dvi);
 
+	drm_bridge_set_node(&dvi->bridge, fwnode);
 	dvi->bridge.funcs = &tfp410_bridge_funcs;
-	dvi->bridge.of_node = dev->of_node;
 	dvi->bridge.timings = &dvi->timings;
 	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
 
@@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
 		return ret;
 
 	/* Get the next bridge, connected to port@1. */
-	node = of_graph_get_remote_node(dev->of_node, 1, -1);
-	if (!node)
-		return -ENODEV;
-
-	dvi->next_bridge = of_drm_find_bridge(node);
-	of_node_put(node);
-
-	if (!dvi->next_bridge)
+	dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
+	if (IS_ERR(dvi->next_bridge)) {
+		ret = PTR_ERR(dvi->next_bridge);
+		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
+		return ret;
+	} else if (!dvi->next_bridge) {
+		dev_dbg(dev, "Next bridge not found, deferring probe\n");
 		return -EPROBE_DEFER;
+	}
 
 	/* Get the powerdown GPIO. */
 	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
@@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
 /* There is currently no i2c functionality. */
 static int tfp410_i2c_probe(struct i2c_client *client)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
 	int reg;
 
-	if (!client->dev.of_node ||
-	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
+	if (!fwnode || fwnode_property_read_u32(fwnode, "reg", &reg)) {
 		dev_err(&client->dev,
 			"Can't get i2c reg property from device-tree\n");
 		return -ENXIO;
-- 
2.34.1

Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Tue, Apr 23, 2024 at 03:19:02AM +0800, Sui Jingfeng wrote:
> Make this driver DT-independent by calling the freshly created helpers,
> which reduce boilerplate and open the door for otherwise use cases. No
> functional changes for DT based systems.
> 
> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> ---
>  drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++++++++++++++---------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> index c7bef5c23927..58dc7492844f 100644
> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> @@ -266,8 +266,9 @@ static const struct drm_bridge_timings tfp410_default_timings = {
>  
>  static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
>  	struct drm_bridge_timings *timings = &dvi->timings;
> -	struct device_node *ep;
> +	struct fwnode_handle *ep;
>  	u32 pclk_sample = 0;
>  	u32 bus_width = 24;
>  	u32 deskew = 0;
> @@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  	 * and EDGE pins. They are specified in DT through endpoint properties
>  	 * and vendor-specific properties.
>  	 */
> -	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
>  	if (!ep)
>  		return -EINVAL;
>  
>  	/* Get the sampling edge from the endpoint. */
> -	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
> -	of_property_read_u32(ep, "bus-width", &bus_width);
> -	of_node_put(ep);
> +	fwnode_property_read_u32(ep, "pclk-sample", &pclk_sample);
> +	fwnode_property_read_u32(ep, "bus-width", &bus_width);
> +	fwnode_handle_put(ep);
>  
>  	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
>  
> @@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  	}
>  
>  	/* Get the setup and hold time from vendor-specific properties. */
> -	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
> +	fwnode_property_read_u32(fwnode, "ti,deskew", &deskew);
>  	if (deskew > 7)
>  		return -EINVAL;
>  
> @@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>  
>  static int tfp410_init(struct device *dev, bool i2c)
>  {
> -	struct device_node *node;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>  	struct tfp410 *dvi;
>  	int ret;
>  
> -	if (!dev->of_node) {
> -		dev_err(dev, "device-tree data is missing\n");
> +	if (!fwnode) {
> +		dev_err(dev, "firmware data is missing\n");
>  		return -ENXIO;
>  	}
>  
> @@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
>  	dvi->dev = dev;
>  	dev_set_drvdata(dev, dvi);
>  
> +	drm_bridge_set_node(&dvi->bridge, fwnode);
>  	dvi->bridge.funcs = &tfp410_bridge_funcs;
> -	dvi->bridge.of_node = dev->of_node;
>  	dvi->bridge.timings = &dvi->timings;
>  	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
>  
> @@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
>  		return ret;
>  
>  	/* Get the next bridge, connected to port@1. */
> -	node = of_graph_get_remote_node(dev->of_node, 1, -1);
> -	if (!node)
> -		return -ENODEV;
> -
> -	dvi->next_bridge = of_drm_find_bridge(node);
> -	of_node_put(node);
> -
> -	if (!dvi->next_bridge)
> +	dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
> +	if (IS_ERR(dvi->next_bridge)) {
> +		ret = PTR_ERR(dvi->next_bridge);
> +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
> +		return ret;

Same comment regarding dev_err_probe().

LGTM otherwise.

> +	} else if (!dvi->next_bridge) {
> +		dev_dbg(dev, "Next bridge not found, deferring probe\n");
>  		return -EPROBE_DEFER;

Looking at the bolerplate code, I think it would be better to make
drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.


> +	}
>  
>  	/* Get the powerdown GPIO. */
>  	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
> @@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
>  /* There is currently no i2c functionality. */
>  static int tfp410_i2c_probe(struct i2c_client *client)
>  {
> +	struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
>  	int reg;
>  
> -	if (!client->dev.of_node ||
> -	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
> +	if (!fwnode || fwnode_property_read_u32(fwnode, "reg", &reg)) {
>  		dev_err(&client->dev,
>  			"Can't get i2c reg property from device-tree\n");
>  		return -ENXIO;
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties

[Sui Jingfeng]

Hi,


On 2024/4/23 04:08, Dmitry Baryshkov wrote:
> On Tue, Apr 23, 2024 at 03:19:02AM +0800, Sui Jingfeng wrote:
>> Make this driver DT-independent by calling the freshly created helpers,
>> which reduce boilerplate and open the door for otherwise use cases. No
>> functional changes for DT based systems.
>>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++++++++++++++---------------
>>   1 file changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
>> index c7bef5c23927..58dc7492844f 100644
>> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
>> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
>> @@ -266,8 +266,9 @@ static const struct drm_bridge_timings tfp410_default_timings = {
>>   
>>   static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>   {
>> +	struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
>>   	struct drm_bridge_timings *timings = &dvi->timings;
>> -	struct device_node *ep;
>> +	struct fwnode_handle *ep;
>>   	u32 pclk_sample = 0;
>>   	u32 bus_width = 24;
>>   	u32 deskew = 0;
>> @@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>   	 * and EDGE pins. They are specified in DT through endpoint properties
>>   	 * and vendor-specific properties.
>>   	 */
>> -	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
>> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
>>   	if (!ep)
>>   		return -EINVAL;
>>   
>>   	/* Get the sampling edge from the endpoint. */
>> -	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
>> -	of_property_read_u32(ep, "bus-width", &bus_width);
>> -	of_node_put(ep);
>> +	fwnode_property_read_u32(ep, "pclk-sample", &pclk_sample);
>> +	fwnode_property_read_u32(ep, "bus-width", &bus_width);
>> +	fwnode_handle_put(ep);
>>   
>>   	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
>>   
>> @@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>   	}
>>   
>>   	/* Get the setup and hold time from vendor-specific properties. */
>> -	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
>> +	fwnode_property_read_u32(fwnode, "ti,deskew", &deskew);
>>   	if (deskew > 7)
>>   		return -EINVAL;
>>   
>> @@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>   
>>   static int tfp410_init(struct device *dev, bool i2c)
>>   {
>> -	struct device_node *node;
>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>>   	struct tfp410 *dvi;
>>   	int ret;
>>   
>> -	if (!dev->of_node) {
>> -		dev_err(dev, "device-tree data is missing\n");
>> +	if (!fwnode) {
>> +		dev_err(dev, "firmware data is missing\n");
>>   		return -ENXIO;
>>   	}
>>   
>> @@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
>>   	dvi->dev = dev;
>>   	dev_set_drvdata(dev, dvi);
>>   
>> +	drm_bridge_set_node(&dvi->bridge, fwnode);
>>   	dvi->bridge.funcs = &tfp410_bridge_funcs;
>> -	dvi->bridge.of_node = dev->of_node;
>>   	dvi->bridge.timings = &dvi->timings;
>>   	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
>>   
>> @@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
>>   		return ret;
>>   
>>   	/* Get the next bridge, connected to port@1. */
>> -	node = of_graph_get_remote_node(dev->of_node, 1, -1);
>> -	if (!node)
>> -		return -ENODEV;
>> -
>> -	dvi->next_bridge = of_drm_find_bridge(node);
>> -	of_node_put(node);
>> -
>> -	if (!dvi->next_bridge)
>> +	dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
>> +	if (IS_ERR(dvi->next_bridge)) {
>> +		ret = PTR_ERR(dvi->next_bridge);
>> +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
>> +		return ret;
> Same comment regarding dev_err_probe().
>
> LGTM otherwise.


My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER,
this is known for sure. this can be used as a prior(priori) knowledge. This is
intentionally by design.


Calling the dev_err_probe() just introduce extra overhead on non EPROBE_DEFER
cases. Hence, It is useless to use dev_err_probe() at here.


>> +	} else if (!dvi->next_bridge) {
>> +		dev_dbg(dev, "Next bridge not found, deferring probe\n");
>>   		return -EPROBE_DEFER;
> Looking at the bolerplate code, I think it would be better to make
> drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.
>
The drm_bridge_find_next_bridge_by_fwnode() function itself can not
reliable detect if the driver(the remote bridge) already probed or not.

Hence, as a core helper function, we can not guarantee that return
-EPROBE_DEFER is always correct.

While, return NULL is always correct. The NULL can stand for two meanings.
One is that the next bridge is really don't exist, may happen when the
caller provided a wrong fwnode argument.

Another case is that the next bridge exists but not probed yet, and
drm_bridge_find_next_bridge_by_fwnode() can return NULL when it gets called
too early.

Therefore, it is better to left to the users of this function to process
the NULL return value. As driver instances has some extra prior knowledge.
And can be controlled by drm bridge driver author.

>> +	}
>>   
>>   	/* Get the powerdown GPIO. */
>>   	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
>> @@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
>>   /* There is currently no i2c functionality. */
>>   static int tfp410_i2c_probe(struct i2c_client *client)
>>   {
>> +	struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
>>   	int reg;
>>   
>> -	if (!client->dev.of_node ||
>> -	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
>> +	if (!fwnode || fwnode_property_read_u32(fwnode, "reg", &reg)) {
>>   		dev_err(&client->dev,
>>   			"Can't get i2c reg property from device-tree\n");
>>   		return -ENXIO;
>> -- 
>> 2.34.1
>>
-- 
Best regards,
Sui

Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties

[Dmitry Baryshkov]

On Sun, Apr 28, 2024 at 02:43:20AM +0800, Sui Jingfeng wrote:
> Hi,
> 
> 
> On 2024/4/23 04:08, Dmitry Baryshkov wrote:
> > On Tue, Apr 23, 2024 at 03:19:02AM +0800, Sui Jingfeng wrote:
> > > Make this driver DT-independent by calling the freshly created helpers,
> > > which reduce boilerplate and open the door for otherwise use cases. No
> > > functional changes for DT based systems.
> > > 
> > > Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
> > > ---
> > >   drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++++++++++++++---------------
> > >   1 file changed, 21 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
> > > index c7bef5c23927..58dc7492844f 100644
> > > --- a/drivers/gpu/drm/bridge/ti-tfp410.c
> > > +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
> > > @@ -266,8 +266,9 @@ static const struct drm_bridge_timings tfp410_default_timings = {
> > >   static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> > >   {
> > > +	struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
> > >   	struct drm_bridge_timings *timings = &dvi->timings;
> > > -	struct device_node *ep;
> > > +	struct fwnode_handle *ep;
> > >   	u32 pclk_sample = 0;
> > >   	u32 bus_width = 24;
> > >   	u32 deskew = 0;
> > > @@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> > >   	 * and EDGE pins. They are specified in DT through endpoint properties
> > >   	 * and vendor-specific properties.
> > >   	 */
> > > -	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
> > > +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
> > >   	if (!ep)
> > >   		return -EINVAL;
> > >   	/* Get the sampling edge from the endpoint. */
> > > -	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
> > > -	of_property_read_u32(ep, "bus-width", &bus_width);
> > > -	of_node_put(ep);
> > > +	fwnode_property_read_u32(ep, "pclk-sample", &pclk_sample);
> > > +	fwnode_property_read_u32(ep, "bus-width", &bus_width);
> > > +	fwnode_handle_put(ep);
> > >   	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
> > > @@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> > >   	}
> > >   	/* Get the setup and hold time from vendor-specific properties. */
> > > -	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
> > > +	fwnode_property_read_u32(fwnode, "ti,deskew", &deskew);
> > >   	if (deskew > 7)
> > >   		return -EINVAL;
> > > @@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
> > >   static int tfp410_init(struct device *dev, bool i2c)
> > >   {
> > > -	struct device_node *node;
> > > +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >   	struct tfp410 *dvi;
> > >   	int ret;
> > > -	if (!dev->of_node) {
> > > -		dev_err(dev, "device-tree data is missing\n");
> > > +	if (!fwnode) {
> > > +		dev_err(dev, "firmware data is missing\n");
> > >   		return -ENXIO;
> > >   	}
> > > @@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
> > >   	dvi->dev = dev;
> > >   	dev_set_drvdata(dev, dvi);
> > > +	drm_bridge_set_node(&dvi->bridge, fwnode);
> > >   	dvi->bridge.funcs = &tfp410_bridge_funcs;
> > > -	dvi->bridge.of_node = dev->of_node;
> > >   	dvi->bridge.timings = &dvi->timings;
> > >   	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
> > > @@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
> > >   		return ret;
> > >   	/* Get the next bridge, connected to port@1. */
> > > -	node = of_graph_get_remote_node(dev->of_node, 1, -1);
> > > -	if (!node)
> > > -		return -ENODEV;
> > > -
> > > -	dvi->next_bridge = of_drm_find_bridge(node);
> > > -	of_node_put(node);
> > > -
> > > -	if (!dvi->next_bridge)
> > > +	dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
> > > +	if (IS_ERR(dvi->next_bridge)) {
> > > +		ret = PTR_ERR(dvi->next_bridge);
> > > +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
> > > +		return ret;
> > Same comment regarding dev_err_probe().
> > 
> > LGTM otherwise.
> 
> 
> My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER,
> this is known for sure. this can be used as a prior(priori) knowledge. This is
> intentionally by design.
> 
> 
> Calling the dev_err_probe() just introduce extra overhead on non EPROBE_DEFER
> cases. Hence, It is useless to use dev_err_probe() at here.
> 
> 
> > > +	} else if (!dvi->next_bridge) {
> > > +		dev_dbg(dev, "Next bridge not found, deferring probe\n");
> > >   		return -EPROBE_DEFER;
> > Looking at the bolerplate code, I think it would be better to make
> > drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.
> > 
> The drm_bridge_find_next_bridge_by_fwnode() function itself can not
> reliable detect if the driver(the remote bridge) already probed or not.
> 
> Hence, as a core helper function, we can not guarantee that return
> -EPROBE_DEFER is always correct.
> 
> While, return NULL is always correct. The NULL can stand for two meanings.
> One is that the next bridge is really don't exist, may happen when the
> caller provided a wrong fwnode argument.

Please take a look at drm_of_find_panel_or_bridge(). Returning specific
error code is always better than returning just NULL. As you have
pointed yourself, there are (at least) two cases when your function
returns NULL. Caller can not identify them unless the function returns
proper error code.

> Another case is that the next bridge exists but not probed yet, and
> drm_bridge_find_next_bridge_by_fwnode() can return NULL when it gets called
> too early.
> 
> Therefore, it is better to left to the users of this function to process
> the NULL return value. As driver instances has some extra prior knowledge.
> And can be controlled by drm bridge driver author.

he driver has no prior knowledge if there is a remote fwnode/ofnode or
if there is none.

> 
> > > +	}
> > >   	/* Get the powerdown GPIO. */
> > >   	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
> > > @@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
> > >   /* There is currently no i2c functionality. */
> > >   static int tfp410_i2c_probe(struct i2c_client *client)
> > >   {
> > > +	struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
> > >   	int reg;
> > > -	if (!client->dev.of_node ||
> > > -	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
> > > +	if (!fwnode || fwnode_property_read_u32(fwnode, "reg", &reg)) {
> > >   		dev_err(&client->dev,
> > >   			"Can't get i2c reg property from device-tree\n");
> > >   		return -ENXIO;
> > > -- 
> > > 2.34.1
> > > 
> -- 
> Best regards,
> Sui
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 8/9] drm/bridge: tfp410: Use fwnode API to acquire device properties

[Sui Jingfeng]

Hi,


On 2024/4/28 03:17, Dmitry Baryshkov wrote:
> On Sun, Apr 28, 2024 at 02:43:20AM +0800, Sui Jingfeng wrote:
>> Hi,
>>
>>
>> On 2024/4/23 04:08, Dmitry Baryshkov wrote:
>>> On Tue, Apr 23, 2024 at 03:19:02AM +0800, Sui Jingfeng wrote:
>>>> Make this driver DT-independent by calling the freshly created helpers,
>>>> which reduce boilerplate and open the door for otherwise use cases. No
>>>> functional changes for DT based systems.
>>>>
>>>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>>>> ---
>>>>    drivers/gpu/drm/bridge/ti-tfp410.c | 41 +++++++++++++++---------------
>>>>    1 file changed, 21 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
>>>> index c7bef5c23927..58dc7492844f 100644
>>>> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
>>>> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
>>>> @@ -266,8 +266,9 @@ static const struct drm_bridge_timings tfp410_default_timings = {
>>>>    static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>>>    {
>>>> +	struct fwnode_handle *fwnode = dev_fwnode(dvi->dev);
>>>>    	struct drm_bridge_timings *timings = &dvi->timings;
>>>> -	struct device_node *ep;
>>>> +	struct fwnode_handle *ep;
>>>>    	u32 pclk_sample = 0;
>>>>    	u32 bus_width = 24;
>>>>    	u32 deskew = 0;
>>>> @@ -288,14 +289,14 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>>>    	 * and EDGE pins. They are specified in DT through endpoint properties
>>>>    	 * and vendor-specific properties.
>>>>    	 */
>>>> -	ep = of_graph_get_endpoint_by_regs(dvi->dev->of_node, 0, 0);
>>>> +	ep = fwnode_graph_get_endpoint_by_id(fwnode, 0, 0, 0);
>>>>    	if (!ep)
>>>>    		return -EINVAL;
>>>>    	/* Get the sampling edge from the endpoint. */
>>>> -	of_property_read_u32(ep, "pclk-sample", &pclk_sample);
>>>> -	of_property_read_u32(ep, "bus-width", &bus_width);
>>>> -	of_node_put(ep);
>>>> +	fwnode_property_read_u32(ep, "pclk-sample", &pclk_sample);
>>>> +	fwnode_property_read_u32(ep, "bus-width", &bus_width);
>>>> +	fwnode_handle_put(ep);
>>>>    	timings->input_bus_flags = DRM_BUS_FLAG_DE_HIGH;
>>>> @@ -324,7 +325,7 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>>>    	}
>>>>    	/* Get the setup and hold time from vendor-specific properties. */
>>>> -	of_property_read_u32(dvi->dev->of_node, "ti,deskew", &deskew);
>>>> +	fwnode_property_read_u32(fwnode, "ti,deskew", &deskew);
>>>>    	if (deskew > 7)
>>>>    		return -EINVAL;
>>>> @@ -336,12 +337,12 @@ static int tfp410_parse_timings(struct tfp410 *dvi, bool i2c)
>>>>    static int tfp410_init(struct device *dev, bool i2c)
>>>>    {
>>>> -	struct device_node *node;
>>>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>>>>    	struct tfp410 *dvi;
>>>>    	int ret;
>>>> -	if (!dev->of_node) {
>>>> -		dev_err(dev, "device-tree data is missing\n");
>>>> +	if (!fwnode) {
>>>> +		dev_err(dev, "firmware data is missing\n");
>>>>    		return -ENXIO;
>>>>    	}
>>>> @@ -352,8 +353,8 @@ static int tfp410_init(struct device *dev, bool i2c)
>>>>    	dvi->dev = dev;
>>>>    	dev_set_drvdata(dev, dvi);
>>>> +	drm_bridge_set_node(&dvi->bridge, fwnode);
>>>>    	dvi->bridge.funcs = &tfp410_bridge_funcs;
>>>> -	dvi->bridge.of_node = dev->of_node;
>>>>    	dvi->bridge.timings = &dvi->timings;
>>>>    	dvi->bridge.type = DRM_MODE_CONNECTOR_DVID;
>>>> @@ -362,15 +363,15 @@ static int tfp410_init(struct device *dev, bool i2c)
>>>>    		return ret;
>>>>    	/* Get the next bridge, connected to port@1. */
>>>> -	node = of_graph_get_remote_node(dev->of_node, 1, -1);
>>>> -	if (!node)
>>>> -		return -ENODEV;
>>>> -
>>>> -	dvi->next_bridge = of_drm_find_bridge(node);
>>>> -	of_node_put(node);
>>>> -
>>>> -	if (!dvi->next_bridge)
>>>> +	dvi->next_bridge = drm_bridge_find_next_bridge_by_fwnode(fwnode, 1);
>>>> +	if (IS_ERR(dvi->next_bridge)) {
>>>> +		ret = PTR_ERR(dvi->next_bridge);
>>>> +		dev_err(dev, "Error in founding the next bridge: %d\n", ret);
>>>> +		return ret;
>>> Same comment regarding dev_err_probe().
>>>
>>> LGTM otherwise.
>>
>> My drm_bridge_find_next_bridge_by_fwnode() function won't return -EPROBE_DEFER,
>> this is known for sure. this can be used as a prior(priori) knowledge. This is
>> intentionally by design.
>>
>>
>> Calling the dev_err_probe() just introduce extra overhead on non EPROBE_DEFER
>> cases. Hence, It is useless to use dev_err_probe() at here.
>>
>>
>>>> +	} else if (!dvi->next_bridge) {
>>>> +		dev_dbg(dev, "Next bridge not found, deferring probe\n");
>>>>    		return -EPROBE_DEFER;
>>> Looking at the bolerplate code, I think it would be better to make
>>> drm_bridge_find_next_bridge_by_fwnode() reutrn -EPROBE_DEFER on its own.
>>>
>> The drm_bridge_find_next_bridge_by_fwnode() function itself can not
>> reliable detect if the driver(the remote bridge) already probed or not.
>>
>> Hence, as a core helper function, we can not guarantee that return
>> -EPROBE_DEFER is always correct.
>>
>> While, return NULL is always correct. The NULL can stand for two meanings.
>> One is that the next bridge is really don't exist, may happen when the
>> caller provided a wrong fwnode argument.
> Please take a look at drm_of_find_panel_or_bridge().


The function name seems to hint that a panel is not a bridge, while panel can be drm bridge.
display connector can also be a bridge, display connector can also be within a bridge.
There also has HPD fake bridge.

so maybe drm_of_find_panel_or_connector_or_hpd_bridge()?


My function intend to use one word "bridge" stand for all at this moment.


> Returning specific
> error code is always better than returning just NULL. As you have
> pointed yourself, there are (at least) two cases when your function
> returns NULL. Caller can not identify them unless the function returns
> proper error code.

No, you miss the point.

The point is that the caller *don't need* to identify them.
Just tears down (quit with -EPROBE_DEFER returned) is enough.
This is also what's other drivers do.

>
>> Another case is that the next bridge exists but not probed yet, and
>> drm_bridge_find_next_bridge_by_fwnode() can return NULL when it gets called
>> too early.
>>
>> Therefore, it is better to left to the users of this function to process
>> the NULL return value. As driver instances has some extra prior knowledge.
>> And can be controlled by drm bridge driver author.
> he driver has no prior knowledge if there is a remote fwnode/ofnode or
> if there is none.

No,

  
As I have told you several time, the DT/fwnode graph speak everything.
Display bridge driver can query firmware(DT/ACPI) to know if the next bridge
is present, is it really meant to be used and how many bridges in the chains.

If there has complete OF/fwnode graph and the graph say that there has next
bridge in the chain. Then the driver has to return -EPROBE_DEFER if he can't
find the next.

And the most important thing is that it is the bridge drivers responsibility
or authority to take whatever actions when drm_bridge_find_next_bridge_by_fwnode()
can return NULL. Core helpers is meant to be lightweight only, there no need
to introduce this overhead.
  
Ok. I think I have been patient long enough.
You questions and/or reviews have been given polite replies, that's it.

>>>> +	}
>>>>    	/* Get the powerdown GPIO. */
>>>>    	dvi->powerdown = devm_gpiod_get_optional(dev, "powerdown",
>>>> @@ -422,10 +423,10 @@ static struct platform_driver tfp410_platform_driver = {
>>>>    /* There is currently no i2c functionality. */
>>>>    static int tfp410_i2c_probe(struct i2c_client *client)
>>>>    {
>>>> +	struct fwnode_handle *fwnode = dev_fwnode(&client->dev);
>>>>    	int reg;
>>>> -	if (!client->dev.of_node ||
>>>> -	    of_property_read_u32(client->dev.of_node, "reg", &reg)) {
>>>> +	if (!fwnode || fwnode_property_read_u32(fwnode, "reg", &reg)) {
>>>>    		dev_err(&client->dev,
>>>>    			"Can't get i2c reg property from device-tree\n");
>>>>    		return -ENXIO;
>>>> -- 
>>>> 2.34.1
>>>>
>> -- 
>> Best regards,
>> Sui
>>
-- 
Best regards,
Sui

[PATCH v4 9/9] drm/bridge: tfp410: Add platform module alias

[Sui Jingfeng]

Otherwise when compiled as module, this driver will not be probed on
non-DT environment. This is a fundamential step to make this driver
truely OF-independent.

Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index 58dc7492844f..6a0a0bbe3c6b 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -504,3 +504,4 @@ module_exit(tfp410_module_exit);
 MODULE_AUTHOR("Jyri Sarha <jsarha@ti.com>");
 MODULE_DESCRIPTION("TI TFP410 DVI bridge driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:tfp410");
-- 
2.34.1

Re: [PATCH v4 9/9] drm/bridge: tfp410: Add platform module alias

[Krzysztof Kozlowski]

On 22/04/2024 21:19, Sui Jingfeng wrote:
> Otherwise when compiled as module, this driver will not be probed on
> non-DT environment. This is a fundamential step to make this driver
> truely OF-independent.

NAK.

You should not need MODULE_ALIAS() in normal cases. If you need it,
usually it means your device ID table is wrong (e.g. misses either
entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
for incomplete ID table.

Just check your aliases and look what is there. You already have
i2c:tfp410 alias. If you need platform one, for some reason, explain
what is your matching path and add appropriate ID table. With that
explanation, of course.

Best regards,
Krzysztof

Re: [PATCH v4 9/9] drm/bridge: tfp410: Add platform module alias

[Sui Jingfeng]

Hi,

On 2024/4/23 16:05, Krzysztof Kozlowski wrote:
> On 22/04/2024 21:19, Sui Jingfeng wrote:
>> Otherwise when compiled as module, this driver will not be probed on
>> non-DT environment. This is a fundamential step to make this driver
>> truely OF-independent.
> NAK.


:( ...


>
> You should not need MODULE_ALIAS() in normal cases. If you need it,
> usually it means your device ID table is wrong (e.g. misses either
> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
> for incomplete ID table.


I think I could give you a reason.

1) When compile this driver into linux kernel

This driver will be probed if we create a platform device manually with the name "tfp410".
This is also true for the display-connector driver(drivers/gpu/drm/bridge/display-connector.c),
see patch 0005 of this series  and the simple-bridge driver(drivers/gpu/drm/bridge/simple-bridge.c)
see patch 0003 of this series.

2) But when compile this driver as module, creating a platform device manually with the same name
*won't* make those platform driver probed. I think, this is *inconsistent behavior*. Therefore, I
add MODULE_ALIAS() to keep the behavior consistent. That is, a driver, should be able to be probed
regardless it is compile as a kernel module or it is built into the kernel.


> Just check your aliases and look what is there. You already have
> i2c:tfp410 alias.

Right, but the i2c:tfp410 alias only guarantee the driver can be probed successfully
when tfp410 working as I2C slave. tfp410 can also works as a transparent bridge.


>   If you need platform one, for some reason, explain
> what is your matching path and add appropriate ID table. With that
> explanation, of course.

When tfp410 works as a transparent bridge. The device itself is just a platform device.
similar with the display-connector.c and simple-bridge.c.

It is not discoverable by the system on non-DT environment, this is the root problem.
I said the various display bridges drivers are fully DT dependent, Dimtry didn't agree!

He said "I can not agree here. It doesn't depend on DT." and then asks me to developing
some other solution witch could preserve code sharing. So here it is.


> Best regards,
> Krzysztof
>
-- 
Best regards,
Sui

Re: [PATCH v4 9/9] drm/bridge: tfp410: Add platform module alias

[Krzysztof Kozlowski]

On 23/04/2024 12:12, Sui Jingfeng wrote:
> Hi,
> 
> On 2024/4/23 16:05, Krzysztof Kozlowski wrote:
>> On 22/04/2024 21:19, Sui Jingfeng wrote:
>>> Otherwise when compiled as module, this driver will not be probed on
>>> non-DT environment. This is a fundamential step to make this driver
>>> truely OF-independent.
>> NAK.
> 
> 
> :( ...
> 
> 
>>
>> You should not need MODULE_ALIAS() in normal cases. If you need it,
>> usually it means your device ID table is wrong (e.g. misses either
>> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
>> for incomplete ID table.
> 
> 
> I think I could give you a reason.
> 
> 1) When compile this driver into linux kernel
> 
> This driver will be probed if we create a platform device manually with the name "tfp410".

Then do not create devices manually. This is not y2000 to use board files.

> This is also true for the display-connector driver(drivers/gpu/drm/bridge/display-connector.c),
> see patch 0005 of this series  and the simple-bridge driver(drivers/gpu/drm/bridge/simple-bridge.c)
> see patch 0003 of this series.

They have the same problem.

> 
> 2) But when compile this driver as module, creating a platform device manually with the same name
> *won't* make those platform driver probed. I think, this is *inconsistent behavior*. Therefore, I
> add MODULE_ALIAS() to keep the behavior consistent. That is, a driver, should be able to be probed
> regardless it is compile as a kernel module or it is built into the kernel.
> 

That's obvious. Please focus on the actual issue here.

> 
>> Just check your aliases and look what is there. You already have
>> i2c:tfp410 alias.
> 
> Right, but the i2c:tfp410 alias only guarantee the driver can be probed successfully
> when tfp410 working as I2C slave. tfp410 can also works as a transparent bridge.

So which bus or driver instantiates the device? What use case is this?

> 
> 
>>   If you need platform one, for some reason, explain
>> what is your matching path and add appropriate ID table. With that
>> explanation, of course.
> 
> When tfp410 works as a transparent bridge. The device itself is just a platform device.
> similar with the display-connector.c and simple-bridge.c.
> 
> It is not discoverable by the system on non-DT environment, this is the root problem.
> I said the various display bridges drivers are fully DT dependent, Dimtry didn't agree!
> 
> He said "I can not agree here. It doesn't depend on DT." and then asks me to developing
> some other solution witch could preserve code sharing. So here it is.


You wrote long message without actually reading my answer early. I
already gave you the solution. Address that one.

Best regards,
Krzysztof

Re: [PATCH v4 9/9] drm/bridge: tfp410: Add platform module alias

[Sui Jingfeng]

On 2024/4/23 18:20, Krzysztof Kozlowski wrote:
> On 23/04/2024 12:12, Sui Jingfeng wrote:
>> Hi,
>>
>> On 2024/4/23 16:05, Krzysztof Kozlowski wrote:
>>> On 22/04/2024 21:19, Sui Jingfeng wrote:
>>>> Otherwise when compiled as module, this driver will not be probed on
>>>> non-DT environment. This is a fundamential step to make this driver
>>>> truely OF-independent.
>>> NAK.
>>
>> :( ...
>>
>>
>>> You should not need MODULE_ALIAS() in normal cases. If you need it,
>>> usually it means your device ID table is wrong (e.g. misses either
>>> entries or MODULE_DEVICE_TABLE()). MODULE_ALIAS() is not a substitute
>>> for incomplete ID table.
>>
>> I think I could give you a reason.
>>
>> 1) When compile this driver into linux kernel
>>
>> This driver will be probed if we create a platform device manually with the name "tfp410".
> Then do not create devices manually. This is not y2000 to use board files.


Not exactly, creating devices manually can be modern and fancy approach.
Complex driver need to create devices manually to paper over the issue(-EPROBE_DEFER)
raised with cross drivers subsystem design. Or for the purpose of better modularization.
See etnaviv, vkms, efifb, aux-bridge, aux-bus, ect.

OK, I know what you means here.

>> This is also true for the display-connector driver(drivers/gpu/drm/bridge/display-connector.c),
>> see patch 0005 of this series  and the simple-bridge driver(drivers/gpu/drm/bridge/simple-bridge.c)
>> see patch 0003 of this series.
> They have the same problem.
>
>> 2) But when compile this driver as module, creating a platform device manually with the same name
>> *won't* make those platform driver probed. I think, this is *inconsistent behavior*. Therefore, I
>> add MODULE_ALIAS() to keep the behavior consistent. That is, a driver, should be able to be probed
>> regardless it is compile as a kernel module or it is built into the kernel.
>>
> That's obvious. Please focus on the actual issue here.
>
>>> Just check your aliases and look what is there. You already have
>>> i2c:tfp410 alias.
>> Right, but the i2c:tfp410 alias only guarantee the driver can be probed successfully
>> when tfp410 working as I2C slave. tfp410 can also works as a transparent bridge.
> So which bus or driver instantiates the device? What use case is this?
>
>>
>>>    If you need platform one, for some reason, explain
>>> what is your matching path and add appropriate ID table. With that
>>> explanation, of course.
>> When tfp410 works as a transparent bridge. The device itself is just a platform device.
>> similar with the display-connector.c and simple-bridge.c.
>>
>> It is not discoverable by the system on non-DT environment, this is the root problem.
>> I said the various display bridges drivers are fully DT dependent, Dimtry didn't agree!
>>
>> He said "I can not agree here. It doesn't depend on DT." and then asks me to developing
>> some other solution witch could preserve code sharing. So here it is.
>
> You wrote long message without actually reading my answer early. I
> already gave you the solution. Address that one.

Use MODULE_DEVICE_TABLE() instead? OK, I understand then. Thanks a lot 
for the education.


> Best regards,
> Krzysztof
>
-- 
Best regards,
Sui

Re: [PATCH v4 9/9] drm/bridge: tfp410: Add platform module alias

[Krzysztof Kozlowski]

On 23/04/2024 12:44, Sui Jingfeng wrote:
>>>>    If you need platform one, for some reason, explain
>>>> what is your matching path and add appropriate ID table. With that
>>>> explanation, of course.
>>> When tfp410 works as a transparent bridge. The device itself is just a platform device.
>>> similar with the display-connector.c and simple-bridge.c.
>>>
>>> It is not discoverable by the system on non-DT environment, this is the root problem.
>>> I said the various display bridges drivers are fully DT dependent, Dimtry didn't agree!
>>>
>>> He said "I can not agree here. It doesn't depend on DT." and then asks me to developing
>>> some other solution witch could preserve code sharing. So here it is.
>>
>> You wrote long message without actually reading my answer early. I
>> already gave you the solution. Address that one.
> 
> Use MODULE_DEVICE_TABLE() instead? OK, I understand then. Thanks a lot 
> for the education.


Yes, at least for something which is real driver.

Best regards,
Krzysztof