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