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@xxxxxxxxx>
> > ---
> >   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