[alsa-devel] [RFC v2 00/11] Enable HDA Codec support on Intel Platforms (Series2)

[Takashi Iwai]

On Tue, 12 Dec 2017 17:37:56 +0100,
Ughreja, Rakesh A wrote:
> 
> 
> 
> >-----Original Message-----
> >From: Takashi Iwai [mailto:tiwai at suse.de]
> >Sent: Tuesday, December 12, 2017 12:19 AM
> >To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>
> >Cc: alsa-devel at alsa-project.org; broonie at kernel.org;
> >liam.r.girdwood at linux.intel.com; pierre-louis.bossart at linux.intel.com; Koul,
> >Vinod <vinod.koul at intel.com>; Patches Audio <patches.audio at 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 at 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
>