[PATCH 1/2] component: Add documentation

[PATCH 1/2] component: Add documentation

[Daniel Vetter]

Someone owes me a beer ...

While typing these I think doing an s/component_master/aggregate/
would be useful:
- it's shorter :-)
- I think component/aggregate is much more meaningful naming than
  component/puppetmaster or something like that. At least to my
  English ear "aggregate" emphasizes much more the "assemble a pile of
  things into something bigger" aspect, and there's not really much
  of a control hierarchy between aggregate and constituing components.

But that's way more than a quick doc typing exercise ...

Thanks to Ram for commenting on an initial draft of these docs.

Cc: "C, Ramalingam" <ramalingam.c@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/driver-api/device_link.rst |   3 +
 Documentation/driver-api/index.rst       |   1 +
 drivers/base/component.c                 | 107 ++++++++++++++++++++++-
 include/linux/component.h                |  70 +++++++++++++++
 4 files changed, 178 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
index d6763272e747..2d5919b2b337 100644
--- a/Documentation/driver-api/device_link.rst
+++ b/Documentation/driver-api/device_link.rst
@@ -1,6 +1,9 @@
 .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>`
 .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`
 
+
+.. _device_link:
+
 ============
 Device links
 ============
diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
index ab38ced66a44..c0b600ed9961 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -22,6 +22,7 @@ available subsections can be seen below.
    device_connection
    dma-buf
    device_link
+   component
    message-based
    sound
    frame-buffer
diff --git a/drivers/base/component.c b/drivers/base/component.c
index ddcea8739c12..e5b04bce8544 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -16,6 +16,33 @@
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 
+/**
+ * DOC: overview
+ *
+ * The component frameworks allows drivers to collect a pile of sub-devices,
+ * including their bound drivers, into an aggregate driver. Various subsystem
+ * already provide functions to get hold of such components, e.g.
+ * of_clk_get_by_name(). Anytime there's such a subsystem specific way to find a
+ * a device the component framework should not be used. The component framework
+ * fills the niche of aggregate drivers for specific hardware, where further
+ * standardization into a subsystem doesn't make sense. The common example is
+ * when a logical device (e.g. a DRM display driver) is spread around the SoC on
+ * various component (scanout engines, blending blocks, transcoders for various
+ * outputs and so on).
+ *
+ * The component framework also doesn't solve runtime dependencies, e.g. for
+ * system suspend and resume operations. See also :ref:`device
+ * links<device_link>`.
+ *
+ * Components are registered using component_add() and unregistered with
+ * component_del(), usually from the driver's probe and disconnect functions.
+ *
+ * Aggregate drivers first assemble a component match list of what they need
+ * using component_match_add(). This is then registered as an aggregate driver
+ * using component_master_add_with_match(), and unregistered using
+ * component_master_del().
+ */
+
 struct component;
 
 struct component_match_array {
@@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev,
 	return 0;
 }
 
-/*
- * Add a component to be matched, with a release function.
+/**
+ * component_match_add_release - add a compent match with release callback
+ * @master: device with the aggregate driver
+ * @matchptr: pointer to the list of component matches
+ * @release: release function for @compare_data
+ * @compare: compare function to match against all components
+ * @compare_data: opaque pointer passed to the @compare function
+ *
+ * This adds a new component match to the list stored in @matchptr, which the
+ * @master aggregate driver needs to function. @matchptr must be initialized to
+ * NULL before adding the first match.
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions. At that point @release will be called, to free any references held
+ * by @compare_data, e.g. when @compare_data is a &device_node that must be
+ * released with of_node_put().
  *
- * The match array is first created or extended if necessary.
+ * See also component_match_add().
  */
 void component_match_add_release(struct device *master,
 	struct component_match **matchptr,
@@ -367,6 +408,18 @@ static void free_master(struct master *master)
 	kfree(master);
 }
 
+/**
+ * component_master_add_with_match - register an aggregate driver
+ * @dev: device with the aggregate driver
+ * @ops: callbacks for the aggregate driver
+ * @match: component match list for the aggregate driver
+ *
+ * Registers a new aggregate driver consisting of the components added to @match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available it will be assembled by calling
+ * &component_master_ops.bind from @ops. Must be unregistered by calling
+ * component_master_del().
+ */
 int component_master_add_with_match(struct device *dev,
 	const struct component_master_ops *ops,
 	struct component_match *match)
@@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
+/**
+ * component_master_del - unregister an aggregate driver
+ * @dev: device with the aggregate driver
+ * @ops: callbacks for the aggregate driver
+ *
+ * Unregistered an aggregate driver registered with
+ * component_master_add_with_match(). If necessary the aggregate driver is first
+ * disassembled by calling &component_master_ops.unbind from @ops.
+ */
 void component_master_del(struct device *dev,
 	const struct component_master_ops *ops)
 {
@@ -430,6 +492,15 @@ static void component_unbind(struct component *component,
 	devres_release_group(component->dev, component);
 }
 
+/**
+ * component_unbind_all - unbind all component to an aggregate driver
+ * @master_dev: device with the aggregate driver
+ * @data: opaque pointer, passed to all components
+ *
+ * This unbinds all components to the aggregate @dev by passing @data to their
+ * &component_ops.unbind functions. Should be called from
+ * &component_master_ops.unbind.
+ */
 void component_unbind_all(struct device *master_dev, void *data)
 {
 	struct master *master;
@@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master,
 	return ret;
 }
 
+/**
+ * component_bind_all - bind all component to an aggregate driver
+ * @master_dev: device with the aggregate driver
+ * @data: opaque pointer, passed to all components
+ *
+ * This binds all components to the aggregate @dev by passing @data to their
+ * &component_ops.bind functions. Should be called from
+ * &component_master_ops.bind.
+ */
 int component_bind_all(struct device *master_dev, void *data)
 {
 	struct master *master;
@@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data)
 }
 EXPORT_SYMBOL_GPL(component_bind_all);
 
+/**
+ * component_add - register a component
+ * @dev: component device
+ * @ops: component callbacks
+ *
+ * Register a new component for @dev. Functions in @ops will be call when the
+ * aggregate driver is ready to bind the overall driver by calling
+ * component_bind_all(). See also &struct component_ops.
+ *
+ * The component needs to be unregistered again at driver unload/disconnect by
+ * calling component_del().
+ */
 int component_add(struct device *dev, const struct component_ops *ops)
 {
 	struct component *component;
@@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops)
 }
 EXPORT_SYMBOL_GPL(component_add);
 
+/**
+ * component_del - unregister a component
+ * @dev: component device
+ * @ops: component callbacks
+ *
+ * Unregister a component added with component_add(). If the component is bound
+ * into an aggregate driver this will force the entire aggrate driver, including
+ * all its components, to be unbound.
+ */
 void component_del(struct device *dev, const struct component_ops *ops)
 {
 	struct component *c, *component = NULL;
diff --git a/include/linux/component.h b/include/linux/component.h
index e71fbbbc74e2..67a899dd2e10 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -4,11 +4,31 @@
 
 #include <linux/stddef.h>
 
+
 struct device;
 
+/**
+ * struct component_ops - callbacks for component drivers
+ *
+ * Components are registered with component_add() and unregistered with
+ * component_del().
+ */
 struct component_ops {
+	/**
+	 * @bind:
+	 *
+	 * Called through component_bind_all() when the aggregate driver is
+	 * ready to bind the overall driver.
+	 */
 	int (*bind)(struct device *comp, struct device *master,
 		    void *master_data);
+	/**
+	 * @unbind:
+	 *
+	 * Called through component_unbind_all() when the aggregate driver is
+	 * ready to bind the overall driver, or when component_bind_all() fails
+	 * part-ways through and needs to unbind some already bound components.
+	 */
 	void (*unbind)(struct device *comp, struct device *master,
 		       void *master_data);
 };
@@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);
 
 struct master;
 
+/**
+ * struct component_master_ops - callback for the aggregate driver
+ *
+ * Aggregate drivers are registered with component_master_add_with_match() and
+ * unregistered with component_master_del().
+ */
 struct component_master_ops {
+	/**
+	 * @bind:
+	 *
+	 * Called when all components or the aggregate driver, as specified in
+	 * the match list passed to component_master_add_with_match(), are
+	 * ready. Usually there are 3 steps to bind an aggregate driver:
+	 *
+	 * 1. Allocate a structure for the aggregate driver.
+	 *
+	 * 2. Bind all components to the aggregate driver by calling
+	 *    component_bind_all() with the aggregate driver structure as opaque
+	 *    pointer data.
+	 *
+	 * 3. Register the aggregate driver with the subsystem to publish its
+	 *    interfaces.
+	 *
+	 * Note that the lifetime of the aggregate driver does not align with
+	 * any of the underlying &struct device instances. Therefore devm cannot
+	 * be used and all resources acquired or allocated in this callback must
+	 * be expecitly released in the @unbind callback.
+	 */
 	int (*bind)(struct device *master);
+	/**
+	 * @unbind:
+	 *
+	 * Called when either the aggregate driver, using
+	 * component_master_del(), or one of its components, using
+	 * component_del(), is unregistered.
+	 */
 	void (*unbind)(struct device *master);
 };
 
@@ -38,6 +92,22 @@ void component_match_add_release(struct device *master,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *), void *compare_data);
 
+/**
+ * component_match_add - add a compent match
+ * @master: device with the aggregate driver
+ * @matchptr: pointer to the list of component matches
+ * @compare: compare function to match against all components
+ * @compare_data: opaque pointer passed to the @compare function
+ *
+ * This adds a new component match to the list stored in @matchptr, which the
+ * @master aggregate driver needs to function. @matchptr must be initialized to
+ * NULL before adding the first match.
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions.
+ *
+ * See also component_match_add_release().
+ */
 static inline void component_match_add(struct device *master,
 	struct component_match **matchptr,
 	int (*compare)(struct device *, void *), void *compare_data)
-- 
2.20.1

[PATCH 2/2] components: multiple components for a device

[Daniel Vetter]

Component framework is extended to support multiple components for
a struct device. These will be matched with different masters based on
its sub component value.

We are introducing this, as I915 needs two different components
with different subcomponent value, which will be matched to two
different component masters(Audio and HDCP) based on the subcomponent
values.

v2: Add documenation.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v1 code)
Signed-off-by: Ramalingam C <ramalingam.c@intel.com> (v1 commit message)
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/base/component.c  | 159 +++++++++++++++++++++++++++++---------
 include/linux/component.h |  10 ++-
 2 files changed, 130 insertions(+), 39 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index e5b04bce8544..eb7915fc5278 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -48,6 +48,7 @@ struct component;
 struct component_match_array {
 	void *data;
 	int (*compare)(struct device *, void *);
+	int (*compare_typed)(struct device *, int, void *);
 	void (*release)(struct device *, void *);
 	struct component *component;
 	bool duplicate;
@@ -75,6 +76,7 @@ struct component {
 	bool bound;
 
 	const struct component_ops *ops;
+	int subcomponent;
 	struct device *dev;
 };
 
@@ -159,7 +161,7 @@ static struct master *__master_find(struct device *dev,
 }
 
 static struct component *find_component(struct master *master,
-	int (*compare)(struct device *, void *), void *compare_data)
+	struct component_match_array *mc)
 {
 	struct component *c;
 
@@ -167,8 +169,13 @@ static struct component *find_component(struct master *master,
 		if (c->master && c->master != master)
 			continue;
 
-		if (compare(c->dev, compare_data))
+		if (mc->compare_typed) {
+			if (mc->compare_typed(c->dev, c->subcomponent,
+					      mc->data))
+				return c;
+		} else if (mc->compare(c->dev, mc->data)) {
 			return c;
+		}
 	}
 
 	return NULL;
@@ -193,7 +200,7 @@ static int find_components(struct master *master)
 		if (match->compare[i].component)
 			continue;
 
-		c = find_component(master, mc->compare, mc->data);
+		c = find_component(master, mc);
 		if (!c) {
 			ret = -ENXIO;
 			break;
@@ -328,29 +335,12 @@ static int component_match_realloc(struct device *dev,
 	return 0;
 }
 
-/**
- * component_match_add_release - add a compent match with release callback
- * @master: device with the aggregate driver
- * @matchptr: pointer to the list of component matches
- * @release: release function for @compare_data
- * @compare: compare function to match against all components
- * @compare_data: opaque pointer passed to the @compare function
- *
- * This adds a new component match to the list stored in @matchptr, which the
- * @master aggregate driver needs to function. @matchptr must be initialized to
- * NULL before adding the first match.
- *
- * The allocated match list in @matchptr is automatically released using devm
- * actions. At that point @release will be called, to free any references held
- * by @compare_data, e.g. when @compare_data is a &device_node that must be
- * released with of_node_put().
- *
- * See also component_match_add().
- */
-void component_match_add_release(struct device *master,
+static void __component_match_add(struct device *master,
 	struct component_match **matchptr,
 	void (*release)(struct device *, void *),
-	int (*compare)(struct device *, void *), void *compare_data)
+	int (*compare)(struct device *, void *),
+	int (*compare_typed)(struct device *, int, void *),
+	void *compare_data)
 {
 	struct component_match *match = *matchptr;
 
@@ -382,13 +372,69 @@ void component_match_add_release(struct device *master,
 	}
 
 	match->compare[match->num].compare = compare;
+	match->compare[match->num].compare_typed = compare_typed;
 	match->compare[match->num].release = release;
 	match->compare[match->num].data = compare_data;
 	match->compare[match->num].component = NULL;
 	match->num++;
 }
+
+/**
+ * component_match_add_release - add a compent match with release callback
+ * @master: device with the aggregate driver
+ * @matchptr: pointer to the list of component matches
+ * @release: release function for @compare_data
+ * @compare: compare function to match against all components
+ * @compare_data: opaque pointer passed to the @compare function
+ *
+ * This adds a new component match to the list stored in @matchptr, which the
+ * @master aggregate driver needs to function. @matchptr must be initialized to
+ * NULL before adding the first match.
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions. At that point @release will be called, to free any references held
+ * by @compare_data, e.g. when @compare_data is a &device_node that must be
+ * released with of_node_put(). This only matches against components
+ * added with component_add().
+ *
+ * See also component_match_add() and component_match_add_typed().
+ */
+void component_match_add_release(struct device *master,
+	struct component_match **matchptr,
+	void (*release)(struct device *, void *),
+	int (*compare)(struct device *, void *), void *compare_data)
+{
+	__component_match_add(master, matchptr, release, compare, NULL,
+			      compare_data);
+}
 EXPORT_SYMBOL(component_match_add_release);
 
+/**
+ * component_match_add_typed - add a compent match for a typed component
+ * @master: device with the aggregate driver
+ * @matchptr: pointer to the list of component matches
+ * @compare_typed: compare function to match against all typed components
+ * @compare_data: opaque pointer passed to the @compare function
+ *
+ * This adds a new component match to the list stored in @matchptr, which the
+ * @master aggregate driver needs to function. @matchptr must be initialized to
+ * NULL before adding the first match. This only matches against components
+ * added with component_add_typed().
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions.
+ *
+ * See also component_match_add_release() and component_match_add_typed().
+ */
+void component_match_add_typed(struct device *master,
+	struct component_match **matchptr,
+	int (*compare_typed)(struct device *, int, void *), void *compare_data)
+{
+	__component_match_add(master, matchptr, NULL, NULL, compare_typed,
+			      compare_data);
+}
+EXPORT_SYMBOL(component_match_add_typed);
+
 static void free_master(struct master *master)
 {
 	struct component_match *match = master->match;
@@ -617,19 +663,8 @@ int component_bind_all(struct device *master_dev, void *data)
 }
 EXPORT_SYMBOL_GPL(component_bind_all);
 
-/**
- * component_add - register a component
- * @dev: component device
- * @ops: component callbacks
- *
- * Register a new component for @dev. Functions in @ops will be call when the
- * aggregate driver is ready to bind the overall driver by calling
- * component_bind_all(). See also &struct component_ops.
- *
- * The component needs to be unregistered again at driver unload/disconnect by
- * calling component_del().
- */
-int component_add(struct device *dev, const struct component_ops *ops)
+static int __component_add(struct device *dev, const struct component_ops *ops,
+	int subcomponent)
 {
 	struct component *component;
 	int ret;
@@ -640,6 +675,7 @@ int component_add(struct device *dev, const struct component_ops *ops)
 
 	component->ops = ops;
 	component->dev = dev;
+	component->subcomponent = subcomponent;
 
 	dev_dbg(dev, "adding component (ops %ps)\n", ops);
 
@@ -658,6 +694,55 @@ int component_add(struct device *dev, const struct component_ops *ops)
 
 	return ret < 0 ? ret : 0;
 }
+
+/**
+ * component_add_typed - register a component
+ * @dev: component device
+ * @ops: component callbacks
+ * @subcomponent: nonzero identifier for subcomponents
+ *
+ * Register a new component for @dev. Functions in @ops will be call when the
+ * aggregate driver is ready to bind the overall driver by calling
+ * component_bind_all(). See also &struct component_ops.
+ *
+ * @subcomponent must be nonzero and is used to differentiate between multiple
+ * components registerd on the same device @dev. These components are match
+ * using component_match_add_typed().
+ *
+ * The component needs to be unregistered again at driver unload/disconnect by
+ * calling component_del().
+ *
+ * See also component_add().
+ */
+int component_add_typed(struct device *dev, const struct component_ops *ops,
+	int subcomponent)
+{
+	if (WARN_ON(subcomponent == 0))
+		return -EINVAL;
+
+	return __component_add(dev, ops, subcomponent);
+}
+EXPORT_SYMBOL_GPL(component_add_typed);
+
+/**
+ * component_add - register a component
+ * @dev: component device
+ * @ops: component callbacks
+ *
+ * Register a new component for @dev. Functions in @ops will be call when the
+ * aggregate driver is ready to bind the overall driver by calling
+ * component_bind_all(). See also &struct component_ops.
+ *
+ * The component needs to be unregistered again at driver unload/disconnect by
+ * calling component_del().
+ *
+ * See also component_add_typed() for a variant that allows multipled different
+ * components on the same device.
+ */
+int component_add(struct device *dev, const struct component_ops *ops)
+{
+	return __component_add(dev, ops, 0);
+}
 EXPORT_SYMBOL_GPL(component_add);
 
 /**
diff --git a/include/linux/component.h b/include/linux/component.h
index 67a899dd2e10..9e69e2117f0b 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -34,6 +34,8 @@ struct component_ops {
 };
 
 int component_add(struct device *, const struct component_ops *);
+int component_add_typed(struct device *dev, const struct component_ops *ops,
+	int subcomponent);
 void component_del(struct device *, const struct component_ops *);
 
 int component_bind_all(struct device *master, void *master_data);
@@ -91,6 +93,9 @@ void component_match_add_release(struct device *master,
 	struct component_match **matchptr,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *), void *compare_data);
+void component_match_add_typed(struct device *master,
+	struct component_match **matchptr,
+	int (*compare_typed)(struct device *, int, void *), void *compare_data);
 
 /**
  * component_match_add - add a compent match
@@ -101,12 +106,13 @@ void component_match_add_release(struct device *master,
  *
  * This adds a new component match to the list stored in @matchptr, which the
  * @master aggregate driver needs to function. @matchptr must be initialized to
- * NULL before adding the first match.
+ * NULL before adding the first match. This only matches against components
+ * added with component_add().
  *
  * The allocated match list in @matchptr is automatically released using devm
  * actions.
  *
- * See also component_match_add_release().
+ * See also component_match_add_release() and component_match_add_typed().
  */
 static inline void component_match_add(struct device *master,
 	struct component_match **matchptr,
-- 
2.20.1

Re: [PATCH 2/2] components: multiple components for a device

[Daniel Vetter]

On Thu, Jan 31, 2019 at 03:46:40PM +0100, Daniel Vetter wrote:
> Component framework is extended to support multiple components for
> a struct device. These will be matched with different masters based on
> its sub component value.
> 
> We are introducing this, as I915 needs two different components
> with different subcomponent value, which will be matched to two
> different component masters(Audio and HDCP) based on the subcomponent
> values.
> 
> v2: Add documenation.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (v1 code)
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com> (v1 commit message)
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Hi Greg&Russell,

Now that there's nice documentation on everything, any comments on these
two patches?

Thanks, Daniel

> ---
>  drivers/base/component.c  | 159 +++++++++++++++++++++++++++++---------
>  include/linux/component.h |  10 ++-
>  2 files changed, 130 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index e5b04bce8544..eb7915fc5278 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -48,6 +48,7 @@ struct component;
>  struct component_match_array {
>  	void *data;
>  	int (*compare)(struct device *, void *);
> +	int (*compare_typed)(struct device *, int, void *);
>  	void (*release)(struct device *, void *);
>  	struct component *component;
>  	bool duplicate;
> @@ -75,6 +76,7 @@ struct component {
>  	bool bound;
>  
>  	const struct component_ops *ops;
> +	int subcomponent;
>  	struct device *dev;
>  };
>  
> @@ -159,7 +161,7 @@ static struct master *__master_find(struct device *dev,
>  }
>  
>  static struct component *find_component(struct master *master,
> -	int (*compare)(struct device *, void *), void *compare_data)
> +	struct component_match_array *mc)
>  {
>  	struct component *c;
>  
> @@ -167,8 +169,13 @@ static struct component *find_component(struct master *master,
>  		if (c->master && c->master != master)
>  			continue;
>  
> -		if (compare(c->dev, compare_data))
> +		if (mc->compare_typed) {
> +			if (mc->compare_typed(c->dev, c->subcomponent,
> +					      mc->data))
> +				return c;
> +		} else if (mc->compare(c->dev, mc->data)) {
>  			return c;
> +		}
>  	}
>  
>  	return NULL;
> @@ -193,7 +200,7 @@ static int find_components(struct master *master)
>  		if (match->compare[i].component)
>  			continue;
>  
> -		c = find_component(master, mc->compare, mc->data);
> +		c = find_component(master, mc);
>  		if (!c) {
>  			ret = -ENXIO;
>  			break;
> @@ -328,29 +335,12 @@ static int component_match_realloc(struct device *dev,
>  	return 0;
>  }
>  
> -/**
> - * component_match_add_release - add a compent match with release callback
> - * @master: device with the aggregate driver
> - * @matchptr: pointer to the list of component matches
> - * @release: release function for @compare_data
> - * @compare: compare function to match against all components
> - * @compare_data: opaque pointer passed to the @compare function
> - *
> - * This adds a new component match to the list stored in @matchptr, which the
> - * @master aggregate driver needs to function. @matchptr must be initialized to
> - * NULL before adding the first match.
> - *
> - * The allocated match list in @matchptr is automatically released using devm
> - * actions. At that point @release will be called, to free any references held
> - * by @compare_data, e.g. when @compare_data is a &device_node that must be
> - * released with of_node_put().
> - *
> - * See also component_match_add().
> - */
> -void component_match_add_release(struct device *master,
> +static void __component_match_add(struct device *master,
>  	struct component_match **matchptr,
>  	void (*release)(struct device *, void *),
> -	int (*compare)(struct device *, void *), void *compare_data)
> +	int (*compare)(struct device *, void *),
> +	int (*compare_typed)(struct device *, int, void *),
> +	void *compare_data)
>  {
>  	struct component_match *match = *matchptr;
>  
> @@ -382,13 +372,69 @@ void component_match_add_release(struct device *master,
>  	}
>  
>  	match->compare[match->num].compare = compare;
> +	match->compare[match->num].compare_typed = compare_typed;
>  	match->compare[match->num].release = release;
>  	match->compare[match->num].data = compare_data;
>  	match->compare[match->num].component = NULL;
>  	match->num++;
>  }
> +
> +/**
> + * component_match_add_release - add a compent match with release callback
> + * @master: device with the aggregate driver
> + * @matchptr: pointer to the list of component matches
> + * @release: release function for @compare_data
> + * @compare: compare function to match against all components
> + * @compare_data: opaque pointer passed to the @compare function
> + *
> + * This adds a new component match to the list stored in @matchptr, which the
> + * @master aggregate driver needs to function. @matchptr must be initialized to
> + * NULL before adding the first match.
> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions. At that point @release will be called, to free any references held
> + * by @compare_data, e.g. when @compare_data is a &device_node that must be
> + * released with of_node_put(). This only matches against components
> + * added with component_add().
> + *
> + * See also component_match_add() and component_match_add_typed().
> + */
> +void component_match_add_release(struct device *master,
> +	struct component_match **matchptr,
> +	void (*release)(struct device *, void *),
> +	int (*compare)(struct device *, void *), void *compare_data)
> +{
> +	__component_match_add(master, matchptr, release, compare, NULL,
> +			      compare_data);
> +}
>  EXPORT_SYMBOL(component_match_add_release);
>  
> +/**
> + * component_match_add_typed - add a compent match for a typed component
> + * @master: device with the aggregate driver
> + * @matchptr: pointer to the list of component matches
> + * @compare_typed: compare function to match against all typed components
> + * @compare_data: opaque pointer passed to the @compare function
> + *
> + * This adds a new component match to the list stored in @matchptr, which the
> + * @master aggregate driver needs to function. @matchptr must be initialized to
> + * NULL before adding the first match. This only matches against components
> + * added with component_add_typed().
> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions.
> + *
> + * See also component_match_add_release() and component_match_add_typed().
> + */
> +void component_match_add_typed(struct device *master,
> +	struct component_match **matchptr,
> +	int (*compare_typed)(struct device *, int, void *), void *compare_data)
> +{
> +	__component_match_add(master, matchptr, NULL, NULL, compare_typed,
> +			      compare_data);
> +}
> +EXPORT_SYMBOL(component_match_add_typed);
> +
>  static void free_master(struct master *master)
>  {
>  	struct component_match *match = master->match;
> @@ -617,19 +663,8 @@ int component_bind_all(struct device *master_dev, void *data)
>  }
>  EXPORT_SYMBOL_GPL(component_bind_all);
>  
> -/**
> - * component_add - register a component
> - * @dev: component device
> - * @ops: component callbacks
> - *
> - * Register a new component for @dev. Functions in @ops will be call when the
> - * aggregate driver is ready to bind the overall driver by calling
> - * component_bind_all(). See also &struct component_ops.
> - *
> - * The component needs to be unregistered again at driver unload/disconnect by
> - * calling component_del().
> - */
> -int component_add(struct device *dev, const struct component_ops *ops)
> +static int __component_add(struct device *dev, const struct component_ops *ops,
> +	int subcomponent)
>  {
>  	struct component *component;
>  	int ret;
> @@ -640,6 +675,7 @@ int component_add(struct device *dev, const struct component_ops *ops)
>  
>  	component->ops = ops;
>  	component->dev = dev;
> +	component->subcomponent = subcomponent;
>  
>  	dev_dbg(dev, "adding component (ops %ps)\n", ops);
>  
> @@ -658,6 +694,55 @@ int component_add(struct device *dev, const struct component_ops *ops)
>  
>  	return ret < 0 ? ret : 0;
>  }
> +
> +/**
> + * component_add_typed - register a component
> + * @dev: component device
> + * @ops: component callbacks
> + * @subcomponent: nonzero identifier for subcomponents
> + *
> + * Register a new component for @dev. Functions in @ops will be call when the
> + * aggregate driver is ready to bind the overall driver by calling
> + * component_bind_all(). See also &struct component_ops.
> + *
> + * @subcomponent must be nonzero and is used to differentiate between multiple
> + * components registerd on the same device @dev. These components are match
> + * using component_match_add_typed().
> + *
> + * The component needs to be unregistered again at driver unload/disconnect by
> + * calling component_del().
> + *
> + * See also component_add().
> + */
> +int component_add_typed(struct device *dev, const struct component_ops *ops,
> +	int subcomponent)
> +{
> +	if (WARN_ON(subcomponent == 0))
> +		return -EINVAL;
> +
> +	return __component_add(dev, ops, subcomponent);
> +}
> +EXPORT_SYMBOL_GPL(component_add_typed);
> +
> +/**
> + * component_add - register a component
> + * @dev: component device
> + * @ops: component callbacks
> + *
> + * Register a new component for @dev. Functions in @ops will be call when the
> + * aggregate driver is ready to bind the overall driver by calling
> + * component_bind_all(). See also &struct component_ops.
> + *
> + * The component needs to be unregistered again at driver unload/disconnect by
> + * calling component_del().
> + *
> + * See also component_add_typed() for a variant that allows multipled different
> + * components on the same device.
> + */
> +int component_add(struct device *dev, const struct component_ops *ops)
> +{
> +	return __component_add(dev, ops, 0);
> +}
>  EXPORT_SYMBOL_GPL(component_add);
>  
>  /**
> diff --git a/include/linux/component.h b/include/linux/component.h
> index 67a899dd2e10..9e69e2117f0b 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -34,6 +34,8 @@ struct component_ops {
>  };
>  
>  int component_add(struct device *, const struct component_ops *);
> +int component_add_typed(struct device *dev, const struct component_ops *ops,
> +	int subcomponent);
>  void component_del(struct device *, const struct component_ops *);
>  
>  int component_bind_all(struct device *master, void *master_data);
> @@ -91,6 +93,9 @@ void component_match_add_release(struct device *master,
>  	struct component_match **matchptr,
>  	void (*release)(struct device *, void *),
>  	int (*compare)(struct device *, void *), void *compare_data);
> +void component_match_add_typed(struct device *master,
> +	struct component_match **matchptr,
> +	int (*compare_typed)(struct device *, int, void *), void *compare_data);
>  
>  /**
>   * component_match_add - add a compent match
> @@ -101,12 +106,13 @@ void component_match_add_release(struct device *master,
>   *
>   * This adds a new component match to the list stored in @matchptr, which the
>   * @master aggregate driver needs to function. @matchptr must be initialized to
> - * NULL before adding the first match.
> + * NULL before adding the first match. This only matches against components
> + * added with component_add().
>   *
>   * The allocated match list in @matchptr is automatically released using devm
>   * actions.
>   *
> - * See also component_match_add_release().
> + * See also component_match_add_release() and component_match_add_typed().
>   */
>  static inline void component_match_add(struct device *master,
>  	struct component_match **matchptr,
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] component: Add documentation

[Rafael J. Wysocki]

 w/compOn Thu, Jan 31, 2019 at 3:46 PM Daniel Vetter
<daniel.vetter@ffwll.ch> wrote:
>
> Someone owes me a beer ...
>
> While typing these I think doing an s/component_master/aggregate/
> would be useful:
> - it's shorter :-)
> - I think component/aggregate is much more meaningful naming than
>   component/puppetmaster or something like that. At least to my
>   English ear "aggregate" emphasizes much more the "assemble a pile of
>   things into something bigger" aspect, and there's not really much
>   of a control hierarchy between aggregate and constituing components.
>
> But that's way more than a quick doc typing exercise ...
>
> Thanks to Ram for commenting on an initial draft of these docs.

Look goods to me overall (even though I'm not super-familiar with the
component framework), but see below.

> Cc: "C, Ramalingam" <ramalingam.c@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/driver-api/device_link.rst |   3 +
>  Documentation/driver-api/index.rst       |   1 +
>  drivers/base/component.c                 | 107 ++++++++++++++++++++++-
>  include/linux/component.h                |  70 +++++++++++++++
>  4 files changed, 178 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
> index d6763272e747..2d5919b2b337 100644
> --- a/Documentation/driver-api/device_link.rst
> +++ b/Documentation/driver-api/device_link.rst
> @@ -1,6 +1,9 @@
>  .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>`
>  .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`
>
> +
> +.. _device_link:
> +
>  ============
>  Device links
>  ============
> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> index ab38ced66a44..c0b600ed9961 100644
> --- a/Documentation/driver-api/index.rst
> +++ b/Documentation/driver-api/index.rst
> @@ -22,6 +22,7 @@ available subsections can be seen below.
>     device_connection
>     dma-buf
>     device_link
> +   component

Do I think correctly that this doc is going to be generated
automatically from the kerneldoc comments in component.c?

>     message-based
>     sound
>     frame-buffer
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index ddcea8739c12..e5b04bce8544 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -16,6 +16,33 @@
>  #include <linux/slab.h>
>  #include <linux/debugfs.h>
>
> +/**
> + * DOC: overview
> + *
> + * The component frameworks allows drivers to collect a pile of sub-devices,

s/frameworks/framework/

> + * including their bound drivers, into an aggregate driver. Various subsystem

s/subsystem/subsystems/

> + * already provide functions to get hold of such components, e.g.
> + * of_clk_get_by_name(). Anytime there's such a subsystem specific way to find a
> + * a device the component framework should not be used.

I would use a positive statement here, like "The component framework
can be used when such a subsystem-specific way to find a device is not
available".

> The component framework
> + * fills the niche of aggregate drivers for specific hardware, where further
> + * standardization into a subsystem doesn't make sense.

I would say "would not be practical" instead of "doesn't make sense".

> The common example is
> + * when a logical device (e.g. a DRM display driver) is spread around the SoC on
> + * various component (scanout engines, blending blocks, transcoders for various
> + * outputs and so on).
> + *
> + * The component framework also doesn't solve runtime dependencies, e.g. for
> + * system suspend and resume operations. See also :ref:`device
> + * links<device_link>`.
> + *
> + * Components are registered using component_add() and unregistered with
> + * component_del(), usually from the driver's probe and disconnect functions.
> + *
> + * Aggregate drivers first assemble a component match list of what they need
> + * using component_match_add(). This is then registered as an aggregate driver
> + * using component_master_add_with_match(), and unregistered using
> + * component_master_del().
> + */
> +
>  struct component;
>
>  struct component_match_array {
> @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev,
>         return 0;
>  }
>
> -/*
> - * Add a component to be matched, with a release function.
> +/**
> + * component_match_add_release - add a compent match with release callback

s/compent/component/ ?

> + * @master: device with the aggregate driver
> + * @matchptr: pointer to the list of component matches
> + * @release: release function for @compare_data
> + * @compare: compare function to match against all components
> + * @compare_data: opaque pointer passed to the @compare function
> + *
> + * This adds a new component match to the list stored in @matchptr, which the
> + * @master aggregate driver needs to function. @matchptr must be initialized to
> + * NULL before adding the first match.
> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions. At that point @release will be called, to free any references held
> + * by @compare_data, e.g. when @compare_data is a &device_node that must be
> + * released with of_node_put().
>   *
> - * The match array is first created or extended if necessary.
> + * See also component_match_add().
>   */
>  void component_match_add_release(struct device *master,
>         struct component_match **matchptr,
> @@ -367,6 +408,18 @@ static void free_master(struct master *master)
>         kfree(master);
>  }
>
> +/**
> + * component_master_add_with_match - register an aggregate driver
> + * @dev: device with the aggregate driver
> + * @ops: callbacks for the aggregate driver
> + * @match: component match list for the aggregate driver
> + *
> + * Registers a new aggregate driver consisting of the components added to @match
> + * by calling one of the component_match_add() functions. Once all components in
> + * @match are available it will be assembled by calling

A comma seems to be missing after "available".

> + * &component_master_ops.bind from @ops. Must be unregistered by calling
> + * component_master_del().
> + */
>  int component_master_add_with_match(struct device *dev,
>         const struct component_master_ops *ops,
>         struct component_match *match)
> @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(component_master_add_with_match);
>
> +/**
> + * component_master_del - unregister an aggregate driver
> + * @dev: device with the aggregate driver
> + * @ops: callbacks for the aggregate driver
> + *
> + * Unregistered an aggregate driver registered with

s/Unregistered/Unregisters/ ?

> + * component_master_add_with_match(). If necessary the aggregate driver is first
> + * disassembled by calling &component_master_ops.unbind from @ops.

Q: How does the &component_master_ops.unbind annotation work?  Does it
produce any special output?

> + */
>  void component_master_del(struct device *dev,
>         const struct component_master_ops *ops)
>  {
> @@ -430,6 +492,15 @@ static void component_unbind(struct component *component,
>         devres_release_group(component->dev, component);
>  }
>
> +/**
> + * component_unbind_all - unbind all component to an aggregate driver
> + * @master_dev: device with the aggregate driver
> + * @data: opaque pointer, passed to all components
> + *
> + * This unbinds all components to the aggregate @dev by passing @data to their

I guess "This" is redundant.

> + * &component_ops.unbind functions. Should be called from
> + * &component_master_ops.unbind.
> + */
>  void component_unbind_all(struct device *master_dev, void *data)
>  {
>         struct master *master;
> @@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master,
>         return ret;
>  }
>
> +/**
> + * component_bind_all - bind all component to an aggregate driver
> + * @master_dev: device with the aggregate driver
> + * @data: opaque pointer, passed to all components
> + *
> + * This binds all components to the aggregate @dev by passing @data to their

Likewise.

> + * &component_ops.bind functions. Should be called from
> + * &component_master_ops.bind.
> + */
>  int component_bind_all(struct device *master_dev, void *data)
>  {
>         struct master *master;
> @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data)
>  }
>  EXPORT_SYMBOL_GPL(component_bind_all);
>
> +/**
> + * component_add - register a component
> + * @dev: component device
> + * @ops: component callbacks
> + *
> + * Register a new component for @dev. Functions in @ops will be call when the

s/call/called/

> + * aggregate driver is ready to bind the overall driver by calling
> + * component_bind_all(). See also &struct component_ops.
> + *
> + * The component needs to be unregistered again at driver unload/disconnect by
> + * calling component_del().
> + */
>  int component_add(struct device *dev, const struct component_ops *ops)
>  {
>         struct component *component;
> @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(component_add);
>
> +/**
> + * component_del - unregister a component
> + * @dev: component device
> + * @ops: component callbacks
> + *
> + * Unregister a component added with component_add(). If the component is bound
> + * into an aggregate driver this will force the entire aggrate driver, including

A comma is missing after "driver".  Also s/aggrate/aggregate/

> + * all its components, to be unbound.
> + */
>  void component_del(struct device *dev, const struct component_ops *ops)
>  {
>         struct component *c, *component = NULL;
> diff --git a/include/linux/component.h b/include/linux/component.h
> index e71fbbbc74e2..67a899dd2e10 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -4,11 +4,31 @@
>
>  #include <linux/stddef.h>
>
> +
>  struct device;
>
> +/**
> + * struct component_ops - callbacks for component drivers
> + *
> + * Components are registered with component_add() and unregistered with
> + * component_del().
> + */
>  struct component_ops {
> +       /**
> +        * @bind:
> +        *
> +        * Called through component_bind_all() when the aggregate driver is
> +        * ready to bind the overall driver.
> +        */
>         int (*bind)(struct device *comp, struct device *master,
>                     void *master_data);
> +       /**
> +        * @unbind:
> +        *
> +        * Called through component_unbind_all() when the aggregate driver is
> +        * ready to bind the overall driver, or when component_bind_all() fails
> +        * part-ways through and needs to unbind some already bound components.
> +        */
>         void (*unbind)(struct device *comp, struct device *master,
>                        void *master_data);
>  };
> @@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);
>
>  struct master;
>
> +/**
> + * struct component_master_ops - callback for the aggregate driver
> + *
> + * Aggregate drivers are registered with component_master_add_with_match() and
> + * unregistered with component_master_del().
> + */
>  struct component_master_ops {
> +       /**
> +        * @bind:
> +        *
> +        * Called when all components or the aggregate driver, as specified in
> +        * the match list passed to component_master_add_with_match(), are
> +        * ready. Usually there are 3 steps to bind an aggregate driver:
> +        *
> +        * 1. Allocate a structure for the aggregate driver.
> +        *
> +        * 2. Bind all components to the aggregate driver by calling
> +        *    component_bind_all() with the aggregate driver structure as opaque
> +        *    pointer data.
> +        *
> +        * 3. Register the aggregate driver with the subsystem to publish its
> +        *    interfaces.
> +        *
> +        * Note that the lifetime of the aggregate driver does not align with
> +        * any of the underlying &struct device instances. Therefore devm cannot
> +        * be used and all resources acquired or allocated in this callback must
> +        * be expecitly released in the @unbind callback.

s/expecitly/explicitly/

> +        */
>         int (*bind)(struct device *master);
> +       /**
> +        * @unbind:
> +        *
> +        * Called when either the aggregate driver, using
> +        * component_master_del(), or one of its components, using
> +        * component_del(), is unregistered.
> +        */
>         void (*unbind)(struct device *master);
>  };
>
> @@ -38,6 +92,22 @@ void component_match_add_release(struct device *master,
>         void (*release)(struct device *, void *),
>         int (*compare)(struct device *, void *), void *compare_data);
>
> +/**
> + * component_match_add - add a compent match
> + * @master: device with the aggregate driver
> + * @matchptr: pointer to the list of component matches
> + * @compare: compare function to match against all components
> + * @compare_data: opaque pointer passed to the @compare function
> + *
> + * This adds a new component match to the list stored in @matchptr, which the

"This" appears to be redundant.

> + * @master aggregate driver needs to function. @matchptr must be initialized to
> + * NULL before adding the first match.
> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions.
> + *
> + * See also component_match_add_release().
> + */
>  static inline void component_match_add(struct device *master,
>         struct component_match **matchptr,
>         int (*compare)(struct device *, void *), void *compare_data)
> --
> 2.20.1
>

Re: [PATCH 1/2] component: Add documentation

[Daniel Vetter]

On Tue, Feb 05, 2019 at 11:47:58AM +0100, Rafael J. Wysocki wrote:
>  w/compOn Thu, Jan 31, 2019 at 3:46 PM Daniel Vetter
> <daniel.vetter@ffwll.ch> wrote:
> >
> > Someone owes me a beer ...
> >
> > While typing these I think doing an s/component_master/aggregate/
> > would be useful:
> > - it's shorter :-)
> > - I think component/aggregate is much more meaningful naming than
> >   component/puppetmaster or something like that. At least to my
> >   English ear "aggregate" emphasizes much more the "assemble a pile of
> >   things into something bigger" aspect, and there's not really much
> >   of a control hierarchy between aggregate and constituing components.
> >
> > But that's way more than a quick doc typing exercise ...
> >
> > Thanks to Ram for commenting on an initial draft of these docs.
> 
> Look goods to me overall (even though I'm not super-familiar with the
> component framework), but see below.
> 
> > Cc: "C, Ramalingam" <ramalingam.c@intel.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>
> > Cc: Jaroslav Kysela <perex@perex.cz>
> > Cc: Takashi Iwai <tiwai@suse.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  Documentation/driver-api/device_link.rst |   3 +
> >  Documentation/driver-api/index.rst       |   1 +
> >  drivers/base/component.c                 | 107 ++++++++++++++++++++++-
> >  include/linux/component.h                |  70 +++++++++++++++
> >  4 files changed, 178 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
> > index d6763272e747..2d5919b2b337 100644
> > --- a/Documentation/driver-api/device_link.rst
> > +++ b/Documentation/driver-api/device_link.rst
> > @@ -1,6 +1,9 @@
> >  .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>`
> >  .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`
> >
> > +
> > +.. _device_link:
> > +
> >  ============
> >  Device links
> >  ============
> > diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> > index ab38ced66a44..c0b600ed9961 100644
> > --- a/Documentation/driver-api/index.rst
> > +++ b/Documentation/driver-api/index.rst
> > @@ -22,6 +22,7 @@ available subsections can be seen below.
> >     device_connection
> >     dma-buf
> >     device_link
> > +   component
> 
> Do I think correctly that this doc is going to be generated
> automatically from the kerneldoc comments in component.c?

No. I failed to git add component.rst, which has the necessary kernel-doc
directives to generate the documentation from the source code comments.

I've implemented all your other suggestions, thanks a lot for taking a
lot. Will send out v2 asap.

Thanks, Daniel

> 
> >     message-based
> >     sound
> >     frame-buffer
> > diff --git a/drivers/base/component.c b/drivers/base/component.c
> > index ddcea8739c12..e5b04bce8544 100644
> > --- a/drivers/base/component.c
> > +++ b/drivers/base/component.c
> > @@ -16,6 +16,33 @@
> >  #include <linux/slab.h>
> >  #include <linux/debugfs.h>
> >
> > +/**
> > + * DOC: overview
> > + *
> > + * The component frameworks allows drivers to collect a pile of sub-devices,
> 
> s/frameworks/framework/
> 
> > + * including their bound drivers, into an aggregate driver. Various subsystem
> 
> s/subsystem/subsystems/
> 
> > + * already provide functions to get hold of such components, e.g.
> > + * of_clk_get_by_name(). Anytime there's such a subsystem specific way to find a
> > + * a device the component framework should not be used.
> 
> I would use a positive statement here, like "The component framework
> can be used when such a subsystem-specific way to find a device is not
> available".
> 
> > The component framework
> > + * fills the niche of aggregate drivers for specific hardware, where further
> > + * standardization into a subsystem doesn't make sense.
> 
> I would say "would not be practical" instead of "doesn't make sense".
> 
> > The common example is
> > + * when a logical device (e.g. a DRM display driver) is spread around the SoC on
> > + * various component (scanout engines, blending blocks, transcoders for various
> > + * outputs and so on).
> > + *
> > + * The component framework also doesn't solve runtime dependencies, e.g. for
> > + * system suspend and resume operations. See also :ref:`device
> > + * links<device_link>`.
> > + *
> > + * Components are registered using component_add() and unregistered with
> > + * component_del(), usually from the driver's probe and disconnect functions.
> > + *
> > + * Aggregate drivers first assemble a component match list of what they need
> > + * using component_match_add(). This is then registered as an aggregate driver
> > + * using component_master_add_with_match(), and unregistered using
> > + * component_master_del().
> > + */
> > +
> >  struct component;
> >
> >  struct component_match_array {
> > @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev,
> >         return 0;
> >  }
> >
> > -/*
> > - * Add a component to be matched, with a release function.
> > +/**
> > + * component_match_add_release - add a compent match with release callback
> 
> s/compent/component/ ?
> 
> > + * @master: device with the aggregate driver
> > + * @matchptr: pointer to the list of component matches
> > + * @release: release function for @compare_data
> > + * @compare: compare function to match against all components
> > + * @compare_data: opaque pointer passed to the @compare function
> > + *
> > + * This adds a new component match to the list stored in @matchptr, which the
> > + * @master aggregate driver needs to function. @matchptr must be initialized to
> > + * NULL before adding the first match.
> > + *
> > + * The allocated match list in @matchptr is automatically released using devm
> > + * actions. At that point @release will be called, to free any references held
> > + * by @compare_data, e.g. when @compare_data is a &device_node that must be
> > + * released with of_node_put().
> >   *
> > - * The match array is first created or extended if necessary.
> > + * See also component_match_add().
> >   */
> >  void component_match_add_release(struct device *master,
> >         struct component_match **matchptr,
> > @@ -367,6 +408,18 @@ static void free_master(struct master *master)
> >         kfree(master);
> >  }
> >
> > +/**
> > + * component_master_add_with_match - register an aggregate driver
> > + * @dev: device with the aggregate driver
> > + * @ops: callbacks for the aggregate driver
> > + * @match: component match list for the aggregate driver
> > + *
> > + * Registers a new aggregate driver consisting of the components added to @match
> > + * by calling one of the component_match_add() functions. Once all components in
> > + * @match are available it will be assembled by calling
> 
> A comma seems to be missing after "available".
> 
> > + * &component_master_ops.bind from @ops. Must be unregistered by calling
> > + * component_master_del().
> > + */
> >  int component_master_add_with_match(struct device *dev,
> >         const struct component_master_ops *ops,
> >         struct component_match *match)
> > @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev,
> >  }
> >  EXPORT_SYMBOL_GPL(component_master_add_with_match);
> >
> > +/**
> > + * component_master_del - unregister an aggregate driver
> > + * @dev: device with the aggregate driver
> > + * @ops: callbacks for the aggregate driver
> > + *
> > + * Unregistered an aggregate driver registered with
> 
> s/Unregistered/Unregisters/ ?
> 
> > + * component_master_add_with_match(). If necessary the aggregate driver is first
> > + * disassembled by calling &component_master_ops.unbind from @ops.
> 
> Q: How does the &component_master_ops.unbind annotation work?  Does it
> produce any special output?
> 
> > + */
> >  void component_master_del(struct device *dev,
> >         const struct component_master_ops *ops)
> >  {
> > @@ -430,6 +492,15 @@ static void component_unbind(struct component *component,
> >         devres_release_group(component->dev, component);
> >  }
> >
> > +/**
> > + * component_unbind_all - unbind all component to an aggregate driver
> > + * @master_dev: device with the aggregate driver
> > + * @data: opaque pointer, passed to all components
> > + *
> > + * This unbinds all components to the aggregate @dev by passing @data to their
> 
> I guess "This" is redundant.
> 
> > + * &component_ops.unbind functions. Should be called from
> > + * &component_master_ops.unbind.
> > + */
> >  void component_unbind_all(struct device *master_dev, void *data)
> >  {
> >         struct master *master;
> > @@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master,
> >         return ret;
> >  }
> >
> > +/**
> > + * component_bind_all - bind all component to an aggregate driver
> > + * @master_dev: device with the aggregate driver
> > + * @data: opaque pointer, passed to all components
> > + *
> > + * This binds all components to the aggregate @dev by passing @data to their
> 
> Likewise.
> 
> > + * &component_ops.bind functions. Should be called from
> > + * &component_master_ops.bind.
> > + */
> >  int component_bind_all(struct device *master_dev, void *data)
> >  {
> >         struct master *master;
> > @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data)
> >  }
> >  EXPORT_SYMBOL_GPL(component_bind_all);
> >
> > +/**
> > + * component_add - register a component
> > + * @dev: component device
> > + * @ops: component callbacks
> > + *
> > + * Register a new component for @dev. Functions in @ops will be call when the
> 
> s/call/called/
> 
> > + * aggregate driver is ready to bind the overall driver by calling
> > + * component_bind_all(). See also &struct component_ops.
> > + *
> > + * The component needs to be unregistered again at driver unload/disconnect by
> > + * calling component_del().
> > + */
> >  int component_add(struct device *dev, const struct component_ops *ops)
> >  {
> >         struct component *component;
> > @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops)
> >  }
> >  EXPORT_SYMBOL_GPL(component_add);
> >
> > +/**
> > + * component_del - unregister a component
> > + * @dev: component device
> > + * @ops: component callbacks
> > + *
> > + * Unregister a component added with component_add(). If the component is bound
> > + * into an aggregate driver this will force the entire aggrate driver, including
> 
> A comma is missing after "driver".  Also s/aggrate/aggregate/
> 
> > + * all its components, to be unbound.
> > + */
> >  void component_del(struct device *dev, const struct component_ops *ops)
> >  {
> >         struct component *c, *component = NULL;
> > diff --git a/include/linux/component.h b/include/linux/component.h
> > index e71fbbbc74e2..67a899dd2e10 100644
> > --- a/include/linux/component.h
> > +++ b/include/linux/component.h
> > @@ -4,11 +4,31 @@
> >
> >  #include <linux/stddef.h>
> >
> > +
> >  struct device;
> >
> > +/**
> > + * struct component_ops - callbacks for component drivers
> > + *
> > + * Components are registered with component_add() and unregistered with
> > + * component_del().
> > + */
> >  struct component_ops {
> > +       /**
> > +        * @bind:
> > +        *
> > +        * Called through component_bind_all() when the aggregate driver is
> > +        * ready to bind the overall driver.
> > +        */
> >         int (*bind)(struct device *comp, struct device *master,
> >                     void *master_data);
> > +       /**
> > +        * @unbind:
> > +        *
> > +        * Called through component_unbind_all() when the aggregate driver is
> > +        * ready to bind the overall driver, or when component_bind_all() fails
> > +        * part-ways through and needs to unbind some already bound components.
> > +        */
> >         void (*unbind)(struct device *comp, struct device *master,
> >                        void *master_data);
> >  };
> > @@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);
> >
> >  struct master;
> >
> > +/**
> > + * struct component_master_ops - callback for the aggregate driver
> > + *
> > + * Aggregate drivers are registered with component_master_add_with_match() and
> > + * unregistered with component_master_del().
> > + */
> >  struct component_master_ops {
> > +       /**
> > +        * @bind:
> > +        *
> > +        * Called when all components or the aggregate driver, as specified in
> > +        * the match list passed to component_master_add_with_match(), are
> > +        * ready. Usually there are 3 steps to bind an aggregate driver:
> > +        *
> > +        * 1. Allocate a structure for the aggregate driver.
> > +        *
> > +        * 2. Bind all components to the aggregate driver by calling
> > +        *    component_bind_all() with the aggregate driver structure as opaque
> > +        *    pointer data.
> > +        *
> > +        * 3. Register the aggregate driver with the subsystem to publish its
> > +        *    interfaces.
> > +        *
> > +        * Note that the lifetime of the aggregate driver does not align with
> > +        * any of the underlying &struct device instances. Therefore devm cannot
> > +        * be used and all resources acquired or allocated in this callback must
> > +        * be expecitly released in the @unbind callback.
> 
> s/expecitly/explicitly/
> 
> > +        */
> >         int (*bind)(struct device *master);
> > +       /**
> > +        * @unbind:
> > +        *
> > +        * Called when either the aggregate driver, using
> > +        * component_master_del(), or one of its components, using
> > +        * component_del(), is unregistered.
> > +        */
> >         void (*unbind)(struct device *master);
> >  };
> >
> > @@ -38,6 +92,22 @@ void component_match_add_release(struct device *master,
> >         void (*release)(struct device *, void *),
> >         int (*compare)(struct device *, void *), void *compare_data);
> >
> > +/**
> > + * component_match_add - add a compent match
> > + * @master: device with the aggregate driver
> > + * @matchptr: pointer to the list of component matches
> > + * @compare: compare function to match against all components
> > + * @compare_data: opaque pointer passed to the @compare function
> > + *
> > + * This adds a new component match to the list stored in @matchptr, which the
> 
> "This" appears to be redundant.
> 
> > + * @master aggregate driver needs to function. @matchptr must be initialized to
> > + * NULL before adding the first match.
> > + *
> > + * The allocated match list in @matchptr is automatically released using devm
> > + * actions.
> > + *
> > + * See also component_match_add_release().
> > + */
> >  static inline void component_match_add(struct device *master,
> >         struct component_match **matchptr,
> >         int (*compare)(struct device *, void *), void *compare_data)
> > --
> > 2.20.1
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH] component: Add documentation

[Daniel Vetter]

Someone owes me a beer ...

While typing these I think doing an s/component_master/aggregate/
would be useful:
- it's shorter :-)
- I think component/aggregate is much more meaningful naming than
  component/puppetmaster or something like that. At least to my
  English ear "aggregate" emphasizes much more the "assemble a pile of
  things into something bigger" aspect, and there's not really much
  of a control hierarchy between aggregate and constituing components.

But that's way more than a quick doc typing exercise ...

Thanks to Ram for commenting on an initial draft of these docs.

v2: Review from Rafael:
- git add Documenation/driver-api/component.rst
- lots of polish to the wording + spelling fixes.

Cc: "C, Ramalingam" <ramalingam.c@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Rafael J. Wysocki <rafael@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/driver-api/component.rst   |  17 ++++
 Documentation/driver-api/device_link.rst |   3 +
 Documentation/driver-api/index.rst       |   1 +
 drivers/base/component.c                 | 107 ++++++++++++++++++++++-
 include/linux/component.h                |  70 +++++++++++++++
 5 files changed, 195 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/driver-api/component.rst

diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
new file mode 100644
index 000000000000..3407ff0424b9
--- /dev/null
+++ b/Documentation/driver-api/component.rst
@@ -0,0 +1,17 @@
+=========================================
+Component Framework for Aggregate Drivers
+=========================================
+
+.. kernel-doc:: drivers/base/component.c
+   :doc: overview
+
+
+API
+===
+
+.. kernel-doc:: include/linux/component.h
+   :internal:
+
+.. kernel-doc:: drivers/base/component.c
+   :export:
+
diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
index d6763272e747..2d5919b2b337 100644
--- a/Documentation/driver-api/device_link.rst
+++ b/Documentation/driver-api/device_link.rst
@@ -1,6 +1,9 @@
 .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>`
 .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`
 
+
+.. _device_link:
+
 ============
 Device links
 ============
diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
index ab38ced66a44..c0b600ed9961 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -22,6 +22,7 @@ available subsections can be seen below.
    device_connection
    dma-buf
    device_link
+   component
    message-based
    sound
    frame-buffer
diff --git a/drivers/base/component.c b/drivers/base/component.c
index ddcea8739c12..4851e1006f11 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -16,6 +16,33 @@
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 
+/**
+ * DOC: overview
+ *
+ * The component framework allows drivers to collect a pile of sub-devices,
+ * including their bound drivers, into an aggregate driver. Various subsystems
+ * already provide functions to get hold of such components, e.g.
+ * of_clk_get_by_name(). The component framework can be used when such a
+ * subsystem-specific way to find a device is not available: The component
+ * framework fills the niche of aggregate drivers for specific hardware, where
+ * further standardization into a subsystem would not be practical. The common
+ * example is when a logical device (e.g. a DRM display driver) is spread around
+ * the SoC on various component (scanout engines, blending blocks, transcoders
+ * for various outputs and so on).
+ *
+ * The component framework also doesn't solve runtime dependencies, e.g. for
+ * system suspend and resume operations. See also :ref:`device
+ * links<device_link>`.
+ *
+ * Components are registered using component_add() and unregistered with
+ * component_del(), usually from the driver's probe and disconnect functions.
+ *
+ * Aggregate drivers first assemble a component match list of what they need
+ * using component_match_add(). This is then registered as an aggregate driver
+ * using component_master_add_with_match(), and unregistered using
+ * component_master_del().
+ */
+
 struct component;
 
 struct component_match_array {
@@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev,
 	return 0;
 }
 
-/*
- * Add a component to be matched, with a release function.
+/**
+ * component_match_add_release - add a component match with release callback
+ * @master: device with the aggregate driver
+ * @matchptr: pointer to the list of component matches
+ * @release: release function for @compare_data
+ * @compare: compare function to match against all components
+ * @compare_data: opaque pointer passed to the @compare function
+ *
+ * This adds a new component match to the list stored in @matchptr, which the
+ * @master aggregate driver needs to function. @matchptr must be initialized to
+ * NULL before adding the first match.
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions. At that point @release will be called, to free any references held
+ * by @compare_data, e.g. when @compare_data is a &device_node that must be
+ * released with of_node_put().
  *
- * The match array is first created or extended if necessary.
+ * See also component_match_add().
  */
 void component_match_add_release(struct device *master,
 	struct component_match **matchptr,
@@ -367,6 +408,18 @@ static void free_master(struct master *master)
 	kfree(master);
 }
 
+/**
+ * component_master_add_with_match - register an aggregate driver
+ * @dev: device with the aggregate driver
+ * @ops: callbacks for the aggregate driver
+ * @match: component match list for the aggregate driver
+ *
+ * Registers a new aggregate driver consisting of the components added to @match
+ * by calling one of the component_match_add() functions. Once all components in
+ * @match are available, it will be assembled by calling
+ * &component_master_ops.bind from @ops. Must be unregistered by calling
+ * component_master_del().
+ */
 int component_master_add_with_match(struct device *dev,
 	const struct component_master_ops *ops,
 	struct component_match *match)
@@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(component_master_add_with_match);
 
+/**
+ * component_master_del - unregister an aggregate driver
+ * @dev: device with the aggregate driver
+ * @ops: callbacks for the aggregate driver
+ *
+ * Unregisters an aggregate driver registered with
+ * component_master_add_with_match(). If necessary the aggregate driver is first
+ * disassembled by calling &component_master_ops.unbind from @ops.
+ */
 void component_master_del(struct device *dev,
 	const struct component_master_ops *ops)
 {
@@ -430,6 +492,15 @@ static void component_unbind(struct component *component,
 	devres_release_group(component->dev, component);
 }
 
+/**
+ * component_unbind_all - unbind all component to an aggregate driver
+ * @master_dev: device with the aggregate driver
+ * @data: opaque pointer, passed to all components
+ *
+ * Unbinds all components to the aggregate @dev by passing @data to their
+ * &component_ops.unbind functions. Should be called from
+ * &component_master_ops.unbind.
+ */
 void component_unbind_all(struct device *master_dev, void *data)
 {
 	struct master *master;
@@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master,
 	return ret;
 }
 
+/**
+ * component_bind_all - bind all component to an aggregate driver
+ * @master_dev: device with the aggregate driver
+ * @data: opaque pointer, passed to all components
+ *
+ * Binds all components to the aggregate @dev by passing @data to their
+ * &component_ops.bind functions. Should be called from
+ * &component_master_ops.bind.
+ */
 int component_bind_all(struct device *master_dev, void *data)
 {
 	struct master *master;
@@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data)
 }
 EXPORT_SYMBOL_GPL(component_bind_all);
 
+/**
+ * component_add - register a component
+ * @dev: component device
+ * @ops: component callbacks
+ *
+ * Register a new component for @dev. Functions in @ops will be called when the
+ * aggregate driver is ready to bind the overall driver by calling
+ * component_bind_all(). See also &struct component_ops.
+ *
+ * The component needs to be unregistered again at driver unload/disconnect by
+ * calling component_del().
+ */
 int component_add(struct device *dev, const struct component_ops *ops)
 {
 	struct component *component;
@@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops)
 }
 EXPORT_SYMBOL_GPL(component_add);
 
+/**
+ * component_del - unregister a component
+ * @dev: component device
+ * @ops: component callbacks
+ *
+ * Unregister a component added with component_add(). If the component is bound
+ * into an aggregate driver, this will force the entire aggregate driver, including
+ * all its components, to be unbound.
+ */
 void component_del(struct device *dev, const struct component_ops *ops)
 {
 	struct component *c, *component = NULL;
diff --git a/include/linux/component.h b/include/linux/component.h
index e71fbbbc74e2..b5a989e3871e 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -4,11 +4,31 @@
 
 #include <linux/stddef.h>
 
+
 struct device;
 
+/**
+ * struct component_ops - callbacks for component drivers
+ *
+ * Components are registered with component_add() and unregistered with
+ * component_del().
+ */
 struct component_ops {
+	/**
+	 * @bind:
+	 *
+	 * Called through component_bind_all() when the aggregate driver is
+	 * ready to bind the overall driver.
+	 */
 	int (*bind)(struct device *comp, struct device *master,
 		    void *master_data);
+	/**
+	 * @unbind:
+	 *
+	 * Called through component_unbind_all() when the aggregate driver is
+	 * ready to bind the overall driver, or when component_bind_all() fails
+	 * part-ways through and needs to unbind some already bound components.
+	 */
 	void (*unbind)(struct device *comp, struct device *master,
 		       void *master_data);
 };
@@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);
 
 struct master;
 
+/**
+ * struct component_master_ops - callback for the aggregate driver
+ *
+ * Aggregate drivers are registered with component_master_add_with_match() and
+ * unregistered with component_master_del().
+ */
 struct component_master_ops {
+	/**
+	 * @bind:
+	 *
+	 * Called when all components or the aggregate driver, as specified in
+	 * the match list passed to component_master_add_with_match(), are
+	 * ready. Usually there are 3 steps to bind an aggregate driver:
+	 *
+	 * 1. Allocate a structure for the aggregate driver.
+	 *
+	 * 2. Bind all components to the aggregate driver by calling
+	 *    component_bind_all() with the aggregate driver structure as opaque
+	 *    pointer data.
+	 *
+	 * 3. Register the aggregate driver with the subsystem to publish its
+	 *    interfaces.
+	 *
+	 * Note that the lifetime of the aggregate driver does not align with
+	 * any of the underlying &struct device instances. Therefore devm cannot
+	 * be used and all resources acquired or allocated in this callback must
+	 * be explicitly released in the @unbind callback.
+	 */
 	int (*bind)(struct device *master);
+	/**
+	 * @unbind:
+	 *
+	 * Called when either the aggregate driver, using
+	 * component_master_del(), or one of its components, using
+	 * component_del(), is unregistered.
+	 */
 	void (*unbind)(struct device *master);
 };
 
@@ -38,6 +92,22 @@ void component_match_add_release(struct device *master,
 	void (*release)(struct device *, void *),
 	int (*compare)(struct device *, void *), void *compare_data);
 
+/**
+ * component_match_add - add a compent match
+ * @master: device with the aggregate driver
+ * @matchptr: pointer to the list of component matches
+ * @compare: compare function to match against all components
+ * @compare_data: opaque pointer passed to the @compare function
+ *
+ * Adds a new component match to the list stored in @matchptr, which the
+ * @master aggregate driver needs to function. @matchptr must be initialized to
+ * NULL before adding the first match.
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions.
+ *
+ * See also component_match_add_release().
+ */
 static inline void component_match_add(struct device *master,
 	struct component_match **matchptr,
 	int (*compare)(struct device *, void *), void *compare_data)
-- 
2.20.1

Re: [PATCH] component: Add documentation

[Russell King - ARM Linux admin]

On Tue, Feb 05, 2019 at 05:21:07PM +0100, Daniel Vetter wrote:
> Someone owes me a beer ...

I find that deeply offensive - it is clearly directed at me personally
as author of the component helper.

There are double-standards in the kernel ecosystem with respect to
documentation - there are entire subsystems way more complicated than
the component *helper* which are lacking in documentation, and the
subsystem authors response to requests to change that basically get
ignored, or the response is "write the documentation yourself".

Why does there seem to be one rule for me and one rule for everyone
else?

Please remove this line.

> 
> While typing these I think doing an s/component_master/aggregate/
> would be useful:
> - it's shorter :-)
> - I think component/aggregate is much more meaningful naming than
>   component/puppetmaster or something like that. At least to my
>   English ear "aggregate" emphasizes much more the "assemble a pile of
>   things into something bigger" aspect, and there's not really much
>   of a control hierarchy between aggregate and constituing components.
> 
> But that's way more than a quick doc typing exercise ...
> 
> Thanks to Ram for commenting on an initial draft of these docs.
> 
> v2: Review from Rafael:
> - git add Documenation/driver-api/component.rst
> - lots of polish to the wording + spelling fixes.
> 
> Cc: "C, Ramalingam" <ramalingam.c@intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Rafael J. Wysocki <rafael@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/driver-api/component.rst   |  17 ++++
>  Documentation/driver-api/device_link.rst |   3 +
>  Documentation/driver-api/index.rst       |   1 +
>  drivers/base/component.c                 | 107 ++++++++++++++++++++++-
>  include/linux/component.h                |  70 +++++++++++++++
>  5 files changed, 195 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/driver-api/component.rst
> 
> diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
> new file mode 100644
> index 000000000000..3407ff0424b9
> --- /dev/null
> +++ b/Documentation/driver-api/component.rst
> @@ -0,0 +1,17 @@
> +=========================================
> +Component Framework for Aggregate Drivers
> +=========================================
> +
> +.. kernel-doc:: drivers/base/component.c
> +   :doc: overview
> +
> +
> +API
> +===
> +
> +.. kernel-doc:: include/linux/component.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/base/component.c
> +   :export:
> +
> diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst
> index d6763272e747..2d5919b2b337 100644
> --- a/Documentation/driver-api/device_link.rst
> +++ b/Documentation/driver-api/device_link.rst
> @@ -1,6 +1,9 @@
>  .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>`
>  .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`
>  
> +
> +.. _device_link:
> +
>  ============
>  Device links
>  ============
> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> index ab38ced66a44..c0b600ed9961 100644
> --- a/Documentation/driver-api/index.rst
> +++ b/Documentation/driver-api/index.rst
> @@ -22,6 +22,7 @@ available subsections can be seen below.
>     device_connection
>     dma-buf
>     device_link
> +   component
>     message-based
>     sound
>     frame-buffer
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index ddcea8739c12..4851e1006f11 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -16,6 +16,33 @@
>  #include <linux/slab.h>
>  #include <linux/debugfs.h>
>  
> +/**
> + * DOC: overview
> + *
> + * The component framework allows drivers to collect a pile of sub-devices,

Helper.

> + * including their bound drivers, into an aggregate driver. Various subsystems
> + * already provide functions to get hold of such components, e.g.
> + * of_clk_get_by_name(). The component framework can be used when such a

helper

> + * subsystem-specific way to find a device is not available: The component
> + * framework fills the niche of aggregate drivers for specific hardware, where

helper

> + * further standardization into a subsystem would not be practical. The common
> + * example is when a logical device (e.g. a DRM display driver) is spread around
> + * the SoC on various component (scanout engines, blending blocks, transcoders
> + * for various outputs and so on).
> + *
> + * The component framework also doesn't solve runtime dependencies, e.g. for

helper

> + * system suspend and resume operations. See also :ref:`device
> + * links<device_link>`.
> + *
> + * Components are registered using component_add() and unregistered with
> + * component_del(), usually from the driver's probe and disconnect functions.
> + *
> + * Aggregate drivers first assemble a component match list of what they need
> + * using component_match_add(). This is then registered as an aggregate driver
> + * using component_master_add_with_match(), and unregistered using
> + * component_master_del().
> + */
> +
>  struct component;
>  
>  struct component_match_array {
> @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev,
>  	return 0;
>  }
>  
> -/*
> - * Add a component to be matched, with a release function.
> +/**
> + * component_match_add_release - add a component match with release callback
> + * @master: device with the aggregate driver
> + * @matchptr: pointer to the list of component matches
> + * @release: release function for @compare_data
> + * @compare: compare function to match against all components
> + * @compare_data: opaque pointer passed to the @compare function
> + *
> + * This adds a new component match to the list stored in @matchptr, which the
> + * @master aggregate driver needs to function. @matchptr must be initialized to
> + * NULL before adding the first match.

The last sentence is confusing - it suggests that "matchptr" should
itself be null, rather than the pointer matchptr points at.  "The
list of component matches pointed to by @matchptr must be initialised
to NULL" would probably be better.  Same for component_match_add().

> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions. At that point @release will be called, to free any references held
> + * by @compare_data, e.g. when @compare_data is a &device_node that must be
> + * released with of_node_put().

"using devm actions, where upon @release will be called to free any
references held by @compare_data ..."

>   *
> - * The match array is first created or extended if necessary.
> + * See also component_match_add().
>   */
>  void component_match_add_release(struct device *master,
>  	struct component_match **matchptr,
> @@ -367,6 +408,18 @@ static void free_master(struct master *master)
>  	kfree(master);
>  }
>  
> +/**
> + * component_master_add_with_match - register an aggregate driver
> + * @dev: device with the aggregate driver
> + * @ops: callbacks for the aggregate driver
> + * @match: component match list for the aggregate driver
> + *
> + * Registers a new aggregate driver consisting of the components added to @match
> + * by calling one of the component_match_add() functions. Once all components in

As there is a function called component_match_add(), this doesn't
make too much sense as it directs people only to that function.  It
would be better to mention both here.  (Have you checked how it comes
out as a HTML document with the hyperlinks?)

> + * @match are available, it will be assembled by calling
> + * &component_master_ops.bind from @ops. Must be unregistered by calling
> + * component_master_del().
> + */
>  int component_master_add_with_match(struct device *dev,
>  	const struct component_master_ops *ops,
>  	struct component_match *match)
> @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(component_master_add_with_match);
>  
> +/**
> + * component_master_del - unregister an aggregate driver
> + * @dev: device with the aggregate driver
> + * @ops: callbacks for the aggregate driver
> + *
> + * Unregisters an aggregate driver registered with
> + * component_master_add_with_match(). If necessary the aggregate driver is first
> + * disassembled by calling &component_master_ops.unbind from @ops.
> + */
>  void component_master_del(struct device *dev,
>  	const struct component_master_ops *ops)
>  {
> @@ -430,6 +492,15 @@ static void component_unbind(struct component *component,
>  	devres_release_group(component->dev, component);
>  }
>  
> +/**
> + * component_unbind_all - unbind all component to an aggregate driver
> + * @master_dev: device with the aggregate driver
> + * @data: opaque pointer, passed to all components
> + *
> + * Unbinds all components to the aggregate @dev by passing @data to their
> + * &component_ops.unbind functions. Should be called from
> + * &component_master_ops.unbind.
> + */
>  void component_unbind_all(struct device *master_dev, void *data)
>  {
>  	struct master *master;
> @@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master,
>  	return ret;
>  }
>  
> +/**
> + * component_bind_all - bind all component to an aggregate driver
> + * @master_dev: device with the aggregate driver
> + * @data: opaque pointer, passed to all components
> + *
> + * Binds all components to the aggregate @dev by passing @data to their
> + * &component_ops.bind functions. Should be called from
> + * &component_master_ops.bind.
> + */
>  int component_bind_all(struct device *master_dev, void *data)
>  {
>  	struct master *master;
> @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data)
>  }
>  EXPORT_SYMBOL_GPL(component_bind_all);
>  
> +/**
> + * component_add - register a component
> + * @dev: component device
> + * @ops: component callbacks
> + *
> + * Register a new component for @dev. Functions in @ops will be called when the
> + * aggregate driver is ready to bind the overall driver by calling
> + * component_bind_all(). See also &struct component_ops.
> + *
> + * The component needs to be unregistered again at driver unload/disconnect by

"again" is unnecessary.

> + * calling component_del().
> + */
>  int component_add(struct device *dev, const struct component_ops *ops)
>  {
>  	struct component *component;
> @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops)
>  }
>  EXPORT_SYMBOL_GPL(component_add);
>  
> +/**
> + * component_del - unregister a component
> + * @dev: component device
> + * @ops: component callbacks
> + *
> + * Unregister a component added with component_add(). If the component is bound
> + * into an aggregate driver, this will force the entire aggregate driver, including
> + * all its components, to be unbound.
> + */
>  void component_del(struct device *dev, const struct component_ops *ops)
>  {
>  	struct component *c, *component = NULL;
> diff --git a/include/linux/component.h b/include/linux/component.h
> index e71fbbbc74e2..b5a989e3871e 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -4,11 +4,31 @@
>  
>  #include <linux/stddef.h>
>  
> +
>  struct device;
>  
> +/**
> + * struct component_ops - callbacks for component drivers
> + *
> + * Components are registered with component_add() and unregistered with
> + * component_del().
> + */
>  struct component_ops {
> +	/**
> +	 * @bind:
> +	 *
> +	 * Called through component_bind_all() when the aggregate driver is
> +	 * ready to bind the overall driver.
> +	 */
>  	int (*bind)(struct device *comp, struct device *master,
>  		    void *master_data);
> +	/**
> +	 * @unbind:
> +	 *
> +	 * Called through component_unbind_all() when the aggregate driver is
> +	 * ready to bind the overall driver, or when component_bind_all() fails
> +	 * part-ways through and needs to unbind some already bound components.
> +	 */
>  	void (*unbind)(struct device *comp, struct device *master,
>  		       void *master_data);
>  };
> @@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);
>  
>  struct master;
>  
> +/**
> + * struct component_master_ops - callback for the aggregate driver
> + *
> + * Aggregate drivers are registered with component_master_add_with_match() and
> + * unregistered with component_master_del().
> + */
>  struct component_master_ops {
> +	/**
> +	 * @bind:
> +	 *
> +	 * Called when all components or the aggregate driver, as specified in
> +	 * the match list passed to component_master_add_with_match(), are
> +	 * ready. Usually there are 3 steps to bind an aggregate driver:
> +	 *
> +	 * 1. Allocate a structure for the aggregate driver.
> +	 *
> +	 * 2. Bind all components to the aggregate driver by calling
> +	 *    component_bind_all() with the aggregate driver structure as opaque
> +	 *    pointer data.

These two aren't part of the component helper specification, although
nailing down what is passed per subsystem would be a good idea to allow
components to be re-used within the subsystem.  For DRM, it should be
the struct drm_device.

> +	 *
> +	 * 3. Register the aggregate driver with the subsystem to publish its
> +	 *    interfaces.
> +	 *
> +	 * Note that the lifetime of the aggregate driver does not align with
> +	 * any of the underlying &struct device instances. Therefore devm cannot
> +	 * be used and all resources acquired or allocated in this callback must
> +	 * be explicitly released in the @unbind callback.
> +	 */
>  	int (*bind)(struct device *master);
> +	/**
> +	 * @unbind:
> +	 *
> +	 * Called when either the aggregate driver, using
> +	 * component_master_del(), or one of its components, using
> +	 * component_del(), is unregistered.
> +	 */
>  	void (*unbind)(struct device *master);
>  };
>  
> @@ -38,6 +92,22 @@ void component_match_add_release(struct device *master,
>  	void (*release)(struct device *, void *),
>  	int (*compare)(struct device *, void *), void *compare_data);
>  
> +/**
> + * component_match_add - add a compent match
> + * @master: device with the aggregate driver
> + * @matchptr: pointer to the list of component matches
> + * @compare: compare function to match against all components
> + * @compare_data: opaque pointer passed to the @compare function
> + *
> + * Adds a new component match to the list stored in @matchptr, which the
> + * @master aggregate driver needs to function. @matchptr must be initialized to
> + * NULL before adding the first match.
> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions.
> + *
> + * See also component_match_add_release().
> + */
>  static inline void component_match_add(struct device *master,
>  	struct component_match **matchptr,
>  	int (*compare)(struct device *, void *), void *compare_data)
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] component: Add documentation

[Daniel Vetter]

On Tue, Feb 05, 2019 at 04:49:02PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Feb 05, 2019 at 05:21:07PM +0100, Daniel Vetter wrote:
> > Someone owes me a beer ...
> 
> I find that deeply offensive - it is clearly directed at me personally
> as author of the component helper.
> 
> There are double-standards in the kernel ecosystem with respect to
> documentation - there are entire subsystems way more complicated than
> the component *helper* which are lacking in documentation, and the
> subsystem authors response to requests to change that basically get
> ignored, or the response is "write the documentation yourself".
> 
> Why does there seem to be one rule for me and one rule for everyone
> else?
> 
> Please remove this line.

Will do, but wasn't aimed at you at all, but at Greg for asking for the
documentation. But yeah that wasn't clear at all, my apologies.

Will apply all your suggestions except for the ones I'm commenting on here
in my reply.

[snip]

> > +/**
> > + * component_master_add_with_match - register an aggregate driver
> > + * @dev: device with the aggregate driver
> > + * @ops: callbacks for the aggregate driver
> > + * @match: component match list for the aggregate driver
> > + *
> > + * Registers a new aggregate driver consisting of the components added to @match
> > + * by calling one of the component_match_add() functions. Once all components in
> 
> As there is a function called component_match_add(), this doesn't
> make too much sense as it directs people only to that function.  It
> would be better to mention both here.  (Have you checked how it comes
> out as a HTML document with the hyperlinks?)

component_match_add() creates a link to the kerneldoc for the
component_match_add. There I've added some text to point to all the other
variants of this family of functions. Same for all the other variants,
those should all have links to the others.


[snip]

> > +/**
> > + * struct component_master_ops - callback for the aggregate driver
> > + *
> > + * Aggregate drivers are registered with component_master_add_with_match() and
> > + * unregistered with component_master_del().
> > + */
> >  struct component_master_ops {
> > +	/**
> > +	 * @bind:
> > +	 *
> > +	 * Called when all components or the aggregate driver, as specified in
> > +	 * the match list passed to component_master_add_with_match(), are
> > +	 * ready. Usually there are 3 steps to bind an aggregate driver:
> > +	 *
> > +	 * 1. Allocate a structure for the aggregate driver.
> > +	 *
> > +	 * 2. Bind all components to the aggregate driver by calling
> > +	 *    component_bind_all() with the aggregate driver structure as opaque
> > +	 *    pointer data.
> 
> These two aren't part of the component helper specification, although
> nailing down what is passed per subsystem would be a good idea to allow
> components to be re-used within the subsystem.  For DRM, it should be
> the struct drm_device.

Hm, great point. I have no idea where we should put it so people find
this. Definitely not here, since this isn't drivers/gpu. I think adding a
small section to the driver initialization docs we already have would make
sense.

I'll add another patch for that in this series, with this one here that
drm paragraph will even have somewhere meaningful to point to!

Thanks for your review.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch