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

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

[Cezary Rojewski]

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.

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)
 {
+	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;
 
-	spin_lock_init(&bus->reg_lock);
+	spin_lock_init(&bus->core.reg_lock);
 #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;
-
+#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)
 /*
-- 
2.25.1

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

[Pierre-Louis Bossart]

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)
>  /*
> 

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

[Cezary Rojewski]

On 2021-10-18 11:18 PM, Pierre-Louis Bossart wrote:
> 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.
> 

Thanks for the comments!

Pretty sure you overestimated the goal of this patch, though. In both 
skylake and sof -drivers various bus->xxx and bus->core.xxx fields are 
scattered and initialized with basically the exact same values. These 
values more often than not even match the sound/pci/hda ones. The 
ultimate goal is probably to extract similar parts and move them to 
snd_hdac_bus_init() or some other wrapper. For now, 'ext-bus' is being 
addressed.

Patch would have 'greater' effect if not for the fact that sof-intel-hda 
code conditionally initializes several fields for the reasons unknown to 
me. So, instead of just removing those assignments, preproccesor 
directive is used instead.


...

>>   /**
>> - * 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?
> 

Ack.

...

>> @@ -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()?
> 

I don't understand the comment. First argument in parameter-list for 
function sof_hda_bus_init() has its type changed from 'struct hdac_bus 
*' to 'struct hda_bus *'. Without updating those assignments, code 
wouldn't compile with CONFIG_SND_SOC_SOF_HDA disabled.

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

It's not a different one, it's exactly the same 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

Well, I've just inserted #endif in place of this newline. No problem 
with appending additional Enter if that's what you prefer.


Czarek

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

[Kai Vehmanen]

Hey,

On Mon, 18 Oct 2021, 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.
[...]
> --- a/sound/hda/ext/hdac_ext_bus.c
> +++ b/sound/hda/ext/hdac_ext_bus.c
[..]
> -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)
[...]
> -	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);

hmm. It's not clear whether we should initialize the regular hdac_bus 
fields in the ext_bus_init(). For plain HDA, these are also initialized
in the caller. E.g. in sound/pci/hda/hda_controller.c.

So if we start cleaning up, should we go further put this in 
snd_hdac_bus_init()? 

Then another is what is the right place for settings like "sync_write = 
1". While this settings applies to all current users of the extended
bus, this is really hw specific setting and not really a property of 
the extended bus, so this feels a bit out-of-place.

Br, Kai

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

[Cezary Rojewski]

On 2021-10-19 11:16 AM, Kai Vehmanen wrote:
> Hey,
> 
> On Mon, 18 Oct 2021, 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.
> [...]
>> --- a/sound/hda/ext/hdac_ext_bus.c
>> +++ b/sound/hda/ext/hdac_ext_bus.c
> [..]
>> -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)
> [...]
>> -	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);
> 
> hmm. It's not clear whether we should initialize the regular hdac_bus
> fields in the ext_bus_init(). For plain HDA, these are also initialized
> in the caller. E.g. in sound/pci/hda/hda_controller.c.
> 
> So if we start cleaning up, should we go further put this in
> snd_hdac_bus_init()?
> 
> Then another is what is the right place for settings like "sync_write =
> 1". While this settings applies to all current users of the extended
> bus, this is really hw specific setting and not really a property of
> the extended bus, so this feels a bit out-of-place.

I'm rather in favor of bigger cleanups. We can definitely move further 
in the future : )

Notice that some 'core' field are being initialized in 
snd_hda_ext_bus_init() for a very long time.
The problem with moving all 'core' bits to snd_hdac_bus_init() is that 
some of these are not 1:1 between ASoC and ALSA hda-drivers. Also, 
hda_tegra differs from hda_intel too. I could update said function while 
not removing any assignments which differ from the default. Maybe let's 
just go for it..

TLDR: treat snd_hdac_bus_init() as function initializing fields with 
defaults. Any you don't like? Change after its invocation.

BTW, I don't see any problems with ->sync_write=1 as from software 
perspective it is a bus property. Otherwise, interface change is 
required to reflect this 'misleading' information.


Regards,
Czarek

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

[Pierre-Louis Bossart]

>> hmm. It's not clear whether we should initialize the regular hdac_bus
>> fields in the ext_bus_init(). For plain HDA, these are also initialized
>> in the caller. E.g. in sound/pci/hda/hda_controller.c.
>>
>> So if we start cleaning up, should we go further put this in
>> snd_hdac_bus_init()?
>>
>> Then another is what is the right place for settings like "sync_write =
>> 1". While this settings applies to all current users of the extended
>> bus, this is really hw specific setting and not really a property of
>> the extended bus, so this feels a bit out-of-place.
> 
> I'm rather in favor of bigger cleanups. We can definitely move further
> in the future : )

I am not opposed to updates in this hdaudio-ext part, but I am in favor
of less ambitious step-by-step changes.

a) This is legacy code where the initial authors have moved on, and it's
very hard to figure out what the intent was. It's quite common to have
discussion on feature v. bug, see e.g. the discussion we had on this in
https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119

b) In addition, this code is shared between two teams with separate
testing/CI capabilities and limited number of commercial/shipping
devices. There is a very large risk of introducing bugs even with the
best intentions.

Before we do any changes in functionality, there are already basic steps
that can be done, e.g. using consistent naming between variables, the
existing code is just confusing as can be:

struct hdac_stream *stream;
struct hdac_ext_stream *stream;
struct hdac_stream *hstream;
struct hdac_ext_stream *hstream;

I started basic cleanups here:
https://github.com/thesofproject/linux/pull/3158, I haven't had time to
complete this because of more important locking issues, but I intend to
send those clarifications for the next cycle.

Again before we do large changes let's think of smaller steps and how we
are going to validate those changes.

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

[Cezary Rojewski]

On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
>>> hmm. It's not clear whether we should initialize the regular hdac_bus
>>> fields in the ext_bus_init(). For plain HDA, these are also initialized
>>> in the caller. E.g. in sound/pci/hda/hda_controller.c.
>>>
>>> So if we start cleaning up, should we go further put this in
>>> snd_hdac_bus_init()?
>>>
>>> Then another is what is the right place for settings like "sync_write =
>>> 1". While this settings applies to all current users of the extended
>>> bus, this is really hw specific setting and not really a property of
>>> the extended bus, so this feels a bit out-of-place.
>>
>> I'm rather in favor of bigger cleanups. We can definitely move further
>> in the future : )
> 
> I am not opposed to updates in this hdaudio-ext part, but I am in favor
> of less ambitious step-by-step changes.
> 
> a) This is legacy code where the initial authors have moved on, and it's
> very hard to figure out what the intent was. It's quite common to have
> discussion on feature v. bug, see e.g. the discussion we had on this in
> https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
> 
> b) In addition, this code is shared between two teams with separate
> testing/CI capabilities and limited number of commercial/shipping
> devices. There is a very large risk of introducing bugs even with the
> best intentions.
> 
> Before we do any changes in functionality, there are already basic steps
> that can be done, e.g. using consistent naming between variables, the
> existing code is just confusing as can be:
> 
> struct hdac_stream *stream;
> struct hdac_ext_stream *stream;
> struct hdac_stream *hstream;
> struct hdac_ext_stream *hstream;
> 
> I started basic cleanups here:
> https://github.com/thesofproject/linux/pull/3158, I haven't had time to
> complete this because of more important locking issues, but I intend to
> send those clarifications for the next cycle.
> 
> Again before we do large changes let's think of smaller steps and how we
> are going to validate those changes.

Agree, step-by-step is the way to go.

Isn't this patch a 'step' though? This isn't remotely a refactor, just 
gathering of common parts of ext-bus initialization. We could start with 
this so legacy users are unaffected, then have snd_hdac_bus_init() 
updated so snd_hdac_ext_bus_init() is devoid of 'core' fields 
assignments as suggested by Kai.


Regards,
Czarek

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

[Pierre-Louis Bossart]

On 10/19/21 12:32 PM, Cezary Rojewski wrote:
> On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:
>>>> hmm. It's not clear whether we should initialize the regular hdac_bus
>>>> fields in the ext_bus_init(). For plain HDA, these are also initialized
>>>> in the caller. E.g. in sound/pci/hda/hda_controller.c.
>>>>
>>>> So if we start cleaning up, should we go further put this in
>>>> snd_hdac_bus_init()?
>>>>
>>>> Then another is what is the right place for settings like "sync_write =
>>>> 1". While this settings applies to all current users of the extended
>>>> bus, this is really hw specific setting and not really a property of
>>>> the extended bus, so this feels a bit out-of-place.
>>>
>>> I'm rather in favor of bigger cleanups. We can definitely move further
>>> in the future : )
>>
>> I am not opposed to updates in this hdaudio-ext part, but I am in favor
>> of less ambitious step-by-step changes.
>>
>> a) This is legacy code where the initial authors have moved on, and it's
>> very hard to figure out what the intent was. It's quite common to have
>> discussion on feature v. bug, see e.g. the discussion we had on this in
>> https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
>>
>>
>> b) In addition, this code is shared between two teams with separate
>> testing/CI capabilities and limited number of commercial/shipping
>> devices. There is a very large risk of introducing bugs even with the
>> best intentions.
>>
>> Before we do any changes in functionality, there are already basic steps
>> that can be done, e.g. using consistent naming between variables, the
>> existing code is just confusing as can be:
>>
>> struct hdac_stream *stream;
>> struct hdac_ext_stream *stream;
>> struct hdac_stream *hstream;
>> struct hdac_ext_stream *hstream;
>>
>> I started basic cleanups here:
>> https://github.com/thesofproject/linux/pull/3158, I haven't had time to
>> complete this because of more important locking issues, but I intend to
>> send those clarifications for the next cycle.
>>
>> Again before we do large changes let's think of smaller steps and how we
>> are going to validate those changes.
> 
> Agree, step-by-step is the way to go.
> 
> Isn't this patch a 'step' though? This isn't remotely a refactor, just
> gathering of common parts of ext-bus initialization. We could start with
> this so legacy users are unaffected, then have snd_hdac_bus_init()
> updated so snd_hdac_ext_bus_init() is devoid of 'core' fields
> assignments as suggested by Kai.

If it was just moving common parts, I would have no issue. My main
objection is that you went one step further and started renaming stuff
in rather confusing ways, e.g.

-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,

- * snd_hdac_ext_bus_init - initialize a HD-audio extended bus
+ * snd_hda_ext_bus_init - initialize a HD-audio extended bus

hda_bus is a definition from hda_codec.h, I don't see a reason why we
should use this structure when the vast majority of the code uses
hdac_bus. And the purpose of hdac_ext is really to deal with
*controller* extensions to couple/decouple DMAs. There is no dependency
on codecs at that level.

Even if there was, I also don't really see why/when we should start
using hda_ext v. hdac_ext, the naming convention completely escapes me.
It would seem logical to me to only use hdac_ext as an extension of
hdac_, no?

I also don't get what this means:
+	snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");

what is 'sklbus' and what purpose are you trying to accomplish with this
change?

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

[Cezary Rojewski]

On 2021-10-19 8:42 PM, Pierre-Louis Bossart wrote:
> On 10/19/21 12:32 PM, Cezary Rojewski wrote:
>> On 2021-10-19 6:58 PM, Pierre-Louis Bossart wrote:

...

>>> I am not opposed to updates in this hdaudio-ext part, but I am in favor
>>> of less ambitious step-by-step changes.
>>>
>>> a) This is legacy code where the initial authors have moved on, and it's
>>> very hard to figure out what the intent was. It's quite common to have
>>> discussion on feature v. bug, see e.g. the discussion we had on this in
>>> https://github.com/thesofproject/linux/pull/3175#pullrequestreview-772674119
>>>
>>>
>>> b) In addition, this code is shared between two teams with separate
>>> testing/CI capabilities and limited number of commercial/shipping
>>> devices. There is a very large risk of introducing bugs even with the
>>> best intentions.
>>>
>>> Before we do any changes in functionality, there are already basic steps
>>> that can be done, e.g. using consistent naming between variables, the
>>> existing code is just confusing as can be:
>>>
>>> struct hdac_stream *stream;
>>> struct hdac_ext_stream *stream;
>>> struct hdac_stream *hstream;
>>> struct hdac_ext_stream *hstream;
>>>
>>> I started basic cleanups here:
>>> https://github.com/thesofproject/linux/pull/3158, I haven't had time to
>>> complete this because of more important locking issues, but I intend to
>>> send those clarifications for the next cycle.
>>>
>>> Again before we do large changes let's think of smaller steps and how we
>>> are going to validate those changes.
>>
>> Agree, step-by-step is the way to go.
>>
>> Isn't this patch a 'step' though? This isn't remotely a refactor, just
>> gathering of common parts of ext-bus initialization. We could start with
>> this so legacy users are unaffected, then have snd_hdac_bus_init()
>> updated so snd_hdac_ext_bus_init() is devoid of 'core' fields
>> assignments as suggested by Kai.
> 
> If it was just moving common parts, I would have no issue. My main
> objection is that you went one step further and started renaming stuff
> in rather confusing ways, e.g.
> 
> -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,
> 
> - * snd_hdac_ext_bus_init - initialize a HD-audio extended bus
> + * snd_hda_ext_bus_init - initialize a HD-audio extended bus

Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is 
argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't 
believe that's confusing to anybody.

No problem with reverting naming change for now - we can streamline 
naming later.

In regard to sof_hda_bus_init(): I don't see any renaming here, just 
argument type changes to match new snd_hda_ext_bus_init() usage.

> hda_bus is a definition from hda_codec.h, I don't see a reason why we
> should use this structure when the vast majority of the code uses
> hdac_bus. And the purpose of hdac_ext is really to deal with
> *controller* extensions to couple/decouple DMAs. There is no dependency
> on codecs at that level.

hda_bus is the base for all HDAudio drivers:
struct azx
struct skl_dev
struct sof_intel_hda_dev

So no, what vast majority of code actually does is hda_bus/codec to 
hdac_bus/codec (and vice-versa) translation when in fact all the drivers 
are hda_* based. If you speak of confusion, that's the confusion that 
should be addressed in the future..

> Even if there was, I also don't really see why/when we should start
> using hda_ext v. hdac_ext, the naming convention completely escapes me.
> It would seem logical to me to only use hdac_ext as an extension of
> hdac_, no?
> 
> I also don't get what this means:
> +	snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
> 
> what is 'sklbus' and what purpose are you trying to accomplish with this
> change?
> 

Well, please see the updated declaration of snd_hda_ext_bus_init() in 
this very patch and then the existing code of 
sound/soc/intel/skylake/skl.c - skl_create().
Last argument in updated declaration reads 'modelname'. Skylake-driver 
has its own, SOF initializes it differently.


Regads,
Czarek

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

[Pierre-Louis Bossart]

>> If it was just moving common parts, I would have no issue. My main
>> objection is that you went one step further and started renaming stuff
>> in rather confusing ways, e.g.
>>
>> -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,
>>
>> - * snd_hdac_ext_bus_init - initialize a HD-audio extended bus
>> + * snd_hda_ext_bus_init - initialize a HD-audio extended bus
> 
> Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is
> argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't
> believe that's confusing to anybody.

it is confusing to me, myself and I. The main point is that it blurs
layers.

The hdaudio_ext library deals with Intel controller extensions for DMA
management and does not need to know about a larger container.

> No problem with reverting naming change for now - we can streamline
> naming later.
> 
> In regard to sof_hda_bus_init(): I don't see any renaming here, just
> argument type changes to match new snd_hda_ext_bus_init() usage.
> 
>> hda_bus is a definition from hda_codec.h, I don't see a reason why we
>> should use this structure when the vast majority of the code uses
>> hdac_bus. And the purpose of hdac_ext is really to deal with
>> *controller* extensions to couple/decouple DMAs. There is no dependency
>> on codecs at that level.
> 
> hda_bus is the base for all HDAudio drivers:
> struct azx
> struct skl_dev
> struct sof_intel_hda_dev
> 
> So no, what vast majority of code actually does is hda_bus/codec to
> hdac_bus/codec (and vice-versa) translation when in fact all the drivers
> are hda_* based. If you speak of confusion, that's the confusion that
> should be addressed in the future..

I maintain that the hdaudio_ext library is about Intel-specific changes
on the controller level and only with DMAs.

/**
 * hdac_ext_stream: HDAC extended stream for extended HDA caps
 *
 * @hstream: hdac_stream
 * @pphc_addr: processing pipe host stream pointer
 * @pplc_addr: processing pipe link stream pointer
 * @spib_addr: software position in buffers stream pointer
 * @fifo_addr: software position Max fifos stream pointer
 * @dpibr_addr: DMA position in buffer resume pointer
 * @dpib: DMA position in buffer
 * @lpib: Linear position in buffer
 * @decoupled: stream host and link is decoupled
 * @link_locked: link is locked
 * @link_prepared: link is prepared
 * @link_substream: link substream
 */

It's not really a surprise that there's confusion, even the HDaudio spec
describes controller, DMA, link and codec without clearly calling out
boundaries between layers.

> 
>> Even if there was, I also don't really see why/when we should start
>> using hda_ext v. hdac_ext, the naming convention completely escapes me.
>> It would seem logical to me to only use hdac_ext as an extension of
>> hdac_, no?
>>
>> I also don't get what this means:
>> +    snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
>>
>> what is 'sklbus' and what purpose are you trying to accomplish with this
>> change?
>>
> 
> Well, please see the updated declaration of snd_hda_ext_bus_init() in
> this very patch and then the existing code of
> sound/soc/intel/skylake/skl.c - skl_create().
> Last argument in updated declaration reads 'modelname'. Skylake-driver
> has its own, SOF initializes it differently.

Not sure why you have your own?

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

[Cezary Rojewski]

On 2021-10-21 7:19 PM, Pierre-Louis Bossart wrote:
> 
>>> If it was just moving common parts, I would have no issue. My main
>>> objection is that you went one step further and started renaming stuff
>>> in rather confusing ways, e.g.
>>>
>>> -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,
>>>
>>> - * snd_hdac_ext_bus_init - initialize a HD-audio extended bus
>>> + * snd_hda_ext_bus_init - initialize a HD-audio extended bus
>>
>> Reason for renaming snd_hdac_ext_bus_init() to snd_hda_ext_bus_init() is
>> argument type change: 'struct hdac_bus *' to struct hda_bus *'. Don't
>> believe that's confusing to anybody.
> 
> it is confusing to me, myself and I. The main point is that it blurs
> layers.
> 
> The hdaudio_ext library deals with Intel controller extensions for DMA
> management and does not need to know about a larger container.

Ok. Will leave the naming part for future patches while leaving argument 
list as is.

...

>>> Even if there was, I also don't really see why/when we should start
>>> using hda_ext v. hdac_ext, the naming convention completely escapes me.
>>> It would seem logical to me to only use hdac_ext as an extension of
>>> hdac_, no?
>>>
>>> I also don't get what this means:
>>> +    snd_hda_ext_bus_init(hbus, pci, NULL, ext_ops, "sklbus");
>>>
>>> what is 'sklbus' and what purpose are you trying to accomplish with this
>>> change?
>>>
>>
>> Well, please see the updated declaration of snd_hda_ext_bus_init() in
>> this very patch and then the existing code of
>> sound/soc/intel/skylake/skl.c - skl_create().
>> Last argument in updated declaration reads 'modelname'. Skylake-driver
>> has its own, SOF initializes it differently.
> 
> Not sure why you have your own?
> 

Not sure I understand the question. If you are talking about changing 
string 'sklbus' to something else, then I don't believe mixing changes: 
update to actual values assigned and assignment relocation in one patch 
is good. I used 'sklbus' as that's what is being currently assigned to 
->modelname within skl_create(). Such approach makes the change more 
transparent.


Regards,
Czarek

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

[Pierre-Louis Bossart]

>>> Well, please see the updated declaration of snd_hda_ext_bus_init() in
>>> this very patch and then the existing code of
>>> sound/soc/intel/skylake/skl.c - skl_create().
>>> Last argument in updated declaration reads 'modelname'. Skylake-driver
>>> has its own, SOF initializes it differently.
>>
>> Not sure why you have your own?
>>
> 
> Not sure I understand the question. If you are talking about changing
> string 'sklbus' to something else, then I don't believe mixing changes:
> update to actual values assigned and assignment relocation in one patch
> is good. I used 'sklbus' as that's what is being currently assigned to
> ->modelname within skl_create(). Such approach makes the change more
> transparent.

What I meant is that this 'modelname' is a module parameter for legacy
and SOF driver, it's attached to the bus, but eventually used by the codec

hda_codec.c:    if (codec->bus->modelname) {
hda_codec.c:            codec->modelname =
kstrdup(codec->bus->modelname, GFP_KERNEL);

and even further down used to apply board-specific fixups.

"sklbus" doesn't seem to be related to codecs, boards or fixups, so not
sure what this parameter does in the end?

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4049 bytes --]

Hi Cezary,

I love your patch! Perhaps something to improve:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next v5.15-rc6 next-20211022]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cezary-Rojewski/ASoC-Intel-Unify-HDAudio-ext-bus-initialization/20211019-032047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: x86_64-randconfig-a012-20211022 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5dc339d9825f1dbe788cfb69c88210a59bbf8e3a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/997e684694fcfc54bfef7a1c21e1788bc52a29af
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cezary-Rojewski/ASoC-Intel-Unify-HDAudio-ext-bus-initialization/20211019-032047
        git checkout 997e684694fcfc54bfef7a1c21e1788bc52a29af
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> sound/soc/intel/skylake/skl.c:864:19: warning: variable 'bus' set but not used [-Wunused-but-set-variable]
           struct hdac_bus *bus;
                            ^
   1 warning generated.


vim +/bus +864 sound/soc/intel/skylake/skl.c

ab1b732d53c18f Vinod Koul         2017-05-04  855  
d8c2dab8381d58 Jeeja KP           2015-07-09  856  /*
d8c2dab8381d58 Jeeja KP           2015-07-09  857   * constructor
d8c2dab8381d58 Jeeja KP           2015-07-09  858   */
d8c2dab8381d58 Jeeja KP           2015-07-09  859  static int skl_create(struct pci_dev *pci,
bcc2a2dc3ba8c3 Cezary Rojewski    2019-07-23  860  		      struct skl_dev **rskl)
d8c2dab8381d58 Jeeja KP           2015-07-09  861  {
6bae5ea9498926 Rakesh Ughreja     2018-08-22  862  	struct hdac_ext_bus_ops *ext_ops = NULL;
bcc2a2dc3ba8c3 Cezary Rojewski    2019-07-23  863  	struct skl_dev *skl;
76f56fae1cf904 Rakesh Ughreja     2018-06-01 @864  	struct hdac_bus *bus;
00deadb5d86a3c Rakesh Ughreja     2018-08-22  865  	struct hda_bus *hbus;
d8c2dab8381d58 Jeeja KP           2015-07-09  866  	int err;
d8c2dab8381d58 Jeeja KP           2015-07-09  867  
d8c2dab8381d58 Jeeja KP           2015-07-09  868  	*rskl = NULL;
d8c2dab8381d58 Jeeja KP           2015-07-09  869  
d8c2dab8381d58 Jeeja KP           2015-07-09  870  	err = pci_enable_device(pci);
d8c2dab8381d58 Jeeja KP           2015-07-09  871  	if (err < 0)
d8c2dab8381d58 Jeeja KP           2015-07-09  872  		return err;
d8c2dab8381d58 Jeeja KP           2015-07-09  873  
d8c2dab8381d58 Jeeja KP           2015-07-09  874  	skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL);
d8c2dab8381d58 Jeeja KP           2015-07-09  875  	if (!skl) {
d8c2dab8381d58 Jeeja KP           2015-07-09  876  		pci_disable_device(pci);
d8c2dab8381d58 Jeeja KP           2015-07-09  877  		return -ENOMEM;
d8c2dab8381d58 Jeeja KP           2015-07-09  878  	}
76f56fae1cf904 Rakesh Ughreja     2018-06-01  879  
00deadb5d86a3c Rakesh Ughreja     2018-08-22  880  	hbus = skl_to_hbus(skl);
76f56fae1cf904 Rakesh Ughreja     2018-06-01  881  	bus = skl_to_bus(skl);
6bae5ea9498926 Rakesh Ughreja     2018-08-22  882  
776cb3b80ede9e Amadeusz Sławiński 2019-06-17  883  	INIT_LIST_HEAD(&skl->ppl_list);
776cb3b80ede9e Amadeusz Sławiński 2019-06-17  884  	INIT_LIST_HEAD(&skl->bind_list);
776cb3b80ede9e Amadeusz Sławiński 2019-06-17  885  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35732 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4123 bytes --]

Hi Cezary,

I love your patch! Perhaps something to improve:

[auto build test WARNING on broonie-sound/for-next]
[also build test WARNING on tiwai-sound/for-next v5.15-rc6 next-20211022]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Cezary-Rojewski/ASoC-Intel-Unify-HDAudio-ext-bus-initialization/20211019-032047
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: x86_64-randconfig-a012-20211022 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5dc339d9825f1dbe788cfb69c88210a59bbf8e3a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/997e684694fcfc54bfef7a1c21e1788bc52a29af
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Cezary-Rojewski/ASoC-Intel-Unify-HDAudio-ext-bus-initialization/20211019-032047
        git checkout 997e684694fcfc54bfef7a1c21e1788bc52a29af
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> sound/soc/intel/skylake/skl.c:864:19: warning: variable 'bus' set but not used [-Wunused-but-set-variable]
           struct hdac_bus *bus;
                            ^
   1 warning generated.


vim +/bus +864 sound/soc/intel/skylake/skl.c

ab1b732d53c18f Vinod Koul         2017-05-04  855  
d8c2dab8381d58 Jeeja KP           2015-07-09  856  /*
d8c2dab8381d58 Jeeja KP           2015-07-09  857   * constructor
d8c2dab8381d58 Jeeja KP           2015-07-09  858   */
d8c2dab8381d58 Jeeja KP           2015-07-09  859  static int skl_create(struct pci_dev *pci,
bcc2a2dc3ba8c3 Cezary Rojewski    2019-07-23  860  		      struct skl_dev **rskl)
d8c2dab8381d58 Jeeja KP           2015-07-09  861  {
6bae5ea9498926 Rakesh Ughreja     2018-08-22  862  	struct hdac_ext_bus_ops *ext_ops = NULL;
bcc2a2dc3ba8c3 Cezary Rojewski    2019-07-23  863  	struct skl_dev *skl;
76f56fae1cf904 Rakesh Ughreja     2018-06-01 @864  	struct hdac_bus *bus;
00deadb5d86a3c Rakesh Ughreja     2018-08-22  865  	struct hda_bus *hbus;
d8c2dab8381d58 Jeeja KP           2015-07-09  866  	int err;
d8c2dab8381d58 Jeeja KP           2015-07-09  867  
d8c2dab8381d58 Jeeja KP           2015-07-09  868  	*rskl = NULL;
d8c2dab8381d58 Jeeja KP           2015-07-09  869  
d8c2dab8381d58 Jeeja KP           2015-07-09  870  	err = pci_enable_device(pci);
d8c2dab8381d58 Jeeja KP           2015-07-09  871  	if (err < 0)
d8c2dab8381d58 Jeeja KP           2015-07-09  872  		return err;
d8c2dab8381d58 Jeeja KP           2015-07-09  873  
d8c2dab8381d58 Jeeja KP           2015-07-09  874  	skl = devm_kzalloc(&pci->dev, sizeof(*skl), GFP_KERNEL);
d8c2dab8381d58 Jeeja KP           2015-07-09  875  	if (!skl) {
d8c2dab8381d58 Jeeja KP           2015-07-09  876  		pci_disable_device(pci);
d8c2dab8381d58 Jeeja KP           2015-07-09  877  		return -ENOMEM;
d8c2dab8381d58 Jeeja KP           2015-07-09  878  	}
76f56fae1cf904 Rakesh Ughreja     2018-06-01  879  
00deadb5d86a3c Rakesh Ughreja     2018-08-22  880  	hbus = skl_to_hbus(skl);
76f56fae1cf904 Rakesh Ughreja     2018-06-01  881  	bus = skl_to_bus(skl);
6bae5ea9498926 Rakesh Ughreja     2018-08-22  882  
776cb3b80ede9e Amadeusz Sławiński 2019-06-17  883  	INIT_LIST_HEAD(&skl->ppl_list);
776cb3b80ede9e Amadeusz Sławiński 2019-06-17  884  	INIT_LIST_HEAD(&skl->bind_list);
776cb3b80ede9e Amadeusz Sławiński 2019-06-17  885  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35732 bytes --]