Re: [PATCH 2/4] ALSA: hda: Make snd_hda_codec_device_init() the only codec constructor

[Takashi Iwai <tiwai@suse.de>]

On Wed, 20 Jul 2022 15:06:20 +0200,
Cezary Rojewski wrote:
> 
> Refactor snd_hdac_ext_bus_device_init() so that it makes use of
> snd_hda_codec_device_init() to create and initialize new codec device.
> Causes the latter to become the sole codec device constructor.
> 
> Users of the refactored function are updated accordingly and now also
> take responsibility for assigning driver's private data as that task is
> no longer performed by hdac_hda_dev_probe().

Hrm, this doesn't look really right.  It means you'll introduce a hard
dependency chain in a reverse order: snd-hda-ext-core ->
snd-hda-codec.

Originally, the ext bus code was written completely independent from
the legacy HD-audio implementations, and hdac-hda driver was a kind of
wrapper / bridge for the legacy codec over the ext bus.  If we want
change this rule and make the legacy HD-audio codec always tied with
the ext bus, a likely better way would be to call
snd_hda_codec_device_init() in the caller's side (e.g. skl or sof),
then pass the newly created codec object to
snd_hdac_ext_bus_device_init() for further initialization.


thanks,

Takashi

> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hdaudio_ext.h     |  4 ++--
>  sound/hda/ext/hdac_ext_bus.c    | 34 +++++++++++++++------------------
>  sound/soc/intel/skylake/skl.c   | 24 ++++++++++-------------
>  sound/soc/sof/intel/hda-codec.c | 29 ++++++++++++----------------
>  4 files changed, 39 insertions(+), 52 deletions(-)
> 
> diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
> index d26234f9ee46..25c7b13db278 100644
> --- a/include/sound/hdaudio_ext.h
> +++ b/include/sound/hdaudio_ext.h
> @@ -11,8 +11,8 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
>  		      const struct hdac_ext_bus_ops *ext_ops);
>  
>  void snd_hdac_ext_bus_exit(struct hdac_bus *bus);
> -int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
> -				struct hdac_device *hdev, int type);
> +struct hda_codec *
> +snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type);
>  void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev);
>  void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus);
>  
> diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
> index 765c40a6ccba..bd3c7124aca1 100644
> --- a/sound/hda/ext/hdac_ext_bus.c
> +++ b/sound/hda/ext/hdac_ext_bus.c
> @@ -12,6 +12,7 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> +#include <sound/hda_codec.h>
>  #include <sound/hdaudio_ext.h>
>  
>  MODULE_DESCRIPTION("HDA extended core");
> @@ -67,39 +68,34 @@ static void default_release(struct device *dev)
>  
>  /**
>   * snd_hdac_ext_bus_device_init - initialize the HDA extended codec base device
> - * @bus: hdac bus to attach to
> + * @bus: hda bus to attach to
>   * @addr: codec address
> - * @hdev: hdac device to init
>   * @type: codec type (HDAC_DEV_*) to use for this device
>   *
> - * Returns zero for success or a negative error code.
> + * Returns pointer to newly created codec or ERR_PTR.
>   */
> -int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
> -				 struct hdac_device *hdev, int type)
> +struct hda_codec *
> +snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr, int type)
>  {
> -	char name[15];
> +	struct hda_codec *codec;
>  	int ret;
>  
> -	hdev->bus = bus;
> -
> -	snprintf(name, sizeof(name), "ehdaudio%dD%d", bus->idx, addr);
> -
> -	ret  = snd_hdac_device_init(hdev, bus, name, addr);
> -	if (ret < 0) {
> +	codec = snd_hda_codec_device_init(to_hda_bus(bus), addr, "ehdaudio%dD%d", bus->idx, addr);
> +	if (IS_ERR(codec)) {
>  		dev_err(bus->dev, "device init failed for hdac device\n");
> -		return ret;
> +		return codec;
>  	}
> -	hdev->type = type;
> -	hdev->dev.release = default_release;
> +	codec->core.type = type;
> +	codec->core.dev.release = default_release;
>  
> -	ret = snd_hdac_device_register(hdev);
> +	ret = snd_hdac_device_register(&codec->core);
>  	if (ret) {
>  		dev_err(bus->dev, "failed to register hdac device\n");
> -		snd_hdac_ext_bus_device_exit(hdev);
> -		return ret;
> +		snd_hdac_ext_bus_device_exit(&codec->core);
> +		return ERR_PTR(ret);
>  	}
>  
> -	return 0;
> +	return codec;
>  }
>  EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init);
>  
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 7e573a887118..5637292c2aa9 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -700,9 +700,8 @@ static int probe_codec(struct hdac_bus *bus, int addr)
>  	struct skl_dev *skl = bus_to_skl(bus);
>  #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC)
>  	struct hdac_hda_priv *hda_codec;
> -	int err;
>  #endif
> -	struct hdac_device *hdev;
> +	struct hda_codec *codec;
>  
>  	mutex_lock(&bus->cmd_mutex);
>  	snd_hdac_bus_send_cmd(bus, cmd);
> @@ -718,25 +717,22 @@ static int probe_codec(struct hdac_bus *bus, int addr)
>  	if (!hda_codec)
>  		return -ENOMEM;
>  
> -	hda_codec->codec->bus = skl_to_hbus(skl);
> -	hdev = &hda_codec->codec->core;
> +	codec = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC);
> +	if (IS_ERR(codec))
> +		return PTR_ERR(codec);
>  
> -	err = snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
> -	if (err < 0)
> -		return err;
> +	hda_codec->codec = codec;
> +	dev_set_drvdata(&codec->core.dev, hda_codec);
>  
>  	/* use legacy bus only for HDA codecs, idisp uses ext bus */
>  	if ((res & 0xFFFF0000) != IDISP_INTEL_VENDOR_ID) {
> -		hdev->type = HDA_DEV_LEGACY;
> -		load_codec_module(hda_codec->codec);
> +		codec->core.type = HDA_DEV_LEGACY;
> +		load_codec_module(codec);
>  	}
>  	return 0;
>  #else
> -	hdev = devm_kzalloc(&skl->pci->dev, sizeof(*hdev), GFP_KERNEL);
> -	if (!hdev)
> -		return -ENOMEM;
> -
> -	return snd_hdac_ext_bus_device_init(bus, addr, hdev, HDA_DEV_ASOC);
> +	codec = snd_hdac_ext_bus_device_init(bus, addr, HDA_DEV_ASOC);
> +	return PTR_ERR_OR_ZERO(codec);
>  #endif /* CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC */
>  }
>  
> diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c
> index de7c53224ac3..7c3ea4a12d63 100644
> --- a/sound/soc/sof/intel/hda-codec.c
> +++ b/sound/soc/sof/intel/hda-codec.c
> @@ -115,11 +115,10 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
>  {
>  #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
>  	struct hdac_hda_priv *hda_priv;
> -	struct hda_codec *codec;
>  	int type = HDA_DEV_LEGACY;
>  #endif
>  	struct hda_bus *hbus = sof_to_hbus(sdev);
> -	struct hdac_device *hdev;
> +	struct hda_codec *codec;
>  	u32 hda_cmd = (address << 28) | (AC_NODE_ROOT << 20) |
>  		(AC_VERB_PARAMETERS << 8) | AC_PAR_VENDOR_ID;
>  	u32 resp = -1;
> @@ -142,20 +141,19 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
>  	if (!hda_priv)
>  		return -ENOMEM;
>  
> -	hda_priv->codec->bus = hbus;
> -	hdev = &hda_priv->codec->core;
> -	codec = hda_priv->codec;
> -
>  	/* only probe ASoC codec drivers for HDAC-HDMI */
>  	if (!hda_codec_use_common_hdmi && (resp & 0xFFFF0000) == IDISP_VID_INTEL)
>  		type = HDA_DEV_ASOC;
>  
> -	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, type);
> -	if (ret < 0)
> -		return ret;
> +	codec = snd_hdac_ext_bus_device_init(&hbus->core, address, type);
> +	if (IS_ERR(codec))
> +		return PTR_ERR(codec);
> +
> +	hda_priv->codec = codec;
> +	dev_set_drvdata(&codec->core.dev, hda_priv);
>  
>  	if ((resp & 0xFFFF0000) == IDISP_VID_INTEL) {
> -		if (!hdev->bus->audio_component) {
> +		if (!hbus->core.audio_component) {
>  			dev_dbg(sdev->dev,
>  				"iDisp hw present but no driver\n");
>  			ret = -ENOENT;
> @@ -181,15 +179,12 @@ static int hda_codec_probe(struct snd_sof_dev *sdev, int address,
>  
>  out:
>  	if (ret < 0) {
> -		snd_hdac_device_unregister(hdev);
> -		put_device(&hdev->dev);
> +		snd_hdac_device_unregister(&codec->core);
> +		put_device(&codec->core.dev);
>  	}
>  #else
> -	hdev = devm_kzalloc(sdev->dev, sizeof(*hdev), GFP_KERNEL);
> -	if (!hdev)
> -		return -ENOMEM;
> -
> -	ret = snd_hdac_ext_bus_device_init(&hbus->core, address, hdev, HDA_DEV_ASOC);
> +	codec = snd_hdac_ext_bus_device_init(&hbus->core, address, HDA_DEV_ASOC);
> +	ret = PTR_ERR_OR_ZERO(codec);
>  #endif
>  
>  	return ret;
> -- 
> 2.25.1
>