[PATCH v4 11/17] ASoC: Intel: avs: Firmware resources management utilities

[Cezary Rojewski]

On 2022-03-11 9:30 PM, Pierre-Louis Bossart wrote:
> On 3/11/22 11:20, Cezary Rojewski wrote:


...

>> Sorry for the typo: s/avs_module_info/avs_module_entry/.
>>
>>>> runtime parameters.
>>>
>>> you clarified the mechanism but not the definition of 'module-type'?
>>>
>>> the definition doesn't really help.
>>>
>>> struct avs_module_type {
>>>      u32 load_type:4;
>>>      u32 auto_start:1;
>>>      u32 domain_ll:1;
>>>      u32 domain_dp:1;
>>>      u32 lib_code:1;
>>>      u32 rsvd:24;
>>> } __packed;
>>>
>>> I see nothing that would e.g. identify a mixer from a gain. The 
>>> definition of 'type' seems to refer to low-level properties, not what 
>>> the module actually does?
>>
>>
>> There is no "module-type" enum that software can rely on. We rely on 
>> hardcoded GUIDs instead. "module-type" is represented by struct 
>> avs_module_entry (per type) in context of MODULE_INFO IPC. All these 
>> names are indented to match firmware equivalents to make it easier to 
>> switch between the two worlds.
> 
> So the initial kernel-doc I commented on is still quite convoluted, you 
> are referring to a 'module-type' that's not really well defined or 
> useful for a driver.
> 
> Given the definition:
> 
> struct avs_mods_info {
>      u32 count;
>      struct avs_module_entry entries[];
> } __packed;
> 
> 
> wouldn't this be simpler/less confusing:
> 
> "
> @mods_info: Available array of module entries, obtained through
> MODULES_INFO message
> "

The major problem is the convoluted naming within the firmware itself.

The decision for the naming all the members as they are is dictated by 
the fact that there is much, much higher chance for Intel audio software 
or firmware developer to do some major/meaningful changes to the 
avs-driver as the kernel grows than a person outside that circle. Not 
everyone might agree, but that's the democratic decision made by the 
team in the early days of this driver. And thus, having close 
name-matching between the driver and the firmware makes it easier for 
given developer to switch between the two projects.

Agree on the rewording. There are probably more of such wordings within 
the code so this might span more than 1 file.


Regards,
Czarek