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