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

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

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