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
