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
