| Message ID | 20220207114906.3759800-4-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda: Expose codec organization functions | expand |
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
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
On Tue, 08 Feb 2022 17:34:47 +0100, Cezary Rojewski wrote: > > 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. Hmm, but snd_hda_codec_device_new() also does the actual hardware initialization including the power-up sequence, and there is a call snd_component_add() with the card instance. How the function would work properly before the card instantiation? You seem to have handled only snd_device_new() and not touching other code where the card (or the hardware access) is involved. If the purpose is to get the fields of snd_hda_codec to be initialized, those init code can be factored out to another function, so that it works certainly without card. Takashi
>>> 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. New capabilities are always welcome, what I am missing is how important your suggestion is for end users or OEMs. The main reason for using a DSP-based driver on a HDaudio Intel platform is when DMICs connected to the PCH, since this link cannot be handled by the legacy driver. Those sort of form factors typically have analog playback and capture only. UCM exposes only analog playback and capture. Desktops without DMICs generally rely on the legacy driver and have different sorts of problems with jack retasking and other time-consuming problems. Exposing additional DAIs in a DSP-based driver when supported by the codec is a good idea on paper, but it's far from straightforward. a) it assumes that there are indeed additional DAIs. Is this really the case on the SKL/KBL devices you are targeting? It's not on newer CML..ADL devices we've been supporting with SOF. b) it assumes that what is exposed by the codec actually does work - and the number of quirks tells us to be cautious. We routinely get reports that even Intel NUCs don't have the right quirks in the kernel... c) and then it creates new problems for the topology that may expose paths that may or may not be supported by the codec. I am not aware of any existing APIs to take down or enable a path conditionally - though it's been a problem for a very long time for SSP and DMIC enablement that are not always correctly described, and any suggestions to improve this limitation would have my full support. FWIW, in our latest SOF work we went back to handling ONE DAI with analog playback and capture and ditched the 'digital playback'. Trying to do more led us to too many issues of 'works on platform A' and 'does not work on platform B', and sometimes with different answers depending on which BIOS version is used. IMHO what's really problematic for HDaudio is the support for amplifiers located behind the HDaudio codec, for which we more often than not are missing the I2C configuration sequences. Suspend-resume is a recurring problem as well. I am not saying no for the sake of saying no, I have just never heard of anyone complain about restrictions on the number of DAIs in the HDaudio world.
On 2022-02-08 6:14 PM, Takashi Iwai wrote: > Hmm, but snd_hda_codec_device_new() also does the actual hardware > initialization including the power-up sequence, and there is a call > snd_component_add() with the card instance. How the function would > work properly before the card instantiation? You seem to have handled > only snd_device_new() and not touching other code where the card (or > the hardware access) is involved. > > If the purpose is to get the fields of snd_hda_codec to be > initialized, those init code can be factored out to another function, > so that it works certainly without card. I was anticipating snd_hda_codec_device_init() related question so much and was so ready for it that I answered like an autobot, regardless of fact that people were not asking about snd_hda_codec_device_init() vs snd_hda_codec_device_new() directly and that's not even the patch where the change is added. And I wasn't even drinking coffee for the past few days.. Meh.. In regard to snddev_managed, If I'm not mistaken, the problem is connected to the 'dev_ops' (.dev_register, .dev_free) provided for snd_device_new(). To have basically 0% custom HDA logic in our code, rely solely on code found in sound/hda and sound/pci/hda ability to control when snd_hda_codec_register() and snd_hda_codec_unregister() is called was needed. snd_soc_bind_card() invokes snd_card_register() which in consequence leads to snd_device_register_all() and that to automatic ->dev_register call. That call involves PM operations, and at that moment, codec is not ready for it. In the end, ability to call these in correct order allowed all codecs that communicate with avs-driver to remain on HDA_DEV_LEGACY level. To answer the remaining question found in your message: The purpose of the change was not to get the fields of snd_hda_codec out of the function and have them initialized elsewhere. All other operations found in the snd_hda_codec_device_new() are required and their ordering is not problematic so they have been left untouched. Regards, Czarek
On 2022-02-09 11:21 AM, Cezary Rojewski wrote: > snd_soc_bind_card() invokes snd_card_register() which in consequence > leads to snd_device_register_all() and that to automatic ->dev_register > call. That call involves PM operations, and at that moment, codec is not > ready for it. By that I mean: bus driver (here, avs-driver) has some saying on the PM matter too e.g.: sets their (codecs) status to suspended via pm_runtime_set_suspended() so the bus runtime suspend is not blocked by codecs that could possibly never complete their probing. Regards, Czarek
On 2022-02-08 6:46 PM, Pierre-Louis Bossart wrote: > New capabilities are always welcome, what I am missing is how important > your suggestion is for end users or OEMs. Users won't notice and why would we like them to notice? The intended behavior is 1:1 with hda legacy one. > The main reason for using a DSP-based driver on a HDaudio Intel platform > is when DMICs connected to the PCH, since this link cannot be handled by > the legacy driver. Those sort of form factors typically have analog > playback and capture only. UCM exposes only analog playback and capture. > > Desktops without DMICs generally rely on the legacy driver and have > different sorts of problems with jack retasking and other time-consuming > problems. I believe we're all aware why and when DSP-drivers are chosen over HDA legacy driver. That's orthogonal to the current subject. > Exposing additional DAIs in a DSP-based driver when supported by the > codec is a good idea on paper, but it's far from straightforward. The outcome is reduction in number of DAIs exposed for basically all codecs except for HDMIs as DSP-based solutions are hardcoding 3-DAIs. Here, the intended behavior is to be 1:1 with hda legacy behavior, not hardcoding. > a) it assumes that there are indeed additional DAIs. Is this really the > case on the SKL/KBL devices you are targeting? It's not on newer > CML..ADL devices we've been supporting with SOF. It does not _assume_, it _reads_. The outcome is reduction of DAIs exposed rather than hardcoding Analog/Alt Analog/Digital endpoints what is currently the case. > b) it assumes that what is exposed by the codec actually does work - and > the number of quirks tells us to be cautious. We routinely get reports > that even Intel NUCs don't have the right quirks in the kernel... Patches just expose functions. Logic stays the same. As it is sound/pci/hda code we're talking about, that code is quirk-friendly - it contains several "patches" and quirks already. > c) and then it creates new problems for the topology that may expose > paths that may or may not be supported by the codec. I am not aware of > any existing APIs to take down or enable a path conditionally - though > it's been a problem for a very long time for SSP and DMIC enablement > that are not always correctly described, and any suggestions to improve > this limitation would have my full support. The default for topology is to expose just single analog endpoint as majority of codecs expose nothing else. Moreover, having just 1 FE defined in topology can be used to limit the usecases where we know codec says it exposes more but in fact these endpoints aren't functional. For specific codecs that expose more, a specific topology can be provided just like it's done for any DSP-driver today. > FWIW, in our latest SOF work we went back to handling ONE DAI with > analog playback and capture and ditched the 'digital playback'. Trying > to do more led us to too many issues of 'works on platform A' and 'does > not work on platform B', and sometimes with different answers depending > on which BIOS version is used. > > IMHO what's really problematic for HDaudio is the support for amplifiers > located behind the HDaudio codec, for which we more often than not are > missing the I2C configuration sequences. Suspend-resume is a recurring > problem as well. > > I am not saying no for the sake of saying no, I have just never heard of > anyone complain about restrictions on the number of DAIs in the HDaudio > world. I believe our goals align. Rather than hardcoding Analog/Alt Analog/Digital endpoints as it's done currently, when codec most of the time do not have them working anyway, rely on behavior found in sound/hda and sound/pci/hda. If there are some problems there it's win:win for us and legacy driver. Fix one spot, have both drivers happy. Regards, Czarek
On Wed, 09 Feb 2022 11:38:23 +0100, Cezary Rojewski wrote: > > On 2022-02-09 11:21 AM, Cezary Rojewski wrote: > > snd_soc_bind_card() invokes snd_card_register() which in consequence > > leads to snd_device_register_all() and that to automatic > > ->dev_register call. That call involves PM operations, and at that > > moment, codec is not ready for it. > By that I mean: bus driver (here, avs-driver) has some saying on the > PM matter too e.g.: sets their (codecs) status to suspended via > pm_runtime_set_suspended() so the bus runtime suspend is not blocked > by codecs that could possibly never complete their probing. Hm, but the runtime PM of hda_codec device is suppressed at first in snd_hda_codec_device_new() via pm_runtime_forbid(), i.e. the codec should be kept running until it reaches to snd_hda_codec_register(), it shouldn't be runtime-suspended beforehand. I might overlook something, but it's a bit hard to follow... thanks, Takashi
On 2/9/22 06:05, Cezary Rojewski wrote: >> FWIW, in our latest SOF work we went back to handling ONE DAI with >> analog playback and capture and ditched the 'digital playback'. Trying >> to do more led us to too many issues of 'works on platform A' and 'does >> not work on platform B', and sometimes with different answers depending >> on which BIOS version is used. >> >> IMHO what's really problematic for HDaudio is the support for amplifiers >> located behind the HDaudio codec, for which we more often than not are >> missing the I2C configuration sequences. Suspend-resume is a recurring >> problem as well. >> >> I am not saying no for the sake of saying no, I have just never heard of >> anyone complain about restrictions on the number of DAIs in the HDaudio >> world. > > > I believe our goals align. Rather than hardcoding Analog/Alt > Analog/Digital endpoints as it's done currently, when codec most of the > time do not have them working anyway, rely on behavior found in > sound/hda and sound/pci/hda. If there are some problems there it's > win:win for us and legacy driver. Fix one spot, have both drivers happy. I don't quite see the alignment: the only thing that we've seen work reliably and that we do need is the analog part, and we want to get rid of the other paths which we can't test in the first place. I must admit I don't recall why we bothered to expose those alt analog and digital paths back in 2018, they have not been used nor tested by anyone. Now if this patch helped to make sure we do indeed have an analog path, that'd be fine. That would indeed avoid a hard-coded decision and report configuration errors. I just don't see what exposing additional paths brings, they never worked reliably for us when we tried. IIRC half of our team gets an error with the Digital playback stream on Up Extreme and the other half can play just fine - albeit with no connector to actually see what the output is. FWIW there are many things made possible by HDaudio, in practice we often have to limit ourselves to what is known to work and what is needed by end-products. You could spend all your time chasing configuration issues, missing NIDS and bad verbs. Been there, done that.
On 2022-02-09 5:08 PM, Pierre-Louis Bossart wrote: > > > On 2/9/22 06:05, Cezary Rojewski wrote: >> I believe our goals align. Rather than hardcoding Analog/Alt >> Analog/Digital endpoints as it's done currently, when codec most of the >> time do not have them working anyway, rely on behavior found in >> sound/hda and sound/pci/hda. If there are some problems there it's >> win:win for us and legacy driver. Fix one spot, have both drivers happy. > > I don't quite see the alignment: the only thing that we've seen work > reliably and that we do need is the analog part, and we want to get rid > of the other paths which we can't test in the first place. I must admit > I don't recall why we bothered to expose those alt analog and digital > paths back in 2018, they have not been used nor tested by anyone. > > Now if this patch helped to make sure we do indeed have an analog path, > that'd be fine. That would indeed avoid a hard-coded decision and report > configuration errors. > > I just don't see what exposing additional paths brings, they never > worked reliably for us when we tried. IIRC half of our team gets an > error with the Digital playback stream on Up Extreme and the other half > can play just fine - albeit with no connector to actually see what the > output is. > > FWIW there are many things made possible by HDaudio, in practice we > often have to limit ourselves to what is known to work and what is > needed by end-products. You could spend all your time chasing > configuration issues, missing NIDS and bad verbs. Been there, done that. Again, we are not trying to force-expose stuff which does not work. In majority of the cases, non-HDMI codecs we're dealing with notify about just single analog endpoint. For now, it's 100% of the cases, but I'm aware of fact that RVPs and a dozen of Dell/Lenovo/Acer laptops do not equal to entire market. Remember that you can always use topology to "gate" userspace from streaming through endpoints which we do not work. And right now, we are working with topologies supporting single endpoint for non-HDMI devices. So, this is a clear upgrade when compared to Analog/Alt Analog/Digitalh-hardcoded configuration used currently. That's on top of aligning with hda legacy behavior. Regards, Czarek
> Again, we are not trying to force-expose stuff which does not work. In > majority of the cases, non-HDMI codecs we're dealing with notify about > just single analog endpoint. For now, it's 100% of the cases, but I'm > aware of fact that RVPs and a dozen of Dell/Lenovo/Acer laptops do not > equal to entire market. We are in agreement, but since we don't have any ability to test those alt/digital parts my take is 'don't even bother about them'. > Remember that you can always use topology to "gate" userspace from > streaming through endpoints which we do not work. And right now, we are > working with topologies supporting single endpoint for non-HDMI devices. May I ask how you 'gate' parts of a topology? Or did you mean that you use different topologies? > > So, this is a clear upgrade when compared to Analog/Alt > Analog/Digitalh-hardcoded configuration used currently. That's on top of > aligning with hda legacy behavior. Agree, but it still leaves the door open to exposing those paths which may or may not work - no one has ever tested them. It's better IMHO to only allow for the analog path. If we can detect the presence of this path, great.
On 2022-02-11 4:23 PM, Pierre-Louis Bossart wrote: > >> Again, we are not trying to force-expose stuff which does not work. In >> majority of the cases, non-HDMI codecs we're dealing with notify about >> just single analog endpoint. For now, it's 100% of the cases, but I'm >> aware of fact that RVPs and a dozen of Dell/Lenovo/Acer laptops do not >> equal to entire market. > > We are in agreement, but since we don't have any ability to test those > alt/digital parts my take is 'don't even bother about them'. > >> Remember that you can always use topology to "gate" userspace from >> streaming through endpoints which we do not work. And right now, we are >> working with topologies supporting single endpoint for non-HDMI devices. > > May I ask how you 'gate' parts of a topology? Or did you mean that you > use different topologies? > >> >> So, this is a clear upgrade when compared to Analog/Alt >> Analog/Digitalh-hardcoded configuration used currently. That's on top of >> aligning with hda legacy behavior. > > Agree, but it still leaves the door open to exposing those paths which > may or may not work - no one has ever tested them. It's better IMHO to > only allow for the analog path. If we can detect the presence of this > path, great. You may define topology with lower number of FE, thus limiting number of options available to user, possibly shielding them from untested scenarios. E.g.: Have a 3-DAI HDMI codec yet provide 2-FE topology file for it because you know 3rd endpoint is faulty. Only two devices will show up in userspace effectively making users unable to play on the 3rd one. Again, that's a default - expose fewer endpoints (here, just single analog one) for non-HDMI codecs. Of course, there is small number of users who may want to stream on their Alt Analog/Digital endpoints. We do not want 'Conexant story' to repeat all over again. So, for them, non-default (topology) could be provided to enable streaming on whatever they want. The chosen approach does not hinder either of the sides. Regards, Czarek
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 5e3cbcca42f0..f74abc13414f 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -312,9 +312,12 @@ snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr, int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, unsigned int codec_addr, struct hda_codec **codecp); 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); int snd_hda_codec_configure(struct hda_codec *codec); int snd_hda_codec_update_widgets(struct hda_codec *codec); +void snd_hda_codec_register(struct hda_codec *codec); +void snd_hda_codec_unregister(struct hda_codec *codec); /* * low level functions diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 3787060ad77f..4406b7983c07 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -813,7 +813,12 @@ void snd_hda_codec_display_power(struct hda_codec *codec, bool enable) snd_hdac_display_power(&codec->bus->core, codec->addr, enable); } -/* also called from hda_bind.c */ +/** + * snd_hda_codec_register - Finalize codec initialization + * @codec: codec device to register + * + * Also called from hda_bind.c + */ void snd_hda_codec_register(struct hda_codec *codec) { if (codec->registered) @@ -826,6 +831,7 @@ void snd_hda_codec_register(struct hda_codec *codec) codec->registered = 1; } } +EXPORT_SYMBOL_GPL(snd_hda_codec_register); static int snd_hda_codec_dev_register(struct snd_device *device) { @@ -833,10 +839,12 @@ static int snd_hda_codec_dev_register(struct snd_device *device) return 0; } -static int snd_hda_codec_dev_free(struct snd_device *device) +/** + * snd_hda_codec_unregister - Unregister specified codec device + * @codec: codec device to unregister + */ +void snd_hda_codec_unregister(struct hda_codec *codec) { - struct hda_codec *codec = device->device_data; - codec->in_freeing = 1; /* * snd_hda_codec_device_new() is used by legacy HDA and ASoC driver. @@ -853,7 +861,12 @@ static int snd_hda_codec_dev_free(struct snd_device *device) */ if (codec->core.type == HDA_DEV_LEGACY) put_device(hda_codec_dev(codec)); +} +EXPORT_SYMBOL_GPL(snd_hda_codec_unregister); +static int snd_hda_codec_dev_free(struct snd_device *device) +{ + snd_hda_codec_unregister(device->device_data); return 0; } @@ -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); } EXPORT_SYMBOL_GPL(snd_hda_codec_new); 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; + } /* PM runtime needs to be enabled later after binding codec */ pm_runtime_forbid(&codec->core.dev); diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 8621f576446b..4c52dfb615bc 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -135,7 +135,6 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name, #define snd_hda_add_vmaster(codec, name, tlv, followers, suffix, access) \ __snd_hda_add_vmaster(codec, name, tlv, followers, suffix, true, access, NULL) int snd_hda_codec_reset(struct hda_codec *codec); -void snd_hda_codec_register(struct hda_codec *codec); void snd_hda_codec_cleanup_for_unbind(struct hda_codec *codec); void snd_hda_codec_disconnect_pcms(struct hda_codec *codec); diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index de5955db0a5f..667f3df239c7 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -413,7 +413,7 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component) HDA_CODEC_IDX_CONTROLLER, true); ret = snd_hda_codec_device_new(hcodec->bus, component->card->snd_card, - hdev->addr, hcodec); + hdev->addr, hcodec, true); if (ret < 0) { dev_err(&hdev->dev, "failed to create hda codec %d\n", ret); goto error_no_pm;
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. Due to ALSA-device vs ASoC-component organization differences, new 'snddev_managed' argument is specified allowing for better control over codec registration process. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- include/sound/hda_codec.h | 5 ++++- sound/pci/hda/hda_codec.c | 35 ++++++++++++++++++++++++++--------- sound/pci/hda/hda_local.h | 1 - sound/soc/codecs/hdac_hda.c | 2 +- 4 files changed, 31 insertions(+), 12 deletions(-)