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

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

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