[08/13] ASoC: Track which components have been registered with snd_soc_register_component()
diff mbox

Message ID 1395129736-11938-9-git-send-email-lars@metafoo.de
State Accepted
Commit 98e639fb8a3ed1bf2bd512626c3cfc2992a57113
Headers show

Commit Message

Lars-Peter Clausen March 18, 2014, 8:02 a.m. UTC
snd_soc_unregister_component() takes the parent device of the component as a
parameter and then looks up the component based on this. This is a problem if
multiple components are registered for the same parent device. Currently drivers
do not do this, but some drivers register a CPU DAI component and a platform for
the same parent device. This will become a problem once platforms are also made
components. To make sure that snd_soc_unregister_component() will not
accidentally unregister the platform in such a case only consider components
that were registered with snd_soc_register_component(). This is only meant as
short term stopgap solution to be able to continue componentisation. Long term
we'll need something different.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 include/sound/soc.h  | 1 +
 sound/soc/soc-core.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Charles Keepax March 24, 2014, 11:18 a.m. UTC | #1
On Tue, Mar 18, 2014 at 09:02:11AM +0100, Lars-Peter Clausen wrote:
> snd_soc_unregister_component() takes the parent device of the component as a
> parameter and then looks up the component based on this. This is a problem if
> multiple components are registered for the same parent device. Currently drivers
> do not do this, but some drivers register a CPU DAI component and a platform for
> the same parent device. This will become a problem once platforms are also made
> components. To make sure that snd_soc_unregister_component() will not
> accidentally unregister the platform in such a case only consider components
> that were registered with snd_soc_register_component(). This is only meant as
> short term stopgap solution to be able to continue componentisation. Long term
> we'll need something different.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> ---

Do we have any thoughts on what might be a longer term solution
here? Is the parent device actually the right thing to pass to
snd_soc_unregister_component, it feels like that is probably not
going to be enough to accurately identify the component?

Thanks,
Charles
Mark Brown March 24, 2014, 11:33 a.m. UTC | #2
On Mon, Mar 24, 2014 at 11:18:05AM +0000, Charles Keepax wrote:
> On Tue, Mar 18, 2014 at 09:02:11AM +0100, Lars-Peter Clausen wrote:

> > snd_soc_unregister_component() takes the parent device of the component as a
> > parameter and then looks up the component based on this. This is a problem if
> > multiple components are registered for the same parent device. Currently drivers

> Do we have any thoughts on what might be a longer term solution
> here? Is the parent device actually the right thing to pass to
> snd_soc_unregister_component, it feels like that is probably not
> going to be enough to accurately identify the component?

It's not clear to me that in the longer term it makes sense for anything
that is a single struct device to be registering multiple components in
the first place.  In the short term we register things separately so it
makes sense to continue to support that to ease transition but it's not
clear to me that it's a good idea.
Lars-Peter Clausen March 24, 2014, 11:40 a.m. UTC | #3
On 03/24/2014 12:18 PM, Charles Keepax wrote:
> On Tue, Mar 18, 2014 at 09:02:11AM +0100, Lars-Peter Clausen wrote:
>> snd_soc_unregister_component() takes the parent device of the component as a
>> parameter and then looks up the component based on this. This is a problem if
>> multiple components are registered for the same parent device. Currently drivers
>> do not do this, but some drivers register a CPU DAI component and a platform for
>> the same parent device. This will become a problem once platforms are also made
>> components. To make sure that snd_soc_unregister_component() will not
>> accidentally unregister the platform in such a case only consider components
>> that were registered with snd_soc_register_component(). This is only meant as
>> short term stopgap solution to be able to continue componentisation. Long term
>> we'll need something different.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>> ---
>
> Do we have any thoughts on what might be a longer term solution
> here? Is the parent device actually the right thing to pass to
> snd_soc_unregister_component, it feels like that is probably not
> going to be enough to accurately identify the component?
>
> Thanks,
> Charles
>

My plan is to let it take a pointer to the component itself. I'm just not 
too sure about the implementation details yet.

- Lars
Mark Brown March 24, 2014, 11:48 a.m. UTC | #4
On Mon, Mar 24, 2014 at 12:40:49PM +0100, Lars-Peter Clausen wrote:

> My plan is to let it take a pointer to the component itself. I'm just not
> too sure about the implementation details yet.

The question there is why you'd ever want to only unregister a single
thing that the device registered.  devm_ does make this a bit more
academic but still.
Lars-Peter Clausen March 24, 2014, 12:07 p.m. UTC | #5
On 03/24/2014 12:48 PM, Mark Brown wrote:
> On Mon, Mar 24, 2014 at 12:40:49PM +0100, Lars-Peter Clausen wrote:
>
>> My plan is to let it take a pointer to the component itself. I'm just not
>> too sure about the implementation details yet.
>
> The question there is why you'd ever want to only unregister a single
> thing that the device registered.  devm_ does make this a bit more
> academic but still.
>

I you probably wouldn't, but I think it makes the interface cleaner and devm 
doesn't really care whether we store a pointer to the parent device or a 
pointer to the component in the devm data.

- Lars
Mark Brown March 24, 2014, 12:26 p.m. UTC | #6
On Mon, Mar 24, 2014 at 01:07:08PM +0100, Lars-Peter Clausen wrote:
> On 03/24/2014 12:48 PM, Mark Brown wrote:

> >The question there is why you'd ever want to only unregister a single
> >thing that the device registered.  devm_ does make this a bit more
> >academic but still.

> I you probably wouldn't, but I think it makes the interface cleaner and devm
> doesn't really care whether we store a pointer to the parent device or a
> pointer to the component in the devm data.

Yes, of course the other bit is not wanting to register multiple
components in the first place (partly for this reason).

Patch
diff mbox

diff --git a/include/sound/soc.h b/include/sound/soc.h
index a355d0f..f8a79c1 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -662,6 +662,7 @@  struct snd_soc_component {
 	unsigned int active;
 
 	unsigned int ignore_pmdown_time:1; /* pmdown_time is ignored at stop */
+	unsigned int registered_as_component:1;
 
 	struct list_head list;
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 4df9859..1ee3dd6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3885,6 +3885,7 @@  int snd_soc_register_component(struct device *dev,
 	}
 
 	cmpnt->ignore_pmdown_time = true;
+	cmpnt->registered_as_component = true;
 
 	return __snd_soc_register_component(dev, cmpnt, cmpnt_drv, NULL,
 					    dai_drv, num_dai, true);
@@ -3900,7 +3901,7 @@  void snd_soc_unregister_component(struct device *dev)
 	struct snd_soc_component *cmpnt;
 
 	list_for_each_entry(cmpnt, &component_list, list) {
-		if (dev == cmpnt->dev)
+		if (dev == cmpnt->dev && cmpnt->registered_as_component)
 			goto found;
 	}
 	return;