[01/30] drm: Introduce drm_bridge_init()
diff mbox series

Message ID 20191126131541.47393-2-mihail.atanassov@arm.com
State New
Headers show
Series
  • drm/bridge: Add device links for lifetime control
Related show

Commit Message

Mihail Atanassov Nov. 26, 2019, 1:15 p.m. UTC
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(+)

Comments

Daniel Vetter Nov. 26, 2019, 2:26 p.m. UTC | #1
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
>
Mihail Atanassov Nov. 26, 2019, 3:55 p.m. UTC | #2
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);
> 
>
Daniel Vetter Nov. 26, 2019, 5:04 p.m. UTC | #3
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
Sam Ravnborg Nov. 26, 2019, 7:24 p.m. UTC | #4
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
Mihail Atanassov Nov. 27, 2019, 11:05 a.m. UTC | #5
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?
Daniel Vetter Nov. 27, 2019, 11:31 a.m. UTC | #6
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
james qian wang (Arm Technology China) Dec. 2, 2019, 5:55 a.m. UTC | #7
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);
Daniel Vetter Dec. 2, 2019, 8:49 a.m. UTC | #8
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
james qian wang (Arm Technology China) Dec. 3, 2019, 6:12 a.m. UTC | #9
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
diff mbox series

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);