Re: [PATCH] ASoC: Intel: Unify HDAudio-ext bus initialization

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 10/18/21 2:21 PM, Cezary Rojewski wrote:
> HDAudio-extended bus initialization parts are scattered throughout Intel
> ADSP drivers code. Gather them up in snd_hda_ext_bus_init() to provide
> unified initialization point.

What unification are we talking about?

The code for HDaudio differs a great deal between the two Intel drivers,
and specifically the 'nocodec' mode in SOF does not rely on this
library, so there's no burning desire on my side to add this dependency
when we carefully tried to avoid it to use the DMA parts only.

I would add we recently looked at the code and the coupling/decoupling
in this library seems questionable if not broken.

edit: this patch also seems to add a layer of indirection through a
'core' layer, not sure where this is going at all. I must be missing
something.

CC: Ranjani and Kai.

> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/hdaudio_ext.h   |  9 ++++++---
>  sound/hda/ext/hdac_ext_bus.c  | 29 ++++++++++++++++++++---------
>  sound/soc/intel/skylake/skl.c |  9 +--------
>  sound/soc/sof/intel/hda-bus.c | 16 +++++++++-------
>  sound/soc/sof/intel/hda.c     |  5 +++--
>  sound/soc/sof/intel/hda.h     |  3 ++-
>  6 files changed, 41 insertions(+), 30 deletions(-)
> 
> diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
> index 375581634143..d1f6e9f7c057 100644
> --- a/include/sound/hdaudio_ext.h
> +++ b/include/sound/hdaudio_ext.h
> @@ -2,11 +2,14 @@
>  #ifndef __SOUND_HDAUDIO_EXT_H
>  #define __SOUND_HDAUDIO_EXT_H
>  
> +#include <linux/pci.h>
>  #include <sound/hdaudio.h>
> +#include <sound/hda_codec.h>
>  
> -int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
> -		      const struct hdac_bus_ops *ops,
> -		      const struct hdac_ext_bus_ops *ext_ops);
> +int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
> +			 const struct hdac_bus_ops *ops,
> +			 const struct hdac_ext_bus_ops *ext_ops,
> +			 const char *model);
>  
>  void snd_hdac_ext_bus_exit(struct hdac_bus *bus);
>  int snd_hdac_ext_bus_device_init(struct hdac_bus *bus, int addr,
> diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
> index 765c40a6ccba..a89e2e80ea4c 100644
> --- a/sound/hda/ext/hdac_ext_bus.c
> +++ b/sound/hda/ext/hdac_ext_bus.c
> @@ -10,15 +10,17 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/pci.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <sound/hdaudio_ext.h>
> +#include <sound/hda_codec.h>
>  
>  MODULE_DESCRIPTION("HDA extended core");
>  MODULE_LICENSE("GPL v2");
>  
>  /**
> - * snd_hdac_ext_bus_init - initialize a HD-audio extended bus
> + * snd_hda_ext_bus_init - initialize a HD-audio extended bus
>   * @bus: the pointer to HDAC bus object
>   * @dev: device pointer
>   * @ops: bus verb operators
> @@ -26,28 +28,37 @@ MODULE_LICENSE("GPL v2");
>   *
>   * Returns 0 if successful, or a negative error code.
>   */
> -int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
> -			const struct hdac_bus_ops *ops,
> -			const struct hdac_ext_bus_ops *ext_ops)
> +int snd_hda_ext_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
> +			 const struct hdac_bus_ops *ops,
> +			 const struct hdac_ext_bus_ops *ext_ops,
> +			 const char *model)

missing kernel doc update?

>  {
> +	struct hdac_bus *base = &bus->core;
>  	int ret;
>  
> -	ret = snd_hdac_bus_init(bus, dev, ops);
> +	ret = snd_hdac_bus_init(base, &pdev->dev, ops);
>  	if (ret < 0)
>  		return ret;
>  
> -	bus->ext_ops = ext_ops;
> +	base->ext_ops = ext_ops;
>  	/* FIXME:
>  	 * Currently only one bus is supported, if there is device with more
>  	 * buses, bus->idx should be greater than 0, but there needs to be a
>  	 * reliable way to always assign same number.
>  	 */
> -	bus->idx = 0;
> -	bus->cmd_dma_state = true;
> +	base->idx = 0;
> +	base->cmd_dma_state = true;
> +	base->use_posbuf = 1;
> +	base->bdl_pos_adj = 0;
> +	base->sync_write = 1;
> +	bus->pci = pdev;
> +	bus->modelname = model;
> +	bus->mixer_assigned = -1;
> +	mutex_init(&bus->prepare_mutex);
>  
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init);
> +EXPORT_SYMBOL_GPL(snd_hda_ext_bus_init);
>  
>  /**
>   * snd_hdac_ext_bus_exit - clean up a HD-audio extended bus
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 5b1a15e39912..95de41d14e56 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -886,16 +886,9 @@ static int skl_create(struct pci_dev *pci,
>  #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC)
>  	ext_ops = snd_soc_hdac_hda_get_ops();
>  #endif
> -	snd_hdac_ext_bus_init(bus, &pci->dev, NULL, ext_ops);
> -	bus->use_posbuf = 1;
> +	snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
>  	skl->pci = pci;
>  	INIT_WORK(&skl->probe_work, skl_probe_work);
> -	bus->bdl_pos_adj = 0;
> -
> -	mutex_init(&hbus->prepare_mutex);
> -	hbus->pci = pci;
> -	hbus->mixer_assigned = -1;
> -	hbus->modelname = "sklbus";
>  
>  	*rskl = skl;
>  
> diff --git a/sound/soc/sof/intel/hda-bus.c b/sound/soc/sof/intel/hda-bus.c
> index 30025d3c16b6..5d5081f80e88 100644
> --- a/sound/soc/sof/intel/hda-bus.c
> +++ b/sound/soc/sof/intel/hda-bus.c
> @@ -8,6 +8,7 @@
>  // Authors: Keyon Jie <yang.jie@linux.intel.com>
>  
>  #include <linux/io.h>
> +#include <linux/pci.h>
>  #include <sound/hdaudio.h>
>  #include <sound/hda_i915.h>
>  #include "../sof-priv.h"
> @@ -53,24 +54,25 @@ static const struct hdac_bus_ops bus_core_ops = {
>  /*
>   * This can be used for both with/without hda link support.
>   */
> -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev)
> +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
> +		      const char *model)
>  {
>  #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
> -	snd_hdac_ext_bus_init(bus, dev, &bus_core_ops, sof_hda_ext_ops);
> +	snd_hda_ext_bus_init(bus, pdev, &bus_core_ops, sof_hda_ext_ops, model);
>  #else /* CONFIG_SND_SOC_SOF_HDA */
>  	memset(bus, 0, sizeof(*bus));
> -	bus->dev = dev;
> +	bus->core.dev = &pdev->dev;
>  
> -	INIT_LIST_HEAD(&bus->stream_list);
> +	INIT_LIST_HEAD(&bus->core.stream_list);
>  
> -	bus->irq = -1;
> +	bus->core.irq = -1;
>  
>  	/*
>  	 * There is only one HDA bus atm. keep the index as 0.
>  	 * Need to fix when there are more than one HDA bus.
>  	 */
> -	bus->idx = 0;
> +	bus->core.idx = 0;

why is this level of indirection through 'core' needed in this code
which doesn't use the hdaudio-ext library?

the changes here have nothing to do with snd_hda_ext_bus_init()?

> -	spin_lock_init(&bus->reg_lock);
> +	spin_lock_init(&bus->core.reg_lock);

same, we've just added reg_lock everywhere, why use a different one

>  #endif /* CONFIG_SND_SOC_SOF_HDA */
>  }
> diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
> index fbc2421c77f8..03a68d286c7c 100644
> --- a/sound/soc/sof/intel/hda.c
> +++ b/sound/soc/sof/intel/hda.c
> @@ -609,8 +609,9 @@ static int hda_init(struct snd_sof_dev *sdev)
>  	bus = sof_to_bus(sdev);
>  
>  	/* HDA bus init */
> -	sof_hda_bus_init(bus, &pci->dev);
> +	sof_hda_bus_init(hbus, pci, hda_model);
>  
> +#if !IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
>  	bus->use_posbuf = 1;
>  	bus->bdl_pos_adj = 0;
>  	bus->sync_write = 1;
> @@ -619,7 +620,7 @@ static int hda_init(struct snd_sof_dev *sdev)
>  	hbus->pci = pci;
>  	hbus->mixer_assigned = -1;
>  	hbus->modelname = hda_model;
> -

spurious line change

> +#endif
>  	/* initialise hdac bus */
>  	bus->addr = pci_resource_start(pci, 0);
>  	bus->remap_addr = pci_ioremap_bar(pci, 0);
> diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h
> index 1195018a1f4f..a4ec88f36512 100644
> --- a/sound/soc/sof/intel/hda.h
> +++ b/sound/soc/sof/intel/hda.h
> @@ -635,7 +635,8 @@ void hda_dsp_ctrl_stop_chip(struct snd_sof_dev *sdev);
>  /*
>   * HDA bus operations.
>   */
> -void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev);
> +void sof_hda_bus_init(struct hda_bus *bus, struct pci_dev *pdev,
> +		      const char *model);
>  
>  #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
>  /*
>