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
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 <[email protected]>
---
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 <[email protected]>");
MODULE_DESCRIPTION("Simple DRM bridge driver");
MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:simple-bridge");
--
2.25.1
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 <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>");
MODULE_DESCRIPTION("Display connector driver");
MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:display-connector");
--
2.25.1
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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 <[email protected]>");
> MODULE_DESCRIPTION("Simple DRM bridge driver");
> MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:simple-bridge");
This is an unrelated change.
--
Regards,
Laurent Pinchart
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
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 <[email protected]>
>> ---
>> 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
>>
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<[email protected]>");
>> 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.
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
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 <[email protected]>
> >> ---
> >> 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
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 <[email protected]>
> > > ---
> > > 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
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