Re: [PATCH 3/4] ALSA: hda: Update and expose codec register procedures

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2022-02-08 4:54 PM, Kai Vehmanen wrote:
> Hi,
> 
> On Mon, 7 Feb 2022, Cezary Rojewski wrote:
> 
>> With few changes, snd_hda_codec_register() and its
>> unregister-counterpart can be re-used by ASoC drivers. While at it,
>> provide kernel doc for the exposed functions.
> 
> thanks, there is no doubt room to improve the HDA<->asoc interaction
> still and increase reuse. Some questions:

Thanks for taking time in reviewing the patches, Kai!

>> Due to ALSA-device vs ASoC-component organization differences, new
>> 'snddev_managed' argument is specified allowing for better control over
>> codec registration process.
> 
> Can you maybe clarify this? The existing code to handle the different
> paths is already quite hairy (e.g. code in
> hda_codec.c:snd_hda_codec_dev_free() that does different actions
> depending on whether "codec->core.type == HDA_DEV_LEGACY) is true or
> false), so we'd need to have very clear semantics for the
> "snddev_managed".

It's rather straightforward - ASoC does not provide you with pointer to 
struct snd_card until all components are accounted for. 
snd_hda_codec_device_new() in its current form needs such information 
upfront though. As we want to create only as many DAIs (and other ASoC 
components like links and routes) as needed, codec's ->pcm_list_head 
needs to be built before codec's probing can be completed. But in order 
to have hda codec to fill ->pcm_list_head for, you need to create it. 
And in order to create it you need snd_card pointer which ASoC won't 
give you before you complete component probing.

Typical chicken and egg problem. And that's why additional option is 
provided - it solves that problem.

>> @@ -940,12 +953,13 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
>>   		return PTR_ERR(codec);
>>   	*codecp = codec;
>>   
>> -	return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
>> +	return snd_hda_codec_device_new(bus, card, codec_addr, *codecp, false);
> 
> So this sets snddev_managed to "false" for snd-hda-intel? How is this
> expected to work?

Good catch! It is supposed to be 'true' by default. I checked previous 
'releases' of this patch: between internal RFC v1 -> RFC v2 this mistake 
got in, and probably because I've rebased the driver during that time 
and run into several conflicts which I had to fix manually.

Will update in v2.

>>   int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
>> -			unsigned int codec_addr, struct hda_codec *codec)
>> +			unsigned int codec_addr, struct hda_codec *codec,
>> +			bool snddev_managed)
>>   {
>>   	char component[31];
>>   	hda_nid_t fg;
>> @@ -1020,9 +1034,12 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
>>   		codec->core.subsystem_id, codec->core.revision_id);
>>   	snd_component_add(card, component);
> [...]
>> -	err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
>> -	if (err < 0)
>> -		goto error;
>> +	if (snddev_managed) {
>> +		/* ASoC features component management instead */
>> +		err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
>> +		if (err < 0)
>> +			goto error;
>> +	}
> 
> I might misunderstand semantics of snddev_managed here, but given
> in case of non-ASoC use, snddev_managed is false, this would
> mean we don't call snd_device_new() at all...? I don't see where this is
> added elsewhere in the series, so this would seem to break the flow for
> non-ASoC case.

Same as above.


Regards,
Czarek