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