diff mbox series

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

Message ID 1548917996-28081-2-git-send-email-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Implement HDCP2.2 | expand

Commit Message

Ramalingam C Jan. 31, 2019, 6:59 a.m. UTC 
From: Daniel Vetter <daniel.vetter@ffwll.ch>

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.

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>
---
 drivers/base/component.c  | 67 +++++++++++++++++++++++++++++++++++++++--------
 include/linux/component.h |  5 ++++
 2 files changed, 61 insertions(+), 11 deletions(-)

Comments

Greg Kroah-Hartman Jan. 31, 2019, 7:50 a.m. UTC | #1 
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 :(

greg k-h
Daniel Vetter Jan. 31, 2019, 8 a.m. UTC | #2 
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
Greg Kroah-Hartman Jan. 31, 2019, 8:12 a.m. UTC | #3 
On Thu, Jan 31, 2019 at 09:00:30AM +0100, Daniel Vetter wrote:
> 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 ...

I thought I responded that I would love to have something for the new
stuff at the very least.

And it's not nice to create new global functions without a single line
of comment for them as to what they are supposed to be used for.  So I'm
going to insist on that happening here at the very least.

thanks,

greg k-h
Daniel Vetter Jan. 31, 2019, 8:39 a.m. UTC | #4 
On Thu, Jan 31, 2019 at 09:12:45AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Jan 31, 2019 at 09:00:30AM +0100, Daniel Vetter wrote:
> > 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 ...
> 
> I thought I responded that I would love to have something for the new
> stuff at the very least.

Didn't see anything fly by ...

> And it's not nice to create new global functions without a single line
> of comment for them as to what they are supposed to be used for.  So I'm
> going to insist on that happening here at the very least.

I'll type the entire thing then. I get the "new stuff at least" drill to
fill gaps (use it plenty myself), but just doesn't make sense here at all
...

Cheers, Daniel
diff mbox series

Patch

diff --git a/drivers/base/component.c b/drivers/base/component.c
index ddcea8739c12..30309c0449ed 100644
--- a/drivers/base/component.c
+++ b/drivers/base/component.c
@@ -21,6 +21,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;
@@ -48,6 +49,7 @@  struct component {
 	bool bound;
 
 	const struct component_ops *ops;
+	int subcomponent;
 	struct device *dev;
 };
 
@@ -132,7 +134,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;
 
@@ -140,8 +142,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;
@@ -166,7 +173,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;
@@ -301,15 +308,12 @@  static int component_match_realloc(struct device *dev,
 	return 0;
 }
 
-/*
- * Add a component to be matched, with a release function.
- *
- * The match array is first created or extended if necessary.
- */
-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;
 
@@ -341,13 +345,37 @@  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++;
 }
+
+/*
+ * Add a component to be matched, with a release function.
+ *
+ * The match array is first created or extended if necessary.
+ */
+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);
 
+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;
@@ -537,7 +565,8 @@  int component_bind_all(struct device *master_dev, void *data)
 }
 EXPORT_SYMBOL_GPL(component_bind_all);
 
-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;
@@ -548,6 +577,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);
 
@@ -566,6 +596,21 @@  int component_add(struct device *dev, const struct component_ops *ops)
 
 	return ret < 0 ? ret : 0;
 }
+
+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);
+
+int component_add(struct device *dev, const struct component_ops *ops)
+{
+	return __component_add(dev, ops, 0);
+}
 EXPORT_SYMBOL_GPL(component_add);
 
 void component_del(struct device *dev, const struct component_ops *ops)
diff --git a/include/linux/component.h b/include/linux/component.h
index e71fbbbc74e2..6d7516f0546e 100644
--- a/include/linux/component.h
+++ b/include/linux/component.h
@@ -14,6 +14,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);
@@ -37,6 +39,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);
 
 static inline void component_match_add(struct device *master,
 	struct component_match **matchptr,