On Tue, 12 Dec 2017 17:37:56 +0100, Ughreja, Rakesh A wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, December 12, 2017 12:19 AM To: Ughreja, Rakesh A rakesh.a.ughreja@intel.com Cc: alsa-devel@alsa-project.org; broonie@kernel.org; liam.r.girdwood@linux.intel.com; pierre-louis.bossart@linux.intel.com; Koul, Vinod vinod.koul@intel.com; Patches Audio patches.audio@intel.com Subject: Re: [RFC v2 00/11] Enable HDA Codec support on Intel Platforms (Series2)
The registration should be triggered from the top-level, i.e. the controller driver, once after all codecs get probed. Otherwise, it becomes racy. The user-space will start poking the device before all the stuff becomes ready.
Thank you very much, I understand now.
That said, at the point when the codec is probed, the card shouldn't be registered yet. The card gets registered after the all components are bound and become ready -- which is done a single snd_card_register() call. At this point, the access points to user-space are provided.
So based on your suggestion when I relooked at the code, I could remove the new function that I added. I could completely reuse the existing function with little change.
Does it look okay ?
Lesser, better :)
Following is the patch, which I tested and it works for me. I will include this in next series but for now it's only for quick reference.
I am calling this API from the hdac_hda codec driver with argument HDA_DEV_ASOC to make sure that it does not register with the bus again. I am not able to call this API from ASoC Platform driver because I don't have snd_card pointer at the time of machine driver probe.
Following is the patch.
Modify snd_hda_codec_new API so that it can work for legacy as well as ASoC codec drivers. The API takes an additional argument dev_type as HDA_DEV_LEGACY or HDA_DEV_ASOC to distinguish the caller.
The idea looks good, but I'd prefer different functions instead of mutating the behavior depending on the flag. i.e. keep snd_hda_codec_new(), and introduce another API (snd_hda_codec_init() or whatever). ASoC driver can call the latter while the legacy driver keeps calling the former.
This would factor out the allocation and snd_hdac_device_init() calls from the old snd_hda_codec_new() to a new snd_hda_codec_new(). snd_hda_codec_init() would do the rest. snd_hda_codec_new() calls snd_hda_codec_init() at the end of the function.
Does this sound good for you?
thanks,
Takashi
Signed-off-by: Rakesh Ughreja rakesh.a.ughreja@intel.com
sound/pci/hda/hda_codec.c | 30 +++++++++++++++++++----------- sound/pci/hda/hda_codec.h | 3 ++- sound/pci/hda/hda_controller.c | 2 +- 3 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 085fd9e..252779c 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -865,7 +865,8 @@ static void snd_hda_codec_dev_release(struct device *dev)
- Returns 0 if successful, or a negative error code.
*/ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
unsigned int codec_addr, struct hda_codec **codecp)
unsigned int codec_addr, struct hda_codec **codecp,unsigned int dev_type){ struct hda_codec *codec; char component[31]; @@ -882,20 +883,27 @@ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card, if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS)) return -EINVAL;
- codec = kzalloc(sizeof(*codec), GFP_KERNEL);
- if (!codec)
return -ENOMEM;
- if (dev_type == HDA_DEV_LEGACY) {
- sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);
- err = snd_hdac_device_init(&codec->core, &bus->core, component,
codec_addr);- if (err < 0) {
kfree(codec);return err;
codec = kzalloc(sizeof(*codec), GFP_KERNEL);if (!codec)return -ENOMEM;sprintf(component, "hdaudioC%dD%d", card->number, codec_addr);err = snd_hdac_device_init(&codec->core, &bus->core, component,codec_addr);if (err < 0) {kfree(codec);return err;}codec->core.type = HDA_DEV_LEGACY;} else {
codec = *codecp;}
codec->core.dev.release = snd_hda_codec_dev_release;
codec->core.type = HDA_DEV_LEGACY; codec->core.exec_verb = codec_exec_verb;
codec->bus = bus;
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index d3099db..92815b7 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -306,7 +306,8 @@ struct hda_codec {
- constructors
*/ int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
unsigned int codec_addr, struct hda_codec **codecp);
unsigned int codec_addr, struct hda_codec **codecp,unsigned int dev_type);int snd_hda_codec_configure(struct hda_codec *codec); int snd_hda_codec_update_widgets(struct hda_codec *codec);
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index d1eb148..ec887d7 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -1318,7 +1318,7 @@ int azx_probe_codecs(struct azx *chip, unsigned int max_slots) for (c = 0; c < max_slots; c++) { if ((bus->codec_mask & (1 << c)) & chip->codec_probe_mask) { struct hda_codec *codec;
err = snd_hda_codec_new(&chip->bus, chip->card, c, &codec);
err = snd_hda_codec_new(&chip->bus, chip->card, c, &codec, HDA_DEV_LEGACY); if (err < 0) continue; codec->jackpoll_interval = get_jackpoll_interval(chip);-- 2.7.4