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

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

On 2022-05-04 1:26 PM, Péter Ujfalusi wrote:
> On 04/05/2022 12:41, Amadeusz Sławiński wrote:
>> On 4/29/2022 4:01 PM, Cezary Rojewski wrote:
>>>>> +#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." ;)
> 
> In mainline kernel it is (as of 5.18.0-rc5):
> /**
>   * snd_soc_acpi_mach_params: interface for machine driver configuration
>   *
>   * @acpi_ipc_irq_index: used for BYT-CR detection
>   * @platform: string used for HDAudio codec support
>   * @codec_mask: used for HDAudio support
>   * @dmic_num: number of SoC- or chipset-attached PDM digital microphones
>   * @common_hdmi_codec_drv: use commom HDAudio HDMI codec driver
>   * @link_mask: SoundWire links enabled on the board
>   * @links: array of SoundWire link _ADR descriptors, null terminated
>   * @i2s_link_mask: I2S/TDM links enabled on the board
>   * @num_dai_drivers: number of elements in @dai_drivers
>   * @dai_drivers: pointer to dai_drivers, used e.g. in nocodec mode
>   */
> 
> link_mask is for SDW
> i2s_link_mask is for I2S
> 
> It might be nicer to prefix the bare link_mask with sdw, it might
> happen, but the comment is pretty explicit. I would not dare to use
> link_mask for DMIC for example...
> 


Ok, only now I realized that the 'i2s_link_mask' is already part of 
upstream kernel - commit:

ASoC: soc-acpi: add information on I2S/TDM link mask

from Mar 8th 2022. I agree on the "sdw_" prefix part. Right now, the 
name gives plenty of room for interpretation.


Regards,
Czarek