[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

This patch series tries to simplify the code of simple devices in case
they are part of componentised subsystems, are declared in a DT, and
are not using the component bin/unbind functions.

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

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

This patch series tries to simplify the code of simple devices in case
they are part of componentised subsystems, are declared in a DT, and
are not using the component bin/unbind functions.

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

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

This patch series tries to simplify the code of simple devices in case
they are part of componentised subsystems, are declared in a DT, and
are not using the component bin/unbind functions.

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

[PATCH RFC 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 RFC 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 RFC 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

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

[Russell King - ARM Linux]

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

NAK.  If this is done, there's absolutely no point to this code.

-- 
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 RFC 1/2] drivers/base: permit base components to omit the bind/unbind ops

[Russell King - ARM Linux]

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

NAK.  If this is done, there's absolutely no point to this code.

-- 
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".

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

[Russell King - ARM Linux]

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

NAK.  If this is done, there's absolutely no point to this code.

-- 
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".

[PATCH RFC 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 RFC 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

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

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 05:53:27PM +0100, Jean-Francois Moine wrote:
> 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.

A component with no operations makes precisely no sense - with that,
there's no way for the component to be a stand-alone driver.  Your
approach forces your ideas onto every DT device that is referenced
as a phandle.  That's extremely restrictive.

I don't want the component stuff knowing anything about OF.  I don't
want it knowing about driver matching.  I don't want it knowing about
ACPI either.  That's the whole point behind it - it is 100% agnostic
about how that stuff works.

The model is quite simply this:

- a master device is the covering component for the "card"
  - the master device knows what components to expect by some means.
    In the case of DT, that's by phandle references to the components.
  - the master device handles the component independent setup of the
    "card", creating the common resources that are required.  When it's
    ready, it asks for the components to be bound.
  - upon removal of any component, the master component is unbound,
    which triggers the removal of the "card" from the subsystem.
  - as part of the removal, sub-components are unbound.
  - the master device should have as /little/ knowledge about the
    components as possible to permit component re-use.

- a component driver should be independent of it's master.
  - A component which is probed from the device model should simply
    register itself using component_add() with an appropriate operations
    structure, and a removal function which deletes itself.
  - When the driver is ready to be initialised (when the "card" level
    resources have been set in place) the "bind" method will be called.
    At this point, the component does everything that a classical driver
    model driver would do in it's probe callback.
  - unbind is the same as remove in the classical driver model.

So, please, no DT stuff in the component support - it's simply not
required.

-- 
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 RFC 2/2] drivers/base: declare phandle DT nodes as components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 05:53:27PM +0100, Jean-Francois Moine wrote:
> 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.

A component with no operations makes precisely no sense - with that,
there's no way for the component to be a stand-alone driver.  Your
approach forces your ideas onto every DT device that is referenced
as a phandle.  That's extremely restrictive.

I don't want the component stuff knowing anything about OF.  I don't
want it knowing about driver matching.  I don't want it knowing about
ACPI either.  That's the whole point behind it - it is 100% agnostic
about how that stuff works.

The model is quite simply this:

- a master device is the covering component for the "card"
  - the master device knows what components to expect by some means.
    In the case of DT, that's by phandle references to the components.
  - the master device handles the component independent setup of the
    "card", creating the common resources that are required.  When it's
    ready, it asks for the components to be bound.
  - upon removal of any component, the master component is unbound,
    which triggers the removal of the "card" from the subsystem.
  - as part of the removal, sub-components are unbound.
  - the master device should have as /little/ knowledge about the
    components as possible to permit component re-use.

- a component driver should be independent of it's master.
  - A component which is probed from the device model should simply
    register itself using component_add() with an appropriate operations
    structure, and a removal function which deletes itself.
  - When the driver is ready to be initialised (when the "card" level
    resources have been set in place) the "bind" method will be called.
    At this point, the component does everything that a classical driver
    model driver would do in it's probe callback.
  - unbind is the same as remove in the classical driver model.

So, please, no DT stuff in the component support - it's simply not
required.

-- 
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".

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

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 05:53:27PM +0100, Jean-Francois Moine wrote:
> 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.

A component with no operations makes precisely no sense - with that,
there's no way for the component to be a stand-alone driver.  Your
approach forces your ideas onto every DT device that is referenced
as a phandle.  That's extremely restrictive.

I don't want the component stuff knowing anything about OF.  I don't
want it knowing about driver matching.  I don't want it knowing about
ACPI either.  That's the whole point behind it - it is 100% agnostic
about how that stuff works.

The model is quite simply this:

- a master device is the covering component for the "card"
  - the master device knows what components to expect by some means.
    In the case of DT, that's by phandle references to the components.
  - the master device handles the component independent setup of the
    "card", creating the common resources that are required.  When it's
    ready, it asks for the components to be bound.
  - upon removal of any component, the master component is unbound,
    which triggers the removal of the "card" from the subsystem.
  - as part of the removal, sub-components are unbound.
  - the master device should have as /little/ knowledge about the
    components as possible to permit component re-use.

- a component driver should be independent of it's master.
  - A component which is probed from the device model should simply
    register itself using component_add() with an appropriate operations
    structure, and a removal function which deletes itself.
  - When the driver is ready to be initialised (when the "card" level
    resources have been set in place) the "bind" method will be called.
    At this point, the component does everything that a classical driver
    model driver would do in it's probe callback.
  - unbind is the same as remove in the classical driver model.

So, please, no DT stuff in the component support - it's simply not
required.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> This patch series tries to simplify the code of simple devices in case
> they are part of componentised subsystems, are declared in a DT, and
> are not using the component bin/unbind functions.

I wonder - I said earlier today that this works absolutely fine without
modification with DT, so why are you messing about with it adding DT
support?

This is totally the wrong approach.  The idea is that this deals with
/devices/ and /devices/ only.  It groups up /devices/.

It's up to the add_component callback to the master device to decide
how to deal with that.

> Jean-Francois Moine (2):
>   drivers/base: permit base components to omit the bind/unbind ops

And this patch has me wondering if you even understand how to use
this...  The master bind/unbind callbacks are the ones which establish
the "card" based context with the subsystem.

Please, before buggering up this nicely designed implementation, please
/first/ look at the imx-drm rework which was posted back in early January
which illustrates how this is used in a DT context - which is something
I've already pointed you at once today already.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> This patch series tries to simplify the code of simple devices in case
> they are part of componentised subsystems, are declared in a DT, and
> are not using the component bin/unbind functions.

I wonder - I said earlier today that this works absolutely fine without
modification with DT, so why are you messing about with it adding DT
support?

This is totally the wrong approach.  The idea is that this deals with
/devices/ and /devices/ only.  It groups up /devices/.

It's up to the add_component callback to the master device to decide
how to deal with that.

> Jean-Francois Moine (2):
>   drivers/base: permit base components to omit the bind/unbind ops

And this patch has me wondering if you even understand how to use
this...  The master bind/unbind callbacks are the ones which establish
the "card" based context with the subsystem.

Please, before buggering up this nicely designed implementation, please
/first/ look at the imx-drm rework which was posted back in early January
which illustrates how this is used in a DT context - which is something
I've already pointed you at once today already.

-- 
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".

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> This patch series tries to simplify the code of simple devices in case
> they are part of componentised subsystems, are declared in a DT, and
> are not using the component bin/unbind functions.

I wonder - I said earlier today that this works absolutely fine without
modification with DT, so why are you messing about with it adding DT
support?

This is totally the wrong approach.  The idea is that this deals with
/devices/ and /devices/ only.  It groups up /devices/.

It's up to the add_component callback to the master device to decide
how to deal with that.

> Jean-Francois Moine (2):
>   drivers/base: permit base components to omit the bind/unbind ops

And this patch has me wondering if you even understand how to use
this...  The master bind/unbind callbacks are the ones which establish
the "card" based context with the subsystem.

Please, before buggering up this nicely designed implementation, please
/first/ look at the imx-drm rework which was posted back in early January
which illustrates how this is used in a DT context - which is something
I've already pointed you at once today already.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

On Fri, 7 Feb 2014 17:33:26 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> > This patch series tries to simplify the code of simple devices in case
> > they are part of componentised subsystems, are declared in a DT, and
> > are not using the component bin/unbind functions.
> 
> I wonder - I said earlier today that this works absolutely fine without
> modification with DT, so why are you messing about with it adding DT
> support?
> 
> This is totally the wrong approach.  The idea is that this deals with
> /devices/ and /devices/ only.  It groups up /devices/.
> 
> It's up to the add_component callback to the master device to decide
> how to deal with that.
> 
> > Jean-Francois Moine (2):
> >   drivers/base: permit base components to omit the bind/unbind ops
> 
> And this patch has me wondering if you even understand how to use
> this...  The master bind/unbind callbacks are the ones which establish
> the "card" based context with the subsystem.
> 
> Please, before buggering up this nicely designed implementation, please
> /first/ look at the imx-drm rework which was posted back in early January
> which illustrates how this is used in a DT context - which is something
> I've already pointed you at once today already.

As I told in a previous mail, your code works fine in my DT-based
Cubox. I am rewriting the TDA988x as a normal encoder/connector, and,
yes, the bind/unbind functions are useful in this case.

But you opened a door. In a DT context, you know that the probe_defer
mechanism does not work correctly. Your work permits to solve delicate
cases: your component_add tells exactly when a device is available, and
the master bind callback is the green signal for the device waiting for
its resources. Indeed, your system was not created for such a usage,
but it works as it is (anyway, the component bind/unbind functions may
be empty...).

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

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

On Fri, 7 Feb 2014 17:33:26 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> > This patch series tries to simplify the code of simple devices in case
> > they are part of componentised subsystems, are declared in a DT, and
> > are not using the component bin/unbind functions.
> 
> I wonder - I said earlier today that this works absolutely fine without
> modification with DT, so why are you messing about with it adding DT
> support?
> 
> This is totally the wrong approach.  The idea is that this deals with
> /devices/ and /devices/ only.  It groups up /devices/.
> 
> It's up to the add_component callback to the master device to decide
> how to deal with that.
> 
> > Jean-Francois Moine (2):
> >   drivers/base: permit base components to omit the bind/unbind ops
> 
> And this patch has me wondering if you even understand how to use
> this...  The master bind/unbind callbacks are the ones which establish
> the "card" based context with the subsystem.
> 
> Please, before buggering up this nicely designed implementation, please
> /first/ look at the imx-drm rework which was posted back in early January
> which illustrates how this is used in a DT context - which is something
> I've already pointed you at once today already.

As I told in a previous mail, your code works fine in my DT-based
Cubox. I am rewriting the TDA988x as a normal encoder/connector, and,
yes, the bind/unbind functions are useful in this case.

But you opened a door. In a DT context, you know that the probe_defer
mechanism does not work correctly. Your work permits to solve delicate
cases: your component_add tells exactly when a device is available, and
the master bind callback is the green signal for the device waiting for
its resources. Indeed, your system was not created for such a usage,
but it works as it is (anyway, the component bind/unbind functions may
be empty...).

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

Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 07:42:04PM +0100, Jean-Francois Moine wrote:
> On Fri, 7 Feb 2014 17:33:26 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> > > This patch series tries to simplify the code of simple devices in case
> > > they are part of componentised subsystems, are declared in a DT, and
> > > are not using the component bin/unbind functions.
> > 
> > I wonder - I said earlier today that this works absolutely fine without
> > modification with DT, so why are you messing about with it adding DT
> > support?
> > 
> > This is totally the wrong approach.  The idea is that this deals with
> > /devices/ and /devices/ only.  It groups up /devices/.
> > 
> > It's up to the add_component callback to the master device to decide
> > how to deal with that.
> > 
> > > Jean-Francois Moine (2):
> > >   drivers/base: permit base components to omit the bind/unbind ops
> > 
> > And this patch has me wondering if you even understand how to use
> > this...  The master bind/unbind callbacks are the ones which establish
> > the "card" based context with the subsystem.
> > 
> > Please, before buggering up this nicely designed implementation, please
> > /first/ look at the imx-drm rework which was posted back in early January
> > which illustrates how this is used in a DT context - which is something
> > I've already pointed you at once today already.
> 
> As I told in a previous mail, your code works fine in my DT-based
> Cubox. I am rewriting the TDA988x as a normal encoder/connector, and,
> yes, the bind/unbind functions are useful in this case.

So, which bit of "I've already got that" was missed?

> But you opened a door. In a DT context, you know that the probe_defer
> mechanism does not work correctly. Your work permits to solve delicate
> cases: your component_add tells exactly when a device is available, and
> the master bind callback is the green signal for the device waiting for
> its resources. Indeed, your system was not created for such a usage,
> but it works as it is (anyway, the component bind/unbind functions may
> be empty...).

Sorry.  Deferred probe does work, it's been tested with imx-drm, not
only from the master component but also the sub-components.  There's
no problem here.

And no component bind/unbind function should ever be empty.

Again, I put it to you that you don't understand this layer.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 07:42:04PM +0100, Jean-Francois Moine wrote:
> On Fri, 7 Feb 2014 17:33:26 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> > > This patch series tries to simplify the code of simple devices in case
> > > they are part of componentised subsystems, are declared in a DT, and
> > > are not using the component bin/unbind functions.
> > 
> > I wonder - I said earlier today that this works absolutely fine without
> > modification with DT, so why are you messing about with it adding DT
> > support?
> > 
> > This is totally the wrong approach.  The idea is that this deals with
> > /devices/ and /devices/ only.  It groups up /devices/.
> > 
> > It's up to the add_component callback to the master device to decide
> > how to deal with that.
> > 
> > > Jean-Francois Moine (2):
> > >   drivers/base: permit base components to omit the bind/unbind ops
> > 
> > And this patch has me wondering if you even understand how to use
> > this...  The master bind/unbind callbacks are the ones which establish
> > the "card" based context with the subsystem.
> > 
> > Please, before buggering up this nicely designed implementation, please
> > /first/ look at the imx-drm rework which was posted back in early January
> > which illustrates how this is used in a DT context - which is something
> > I've already pointed you at once today already.
> 
> As I told in a previous mail, your code works fine in my DT-based
> Cubox. I am rewriting the TDA988x as a normal encoder/connector, and,
> yes, the bind/unbind functions are useful in this case.

So, which bit of "I've already got that" was missed?

> But you opened a door. In a DT context, you know that the probe_defer
> mechanism does not work correctly. Your work permits to solve delicate
> cases: your component_add tells exactly when a device is available, and
> the master bind callback is the green signal for the device waiting for
> its resources. Indeed, your system was not created for such a usage,
> but it works as it is (anyway, the component bind/unbind functions may
> be empty...).

Sorry.  Deferred probe does work, it's been tested with imx-drm, not
only from the master component but also the sub-components.  There's
no problem here.

And no component bind/unbind function should ever be empty.

Again, I put it to you that you don't understand this layer.

-- 
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".

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 07:42:04PM +0100, Jean-Francois Moine wrote:
> On Fri, 7 Feb 2014 17:33:26 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> > > This patch series tries to simplify the code of simple devices in case
> > > they are part of componentised subsystems, are declared in a DT, and
> > > are not using the component bin/unbind functions.
> > 
> > I wonder - I said earlier today that this works absolutely fine without
> > modification with DT, so why are you messing about with it adding DT
> > support?
> > 
> > This is totally the wrong approach.  The idea is that this deals with
> > /devices/ and /devices/ only.  It groups up /devices/.
> > 
> > It's up to the add_component callback to the master device to decide
> > how to deal with that.
> > 
> > > Jean-Francois Moine (2):
> > >   drivers/base: permit base components to omit the bind/unbind ops
> > 
> > And this patch has me wondering if you even understand how to use
> > this...  The master bind/unbind callbacks are the ones which establish
> > the "card" based context with the subsystem.
> > 
> > Please, before buggering up this nicely designed implementation, please
> > /first/ look at the imx-drm rework which was posted back in early January
> > which illustrates how this is used in a DT context - which is something
> > I've already pointed you at once today already.
> 
> As I told in a previous mail, your code works fine in my DT-based
> Cubox. I am rewriting the TDA988x as a normal encoder/connector, and,
> yes, the bind/unbind functions are useful in this case.

So, which bit of "I've already got that" was missed?

> But you opened a door. In a DT context, you know that the probe_defer
> mechanism does not work correctly. Your work permits to solve delicate
> cases: your component_add tells exactly when a device is available, and
> the master bind callback is the green signal for the device waiting for
> its resources. Indeed, your system was not created for such a usage,
> but it works as it is (anyway, the component bind/unbind functions may
> be empty...).

Sorry.  Deferred probe does work, it's been tested with imx-drm, not
only from the master component but also the sub-components.  There's
no problem here.

And no component bind/unbind function should ever be empty.

Again, I put it to you that you don't understand this layer.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:59:11PM +0000, Russell King - ARM Linux wrote:
> Sorry.  Deferred probe does work, it's been tested with imx-drm, not
> only from the master component but also the sub-components.  There's
> no problem here.

Here's the proof that it also works with the Cubox, and armada DRM:

[drm] Initialized drm 1.1.0 20060810
...
armada-drm armada-510-drm: master bind failed: -517
i2c 0-0070: Driver tda998x requests probe deferral
...
tda998x 0-0070: found TDA19988
armada-drm armada-510-drm: bound 0-0070 (ops tda998x_ops)

So, in the above sequence, the armada DRM driver was bound to its driver
initially, but the TDA998x driver wasn't.

Then, the TDA998x driver is bound, which completes the requirements for
the DRM master.  So the system attempts to bind.

In doing so, the master probe function discovers a missing clock (because
the SIL5531 driver hasn't probed) and it returns -EPROBE_DEFER.  This
causes the probe of the TDA998x to be deferred.

Later, deferred probes are run - at this time the SIL5531 driver has
probed its device, and the clocks are now available.  So when the TDA998x
driver is re-probed, it retriggers the binding attempt, and as the clock
can now be found, the system is bound and the DRM system for the device
is initialised.

I've just committed a patch locally which makes Armada DRM fully use
the component helper, which removes in totality the four armada_output.*
and armada_slave.* files since they're no longer required:

[cubox-3.13 e2713ff5ac2f] DRM: armada: remove non-component support
 7 files changed, 8 insertions(+), 437 deletions(-)
 delete mode 100644 drivers/gpu/drm/armada/armada_output.c
 delete mode 100644 drivers/gpu/drm/armada/armada_output.h
 delete mode 100644 drivers/gpu/drm/armada/armada_slave.c
 delete mode 100644 drivers/gpu/drm/armada/armada_slave.h

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:59:11PM +0000, Russell King - ARM Linux wrote:
> Sorry.  Deferred probe does work, it's been tested with imx-drm, not
> only from the master component but also the sub-components.  There's
> no problem here.

Here's the proof that it also works with the Cubox, and armada DRM:

[drm] Initialized drm 1.1.0 20060810
...
armada-drm armada-510-drm: master bind failed: -517
i2c 0-0070: Driver tda998x requests probe deferral
...
tda998x 0-0070: found TDA19988
armada-drm armada-510-drm: bound 0-0070 (ops tda998x_ops)

So, in the above sequence, the armada DRM driver was bound to its driver
initially, but the TDA998x driver wasn't.

Then, the TDA998x driver is bound, which completes the requirements for
the DRM master.  So the system attempts to bind.

In doing so, the master probe function discovers a missing clock (because
the SIL5531 driver hasn't probed) and it returns -EPROBE_DEFER.  This
causes the probe of the TDA998x to be deferred.

Later, deferred probes are run - at this time the SIL5531 driver has
probed its device, and the clocks are now available.  So when the TDA998x
driver is re-probed, it retriggers the binding attempt, and as the clock
can now be found, the system is bound and the DRM system for the device
is initialised.

I've just committed a patch locally which makes Armada DRM fully use
the component helper, which removes in totality the four armada_output.*
and armada_slave.* files since they're no longer required:

[cubox-3.13 e2713ff5ac2f] DRM: armada: remove non-component support
 7 files changed, 8 insertions(+), 437 deletions(-)
 delete mode 100644 drivers/gpu/drm/armada/armada_output.c
 delete mode 100644 drivers/gpu/drm/armada/armada_output.h
 delete mode 100644 drivers/gpu/drm/armada/armada_slave.c
 delete mode 100644 drivers/gpu/drm/armada/armada_slave.h

-- 
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".

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:59:11PM +0000, Russell King - ARM Linux wrote:
> Sorry.  Deferred probe does work, it's been tested with imx-drm, not
> only from the master component but also the sub-components.  There's
> no problem here.

Here's the proof that it also works with the Cubox, and armada DRM:

[drm] Initialized drm 1.1.0 20060810
...
armada-drm armada-510-drm: master bind failed: -517
i2c 0-0070: Driver tda998x requests probe deferral
...
tda998x 0-0070: found TDA19988
armada-drm armada-510-drm: bound 0-0070 (ops tda998x_ops)

So, in the above sequence, the armada DRM driver was bound to its driver
initially, but the TDA998x driver wasn't.

Then, the TDA998x driver is bound, which completes the requirements for
the DRM master.  So the system attempts to bind.

In doing so, the master probe function discovers a missing clock (because
the SIL5531 driver hasn't probed) and it returns -EPROBE_DEFER.  This
causes the probe of the TDA998x to be deferred.

Later, deferred probes are run - at this time the SIL5531 driver has
probed its device, and the clocks are now available.  So when the TDA998x
driver is re-probed, it retriggers the binding attempt, and as the clock
can now be found, the system is bound and the DRM system for the device
is initialised.

I've just committed a patch locally which makes Armada DRM fully use
the component helper, which removes in totality the four armada_output.*
and armada_slave.* files since they're no longer required:

[cubox-3.13 e2713ff5ac2f] DRM: armada: remove non-component support
 7 files changed, 8 insertions(+), 437 deletions(-)
 delete mode 100644 drivers/gpu/drm/armada/armada_output.c
 delete mode 100644 drivers/gpu/drm/armada/armada_output.h
 delete mode 100644 drivers/gpu/drm/armada/armada_slave.c
 delete mode 100644 drivers/gpu/drm/armada/armada_slave.h

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> This patch series tries to simplify the code of simple devices in case
> they are part of componentised subsystems, are declared in a DT, and
> are not using the component bin/unbind functions.

Here's my changes to the TDA998x driver to add support for the component
helper.  The TDA998x driver retains support for the old way so that
drivers can be transitioned.  For any one DRM "card" the transition to
using the component layer must be all-in or all-out - partial transitions
are not permitted with the simple locking implementation currently in
the component helper due to the possibility of deadlock.  (Master
binds, holding the component lock, master declares i2c device, i2c
device is bound to tda998x, which tries to register with the component
layer, trying to take the held lock.)

http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=unstable/tda998x-devel

It's marked unstable because the two "drivers/base" commits in there are
_not_ the upstream commits.  You'll notice that I just sent one of those
to Gregkh in patch form.

Now, the bits which aren't in that branch but which update the Armada
driver is the below, which needs some clean up and removal of some local
differences I have in my cubox tree, but nicely illustrates why /your/
patches to the component stuff is the wrong approach.

Not only does the patch below add support for using the componentised
TDA998x driver in the above link, but it allows that componentised support
to be specified not only via platform data but also via DT.  The DT
stuff is untested, but it works exactly the same way as imx-drm does
which has been tested.

And yes, I'm thinking that maybe moving compare_of() into the component
support so that drivers can share this generic function may be a good
idea.

So... as I've been saying, no changes to component.c are required here.

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 6f6554bac93f..1f2b7c60bff9 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -6,7 +6,9 @@
  * published by the Free Software Foundation.
  */
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/module.h>
+#include <linux/platform_data/armada_drm.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include "armada_crtc.h"
@@ -53,6 +55,11 @@ static const struct armada_drm_slave_config tda19988_config = {
 };
 #endif
 
+static bool is_componentized(struct device *dev)
+{
+	return dev->of_node || dev->platform_data;
+}
+
 static void armada_drm_unref_work(struct work_struct *work)
 {
 	struct armada_private *priv =
@@ -171,16 +178,22 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 			goto err_kms;
 	}
 
+	if (is_componentized(dev->dev)) {
+		ret = component_bind_all(dev->dev, dev);
+		if (ret)
+			goto err_kms;
+	} else {
 #ifdef CONFIG_DRM_ARMADA_TDA1998X
-	ret = armada_drm_connector_slave_create(dev, &tda19988_config);
-	if (ret)
-		goto err_kms;
+		ret = armada_drm_connector_slave_create(dev, &tda19988_config);
+		if (ret)
+			goto err_kms;
 #endif
 #ifdef CONFIG_DRM_ARMADA_TDA1998X_NXP
-	ret = armada_drm_tda19988_nxp_create(dev);
-	if (ret)
-		goto err_kms;
+		ret = armada_drm_tda19988_nxp_create(dev);
+		if (ret)
+			goto err_kms;
 #endif
+	}
 
 	ret = drm_vblank_init(dev, n);
 	if (ret)
@@ -204,6 +217,10 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	drm_irq_uninstall(dev);
  err_kms:
 	drm_mode_config_cleanup(dev);
+
+	if (is_componentized(dev->dev))
+		component_unbind_all(dev->dev, dev);
+
 	drm_mm_takedown(&priv->linear);
 	flush_work(&priv->fb_unref_work);
 
@@ -218,6 +235,10 @@ static int armada_drm_unload(struct drm_device *dev)
 	armada_fbdev_fini(dev);
 	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
+
+	if (is_componentized(dev->dev))
+		component_unbind_all(dev->dev, dev);
+
 	drm_mm_takedown(&priv->linear);
 	flush_work(&priv->fb_unref_work);
 	dev->dev_private = NULL;
@@ -380,14 +401,82 @@ static struct drm_driver armada_drm_driver = {
 	.fops			= &armada_drm_fops,
 };
 
+static int armada_drm_bind(struct device *dev)
+{
+	return drm_platform_init(&armada_drm_driver, to_platform_device(dev));
+}
+
+static void armada_drm_unbind(struct device *dev)
+{
+	drm_platform_exit(&armada_drm_driver, to_platform_device(dev));
+}
+
+static int compare_of(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static int armada_drm_compare_name(struct device *dev, void *data)
+{
+	const char *name = data;
+	return !strcmp(dev_name(dev), name);
+}
+
+static int armada_drm_add_components(struct device *dev, struct master *master)
+{
+	int i, ret = -ENXIO;
+
+	if (dev->of_node) {
+		struct device_node *np = dev->of_node;
+
+		for (i = 0; ; i++) {
+			struct device_node *node;
+
+			node = of_parse_phandle(np, "connectors", i);
+			if (!node)
+				break;
+
+			ret = component_master_add_child(master, compare_of,
+							 node);
+			of_node_put(node);
+			if (ret)
+				break;
+		}
+	} else if (dev->platform_data) {
+		struct armada_drm_platform_data *data = dev->platform_data;
+
+		for (i = 0; i < data->num_devices; i++) {
+			ret = component_master_add_child(master,
+					armada_drm_compare_name,
+					(void *)data->devices[i]);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+
+static const struct component_master_ops armada_master_ops = {
+	.add_components = armada_drm_add_components,
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
+
 static int armada_drm_probe(struct platform_device *pdev)
 {
-	return drm_platform_init(&armada_drm_driver, pdev);
+	if (is_componentized(&pdev->dev))
+		return component_master_add(&pdev->dev, &armada_master_ops);
+	else
+		return drm_platform_init(&armada_drm_driver, pdev);
 }
 
 static int armada_drm_remove(struct platform_device *pdev)
 {
-	drm_platform_exit(&armada_drm_driver, pdev);
+	if (is_componentized(&pdev->dev))
+		component_master_del(&pdev->dev, &armada_master_ops);
+	else
+		drm_platform_exit(&armada_drm_driver, pdev);
 	return 0;
 }
 


-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> This patch series tries to simplify the code of simple devices in case
> they are part of componentised subsystems, are declared in a DT, and
> are not using the component bin/unbind functions.

Here's my changes to the TDA998x driver to add support for the component
helper.  The TDA998x driver retains support for the old way so that
drivers can be transitioned.  For any one DRM "card" the transition to
using the component layer must be all-in or all-out - partial transitions
are not permitted with the simple locking implementation currently in
the component helper due to the possibility of deadlock.  (Master
binds, holding the component lock, master declares i2c device, i2c
device is bound to tda998x, which tries to register with the component
layer, trying to take the held lock.)

http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=unstable/tda998x-devel

It's marked unstable because the two "drivers/base" commits in there are
_not_ the upstream commits.  You'll notice that I just sent one of those
to Gregkh in patch form.

Now, the bits which aren't in that branch but which update the Armada
driver is the below, which needs some clean up and removal of some local
differences I have in my cubox tree, but nicely illustrates why /your/
patches to the component stuff is the wrong approach.

Not only does the patch below add support for using the componentised
TDA998x driver in the above link, but it allows that componentised support
to be specified not only via platform data but also via DT.  The DT
stuff is untested, but it works exactly the same way as imx-drm does
which has been tested.

And yes, I'm thinking that maybe moving compare_of() into the component
support so that drivers can share this generic function may be a good
idea.

So... as I've been saying, no changes to component.c are required here.

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 6f6554bac93f..1f2b7c60bff9 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -6,7 +6,9 @@
  * published by the Free Software Foundation.
  */
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/module.h>
+#include <linux/platform_data/armada_drm.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include "armada_crtc.h"
@@ -53,6 +55,11 @@ static const struct armada_drm_slave_config tda19988_config = {
 };
 #endif
 
+static bool is_componentized(struct device *dev)
+{
+	return dev->of_node || dev->platform_data;
+}
+
 static void armada_drm_unref_work(struct work_struct *work)
 {
 	struct armada_private *priv =
@@ -171,16 +178,22 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 			goto err_kms;
 	}
 
+	if (is_componentized(dev->dev)) {
+		ret = component_bind_all(dev->dev, dev);
+		if (ret)
+			goto err_kms;
+	} else {
 #ifdef CONFIG_DRM_ARMADA_TDA1998X
-	ret = armada_drm_connector_slave_create(dev, &tda19988_config);
-	if (ret)
-		goto err_kms;
+		ret = armada_drm_connector_slave_create(dev, &tda19988_config);
+		if (ret)
+			goto err_kms;
 #endif
 #ifdef CONFIG_DRM_ARMADA_TDA1998X_NXP
-	ret = armada_drm_tda19988_nxp_create(dev);
-	if (ret)
-		goto err_kms;
+		ret = armada_drm_tda19988_nxp_create(dev);
+		if (ret)
+			goto err_kms;
 #endif
+	}
 
 	ret = drm_vblank_init(dev, n);
 	if (ret)
@@ -204,6 +217,10 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	drm_irq_uninstall(dev);
  err_kms:
 	drm_mode_config_cleanup(dev);
+
+	if (is_componentized(dev->dev))
+		component_unbind_all(dev->dev, dev);
+
 	drm_mm_takedown(&priv->linear);
 	flush_work(&priv->fb_unref_work);
 
@@ -218,6 +235,10 @@ static int armada_drm_unload(struct drm_device *dev)
 	armada_fbdev_fini(dev);
 	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
+
+	if (is_componentized(dev->dev))
+		component_unbind_all(dev->dev, dev);
+
 	drm_mm_takedown(&priv->linear);
 	flush_work(&priv->fb_unref_work);
 	dev->dev_private = NULL;
@@ -380,14 +401,82 @@ static struct drm_driver armada_drm_driver = {
 	.fops			= &armada_drm_fops,
 };
 
+static int armada_drm_bind(struct device *dev)
+{
+	return drm_platform_init(&armada_drm_driver, to_platform_device(dev));
+}
+
+static void armada_drm_unbind(struct device *dev)
+{
+	drm_platform_exit(&armada_drm_driver, to_platform_device(dev));
+}
+
+static int compare_of(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static int armada_drm_compare_name(struct device *dev, void *data)
+{
+	const char *name = data;
+	return !strcmp(dev_name(dev), name);
+}
+
+static int armada_drm_add_components(struct device *dev, struct master *master)
+{
+	int i, ret = -ENXIO;
+
+	if (dev->of_node) {
+		struct device_node *np = dev->of_node;
+
+		for (i = 0; ; i++) {
+			struct device_node *node;
+
+			node = of_parse_phandle(np, "connectors", i);
+			if (!node)
+				break;
+
+			ret = component_master_add_child(master, compare_of,
+							 node);
+			of_node_put(node);
+			if (ret)
+				break;
+		}
+	} else if (dev->platform_data) {
+		struct armada_drm_platform_data *data = dev->platform_data;
+
+		for (i = 0; i < data->num_devices; i++) {
+			ret = component_master_add_child(master,
+					armada_drm_compare_name,
+					(void *)data->devices[i]);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+
+static const struct component_master_ops armada_master_ops = {
+	.add_components = armada_drm_add_components,
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
+
 static int armada_drm_probe(struct platform_device *pdev)
 {
-	return drm_platform_init(&armada_drm_driver, pdev);
+	if (is_componentized(&pdev->dev))
+		return component_master_add(&pdev->dev, &armada_master_ops);
+	else
+		return drm_platform_init(&armada_drm_driver, pdev);
 }
 
 static int armada_drm_remove(struct platform_device *pdev)
 {
-	drm_platform_exit(&armada_drm_driver, pdev);
+	if (is_componentized(&pdev->dev))
+		component_master_del(&pdev->dev, &armada_master_ops);
+	else
+		drm_platform_exit(&armada_drm_driver, pdev);
 	return 0;
 }
 


-- 
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".

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Fri, Feb 07, 2014 at 06:11:08PM +0100, Jean-Francois Moine wrote:
> This patch series tries to simplify the code of simple devices in case
> they are part of componentised subsystems, are declared in a DT, and
> are not using the component bin/unbind functions.

Here's my changes to the TDA998x driver to add support for the component
helper.  The TDA998x driver retains support for the old way so that
drivers can be transitioned.  For any one DRM "card" the transition to
using the component layer must be all-in or all-out - partial transitions
are not permitted with the simple locking implementation currently in
the component helper due to the possibility of deadlock.  (Master
binds, holding the component lock, master declares i2c device, i2c
device is bound to tda998x, which tries to register with the component
layer, trying to take the held lock.)

http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=unstable/tda998x-devel

It's marked unstable because the two "drivers/base" commits in there are
_not_ the upstream commits.  You'll notice that I just sent one of those
to Gregkh in patch form.

Now, the bits which aren't in that branch but which update the Armada
driver is the below, which needs some clean up and removal of some local
differences I have in my cubox tree, but nicely illustrates why /your/
patches to the component stuff is the wrong approach.

Not only does the patch below add support for using the componentised
TDA998x driver in the above link, but it allows that componentised support
to be specified not only via platform data but also via DT.  The DT
stuff is untested, but it works exactly the same way as imx-drm does
which has been tested.

And yes, I'm thinking that maybe moving compare_of() into the component
support so that drivers can share this generic function may be a good
idea.

So... as I've been saying, no changes to component.c are required here.

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 6f6554bac93f..1f2b7c60bff9 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -6,7 +6,9 @@
  * published by the Free Software Foundation.
  */
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/module.h>
+#include <linux/platform_data/armada_drm.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include "armada_crtc.h"
@@ -53,6 +55,11 @@ static const struct armada_drm_slave_config tda19988_config = {
 };
 #endif
 
+static bool is_componentized(struct device *dev)
+{
+	return dev->of_node || dev->platform_data;
+}
+
 static void armada_drm_unref_work(struct work_struct *work)
 {
 	struct armada_private *priv =
@@ -171,16 +178,22 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 			goto err_kms;
 	}
 
+	if (is_componentized(dev->dev)) {
+		ret = component_bind_all(dev->dev, dev);
+		if (ret)
+			goto err_kms;
+	} else {
 #ifdef CONFIG_DRM_ARMADA_TDA1998X
-	ret = armada_drm_connector_slave_create(dev, &tda19988_config);
-	if (ret)
-		goto err_kms;
+		ret = armada_drm_connector_slave_create(dev, &tda19988_config);
+		if (ret)
+			goto err_kms;
 #endif
 #ifdef CONFIG_DRM_ARMADA_TDA1998X_NXP
-	ret = armada_drm_tda19988_nxp_create(dev);
-	if (ret)
-		goto err_kms;
+		ret = armada_drm_tda19988_nxp_create(dev);
+		if (ret)
+			goto err_kms;
 #endif
+	}
 
 	ret = drm_vblank_init(dev, n);
 	if (ret)
@@ -204,6 +217,10 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags)
 	drm_irq_uninstall(dev);
  err_kms:
 	drm_mode_config_cleanup(dev);
+
+	if (is_componentized(dev->dev))
+		component_unbind_all(dev->dev, dev);
+
 	drm_mm_takedown(&priv->linear);
 	flush_work(&priv->fb_unref_work);
 
@@ -218,6 +235,10 @@ static int armada_drm_unload(struct drm_device *dev)
 	armada_fbdev_fini(dev);
 	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
+
+	if (is_componentized(dev->dev))
+		component_unbind_all(dev->dev, dev);
+
 	drm_mm_takedown(&priv->linear);
 	flush_work(&priv->fb_unref_work);
 	dev->dev_private = NULL;
@@ -380,14 +401,82 @@ static struct drm_driver armada_drm_driver = {
 	.fops			= &armada_drm_fops,
 };
 
+static int armada_drm_bind(struct device *dev)
+{
+	return drm_platform_init(&armada_drm_driver, to_platform_device(dev));
+}
+
+static void armada_drm_unbind(struct device *dev)
+{
+	drm_platform_exit(&armada_drm_driver, to_platform_device(dev));
+}
+
+static int compare_of(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static int armada_drm_compare_name(struct device *dev, void *data)
+{
+	const char *name = data;
+	return !strcmp(dev_name(dev), name);
+}
+
+static int armada_drm_add_components(struct device *dev, struct master *master)
+{
+	int i, ret = -ENXIO;
+
+	if (dev->of_node) {
+		struct device_node *np = dev->of_node;
+
+		for (i = 0; ; i++) {
+			struct device_node *node;
+
+			node = of_parse_phandle(np, "connectors", i);
+			if (!node)
+				break;
+
+			ret = component_master_add_child(master, compare_of,
+							 node);
+			of_node_put(node);
+			if (ret)
+				break;
+		}
+	} else if (dev->platform_data) {
+		struct armada_drm_platform_data *data = dev->platform_data;
+
+		for (i = 0; i < data->num_devices; i++) {
+			ret = component_master_add_child(master,
+					armada_drm_compare_name,
+					(void *)data->devices[i]);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+
+static const struct component_master_ops armada_master_ops = {
+	.add_components = armada_drm_add_components,
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
+
 static int armada_drm_probe(struct platform_device *pdev)
 {
-	return drm_platform_init(&armada_drm_driver, pdev);
+	if (is_componentized(&pdev->dev))
+		return component_master_add(&pdev->dev, &armada_master_ops);
+	else
+		return drm_platform_init(&armada_drm_driver, pdev);
 }
 
 static int armada_drm_remove(struct platform_device *pdev)
 {
-	drm_platform_exit(&armada_drm_driver, pdev);
+	if (is_componentized(&pdev->dev))
+		component_master_del(&pdev->dev, &armada_master_ops);
+	else
+		drm_platform_exit(&armada_drm_driver, pdev);
 	return 0;
 }
 


-- 
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".

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

On Fri, 7 Feb 2014 20:23:51 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Here's my changes to the TDA998x driver to add support for the component
> helper.  The TDA998x driver retains support for the old way so that
> drivers can be transitioned.  For any one DRM "card" the transition to

I rewrote the tda998x as a simple encoder+connector (i.e. not a
slave_encoder) with your component helper, and the code is much clearer
and simpler: the DRM driver has nothing to do except to know that the
tda998x is a component and to set the possible_crtcs.

AFAIK, only the tilcdc drm driver is using the tda998x as a
slave_encoder. It does a (encoder+connector) conversion to
(slave_encoder). Then, in your changes in the TDA998x, you do a
(slave_encoder) translation to (encoder+connector).
This seems rather complicated!

I think it would be easier to use your component helper and rewrite
(remove?) tilcdc_slave.c.

> And yes, I'm thinking that maybe moving compare_of() into the component
> support so that drivers can share this generic function may be a good
> idea.

This function exists already in drivers/of/platform.c as
of_dev_node_match(). It just needs to be exported.

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

Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

On Fri, 7 Feb 2014 20:23:51 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Here's my changes to the TDA998x driver to add support for the component
> helper.  The TDA998x driver retains support for the old way so that
> drivers can be transitioned.  For any one DRM "card" the transition to

I rewrote the tda998x as a simple encoder+connector (i.e. not a
slave_encoder) with your component helper, and the code is much clearer
and simpler: the DRM driver has nothing to do except to know that the
tda998x is a component and to set the possible_crtcs.

AFAIK, only the tilcdc drm driver is using the tda998x as a
slave_encoder. It does a (encoder+connector) conversion to
(slave_encoder). Then, in your changes in the TDA998x, you do a
(slave_encoder) translation to (encoder+connector).
This seems rather complicated!

I think it would be easier to use your component helper and rewrite
(remove?) tilcdc_slave.c.

> And yes, I'm thinking that maybe moving compare_of() into the component
> support so that drivers can share this generic function may be a good
> idea.

This function exists already in drivers/of/platform.c as
of_dev_node_match(). It just needs to be exported.

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

Re: [PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Jean-Francois Moine]

On Fri, 7 Feb 2014 20:23:51 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> Here's my changes to the TDA998x driver to add support for the component
> helper.  The TDA998x driver retains support for the old way so that
> drivers can be transitioned.  For any one DRM "card" the transition to

I rewrote the tda998x as a simple encoder+connector (i.e. not a
slave_encoder) with your component helper, and the code is much clearer
and simpler: the DRM driver has nothing to do except to know that the
tda998x is a component and to set the possible_crtcs.

AFAIK, only the tilcdc drm driver is using the tda998x as a
slave_encoder. It does a (encoder+connector) conversion to
(slave_encoder). Then, in your changes in the TDA998x, you do a
(slave_encoder) translation to (encoder+connector).
This seems rather complicated!

I think it would be easier to use your component helper and rewrite
(remove?) tilcdc_slave.c.

> And yes, I'm thinking that maybe moving compare_of() into the component
> support so that drivers can share this generic function may be a good
> idea.

This function exists already in drivers/of/platform.c as
of_dev_node_match(). It just needs to be exported.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Sun, Feb 09, 2014 at 10:22:19AM +0100, Jean-Francois Moine wrote:
> On Fri, 7 Feb 2014 20:23:51 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > Here's my changes to the TDA998x driver to add support for the component
> > helper.  The TDA998x driver retains support for the old way so that
> > drivers can be transitioned.  For any one DRM "card" the transition to
> 
> I rewrote the tda998x as a simple encoder+connector (i.e. not a
> slave_encoder) with your component helper, and the code is much clearer
> and simpler: the DRM driver has nothing to do except to know that the
> tda998x is a component and to set the possible_crtcs.

That's exactly what I've done - the slave encoder veneer can be simply
deleted when tilcdc is converted.

> AFAIK, only the tilcdc drm driver is using the tda998x as a
> slave_encoder. It does a (encoder+connector) conversion to
> (slave_encoder). Then, in your changes in the TDA998x, you do a
> (slave_encoder) translation to (encoder+connector).
> This seems rather complicated!

No.  I first split out the slave encoder functions to be a veneer onto
the tda998x backend, and then add the encoder & connector component
support.  Later, the slave encoder veneer can be deleted.

The reason for this is that virtually all the tda998x backend is what's
required by the encoder & connector support - which is completely logical
when you realise that the generic slave encoder support is just a veneer
itself adapting the encoder & connector support to a slave encoder.

So, we're basically getting rid of two veneers, but we end up with exactly
the same functionality.

> > And yes, I'm thinking that maybe moving compare_of() into the component
> > support so that drivers can share this generic function may be a good
> > idea.
> 
> This function exists already in drivers/of/platform.c as
> of_dev_node_match(). It just needs to be exported.

Good, thanks for pointing that out.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Sun, Feb 09, 2014 at 10:22:19AM +0100, Jean-Francois Moine wrote:
> On Fri, 7 Feb 2014 20:23:51 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > Here's my changes to the TDA998x driver to add support for the component
> > helper.  The TDA998x driver retains support for the old way so that
> > drivers can be transitioned.  For any one DRM "card" the transition to
> 
> I rewrote the tda998x as a simple encoder+connector (i.e. not a
> slave_encoder) with your component helper, and the code is much clearer
> and simpler: the DRM driver has nothing to do except to know that the
> tda998x is a component and to set the possible_crtcs.

That's exactly what I've done - the slave encoder veneer can be simply
deleted when tilcdc is converted.

> AFAIK, only the tilcdc drm driver is using the tda998x as a
> slave_encoder. It does a (encoder+connector) conversion to
> (slave_encoder). Then, in your changes in the TDA998x, you do a
> (slave_encoder) translation to (encoder+connector).
> This seems rather complicated!

No.  I first split out the slave encoder functions to be a veneer onto
the tda998x backend, and then add the encoder & connector component
support.  Later, the slave encoder veneer can be deleted.

The reason for this is that virtually all the tda998x backend is what's
required by the encoder & connector support - which is completely logical
when you realise that the generic slave encoder support is just a veneer
itself adapting the encoder & connector support to a slave encoder.

So, we're basically getting rid of two veneers, but we end up with exactly
the same functionality.

> > And yes, I'm thinking that maybe moving compare_of() into the component
> > support so that drivers can share this generic function may be a good
> > idea.
> 
> This function exists already in drivers/of/platform.c as
> of_dev_node_match(). It just needs to be exported.

Good, thanks for pointing that out.

-- 
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 RFC 0/2] drivers/base: simplify simple DT-based components

[Russell King - ARM Linux]

On Sun, Feb 09, 2014 at 10:22:19AM +0100, Jean-Francois Moine wrote:
> On Fri, 7 Feb 2014 20:23:51 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > Here's my changes to the TDA998x driver to add support for the component
> > helper.  The TDA998x driver retains support for the old way so that
> > drivers can be transitioned.  For any one DRM "card" the transition to
> 
> I rewrote the tda998x as a simple encoder+connector (i.e. not a
> slave_encoder) with your component helper, and the code is much clearer
> and simpler: the DRM driver has nothing to do except to know that the
> tda998x is a component and to set the possible_crtcs.

That's exactly what I've done - the slave encoder veneer can be simply
deleted when tilcdc is converted.

> AFAIK, only the tilcdc drm driver is using the tda998x as a
> slave_encoder. It does a (encoder+connector) conversion to
> (slave_encoder). Then, in your changes in the TDA998x, you do a
> (slave_encoder) translation to (encoder+connector).
> This seems rather complicated!

No.  I first split out the slave encoder functions to be a veneer onto
the tda998x backend, and then add the encoder & connector component
support.  Later, the slave encoder veneer can be deleted.

The reason for this is that virtually all the tda998x backend is what's
required by the encoder & connector support - which is completely logical
when you realise that the generic slave encoder support is just a veneer
itself adapting the encoder & connector support to a slave encoder.

So, we're basically getting rid of two veneers, but we end up with exactly
the same functionality.

> > And yes, I'm thinking that maybe moving compare_of() into the component
> > support so that drivers can share this generic function may be a good
> > idea.
> 
> This function exists already in drivers/of/platform.c as
> of_dev_node_match(). It just needs to be exported.

Good, thanks for pointing that out.

-- 
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".