From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Mihail Atanassov <Mihail.Atanassov@arm.com> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, David Airlie <airlied@linux.ie>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, nd <nd@arm.com> Subject: Re: [PATCH v2 01/28] drm: Introduce drm_bridge_init() Date: Thu, 5 Dec 2019 14:40:22 +0200 [thread overview] Message-ID: <20191205124022.GA16034@pendragon.ideasonboard.com> (raw) In-Reply-To: <20191204114732.28514-2-mihail.atanassov@arm.com> 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); -- Regards, Laurent Pinchart
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Mihail Atanassov <Mihail.Atanassov@arm.com> Cc: David Airlie <airlied@linux.ie>, nd <nd@arm.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org> Subject: Re: [PATCH v2 01/28] drm: Introduce drm_bridge_init() Date: Thu, 5 Dec 2019 14:40:22 +0200 [thread overview] Message-ID: <20191205124022.GA16034@pendragon.ideasonboard.com> (raw) In-Reply-To: <20191204114732.28514-2-mihail.atanassov@arm.com> 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); -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-12-05 12:40 UTC|newest] Thread overview: 97+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-04 11:48 [PATCH v2 00/28] drm/bridge: Consolidate initialization Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 01/28] drm: Introduce drm_bridge_init() Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 13:10 ` Daniel Vetter 2019-12-04 13:10 ` Daniel Vetter 2019-12-05 12:40 ` Laurent Pinchart [this message] 2019-12-05 12:40 ` Laurent Pinchart 2019-12-05 14:25 ` Mihail Atanassov 2019-12-05 14:25 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 02/28] drm/bridge: adv7511: Use drm_bridge_init() Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 03/28] drm/bridge/analogix: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 13:29 ` Laurent Pinchart 2019-12-05 13:29 ` Laurent Pinchart 2019-12-04 11:48 ` [PATCH v2 04/28] drm/bridge: cdns: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 05/28] drm/bridge: dumb-vga-dac: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 06/28] drm/bridge: lvds-encoder: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 07/28] drm/bridge: megachips-stdpxxxx-ge-b850v3-fw: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 08/28] drm/bridge: nxp-ptn3460: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 10/28] drm/bridge: ps8622: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 09/28] drm/bridge: panel: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 11/28] drm/bridge: sii902x: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 12/28] gpu: drm: bridge: sii9234: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 12:10 ` Neil Armstrong 2019-12-05 12:10 ` Neil Armstrong 2019-12-05 14:25 ` Mihail Atanassov 2019-12-05 14:25 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 13/28] drm/bridge: sil_sii8620: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 14/28] drm/bridge/synopsys: dw-hdmi: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 15/28] drm/bridge/synopsys: dsi: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 16/28] drm/bridge: tc358764: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 17/28] drm/bridge: tc358767: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 19/28] drm/bridge: ti-sn65dsi86: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 18/28] drm/bridge: thc63: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 13:33 ` Laurent Pinchart 2019-12-05 13:33 ` Laurent Pinchart 2019-12-04 11:48 ` [PATCH v2 20/28] drm/bridge: ti-tfp410: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 21/28] drm/exynos: mic: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 22/28] drm/i2c: tda998x: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 23/28] drm/mcde: dsi: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` [PATCH v2 25/28] drm: rcar-du: lvds: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 13:31 ` Laurent Pinchart 2019-12-05 13:31 ` Laurent Pinchart 2019-12-04 11:48 ` [PATCH v2 24/28] drm/mediatek: hdmi: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-10 1:24 ` CK Hu 2019-12-10 1:24 ` CK Hu 2019-12-10 1:24 ` CK Hu 2019-12-10 1:24 ` CK Hu 2019-12-04 11:48 ` [PATCH v2 26/28] drm: rcar-du: lvds: Don't set drm_bridge private pointer Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 13:31 ` Laurent Pinchart 2019-12-05 13:31 ` Laurent Pinchart 2019-12-04 11:48 ` [PATCH v2 27/28] drm/sti: Use drm_bridge_init() Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 13:39 ` Benjamin Gaignard 2019-12-05 13:39 ` Benjamin Gaignard 2019-12-04 11:48 ` [PATCH v2 28/28] drm/msm: " Mihail Atanassov 2019-12-04 11:48 ` Mihail Atanassov 2019-12-05 13:34 ` [PATCH v2 00/28] drm/bridge: Consolidate initialization Laurent Pinchart 2019-12-05 13:34 ` Laurent Pinchart 2019-12-05 13:34 ` Laurent Pinchart 2019-12-09 10:39 ` Neil Armstrong 2019-12-09 10:39 ` Neil Armstrong 2019-12-09 10:39 ` Neil Armstrong 2019-12-09 10:39 ` Neil Armstrong 2019-12-09 11:08 ` Mihail Atanassov 2019-12-09 11:08 ` Mihail Atanassov 2019-12-09 11:08 ` Mihail Atanassov 2019-12-09 11:08 ` Mihail Atanassov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191205124022.GA16034@pendragon.ideasonboard.com \ --to=laurent.pinchart@ideasonboard.com \ --cc=Mihail.Atanassov@arm.com \ --cc=airlied@linux.ie \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=nd@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.