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

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

On 2021-10-21 7:19 PM, Pierre-Louis Bossart wrote:
> 
>>> 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.

Ok. Will leave the naming part for future patches while leaving argument 
list as is.

...

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

Not sure I understand the question. If you are talking about changing 
string 'sklbus' to something else, then I don't believe mixing changes: 
update to actual values assigned and assignment relocation in one patch 
is good. I used 'sklbus' as that's what is being currently assigned to 
->modelname within skl_create(). Such approach makes the change more 
transparent.


Regards,
Czarek