[PATCH 01/30] drm: Introduce drm_bridge_init()

[PATCH 01/30] drm: Introduce drm_bridge_init()

[Mihail Atanassov]

A simple convenience function to initialize the struct drm_bridge.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     |  4 ++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cba537c99e43..cbe680aa6eac 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 
+/**
+ * drm_bridge_init - initialise a drm_bridge structure
+ *
+ * @bridge: bridge control structure
+ * @funcs: control functions
+ * @dev: device
+ * @timings: timing specification for the bridge; optional (may be NULL)
+ * @driver_private: pointer to the bridge driver internal context (may be NULL)
+ */
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
+		     const struct drm_bridge_funcs *funcs,
+		     const struct drm_bridge_timings *timings,
+		     void *driver_private)
+{
+	WARN_ON(!funcs);
+
+	bridge->dev = NULL;
+	bridge->encoder = NULL;
+	bridge->next = NULL;
+
+#ifdef CONFIG_OF
+	bridge->of_node = dev->of_node;
+#endif
+	bridge->timings = timings;
+	bridge->funcs = funcs;
+	bridge->driver_private = driver_private;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
 /**
  * drm_bridge_attach - attach the bridge to an encoder's chain
  *
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index c0a2286a81e9..d6d9d5301551 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -402,6 +402,10 @@ struct drm_bridge {
 
 void drm_bridge_add(struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);
+void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
+		     const struct drm_bridge_funcs *funcs,
+		     const struct drm_bridge_timings *timings,
+		     void *driver_private);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous);
-- 
2.23.0

Re: [PATCH 01/30] drm: Introduce drm_bridge_init()

[Daniel Vetter]

On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
> A simple convenience function to initialize the struct drm_bridge.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>

The commit message here leaves figuring out why we need this to the
reader. Reading ahead the reasons seems to be to roll out bridge->dev
setting for everyone, so that we can set the device_link. Please explain
that in the commit message so the patch is properly motivated.

> ---
>  drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  4 ++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cba537c99e43..cbe680aa6eac 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  }
>  EXPORT_SYMBOL(drm_bridge_remove);
>  
> +/**
> + * drm_bridge_init - initialise a drm_bridge structure
> + *
> + * @bridge: bridge control structure
> + * @funcs: control functions
> + * @dev: device
> + * @timings: timing specification for the bridge; optional (may be NULL)
> + * @driver_private: pointer to the bridge driver internal context (may be NULL)

Please also sprinkle some links to this new function to relevant places,
I'd add at least:

"Drivers should call drm_bridge_init() first." to the kerneldoc for
drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as
the undo function.

And perhaps a longer paragraph to &struct drm_bridge:

"Bridge drivers should call drm_bridge_init() to initialized a bridge
driver, and then register it with drm_bridge_add().

"Users of bridges link a bridge driver into their overall display output
pipeline by calling drm_bridge_attach()."

> + */
> +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> +		     const struct drm_bridge_funcs *funcs,
> +		     const struct drm_bridge_timings *timings,
> +		     void *driver_private)
> +{
> +	WARN_ON(!funcs);
> +
> +	bridge->dev = NULL;

Given that the goal here is to get bridge->dev set up, why not

	WARN_ON(!dev);
	bridge->dev = dev;

That should help us to really move forward with all this.
-Daniel

> +	bridge->encoder = NULL;
> +	bridge->next = NULL;
> +
> +#ifdef CONFIG_OF
> +	bridge->of_node = dev->of_node;
> +#endif
> +	bridge->timings = timings;
> +	bridge->funcs = funcs;
> +	bridge->driver_private = driver_private;
> +}
> +EXPORT_SYMBOL(drm_bridge_init);
> +
>  /**
>   * drm_bridge_attach - attach the bridge to an encoder's chain
>   *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index c0a2286a81e9..d6d9d5301551 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -402,6 +402,10 @@ struct drm_bridge {
>  
>  void drm_bridge_add(struct drm_bridge *bridge);
>  void drm_bridge_remove(struct drm_bridge *bridge);
> +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> +		     const struct drm_bridge_funcs *funcs,
> +		     const struct drm_bridge_timings *timings,
> +		     void *driver_private);
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous);
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 01/30] drm: Introduce drm_bridge_init()

[Mihail Atanassov]

Hi Daniel,

Thanks for the quick review.

On Tuesday, 26 November 2019 14:26:10 GMT Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
> > A simple convenience function to initialize the struct drm_bridge.
> > 
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> 
> The commit message here leaves figuring out why we need this to the
> reader. Reading ahead the reasons seems to be to roll out bridge->dev
> setting for everyone, so that we can set the device_link. Please explain
> that in the commit message so the patch is properly motivated.

Ack, but with one caveat: bridge->dev is the struct drm_device that is
the bridge client, we need to add a bridge->device (patch 29 in this
series) which is the struct device that will manage the bridge lifetime.

> 
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  4 ++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cba537c99e43..cbe680aa6eac 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> >  }
> >  EXPORT_SYMBOL(drm_bridge_remove);
> >  
> > +/**
> > + * drm_bridge_init - initialise a drm_bridge structure
> > + *
> > + * @bridge: bridge control structure
> > + * @funcs: control functions
> > + * @dev: device
> > + * @timings: timing specification for the bridge; optional (may be NULL)
> > + * @driver_private: pointer to the bridge driver internal context (may be NULL)
> 
> Please also sprinkle some links to this new function to relevant places,
> I'd add at least:
> 
> "Drivers should call drm_bridge_init() first." to the kerneldoc for
> drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as
> the undo function.
> 
> And perhaps a longer paragraph to &struct drm_bridge:
> 
> "Bridge drivers should call drm_bridge_init() to initialized a bridge
> driver, and then register it with drm_bridge_add().
> 
> "Users of bridges link a bridge driver into their overall display output
> pipeline by calling drm_bridge_attach()."

Will do.

> 
> > + */
> > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > +		     const struct drm_bridge_funcs *funcs,
> > +		     const struct drm_bridge_timings *timings,
> > +		     void *driver_private)
> > +{
> > +	WARN_ON(!funcs);
> > +
> > +	bridge->dev = NULL;
> 
> Given that the goal here is to get bridge->dev set up, why not
> 
> 	WARN_ON(!dev);
> 	bridge->dev = dev;

See above struct device vs struct drm_device. I add a

	bridge->device = dev;

in patch 29, which takes care of that. I skipped the warn since
there's a dereference of dev, but I now realized it's behind CONFIG_OF,
so I'll add it in for v2.

Yes, 'device' isn't the best of names, but I took Russell's patch
almost as-is, I didn't have any better ideas for bikeshedding.

> 
> That should help us to really move forward with all this.
> -Daniel
> 
> > +	bridge->encoder = NULL;
> > +	bridge->next = NULL;
> > +
> > +#ifdef CONFIG_OF
> > +	bridge->of_node = dev->of_node;
> > +#endif
> > +	bridge->timings = timings;
> > +	bridge->funcs = funcs;
> > +	bridge->driver_private = driver_private;
> > +}
> > +EXPORT_SYMBOL(drm_bridge_init);
> > +
> >  /**
> >   * drm_bridge_attach - attach the bridge to an encoder's chain
> >   *
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index c0a2286a81e9..d6d9d5301551 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -402,6 +402,10 @@ struct drm_bridge {
> >  
> >  void drm_bridge_add(struct drm_bridge *bridge);
> >  void drm_bridge_remove(struct drm_bridge *bridge);
> > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > +		     const struct drm_bridge_funcs *funcs,
> > +		     const struct drm_bridge_timings *timings,
> > +		     void *driver_private);
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  		      struct drm_bridge *previous);
> 
> 


-- 
Mihail

Re: [PATCH 01/30] drm: Introduce drm_bridge_init()

[Daniel Vetter]

On Tue, Nov 26, 2019 at 4:55 PM Mihail Atanassov
<Mihail.Atanassov@arm.com> wrote:
>
> Hi Daniel,
>
> Thanks for the quick review.
>
> On Tuesday, 26 November 2019 14:26:10 GMT Daniel Vetter wrote:
> > On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
> > > A simple convenience function to initialize the struct drm_bridge.
> > >
> > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> >
> > The commit message here leaves figuring out why we need this to the
> > reader. Reading ahead the reasons seems to be to roll out bridge->dev
> > setting for everyone, so that we can set the device_link. Please explain
> > that in the commit message so the patch is properly motivated.
>
> Ack, but with one caveat: bridge->dev is the struct drm_device that is
> the bridge client, we need to add a bridge->device (patch 29 in this
> series) which is the struct device that will manage the bridge lifetime.

Ah yes, ->dev is for drm_bridge_attach.

> >
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
> > >  include/drm/drm_bridge.h     |  4 ++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index cba537c99e43..cbe680aa6eac 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> > >  }
> > >  EXPORT_SYMBOL(drm_bridge_remove);
> > >
> > > +/**
> > > + * drm_bridge_init - initialise a drm_bridge structure
> > > + *
> > > + * @bridge: bridge control structure
> > > + * @funcs: control functions
> > > + * @dev: device
> > > + * @timings: timing specification for the bridge; optional (may be NULL)
> > > + * @driver_private: pointer to the bridge driver internal context (may be NULL)
> >
> > Please also sprinkle some links to this new function to relevant places,
> > I'd add at least:
> >
> > "Drivers should call drm_bridge_init() first." to the kerneldoc for
> > drm_bridge_add. drm_bridge_add should also mention drm_bridge_remove as
> > the undo function.
> >
> > And perhaps a longer paragraph to &struct drm_bridge:
> >
> > "Bridge drivers should call drm_bridge_init() to initialized a bridge
> > driver, and then register it with drm_bridge_add().
> >
> > "Users of bridges link a bridge driver into their overall display output
> > pipeline by calling drm_bridge_attach()."
>
> Will do.
>
> >
> > > + */
> > > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > > +                const struct drm_bridge_funcs *funcs,
> > > +                const struct drm_bridge_timings *timings,
> > > +                void *driver_private)
> > > +{
> > > +   WARN_ON(!funcs);
> > > +
> > > +   bridge->dev = NULL;
> >
> > Given that the goal here is to get bridge->dev set up, why not
> >
> >       WARN_ON(!dev);
> >       bridge->dev = dev;
>
> See above struct device vs struct drm_device. I add a
>
>         bridge->device = dev;
>
> in patch 29, which takes care of that. I skipped the warn since
> there's a dereference of dev, but I now realized it's behind CONFIG_OF,
> so I'll add it in for v2.

Ok, sounds good. Having the WARN_ON in patch 1 should also help making
sure all the conversion patches dtrt (and any future users).
-Daniel

> Yes, 'device' isn't the best of names, but I took Russell's patch
> almost as-is, I didn't have any better ideas for bikeshedding.
>
> >
> > That should help us to really move forward with all this.
> > -Daniel
> >
> > > +   bridge->encoder = NULL;
> > > +   bridge->next = NULL;
> > > +
> > > +#ifdef CONFIG_OF
> > > +   bridge->of_node = dev->of_node;
> > > +#endif
> > > +   bridge->timings = timings;
> > > +   bridge->funcs = funcs;
> > > +   bridge->driver_private = driver_private;
> > > +}
> > > +EXPORT_SYMBOL(drm_bridge_init);
> > > +
> > >  /**
> > >   * drm_bridge_attach - attach the bridge to an encoder's chain
> > >   *
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index c0a2286a81e9..d6d9d5301551 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -402,6 +402,10 @@ struct drm_bridge {
> > >
> > >  void drm_bridge_add(struct drm_bridge *bridge);
> > >  void drm_bridge_remove(struct drm_bridge *bridge);
> > > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > > +                const struct drm_bridge_funcs *funcs,
> > > +                const struct drm_bridge_timings *timings,
> > > +                void *driver_private);
> > >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> > >                   struct drm_bridge *previous);
> >
> >
>
>
> --
> Mihail
>
>
>


--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/30] drm: Introduce drm_bridge_init()

[Sam Ravnborg]

Hi Mihail.

> Ack, but with one caveat: bridge->dev is the struct drm_device that is
> the bridge client, we need to add a bridge->device (patch 29 in this
> series) which is the struct device that will manage the bridge lifetime.
Other places uses the variable name "drm" for a drm_device.
This is less confusion than the "dev" name.

It seems a recent trend to use the variable name "drm" so you can find a
lot of places using "dev".

bike-shedding - but also about readability.

	Sam

Re: [PATCH 01/30] drm: Introduce drm_bridge_init()

[Mihail Atanassov]

On Tuesday, 26 November 2019 19:24:45 GMT Sam Ravnborg wrote:
> Hi Mihail.

Hi Sam,

> 
> > Ack, but with one caveat: bridge->dev is the struct drm_device that is
> > the bridge client, we need to add a bridge->device (patch 29 in this
> > series) which is the struct device that will manage the bridge lifetime.
> Other places uses the variable name "drm" for a drm_device.
> This is less confusion than the "dev" name.
> 
> It seems a recent trend to use the variable name "drm" so you can find a
> lot of places using "dev".
> 
> bike-shedding - but also about readability.
> 
> 	Sam
> 

I'm okay with the idea, I can do a follow-up patch or series for the
rename; I expect it would be a bit hefty to do it prior to this.

@Daniel, thoughts on s/bridge.dev/bridge.drm/ and
s/bridge.device/bridge.dev/ after this series?

-- 
Mihail

Re: [PATCH 01/30] drm: Introduce drm_bridge_init()

[Daniel Vetter]

On Wed, Nov 27, 2019 at 11:05:56AM +0000, Mihail Atanassov wrote:
> On Tuesday, 26 November 2019 19:24:45 GMT Sam Ravnborg wrote:
> > Hi Mihail.
> 
> Hi Sam,
> 
> > 
> > > Ack, but with one caveat: bridge->dev is the struct drm_device that is
> > > the bridge client, we need to add a bridge->device (patch 29 in this
> > > series) which is the struct device that will manage the bridge lifetime.
> > Other places uses the variable name "drm" for a drm_device.
> > This is less confusion than the "dev" name.
> > 
> > It seems a recent trend to use the variable name "drm" so you can find a
> > lot of places using "dev".
> > 
> > bike-shedding - but also about readability.
> > 
> > 	Sam
> > 
> 
> I'm okay with the idea, I can do a follow-up patch or series for the
> rename; I expect it would be a bit hefty to do it prior to this.
> 
> @Daniel, thoughts on s/bridge.dev/bridge.drm/ and
> s/bridge.device/bridge.dev/ after this series?

I'm firmly in the "no opionon" on this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [01/30] drm: Introduce drm_bridge_init()

[james qian wang (Arm Technology China)]

On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
> A simple convenience function to initialize the struct drm_bridge.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     |  4 ++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cba537c99e43..cbe680aa6eac 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  }
>  EXPORT_SYMBOL(drm_bridge_remove);
>  
> +/**
> + * drm_bridge_init - initialise a drm_bridge structure
> + *
> + * @bridge: bridge control structure
> + * @funcs: control functions
> + * @dev: device
> + * @timings: timing specification for the bridge; optional (may be NULL)
> + * @driver_private: pointer to the bridge driver internal context (may be NULL)
> + */
> +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> +		     const struct drm_bridge_funcs *funcs,
> +		     const struct drm_bridge_timings *timings,
> +		     void *driver_private)
> +{
> +	WARN_ON(!funcs);
> +
> +	bridge->dev = NULL;
> +	bridge->encoder = NULL;
> +	bridge->next = NULL;
> +
> +#ifdef CONFIG_OF
> +	bridge->of_node = dev->of_node;
> +#endif
> +	bridge->timings = timings;
> +	bridge->funcs = funcs;
> +	bridge->driver_private = driver_private;

Can we directly put drm_bridge_add() here. then
- User always need to call bridge_init and add together.
- Consistent with others like drm_plane/crtc_init which directly has
  drm_mode_object_add() in it.

James.
> +}
> +EXPORT_SYMBOL(drm_bridge_init);
> +
>  /**
>   * drm_bridge_attach - attach the bridge to an encoder's chain
>   *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index c0a2286a81e9..d6d9d5301551 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -402,6 +402,10 @@ struct drm_bridge {
>  
>  void drm_bridge_add(struct drm_bridge *bridge);
>  void drm_bridge_remove(struct drm_bridge *bridge);
> +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> +		     const struct drm_bridge_funcs *funcs,
> +		     const struct drm_bridge_timings *timings,
> +		     void *driver_private);
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous);

Re: [01/30] drm: Introduce drm_bridge_init()

[Daniel Vetter]

On Mon, Dec 02, 2019 at 05:55:06AM +0000, james qian wang (Arm Technology China) wrote:
> On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
> > A simple convenience function to initialize the struct drm_bridge.
> > 
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     |  4 ++++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cba537c99e43..cbe680aa6eac 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> >  }
> >  EXPORT_SYMBOL(drm_bridge_remove);
> >  
> > +/**
> > + * drm_bridge_init - initialise a drm_bridge structure
> > + *
> > + * @bridge: bridge control structure
> > + * @funcs: control functions
> > + * @dev: device
> > + * @timings: timing specification for the bridge; optional (may be NULL)
> > + * @driver_private: pointer to the bridge driver internal context (may be NULL)
> > + */
> > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > +		     const struct drm_bridge_funcs *funcs,
> > +		     const struct drm_bridge_timings *timings,
> > +		     void *driver_private)
> > +{
> > +	WARN_ON(!funcs);
> > +
> > +	bridge->dev = NULL;
> > +	bridge->encoder = NULL;
> > +	bridge->next = NULL;
> > +
> > +#ifdef CONFIG_OF
> > +	bridge->of_node = dev->of_node;
> > +#endif
> > +	bridge->timings = timings;
> > +	bridge->funcs = funcs;
> > +	bridge->driver_private = driver_private;
> 
> Can we directly put drm_bridge_add() here. then
> - User always need to call bridge_init and add together.
> - Consistent with others like drm_plane/crtc_init which directly has
>   drm_mode_object_add() in it.

Uh no, the trouble here is that drm_bridge_add should actually be called
_register, because it publishes the bridge to the world. I think we even
have a todo item to rename _add to _register ... Once that's done the
bridge can't be changed anymore, all init code must have completed. So
often you need a bit of code between _init() and _register().

drm_mode_object_add is different since for mode objects it doesn't publish
it to the world, that's done with drm_dev_register and
drm_connector_register. drm_mode_object_add just does a bit of internal
house keeping.
-Daniel

> 
> James.
> > +}
> > +EXPORT_SYMBOL(drm_bridge_init);
> > +
> >  /**
> >   * drm_bridge_attach - attach the bridge to an encoder's chain
> >   *
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index c0a2286a81e9..d6d9d5301551 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -402,6 +402,10 @@ struct drm_bridge {
> >  
> >  void drm_bridge_add(struct drm_bridge *bridge);
> >  void drm_bridge_remove(struct drm_bridge *bridge);
> > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > +		     const struct drm_bridge_funcs *funcs,
> > +		     const struct drm_bridge_timings *timings,
> > +		     void *driver_private);
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  		      struct drm_bridge *previous);
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [01/30] drm: Introduce drm_bridge_init()

[james qian wang (Arm Technology China)]

On Mon, Dec 02, 2019 at 09:49:35AM +0100, Daniel Vetter wrote:
> On Mon, Dec 02, 2019 at 05:55:06AM +0000, james qian wang (Arm Technology China) wrote:
> > On Tue, Nov 26, 2019 at 01:15:59PM +0000, Mihail Atanassov wrote:
> > > A simple convenience function to initialize the struct drm_bridge.
> > >
> > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 29 +++++++++++++++++++++++++++++
> > >  include/drm/drm_bridge.h     |  4 ++++
> > >  2 files changed, 33 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index cba537c99e43..cbe680aa6eac 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -89,6 +89,35 @@ void drm_bridge_remove(struct drm_bridge *bridge)
> > >  }
> > >  EXPORT_SYMBOL(drm_bridge_remove);
> > >
> > > +/**
> > > + * drm_bridge_init - initialise a drm_bridge structure
> > > + *
> > > + * @bridge: bridge control structure
> > > + * @funcs: control functions
> > > + * @dev: device
> > > + * @timings: timing specification for the bridge; optional (may be NULL)
> > > + * @driver_private: pointer to the bridge driver internal context (may be NULL)
> > > + */
> > > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > > +              const struct drm_bridge_funcs *funcs,
> > > +              const struct drm_bridge_timings *timings,
> > > +              void *driver_private)
> > > +{
> > > + WARN_ON(!funcs);
> > > +
> > > + bridge->dev = NULL;
> > > + bridge->encoder = NULL;
> > > + bridge->next = NULL;
> > > +
> > > +#ifdef CONFIG_OF
> > > + bridge->of_node = dev->of_node;
> > > +#endif
> > > + bridge->timings = timings;
> > > + bridge->funcs = funcs;
> > > + bridge->driver_private = driver_private;
> >
> > Can we directly put drm_bridge_add() here. then
> > - User always need to call bridge_init and add together.
> > - Consistent with others like drm_plane/crtc_init which directly has
> >   drm_mode_object_add() in it.
>
> Uh no, the trouble here is that drm_bridge_add should actually be called
> _register, because it publishes the bridge to the world. I think we even
> have a todo item to rename _add to _register ... Once that's done the
> bridge can't be changed anymore, all init code must have completed. So
> often you need a bit of code between _init() and _register().
>
> drm_mode_object_add is different since for mode objects it doesn't publish
> it to the world, that's done with drm_dev_register and
> drm_connector_register. drm_mode_object_add just does a bit of internal
> house keeping.
> -Daniel
>

Yes, the name _register() is more better.

And thank you for such detailed explanation.

Thanks
James

> >
> > James.
> > > +}
> > > +EXPORT_SYMBOL(drm_bridge_init);
> > > +
> > >  /**
> > >   * drm_bridge_attach - attach the bridge to an encoder's chain
> > >   *
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index c0a2286a81e9..d6d9d5301551 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -402,6 +402,10 @@ struct drm_bridge {
> > >
> > >  void drm_bridge_add(struct drm_bridge *bridge);
> > >  void drm_bridge_remove(struct drm_bridge *bridge);
> > > +void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> > > +              const struct drm_bridge_funcs *funcs,
> > > +              const struct drm_bridge_timings *timings,
> > > +              void *driver_private);
> > >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> > >                 struct drm_bridge *previous);
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[PATCH 02/30] drm/bridge: adv7511: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 9e13e466e72c..73600d8766f8 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1216,9 +1216,8 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	if (ret)
 		goto err_unregister_cec;
 
-	adv7511->bridge.funcs = &adv7511_bridge_funcs;
-	adv7511->bridge.of_node = dev->of_node;
-
+	drm_bridge_init(&adv7511->bridge, dev, &adv7511_bridge_funcs,
+			NULL, NULL);
 	drm_bridge_add(&adv7511->bridge);
 
 	adv7511_audio_init(dev, adv7511);
-- 
2.23.0

[PATCH 03/30] drm/bridge: anx6345: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/analogix/analogix-anx6345.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
index b4f3a923a52a..130d5c3a07ef 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c
@@ -696,8 +696,6 @@ static int anx6345_i2c_probe(struct i2c_client *client,
 
 	mutex_init(&anx6345->lock);
 
-	anx6345->bridge.of_node = client->dev.of_node;
-
 	anx6345->client = client;
 	i2c_set_clientdata(client, anx6345);
 
@@ -760,7 +758,8 @@ static int anx6345_i2c_probe(struct i2c_client *client,
 	/* Look for supported chip ID */
 	anx6345_poweron(anx6345);
 	if (anx6345_get_chip_id(anx6345)) {
-		anx6345->bridge.funcs = &anx6345_bridge_funcs;
+		drm_bridge_init(&anx6345->bridge, &client->dev,
+				&anx6345_bridge_funcs, NULL, NULL);
 		drm_bridge_add(&anx6345->bridge);
 
 		return 0;
-- 
2.23.0

[PATCH 04/30] drm/bridge: anx78xx: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
index 41867be03751..e37892cdc9cf 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c
@@ -1214,10 +1214,6 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 
 	mutex_init(&anx78xx->lock);
 
-#if IS_ENABLED(CONFIG_OF)
-	anx78xx->bridge.of_node = client->dev.of_node;
-#endif
-
 	anx78xx->client = client;
 	i2c_set_clientdata(client, anx78xx);
 
@@ -1321,8 +1317,8 @@ static int anx78xx_i2c_probe(struct i2c_client *client,
 		goto err_poweroff;
 	}
 
-	anx78xx->bridge.funcs = &anx78xx_bridge_funcs;
-
+	drm_bridge_init(&anx78xx->bridge, &client->dev, &anx78xx_bridge_funcs,
+			NULL, NULL);
 	drm_bridge_add(&anx78xx->bridge);
 
 	/* If cable is pulled out, just poweroff and wait for HPD event */
-- 
2.23.0

[PATCH 05/30] drm/bridge: cdns: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/cdns-dsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c
index 3a5bd4e7fd1e..58b2aa8b6c24 100644
--- a/drivers/gpu/drm/bridge/cdns-dsi.c
+++ b/drivers/gpu/drm/bridge/cdns-dsi.c
@@ -1233,8 +1233,8 @@ static int cdns_dsi_drm_probe(struct platform_device *pdev)
 	 * CDNS_DPI_INPUT.
 	 */
 	input->id = CDNS_DPI_INPUT;
-	input->bridge.funcs = &cdns_dsi_bridge_funcs;
-	input->bridge.of_node = pdev->dev.of_node;
+	drm_bridge_init(&input->bridge, &pdev->dev, &cdns_dsi_bridge_funcs,
+			NULL, NULL);
 
 	/* Mask all interrupts before registering the IRQ handler. */
 	writel(0, dsi->regs + MCTL_MAIN_STS_CTL);
-- 
2.23.0

[PATCH 06/30] drm/bridge: dumb-vga-dac: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/dumb-vga-dac.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dumb-vga-dac.c b/drivers/gpu/drm/bridge/dumb-vga-dac.c
index cc33dc411b9e..896f27272e38 100644
--- a/drivers/gpu/drm/bridge/dumb-vga-dac.c
+++ b/drivers/gpu/drm/bridge/dumb-vga-dac.c
@@ -205,10 +205,8 @@ static int dumb_vga_probe(struct platform_device *pdev)
 		}
 	}
 
-	vga->bridge.funcs = &dumb_vga_bridge_funcs;
-	vga->bridge.of_node = pdev->dev.of_node;
-	vga->bridge.timings = of_device_get_match_data(&pdev->dev);
-
+	drm_bridge_init(&vga->bridge, &pdev->dev, &dumb_vga_bridge_funcs,
+			of_device_get_match_data(&pdev->dev), NULL);
 	drm_bridge_add(&vga->bridge);
 
 	return 0;
-- 
2.23.0

[PATCH 07/30] drm/bridge: lvds-encoder: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/lvds-encoder.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c b/drivers/gpu/drm/bridge/lvds-encoder.c
index e2132a8d5106..155406510416 100644
--- a/drivers/gpu/drm/bridge/lvds-encoder.c
+++ b/drivers/gpu/drm/bridge/lvds-encoder.c
@@ -112,11 +112,10 @@ static int lvds_encoder_probe(struct platform_device *pdev)
 		return PTR_ERR(lvds_encoder->panel_bridge);
 
 	/* The panel_bridge bridge is attached to the panel's of_node,
-	 * but we need a bridge attached to our of_node for our user
-	 * to look up.
+	 * but we need a bridge attached to our of_node (in dev->of_node)
+	 * for our user to look up.
 	 */
-	lvds_encoder->bridge.of_node = dev->of_node;
-	lvds_encoder->bridge.funcs = &funcs;
+	drm_bridge_init(&lvds_encoder->bridge, dev, &funcs, NULL, NULL);
 	drm_bridge_add(&lvds_encoder->bridge);
 
 	platform_set_drvdata(pdev, lvds_encoder);
-- 
2.23.0

[PATCH 08/30] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
index e8a49f6146c6..d567cd63810f 100644
--- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
+++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c
@@ -303,8 +303,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
 	i2c_set_clientdata(stdp4028_i2c, ge_b850v3_lvds_ptr);
 
 	/* drm bridge initialization */
-	ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs;
-	ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node;
+	drm_bridge_init(&ge_b850v3_lvds_ptr->bridge, dev, &ge_b850v3_lvds_funcs,
+			NULL, NULL);
 	drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
 
 	/* Clear pending interrupts since power up. */
-- 
2.23.0

[PATCH 09/30] drm/bridge: nxp-ptn3460: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/nxp-ptn3460.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 57ff01339559..2656a188b434 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -320,8 +320,8 @@ static int ptn3460_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs;
-	ptn_bridge->bridge.of_node = dev->of_node;
+	drm_bridge_init(&ptn_bridge->bridge, dev, &ptn3460_bridge_funcs,
+			NULL, NULL);
 	drm_bridge_add(&ptn_bridge->bridge);
 
 	i2c_set_clientdata(client, ptn_bridge);
-- 
2.23.0

[PATCH 10/30] drm/bridge: panel: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/panel.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index f4e293e7cf64..91d68d0337cc 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -192,11 +192,8 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
 	panel_bridge->connector_type = connector_type;
 	panel_bridge->panel = panel;
 
-	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
-#ifdef CONFIG_OF
-	panel_bridge->bridge.of_node = panel->dev->of_node;
-#endif
-
+	drm_bridge_init(&panel_bridge->bridge, panel->dev,
+			&panel_bridge_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&panel_bridge->bridge);
 
 	return &panel_bridge->bridge;
-- 
2.23.0

[PATCH 11/30] drm/bridge: ps8622: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/parade-ps8622.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c
index b7a72dfdcac3..8454dbb238bb 100644
--- a/drivers/gpu/drm/bridge/parade-ps8622.c
+++ b/drivers/gpu/drm/bridge/parade-ps8622.c
@@ -588,8 +588,7 @@ static int ps8622_probe(struct i2c_client *client,
 		ps8622->bl->props.brightness = PS8622_MAX_BRIGHTNESS;
 	}
 
-	ps8622->bridge.funcs = &ps8622_bridge_funcs;
-	ps8622->bridge.of_node = dev->of_node;
+	drm_bridge_init(&ps8622->bridge, dev, &ps8622_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&ps8622->bridge);
 
 	i2c_set_clientdata(client, ps8622);
-- 
2.23.0

[PATCH 12/30] drm/bridge: sii902x: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/sii902x.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index b70e8c5cf2e1..2a9db621484d 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -1017,9 +1017,8 @@ static int sii902x_probe(struct i2c_client *client,
 			return ret;
 	}
 
-	sii902x->bridge.funcs = &sii902x_bridge_funcs;
-	sii902x->bridge.of_node = dev->of_node;
-	sii902x->bridge.timings = &default_sii902x_timings;
+	drm_bridge_init(&sii902x->bridge, dev, &sii902x_bridge_funcs,
+			&default_sii902x_timings, NULL);
 	drm_bridge_add(&sii902x->bridge);
 
 	sii902x_audio_codec_init(sii902x, dev);
-- 
2.23.0

[PATCH 13/30] gpu: drm: bridge: sii9234: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/sii9234.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii9234.c b/drivers/gpu/drm/bridge/sii9234.c
index f81f81b7051f..bfd3832baa1a 100644
--- a/drivers/gpu/drm/bridge/sii9234.c
+++ b/drivers/gpu/drm/bridge/sii9234.c
@@ -925,8 +925,7 @@ static int sii9234_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ctx);
 
-	ctx->bridge.funcs = &sii9234_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
+	drm_bridge_init(&ctx->bridge, dev, &sii9234_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&ctx->bridge);
 
 	sii9234_cable_in(ctx);
-- 
2.23.0

[PATCH 14/30] drm/bridge: sil_sii8620: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 4c0eef406eb1..482dc2291350 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2337,8 +2337,7 @@ static int sii8620_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, ctx);
 
-	ctx->bridge.funcs = &sii8620_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
+	drm_bridge_init(&ctx->bridge, dev, &sii8620_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&ctx->bridge);
 
 	if (!ctx->extcon)
-- 
2.23.0

[PATCH 15/30] drm/bridge: dw-hdmi: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index dbe38a54870b..6c71ffc9df5a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2898,11 +2898,8 @@ __dw_hdmi_probe(struct platform_device *pdev,
 			hdmi->ddc = NULL;
 	}
 
-	hdmi->bridge.driver_private = hdmi;
-	hdmi->bridge.funcs = &dw_hdmi_bridge_funcs;
-#ifdef CONFIG_OF
-	hdmi->bridge.of_node = pdev->dev.of_node;
-#endif
+	drm_bridge_init(&hdmi->bridge, &pdev->dev, &dw_hdmi_bridge_funcs,
+			NULL, hdmi);
 
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.parent = dev;
-- 
2.23.0

[PATCH 17/30] drm/bridge: tc358764: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/tc358764.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c
index db298f550a5a..861d4df687ee 100644
--- a/drivers/gpu/drm/bridge/tc358764.c
+++ b/drivers/gpu/drm/bridge/tc358764.c
@@ -457,9 +457,7 @@ static int tc358764_probe(struct mipi_dsi_device *dsi)
 	if (ret < 0)
 		return ret;
 
-	ctx->bridge.funcs = &tc358764_bridge_funcs;
-	ctx->bridge.of_node = dev->of_node;
-
+	drm_bridge_init(&ctx->bridge, dev, &tc358764_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&ctx->bridge);
 
 	ret = mipi_dsi_attach(dsi);
-- 
2.23.0

[PATCH 16/30] drm/bridge/synopsys: dsi: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index b6e793bb653c..051f9aaf5867 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -1052,11 +1052,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev,
 		return ERR_PTR(ret);
 	}
 
-	dsi->bridge.driver_private = dsi;
-	dsi->bridge.funcs = &dw_mipi_dsi_bridge_funcs;
-#ifdef CONFIG_OF
-	dsi->bridge.of_node = pdev->dev.of_node;
-#endif
+	drm_bridge_init(&dsi->bridge, &pdev->dev, &dw_mipi_dsi_bridge_funcs,
+			NULL, dsi);
 
 	return dsi;
 }
-- 
2.23.0

[PATCH 18/30] drm/bridge: tc358767: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/tc358767.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8029478ffebb..7846c1925cbb 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1671,8 +1671,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
-	tc->bridge.funcs = &tc_bridge_funcs;
-	tc->bridge.of_node = dev->of_node;
+	drm_bridge_init(&tc->bridge, dev, &tc_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&tc->bridge);
 
 	i2c_set_clientdata(client, tc);
-- 
2.23.0

[PATCH 19/30] drm/bridge: thc63: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 3d74129b2995..abe806db5f4d 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -218,11 +218,8 @@ static int thc63_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	thc63->bridge.driver_private = thc63;
-	thc63->bridge.of_node = pdev->dev.of_node;
-	thc63->bridge.funcs = &thc63_bridge_func;
-	thc63->bridge.timings = &thc63->timings;
-
+	drm_bridge_init(&thc63->bridge, &pdev->dev, &thc63_bridge_func,
+			&thc63->timings, thc63);
 	drm_bridge_add(&thc63->bridge);
 
 	return 0;
-- 
2.23.0

[PATCH 20/30] drm/bridge: ti-sn65dsi86: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 43abf01ebd4c..4e236b46f913 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -765,9 +765,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 	pdata->aux.transfer = ti_sn_aux_transfer;
 	drm_dp_aux_register(&pdata->aux);
 
-	pdata->bridge.funcs = &ti_sn_bridge_funcs;
-	pdata->bridge.of_node = client->dev.of_node;
-
+	drm_bridge_init(&pdata->bridge, &client->dev, &ti_sn_bridge_funcs,
+			NULL, NULL);
 	drm_bridge_add(&pdata->bridge);
 
 	ti_sn_debugfs_init(pdata);
-- 
2.23.0

[PATCH 21/30] drm/bridge: ti-tfp410: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/bridge/ti-tfp410.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index aa3198dc9903..377eca989ff3 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -328,9 +328,8 @@ static int tfp410_init(struct device *dev, bool i2c)
 		return -ENOMEM;
 	dev_set_drvdata(dev, dvi);
 
-	dvi->bridge.funcs = &tfp410_bridge_funcs;
-	dvi->bridge.of_node = dev->of_node;
-	dvi->bridge.timings = &dvi->timings;
+	drm_bridge_init(&dvi->bridge, dev, &tfp410_bridge_funcs, &dvi->timings,
+			NULL);
 	dvi->dev = dev;
 
 	ret = tfp410_parse_timings(dvi, i2c);
-- 
2.23.0

[PATCH 22/30] drm/exynos: mic: Use drm_bridge_init()

[Mihail Atanassov]

No functional change: no logic depends on driver_private being NULL, so
it's safe to set it earlier in exynos_mic_probe.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
index f41d75923557..caad348a5646 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
@@ -309,10 +309,6 @@ static const struct drm_bridge_funcs mic_bridge_funcs = {
 static int exynos_mic_bind(struct device *dev, struct device *master,
 			   void *data)
 {
-	struct exynos_mic *mic = dev_get_drvdata(dev);
-
-	mic->bridge.driver_private = mic;
-
 	return 0;
 }
 
@@ -422,9 +418,7 @@ static int exynos_mic_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, mic);
 
-	mic->bridge.funcs = &mic_bridge_funcs;
-	mic->bridge.of_node = dev->of_node;
-
+	drm_bridge_init(&mic->bridge, dev, &mic_bridge_funcs, NULL, mic);
 	drm_bridge_add(&mic->bridge);
 
 	pm_runtime_enable(dev);
-- 
2.23.0

Re: [PATCH 22/30] drm/exynos: mic: Use drm_bridge_init()

[Inki Dae]

19. 11. 26. 오후 10:16에 Mihail Atanassov 이(가) 쓴 글:
> No functional change: no logic depends on driver_private being NULL, so
> it's safe to set it earlier in exynos_mic_probe.
Acked-by: Inki Dae <inki.dae@samsung.com>

And tested this patch on TM2 and TM2E boards.
Tested-by: Inki Dae <inki.dae@samsung.com>

Thanks,
Inki Dae

> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_mic.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_mic.c b/drivers/gpu/drm/exynos/exynos_drm_mic.c
> index f41d75923557..caad348a5646 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_mic.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_mic.c
> @@ -309,10 +309,6 @@ static const struct drm_bridge_funcs mic_bridge_funcs = {
>  static int exynos_mic_bind(struct device *dev, struct device *master,
>  			   void *data)
>  {
> -	struct exynos_mic *mic = dev_get_drvdata(dev);
> -
> -	mic->bridge.driver_private = mic;
> -
>  	return 0;
>  }
>  
> @@ -422,9 +418,7 @@ static int exynos_mic_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, mic);
>  
> -	mic->bridge.funcs = &mic_bridge_funcs;
> -	mic->bridge.of_node = dev->of_node;
> -
> +	drm_bridge_init(&mic->bridge, dev, &mic_bridge_funcs, NULL, mic);
>  	drm_bridge_add(&mic->bridge);
>  
>  	pm_runtime_enable(dev);
> 

[PATCH 23/30] drm/i2c: tda998x: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a63790d32d75..f7dfa694aff7 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1974,11 +1974,7 @@ static int tda998x_create(struct device *dev)
 			goto fail;
 	}
 
-	priv->bridge.funcs = &tda998x_bridge_funcs;
-#ifdef CONFIG_OF
-	priv->bridge.of_node = dev->of_node;
-#endif
-
+	drm_bridge_init(&priv->bridge, dev, &tda998x_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&priv->bridge);
 
 	return 0;
-- 
2.23.0

[PATCH 24/30] drm/mcde: dsi: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/mcde/mcde_dsi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
index 42fff811653e..d9b9253acccf 100644
--- a/drivers/gpu/drm/mcde/mcde_dsi.c
+++ b/drivers/gpu/drm/mcde/mcde_dsi.c
@@ -955,8 +955,7 @@ static int mcde_dsi_bind(struct device *dev, struct device *master,
 	d->bridge_out = bridge;
 
 	/* Create a bridge for this DSI channel */
-	d->bridge.funcs = &mcde_dsi_bridge_funcs;
-	d->bridge.of_node = dev->of_node;
+	drm_bridge_init(&d->bridge, dev, &mcde_dsi_bridge_funcs, NULL, NULL);
 	drm_bridge_add(&d->bridge);
 
 	/* TODO: first come first serve, use a list */
-- 
2.23.0

Re: [PATCH 24/30] drm/mcde: dsi: Use drm_bridge_init()

[Linus Walleij]

On Tue, Nov 26, 2019 at 2:16 PM Mihail Atanassov
<Mihail.Atanassov@arm.com> wrote:

> No functional change.
>
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH 25/30] drm/mediatek: hdmi: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index f684947c5243..9761a80674d9 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1708,8 +1708,8 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
 
 	mtk_hdmi_register_audio_driver(dev);
 
-	hdmi->bridge.funcs = &mtk_hdmi_bridge_funcs;
-	hdmi->bridge.of_node = pdev->dev.of_node;
+	drm_bridge_init(&hdmi->bridge, &pdev->dev, &mtk_hdmi_bridge_funcs,
+			NULL, NULL);
 	drm_bridge_add(&hdmi->bridge);
 
 	ret = mtk_hdmi_clk_enable_audio(hdmi);
-- 
2.23.0

[PATCH 26/30] drm: rcar-du: lvds: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 8c6c172bbf2e..ac1f29bacfcb 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -881,9 +881,8 @@ static int rcar_lvds_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	lvds->bridge.driver_private = lvds;
-	lvds->bridge.funcs = &rcar_lvds_bridge_ops;
-	lvds->bridge.of_node = pdev->dev.of_node;
+	drm_bridge_init(&lvds->bridge, &pdev->dev, &rcar_lvds_bridge_ops,
+			NULL, lvds);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	lvds->mmio = devm_ioremap_resource(&pdev->dev, mem);
-- 
2.23.0

[PATCH 27/30] drm: rcar-du: lvds: Don't set drm_bridge private pointer

[Mihail Atanassov]

No functional change: it's not used anywhere.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index ac1f29bacfcb..168a718cbcbd 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -882,7 +882,7 @@ static int rcar_lvds_probe(struct platform_device *pdev)
 		return ret;
 
 	drm_bridge_init(&lvds->bridge, &pdev->dev, &rcar_lvds_bridge_ops,
-			NULL, lvds);
+			NULL, NULL);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	lvds->mmio = devm_ioremap_resource(&pdev->dev, mem);
-- 
2.23.0

[PATCH 28/30] drm/sti: sti_vdo: Use drm_bridge_init()

[Mihail Atanassov]

No functional change.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/sti/sti_dvo.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
index 68289b0b063a..20a3956b33bc 100644
--- a/drivers/gpu/drm/sti/sti_dvo.c
+++ b/drivers/gpu/drm/sti/sti_dvo.c
@@ -462,9 +462,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data)
 	if (!bridge)
 		return -ENOMEM;
 
-	bridge->driver_private = dvo;
-	bridge->funcs = &sti_dvo_bridge_funcs;
-	bridge->of_node = dvo->dev.of_node;
+	drm_bridge_init(bridge, &dvo->dev, &sti_dvo_bridge_funcs, NULL, dvo);
 	drm_bridge_add(bridge);
 
 	err = drm_bridge_attach(encoder, bridge, NULL);
-- 
2.23.0

Re: [PATCH 28/30] drm/sti: sti_vdo: Use drm_bridge_init()

[Sam Ravnborg]

Hi Mihail.

On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> No functional change.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> ---
>  drivers/gpu/drm/sti/sti_dvo.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
> index 68289b0b063a..20a3956b33bc 100644
> --- a/drivers/gpu/drm/sti/sti_dvo.c
> +++ b/drivers/gpu/drm/sti/sti_dvo.c
> @@ -462,9 +462,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data)
>  	if (!bridge)
>  		return -ENOMEM;
>  
> -	bridge->driver_private = dvo;
> -	bridge->funcs = &sti_dvo_bridge_funcs;
> -	bridge->of_node = dvo->dev.of_node;
> +	drm_bridge_init(bridge, &dvo->dev, &sti_dvo_bridge_funcs, NULL, dvo);
>  	drm_bridge_add(bridge);
>  
>  	err = drm_bridge_attach(encoder, bridge, NULL);

I can see from grepping that bridge.driver_private is used
in a couple of other files in sti/

Like sti_hdmi.c:
        bridge->driver_private = hdmi;
        bridge->funcs = &sti_hdmi_bridge_funcs;
        drm_bridge_attach(encoder, bridge, NULL);


I wonder if a drm_bridge_init() should be added there.
I did not look closely - but it looked suspisiously.

	Sam

Re: [PATCH 28/30] drm/sti: sti_vdo: Use drm_bridge_init()

[Mihail Atanassov]

On Tuesday, 26 November 2019 19:37:40 GMT Sam Ravnborg wrote:
> Hi Mihail.

Hi Sam,

> 
> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> > No functional change.
> > 
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > ---
> >  drivers/gpu/drm/sti/sti_dvo.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sti/sti_dvo.c b/drivers/gpu/drm/sti/sti_dvo.c
> > index 68289b0b063a..20a3956b33bc 100644
> > --- a/drivers/gpu/drm/sti/sti_dvo.c
> > +++ b/drivers/gpu/drm/sti/sti_dvo.c
> > @@ -462,9 +462,7 @@ static int sti_dvo_bind(struct device *dev, struct device *master, void *data)
> >  	if (!bridge)
> >  		return -ENOMEM;
> >  
> > -	bridge->driver_private = dvo;
> > -	bridge->funcs = &sti_dvo_bridge_funcs;
> > -	bridge->of_node = dvo->dev.of_node;
> > +	drm_bridge_init(bridge, &dvo->dev, &sti_dvo_bridge_funcs, NULL, dvo);
> >  	drm_bridge_add(bridge);
> >  
> >  	err = drm_bridge_attach(encoder, bridge, NULL);
> 
> I can see from grepping that bridge.driver_private is used
> in a couple of other files in sti/
> 
> Like sti_hdmi.c:
>         bridge->driver_private = hdmi;
>         bridge->funcs = &sti_hdmi_bridge_funcs;
>         drm_bridge_attach(encoder, bridge, NULL);
> 
> 
> I wonder if a drm_bridge_init() should be added there.
> I did not look closely - but it looked suspisiously.

My goal with drm_bridge_init() was to get devlinks sorted out for
cross-module uses of a drm_bridge (via of_drm_find_bridge()), so I only
considered locations where drm_bridge_add/remove() were used.

Would you be okay with a promise to push a cleanup of this one and the
one in sti_hda.c after patch 1/30 lands in some form? I'd rather not
make this series much longer, it's already pushing it at 30 :).

> 
> 	Sam
> 


-- 
Mihail

Re: [PATCH 28/30] drm/sti: sti_vdo: Use drm_bridge_init()

[Sam Ravnborg]

Hi Mihail.

> > 
> > I can see from grepping that bridge.driver_private is used
> > in a couple of other files in sti/
> > 
> > Like sti_hdmi.c:
> >         bridge->driver_private = hdmi;
> >         bridge->funcs = &sti_hdmi_bridge_funcs;
> >         drm_bridge_attach(encoder, bridge, NULL);
> > 
> > 
> > I wonder if a drm_bridge_init() should be added there.
> > I did not look closely - but it looked suspisiously.
> 
> My goal with drm_bridge_init() was to get devlinks sorted out for
> cross-module uses of a drm_bridge (via of_drm_find_bridge()), so I only
> considered locations where drm_bridge_add/remove() were used.
> 
> Would you be okay with a promise to push a cleanup of this one and the
> one in sti_hda.c after patch 1/30 lands in some form? I'd rather not
> make this series much longer, it's already pushing it at 30 :).

Absolutely - my drive-by comment was more out of concern if this
was missing. A clean-up later souns good.

	Sam

Re: [PATCH 28/30] drm/sti: sti_vdo: Use drm_bridge_init()

[Benjamin Gaignard]

Le mer. 27 nov. 2019 à 17:19, Sam Ravnborg <sam@ravnborg.org> a écrit :
>
> Hi Mihail.
>
> > >
> > > I can see from grepping that bridge.driver_private is used
> > > in a couple of other files in sti/
> > >
> > > Like sti_hdmi.c:
> > >         bridge->driver_private = hdmi;
> > >         bridge->funcs = &sti_hdmi_bridge_funcs;
> > >         drm_bridge_attach(encoder, bridge, NULL);
> > >
> > >
> > > I wonder if a drm_bridge_init() should be added there.
> > > I did not look closely - but it looked suspisiously.
> >
> > My goal with drm_bridge_init() was to get devlinks sorted out for
> > cross-module uses of a drm_bridge (via of_drm_find_bridge()), so I only
> > considered locations where drm_bridge_add/remove() were used.
> >
> > Would you be okay with a promise to push a cleanup of this one and the
> > one in sti_hda.c after patch 1/30 lands in some form? I'd rather not
> > make this series much longer, it's already pushing it at 30 :).
>
> Absolutely - my drive-by comment was more out of concern if this
> was missing. A clean-up later souns good.

Or you can just do the changes for sti_hdmi and sti_hda in this patch too.

Benjamin

>
>         Sam
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 29/30] drm/bridge: add support for device links to bridge

[Mihail Atanassov]

From: Russell King <rmk+kernel@armlinux.org.uk>

Bridge devices have been a potential for kernel oops as their lifetime
is independent of the DRM device that they are bound to.  Hence, if a
bridge device is unbound while the parent DRM device is using them, the
parent happily continues to use the bridge device, calling the driver
and accessing its objects that have been freed.

This can cause kernel memory corruption and kernel oops.

To control this, use device links to ensure that the parent DRM device
is unbound when the bridge device is unbound, and when the bridge
device is re-bound, automatically rebind the parent DRM device.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Tested-by: Mihail Atanassov <mihail.atanassov@arm.com>
[reworked to use drm_bridge_init() for setting bridge->device]
Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++----------
 include/drm/drm_bridge.h     |  4 +++
 2 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index cbe680aa6eac..e1f8db84651a 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -26,6 +26,7 @@
 #include <linux/mutex.h>
 
 #include <drm/drm_bridge.h>
+#include <drm/drm_device.h>
 #include <drm/drm_encoder.h>
 
 #include "drm_crtc_internal.h"
@@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
 	bridge->encoder = NULL;
 	bridge->next = NULL;
 
+	bridge->device = dev;
 #ifdef CONFIG_OF
 	bridge->of_node = dev->of_node;
 #endif
@@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
 EXPORT_SYMBOL(drm_atomic_bridge_enable);
 
 #ifdef CONFIG_OF
+static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
+					  struct device_node *np, bool link)
+{
+	struct drm_bridge *bridge, *found = NULL;
+	struct device_link *dl;
+
+	mutex_lock(&bridge_lock);
+
+	list_for_each_entry(bridge, &bridge_list, list)
+		if (bridge->of_node == np) {
+			found = bridge;
+			break;
+		}
+
+	if (found && link) {
+		dl = device_link_add(dev->dev, found->device,
+				     DL_FLAG_AUTOPROBE_CONSUMER);
+		if (!dl)
+			found = NULL;
+	}
+
+	mutex_unlock(&bridge_lock);
+
+	return found;
+}
+
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
  *			the global bridge list
@@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
  */
 struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 {
-	struct drm_bridge *bridge;
-
-	mutex_lock(&bridge_lock);
-
-	list_for_each_entry(bridge, &bridge_list, list) {
-		if (bridge->of_node == np) {
-			mutex_unlock(&bridge_lock);
-			return bridge;
-		}
-	}
-
-	mutex_unlock(&bridge_lock);
-	return NULL;
+	return drm_bridge_find(NULL, np, false);
 }
 EXPORT_SYMBOL(of_drm_find_bridge);
+
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
+					      struct device_node *np)
+{
+	return drm_bridge_find(dev, np, true);
+}
+EXPORT_SYMBOL(of_drm_find_bridge_devlink);
 #endif
 
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index d6d9d5301551..68b27c69cc3d 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -382,6 +382,8 @@ struct drm_bridge {
 	struct drm_encoder *encoder;
 	/** @next: the next bridge in the encoder chain */
 	struct drm_bridge *next;
+	/** @device: Linux driver model device */
+	struct device *device;
 #ifdef CONFIG_OF
 	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
@@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
 		     const struct drm_bridge_timings *timings,
 		     void *driver_private);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
+struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
+					      struct device_node *np);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous);
 
-- 
2.23.0

Re: [PATCH 29/30] drm/bridge: add support for device links to bridge

[Daniel Vetter]

On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> From: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Bridge devices have been a potential for kernel oops as their lifetime
> is independent of the DRM device that they are bound to.  Hence, if a
> bridge device is unbound while the parent DRM device is using them, the
> parent happily continues to use the bridge device, calling the driver
> and accessing its objects that have been freed.
> 
> This can cause kernel memory corruption and kernel oops.
> 
> To control this, use device links to ensure that the parent DRM device
> is unbound when the bridge device is unbound, and when the bridge
> device is re-bound, automatically rebind the parent DRM device.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> Tested-by: Mihail Atanassov <mihail.atanassov@arm.com>
> [reworked to use drm_bridge_init() for setting bridge->device]
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>

So I thought the big plan was to put the device_link setup into
drm_bridge_attach, so that it's done for everyone. And we could then
slowly go through the existing drivers that use the component framework to
get this handled correctly.

So my questions:
- is there a problem if we add the device_link for everyone?
- is there an issue if we only add it at drm_bridge_attach time? I kinda
  assumed that it's not needed before that (EPROBE_DEFER should handle
  load dependencies as before), but it could be that some drivers ask for
  a bridge and then check more stuff and then drop the bridge without
  calling drm_bridge_attach. We probably don't have a case like this yet,
  but better robust than sorry.

Anyway, I scrolled through the bridge patches, looked all good, huge
thanks for tackling this! Once we have some agreement on the bigger
questions here I'll try to go through them and review.

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++----------
>  include/drm/drm_bridge.h     |  4 +++
>  2 files changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index cbe680aa6eac..e1f8db84651a 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -26,6 +26,7 @@
>  #include <linux/mutex.h>
>  
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_device.h>
>  #include <drm/drm_encoder.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
>  	bridge->encoder = NULL;
>  	bridge->next = NULL;
>  
> +	bridge->device = dev;
>  #ifdef CONFIG_OF
>  	bridge->of_node = dev->of_node;
>  #endif
> @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
>  EXPORT_SYMBOL(drm_atomic_bridge_enable);
>  
>  #ifdef CONFIG_OF
> +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
> +					  struct device_node *np, bool link)
> +{
> +	struct drm_bridge *bridge, *found = NULL;
> +	struct device_link *dl;
> +
> +	mutex_lock(&bridge_lock);
> +
> +	list_for_each_entry(bridge, &bridge_list, list)
> +		if (bridge->of_node == np) {
> +			found = bridge;
> +			break;
> +		}
> +
> +	if (found && link) {
> +		dl = device_link_add(dev->dev, found->device,
> +				     DL_FLAG_AUTOPROBE_CONSUMER);
> +		if (!dl)
> +			found = NULL;
> +	}
> +
> +	mutex_unlock(&bridge_lock);
> +
> +	return found;
> +}
> +
>  /**
>   * of_drm_find_bridge - find the bridge corresponding to the device node in
>   *			the global bridge list
> @@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
>   */
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  {
> -	struct drm_bridge *bridge;
> -
> -	mutex_lock(&bridge_lock);
> -
> -	list_for_each_entry(bridge, &bridge_list, list) {
> -		if (bridge->of_node == np) {
> -			mutex_unlock(&bridge_lock);
> -			return bridge;
> -		}
> -	}
> -
> -	mutex_unlock(&bridge_lock);
> -	return NULL;
> +	return drm_bridge_find(NULL, np, false);
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> +					      struct device_node *np)
> +{
> +	return drm_bridge_find(dev, np, true);
> +}
> +EXPORT_SYMBOL(of_drm_find_bridge_devlink);
>  #endif
>  
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index d6d9d5301551..68b27c69cc3d 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -382,6 +382,8 @@ struct drm_bridge {
>  	struct drm_encoder *encoder;
>  	/** @next: the next bridge in the encoder chain */
>  	struct drm_bridge *next;
> +	/** @device: Linux driver model device */
> +	struct device *device;
>  #ifdef CONFIG_OF
>  	/** @of_node: device node pointer to the bridge */
>  	struct device_node *of_node;
> @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
>  		     const struct drm_bridge_timings *timings,
>  		     void *driver_private);
>  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> +					      struct device_node *np);
>  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
>  		      struct drm_bridge *previous);
>  
> -- 
> 2.23.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 29/30] drm/bridge: add support for device links to bridge

[Mihail Atanassov]

On Tuesday, 26 November 2019 14:35:34 GMT Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > Bridge devices have been a potential for kernel oops as their lifetime
> > is independent of the DRM device that they are bound to.  Hence, if a
> > bridge device is unbound while the parent DRM device is using them, the
> > parent happily continues to use the bridge device, calling the driver
> > and accessing its objects that have been freed.
> > 
> > This can cause kernel memory corruption and kernel oops.
> > 
> > To control this, use device links to ensure that the parent DRM device
> > is unbound when the bridge device is unbound, and when the bridge
> > device is re-bound, automatically rebind the parent DRM device.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Tested-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > [reworked to use drm_bridge_init() for setting bridge->device]
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> 
> So I thought the big plan was to put the device_link setup into
> drm_bridge_attach, so that it's done for everyone. And we could then
> slowly go through the existing drivers that use the component framework to
> get this handled correctly.
> 
> So my questions:
> - is there a problem if we add the device_link for everyone?

Possibly not, but I didn't want to stir the entire pot :). This is
safer in the sense that it's an opt-in, so a lower chance of
regressions in code and setups that I can't possibly test. If you
think it's worth sticking it in the existing code paths, I can
certainly do that too.

> - is there an issue if we only add it at drm_bridge_attach time? I kinda
>   assumed that it's not needed before that (EPROBE_DEFER should handle
>   load dependencies as before), but it could be that some drivers ask for
>   a bridge and then check more stuff and then drop the bridge without
>   calling drm_bridge_attach. We probably don't have a case like this yet,
>   but better robust than sorry.

I think there would be a race there:

- bridge driver calls drm_bridge_add() in their probe()
- client driver calls of_drm_find_bridge() in their probe()
- bridge driver gets removed, calls drm_bridge_remove()
- client driver uses the now invalid drm_bridge pointer from above to
  do drm_bridge_attach()

With of_drm_bridge_find_devlink(), you get the device_link inside the
bridge_lock so the reference to the drm_bridge will either be valid, or
your driver gets removed right after it's probed, so that the bridge
can be removed, too.

In patch 30/30 I use both the _devlink and the non-_devlink versions
of of_drm_find_bridge(), but I guess there's no harm adding another
refcount on the link, it'll get destroyed if the bridge is removed
regardless, although that may need a DL_FLAG_AUTOREMOVE_CONSUMER.

> 
> Anyway, I scrolled through the bridge patches, looked all good, huge
> thanks for tackling this! Once we have some agreement on the bigger
> questions here I'll try to go through them and review.
> 
> Cheers, Daniel
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++----------
> >  include/drm/drm_bridge.h     |  4 +++
> >  2 files changed, 40 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cbe680aa6eac..e1f8db84651a 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/mutex.h>
> >  
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_device.h>
> >  #include <drm/drm_encoder.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> >  	bridge->encoder = NULL;
> >  	bridge->next = NULL;
> >  
> > +	bridge->device = dev;
> >  #ifdef CONFIG_OF
> >  	bridge->of_node = dev->of_node;
> >  #endif
> > @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
> >  EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >  
> >  #ifdef CONFIG_OF
> > +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
> > +					  struct device_node *np, bool link)
> > +{
> > +	struct drm_bridge *bridge, *found = NULL;
> > +	struct device_link *dl;
> > +
> > +	mutex_lock(&bridge_lock);
> > +
> > +	list_for_each_entry(bridge, &bridge_list, list)
> > +		if (bridge->of_node == np) {
> > +			found = bridge;
> > +			break;
> > +		}
> > +
> > +	if (found && link) {
> > +		dl = device_link_add(dev->dev, found->device,
> > +				     DL_FLAG_AUTOPROBE_CONSUMER);
> > +		if (!dl)mutex
> > +			found = NULL;
> > +	}
> > +
> > +	mutex_unlock(&bridge_lock);
> > +
> > +	return found;
> > +}
> > +
> >  /**
> >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> >   *			the global bridge list
> > @@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >   */
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >  {
> > -	struct drm_bridge *bridge;
> > -
> > -	mutex_lock(&bridge_lock);
> > -
> > -	list_for_each_entry(bridge, &bridge_list, list) {
> > -		if (bridge->of_node == np) {
> > -			mutex_unlock(&bridge_lock);
> > -			return bridge;
> > -		}
> > -	}
> > -
> > -	mutex_unlock(&bridge_lock);
> > -	return NULL;
> > +	return drm_bridge_find(NULL, np, false);
> >  }
> >  EXPORT_SYMBOL(of_drm_find_bridge);
> > +
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > +					      struct device_node *np)
> > +{
> > +	return drm_bridge_find(dev, np, true);
> > +}
> > +EXPORT_SYMBOL(of_drm_find_bridge_devlink);
> >  #endif
> >  
> >  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index d6d9d5301551..68b27c69cc3d 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -382,6 +382,8 @@ struct drm_bridge {
> >  	struct drm_encoder *encoder;
> >  	/** @next: the next bridge in the encoder chain */
> >  	struct drm_bridge *next;
> > +	/** @device: Linux driver model device */
> > +	struct device *device;
> >  #ifdef CONFIG_OF
> >  	/** @of_node: device node pointer to the bridge */
> >  	struct device_node *of_node;
> > @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> >  		     const struct drm_bridge_timings *timings,
> >  		     void *driver_private);
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > +					      struct device_node *np);
> >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  		      struct drm_bridge *previous);
> >  
> 
> 


-- 
Mihail

Re: [PATCH 29/30] drm/bridge: add support for device links to bridge

[Mihail Atanassov]

On Tuesday, 26 November 2019 14:35:34 GMT Daniel Vetter wrote:
> On Tue, Nov 26, 2019 at 01:16:26PM +0000, Mihail Atanassov wrote:
> > From: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > Bridge devices have been a potential for kernel oops as their lifetime
> > is independent of the DRM device that they are bound to.  Hence, if a
> > bridge device is unbound while the parent DRM device is using them, the
> > parent happily continues to use the bridge device, calling the driver
> > and accessing its objects that have been freed.
> > 
> > This can cause kernel memory corruption and kernel oops.
> > 
> > To control this, use device links to ensure that the parent DRM device
> > is unbound when the bridge device is unbound, and when the bridge
> > device is re-bound, automatically rebind the parent DRM device.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Tested-by: Mihail Atanassov <mihail.atanassov@arm.com>
> > [reworked to use drm_bridge_init() for setting bridge->device]
> > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> 
> So I thought the big plan was to put the device_link setup into
> drm_bridge_attach, so that it's done for everyone. And we could then
> slowly go through the existing drivers that use the component framework to
> get this handled correctly.
> 
> So my questions:
> - is there a problem if we add the device_link for everyone?

So after spending time looking at the code and thinking
about it, I'm slowly coming to the conclusion that getting device
links right for everyone in one go is a much bigger task than this
opt-in quick-fix here. I've hit, at the very least, the following
snags in trying to apply it universally:

panel_bridge - removing one via drm_of_panel_bridge_remove() uses
of_drm_find_bridge(), which would add a devlink at a very inopportune
time;

mipi_dsi_host - attach/detach, where e.g. dw-mipi-dsi.c handles bridge
creation/destruction, doesn't correspond directly to a struct device's
lifetime, so the device link would linger longer than is required;

others that add/remove bridges at times different from probe/remove
(drivers using the component framework?).

I think it'd still be valuable even with limiting the scope to drivers
that get their bridge in probe() and drop it in remove() for now, and
only roll it out as an opt-in. Thoughts?

I think to get it right we need to use the links' refcount, with e.g.
of_drm_find_bridge() giving you a refcount of 1, and bridge_detach()
maybe dropping the refcount, but I can envision ways where this breaks
too, so maybe just an of_drm_{get,put}_bridge()?

> - is there an issue if we only add it at drm_bridge_attach time? I kinda
>   assumed that it's not needed before that (EPROBE_DEFER should handle
>   load dependencies as before), but it could be that some drivers ask for
>   a bridge and then check more stuff and then drop the bridge without
>   calling drm_bridge_attach. We probably don't have a case like this yet,
>   but better robust than sorry.
> 
> Anyway, I scrolled through the bridge patches, looked all good, huge
> thanks for tackling this! Once we have some agreement on the bigger
> questions here I'll try to go through them and review.
> 
> Cheers, Daniel
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 49 ++++++++++++++++++++++++++----------
> >  include/drm/drm_bridge.h     |  4 +++
> >  2 files changed, 40 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index cbe680aa6eac..e1f8db84651a 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/mutex.h>
> >  
> >  #include <drm/drm_bridge.h>
> > +#include <drm/drm_device.h>
> >  #include <drm/drm_encoder.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -109,6 +110,7 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> >  	bridge->encoder = NULL;
> >  	bridge->next = NULL;
> >  
> > +	bridge->device = dev;
> >  #ifdef CONFIG_OF
> >  	bridge->of_node = dev->of_node;
> >  #endif
> > @@ -492,6 +494,32 @@ void drm_atomic_bridge_enable(struct drm_bridge *bridge,
> >  EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >  
> >  #ifdef CONFIG_OF
> > +static struct drm_bridge *drm_bridge_find(struct drm_device *dev,
> > +					  struct device_node *np, bool link)
> > +{
> > +	struct drm_bridge *bridge, *found = NULL;
> > +	struct device_link *dl;
> > +
> > +	mutex_lock(&bridge_lock);
> > +
> > +	list_for_each_entry(bridge, &bridge_list, list)
> > +		if (bridge->of_node == np) {
> > +			found = bridge;
> > +			break;
> > +		}
> > +
> > +	if (found && link) {
> > +		dl = device_link_add(dev->dev, found->device,
> > +				     DL_FLAG_AUTOPROBE_CONSUMER);
> > +		if (!dl)
> > +			found = NULL;
> > +	}
> > +
> > +	mutex_unlock(&bridge_lock);
> > +
> > +	return found;
> > +}
> > +
> >  /**
> >   * of_drm_find_bridge - find the bridge corresponding to the device node in
> >   *			the global bridge list
> > @@ -503,21 +531,16 @@ EXPORT_SYMBOL(drm_atomic_bridge_enable);
> >   */
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >  {
> > -	struct drm_bridge *bridge;
> > -
> > -	mutex_lock(&bridge_lock);
> > -
> > -	list_for_each_entry(bridge, &bridge_list, list) {
> > -		if (bridge->of_node == np) {
> > -			mutex_unlock(&bridge_lock);
> > -			return bridge;
> > -		}
> > -	}
> > -
> > -	mutex_unlock(&bridge_lock);
> > -	return NULL;
> > +	return drm_bridge_find(NULL, np, false);
> >  }
> >  EXPORT_SYMBOL(of_drm_find_bridge);
> > +
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > +					      struct device_node *np)
> > +{
> > +	return drm_bridge_find(dev, np, true);
> > +}
> > +EXPORT_SYMBOL(of_drm_find_bridge_devlink);
> >  #endif
> >  
> >  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index d6d9d5301551..68b27c69cc3d 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -382,6 +382,8 @@ struct drm_bridge {
> >  	struct drm_encoder *encoder;
> >  	/** @next: the next bridge in the encoder chain */
> >  	struct drm_bridge *next;
> > +	/** @device: Linux driver model device */
> > +	struct device *device;
> >  #ifdef CONFIG_OF
> >  	/** @of_node: device node pointer to the bridge */
> >  	struct device_node *of_node;
> > @@ -407,6 +409,8 @@ void drm_bridge_init(struct drm_bridge *bridge, struct device *dev,
> >  		     const struct drm_bridge_timings *timings,
> >  		     void *driver_private);
> >  struct drm_bridge *of_drm_find_bridge(struct device_node *np);
> > +struct drm_bridge *of_drm_find_bridge_devlink(struct drm_device *dev,
> > +					      struct device_node *np);
> >  int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
> >  		      struct drm_bridge *previous);
> >  
> 
> 


-- 
Mihail

[PATCH 30/30] drm/komeda: Use drm_bridge interface for pipe outputs

[Mihail Atanassov]

To support more transmitters, we need to allow non-component framework
bridges to be attached to a dummy drm_encoder in our driver.

A/B tested for equivalence against tda998x, and also tested ti_tfp410 as
an alternate transmitter.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 .../gpu/drm/arm/display/komeda/komeda_drv.c   | 54 ++++++-------
 .../gpu/drm/arm/display/komeda/komeda_kms.c   | 77 +++++++++++++++++--
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  2 +
 3 files changed, 100 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index d6c2222c5d33..2870123bef37 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -6,10 +6,10 @@
  */
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/of_graph.h>
 #include <linux/platform_device.h>
-#include <linux/component.h>
 #include <linux/pm_runtime.h>
-#include <drm/drm_of.h>
+#include <drm/drm_bridge.h>
 #include "komeda_dev.h"
 #include "komeda_kms.h"
 
@@ -72,35 +72,29 @@ static int komeda_bind(struct device *dev)
 	return err;
 }
 
-static const struct component_master_ops komeda_master_ops = {
-	.bind	= komeda_bind,
-	.unbind	= komeda_unbind,
-};
-
-static int compare_of(struct device *dev, void *data)
-{
-	return dev->of_node == data;
-}
-
-static void komeda_add_slave(struct device *master,
-			     struct component_match **match,
-			     struct device_node *np,
-			     u32 port, u32 endpoint)
+static int komeda_add_slave(struct device_node *np, u32 port, u32 endpoint)
 {
 	struct device_node *remote;
+	struct drm_bridge *bridge;
+	int ret = 0;
 
 	remote = of_graph_get_remote_node(np, port, endpoint);
-	if (remote) {
-		drm_of_component_match_add(master, match, compare_of, remote);
-		of_node_put(remote);
-	}
+	if (!remote)
+		return 0;
+
+	bridge = of_drm_find_bridge(remote);
+	if (!bridge)
+		ret = -EPROBE_DEFER;
+
+	of_node_put(remote);
+	return ret;
 }
 
 static int komeda_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
 	struct device_node *child;
+	int ret;
 
 	if (!dev->of_node)
 		return -ENODEV;
@@ -109,17 +103,25 @@ static int komeda_platform_probe(struct platform_device *pdev)
 		if (of_node_cmp(child->name, "pipeline") != 0)
 			continue;
 
-		/* add connector */
-		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 0);
-		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
+		/* attach any remote devices if present */
+		ret = komeda_add_slave(child, KOMEDA_OF_PORT_OUTPUT, 0);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
+		ret = komeda_add_slave(child, KOMEDA_OF_PORT_OUTPUT, 1);
+		if (ret) {
+			of_node_put(child);
+			return ret;
+		}
 	}
 
-	return component_master_add_with_match(dev, &komeda_master_ops, match);
+	return komeda_bind(dev);
 }
 
 static int komeda_platform_remove(struct platform_device *pdev)
 {
-	component_master_del(&pdev->dev, &komeda_master_ops);
+	komeda_unbind(&pdev->dev);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index e30a5b43caa9..2fc6cd9956fd 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -4,8 +4,8 @@
  * Author: James.Qian.Wang <james.qian.wang@arm.com>
  *
  */
-#include <linux/component.h>
 #include <linux/interrupt.h>
+#include <linux/of_graph.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -14,6 +14,9 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_irq.h>
+#include <drm/drm_of.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
 
@@ -257,6 +260,69 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
 	config->helper_private = &komeda_mode_config_helpers;
 }
 
+static void komeda_encoder_destroy(struct drm_encoder *encoder)
+{
+	drm_encoder_cleanup(encoder);
+}
+
+static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
+	.destroy = komeda_encoder_destroy,
+};
+
+static int komeda_encoder_attach_bridge(struct komeda_dev *mdev,
+					struct komeda_kms_dev *kms,
+					struct drm_encoder *encoder,
+					struct device_node *np)
+{
+	struct device_node *remote;
+	struct drm_bridge *bridge;
+	u32 crtcs = 0;
+	int ret = 0;
+
+	remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0);
+	if (!remote)
+		return 0;
+
+	bridge = of_drm_find_bridge_devlink(&kms->base, remote);
+	if (!bridge) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	crtcs = drm_of_find_possible_crtcs(&kms->base, remote);
+
+	encoder->possible_crtcs = crtcs ? crtcs : 1;
+
+	ret = drm_encoder_init(&kms->base, encoder, &komeda_dummy_enc_funcs,
+			       DRM_MODE_ENCODER_TMDS, NULL);
+	if (ret)
+		goto exit;
+
+	ret = drm_bridge_attach(encoder, bridge, NULL);
+	if (ret)
+		goto exit;
+
+exit:
+	of_node_put(remote);
+	return ret;
+}
+
+static int komeda_encoder_attach_bridges(struct komeda_kms_dev *kms,
+					 struct komeda_dev *mdev)
+{
+	int i, err;
+
+	for (i = 0; i < kms->n_crtcs; i++) {
+		err = komeda_encoder_attach_bridge(
+			mdev, kms,
+			&kms->encoders[i], mdev->pipelines[i]->of_node);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
 {
 	struct komeda_kms_dev *kms = kzalloc(sizeof(*kms), GFP_KERNEL);
@@ -295,7 +361,7 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
 	if (err)
 		goto cleanup_mode_config;
 
-	err = component_bind_all(mdev->dev, kms);
+	err = komeda_encoder_attach_bridges(kms, mdev);
 	if (err)
 		goto cleanup_mode_config;
 
@@ -305,11 +371,11 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
 			       komeda_kms_irq_handler, IRQF_SHARED,
 			       drm->driver->name, drm);
 	if (err)
-		goto free_component_binding;
+		goto cleanup_mode_config;
 
 	err = mdev->funcs->enable_irq(mdev);
 	if (err)
-		goto free_component_binding;
+		goto cleanup_mode_config;
 
 	drm->irq_enabled = true;
 
@@ -325,8 +391,6 @@ struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev)
 	drm_kms_helper_poll_fini(drm);
 	drm->irq_enabled = false;
 	mdev->funcs->disable_irq(mdev);
-free_component_binding:
-	component_unbind_all(mdev->dev, drm);
 cleanup_mode_config:
 	drm_mode_config_cleanup(drm);
 	komeda_kms_cleanup_private_objs(kms);
@@ -347,7 +411,6 @@ void komeda_kms_detach(struct komeda_kms_dev *kms)
 	drm_atomic_helper_shutdown(drm);
 	drm->irq_enabled = false;
 	mdev->funcs->disable_irq(mdev);
-	component_unbind_all(mdev->dev, drm);
 	drm_mode_config_cleanup(drm);
 	komeda_kms_cleanup_private_objs(kms);
 	drm->dev_private = NULL;
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719..b40ba0f476b1 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -12,6 +12,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_device.h>
+#include <drm/drm_encoder.h>
 #include <drm/drm_writeback.h>
 #include <drm/drm_print.h>
 
@@ -123,6 +124,7 @@ struct komeda_kms_dev {
 	int n_crtcs;
 	/** @crtcs: crtcs list */
 	struct komeda_crtc crtcs[KOMEDA_MAX_PIPELINES];
+	struct drm_encoder encoders[KOMEDA_MAX_PIPELINES];
 };
 
 #define to_kplane(p)	container_of(p, struct komeda_plane, base)
-- 
2.23.0