Message ID | 9b3c3c2c982f31b026fd1516a2b608026d55b1e9.1391792986.git.moinejf@free.fr (mailing list archive) |
---|---|
State | Not Applicable, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1WBp7K-0006at-52; Fri, 07 Feb 2014 18:21:30 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-8) with esmtp id 1WBp7I-0008IO-kK; Fri, 07 Feb 2014 18:21:30 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752065AbaBGRVD (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 7 Feb 2014 12:21:03 -0500 Received: from smtp5-g21.free.fr ([212.27.42.5]:59066 "EHLO smtp5-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751617AbaBGRVB (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 7 Feb 2014 12:21:01 -0500 Received: from localhost (unknown [IPv6:2a01:e35:2f5c:9de0:212:bfff:fe1e:9ce4]) by smtp5-g21.free.fr (Postfix) with ESMTP id E7AAAD4804B; Fri, 7 Feb 2014 18:20:43 +0100 (CET) X-Mailbox-Line: From 9b3c3c2c982f31b026fd1516a2b608026d55b1e9 Mon Sep 17 00:00:00 2001 Message-Id: <9b3c3c2c982f31b026fd1516a2b608026d55b1e9.1391792986.git.moinejf@free.fr> In-Reply-To: <cover.1391792986.git.moinejf@free.fr> References: <cover.1391792986.git.moinejf@free.fr> From: Jean-Francois Moine <moinejf@free.fr> Date: Fri, 7 Feb 2014 16:55:00 +0100 Subject: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops To: Russell King <rmk+kernel@arm.linux.org.uk>, devel@driverdev.osuosl.org Cc: alsa-devel@alsa-project.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, dri-devel@lists.freedesktop.org, Takashi Iwai <tiwai@suse.de>, Sascha Hauer <kernel@pengutronix.de>, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org, Daniel Vetter <daniel@ffwll.ch> Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2014.2.7.171216 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1400_1499 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Jean-Francois Moine
Feb. 7, 2014, 3:55 p.m. UTC
Some simple components don't need to do any specific action on
bind to / unbind from a master component.
This patch permits such components to omit the bind/unbind
operations.
Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
drivers/base/component.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
Comments
On Fri, Feb 07, 2014 at 04:55:00PM +0100, Jean-Francois Moine wrote: > Some simple components don't need to do any specific action on > bind to / unbind from a master component. > > This patch permits such components to omit the bind/unbind > operations. > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > --- > drivers/base/component.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/component.c b/drivers/base/component.c > index c53efe6..0a39d7a 100644 > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -225,7 +225,8 @@ static void component_unbind(struct component *component, > { > WARN_ON(!component->bound); > > - component->ops->unbind(component->dev, master->dev, data); > + if (component->ops) > + component->ops->unbind(component->dev, master->dev, data); This doesn't actually do what the commit message says. This makes component->ops optional, not component->ops->unbind(). A more correct check would be: if (component->ops && component->ops->unbind) > component->bound = false; > > /* Release all resources claimed in the binding of this component */ > @@ -274,7 +275,11 @@ static int component_bind(struct component *component, struct master *master, > dev_dbg(master->dev, "binding %s (ops %ps)\n", > dev_name(component->dev), component->ops); > > - ret = component->ops->bind(component->dev, master->dev, data); > + if (component->ops) > + ret = component->ops->bind(component->dev, master->dev, data); Same here. Thierry
On Mon, Feb 10, 2014 at 01:53:08PM +0100, Thierry Reding wrote: > On Fri, Feb 07, 2014 at 04:55:00PM +0100, Jean-Francois Moine wrote: > > Some simple components don't need to do any specific action on > > bind to / unbind from a master component. > > > > This patch permits such components to omit the bind/unbind > > operations. > > > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr> > > --- > > drivers/base/component.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/component.c b/drivers/base/component.c > > index c53efe6..0a39d7a 100644 > > --- a/drivers/base/component.c > > +++ b/drivers/base/component.c > > @@ -225,7 +225,8 @@ static void component_unbind(struct component *component, > > { > > WARN_ON(!component->bound); > > > > - component->ops->unbind(component->dev, master->dev, data); > > + if (component->ops) > > + component->ops->unbind(component->dev, master->dev, data); > > This doesn't actually do what the commit message says. This makes > component->ops optional, not component->ops->unbind(). > > A more correct check would be: > > if (component->ops && component->ops->unbind) > > > component->bound = false; > > > > /* Release all resources claimed in the binding of this component */ > > @@ -274,7 +275,11 @@ static int component_bind(struct component *component, struct master *master, > > dev_dbg(master->dev, "binding %s (ops %ps)\n", > > dev_name(component->dev), component->ops); > > > > - ret = component->ops->bind(component->dev, master->dev, data); > > + if (component->ops) > > + ret = component->ops->bind(component->dev, master->dev, data); > > Same here. I've NAK'd these patches already - I believe they're based on a mis-understanding of how this should be used. I believe Jean-Francois has only looked at the core, rather than looking at the imx-drm example it was posted with in an attempt to understand it. Omitting the component bind operations is absurd because it makes the component code completely pointless, since there is then no way to control the sequencing of driver initialisation - something which is one of the primary reasons for this code existing in the first place.
On Mon, 10 Feb 2014 13:12:33 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > I've NAK'd these patches already - I believe they're based on a > mis-understanding of how this should be used. I believe Jean-Francois > has only looked at the core, rather than looking at the imx-drm example > it was posted with in an attempt to understand it. > > Omitting the component bind operations is absurd because it makes the > component code completely pointless, since there is then no way to > control the sequencing of driver initialisation - something which is > one of the primary reasons for this code existing in the first place. I perfectly looked at your example and I use it now in my system. You did not see what could be done with your component code. For example, since november, I have not yet the clock probe_defer in the mainline (http://www.spinics.net/lists/arm-kernel/msg306072.html), so, there are 3 solutions: - hope the patch will be some day in the mainline and, today, reboot when the system does not start correctly, - insert a delay in the tda998x and kirkwood probe sequences (delay long enough to be sure the si5351 is started, or loop), - use your component work. In the last case, it is easy: - the si5351 driver calls component_add (with empty ops: it has no interest in the bind/unbind functions) when it is fully started (i.e. registered - that was the subject of my patch), - in the DRM driver, look for the si5351 as a clock in the DT (drm -> encoder -> clock), and add it to the awaited components (CRTCs, encoders..), - in the audio subsystem, look for the si5351 as an external clock in the DT (simple-card -> CPU DAI -> clock) and add it to the awaited components (CPU and CODEC DAIs - yes, the S/PDIF CODEC should also be a component with no bin/unbind ops). Then, when the si5351 is registered, both master components video and audio can safely run.
On Mon, Feb 10, 2014 at 03:35:51PM +0100, Jean-Francois Moine wrote: > On Mon, 10 Feb 2014 13:12:33 +0000 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > > I've NAK'd these patches already - I believe they're based on a > > mis-understanding of how this should be used. I believe Jean-Francois > > has only looked at the core, rather than looking at the imx-drm example > > it was posted with in an attempt to understand it. > > > > Omitting the component bind operations is absurd because it makes the > > component code completely pointless, since there is then no way to > > control the sequencing of driver initialisation - something which is > > one of the primary reasons for this code existing in the first place. > > I perfectly looked at your example and I use it now in my system. > > You did not see what could be done with your component code. For > example, since november, I have not yet the clock probe_defer in the > mainline (http://www.spinics.net/lists/arm-kernel/msg306072.html), so, > there are 3 solutions: > > - hope the patch will be some day in the mainline and, today, reboot > when the system does not start correctly, > > - insert a delay in the tda998x and kirkwood probe sequences (delay > long enough to be sure the si5351 is started, or loop), > > - use your component work. > > In the last case, it is easy: > > - the si5351 driver calls component_add (with empty ops: it has no > interest in the bind/unbind functions) when it is fully started (i.e. > registered - that was the subject of my patch), There is only one word for this: Ewwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww. Definitely not. The component stuff is there to sort out the subsystems which expect a "card" but in DT we supply a set of components. It's not there to sort out dependencies between different subsystems. I've no idea why your patch to add the deferred probing hasn't been acked by Mike yet, but that needs to happen before I take it. Or, split it up in two so I can take the clkdev part and Mike can take the CCF part.
diff --git a/drivers/base/component.c b/drivers/base/component.c index c53efe6..0a39d7a 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -225,7 +225,8 @@ static void component_unbind(struct component *component, { WARN_ON(!component->bound); - component->ops->unbind(component->dev, master->dev, data); + if (component->ops) + component->ops->unbind(component->dev, master->dev, data); component->bound = false; /* Release all resources claimed in the binding of this component */ @@ -274,7 +275,11 @@ static int component_bind(struct component *component, struct master *master, dev_dbg(master->dev, "binding %s (ops %ps)\n", dev_name(component->dev), component->ops); - ret = component->ops->bind(component->dev, master->dev, data); + if (component->ops) + ret = component->ops->bind(component->dev, master->dev, data); + else + ret = 0; + if (!ret) { component->bound = true;