Re: [PATCH v10 01/40] components: multiple components for a device

Daniel Vetter
2019-01-31T08:00:35Z

On Thu, Jan 31, 2019 at 08:50:20AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 31, 2019 at 12:29:17PM +0530, Ramalingam C wrote:
> > +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);
>
> No comment at all as to what this new global function does?
>
> > +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);
>
> Same here, no comments at all?
>
> Please at the very least, document new things that you add, I thought I
> asked for this the last time this patch was posted :(

I replied and asked whether you insist on the docs for this or not, since
nothing has docs (and documenting these two alone is not going to explain
anything frankly). It's also defacto the drm component thing, other
subsystems didn't push their own solution into the core ...

There's also the bikeshed question for what exactly we should call these,
I'm not happy with the _typed suffix. Lockdep uses _class for this, but
kinda doesn't make, _subcomponent is a bit redundant and _sub also doesn't
convey much meaning.

Russell, any better ideas?

Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation