[PATCH 1/4] component: Add documentation

[PATCH 1/4] component: Add documentation

[Daniel Vetter]

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.

v3: Review from Russell:
- s/framework/helper
- clarify the documentation for component_match_add functions.

v4: Remove a few superflous "This".

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
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                 | 106 ++++++++++++++++++++++-
 include/linux/component.h                |  70 +++++++++++++++
 5 files changed, 194 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..2da4a8f20607
--- /dev/null
+++ b/Documentation/driver-api/component.rst
@@ -0,0 +1,17 @@
+======================================
+Component Helper 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..1624c2a892a5 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -16,6 +16,32 @@
 #include <linux/slab.h>
 #include <linux/debugfs.h>
 
+/**
+ * DOC: overview
+ *
+ * The component helper 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 helper can be used when such a
+ * subsystem-specific way to find a device is not available: The component
+ * helper 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 helper 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 +327,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
+ *
+ * Adds a new component match to the list stored in @matchptr, which the @master
+ * aggregate driver needs to function. The list of component matches pointed to
+ * by @matchptr must be initialized to NULL before adding the first match.
+ *
+ * The allocated match list in @matchptr is automatically released using devm
+ * actions, where upon @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 +407,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 +455,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 +491,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 +573,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 +616,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 at driver unload/disconnect by calling
+ * component_del().
+ */
 int component_add(struct device *dev, const struct component_ops *ops)
 {
 	struct component *component;
@@ -568,6 +659,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..83da25bdf59c 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. The list of component matches pointed to
+ * by @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/4] 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.

v3: Rebase on top of updated documenation.

v4: Review from Rafael:
- Remove redundant "This" from kerneldoc (also in the previous patch)
- Streamline the logic in find_component() a bit.

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  | 158 +++++++++++++++++++++++++++++---------
 include/linux/component.h |  10 ++-
 2 files changed, 129 insertions(+), 39 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c
index 1624c2a892a5..7dbc41cccd58 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -47,6 +47,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;
@@ -74,6 +75,7 @@ struct component {
 	bool bound;
 
 	const struct component_ops *ops;
+	int subcomponent;
 	struct device *dev;
 };
 
@@ -158,7 +160,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;
 
@@ -166,7 +168,11 @@ static struct component *find_component(struct master *master,
 		if (c->master && c->master != master)
 			continue;
 
-		if (compare(c->dev, compare_data))
+		if (mc->compare && mc->compare(c->dev, mc->data))
+			return c;
+
+		if (mc->compare_typed &&
+		    mc->compare_typed(c->dev, c->subcomponent, mc->data))
 			return c;
 	}
 
@@ -192,7 +198,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;
@@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev,
 	return 0;
 }
 
-/**
- * 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
- *
- * Adds a new component match to the list stored in @matchptr, which the @master
- * aggregate driver needs to function. The list of component matches pointed to
- * by @matchptr must be initialized to NULL before adding the first match.
- *
- * The allocated match list in @matchptr is automatically released using devm
- * actions, where upon @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;
 
@@ -381,13 +370,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 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
+ *
+ * Adds a new component match to the list stored in @matchptr, which the @master
+ * aggregate driver needs to function. The list of component matches pointed to
+ * by @matchptr must be initialized to 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, where upon @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() 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
+ *
+ * Adds a new component match to the list stored in @matchptr, which the @master
+ * aggregate driver needs to function. The list of component matches pointed to
+ * by @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;
@@ -616,19 +661,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 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 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;
@@ -639,6 +673,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);
 
@@ -657,6 +692,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 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 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 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 83da25bdf59c..30bcc7e590eb 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,
  *
  * Adds a new component match to the list stored in @matchptr, which the @master
  * aggregate driver needs to function. The list of component matches pointed to
- * by @matchptr must be initialized to NULL before adding the first match.
+ * by @matchptr must be initialized to 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

[PATCH 3/4] drm/doc: document recommended component helper usage

[Daniel Vetter]

Now that component has docs it's worth spending a few words and
hyperlinks on recommended best practices in drm.

Cc: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 Documentation/driver-api/component.rst |  2 ++
 Documentation/gpu/drm-internals.rst    |  5 +++++
 drivers/gpu/drm/drm_drv.c              | 14 ++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst
index 2da4a8f20607..57e37590733f 100644
--- a/Documentation/driver-api/component.rst
+++ b/Documentation/driver-api/component.rst
@@ -1,3 +1,5 @@
+.. _component:
+
 ======================================
 Component Helper for Aggregate Drivers
 ======================================
diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index 3ae23a5454ac..966bd2d9f0cc 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -93,6 +93,11 @@ Device Instance and Driver Handling
 Driver Load
 -----------
 
+Component Helper Usage
+~~~~~~~~~~~~~~~~~~~~~~
+
+.. kernel-doc:: drivers/gpu/drm/drm_drv.c
+   :doc: component helper usage recommendations
 
 IRQ Helper Library
 ~~~~~~~~~~~~~~~~~~
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 381581b01d48..aae413003705 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -456,6 +456,20 @@ static void drm_fs_inode_free(struct inode *inode)
 	}
 }
 
+/**
+ * DOC: component helper usage recommendations
+ *
+ * DRM drivers that drive hardware where a logical device consists of a pile of
+ * independent hardware blocks are recommended to use the :ref:`component helper
+ * library<component>`. The entire device initialization procedure should be run
+ * from the &component_master_ops.master_bind callback, starting with
+ * drm_dev_init(), then binding all components with component_bind_all() and
+ * finishing with drm_dev_register(). For consistency and easier sharing of
+ * components across drivers the opaque pointer passed to all components through
+ * component_bind_all() should point at &struct drm_device of the device
+ * instance, not some driver specific private structure.
+ */
+
 /**
  * drm_dev_init - Initialise new DRM device
  * @dev: DRM device
-- 
2.20.1

[PATCH 4/4] i915/snd_hdac: I915 subcomponent for the snd_hdac

[Daniel Vetter]

Since we need multiple components for I915 for different purposes
(Audio & Mei_hdcp), we adopt the subcomponents methodology introduced
by the previous patch (mentioned below).

	Author: Daniel Vetter <daniel.vetter@ffwll.ch>
	Date:   Mon Jan 28 17:08:20 2019 +0530

	    components: multiple components for a device

Reviewed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by-by: Ramalingam C <ramalinagm.c@intel.com> (commit message)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> (code)
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>
---
 drivers/gpu/drm/i915/intel_audio.c | 4 +++-
 include/drm/i915_component.h       | 4 ++++
 include/sound/hda_component.h      | 5 +++--
 sound/hda/hdac_component.c         | 4 ++--
 sound/hda/hdac_i915.c              | 6 ++++--
 5 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index de26cd0a5497..5104c6bbd66f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -984,7 +984,9 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	ret = component_add(dev_priv->drm.dev, &i915_audio_component_bind_ops);
+	ret = component_add_typed(dev_priv->drm.dev,
+				  &i915_audio_component_bind_ops,
+				  I915_COMPONENT_AUDIO);
 	if (ret < 0) {
 		DRM_ERROR("failed to add audio component (%d)\n", ret);
 		/* continue with reduced functionality */
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index fca22d463e1b..72fbb037f9b3 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -26,6 +26,10 @@
 
 #include "drm_audio_component.h"
 
+enum i915_component_type {
+	I915_COMPONENT_AUDIO = 1,
+};
+
 /* MAX_PORT is the number of port
  * It must be sync with I915_MAX_PORTS defined i915_drv.h
  */
diff --git a/include/sound/hda_component.h b/include/sound/hda_component.h
index 2ec31b358950..d4804c72d959 100644
--- a/include/sound/hda_component.h
+++ b/include/sound/hda_component.h
@@ -20,7 +20,7 @@ int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t nid, int dev_id,
 			   bool *audio_enabled, char *buffer, int max_bytes);
 int snd_hdac_acomp_init(struct hdac_bus *bus,
 			const struct drm_audio_component_audio_ops *aops,
-			int (*match_master)(struct device *, void *),
+			int (*match_master)(struct device *, int, void *),
 			size_t extra_size);
 int snd_hdac_acomp_exit(struct hdac_bus *bus);
 int snd_hdac_acomp_register_notifier(struct hdac_bus *bus,
@@ -47,7 +47,8 @@ static inline int snd_hdac_acomp_get_eld(struct hdac_device *codec, hda_nid_t ni
 }
 static inline int snd_hdac_acomp_init(struct hdac_bus *bus,
 				      const struct drm_audio_component_audio_ops *aops,
-				      int (*match_master)(struct device *, void *),
+				      int (*match_master)(struct device *,
+							  int, void *),
 				      size_t extra_size)
 {
 	return -ENODEV;
diff --git a/sound/hda/hdac_component.c b/sound/hda/hdac_component.c
index a6d37b9d6413..5c95933e739a 100644
--- a/sound/hda/hdac_component.c
+++ b/sound/hda/hdac_component.c
@@ -269,7 +269,7 @@ EXPORT_SYMBOL_GPL(snd_hdac_acomp_register_notifier);
  */
 int snd_hdac_acomp_init(struct hdac_bus *bus,
 			const struct drm_audio_component_audio_ops *aops,
-			int (*match_master)(struct device *, void *),
+			int (*match_master)(struct device *, int, void *),
 			size_t extra_size)
 {
 	struct component_match *match = NULL;
@@ -288,7 +288,7 @@ int snd_hdac_acomp_init(struct hdac_bus *bus,
 	bus->audio_component = acomp;
 	devres_add(dev, acomp);
 
-	component_match_add(dev, &match, match_master, bus);
+	component_match_add_typed(dev, &match, match_master, bus);
 	ret = component_master_add_with_match(dev, &hdac_component_master_ops,
 					      match);
 	if (ret < 0)
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 617ff1aa818f..7aee090e3d27 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -82,9 +82,11 @@ void snd_hdac_i915_set_bclk(struct hdac_bus *bus)
 }
 EXPORT_SYMBOL_GPL(snd_hdac_i915_set_bclk);
 
-static int i915_component_master_match(struct device *dev, void *data)
+static int i915_component_master_match(struct device *dev, int subcomponent,
+				       void *data)
 {
-	return !strcmp(dev->driver->name, "i915");
+	return !strcmp(dev->driver->name, "i915") &&
+	       subcomponent == I915_COMPONENT_AUDIO;
 }
 
 /* check whether intel graphics is present */
-- 
2.20.1

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

[Rafael J. Wysocki]

On Fri, Feb 8, 2019 at 12:28 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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.
>
> v3: Rebase on top of updated documenation.
>
> v4: Review from Rafael:
> - Remove redundant "This" from kerneldoc (also in the previous patch)
> - Streamline the logic in find_component() a bit.
>
> 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>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/base/component.c  | 158 +++++++++++++++++++++++++++++---------
>  include/linux/component.h |  10 ++-
>  2 files changed, 129 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 1624c2a892a5..7dbc41cccd58 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -47,6 +47,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;
> @@ -74,6 +75,7 @@ struct component {
>         bool bound;
>
>         const struct component_ops *ops;
> +       int subcomponent;
>         struct device *dev;
>  };
>
> @@ -158,7 +160,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;
>
> @@ -166,7 +168,11 @@ static struct component *find_component(struct master *master,
>                 if (c->master && c->master != master)
>                         continue;
>
> -               if (compare(c->dev, compare_data))
> +               if (mc->compare && mc->compare(c->dev, mc->data))
> +                       return c;
> +
> +               if (mc->compare_typed &&
> +                   mc->compare_typed(c->dev, c->subcomponent, mc->data))
>                         return c;
>         }
>
> @@ -192,7 +198,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;
> @@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev,
>         return 0;
>  }
>
> -/**
> - * 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
> - *
> - * Adds a new component match to the list stored in @matchptr, which the @master
> - * aggregate driver needs to function. The list of component matches pointed to
> - * by @matchptr must be initialized to NULL before adding the first match.
> - *
> - * The allocated match list in @matchptr is automatically released using devm
> - * actions, where upon @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;
>
> @@ -381,13 +370,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 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
> + *
> + * Adds a new component match to the list stored in @matchptr, which the @master
> + * aggregate driver needs to function. The list of component matches pointed to
> + * by @matchptr must be initialized to 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, where upon @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() 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
> + *
> + * Adds a new component match to the list stored in @matchptr, which the @master
> + * aggregate driver needs to function. The list of component matches pointed to
> + * by @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;
> @@ -616,19 +661,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 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 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;
> @@ -639,6 +673,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);
>
> @@ -657,6 +692,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 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 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 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 83da25bdf59c..30bcc7e590eb 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,
>   *
>   * Adds a new component match to the list stored in @matchptr, which the @master
>   * aggregate driver needs to function. The list of component matches pointed to
> - * by @matchptr must be initialized to NULL before adding the first match.
> + * by @matchptr must be initialized to 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/4] components: multiple components for a device

[Russell King - ARM Linux admin]

On Fri, Feb 08, 2019 at 12:27:57AM +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.
> 
> v3: Rebase on top of updated documenation.
> 
> v4: Review from Rafael:
> - Remove redundant "This" from kerneldoc (also in the previous patch)
> - Streamline the logic in find_component() a bit.
> 
> 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  | 158 +++++++++++++++++++++++++++++---------
>  include/linux/component.h |  10 ++-
>  2 files changed, 129 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 1624c2a892a5..7dbc41cccd58 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -47,6 +47,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;
> @@ -74,6 +75,7 @@ struct component {
>  	bool bound;
>  
>  	const struct component_ops *ops;
> +	int subcomponent;
>  	struct device *dev;
>  };
>  
> @@ -158,7 +160,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;
>  
> @@ -166,7 +168,11 @@ static struct component *find_component(struct master *master,
>  		if (c->master && c->master != master)
>  			continue;
>  
> -		if (compare(c->dev, compare_data))
> +		if (mc->compare && mc->compare(c->dev, mc->data))
> +			return c;
> +
> +		if (mc->compare_typed &&
> +		    mc->compare_typed(c->dev, c->subcomponent, mc->data))
>  			return c;
>  	}
>  
> @@ -192,7 +198,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;
> @@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev,
>  	return 0;
>  }
>  
> -/**
> - * 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
> - *
> - * Adds a new component match to the list stored in @matchptr, which the @master
> - * aggregate driver needs to function. The list of component matches pointed to
> - * by @matchptr must be initialized to NULL before adding the first match.
> - *
> - * The allocated match list in @matchptr is automatically released using devm
> - * actions, where upon @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;
>  
> @@ -381,13 +370,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 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
> + *
> + * Adds a new component match to the list stored in @matchptr, which the @master
> + * aggregate driver needs to function. The list of component matches pointed to
> + * by @matchptr must be initialized to 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, where upon @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() 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
> + *
> + * Adds a new component match to the list stored in @matchptr, which the @master
> + * aggregate driver needs to function. The list of component matches pointed to
> + * by @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);

You're still passing some compare data in that may need to be released,
but you are explicitly denying the ability to provide a release
function pointer here.  What was the reasoning behind that decision?
If someone wanted to use this with compare_data being a device_node,
how should that be handled?

Thanks.

> +}
> +EXPORT_SYMBOL(component_match_add_typed);
> +
>  static void free_master(struct master *master)
>  {
>  	struct component_match *match = master->match;
> @@ -616,19 +661,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 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 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;
> @@ -639,6 +673,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);
>  
> @@ -657,6 +692,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 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 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 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 83da25bdf59c..30bcc7e590eb 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,
>   *
>   * Adds a new component match to the list stored in @matchptr, which the @master
>   * aggregate driver needs to function. The list of component matches pointed to
> - * by @matchptr must be initialized to NULL before adding the first match.
> + * by @matchptr must be initialized to 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
> 
> 

-- 
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 1/4] component: Add documentation

[Greg Kroah-Hartman]

On Fri, Feb 08, 2019 at 12:27:56AM +0100, Daniel Vetter wrote:
> 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.
> 
> v3: Review from Russell:
> - s/framework/helper
> - clarify the documentation for component_match_add functions.
> 
> v4: Remove a few superflous "This".
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 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>

Thanks for doing this!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Greg Kroah-Hartman]

On Fri, Feb 08, 2019 at 12:27:57AM +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.
> 
> v3: Rebase on top of updated documenation.
> 
> v4: Review from Rafael:
> - Remove redundant "This" from kerneldoc (also in the previous patch)
> - Streamline the logic in find_component() a bit.
> 
> 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>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

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

[Daniel Vetter]

On Fri, Feb 08, 2019 at 10:28:49AM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Feb 08, 2019 at 12:27:57AM +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.
> > 
> > v3: Rebase on top of updated documenation.
> > 
> > v4: Review from Rafael:
> > - Remove redundant "This" from kerneldoc (also in the previous patch)
> > - Streamline the logic in find_component() a bit.
> > 
> > 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  | 158 +++++++++++++++++++++++++++++---------
> >  include/linux/component.h |  10 ++-
> >  2 files changed, 129 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/base/component.c b/drivers/base/component.c
> > index 1624c2a892a5..7dbc41cccd58 100644
> > --- a/drivers/base/component.c
> > +++ b/drivers/base/component.c
> > @@ -47,6 +47,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;
> > @@ -74,6 +75,7 @@ struct component {
> >  	bool bound;
> >  
> >  	const struct component_ops *ops;
> > +	int subcomponent;
> >  	struct device *dev;
> >  };
> >  
> > @@ -158,7 +160,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;
> >  
> > @@ -166,7 +168,11 @@ static struct component *find_component(struct master *master,
> >  		if (c->master && c->master != master)
> >  			continue;
> >  
> > -		if (compare(c->dev, compare_data))
> > +		if (mc->compare && mc->compare(c->dev, mc->data))
> > +			return c;
> > +
> > +		if (mc->compare_typed &&
> > +		    mc->compare_typed(c->dev, c->subcomponent, mc->data))
> >  			return c;
> >  	}
> >  
> > @@ -192,7 +198,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;
> > @@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev,
> >  	return 0;
> >  }
> >  
> > -/**
> > - * 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
> > - *
> > - * Adds a new component match to the list stored in @matchptr, which the @master
> > - * aggregate driver needs to function. The list of component matches pointed to
> > - * by @matchptr must be initialized to NULL before adding the first match.
> > - *
> > - * The allocated match list in @matchptr is automatically released using devm
> > - * actions, where upon @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;
> >  
> > @@ -381,13 +370,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 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
> > + *
> > + * Adds a new component match to the list stored in @matchptr, which the @master
> > + * aggregate driver needs to function. The list of component matches pointed to
> > + * by @matchptr must be initialized to 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, where upon @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() 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
> > + *
> > + * Adds a new component match to the list stored in @matchptr, which the @master
> > + * aggregate driver needs to function. The list of component matches pointed to
> > + * by @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);
> 
> You're still passing some compare data in that may need to be released,
> but you are explicitly denying the ability to provide a release
> function pointer here.  What was the reasoning behind that decision?
> If someone wanted to use this with compare_data being a device_node,
> how should that be handled?

Same way we've handled it originally, adding the new variant. I'm not a
huge fan of adding api preemptively, without a user. And the data pointer
we pass around in i915 for mei_hdcp and snd-hda-intel don't hold
references (we don't even use them, we just compare the device against
hardcoded expectations). Since the new compare_typed is entirely
orthogonal to whether you have a release function or not, that should be
simple to do.
-Daniel

> 
> Thanks.
> 
> > +}
> > +EXPORT_SYMBOL(component_match_add_typed);
> > +
> >  static void free_master(struct master *master)
> >  {
> >  	struct component_match *match = master->match;
> > @@ -616,19 +661,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 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 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;
> > @@ -639,6 +673,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);
> >  
> > @@ -657,6 +692,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 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 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 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 83da25bdf59c..30bcc7e590eb 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,
> >   *
> >   * Adds a new component match to the list stored in @matchptr, which the @master
> >   * aggregate driver needs to function. The list of component matches pointed to
> > - * by @matchptr must be initialized to NULL before adding the first match.
> > + * by @matchptr must be initialized to 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
> > 
> > 
> 
> -- 
> 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

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

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

[Daniel Vetter]

On Fri, Feb 08, 2019 at 12:51:23PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 08, 2019 at 12:27:56AM +0100, Daniel Vetter wrote:
> > 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.
> > 
> > v3: Review from Russell:
> > - s/framework/helper
> > - clarify the documentation for component_match_add functions.
> > 
> > v4: Remove a few superflous "This".
> > 
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 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>
> 
> Thanks for doing this!
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing (obviously includes Rafael and Russell too)!

For merging I see two options:
- All through drm-intel, but will only land in 5.2 because the feature
  cut-off has already happened for i915. So a bit awkward.
- Topic branch with the 2 component patches plus the snd-hda/drm-i915
  patch, so that you/Takashi/drm can pull it in as needed. The drm
  component doc patch (3/4 in this series) I can pull in later on.

Greg/Takashi, any preferences?

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

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

[Daniel Vetter]

On Fri, Feb 8, 2019 at 4:23 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Feb 08, 2019 at 10:28:49AM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 08, 2019 at 12:27:57AM +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.
> > >
> > > v3: Rebase on top of updated documenation.
> > >
> > > v4: Review from Rafael:
> > > - Remove redundant "This" from kerneldoc (also in the previous patch)
> > > - Streamline the logic in find_component() a bit.
> > >
> > > 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  | 158 +++++++++++++++++++++++++++++---------
> > >  include/linux/component.h |  10 ++-
> > >  2 files changed, 129 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/drivers/base/component.c b/drivers/base/component.c
> > > index 1624c2a892a5..7dbc41cccd58 100644
> > > --- a/drivers/base/component.c
> > > +++ b/drivers/base/component.c
> > > @@ -47,6 +47,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;
> > > @@ -74,6 +75,7 @@ struct component {
> > >     bool bound;
> > >
> > >     const struct component_ops *ops;
> > > +   int subcomponent;
> > >     struct device *dev;
> > >  };
> > >
> > > @@ -158,7 +160,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;
> > >
> > > @@ -166,7 +168,11 @@ static struct component *find_component(struct master *master,
> > >             if (c->master && c->master != master)
> > >                     continue;
> > >
> > > -           if (compare(c->dev, compare_data))
> > > +           if (mc->compare && mc->compare(c->dev, mc->data))
> > > +                   return c;
> > > +
> > > +           if (mc->compare_typed &&
> > > +               mc->compare_typed(c->dev, c->subcomponent, mc->data))
> > >                     return c;
> > >     }
> > >
> > > @@ -192,7 +198,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;
> > > @@ -327,29 +333,12 @@ static int component_match_realloc(struct device *dev,
> > >     return 0;
> > >  }
> > >
> > > -/**
> > > - * 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
> > > - *
> > > - * Adds a new component match to the list stored in @matchptr, which the @master
> > > - * aggregate driver needs to function. The list of component matches pointed to
> > > - * by @matchptr must be initialized to NULL before adding the first match.
> > > - *
> > > - * The allocated match list in @matchptr is automatically released using devm
> > > - * actions, where upon @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;
> > >
> > > @@ -381,13 +370,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 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
> > > + *
> > > + * Adds a new component match to the list stored in @matchptr, which the @master
> > > + * aggregate driver needs to function. The list of component matches pointed to
> > > + * by @matchptr must be initialized to 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, where upon @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() 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
> > > + *
> > > + * Adds a new component match to the list stored in @matchptr, which the @master
> > > + * aggregate driver needs to function. The list of component matches pointed to
> > > + * by @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);
> >
> > You're still passing some compare data in that may need to be released,
> > but you are explicitly denying the ability to provide a release
> > function pointer here.  What was the reasoning behind that decision?
> > If someone wanted to use this with compare_data being a device_node,
> > how should that be handled?
>
> Same way we've handled it originally, adding the new variant. I'm not a
> huge fan of adding api preemptively, without a user. And the data pointer
> we pass around in i915 for mei_hdcp and snd-hda-intel don't hold
> references (we don't even use them, we just compare the device against
> hardcoded expectations). Since the new compare_typed is entirely
> orthogonal to whether you have a release function or not, that should be
> simple to do.

Forgot to add: I also don't expect a user for this. The ->release
callback is used for dt drivers, because of_node_put(). And well
designed device tree will not have multiple components stack in very
awkward ways on the exact same underlying device, that would simply be
rather unclean DT design. x86 otoh, thanks to window's "one pci
device, one driver" expectation is full of that kind of
kludged-together SoC-but-not-really behind one pci device. Therefore I
expect the overlap for this kind of stuff to be 0.

This includes all the hand-rolled hacks we have in i915 that predate
component, e.g. ips on ironlake or the pmu serialization on vlv. So
with a 4/4 rate of possible users of component_typed not needed a
->release callback I think not providing that combo for now is the
right call. And as mentioned, really easy to adjust in case I've
guessed wrong here.
-Daniel

> > > +}
> > > +EXPORT_SYMBOL(component_match_add_typed);
> > > +
> > >  static void free_master(struct master *master)
> > >  {
> > >     struct component_match *match = master->match;
> > > @@ -616,19 +661,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 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 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;
> > > @@ -639,6 +673,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);
> > >
> > > @@ -657,6 +692,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 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 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 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 83da25bdf59c..30bcc7e590eb 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,
> > >   *
> > >   * Adds a new component match to the list stored in @matchptr, which the @master
> > >   * aggregate driver needs to function. The list of component matches pointed to
> > > - * by @matchptr must be initialized to NULL before adding the first match.
> > > + * by @matchptr must be initialized to 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
> > >
> > >
> >
> > --
> > 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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Fri, Feb 8, 2019 at 4:29 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Feb 08, 2019 at 12:51:23PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Feb 08, 2019 at 12:27:56AM +0100, Daniel Vetter wrote:
> > > 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.
> > >
> > > v3: Review from Russell:
> > > - s/framework/helper
> > > - clarify the documentation for component_match_add functions.
> > >
> > > v4: Remove a few superflous "This".
> > >
> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 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>
> >
> > Thanks for doing this!
> >
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Thanks for reviewing (obviously includes Rafael and Russell too)!
>
> For merging I see two options:
> - All through drm-intel, but will only land in 5.2 because the feature
>   cut-off has already happened for i915. So a bit awkward.
> - Topic branch with the 2 component patches plus the snd-hda/drm-i915
>   patch, so that you/Takashi/drm can pull it in as needed. The drm
>   component doc patch (3/4 in this series) I can pull in later on.
>
> Greg/Takashi, any preferences?

Just realized that for the mei side of this series I need a topic
branch anyway, so topic branch it is. I'll prep that so that CI can
beat on it over the w/e, and we can rebase the i915/mei series on top,
and then send out the topic pull to everyone early next week. You can
then all decide whether you need it or not.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Randy Dunlap]

On 2/7/19 3:27 PM, Daniel Vetter wrote:

Hi Daniel,

I have a few possible changes for this documentation (see below).

> ---
>  Documentation/driver-api/component.rst   |  17 ++++
>  Documentation/driver-api/device_link.rst |   3 +
>  Documentation/driver-api/index.rst       |   1 +
>  drivers/base/component.c                 | 106 ++++++++++++++++++++++-
>  include/linux/component.h                |  70 +++++++++++++++
>  5 files changed, 194 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/driver-api/component.rst
> 



> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index ddcea8739c12..1624c2a892a5 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -16,6 +16,32 @@
>  #include <linux/slab.h>
>  #include <linux/debugfs.h>
>  
> +/**
> + * DOC: overview
> + *
> + * The component helper 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 helper can be used when such a
> + * subsystem-specific way to find a device is not available: The component
> + * helper 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

              on various components

> + * for various outputs and so on).
> + *
> + * The component helper 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 +327,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
> + *
> + * Adds a new component match to the list stored in @matchptr, which the @master
> + * aggregate driver needs to function. The list of component matches pointed to
> + * by @matchptr must be initialized to NULL before adding the first match.
> + *
> + * The allocated match list in @matchptr is automatically released using devm
> + * actions, where upon @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 +407,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 +455,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 +491,15 @@ static void component_unbind(struct component *component,
>  	devres_release_group(component->dev, component);
>  }
>  
> +/**
> + * component_unbind_all - unbind all component to an aggregate driver

                                        components {of | from} 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

                             {of | from}

> + * &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 +573,15 @@ static int component_bind(struct component *component, struct master *master,
>  	return ret;
>  }
>  
> +/**
> + * component_bind_all - bind all component to an aggregate driver

                                    components

> + * @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 +616,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 at driver unload/disconnect by calling
> + * component_del().
> + */
>  int component_add(struct device *dev, const struct component_ops *ops)
>  {
>  	struct component *component;
> @@ -568,6 +659,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..83da25bdf59c 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

                                  component match entry (or item)
?

> + * @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. The list of component matches pointed to
> + * by @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)
> 


cheers.
-- 
~Randy