Re: [PATCH] component: Add documentation

[Daniel Vetter <daniel@ffwll.ch>]

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

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

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

[snip]

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

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


[snip]

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

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

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

Thanks for your review.

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