Re: [RFT/DONTMERGE] ASoC: devm_snd_soc_register_component fixup

[Jerome Brunet <jbrunet@baylibre.com>]

On Tue 18 Feb 2020 at 07:47, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> Hi Jerome,
>
> On 17.02.2020 19:06, Jerome Brunet wrote:
>> Hi Marek, would you mind trying the following patch. It should target the
>> component removal intead of removing them all. I'd like to comfirm this is
>> your problem before pushing in this direction. Thanks
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> It helps a bit. There is no warning from sysfs, but vc4-drm is still not 
> registered properly:
>
> raspberrypi-firmware soc:firmware: Attached to firmware from 2019-07-09 
> 14:40
> raspberrypi-clk raspberrypi-clk: CPU frequency range: min 600000000, max 
> 1200000000
> vc4_hdmi 3f902000.hdmi: ASoC: CODEC DAI vc4-hdmi-hifi not registered
> vc4_hdmi 3f902000.hdmi: Could not register sound card: -517
> vc4-drm soc:gpu: failed to bind 3f902000.hdmi (ops vc4_hdmi_ops): -517
> vc4-drm soc:gpu: master bind failed: -517

It explains why the original patch of this thread triggered the issue.

The problem is at drivers/gpu/drm/vc4/vc4-hdmi.c:1108
The driver derives the component name directly from the device name and
assume all 3 components have the same name.

This is not ideal:
* We seen that debugfs was already warning about the name collision
* ASoC should be allowed to set the name

With this patch, the component name of the 2nd and 3rd component changed
but the name claimed by the card driver remain unchanged which is why the
card probe defers.

This particular issue can probably be solved with lookup of the
component name instead of a direct derivation. I'll check.

>
>> ---
>>   include/sound/soc.h                   |  1 +
>>   sound/soc/soc-core.c                  |  8 +++++++
>>   sound/soc/soc-devres.c                | 32 ++++++++++++++++++---------
>>   sound/soc/soc-generic-dmaengine-pcm.c |  2 +-
>>   4 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/sound/soc.h b/include/sound/soc.h
>> index f0e4f36f83bf..e5bfe2609110 100644
>> --- a/include/sound/soc.h
>> +++ b/include/sound/soc.h
>> @@ -442,6 +442,7 @@ int snd_soc_add_component(struct device *dev,
>>   		const struct snd_soc_component_driver *component_driver,
>>   		struct snd_soc_dai_driver *dai_drv,
>>   		int num_dai);
>> +void snd_soc_del_component(struct snd_soc_component *component);
>>   int snd_soc_register_component(struct device *dev,
>>   			 const struct snd_soc_component_driver *component_driver,
>>   			 struct snd_soc_dai_driver *dai_drv, int num_dai);
>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
>> index 6a58a8f6e3c4..bf6a64fbfa52 100644
>> --- a/sound/soc/soc-core.c
>> +++ b/sound/soc/soc-core.c
>> @@ -2599,6 +2599,14 @@ static void snd_soc_del_component_unlocked(struct snd_soc_component *component)
>>   	list_del(&component->list);
>>   }
>>   
>> +void snd_soc_del_component(struct snd_soc_component *component)
>> +{
>> +	mutex_lock(&client_mutex);
>> +	snd_soc_del_component_unlocked(component);
>> +	mutex_unlock(&client_mutex);
>> +}
>> +EXPORT_SYMBOL_GPL(snd_soc_del_component);
>> +
>>   int snd_soc_add_component(struct device *dev,
>>   			struct snd_soc_component *component,
>>   			const struct snd_soc_component_driver *component_driver,
>> diff --git a/sound/soc/soc-devres.c b/sound/soc/soc-devres.c
>> index a9ea172a66a7..d5e9e2bed2ce 100644
>> --- a/sound/soc/soc-devres.c
>> +++ b/sound/soc/soc-devres.c
>> @@ -11,7 +11,7 @@
>>   
>>   static void devm_component_release(struct device *dev, void *res)
>>   {
>> -	snd_soc_unregister_component(*(struct device **)res);
>> +	snd_soc_del_component(*(struct snd_soc_component **)res);
>>   }
>>   
>>   /**
>> @@ -28,21 +28,31 @@ int devm_snd_soc_register_component(struct device *dev,
>>   			 const struct snd_soc_component_driver *cmpnt_drv,
>>   			 struct snd_soc_dai_driver *dai_drv, int num_dai)
>>   {
>> -	struct device **ptr;
>> -	int ret;
>> +	struct snd_soc_component *component;
>> +	struct snd_soc_component **ptr;
>> +	int ret = -ENOMEM;
>> +
>> +	component = devm_kzalloc(dev, sizeof(*component), GFP_KERNEL);
>> +	if (!component)
>> +		return -ENOMEM;
>>   
>>   	ptr = devres_alloc(devm_component_release, sizeof(*ptr), GFP_KERNEL);
>>   	if (!ptr)
>> -		return -ENOMEM;
>> +	        goto err_devres;
>>   
>> -	ret = snd_soc_register_component(dev, cmpnt_drv, dai_drv, num_dai);
>> -	if (ret == 0) {
>> -		*ptr = dev;
>> -		devres_add(dev, ptr);
>> -	} else {
>> -		devres_free(ptr);
>> -	}
>> +	ret = snd_soc_add_component(dev, component, cmpnt_drv, dai_drv,
>> +				    num_dai);
>> +	if (ret)
>> +		goto err_add;
>> +
>> +	*ptr = component;
>> +	devres_add(dev, ptr);
>> +	return 0;
>>   
>> +err_add:
>> +	devres_free(ptr);
>> +err_devres:
>> +	devm_kfree(dev, component);
>>   	return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(devm_snd_soc_register_component);
>> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
>> index 2cc25651661c..a33f21ce2d7a 100644
>> --- a/sound/soc/soc-generic-dmaengine-pcm.c
>> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
>> @@ -474,7 +474,7 @@ void snd_dmaengine_pcm_unregister(struct device *dev)
>>   
>>   	pcm = soc_component_to_pcm(component);
>>   
>> -	snd_soc_unregister_component(dev);
>> +	snd_soc_del_component(component);
>>   	dmaengine_pcm_release_chan(pcm);
>>   	kfree(pcm);
>>   }
>
> Best regards