Someone owes me a beer ...

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

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

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

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

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst index d6763272e747..2d5919b2b337 100644 --- a/Documentation/driver-api/device_link.rst +++ b/Documentation/driver-api/device_link.rst @@ -1,6 +1,9 @@ .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>` .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`

+ +.. _device_link: + ============ Device links ============ diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index ab38ced66a44..c0b600ed9961 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -22,6 +22,7 @@ available subsections can be seen below. device_connection dma-buf device_link + component message-based sound frame-buffer diff --git a/drivers/base/component.c b/drivers/base/component.c index ddcea8739c12..e5b04bce8544 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -16,6 +16,33 @@ #include <linux/slab.h> #include <linux/debugfs.h>

+/** + * DOC: overview + * + * The component frameworks allows drivers to collect a pile of sub-devices, + * including their bound drivers, into an aggregate driver. Various subsystem + * already provide functions to get hold of such components, e.g. + * of_clk_get_by_name(). Anytime there's such a subsystem specific way to find a + * a device the component framework should not be used. The component framework + * fills the niche of aggregate drivers for specific hardware, where further + * standardization into a subsystem doesn't make sense. The common example is + * when a logical device (e.g. a DRM display driver) is spread around the SoC on + * various component (scanout engines, blending blocks, transcoders for various + * outputs and so on). + * + * The component framework also doesn't solve runtime dependencies, e.g. for + * system suspend and resume operations. See also :ref:`device + * links<device_link>`. + * + * Components are registered using component_add() and unregistered with + * component_del(), usually from the driver's probe and disconnect functions. + * + * Aggregate drivers first assemble a component match list of what they need + * using component_match_add(). This is then registered as an aggregate driver + * using component_master_add_with_match(), and unregistered using + * component_master_del(). + */ + struct component;

struct component_match_array { @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev, return 0; }

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

+/** + * component_master_add_with_match - register an aggregate driver + * @dev: device with the aggregate driver + * @ops: callbacks for the aggregate driver + * @match: component match list for the aggregate driver + * + * Registers a new aggregate driver consisting of the components added to @match + * by calling one of the component_match_add() functions. Once all components in + * @match are available it will be assembled by calling + * &component_master_ops.bind from @ops. Must be unregistered by calling + * component_master_del(). + */ int component_master_add_with_match(struct device *dev, const struct component_master_ops *ops, struct component_match *match) @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev, } EXPORT_SYMBOL_GPL(component_master_add_with_match);

+/** + * component_master_del - unregister an aggregate driver + * @dev: device with the aggregate driver + * @ops: callbacks for the aggregate driver + * + * Unregistered an aggregate driver registered with + * component_master_add_with_match(). If necessary the aggregate driver is first + * disassembled by calling &component_master_ops.unbind from @ops. + */ void component_master_del(struct device *dev, const struct component_master_ops *ops) { @@ -430,6 +492,15 @@ static void component_unbind(struct component *component, devres_release_group(component->dev, component); }

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

+/** + * component_bind_all - bind all component to an aggregate driver + * @master_dev: device with the aggregate driver + * @data: opaque pointer, passed to all components + * + * This binds all components to the aggregate @dev by passing @data to their + * &component_ops.bind functions. Should be called from + * &component_master_ops.bind. + */ int component_bind_all(struct device *master_dev, void *data) { struct master *master; @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data) } EXPORT_SYMBOL_GPL(component_bind_all);

+/** + * component_add - register a component + * @dev: component device + * @ops: component callbacks + * + * Register a new component for @dev. Functions in @ops will be call when the + * aggregate driver is ready to bind the overall driver by calling + * component_bind_all(). See also &struct component_ops. + * + * The component needs to be unregistered again at driver unload/disconnect by + * calling component_del(). + */ int component_add(struct device *dev, const struct component_ops *ops) { struct component *component; @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops) } EXPORT_SYMBOL_GPL(component_add);

+/** + * component_del - unregister a component + * @dev: component device + * @ops: component callbacks + * + * Unregister a component added with component_add(). If the component is bound + * into an aggregate driver this will force the entire aggrate driver, including + * all its components, to be unbound. + */ void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; diff --git a/include/linux/component.h b/include/linux/component.h index e71fbbbc74e2..67a899dd2e10 100644 --- a/include/linux/component.h +++ b/include/linux/component.h @@ -4,11 +4,31 @@

#include <linux/stddef.h>

+ struct device;

+/** + * struct component_ops - callbacks for component drivers + * + * Components are registered with component_add() and unregistered with + * component_del(). + */ struct component_ops { + /** + * @bind: + * + * Called through component_bind_all() when the aggregate driver is + * ready to bind the overall driver. + */ int (*bind)(struct device *comp, struct device *master, void *master_data); + /** + * @unbind: + * + * Called through component_unbind_all() when the aggregate driver is + * ready to bind the overall driver, or when component_bind_all() fails + * part-ways through and needs to unbind some already bound components. + */ void (*unbind)(struct device *comp, struct device *master, void *master_data); }; @@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);

struct master;

+/** + * struct component_master_ops - callback for the aggregate driver + * + * Aggregate drivers are registered with component_master_add_with_match() and + * unregistered with component_master_del(). + */ struct component_master_ops { + /** + * @bind: + * + * Called when all components or the aggregate driver, as specified in + * the match list passed to component_master_add_with_match(), are + * ready. Usually there are 3 steps to bind an aggregate driver: + * + * 1. Allocate a structure for the aggregate driver. + * + * 2. Bind all components to the aggregate driver by calling + * component_bind_all() with the aggregate driver structure as opaque + * pointer data. + * + * 3. Register the aggregate driver with the subsystem to publish its + * interfaces. + * + * Note that the lifetime of the aggregate driver does not align with + * any of the underlying &struct device instances. Therefore devm cannot + * be used and all resources acquired or allocated in this callback must + * be expecitly released in the @unbind callback. + */ int (*bind)(struct device *master); + /** + * @unbind: + * + * Called when either the aggregate driver, using + * component_master_del(), or one of its components, using + * component_del(), is unregistered. + */ void (*unbind)(struct device *master); };

@@ -38,6 +92,22 @@ void component_match_add_release(struct device *master, void (*release)(struct device *, void *), int (*compare)(struct device *, void *), void *compare_data);

+/** + * component_match_add - add a compent match + * @master: device with the aggregate driver + * @matchptr: pointer to the list of component matches + * @compare: compare function to match against all components + * @compare_data: opaque pointer passed to the @compare function + * + * This adds a new component match to the list stored in @matchptr, which the + * @master aggregate driver needs to function. @matchptr must be initialized to + * NULL before adding the first match. + * + * The allocated match list in @matchptr is automatically released using devm + * actions. + * + * See also component_match_add_release(). + */ static inline void component_match_add(struct device *master, struct component_match **matchptr, int (*compare)(struct device *, void *), void *compare_data)

On Thu, Jan 31, 2019 at 03:46:40PM +0100, Daniel Vetter wrote:

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

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

v2: Add documenation.

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch (v1 code) Signed-off-by: Ramalingam C ramalingam.c@intel.com (v1 commit message) Cc: Ramalingam C ramalingam.c@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Russell King rmk+kernel@arm.linux.org.uk Cc: Rafael J. Wysocki rafael@kernel.org Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

Hi Greg&Russell,

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

Thanks, Daniel

---

drivers/base/component.c | 159 +++++++++++++++++++++++++++++--------- include/linux/component.h | 10 ++- 2 files changed, 130 insertions(+), 39 deletions(-)

diff --git a/drivers/base/component.c b/drivers/base/component.c index e5b04bce8544..eb7915fc5278 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -48,6 +48,7 @@ struct component; struct component_match_array { void *data; int (*compare)(struct device *, void *);

• int (*compare_typed)(struct device *, int, void *); void (*release)(struct device *, void *); struct component *component; bool duplicate;

@@ -75,6 +76,7 @@ struct component { bool bound;

const struct component_ops *ops;

• int subcomponent; struct device *dev;

};

@@ -159,7 +161,7 @@ static struct master *__master_find(struct device *dev, }

static struct component *find_component(struct master *master,

• int (*compare)(struct device *, void *), void *compare_data)

• struct component_match_array *mc)

{ struct component *c;

@@ -167,8 +169,13 @@ static struct component *find_component(struct master *master, if (c->master && c->master != master) continue;

• if (compare(c->dev, compare_data))
  

• if (mc->compare_typed) {
  

• 	if (mc->compare_typed(c->dev, c->subcomponent,
  

• 			      mc->data))
  

• 		return c;
  

• } else if (mc->compare(c->dev, mc->data)) {
  return c;
  

• }
  

  }

  return NULL;

@@ -193,7 +200,7 @@ static int find_components(struct master *master) if (match->compare[i].component) continue;

• c = find_component(master, mc->compare, mc->data);
  

• c = find_component(master, mc);
  

  if (!c) { ret = -ENXIO; break;

@@ -328,29 +335,12 @@ static int component_match_realloc(struct device *dev, return 0; }

-/**

• • component_match_add_release - add a compent match with release callback
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @release: release function for @compare_data
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match.
• • The allocated match list in @matchptr is automatically released using devm
• • actions. At that point @release will be called, to free any references held
• • by @compare_data, e.g. when @compare_data is a &device_node that must be
• • released with of_node_put().
• • See also component_match_add().
• */

-void component_match_add_release(struct device *master, +static void __component_match_add(struct device *master, struct component_match **matchptr, void (*release)(struct device *, void *),

• int (*compare)(struct device *, void *), void *compare_data)

• int (*compare)(struct device *, void *),
• int (*compare_typed)(struct device *, int, void *),
• void *compare_data)

{ struct component_match *match = *matchptr;

@@ -382,13 +372,69 @@ void component_match_add_release(struct device *master, }

match->compare[match->num].compare = compare;

• match->compare[match->num].compare_typed = compare_typed; match->compare[match->num].release = release; match->compare[match->num].data = compare_data; match->compare[match->num].component = NULL; match->num++;

}

+/**

• • component_match_add_release - add a compent match with release callback
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @release: release function for @compare_data
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match.
• • The allocated match list in @matchptr is automatically released using devm
• • actions. At that point @release will be called, to free any references held
• • by @compare_data, e.g. when @compare_data is a &device_node that must be
• • released with of_node_put(). This only matches against components
• • added with component_add().
• • See also component_match_add() and component_match_add_typed().
• */

+void component_match_add_release(struct device *master,

• struct component_match **matchptr,
• void (*release)(struct device *, void *),
• int (*compare)(struct device *, void *), void *compare_data)

+{

• __component_match_add(master, matchptr, release, compare, NULL,

• 	      compare_data);
  

+} EXPORT_SYMBOL(component_match_add_release);

+/**

• • component_match_add_typed - add a compent match for a typed component
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @compare_typed: compare function to match against all typed components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match. This only matches against components
• • added with component_add_typed().
• • The allocated match list in @matchptr is automatically released using devm
• • actions.
• • See also component_match_add_release() and component_match_add_typed().
• */

+void component_match_add_typed(struct device *master,

• struct component_match **matchptr,
• int (*compare_typed)(struct device *, int, void *), void *compare_data)

+{

• __component_match_add(master, matchptr, NULL, NULL, compare_typed,

• 	      compare_data);
  

+} +EXPORT_SYMBOL(component_match_add_typed);

static void free_master(struct master *master) { struct component_match *match = master->match; @@ -617,19 +663,8 @@ int component_bind_all(struct device *master_dev, void *data) } EXPORT_SYMBOL_GPL(component_bind_all);

-/**

• • component_add - register a component
• • @dev: component device
• • @ops: component callbacks
• • Register a new component for @dev. Functions in @ops will be call when the
• • aggregate driver is ready to bind the overall driver by calling
• • component_bind_all(). See also &struct component_ops.
• • The component needs to be unregistered again at driver unload/disconnect by
• • calling component_del().
• */

-int component_add(struct device *dev, const struct component_ops *ops) +static int __component_add(struct device *dev, const struct component_ops *ops,

• int subcomponent)

{ struct component *component; int ret; @@ -640,6 +675,7 @@ int component_add(struct device *dev, const struct component_ops *ops)

component->ops = ops; component->dev = dev;

• component->subcomponent = subcomponent;

  dev_dbg(dev, "adding component (ops %ps)\n", ops);

@@ -658,6 +694,55 @@ int component_add(struct device *dev, const struct component_ops *ops)

return ret < 0 ? ret : 0; }

+/**

• • component_add_typed - register a component
• • @dev: component device
• • @ops: component callbacks
• • @subcomponent: nonzero identifier for subcomponents
• • Register a new component for @dev. Functions in @ops will be call when the
• • aggregate driver is ready to bind the overall driver by calling
• • component_bind_all(). See also &struct component_ops.
• • @subcomponent must be nonzero and is used to differentiate between multiple
• • components registerd on the same device @dev. These components are match
• • using component_match_add_typed().
• • The component needs to be unregistered again at driver unload/disconnect by
• • calling component_del().
• • See also component_add().
• */

+int component_add_typed(struct device *dev, const struct component_ops *ops,

• int subcomponent)

+{

• if (WARN_ON(subcomponent == 0))

• return -EINVAL;
  

• return __component_add(dev, ops, subcomponent);

+} +EXPORT_SYMBOL_GPL(component_add_typed);

+/**

• • component_add - register a component
• • @dev: component device
• • @ops: component callbacks
• • Register a new component for @dev. Functions in @ops will be call when the
• • aggregate driver is ready to bind the overall driver by calling
• • component_bind_all(). See also &struct component_ops.
• • The component needs to be unregistered again at driver unload/disconnect by
• • calling component_del().
• • See also component_add_typed() for a variant that allows multipled different
• • components on the same device.
• */

+int component_add(struct device *dev, const struct component_ops *ops) +{

• return __component_add(dev, ops, 0);

+} EXPORT_SYMBOL_GPL(component_add);

/** diff --git a/include/linux/component.h b/include/linux/component.h index 67a899dd2e10..9e69e2117f0b 100644 --- a/include/linux/component.h +++ b/include/linux/component.h @@ -34,6 +34,8 @@ struct component_ops { };

int component_add(struct device *, const struct component_ops *); +int component_add_typed(struct device *dev, const struct component_ops *ops,

• int subcomponent);

void component_del(struct device *, const struct component_ops *);

int component_bind_all(struct device *master, void *master_data); @@ -91,6 +93,9 @@ void component_match_add_release(struct device *master, struct component_match **matchptr, void (*release)(struct device *, void *), int (*compare)(struct device *, void *), void *compare_data); +void component_match_add_typed(struct device *master,

• struct component_match **matchptr,
• int (*compare_typed)(struct device *, int, void *), void *compare_data);

/**

• component_match_add - add a compent match

@@ -101,12 +106,13 @@ void component_match_add_release(struct device *master,

• This adds a new component match to the list stored in @matchptr, which the
• @master aggregate driver needs to function. @matchptr must be initialized to

• • NULL before adding the first match.

• • NULL before adding the first match. This only matches against components
• • added with component_add().
  • The allocated match list in @matchptr is automatically released using devm
  • actions.

• • See also component_match_add_release().

• • See also component_match_add_release() and component_match_add_typed().
  */

static inline void component_match_add(struct device *master, struct component_match **matchptr, -- 2.20.1

On Tue, Feb 05, 2019 at 11:47:58AM +0100, Rafael J. Wysocki wrote:

w/compOn Thu, Jan 31, 2019 at 3:46 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Someone owes me a beer ...

While typing these I think doing an s/component_master/aggregate/ would be useful:

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

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

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

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

...

Cc: "C, Ramalingam" ramalingam.c@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Russell King rmk+kernel@arm.linux.org.uk Cc: Rafael J. Wysocki rafael@kernel.org Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

Documentation/driver-api/device_link.rst | 3 + Documentation/driver-api/index.rst | 1 + drivers/base/component.c | 107 ++++++++++++++++++++++- include/linux/component.h | 70 +++++++++++++++ 4 files changed, 178 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst index d6763272e747..2d5919b2b337 100644 --- a/Documentation/driver-api/device_link.rst +++ b/Documentation/driver-api/device_link.rst @@ -1,6 +1,9 @@ .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>` .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`

+.. _device_link:

============ Device links ============ diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index ab38ced66a44..c0b600ed9961 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -22,6 +22,7 @@ available subsections can be seen below. device_connection dma-buf device_link

• component

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

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

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

Thanks, Daniel

...

message-based
sound
frame-buffer

diff --git a/drivers/base/component.c b/drivers/base/component.c index ddcea8739c12..e5b04bce8544 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -16,6 +16,33 @@ #include <linux/slab.h> #include <linux/debugfs.h>

+/**

• • DOC: overview
• • The component frameworks allows drivers to collect a pile of sub-devices,

s/frameworks/framework/

...

• • including their bound drivers, into an aggregate driver. Various subsystem

s/subsystem/subsystems/

...

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

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

...

The component framework

• • fills the niche of aggregate drivers for specific hardware, where further
• • standardization into a subsystem doesn't make sense.

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

...

The common example is

• • when a logical device (e.g. a DRM display driver) is spread around the SoC on
• • various component (scanout engines, blending blocks, transcoders for various
• • outputs and so on).
• • The component framework also doesn't solve runtime dependencies, e.g. for
• • system suspend and resume operations. See also :ref:`device
• • links<device_link>`.
• • Components are registered using component_add() and unregistered with
• • component_del(), usually from the driver's probe and disconnect functions.
• • Aggregate drivers first assemble a component match list of what they need
• • using component_match_add(). This is then registered as an aggregate driver
• • using component_master_add_with_match(), and unregistered using
• • component_master_del().
• */

struct component;

struct component_match_array { @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev, return 0; }

-/*

• • Add a component to be matched, with a release function.

+/**

• • component_match_add_release - add a compent match with release callback

s/compent/component/ ?

...

• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @release: release function for @compare_data
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match.
• • The allocated match list in @matchptr is automatically released using devm
• • actions. At that point @release will be called, to free any references held
• • by @compare_data, e.g. when @compare_data is a &device_node that must be
• • released with of_node_put().

• • The match array is first created or extended if necessary.

• • See also component_match_add().
  */

void component_match_add_release(struct device *master, struct component_match **matchptr, @@ -367,6 +408,18 @@ static void free_master(struct master *master) kfree(master); }

+/**

• • component_master_add_with_match - register an aggregate driver
• • @dev: device with the aggregate driver
• • @ops: callbacks for the aggregate driver
• • @match: component match list for the aggregate driver
• • Registers a new aggregate driver consisting of the components added to @match
• • by calling one of the component_match_add() functions. Once all components in
• • @match are available it will be assembled by calling

A comma seems to be missing after "available".

...

• • &component_master_ops.bind from @ops. Must be unregistered by calling
• • component_master_del().
• */

int component_master_add_with_match(struct device *dev, const struct component_master_ops *ops, struct component_match *match) @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev, } EXPORT_SYMBOL_GPL(component_master_add_with_match);

+/**

• • component_master_del - unregister an aggregate driver
• • @dev: device with the aggregate driver
• • @ops: callbacks for the aggregate driver
• • Unregistered an aggregate driver registered with

s/Unregistered/Unregisters/ ?

...

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

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

...

• */

void component_master_del(struct device *dev, const struct component_master_ops *ops) { @@ -430,6 +492,15 @@ static void component_unbind(struct component *component, devres_release_group(component->dev, component); }

+/**

• • component_unbind_all - unbind all component to an aggregate driver
• • @master_dev: device with the aggregate driver
• • @data: opaque pointer, passed to all components
• • This unbinds all components to the aggregate @dev by passing @data to their

I guess "This" is redundant.

...

• • &component_ops.unbind functions. Should be called from
• • &component_master_ops.unbind.
• */

void component_unbind_all(struct device *master_dev, void *data) { struct master *master; @@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master, return ret; }

+/**

• • component_bind_all - bind all component to an aggregate driver
• • @master_dev: device with the aggregate driver
• • @data: opaque pointer, passed to all components
• • This binds all components to the aggregate @dev by passing @data to their

Likewise.

...

• • &component_ops.bind functions. Should be called from
• • &component_master_ops.bind.
• */

int component_bind_all(struct device *master_dev, void *data) { struct master *master; @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data) } EXPORT_SYMBOL_GPL(component_bind_all);

+/**

• • component_add - register a component
• • @dev: component device
• • @ops: component callbacks
• • Register a new component for @dev. Functions in @ops will be call when the

s/call/called/

...

• • aggregate driver is ready to bind the overall driver by calling
• • component_bind_all(). See also &struct component_ops.
• • The component needs to be unregistered again at driver unload/disconnect by
• • calling component_del().
• */

int component_add(struct device *dev, const struct component_ops *ops) { struct component *component; @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops) } EXPORT_SYMBOL_GPL(component_add);

+/**

• • component_del - unregister a component
• • @dev: component device
• • @ops: component callbacks
• • Unregister a component added with component_add(). If the component is bound
• • into an aggregate driver this will force the entire aggrate driver, including

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

...

• • all its components, to be unbound.
• */

void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; diff --git a/include/linux/component.h b/include/linux/component.h index e71fbbbc74e2..67a899dd2e10 100644 --- a/include/linux/component.h +++ b/include/linux/component.h @@ -4,11 +4,31 @@

#include <linux/stddef.h>

struct device;

+/**

• • struct component_ops - callbacks for component drivers
• • Components are registered with component_add() and unregistered with
• • component_del().
• */

struct component_ops {

•   /**
  

•    * @bind:
  

•    *
  

•    * Called through component_bind_all() when the aggregate driver is
  

•    * ready to bind the overall driver.
  

•    */
    int (*bind)(struct device *comp, struct device *master,
                void *master_data);
  

•   /**
  

•    * @unbind:
  

•    *
  

•    * Called through component_unbind_all() when the aggregate driver is
  

•    * ready to bind the overall driver, or when component_bind_all() fails
  

•    * part-ways through and needs to unbind some already bound components.
  

•    */
    void (*unbind)(struct device *comp, struct device *master,
                   void *master_data);
  

}; @@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);

struct master;

+/**

• • struct component_master_ops - callback for the aggregate driver
• • Aggregate drivers are registered with component_master_add_with_match() and
• • unregistered with component_master_del().
• */

struct component_master_ops {

•   /**
  

•    * @bind:
  

•    *
  

•    * Called when all components or the aggregate driver, as specified in
  

•    * the match list passed to component_master_add_with_match(), are
  

•    * ready. Usually there are 3 steps to bind an aggregate driver:
  

•    *
  

•    * 1. Allocate a structure for the aggregate driver.
  

•    *
  

•    * 2. Bind all components to the aggregate driver by calling
  

•    *    component_bind_all() with the aggregate driver structure as opaque
  

•    *    pointer data.
  

•    *
  

•    * 3. Register the aggregate driver with the subsystem to publish its
  

•    *    interfaces.
  

•    *
  

•    * Note that the lifetime of the aggregate driver does not align with
  

•    * any of the underlying &struct device instances. Therefore devm cannot
  

•    * be used and all resources acquired or allocated in this callback must
  

•    * be expecitly released in the @unbind callback.
  

s/expecitly/explicitly/

...

•    */
    int (*bind)(struct device *master);
  

•   /**
  

•    * @unbind:
  

•    *
  

•    * Called when either the aggregate driver, using
  

•    * component_master_del(), or one of its components, using
  

•    * component_del(), is unregistered.
  

•    */
    void (*unbind)(struct device *master);
  

};

@@ -38,6 +92,22 @@ void component_match_add_release(struct device *master, void (*release)(struct device *, void *), int (*compare)(struct device *, void *), void *compare_data);

+/**

• • component_match_add - add a compent match
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the

"This" appears to be redundant.

...

• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match.
• • The allocated match list in @matchptr is automatically released using devm
• • actions.
• • See also component_match_add_release().
• */

static inline void component_match_add(struct device *master, struct component_match **matchptr, int (*compare)(struct device *, void *), void *compare_data) -- 2.20.1

On Tue, Feb 05, 2019 at 05:21:07PM +0100, Daniel Vetter wrote:

Someone owes me a beer ...

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

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

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

Please remove this line.

While typing these I think doing an s/component_master/aggregate/ would be useful:

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

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

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

v2: Review from Rafael:

• git add Documenation/driver-api/component.rst
• lots of polish to the wording + spelling fixes.

Cc: "C, Ramalingam" ramalingam.c@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Russell King rmk+kernel@arm.linux.org.uk Cc: Rafael J. Wysocki rafael@kernel.org Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

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

diff --git a/Documentation/driver-api/component.rst b/Documentation/driver-api/component.rst new file mode 100644 index 000000000000..3407ff0424b9 --- /dev/null +++ b/Documentation/driver-api/component.rst @@ -0,0 +1,17 @@ +========================================= +Component Framework for Aggregate Drivers +=========================================

+.. kernel-doc:: drivers/base/component.c

• :doc: overview

+API +===

+.. kernel-doc:: include/linux/component.h

• :internal:

+.. kernel-doc:: drivers/base/component.c

• :export:

diff --git a/Documentation/driver-api/device_link.rst b/Documentation/driver-api/device_link.rst index d6763272e747..2d5919b2b337 100644 --- a/Documentation/driver-api/device_link.rst +++ b/Documentation/driver-api/device_link.rst @@ -1,6 +1,9 @@ .. |struct dev_pm_domain| replace:: :c:type:`struct dev_pm_domain <dev_pm_domain>` .. |struct generic_pm_domain| replace:: :c:type:`struct generic_pm_domain <generic_pm_domain>`

+.. _device_link:

============ Device links ============ diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst index ab38ced66a44..c0b600ed9961 100644 --- a/Documentation/driver-api/index.rst +++ b/Documentation/driver-api/index.rst @@ -22,6 +22,7 @@ available subsections can be seen below. device_connection dma-buf device_link

• component message-based sound frame-buffer

diff --git a/drivers/base/component.c b/drivers/base/component.c index ddcea8739c12..4851e1006f11 100644 --- a/drivers/base/component.c +++ b/drivers/base/component.c @@ -16,6 +16,33 @@ #include <linux/slab.h> #include <linux/debugfs.h>

+/**

• • DOC: overview
• • The component framework allows drivers to collect a pile of sub-devices,

Helper.

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

helper

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

helper

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

helper

• • system suspend and resume operations. See also :ref:`device
• • links<device_link>`.
• • Components are registered using component_add() and unregistered with
• • component_del(), usually from the driver's probe and disconnect functions.
• • Aggregate drivers first assemble a component match list of what they need
• • using component_match_add(). This is then registered as an aggregate driver
• • using component_master_add_with_match(), and unregistered using
• • component_master_del().
• */

struct component;

struct component_match_array { @@ -301,10 +328,24 @@ static int component_match_realloc(struct device *dev, return 0; }

-/*

• • Add a component to be matched, with a release function.

+/**

• • component_match_add_release - add a component match with release callback
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @release: release function for @compare_data
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match.

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

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

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

• • The match array is first created or extended if necessary.

• • See also component_match_add().
  */

void component_match_add_release(struct device *master, struct component_match **matchptr, @@ -367,6 +408,18 @@ static void free_master(struct master *master) kfree(master); }

+/**

• • component_master_add_with_match - register an aggregate driver
• • @dev: device with the aggregate driver
• • @ops: callbacks for the aggregate driver
• • @match: component match list for the aggregate driver
• • Registers a new aggregate driver consisting of the components added to @match
• • by calling one of the component_match_add() functions. Once all components in

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

• • @match are available, it will be assembled by calling
• • &component_master_ops.bind from @ops. Must be unregistered by calling
• • component_master_del().
• */

int component_master_add_with_match(struct device *dev, const struct component_master_ops *ops, struct component_match *match) @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev, } EXPORT_SYMBOL_GPL(component_master_add_with_match);

+/**

• • component_master_del - unregister an aggregate driver
• • @dev: device with the aggregate driver
• • @ops: callbacks for the aggregate driver
• • Unregisters an aggregate driver registered with
• • component_master_add_with_match(). If necessary the aggregate driver is first
• • disassembled by calling &component_master_ops.unbind from @ops.
• */

void component_master_del(struct device *dev, const struct component_master_ops *ops) { @@ -430,6 +492,15 @@ static void component_unbind(struct component *component, devres_release_group(component->dev, component); }

+/**

• • component_unbind_all - unbind all component to an aggregate driver
• • @master_dev: device with the aggregate driver
• • @data: opaque pointer, passed to all components
• • Unbinds all components to the aggregate @dev by passing @data to their
• • &component_ops.unbind functions. Should be called from
• • &component_master_ops.unbind.
• */

void component_unbind_all(struct device *master_dev, void *data) { struct master *master; @@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master, return ret; }

+/**

• • component_bind_all - bind all component to an aggregate driver
• • @master_dev: device with the aggregate driver
• • @data: opaque pointer, passed to all components
• • Binds all components to the aggregate @dev by passing @data to their
• • &component_ops.bind functions. Should be called from
• • &component_master_ops.bind.
• */

int component_bind_all(struct device *master_dev, void *data) { struct master *master; @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data) } EXPORT_SYMBOL_GPL(component_bind_all);

+/**

• • component_add - register a component
• • @dev: component device
• • @ops: component callbacks
• • Register a new component for @dev. Functions in @ops will be called when the
• • aggregate driver is ready to bind the overall driver by calling
• • component_bind_all(). See also &struct component_ops.
• • The component needs to be unregistered again at driver unload/disconnect by

"again" is unnecessary.

• • calling component_del().
• */

int component_add(struct device *dev, const struct component_ops *ops) { struct component *component; @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops) } EXPORT_SYMBOL_GPL(component_add);

+/**

• • component_del - unregister a component
• • @dev: component device
• • @ops: component callbacks
• • Unregister a component added with component_add(). If the component is bound
• • into an aggregate driver, this will force the entire aggregate driver, including
• • all its components, to be unbound.
• */

void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; diff --git a/include/linux/component.h b/include/linux/component.h index e71fbbbc74e2..b5a989e3871e 100644 --- a/include/linux/component.h +++ b/include/linux/component.h @@ -4,11 +4,31 @@

#include <linux/stddef.h>

struct device;

+/**

• • struct component_ops - callbacks for component drivers
• • Components are registered with component_add() and unregistered with
• • component_del().
• */

struct component_ops {

• /**

• * @bind:
  

• *
  

• * Called through component_bind_all() when the aggregate driver is
  

• * ready to bind the overall driver.
  

• */
  

  int (*bind)(struct device *comp, struct device *master, void *master_data);
• /**

• * @unbind:
  

• *
  

• * Called through component_unbind_all() when the aggregate driver is
  

• * ready to bind the overall driver, or when component_bind_all() fails
  

• * part-ways through and needs to unbind some already bound components.
  

• */
  

  void (*unbind)(struct device *comp, struct device *master, void *master_data);

}; @@ -21,8 +41,42 @@ void component_unbind_all(struct device *master, void *master_data);

struct master;

+/**

• • struct component_master_ops - callback for the aggregate driver
• • Aggregate drivers are registered with component_master_add_with_match() and
• • unregistered with component_master_del().
• */

struct component_master_ops {

• /**

• * @bind:
  

• *
  

• * Called when all components or the aggregate driver, as specified in
  

• * the match list passed to component_master_add_with_match(), are
  

• * ready. Usually there are 3 steps to bind an aggregate driver:
  

• *
  

• * 1. Allocate a structure for the aggregate driver.
  

• *
  

• * 2. Bind all components to the aggregate driver by calling
  

• *    component_bind_all() with the aggregate driver structure as opaque
  

• *    pointer data.
  

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

• *
  

• * 3. Register the aggregate driver with the subsystem to publish its
  

• *    interfaces.
  

• *
  

• * Note that the lifetime of the aggregate driver does not align with
  

• * any of the underlying &struct device instances. Therefore devm cannot
  

• * be used and all resources acquired or allocated in this callback must
  

• * be explicitly released in the @unbind callback.
  

• */
  

  int (*bind)(struct device *master);
• /**

• * @unbind:
  

• *
  

• * Called when either the aggregate driver, using
  

• * component_master_del(), or one of its components, using
  

• * component_del(), is unregistered.
  

• */
  

  void (*unbind)(struct device *master);

};

@@ -38,6 +92,22 @@ void component_match_add_release(struct device *master, void (*release)(struct device *, void *), int (*compare)(struct device *, void *), void *compare_data);

+/**

• • component_match_add - add a compent match
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • Adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. @matchptr must be initialized to
• • NULL before adding the first match.
• • The allocated match list in @matchptr is automatically released using devm
• • actions.
• • See also component_match_add_release().
• */

static inline void component_match_add(struct device *master, struct component_match **matchptr, int (*compare)(struct device *, void *), void *compare_data) -- 2.20.1

Someone owes me a beer ...

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

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

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

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

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

Cc: "C, Ramalingam" ramalingam.c@intel.com Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Russell King rmk+kernel@arm.linux.org.uk Cc: Rafael J. Wysocki rafael@kernel.org Cc: Jaroslav Kysela perex@perex.cz Cc: Takashi Iwai tiwai@suse.com Cc: Rodrigo Vivi rodrigo.vivi@intel.com Cc: Jani Nikula jani.nikula@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Documentation/driver-api/component.rst | 17 ++++ Documentation/driver-api/device_link.rst | 3 + Documentation/driver-api/index.rst | 1 + drivers/base/component.c | 107 ++++++++++++++++++++++- include/linux/component.h | 71 +++++++++++++++ 5 files changed, 196 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..f34d4b784709 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,25 @@ static int component_match_realloc(struct device *dev, return 0; }

-/* - * Add a component to be matched, with a release function. +/** + * component_match_add_release - add a component match with release callback + * @master: device with the aggregate driver + * @matchptr: pointer to the list of component matches + * @release: release function for @compare_data + * @compare: compare function to match against all components + * @compare_data: opaque pointer passed to the @compare function + * + * This adds a new component match to the list stored in @matchptr, which the + * @master aggregate driver needs to function. 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 +408,18 @@ static void free_master(struct master *master) kfree(master); }

+/** + * component_master_add_with_match - register an aggregate driver + * @dev: device with the aggregate driver + * @ops: callbacks for the aggregate driver + * @match: component match list for the aggregate driver + * + * Registers a new aggregate driver consisting of the components added to @match + * by calling one of the component_match_add() functions. Once all components in + * @match are available, it will be assembled by calling + * &component_master_ops.bind from @ops. Must be unregistered by calling + * component_master_del(). + */ int component_master_add_with_match(struct device *dev, const struct component_master_ops *ops, struct component_match *match) @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev, } EXPORT_SYMBOL_GPL(component_master_add_with_match);

+/** + * component_master_del - unregister an aggregate driver + * @dev: device with the aggregate driver + * @ops: callbacks for the aggregate driver + * + * Unregisters an aggregate driver registered with + * component_master_add_with_match(). If necessary the aggregate driver is first + * disassembled by calling &component_master_ops.unbind from @ops. + */ void component_master_del(struct device *dev, const struct component_master_ops *ops) { @@ -430,6 +492,15 @@ static void component_unbind(struct component *component, devres_release_group(component->dev, component); }

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

+/** + * component_bind_all - bind all component to an aggregate driver + * @master_dev: device with the aggregate driver + * @data: opaque pointer, passed to all components + * + * Binds all components to the aggregate @dev by passing @data to their + * &component_ops.bind functions. Should be called from + * &component_master_ops.bind. + */ int component_bind_all(struct device *master_dev, void *data) { struct master *master; @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data) } EXPORT_SYMBOL_GPL(component_bind_all);

+/** + * component_add - register a component + * @dev: component device + * @ops: component callbacks + * + * Register a new component for @dev. Functions in @ops will be called when the + * aggregate driver is ready to bind the overall driver by calling + * component_bind_all(). See also &struct component_ops. + * + * The component needs to be unregistered at driver unload/disconnect by calling + * component_del(). + */ int component_add(struct device *dev, const struct component_ops *ops) { struct component *component; @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops) } EXPORT_SYMBOL_GPL(component_add);

+/** + * component_del - unregister a component + * @dev: component device + * @ops: component callbacks + * + * Unregister a component added with component_add(). If the component is bound + * into an aggregate driver, this will force the entire aggregate driver, including + * all its components, to be unbound. + */ void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; diff --git a/include/linux/component.h b/include/linux/component.h index e71fbbbc74e2..4911598b4c42 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,23 @@ void component_match_add_release(struct device *master, void (*release)(struct device *, void *), int (*compare)(struct device *, void *), void *compare_data);

+/** + * component_match_add - add a compent match + * @master: device with the aggregate driver + * @matchptr: pointer to the list of component matches + * @compare: compare function to match against all components + * @compare_data: opaque pointer passed to the @compare function + * + * This adds a new component match to the list stored in @matchptr, which the + * @master aggregate driver needs to function. 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)

) On Wed, Feb 6, 2019 at 5:46 PM 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.

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

diff --git a/drivers/base/component.c b/drivers/base/component.c index f34d4b784709..68ccd5a0d5d6 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,8 +168,13 @@ static struct component *find_component(struct master *master, if (c->master && c->master != master) continue;

•           if (compare(c->dev, compare_data))
  

•           if (mc->compare_typed) {
  

•                   if (mc->compare_typed(c->dev, c->subcomponent,
  

•                                         mc->data))
  

This line break looks kind of weird to me,

•                           return c;
  

•           } else if (mc->compare(c->dev, mc->data)) {
                    return c;
  

•           }
  

Also, why don't you do

if (mc->compare(c->dev, mc->data) || (mc->compare_typed && mc->compare_typed(c->dev, c->subcomponent, mc->data))) return c;

The only difference is that ->compare() will run first and if it finds a match, c will be returned right away. Does it matter?

    }

    return NULL;

@@ -192,7 +199,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,30 +334,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
• • This adds a new component match to the list stored in @matchptr, which the

"This" appears to be redundant here (and in some places below too).

On Wed, Feb 06, 2019 at 11:57:04PM +0100, Rafael J. Wysocki wrote:

) On Wed, Feb 6, 2019 at 5:46 PM 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.

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

diff --git a/drivers/base/component.c b/drivers/base/component.c index f34d4b784709..68ccd5a0d5d6 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,8 +168,13 @@ static struct component *find_component(struct master *master, if (c->master && c->master != master) continue;

•           if (compare(c->dev, compare_data))
  

•           if (mc->compare_typed) {
  

•                   if (mc->compare_typed(c->dev, c->subcomponent,
  

•                                         mc->data))
  

This line break looks kind of weird to me,

...

•                           return c;
  

•           } else if (mc->compare(c->dev, mc->data)) {
                    return c;
  

•           }
  

Also, why don't you do

if (mc->compare(c->dev, mc->data) || (mc->compare_typed && mc->compare_typed(c->dev, c->subcomponent, mc->data))) return c;

The only difference is that ->compare() will run first and if it finds a match, c will be returned right away. Does it matter?

Tried it, yes it does matter: We have either ->compare or ->compare_typed, so calling one unconditionally blows up. Can still be streamlined a bit I think. -Daniel

...

    }

    return NULL;

@@ -192,7 +199,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,30 +334,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
• • This adds a new component match to the list stored in @matchptr, which the

"This" appears to be redundant here (and in some places below too).

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

On Wed, Feb 6, 2019 at 5:47 PM Daniel Vetter daniel.vetter@ffwll.ch 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.

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

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

---

Documentation/driver-api/component.rst | 17 ++++ Documentation/driver-api/device_link.rst | 3 + Documentation/driver-api/index.rst | 1 + drivers/base/component.c | 107 ++++++++++++++++++++++- include/linux/component.h | 71 +++++++++++++++ 5 files changed, 196 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..f34d4b784709 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,25 @@ static int component_match_realloc(struct device *dev, return 0; }

-/*

• • Add a component to be matched, with a release function.

+/**

• • component_match_add_release - add a component match with release callback
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @release: release function for @compare_data
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. 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 +408,18 @@ static void free_master(struct master *master) kfree(master); }

+/**

• • component_master_add_with_match - register an aggregate driver
• • @dev: device with the aggregate driver
• • @ops: callbacks for the aggregate driver
• • @match: component match list for the aggregate driver
• • Registers a new aggregate driver consisting of the components added to @match
• • by calling one of the component_match_add() functions. Once all components in
• • @match are available, it will be assembled by calling
• • &component_master_ops.bind from @ops. Must be unregistered by calling
• • component_master_del().
• */

int component_master_add_with_match(struct device *dev, const struct component_master_ops *ops, struct component_match *match) @@ -403,6 +456,15 @@ int component_master_add_with_match(struct device *dev, } EXPORT_SYMBOL_GPL(component_master_add_with_match);

+/**

• • component_master_del - unregister an aggregate driver
• • @dev: device with the aggregate driver
• • @ops: callbacks for the aggregate driver
• • Unregisters an aggregate driver registered with
• • component_master_add_with_match(). If necessary the aggregate driver is first
• • disassembled by calling &component_master_ops.unbind from @ops.
• */

void component_master_del(struct device *dev, const struct component_master_ops *ops) { @@ -430,6 +492,15 @@ static void component_unbind(struct component *component, devres_release_group(component->dev, component); }

+/**

• • component_unbind_all - unbind all component to an aggregate driver
• • @master_dev: device with the aggregate driver
• • @data: opaque pointer, passed to all components
• • Unbinds all components to the aggregate @dev by passing @data to their
• • &component_ops.unbind functions. Should be called from
• • &component_master_ops.unbind.
• */

void component_unbind_all(struct device *master_dev, void *data) { struct master *master; @@ -503,6 +574,15 @@ static int component_bind(struct component *component, struct master *master, return ret; }

+/**

• • component_bind_all - bind all component to an aggregate driver
• • @master_dev: device with the aggregate driver
• • @data: opaque pointer, passed to all components
• • Binds all components to the aggregate @dev by passing @data to their
• • &component_ops.bind functions. Should be called from
• • &component_master_ops.bind.
• */

int component_bind_all(struct device *master_dev, void *data) { struct master *master; @@ -537,6 +617,18 @@ int component_bind_all(struct device *master_dev, void *data) } EXPORT_SYMBOL_GPL(component_bind_all);

+/**

• • component_add - register a component
• • @dev: component device
• • @ops: component callbacks
• • Register a new component for @dev. Functions in @ops will be called when the
• • aggregate driver is ready to bind the overall driver by calling
• • component_bind_all(). See also &struct component_ops.
• • The component needs to be unregistered at driver unload/disconnect by calling
• • component_del().
• */

int component_add(struct device *dev, const struct component_ops *ops) { struct component *component; @@ -568,6 +660,15 @@ int component_add(struct device *dev, const struct component_ops *ops) } EXPORT_SYMBOL_GPL(component_add);

+/**

• • component_del - unregister a component
• • @dev: component device
• • @ops: component callbacks
• • Unregister a component added with component_add(). If the component is bound
• • into an aggregate driver, this will force the entire aggregate driver, including
• • all its components, to be unbound.
• */

void component_del(struct device *dev, const struct component_ops *ops) { struct component *c, *component = NULL; diff --git a/include/linux/component.h b/include/linux/component.h index e71fbbbc74e2..4911598b4c42 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,23 @@ void component_match_add_release(struct device *master, void (*release)(struct device *, void *), int (*compare)(struct device *, void *), void *compare_data);

+/**

• • component_match_add - add a compent match
• • @master: device with the aggregate driver
• • @matchptr: pointer to the list of component matches
• • @compare: compare function to match against all components
• • @compare_data: opaque pointer passed to the @compare function
• • This adds a new component match to the list stored in @matchptr, which the
• • @master aggregate driver needs to function. 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

On Thu, Jan 31, 2019 at 12:29:18PM +0530, Ramalingam C wrote:

From: Daniel Vetter daniel.vetter@ffwll.ch

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

Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: 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

Takashi, can you pls take a look and ack for merging through drm-intel? We can also do a topic branch in case this conflicts with something on the audio side.

Thanks, Daniel

---

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

On 1/31/2019 1:47 PM, Daniel Vetter wrote:

On Thu, Jan 31, 2019 at 12:29:22PM +0530, Ramalingam C wrote:

...

Defining the mei-i915 interface functions and initialization of the interface.

v2: Adjust to the new interface changes. [Tomas] Added further debug logs for the failures at MEI i/f. port in hdcp_port data is equipped to handle -ve values. v3: mei comp is matched for global i915 comp master. [Daniel] In hdcp_shim hdcp_protocol() is replaced with const variable. [Daniel] mei wrappers are adjusted as per the i/f change [Daniel] v4: port initialization is done only at hdcp2_init only [Danvet] v5: I915 registers a subcomponent to be matched with mei_hdcp [Daniel]

Signed-off-by: Ramalingam C ramalingam.c@intel.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

When you make substantial changes to a patch (like here) and decide to keep the r-b, then please indicate that it was for an earlier version. I most definitely didn't review this one that re-adds all the locking :-)

sure :)

What's missing here is the component_del. Not exactly sure why this doesn't blow up.

That is weird. But yes note the absence of _del. I will introduce the the call to hdcp_exit at unload and handle the component_del.

Luckily we don't need a component_del_typed because component_del already takes the (dev, ops) pair, and that's unique.

yes true.

-Ram

-Daniel

...

---

drivers/gpu/drm/i915/i915_drv.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 7 + drivers/gpu/drm/i915/intel_drv.h | 5 + drivers/gpu/drm/i915/intel_hdcp.c | 378 +++++++++++++++++++++++++++++++++++++- include/drm/i915_component.h | 3 + 5 files changed, 393 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index a7aaa1ac4c99..75aff907ba69 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -904,6 +904,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv) mutex_init(&dev_priv->av_mutex); mutex_init(&dev_priv->wm.wm_mutex); mutex_init(&dev_priv->pps_mutex);

• mutex_init(&dev_priv->hdcp_comp_mutex);

  i915_memcpy_init_early(dev_priv); intel_runtime_pm_init_early(dev_priv);

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 22da9df1f0a7..d9a0771af4d1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -55,6 +55,7 @@ #include <drm/drm_util.h> #include <drm/drm_dsc.h> #include <drm/drm_connector.h> +#include <drm/i915_mei_hdcp_interface.h>

#include "i915_fixed.h" #include "i915_params.h" @@ -2043,6 +2044,12 @@ struct drm_i915_private {

struct i915_pmu pmu;

• struct i915_hdcp_comp_master *hdcp_master;
• bool hdcp_comp_added;
• /* Mutex to protect the above hdcp component related values. */
• struct mutex hdcp_comp_mutex;
• /*
  • NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
  • will be rejected. Instead look for a better place.

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0ac870feb5e9..63e009286d5f 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -41,6 +41,7 @@ #include <drm/drm_rect.h> #include <drm/drm_vblank.h> #include <drm/drm_atomic.h> +#include <drm/i915_mei_hdcp_interface.h> #include <media/cec-notifier.h>

struct drm_printer; @@ -385,6 +386,9 @@ struct intel_hdcp_shim { /* Detects panel's hdcp capability. This is optional for HDMI. */ int (*hdcp_capable)(struct intel_digital_port *intel_dig_port, bool *hdcp_capable);

• /* HDCP adaptation(DP/HDMI) required on the port */

• enum hdcp_wired_protocol protocol; };

  struct intel_hdcp {

@@ -405,6 +409,7 @@ struct intel_hdcp { * content can flow only through a link protected by HDCP2.2. */ u8 content_type;

• struct hdcp_port_data port_data; };

  struct intel_connector {

diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index 1a85dc46692d..e0bb5f32ba90 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -7,8 +7,10 @@ */

#include <drm/drm_hdcp.h> +#include <drm/i915_component.h> #include <linux/i2c.h> #include <linux/random.h> +#include <linux/component.h>

#include "intel_drv.h" #include "i915_reg.h" @@ -832,6 +834,348 @@ bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port) return INTEL_GEN(dev_priv) >= 9 && port < PORT_E; }

+static __attribute__((unused)) int +hdcp2_prepare_ake_init(struct intel_connector *connector,

•        struct hdcp2_ake_init *ake_data)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->initiate_hdcp2_session(comp->mei_dev, data, ake_data);
• if (ret)

• DRM_DEBUG_KMS("Prepare_ake_init failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector,

• 		struct hdcp2_ake_send_cert *rx_cert,
  

• 		bool *paired,
  

• 		struct hdcp2_ake_no_stored_km *ek_pub_km,
  

• 		size_t *msg_sz)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->verify_receiver_cert_prepare_km(comp->mei_dev, data,

• 					 rx_cert, paired,
  

• 					 ek_pub_km, msg_sz);
  

• if (ret < 0)

• DRM_DEBUG_KMS("Verify rx_cert failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_verify_hprime(struct intel_connector *connector,

•     struct hdcp2_ake_send_hprime *rx_hprime)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->verify_hprime(comp->mei_dev, data, rx_hprime);
• if (ret < 0)

• DRM_DEBUG_KMS("Verify hprime failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_store_pairing_info(struct intel_connector *connector,

• 	 struct hdcp2_ake_send_pairing_info *pairing_info)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->store_pairing_info(comp->mei_dev, data, pairing_info);
• if (ret < 0)

• DRM_DEBUG_KMS("Store pairing info failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_prepare_lc_init(struct intel_connector *connector,

•       struct hdcp2_lc_init *lc_init)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->initiate_locality_check(comp->mei_dev, data, lc_init);
• if (ret < 0)

• DRM_DEBUG_KMS("Prepare lc_init failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_verify_lprime(struct intel_connector *connector,

•     struct hdcp2_lc_send_lprime *rx_lprime)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->verify_lprime(comp->mei_dev, data, rx_lprime);
• if (ret < 0)

• DRM_DEBUG_KMS("Verify L_Prime failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) +int hdcp2_prepare_skey(struct intel_connector *connector,

•        struct hdcp2_ske_send_eks *ske_data)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->get_session_key(comp->mei_dev, data, ske_data);
• if (ret < 0)

• DRM_DEBUG_KMS("Get session key failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_verify_rep_topology_prepare_ack(struct intel_connector *connector,

• 		      struct hdcp2_rep_send_receiverid_list
  

• 						*rep_topology,
  

• 		      struct hdcp2_rep_send_ack *rep_send_ack)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->repeater_check_flow_prepare_ack(comp->mei_dev, data,

• 					 rep_topology,
  

• 					 rep_send_ack);
  

• if (ret < 0)

• DRM_DEBUG_KMS("Verify rep topology failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) int +hdcp2_verify_mprime(struct intel_connector *connector,

•     struct hdcp2_rep_stream_ready *stream_ready)
  

+{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->verify_mprime(comp->mei_dev, data, stream_ready);
• if (ret < 0)

• DRM_DEBUG_KMS("Verify mprime failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) +int hdcp2_authenticate_port(struct intel_connector *connector) +{

• struct hdcp_port_data *data = &connector->hdcp.port_data;
• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->enable_hdcp_authentication(comp->mei_dev, data);
• if (ret < 0)

• DRM_DEBUG_KMS("Enable hdcp auth failed. %d\n", ret);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) +int hdcp2_close_mei_session(struct intel_connector *connector) +{

• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• struct i915_hdcp_comp_master *comp;
• int ret;
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• comp = dev_priv->hdcp_master;
• if (!comp || !comp->ops) {

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• return -EINVAL;
  

• }
• ret = comp->ops->close_hdcp_session(comp->mei_dev,

• 			     &connector->hdcp.port_data);
  

• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return ret;

+}

+static __attribute__((unused)) +int hdcp2_deauthenticate_port(struct intel_connector *connector) +{

• return hdcp2_close_mei_session(connector);

+}

+static int i915_hdcp_component_bind(struct device *i915_kdev,

• 		    struct device *mei_kdev, void *data)
  

+{

• struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev);
• DRM_DEBUG("I915 HDCP comp bind\n");
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• dev_priv->hdcp_master = (struct i915_hdcp_comp_master *)data;
• dev_priv->hdcp_master->mei_dev = mei_kdev;
• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• return 0;

+}

+static void i915_hdcp_component_unbind(struct device *i915_kdev,

• 		       struct device *mei_kdev, void *data)
  

+{

• struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev);
• DRM_DEBUG("I915 HDCP comp unbind\n");
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• dev_priv->hdcp_master->mei_dev = NULL;
• dev_priv->hdcp_master = NULL;
• mutex_unlock(&dev_priv->hdcp_comp_mutex);

+}

+static const struct component_ops i915_hdcp_component_ops = {

• .bind = i915_hdcp_component_bind,
• .unbind = i915_hdcp_component_unbind,

+};

+static int initialize_hdcp_port_data(struct intel_connector *connector) +{

• struct intel_hdcp *hdcp = &connector->hdcp;
• struct hdcp_port_data *data = &hdcp->port_data;
• data->port = connector->encoder->port;
• data->port_type = (u8)HDCP_PORT_TYPE_INTEGRATED;
• data->protocol = (u8)hdcp->shim->protocol;
• data->k = 1;
• if (!data->streams)

• data->streams = kcalloc(data->k,
  

• 			sizeof(struct hdcp2_streamid_type),
  

• 			GFP_KERNEL);
  

• if (!data->streams) {

• DRM_ERROR("Out of Memory\n");
  

• return -ENOMEM;
  

• }
• data->streams[0].stream_id = 0;
• data->streams[0].stream_type = hdcp->content_type;
• return 0;

+}

• bool is_hdcp2_supported(struct drm_i915_private *dev_priv) { if (!IS_ENABLED(CONFIG_INTEL_MEI_HDCP))

@@ -845,10 +1189,42 @@ static void intel_hdcp2_init(struct intel_connector *connector) { struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct intel_hdcp *hdcp = &connector->hdcp;

• bool add_component = false;

• int ret;

  WARN_ON(!is_hdcp2_supported(dev_priv));

• /* TODO: MEI interface needs to be initialized here */

• ret = initialize_hdcp_port_data(connector);
• if (ret) {

• DRM_DEBUG_KMS("Mei hdcp data init failed\n");
  

• return;
  

• }
• mutex_lock(&dev_priv->hdcp_comp_mutex);
• /*

• * Component for mei is common across the connector.
  

• * Adding the component once is sufficient.
  

• */
  

• if (!dev_priv->hdcp_comp_added) {

• dev_priv->hdcp_comp_added = true;
  

• add_component = true;
  

• }
• mutex_unlock(&dev_priv->hdcp_comp_mutex);
• if (add_component) {

• ret = component_add_typed(dev_priv->drm.dev,
  

• 			  &i915_hdcp_component_ops,
  

• 			  I915_COMPONENT_HDCP);
  

• if (ret < 0) {
  

• 	DRM_DEBUG_KMS("Failed at component add(%d)\n", ret);
  

• 	mutex_lock(&dev_priv->hdcp_comp_mutex);
  

• 	dev_priv->hdcp_comp_added = false;
  

• 	mutex_unlock(&dev_priv->hdcp_comp_mutex);
  

• 	kfree(hdcp->port_data.streams);
  

• 	return;
  

• }
  

• }
• hdcp->hdcp2_supported = true; }

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 72fbb037f9b3..f3851ba3e4c3 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -24,10 +24,13 @@ #ifndef _I915_COMPONENT_H_ #define _I915_COMPONENT_H_

+#include <linux/device.h>

• #include "drm_audio_component.h"

  enum i915_component_type { I915_COMPONENT_AUDIO = 1,

• I915_COMPONENT_HDCP, };

  /* MAX_PORT is the number of port

-- 2.7.4

On Thu, Jan 31, 2019 at 12:29:23PM +0530, Ramalingam C wrote:

"hdcp_encrypted" flag is defined to denote the HDCP1.4 encryption status. This SW tracking is used to determine the need for real hdcp1.4 disable and hdcp_check_link upon CP_IRQ.

On CP_IRQ we filter the CP_IRQ related to the states like Link failure and reauthentication req etc and handle them in hdcp_check_link. CP_IRQ corresponding to the authentication msg availability are ignored.

WARN_ON is added for the abrupt stop of HDCP encryption of a port.

v2: bool is used in struct for the cleaner coding. [Daniel] check_link work_fn is scheduled for cp_irq handling [Daniel]

Just doing a delta-review, v2 addresses my comment about synchronization with the worker, so

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

Signed-off-by: Ramalingam C ramalingam.c@intel.com

drivers/gpu/drm/i915/intel_dp.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 5 ++- drivers/gpu/drm/i915/intel_hdcp.c | 73 ++++++++++++++++++++++++++++----------- 3 files changed, 58 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 815ee68efa2f..9ce05819fc11 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4776,7 +4776,7 @@ static void intel_dp_check_service_irq(struct intel_dp *intel_dp) intel_dp_handle_test_request(intel_dp);

if (val & DP_CP_IRQ)

• intel_hdcp_check_link(intel_dp->attached_connector);
  

• intel_hdcp_handle_cp_irq(intel_dp->attached_connector);
  
  

  if (val & DP_SINK_SPECIFIC_IRQ) DRM_DEBUG_DRIVER("Sink specific irq unhandled\n");

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 63e009286d5f..13a41e8cf4ff 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -399,6 +399,9 @@ struct intel_hdcp { struct delayed_work check_work; struct work_struct prop_work;

• /* HDCP1.4 Encryption status */
• bool hdcp_encrypted;
• /* HDCP2.2 related definitions */ /* Flag indicates whether this connector supports HDCP2.2 or not. */ bool hdcp2_supported;

@@ -2073,10 +2076,10 @@ int intel_hdcp_init(struct intel_connector *connector, bool hdcp2_supported); int intel_hdcp_enable(struct intel_connector *connector); int intel_hdcp_disable(struct intel_connector *connector); -int intel_hdcp_check_link(struct intel_connector *connector); bool is_hdcp_supported(struct drm_i915_private *dev_priv, enum port port); bool intel_hdcp_capable(struct intel_connector *connector); bool is_hdcp2_supported(struct drm_i915_private *dev_priv); +void intel_hdcp_handle_cp_irq(struct intel_connector *connector);

/* intel_psr.c */ #define CAN_PSR(dev_priv) (HAS_PSR(dev_priv) && dev_priv->psr.sink_support) diff --git a/drivers/gpu/drm/i915/intel_hdcp.c b/drivers/gpu/drm/i915/intel_hdcp.c index e0bb5f32ba90..c1b057f1501b 100644 --- a/drivers/gpu/drm/i915/intel_hdcp.c +++ b/drivers/gpu/drm/i915/intel_hdcp.c @@ -74,6 +74,16 @@ bool intel_hdcp_capable(struct intel_connector *connector) return capable; }

+static inline bool intel_hdcp_in_use(struct intel_connector *connector) +{

• struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
• enum port port = connector->encoder->port;
• u32 reg;
• reg = I915_READ(PORT_HDCP_STATUS(port));
• return reg & HDCP_STATUS_ENC;

+}

static int intel_hdcp_poll_ksv_fifo(struct intel_digital_port *intel_dig_port, const struct intel_hdcp_shim *shim) { @@ -668,6 +678,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector) DRM_DEBUG_KMS("[%s:%d] HDCP is being disabled...\n", connector->base.name, connector->base.base.id);

• hdcp->hdcp_encrypted = false; I915_WRITE(PORT_HDCP_CONF(port), 0); if (intel_wait_for_register(dev_priv, PORT_HDCP_STATUS(port), ~0, 0, ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) {

@@ -713,8 +724,10 @@ static int _intel_hdcp_enable(struct intel_connector *connector) /* Incase of authentication failures, HDCP spec expects reauth. */ for (i = 0; i < tries; i++) { ret = intel_hdcp_auth(conn_to_dig_port(connector), hdcp->shim);

• if (!ret)
  

• if (!ret) {
  

• 	hdcp->hdcp_encrypted = true;
  return 0;
  

• }
  
  

  DRM_DEBUG_KMS("HDCP Auth failure (%d)\n", ret);

@@ -741,16 +754,17 @@ int intel_hdcp_check_link(struct intel_connector *connector) enum port port = intel_dig_port->base.port; int ret = 0;

• if (!hdcp->shim)

• return -ENOENT;
  

• mutex_lock(&hdcp->mutex);

• if (hdcp->value == DRM_MODE_CONTENT_PROTECTION_UNDESIRED)

• /* Check_link valid only when HDCP1.4 is enabled */
• if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED ||

•    !hdcp->hdcp_encrypted) {
  

• ret = -EINVAL;
  

  goto out;
• }

• if (!(I915_READ(PORT_HDCP_STATUS(port)) & HDCP_STATUS_ENC)) {

• DRM_ERROR("%s:%d HDCP check failed: link is not encrypted,%x\n",
  

• if (WARN_ON(!intel_hdcp_in_use(connector))) {

• DRM_ERROR("%s:%d HDCP link stopped encryption,%x\n",
    connector->base.name, connector->base.base.id,
    I915_READ(PORT_HDCP_STATUS(port)));
  

  ret = -ENXIO;

@@ -791,18 +805,6 @@ int intel_hdcp_check_link(struct intel_connector *connector) return ret; }

-static void intel_hdcp_check_work(struct work_struct *work) -{

• struct intel_hdcp *hdcp = container_of(to_delayed_work(work),

• 			       struct intel_hdcp,
  

• 			       check_work);
  

• struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
• if (!intel_hdcp_check_link(connector))

• schedule_delayed_work(&hdcp->check_work,
  

• 		      DRM_HDCP_CHECK_PERIOD_MS);
  

-}

static void intel_hdcp_prop_work(struct work_struct *work) { struct intel_hdcp *hdcp = container_of(work, struct intel_hdcp, @@ -1120,6 +1122,18 @@ int hdcp2_deauthenticate_port(struct intel_connector *connector) return hdcp2_close_mei_session(connector); }

+static void intel_hdcp_check_work(struct work_struct *work) +{

• struct intel_hdcp *hdcp = container_of(to_delayed_work(work),

• 			       struct intel_hdcp,
  

• 			       check_work);
  

• struct intel_connector *connector = intel_hdcp_to_connector(hdcp);
• if (!intel_hdcp_check_link(connector))

• schedule_delayed_work(&hdcp->check_work,
  

• 		      DRM_HDCP_CHECK_PERIOD_MS);
  

+}

static int i915_hdcp_component_bind(struct device *i915_kdev, struct device *mei_kdev, void *data) { @@ -1288,7 +1302,8 @@ int intel_hdcp_disable(struct intel_connector *connector)

if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { hdcp->value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED;

• ret = _intel_hdcp_disable(connector);
  

• if (hdcp->hdcp_encrypted)
  

• 	ret = _intel_hdcp_disable(connector);
  

  }

  mutex_unlock(&hdcp->mutex);

@@ -1328,3 +1343,21 @@ void intel_hdcp_atomic_check(struct drm_connector *connector, new_state->crtc); crtc_state->mode_changed = true; }

+/* Handles the CP_IRQ raised from the DP HDCP sink */ +void intel_hdcp_handle_cp_irq(struct intel_connector *connector) +{

• struct intel_hdcp *hdcp = &connector->hdcp;
• if (!hdcp->shim)

• return;
  

• /*

• * CP_IRQ could be triggered due to 1. HDCP2.2 auth msgs availability,
  

• * 2. link failure and 3. repeater reauth request. At present we dont
  

• * handle the CP_IRQ for the HDCP2.2 auth msg availability for read.
  

• * To handle other two causes for CP_IRQ we have the work_fn which is
  

• * scheduled here.
  

• */
  

• schedule_delayed_work(&hdcp->check_work, 0);

+}

2.7.4