[PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Jean-Francois Moine]

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);
 	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;
 
-- 
1.9.rc1

[PATCH v3 2/2] drivers/base: declare phandle DT nodes as components

[Jean-Francois Moine]

At system startup time, some devices depends on the availability of
some other devices before starting. The infrastructure for componentised
subsystems permits to handle this dependence, each driver defining
its own role.

This patch does an automatic creation of the lowest components in
case of DT. This permits simple devices to be part of complex
componentised subsystems without any specific code.

When created from DT, the device dependence is generally indicated by
phandle: a device which is pointed to by a phandle must be started
before the pointing device(s).

So, at device register time, the devices which are pointed to by a
phandle are declared as components, except when they declared
themselves as such in their probe function.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/base/component.c | 12 ++++++++++++
 drivers/base/core.c      | 18 ++++++++++++++++++
 include/linux/of.h       |  2 ++
 3 files changed, 32 insertions(+)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 0a39d7a..3cab26b 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 struct master {
 	struct list_head node;
@@ -336,6 +337,7 @@ EXPORT_SYMBOL_GPL(component_bind_all);
 int component_add(struct device *dev, const struct component_ops *ops)
 {
 	struct component *component;
+	struct device_node *np;
 	int ret;
 
 	component = kzalloc(sizeof(*component), GFP_KERNEL);
@@ -356,6 +358,11 @@ int component_add(struct device *dev, const struct component_ops *ops)
 
 		kfree(component);
 	}
+
+	np = dev->of_node;
+	if (np)
+		np->_flags |= OF_DEV_COMPONENT;
+
 	mutex_unlock(&component_mutex);
 
 	return ret < 0 ? ret : 0;
@@ -365,6 +372,7 @@ EXPORT_SYMBOL_GPL(component_add);
 void component_del(struct device *dev, const struct component_ops *ops)
 {
 	struct component *c, *component = NULL;
+	struct device_node *np;
 
 	mutex_lock(&component_mutex);
 	list_for_each_entry(c, &component_list, node)
@@ -377,6 +385,10 @@ void component_del(struct device *dev, const struct component_ops *ops)
 	if (component && component->master)
 		take_down_master(component->master);
 
+	np = dev->of_node;
+	if (np)
+		np->_flags &= ~OF_DEV_COMPONENT;
+
 	mutex_unlock(&component_mutex);
 
 	WARN_ON(!component);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2b56717..0517b91 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -27,6 +27,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/netdevice.h>
 #include <linux/sysfs.h>
+#include <linux/component.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -980,6 +981,7 @@ int device_add(struct device *dev)
 	struct device *parent = NULL;
 	struct kobject *kobj;
 	struct class_interface *class_intf;
+	struct device_node *np;
 	int error = -EINVAL;
 
 	dev = get_device(dev);
@@ -1088,6 +1090,15 @@ int device_add(struct device *dev)
 				class_intf->add_dev(dev, class_intf);
 		mutex_unlock(&dev->class->p->mutex);
 	}
+
+	/* if DT-created device referenced by phandle, create a component */
+	np = dev->of_node;
+	if (np && np->phandle &&
+	    !(np->_flags & OF_DEV_COMPONENT)) {
+		component_add(dev, NULL);
+		np->_flags |= OF_DEV_IMPLICIT_COMPONENT;
+	}
+
 done:
 	put_device(dev);
 	return error;
@@ -1189,10 +1200,17 @@ void device_del(struct device *dev)
 {
 	struct device *parent = dev->parent;
 	struct class_interface *class_intf;
+	struct device_node *np;
 
 	/* Notify clients of device removal.  This call must come
 	 * before dpm_sysfs_remove().
 	 */
+	np = dev->of_node;
+	if (np && (np->_flags & OF_DEV_COMPONENT)) {
+		component_del(dev, NULL);
+		np->_flags &= ~OF_DEV_IMPLICIT_COMPONENT;
+	}
+
 	if (dev->bus)
 		blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 					     BUS_NOTIFY_DEL_DEVICE, dev);
diff --git a/include/linux/of.h b/include/linux/of.h
index 70c64ba..40f1c34 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -59,6 +59,8 @@ struct device_node {
 	struct	proc_dir_entry *pde;	/* this node's proc directory */
 	struct	kref kref;
 	unsigned long _flags;
+#define OF_DEV_COMPONENT 1
+#define OF_DEV_IMPLICIT_COMPONENT 2
 	void	*data;
 #if defined(CONFIG_SPARC)
 	const char *path_component_name;
-- 
1.9.rc1

[PATCH v3 0/2] *** SUBJECT HERE ***

[Jean-Francois Moine]

*** BLURB HERE ***

Jean-Francois Moine (2):
  drivers/base: permit base components to omit the bind/unbind ops
  drivers/base: declare phandle DT nodes as components

 drivers/base/component.c | 21 +++++++++++++++++++--
 drivers/base/core.c      | 18 ++++++++++++++++++
 include/linux/of.h       |  2 ++
 3 files changed, 39 insertions(+), 2 deletions(-)

-- 
1.9.rc1

Re: [PATCH v3 0/2] *** SUBJECT HERE ***

[Greg Kroah-Hartman]

On Fri, Feb 07, 2014 at 06:09:46PM +0100, Jean-Francois Moine wrote:
> *** BLURB HERE ***

Subject and BLURB forgotten?

Re: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Jean-Francois Moine]

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.


-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v3 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Russell King - ARM Linux]

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.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".