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

[Cezary Rojewski <cezary.rojewski@intel.com>]

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