Re: [PATCH 10/14] ASoC: Intel: avs: Machine board registration

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

On 2022-05-04 11:41 AM, Amadeusz Sławiński wrote:
> On 4/29/2022 4:01 PM, Cezary Rojewski wrote:
> 
>>>> +static struct snd_soc_acpi_mach *dmi_match_quirk(void *arg)
>>>> +{
>>>> +    struct snd_soc_acpi_mach *mach = arg;
>>>> +    const struct dmi_system_id *dmi_id;
>>>> +    struct dmi_system_id *dmi_table;
>>>> +
>>>> +    if (mach->quirk_data == NULL)
>>>> +        return mach;
>>>> +
>>>> +    dmi_table = (struct dmi_system_id *)mach->quirk_data;
>>>> +
>>>> +    dmi_id = dmi_first_match(dmi_table);
>>>> +    if (!dmi_id)
>>>> +        return NULL;
>>>> +
>>>> +    return mach;
>>>> +}
>>>> +
>>>> +#define AVS_SSP(x)        (BIT(x))
>>>> +#define AVS_SSP_RANGE(a, b)    (GENMASK(b, a))
>>>> +
>>>> +/* supported I2S board codec configurations */
>>>> +static struct snd_soc_acpi_mach avs_skl_i2s_machines[] = {
>>>> +    {
>>>> +        .id = "INT343A",
>>>> +        .drv_name = "avs_rt286",
>>>> +        .link_mask = AVS_SSP(0),
>>>
>>> I've told this before, 'link_mask' was introduced for *SoundWire*. 
>>> Please do not overload existing concepts and use this instead:
>>>
>>> @i2s_link_mask: I2S/TDM links enabled on the board
>>
>>
>> Noooo :( Sad panda is sad.
>>
>> 'link_mask' is such a wonderful name as it matches naming used in our 
>> specs - which call BE side 'LINK'.
>>
>> If it's a must then yes, we will resign from using 'link_mask'.
>>
> 
> I'll just note that header kernel doc for link_mask says:
> " * @link_mask: describes required board layout, e.g. for SoundWire."
> I would say there is no assumption that it is SDW only, so we could also 
> argue that if it is it should be named sdw_link_mask and comment be 
> fixed to remove "e.g." ;)


This is a marvelous suggestion! Adheres to fw spec, adheres to kernel 
doc. What more would one want!


Regards,
Czarek