| Message ID | 20240307172334.1753343-2-sui.jingfeng@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Allow using fwnode API to get the next bridge | expand |
On Thu, 7 Mar 2024 at 19:23, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Currently, the various drm bridge drivers relay on OF infrastructures to > works very well. Yet there are platforms and/or don not has OF support. > Such as virtual display drivers, USB display apapters and ACPI based > systems etc. Add fwnode based helpers to fill the niche, this may allows > part of the drm display bridge drivers to work across systems. As the > fwnode based API has wider coverage than DT, it can be used on all systems > in theory. Assumed that the system has valid fwnode graphs established > before drm bridge driver is probed, the fwnode graphs are compatible with > the OF graph. > > Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev> > --- > drivers/gpu/drm/drm_bridge.c | 68 ++++++++++++++++++++++++++++++++++++ > include/drm/drm_bridge.h | 16 +++++++++ > 2 files changed, 84 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 521a71c61b16..1b2d3b89a61d 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -1348,6 +1348,74 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np) > EXPORT_SYMBOL(of_drm_find_bridge); > #endif > > +/** > + * drm_bridge_find_by_fwnode - Find the bridge corresponding to the associated fwnode > + * > + * @fwnode: fwnode for which to find the matching drm_bridge > + * > + * This function looks up a drm_bridge based on its associated fwnode. > + * > + * RETURNS: > + * A reference to the drm_bridge if found, otherwise return NULL. > + */ Please take a look at Documentation/doc-guide/kernel-doc.rst. > +struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode) > +{ > + struct drm_bridge *ret = NULL; > + struct drm_bridge *bridge; > + > + if (!fwnode) > + return NULL; > + > + mutex_lock(&bridge_lock); > + > + list_for_each_entry(bridge, &bridge_list, list) { > + if (bridge->fwnode == fwnode) { > + ret = bridge; > + break; > + } > + } > + > + mutex_unlock(&bridge_lock); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_bridge_find_by_fwnode); EXPORT_SYMBOL_GPL > + > +/** > + * drm_bridge_find_next_bridge_by_fwnode - get the next bridge by fwnode > + * @fwnode: fwnode pointer to the current bridge. > + * @port: identifier of the port node of the next bridge is connected. > + * > + * This function find the next bridge at the current bridge node, assumed > + * that there has valid fwnode graph established. > + * > + * RETURNS: > + * A reference to the drm_bridge if found, %NULL if not found. > + * Otherwise return a negative error code. > + */ > +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(drm_bridge_find_next_bridge_by_fwnode); EXPORT_SYMBOL_GPL > + > 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 3606e1a7f965..d4c95afdd662 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; In my opinion, if you are adding fwnode, we can drop of_node completely. There is no need to keep both of them. > + /** @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 >
Hi, On 2024/3/8 02:43, Dmitry Baryshkov wrote: >> + >> 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 3606e1a7f965..d4c95afdd662 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; > In my opinion, if you are adding fwnode, we can drop of_node > completely. There is no need to keep both of them. But the 'struct device' have both of them contained, we should *follow the core*, right? They are two major firmware subsystems (DT and ACPT), both are great and large, right? Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge are mainly stand for display bridges device. And also to reflect what you said: "to reflect the hardware perfectly" and remove some boilerplate. I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime and Laurent are the advanced programmers who is good enough to do such things, maybe you can ask them for help? Beside this, other reviews are acceptable and will be fixed at the next version, thanks a lot for review. Best Regards, Sui
On 2024/3/8 03:20, Sui Jingfeng wrote:
> I think the drm_bridge should embeds 'struct device'
'struct device' -> 'struct device *'
On Thu, 7 Mar 2024 at 21:20, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2024/3/8 02:43, Dmitry Baryshkov wrote: > >> + > >> 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 3606e1a7f965..d4c95afdd662 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; > > In my opinion, if you are adding fwnode, we can drop of_node > > completely. There is no need to keep both of them. > > > But the 'struct device' have both of them contained, we should *follow the core*, right? > They are two major firmware subsystems (DT and ACPT), both are great and large, right? > Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge > are mainly stand for display bridges device. And also to reflect what you said: "to > reflect the hardware perfectly" and remove some boilerplate. struct device contains both because it is at the root of the hierarchy and it should support both API. drm_bridge is a consumer, so it's fine to have just one. > > I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime > and Laurent are the advanced programmers who is good enough to do such things, maybe > you can ask them for help? Well, you picked up the task ;-) But really, there is nothing so hard about it: - Change of_node to fw_node, apply an automatic patch changing this in bridge drivers. - Make drm_of_bridge functions convert passed of_node and comp After this we can start cleaning up bridge drivers to use fw_node API natively as you did in your patches 2-4. > > Beside this, other reviews are acceptable and will be fixed at the next version, > thanks a lot for review. > > > Best Regards, > Sui >
Hi, On 2024/3/8 03:37, Dmitry Baryshkov wrote: > On Thu, 7 Mar 2024 at 21:20, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> Hi, >> >> >> On 2024/3/8 02:43, Dmitry Baryshkov wrote: >>>> + >>>> 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 3606e1a7f965..d4c95afdd662 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; >>> In my opinion, if you are adding fwnode, we can drop of_node >>> completely. There is no need to keep both of them. >> >> But the 'struct device' have both of them contained, we should *follow the core*, right? >> They are two major firmware subsystems (DT and ACPT), both are great and large, right? >> Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge >> are mainly stand for display bridges device. And also to reflect what you said: "to >> reflect the hardware perfectly" and remove some boilerplate. > struct device contains both because it is at the root of the hierarchy > and it should support both API. drm_bridge is a consumer, so it's fine > to have just one. > What I really means is that if one day a 'struct device *dev' is embedded into struct drm_bridge, we only need to improve(modify) the implement ofdrm_bridge_set_node(). This is *key point*. Currently, they(of_node and fwnode) point to the same thing, it is also fine. For the various drm bridge drivers implementations, the 'struct drm_bridge' is also belong to the driver core category. drm bridges are also play the producer role. >> I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime >> and Laurent are the advanced programmers who is good enough to do such things, maybe >> you can ask them for help? > Well, you picked up the task ;-) Well, my subject(title) is "*Allow* to use fwnode based API to get drm bridges". not "Replace 'OF' with fwnode", My task is to provide an alternative solution for the possible users. And to help improve code sharing and improve code reuse. And remove some boilerplate. Others things beyond that is not being taken by anybody. Thanks. > > But really, there is nothing so hard about it: > - Change of_node to fw_node, apply an automatic patch changing this in > bridge drivers. > - Make drm_of_bridge functions convert passed of_node and comp > > After this we can start cleaning up bridge drivers to use fw_node API > natively as you did in your patches 2-4. Yes, it's not so hard. But I'm a little busy due to other downstream developing tasks. Sorry, very sorry! During the talk with you, I observed that you are very good at fwnode domain. Are you willing to help the community to do something? For example, currently the modern drm bridge framework is corrupted by legacy implement, is it possible for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c which create a drm connector manually, not modernized yet and it's DT dependent. So, there are a lot things to do. Best Regards, Sui.
On Thu, 7 Mar 2024 at 22:32, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2024/3/8 03:37, Dmitry Baryshkov wrote: > > On Thu, 7 Mar 2024 at 21:20, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >> Hi, > >> > >> > >> On 2024/3/8 02:43, Dmitry Baryshkov wrote: > >>>> + > >>>> 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 3606e1a7f965..d4c95afdd662 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; > >>> In my opinion, if you are adding fwnode, we can drop of_node > >>> completely. There is no need to keep both of them. > >> > >> But the 'struct device' have both of them contained, we should *follow the core*, right? > >> They are two major firmware subsystems (DT and ACPT), both are great and large, right? > >> Personally, I think the drm_bridge should embeds 'struct device', after all, drm bridge > >> are mainly stand for display bridges device. And also to reflect what you said: "to > >> reflect the hardware perfectly" and remove some boilerplate. > > struct device contains both because it is at the root of the hierarchy > > and it should support both API. drm_bridge is a consumer, so it's fine > > to have just one. > > > > What I really means is that > if one day a 'struct device *dev' is embedded into struct drm_bridge, > we only need to improve(modify) the implement ofdrm_bridge_set_node(). > This is *key point*. Currently, they(of_node and fwnode) point to the > same thing, it is also fine. > > For the various drm bridge drivers implementations, the 'struct drm_bridge' > is also belong to the driver core category. drm bridges are also play the > producer role. > > >> I think I'm not good enough to do such a big refactor, sorry. I believe that Maxime > >> and Laurent are the advanced programmers who is good enough to do such things, maybe > >> you can ask them for help? > > Well, you picked up the task ;-) > > > Well, my subject(title) is "*Allow* to use fwnode based API to get drm bridges". > not "Replace 'OF' with fwnode", My task is to provide an alternative solution > for the possible users. And to help improve code sharing and improve code reuse. > And remove some boilerplate. Others things beyond that is not being taken by > anybody. Thanks. Ok, I'd defer this to the maintainers decision then. > > > > > > But really, there is nothing so hard about it: > > - Change of_node to fw_node, apply an automatic patch changing this in > > bridge drivers. > > - Make drm_of_bridge functions convert passed of_node and comp > > > > After this we can start cleaning up bridge drivers to use fw_node API > > natively as you did in your patches 2-4. > > > Yes, it's not so hard. But I'm a little busy due to other downstream developing > tasks. Sorry, very sorry! > > During the talk with you, I observed that you are very good at fwnode domain. > Are you willing to help the community to do something? For example, currently > the modern drm bridge framework is corrupted by legacy implement, is it possible > for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c > which create a drm connector manually, not modernized yet and it's DT dependent. > So, there are a lot things to do. Actually, lontium-lt9611uxc.c does both of that ;-) It supports creating a connector and it as well supports attaching to a chain without creating a connector. Pretty nice, isn't it?
Hi, On 2024/3/8 04:40, Dmitry Baryshkov wrote: >>> But really, there is nothing so hard about it: >>> - Change of_node to fw_node, apply an automatic patch changing this in >>> bridge drivers. >>> - Make drm_of_bridge functions convert passed of_node and comp >>> >>> After this we can start cleaning up bridge drivers to use fw_node API >>> natively as you did in your patches 2-4. >> Yes, it's not so hard. But I'm a little busy due to other downstream developing >> tasks. Sorry, very sorry! >> >> During the talk with you, I observed that you are very good at fwnode domain. >> Are you willing to help the community to do something? For example, currently >> the modern drm bridge framework is corrupted by legacy implement, is it possible >> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c >> which create a drm connector manually, not modernized yet and it's DT dependent. >> So, there are a lot things to do. > Actually, lontium-lt9611uxc.c does both of that
On 2024/3/8 05:09, Sui Jingfeng wrote:
> a most experienced
A most experienced programmer & developer.
Hi, On 2024/3/8 04:40, Dmitry Baryshkov wrote: >>> But really, there is nothing so hard about it: >>> - Change of_node to fw_node, apply an automatic patch changing this in >>> bridge drivers. >>> - Make drm_of_bridge functions convert passed of_node and comp >>> >>> After this we can start cleaning up bridge drivers to use fw_node API >>> natively as you did in your patches 2-4. >> Yes, it's not so hard. But I'm a little busy due to other downstream developing >> tasks. Sorry, very sorry! >> >> During the talk with you, I observed that you are very good at fwnode domain. >> Are you willing to help the community to do something? For example, currently >> the modern drm bridge framework is corrupted by legacy implement, is it possible >> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c >> which create a drm connector manually, not modernized yet and it's DT dependent. >> So, there are a lot things to do. > Actually, lontium-lt9611uxc.c does both of that
On Sat, 9 Mar 2024 at 11:33, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2024/3/8 04:40, Dmitry Baryshkov wrote: > >>> But really, there is nothing so hard about it: > >>> - Change of_node to fw_node, apply an automatic patch changing this in > >>> bridge drivers. > >>> - Make drm_of_bridge functions convert passed of_node and comp > >>> > >>> After this we can start cleaning up bridge drivers to use fw_node API > >>> natively as you did in your patches 2-4. > >> Yes, it's not so hard. But I'm a little busy due to other downstream developing > >> tasks. Sorry, very sorry! > >> > >> During the talk with you, I observed that you are very good at fwnode domain. > >> Are you willing to help the community to do something? For example, currently > >> the modern drm bridge framework is corrupted by legacy implement, is it possible > >> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c > >> which create a drm connector manually, not modernized yet and it's DT dependent. > >> So, there are a lot things to do. > > Actually, lontium-lt9611uxc.c does both of that
Hi, On 2024/3/9 18:39, Dmitry Baryshkov wrote: > On Sat, 9 Mar 2024 at 11:33, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: >> Hi, >> >> >> On 2024/3/8 04:40, Dmitry Baryshkov wrote: >>>>> But really, there is nothing so hard about it: >>>>> - Change of_node to fw_node, apply an automatic patch changing this in >>>>> bridge drivers. >>>>> - Make drm_of_bridge functions convert passed of_node and comp >>>>> >>>>> After this we can start cleaning up bridge drivers to use fw_node API >>>>> natively as you did in your patches 2-4. >>>> Yes, it's not so hard. But I'm a little busy due to other downstream developing >>>> tasks. Sorry, very sorry! >>>> >>>> During the talk with you, I observed that you are very good at fwnode domain. >>>> Are you willing to help the community to do something? For example, currently >>>> the modern drm bridge framework is corrupted by legacy implement, is it possible >>>> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c >>>> which create a drm connector manually, not modernized yet and it's DT dependent. >>>> So, there are a lot things to do. >>> Actually, lontium-lt9611uxc.c does both of that
Hi, On 2024/3/9 18:39, Dmitry Baryshkov wrote: >> The code path of "creating a connector" plus the code path of "not creating a connector" >> forms a 'side-by-side' implementation imo. >> >> Besides, I have repeated many times: the DT already speak everything. >> Device drivers can completely know if there is a display connector OF device created and how many >> display bridges in the whole chain. If there are connector device node in the DT, then it should >> has a device driver bound to it(instead of create it manually) for a perfect implementation. As >> you told me we should not*over play* the device-driver model, right? > Please, don't mix the connector node in DT and the drm_connector. If > you check the code, you will see that the driver for hdmi-connector, > dp-connector and other such devices creates a drm_bridge. OK, I'm not mixed them, I'm very clear from the very beginning. I have checked the code years ago. Let's make it clear by iterating one more time: If DT contains one or more HDMI connector node, then there will be one or more display connector platform devices created by OF core, Then, according to your "don't overplay device-driver model" criterion or modern drm bridge standard, we shouldn't create a display connector instance in the drm birdge driver, right? As otherwise we will have two display connector driver (or code) for a single entity, right? Another side effect is that when you create a hdmi display connector instance manually without reference to the DT, then *you made an assumption!*. But there are users who have don't a different need(or different typology), for example, they need to read edid directly from the KMS driver side. This may because the i2c bus is directly connected to the connector part, but the display bridge's I2C slave interface. sii9022, it66121 and tfp410 support this kind of usage. So the real problem is that it is a policy level code when you creating a hdmi display connector instance manually without reference to the DT in a common drm bridge driver, not a mechanism.
On Sat, 9 Mar 2024 at 13:25, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2024/3/9 18:39, Dmitry Baryshkov wrote: > > On Sat, 9 Mar 2024 at 11:33, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > >> Hi, > >> > >> > >> On 2024/3/8 04:40, Dmitry Baryshkov wrote: > >>>>> But really, there is nothing so hard about it: > >>>>> - Change of_node to fw_node, apply an automatic patch changing this in > >>>>> bridge drivers. > >>>>> - Make drm_of_bridge functions convert passed of_node and comp > >>>>> > >>>>> After this we can start cleaning up bridge drivers to use fw_node API > >>>>> natively as you did in your patches 2-4. > >>>> Yes, it's not so hard. But I'm a little busy due to other downstream developing > >>>> tasks. Sorry, very sorry! > >>>> > >>>> During the talk with you, I observed that you are very good at fwnode domain. > >>>> Are you willing to help the community to do something? For example, currently > >>>> the modern drm bridge framework is corrupted by legacy implement, is it possible > >>>> for us to migrate them to modern? Instead of rotting there? such as the lontium-lt9611uxc.c > >>>> which create a drm connector manually, not modernized yet and it's DT dependent. > >>>> So, there are a lot things to do. > >>> Actually, lontium-lt9611uxc.c does both of that
On Sat, 9 Mar 2024 at 14:03, Sui Jingfeng <sui.jingfeng@linux.dev> wrote: > > Hi, > > > On 2024/3/9 18:39, Dmitry Baryshkov wrote: > >> The code path of "creating a connector" plus the code path of "not creating a connector" > >> forms a 'side-by-side' implementation imo. > >> > >> Besides, I have repeated many times: the DT already speak everything. > >> Device drivers can completely know if there is a display connector OF device created and how many > >> display bridges in the whole chain. If there are connector device node in the DT, then it should > >> has a device driver bound to it(instead of create it manually) for a perfect implementation. As > >> you told me we should not*over play* the device-driver model, right? > > Please, don't mix the connector node in DT and the drm_connector. If > > you check the code, you will see that the driver for hdmi-connector, > > dp-connector and other such devices creates a drm_bridge. > > > OK, I'm not mixed them, I'm very clear from the very beginning. I have checked > the code years ago. Let's make it clear by iterating one more time: > > If DT contains one or more HDMI connector node, then there will be one or > more display connector platform devices created by OF core, Then, according to > your "don't overplay device-driver model" criterion or modern drm bridge standard, > we shouldn't create a display connector instance in the drm birdge driver, right? Yeah, if the platform is updated, yes, we do. If there is an hdmi-connector node, I can only assume that the DRM driver also has been updated to pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR. In such case the lt9611uxc driver will not create the drm_connector and everything works as expected. If this is one of the legacy platforms, the DRM driver will not pass the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, but at the same time the DT will not have the connector node. > > As otherwise we will have two display connector driver (or code) for a single entity, > right? > > Another side effect is that > when you create a hdmi display connector instance manually without reference to the > DT, then *you made an assumption!*. But there are users who have don't a different > need(or different typology), for example, they need to read edid directly from the > KMS driver side. This may because the i2c bus is directly connected to the connector > part, but the display bridge's I2C slave interface. sii9022, it66121 and tfp410 support > this kind of usage. > > So the real problem is that it is a policy level code when you creating a hdmi > display connector instance manually without reference to the DT in a common drm bridge > driver, not a mechanism. Only if requested by the DRM driver itself. -- With best wishes Dmitry
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 521a71c61b16..1b2d3b89a61d 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -1348,6 +1348,74 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np) EXPORT_SYMBOL(of_drm_find_bridge); #endif +/** + * drm_bridge_find_by_fwnode - Find the bridge corresponding to the associated fwnode + * + * @fwnode: fwnode for which to find the matching drm_bridge + * + * This function looks up a drm_bridge based on its associated fwnode. + * + * RETURNS: + * A reference to the drm_bridge if found, otherwise return NULL. + */ +struct drm_bridge *drm_bridge_find_by_fwnode(struct fwnode_handle *fwnode) +{ + struct drm_bridge *ret = NULL; + struct drm_bridge *bridge; + + if (!fwnode) + return NULL; + + mutex_lock(&bridge_lock); + + list_for_each_entry(bridge, &bridge_list, list) { + if (bridge->fwnode == fwnode) { + ret = bridge; + break; + } + } + + mutex_unlock(&bridge_lock); + + return ret; +} +EXPORT_SYMBOL(drm_bridge_find_by_fwnode); + +/** + * drm_bridge_find_next_bridge_by_fwnode - get the next bridge by fwnode + * @fwnode: fwnode pointer to the current bridge. + * @port: identifier of the port node of the next bridge is connected. + * + * This function find the next bridge at the current bridge node, assumed + * that there has valid fwnode graph established. + * + * RETURNS: + * A reference to the drm_bridge if found, %NULL if not found. + * Otherwise return a negative error code. + */ +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(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 3606e1a7f965..d4c95afdd662 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
Currently, the various drm bridge drivers relay on OF infrastructures to works very well. Yet there are platforms and/or don not has OF support. Such as virtual display drivers, USB display apapters and ACPI based systems etc. Add fwnode based helpers to fill the niche, this may allows part of the drm display bridge drivers to work across systems. As the fwnode based API has wider coverage than DT, it can be used on all systems in theory. Assumed that the system has valid fwnode graphs established before drm bridge driver is probed, the fwnode graphs are compatible with the OF graph. Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev> --- drivers/gpu/drm/drm_bridge.c | 68 ++++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 16 +++++++++ 2 files changed, 84 insertions(+)