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

[Kai Vehmanen]

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:

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

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

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

Br, Kai