| Message ID | 20191204114732.28514-2-mihail.atanassov@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Consolidate initialization | expand |
On Wed, Dec 04, 2019 at 11:48:02AM +0000, Mihail Atanassov wrote: > A simple convenience function to initialize the struct drm_bridge. The > goal is to standardize initialization for any bridge registered with > drm_bridge_add() so that we can later add device links for consumers of > those bridges. > > v2: > - s/WARN_ON(!funcs)/WARN_ON(!funcs || !dev)/ as suggested by Daniel > - expand on some kerneldoc comments as suggested by Daniel > - update commit message as suggested by Daniel > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_bridge.c | 34 +++++++++++++++++++++++++++++++++- > include/drm/drm_bridge.h | 15 ++++++++++++++- > 2 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index cba537c99e43..50e1c1b46e20 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -64,7 +64,10 @@ static DEFINE_MUTEX(bridge_lock); > static LIST_HEAD(bridge_list); > > /** > - * drm_bridge_add - add the given bridge to the global bridge list > + * drm_bridge_add - add the given bridge to the global bridge list. > + * > + * Drivers should call drm_bridge_init() prior adding it to the list. > + * Drivers should call drm_bridge_remove() to clean up the bridge list. > * > * @bridge: bridge control structure > */ > @@ -89,6 +92,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 associated with this drm_bridge > + * @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 || !dev); > + > + 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..949e4f401a53 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -373,7 +373,16 @@ struct drm_bridge_timings { > }; > > /** > - * struct drm_bridge - central DRM bridge control structure > + * struct drm_bridge - central DRM bridge control structure. > + * > + * Bridge drivers should call drm_bridge_init() to initialize 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(). > + * > + * Modular drivers in OF systems can query the list of registered bridges > + * with of_drm_find_bridge(). > */ > struct drm_bridge { > /** @dev: DRM device this bridge belongs to */ > @@ -402,6 +411,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 >
Hi Mihail, Thank you for the patch. On Wed, Dec 04, 2019 at 11:48:02AM +0000, Mihail Atanassov wrote: > A simple convenience function to initialize the struct drm_bridge. The > goal is to standardize initialization for any bridge registered with > drm_bridge_add() so that we can later add device links for consumers of > those bridges. > > v2: > - s/WARN_ON(!funcs)/WARN_ON(!funcs || !dev)/ as suggested by Daniel > - expand on some kerneldoc comments as suggested by Daniel > - update commit message as suggested by Daniel > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> > --- > drivers/gpu/drm/drm_bridge.c | 34 +++++++++++++++++++++++++++++++++- > include/drm/drm_bridge.h | 15 ++++++++++++++- > 2 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index cba537c99e43..50e1c1b46e20 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -64,7 +64,10 @@ static DEFINE_MUTEX(bridge_lock); > static LIST_HEAD(bridge_list); > > /** > - * drm_bridge_add - add the given bridge to the global bridge list > + * drm_bridge_add - add the given bridge to the global bridge list. You add a final period here and in the documentation of struct drm_bridge, but the new function you're adding doesn't have one :-) I'd drop the period here and for drm_bridge to be consistent with the rest of the code. > + * > + * Drivers should call drm_bridge_init() prior adding it to the list. s/should/shall/ s/prior adding it/prior to adding the bridge/ > + * Drivers should call drm_bridge_remove() to clean up the bridge list. I'd replace this sentence with "Before deleting a bridge (usually when the driver is unbound from the device), drivers shall call drm_bridge_remove() to remove it from the global list." > * > * @bridge: bridge control structure > */ > @@ -89,6 +92,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) > } > EXPORT_SYMBOL(drm_bridge_remove); > > +/** > + * drm_bridge_init - initialise a drm_bridge structure initialise or initialize ? :-) > + * > + * @bridge: bridge control structure > + * @funcs: control functions > + * @dev: device associated with this drm_bridge dev goes before funcs > + * @timings: timing specification for the bridge; optional (may be NULL) > + * @driver_private: pointer to the bridge driver internal context (may be NULL) I'm not too sure about the last two parameters. Having NULL, NULL in most callers is confusing, and having a void * as one of the parameters means that the compiler won't catch errors if the two parameters are reversed. I think this is quite error prone. There are very few drivers using driver_private (I count 6 of them, with 2 additional drivers that set driver_private but never use it). How about embedding the bridge structure in those 6 drivers and getting rid of the field altogether ? This could be part of a separate series, but in any case I'd drop driver_private from drm_bridge_init(). > + */ > +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 || !dev); > + > + bridge->dev = NULL; NULL ? Shouldn't this be dev ? > + 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..949e4f401a53 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -373,7 +373,16 @@ struct drm_bridge_timings { > }; > > /** > - * struct drm_bridge - central DRM bridge control structure > + * struct drm_bridge - central DRM bridge control structure. > + * > + * Bridge drivers should call drm_bridge_init() to initialize a bridge > + * driver, and then register it with drm_bridge_add(). s/bridge driver/bridge structure/ (or drm_bridge structure) > + * > + * Users of bridges link a bridge driver into their overall display output > + * pipeline by calling drm_bridge_attach(). > + * > + * Modular drivers in OF systems can query the list of registered bridges > + * with of_drm_find_bridge(). > */ > struct drm_bridge { > /** @dev: DRM device this bridge belongs to */ > @@ -402,6 +411,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);
Hi Laurent, On Thursday, 5 December 2019 12:40:22 GMT Laurent Pinchart wrote: > Hi Mihail, > > Thank you for the patch. Thanks for the quick reviews :). > > On Wed, Dec 04, 2019 at 11:48:02AM +0000, Mihail Atanassov wrote: > > A simple convenience function to initialize the struct drm_bridge. The > > goal is to standardize initialization for any bridge registered with > > drm_bridge_add() so that we can later add device links for consumers of > > those bridges. > > > > v2: > > - s/WARN_ON(!funcs)/WARN_ON(!funcs || !dev)/ as suggested by Daniel > > - expand on some kerneldoc comments as suggested by Daniel > > - update commit message as suggested by Daniel > > > > Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> > > --- > > drivers/gpu/drm/drm_bridge.c | 34 +++++++++++++++++++++++++++++++++- > > include/drm/drm_bridge.h | 15 ++++++++++++++- > > 2 files changed, 47 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index cba537c99e43..50e1c1b46e20 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -64,7 +64,10 @@ static DEFINE_MUTEX(bridge_lock); > > static LIST_HEAD(bridge_list); > > > > /** > > - * drm_bridge_add - add the given bridge to the global bridge list > > + * drm_bridge_add - add the given bridge to the global bridge list. > > You add a final period here and in the documentation of struct > drm_bridge, but the new function you're adding doesn't have one :-) I'd > drop the period here and for drm_bridge to be consistent with the rest > of the code. > > > + * > > + * Drivers should call drm_bridge_init() prior adding it to the list. > > s/should/shall/ > s/prior adding it/prior to adding the bridge/ > > > + * Drivers should call drm_bridge_remove() to clean up the bridge list. > > I'd replace this sentence with "Before deleting a bridge (usually when > the driver is unbound from the device), drivers shall call > drm_bridge_remove() to remove it from the global list." > > > * > > * @bridge: bridge control structure > > */ > > @@ -89,6 +92,35 @@ void drm_bridge_remove(struct drm_bridge *bridge) > > } > > EXPORT_SYMBOL(drm_bridge_remove); > > > > +/** > > + * drm_bridge_init - initialise a drm_bridge structure > > initialise or initialize ? :-) I have absolutely no clue :). Judging by the question I'm guessing the correct answer for the kernel is US spelling. > > > + * > > + * @bridge: bridge control structure > > + * @funcs: control functions > > + * @dev: device associated with this drm_bridge > > dev goes before funcs > > > + * @timings: timing specification for the bridge; optional (may be NULL) > > + * @driver_private: pointer to the bridge driver internal context (may be NULL) > > I'm not too sure about the last two parameters. Having NULL, NULL in > most callers is confusing, and having a void * as one of the parameters > means that the compiler won't catch errors if the two parameters are > reversed. I think this is quite error prone. > > There are very few drivers using driver_private (I count 6 of them, with > 2 additional drivers that set driver_private but never use it). How > about embedding the bridge structure in those 6 drivers and getting rid > of the field altogether ? This could be part of a separate series, but > in any case I'd drop driver_private from drm_bridge_init(). Ok, I'll do that first before refreshing this series. > > > + */ > > +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 || !dev); > > + > > + bridge->dev = NULL; > > NULL ? Shouldn't this be dev ? Hehe, Daniel got caught on that one, too :). This is the drm_device pointer for the bound consumer, not the struct device that the bridge's lifetime is tied to. I was planning to rename them with my (eventual) device_links addition (some discussion around it here: https://patchwork.freedesktop.org/patch/342472/?series=70039&rev=1). I guess if I do the drm_device part of the rename first, this patch will look less confusing, so I'll do that too. > > > + 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..949e4f401a53 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -373,7 +373,16 @@ struct drm_bridge_timings { > > }; > > > > /** > > - * struct drm_bridge - central DRM bridge control structure > > + * struct drm_bridge - central DRM bridge control structure. > > + * > > + * Bridge drivers should call drm_bridge_init() to initialize a bridge > > + * driver, and then register it with drm_bridge_add(). > > s/bridge driver/bridge structure/ (or drm_bridge structure) > > > + * > > + * Users of bridges link a bridge driver into their overall display output > > + * pipeline by calling drm_bridge_attach(). > > + * > > + * Modular drivers in OF systems can query the list of registered bridges > > + * with of_drm_find_bridge(). > > */ > > struct drm_bridge { > > /** @dev: DRM device this bridge belongs to */ > > @@ -402,6 +411,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); > >
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index cba537c99e43..50e1c1b46e20 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -64,7 +64,10 @@ static DEFINE_MUTEX(bridge_lock); static LIST_HEAD(bridge_list); /** - * drm_bridge_add - add the given bridge to the global bridge list + * drm_bridge_add - add the given bridge to the global bridge list. + * + * Drivers should call drm_bridge_init() prior adding it to the list. + * Drivers should call drm_bridge_remove() to clean up the bridge list. * * @bridge: bridge control structure */ @@ -89,6 +92,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 associated with this drm_bridge + * @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 || !dev); + + 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..949e4f401a53 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -373,7 +373,16 @@ struct drm_bridge_timings { }; /** - * struct drm_bridge - central DRM bridge control structure + * struct drm_bridge - central DRM bridge control structure. + * + * Bridge drivers should call drm_bridge_init() to initialize 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(). + * + * Modular drivers in OF systems can query the list of registered bridges + * with of_drm_find_bridge(). */ struct drm_bridge { /** @dev: DRM device this bridge belongs to */ @@ -402,6 +411,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);
A simple convenience function to initialize the struct drm_bridge. The goal is to standardize initialization for any bridge registered with drm_bridge_add() so that we can later add device links for consumers of those bridges. v2: - s/WARN_ON(!funcs)/WARN_ON(!funcs || !dev)/ as suggested by Daniel - expand on some kerneldoc comments as suggested by Daniel - update commit message as suggested by Daniel Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com> --- drivers/gpu/drm/drm_bridge.c | 34 +++++++++++++++++++++++++++++++++- include/drm/drm_bridge.h | 15 ++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-)