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@xxxxxxxxx>
> ---
> 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
>
