Re: [PATCH] component: Add documentation
On Wed, Feb 6, 2019 at 5:47 PM Daniel Vetter wrote:
>
> While typing these I think doing an s/component_master/aggregate/
> would be useful:
> - it's shorter :-)
> - I think component/aggregate is much more meaningful naming than
> component/puppetmaster or something like that. At least to my
> English ear "aggregate" emphasizes much more the "assemble a pile of
> things into something bigger" aspect, and there's not really much
> of a control hierarchy between aggregate and constituing components.
>
> But that's way more than a quick doc typing exercise ...
>
> Thanks to Ram for commenting on an initial draft of these docs.
>
> v2: Review from Rafael:
> - git add Documenation/driver-api/component.rst
> - lots of polish to the wording + spelling fixes.
>
> v3: Review from Russell:
> - s/framework/helper
> - clarify the documentation for component_match_add functions.
>
> Cc: "C, Ramalingam"
> Cc: Greg Kroah-Hartman
> Cc: Russell King
> Cc: Rafael J. Wysocki
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Cc: Rodrigo Vivi
> Cc: Jani Nikula
> Signed-off-by: Daniel Vetter
Reviewed-by: Rafael J. Wysocki
> ---
> 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 ..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
> `
> .. |struct generic_pm_domain| replace:: :c:type:`struct 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
> #include
>
> +/**
> + * 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`.
> + *
> + * 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
[PATCH] component: Add documentation
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"
Cc: Greg Kroah-Hartman
Cc: Russell King
Cc: Rafael J. Wysocki
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: Rodrigo Vivi
Cc: Jani Nikula
Signed-off-by: Daniel Vetter
---
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 ..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
`
.. |struct generic_pm_domain| replace:: :c:type:`struct 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
#include
+/**
+ * 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`.
+ *
+ * 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
Re: [PATCH] component: Add documentation
On Tue, Feb 05, 2019 at 04:49:02PM +, Russell King - ARM Linux admin wrote:
> On Tue, Feb 05, 2019 at 05:21:07PM +0100, Daniel Vetter wrote:
> > Someone owes me a beer ...
>
> I find that deeply offensive - it is clearly directed at me personally
> as author of the component helper.
>
> There are double-standards in the kernel ecosystem with respect to
> documentation - there are entire subsystems way more complicated than
> the component *helper* which are lacking in documentation, and the
> subsystem authors response to requests to change that basically get
> ignored, or the response is "write the documentation yourself".
>
> Why does there seem to be one rule for me and one rule for everyone
> else?
>
> Please remove this line.
Will do, but wasn't aimed at you at all, but at Greg for asking for the
documentation. But yeah that wasn't clear at all, my apologies.
Will apply all your suggestions except for the ones I'm commenting on here
in my reply.
[snip]
> > +/**
> > + * component_master_add_with_match - register an aggregate driver
> > + * @dev: device with the aggregate driver
> > + * @ops: callbacks for the aggregate driver
> > + * @match: component match list for the aggregate driver
> > + *
> > + * Registers a new aggregate driver consisting of the components added to
> > @match
> > + * by calling one of the component_match_add() functions. Once all
> > components in
>
> As there is a function called component_match_add(), this doesn't
> make too much sense as it directs people only to that function. It
> would be better to mention both here. (Have you checked how it comes
> out as a HTML document with the hyperlinks?)
component_match_add() creates a link to the kerneldoc for the
component_match_add. There I've added some text to point to all the other
variants of this family of functions. Same for all the other variants,
those should all have links to the others.
[snip]
> > +/**
> > + * struct component_master_ops - callback for the aggregate driver
> > + *
> > + * Aggregate drivers are registered with component_master_add_with_match()
> > and
> > + * unregistered with component_master_del().
> > + */
> > struct component_master_ops {
> > + /**
> > +* @bind:
> > +*
> > +* Called when all components or the aggregate driver, as specified in
> > +* the match list passed to component_master_add_with_match(), are
> > +* ready. Usually there are 3 steps to bind an aggregate driver:
> > +*
> > +* 1. Allocate a structure for the aggregate driver.
> > +*
> > +* 2. Bind all components to the aggregate driver by calling
> > +*component_bind_all() with the aggregate driver structure as opaque
> > +*pointer data.
>
> These two aren't part of the component helper specification, although
> nailing down what is passed per subsystem would be a good idea to allow
> components to be re-used within the subsystem. For DRM, it should be
> the struct drm_device.
Hm, great point. I have no idea where we should put it so people find
this. Definitely not here, since this isn't drivers/gpu. I think adding a
small section to the driver initialization docs we already have would make
sense.
I'll add another patch for that in this series, with this one here that
drm paragraph will even have somewhere meaningful to point to!
Thanks for your review.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] component: Add documentation
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" > Cc: Greg Kroah-Hartman > Cc: Russell King > Cc: Rafael J. Wysocki > Cc: Jaroslav Kysela > Cc: Takashi Iwai > Cc: Rodrigo Vivi > Cc: Jani Nikula > Signed-off-by: Daniel Vetter > --- > 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 ..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 > ` > .. |struct generic_pm_domain| replace:: :c:type:`struct 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 > #include > > +/** > + * 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`. > + * > + * 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
[PATCH] component: Add documentation
Someone owes me a beer ...
While typing these I think doing an s/component_master/aggregate/
would be useful:
- it's shorter :-)
- I think component/aggregate is much more meaningful naming than
component/puppetmaster or something like that. At least to my
English ear "aggregate" emphasizes much more the "assemble a pile of
things into something bigger" aspect, and there's not really much
of a control hierarchy between aggregate and constituing components.
But that's way more than a quick doc typing exercise ...
Thanks to Ram for commenting on an initial draft of these docs.
v2: Review from Rafael:
- git add Documenation/driver-api/component.rst
- lots of polish to the wording + spelling fixes.
Cc: "C, Ramalingam"
Cc: Greg Kroah-Hartman
Cc: Russell King
Cc: Rafael J. Wysocki
Cc: Jaroslav Kysela
Cc: Takashi Iwai
Cc: Rodrigo Vivi
Cc: Jani Nikula
Signed-off-by: Daniel Vetter
---
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 ..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
`
.. |struct generic_pm_domain| replace:: :c:type:`struct 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
#include
+/**
+ * DOC: overview
+ *
+ * The component framework allows drivers to collect a pile of sub-devices,
+ * including their bound drivers, into an aggregate driver. Various subsystems
+ * already provide functions to get hold of such components, e.g.
+ * of_clk_get_by_name(). The component framework can be used when such a
+ * subsystem-specific way to find a device is not available: The component
+ * framework fills the niche of aggregate drivers for specific hardware, where
+ * further standardization into a subsystem would not be practical. The common
+ * example is when a logical device (e.g. a DRM display driver) is spread
around
+ * the SoC on various component (scanout engines, blending blocks, transcoders
+ * for various outputs and so on).
+ *
+ * The component framework also doesn't solve runtime dependencies, e.g. for
+ * system suspend and resume operations. See also :ref:`device
+ * links`.
+ *
+ * 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
+ *
