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
