On Tue, 12 Dec 2017 17:37:56 +0100, Ughreja, Rakesh A wrote: > > > > >-----Original Message----- > >From: Takashi Iwai [mailto:tiwai@xxxxxxx] > >Sent: Tuesday, December 12, 2017 12:19 AM > >To: Ughreja, Rakesh A <rakesh.a.ughreja@xxxxxxxxx> > >Cc: alsa-devel@xxxxxxxxxxxxxxxx; broonie@xxxxxxxxxx; > >liam.r.girdwood@xxxxxxxxxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; Koul, > >Vinod <vinod.koul@xxxxxxxxx>; Patches Audio <patches.audio@xxxxxxxxx> > >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@xxxxxxxxx> > --- > 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 > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
