Re: [PATCH 2/3] components: multiple components for a device

From: Daniel Vetter <daniel@ffwll.ch>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Takashi Iwai <tiwai@suse.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Jaroslav Kysela <perex@perex.cz>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Russell King <rmk+kernel@arm.linux.org.uk>
Subject: Re: [PATCH 2/3] components: multiple components for a device
Date: Thu, 7 Feb 2019 23:40:24 +0100	[thread overview]
Message-ID: <20190207224024.GF23159@phenom.ffwll.local> (raw)
In-Reply-To: <CAJZ5v0gR6cGb=baEOGz=EPTsLgLt-RwA5WpMgivzx9asMQO3=g@mail.gmail.com>

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

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