Received-SPF: pass (google.com: domain of linux-kernel+bounces-72797-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <0940c052-d834-4ddd-98ac-8395ab48ceb8@amd.com>
Date: Tue, 20 Feb 2024 15:45:25 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ASoC: SOF: amd: fix soundwire dependencies
Content-Language: en-US
To: Arnd Bergmann <arnd@arndb.de>, Arnd Bergmann <arnd@kernel.org>,
 Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
 Liam Girdwood <lgirdwood@gmail.com>,
 Peter Ujfalusi <peter.ujfalusi@linux.intel.com>,
 Bard Liao <yung-chuan.liao@linux.intel.com>,
 Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
 Daniel Baluta <daniel.baluta@nxp.com>, Mark Brown <broonie@kernel.org>
Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>,
 Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
 Venkata Prasad Potturu <venkataprasad.potturu@amd.com>,
 sound-open-firmware@alsa-project.org, linux-sound@vger.kernel.org,
 linux-kernel@vger.kernel.org,
 "Dommati, Sunil-kumar" <Sunil-kumar.Dommati@amd.com>
References: <20240219093900.644574-1-arnd@kernel.org>
 <465d1076-163c-4933-a9b5-e4e8736f5748@amd.com>
 <412c0a42-4c19-48dd-aa4e-de39b6ec3099@app.fastmail.com>
 <343f7b1d-1b4e-402e-bd54-4473b91b6f79@amd.com>
 <65f13ad3-989a-4699-9695-d7b1a95297eb@app.fastmail.com>
 <779d72f1-832a-4fef-a529-ebe8100b04f9@amd.com>
 <96f6e105-0e89-429f-a8c5-2e64087d7771@app.fastmail.com>
From: "Mukunda,Vijendar" <vijendar.mukunda@amd.com>
In-Reply-To: <96f6e105-0e89-429f-a8c5-2e64087d7771@app.fastmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RkZvcEV0UzNqTFRPR2xFVDlodzV0MmYvVUNiM1ZNY3YwUXlGWkFMVFJXZkJX?=
 =?utf-8?B?VFNzNm9EWFE0MWoxRTkyWWdkNEtwQ2x5SzRCbEsxWG02ZGYrd1AyZ3Y5Rk5r?=
 =?utf-8?B?eWdvMTJCa3RINFF6Z2xkYjlqZVEzZlJ4YUViaWRjamxwaFZpSjFScVlHNGdu?=
 =?utf-8?B?cytab00xdWNxMjd0TkZPQjE2OVFSTGhpOWtkTnhoV051RzdQV29QTUh0enov?=
 =?utf-8?B?SDJPUHhNOFJEOVRaVmhoUE5QY2k4d2RUOU94elV1NEplTmhod25Od04zVmto?=
 =?utf-8?B?VC94T2Z5UldXVkpUUjU0MnhnZlJ5cS9xYk5TY2krZXpneWRyMVNIRjlrY3dU?=
 =?utf-8?B?ckM3VXNRK3QxdnpQT2FhR2FIdDhiZkVWcUJxRG5qRm9jV0oyNkNtOTVldVBQ?=
 =?utf-8?B?ZGZGU21HSHIvZjRtSDVJaFlkWENXd0dmalNYSGRXOVNEZEpZcjVDRVhtTUli?=
 =?utf-8?B?SVdCakpzUDN6SmNQMDRHVlk5N0lramZENTZUSFBaWkYwNjBhL0pBRGxudUg5?=
 =?utf-8?B?VnlVeXkrRmozM21zWWRXcG5qRVBpUUpaT01VZGFtaSs4MTlqWDgwazBpOUt3?=
 =?utf-8?B?amI5S29aMDVQSVQ3TnIwWHpBTkliODRlZG5STEZGeUNMK25RRlJxajdPZEFQ?=
 =?utf-8?B?SmVVR1FCTE1VUkpHSHBCRUpnVENLalV3NlV5U0pKWFN2RnFkai8wSTN1V1R5?=
 =?utf-8?B?c2ZhQUJ5SEVBQjI3YWtBdEpDb2JOaVZ6b1RjdVRuVDZXSWdaQkhLZzkzOHEw?=
 =?utf-8?B?d0g3WU84NjdNV0lhcmFVVGpjWU1MT0JOWDBGTTlRbDZ2TEJvc3RkNCt6RVRT?=
 =?utf-8?B?WmxBbzIxQlhaTFlvMlkvUTJ4dkl1a0RlRWFvSU44bjF4MW13ajdmVUJmdk5a?=
 =?utf-8?B?bXRBWEhsNHBGZTJkZ0s2UE1Tek1WRFBFd0w2WWQwVmUzSmZsM3FBMUg5SHN3?=
 =?utf-8?B?UDE0M1hvRzNKU09XZFQzVXRRcmhPaGFuY3dRMFd4SURvYmRhbjlWYXdrOEhu?=
 =?utf-8?B?NGxsMEt1SkRZMUlybWkxVDM4d0p0bGNQd080QmxpTzRhdTRJOU1vUUVYZ0JC?=
 =?utf-8?B?Q3ZVNlc4VlNyRzdCOFVmdHBSV3Uwa3BsNTNFVjg2MGo1R01rMEFIc3ZURHlW?=
 =?utf-8?B?Rmc1cjZXajZJK25iNllFU2duNGdmWHRqVnVCdkVsWUdTeHVDKzYwamJtNUdS?=
 =?utf-8?B?ZWwwL0xidTR0YnRsQnpHL1U4ZktmTnFZdUxuSEh4dGtXbWJPRDYzd2haMGRu?=
 =?utf-8?B?THZ4N0lvMjAvME9rK2tualNFOUhiTlJ0VTR2eE14QzdTeDcva25xcmxYZ2la?=
 =?utf-8?B?cTRCY2VFRm1wUVk2TmtxTzBJRjF2aGxKMVJsVzJTT3FKOEZsM2tnMGYyOERr?=
 =?utf-8?B?dTNxY055bVFhdG1ncVlQeWUvMXEvNTc4RU80a1l5V2RKZGJyUUdRRmcyMjdi?=
 =?utf-8?B?a25qNmx4TWhGcWUrM0VXQXQ0bDhlaUJVV2RQZ2RDakc2V3RMZjRKM0tYdlUw?=
 =?utf-8?B?V2dLbDkwa2hFQlEvb0RzTFNicUNjeXgraHE1bWxMNlNCNWt5RzVXdkR2Z2pU?=
 =?utf-8?B?ZHFKaWp3c2VQVVVhVncwY29vaU1tZzhWRk9CaEw0ajJXSDdVQWpTcU9GUEdD?=
 =?utf-8?B?R2RCVkxlRkg3U3prZkFNc1AyMFVMTHhxSGU2SVpJeGFiNDRPZFNTSkFaWDk1?=
 =?utf-8?B?M3krNGNVRlJXaUliNkczZ0txMnZ1N0JSYmxSYWVVdE50WWN4YXA3dVhGcEMv?=
 =?utf-8?B?Y014M1BlRjNNZnF2dU0vWEg1ODEzU3hlQmltcWxHVzNXdmFqMHhHM01xcTdL?=
 =?utf-8?B?SjllbEczUXgvSnNrcm52c0VyeVcvbC9MVEVMZ0lQeGswTHUzdW1Kb0lPUlhx?=
 =?utf-8?B?NGkyZEEwTDJhYWMyTVZBdmVHUnZjUmwxQWRxaWZIWmJubm9sR2wzbDZENkhv?=
 =?utf-8?B?Y3JPUWpoTFRQaGE3Tlk5L0pIdDhpUkc4azdrZk9RM2pnVXpweDBUbDgzcmxS?=
 =?utf-8?B?S0xRRmc3d09JaFJVZHRaamZncWhzRVduN1BXQUpVOFVmNmJkVlNpaVgyVGRC?=
 =?utf-8?B?RVJ5aWNOYmw1VmVtMGRyRVhMQ2k1akc3dkg0WUhCcUgzYTYwUVlwaVc3VnU3?=
 =?utf-8?Q?5G0GNZ3fM1o7GbV0xiuBObRyt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4b50cd41-68ef-40d4-32a1-08dc31fce15e
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB4123.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 10:15:36.8924
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: I/IFFo2uKgcFg+RjEGvxyTuFfjSxaIQVHFAudRzqvKIWk5fV+mVEPa3bNW0mMK6vIr3DsEMyXQs1K/VkMU50zw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR12MB5428

On 20/02/24 13:51, Arnd Bergmann wrote:
> On Tue, Feb 20, 2024, at 08:54, Mukunda,Vijendar wrote:
>> On 20/02/24 12:40, Arnd Bergmann wrote:
>>> On Tue, Feb 20, 2024, at 07:23, Mukunda,Vijendar wrote:
>>>> On 20/02/24 11:43, Arnd Bergmann wrote:
>>>>> On Tue, Feb 20, 2024, at 06:57, Mukunda,Vijendar wrote:
>>>>>> On 19/02/24 15:08, Arnd Bergmann wrote:
>>>>>>> From: Arnd Bergmann <arnd@arndb.de>
>>>>>>> In normal configs, they should all either be built-in or all loadable
>>>>>>> modules anyway, so this simplification does not limit any real usecases.
>>>>>> Tested this patch. SOUNWIRE_AMD flag is not selected by default causing
>>>>>> AMD SOF driver for ACP 6.3 platform is build without enabling SoundWire.
>>>>> Yes, that is what I described. But as SOUNWIRE_AMD is a user visible
>>>>> symbol, there is no problem in expecting users to enable it when they
>>>>> have this hardware, and distros just enable all the drivers anyway.
>>>> Want to set SOUNDWIRE_AMD flag by default, similar to Intel & Qcom
>>>> platforms instead of explicitly enabling the Kconfig option.
>>> Maybe use 'default SND_SOC_SOF_AMD_TOPLEVEL' then?
>> Didn't get your point.
>>
>> Even with the current patch, if we select 'SOUNDWIRE_AMD' flag explicitly
>> AMD ACP63 SOF driver Kconfig option is not visible for user configuration.
>> This results in ACP63 SOF driver won't be built at all.
> I don't understand what you mean here. What I see
> in linux-next both with and without my patch is
>
> config SND_SOC_SOF_AMD_ACP63
>         tristate "SOF support for ACP6.3 platform"
>         depends on SND_SOC_SOF_PCI
>
> so it clearly should be visible as long as SND_SOC_SOF_PCI
> is enabled.
>
> There is still a problem that SND_SOC_SOF_AMD_TOPLEVEL
> can't use my "depends on SOUNDWIRE_AMD || !SOUNDWIRE_AMD"
> trick if SOUNDWIRE_AMD in turn uses
> "default SND_SOC_SOF_AMD_TOPLEVEL", but I don't think you
> meant that, right?
Yes, you are correct.
>
>>> I don't think copying the mistake from the intel driver
>>> is helpful, though I agree it would be nice to be consistent
>>> between them.
>>>
>>> As a general rule, you should not have a Kconfig symbol that
>>> is both user visible and also selected by the drivers that
>>> depend on it.
>>>
>>> To avoid the dependency problems from coming back and keep
>>> the complexity to a minimum, I think there are two logical
>>> ways to handle soundwire:
>>>
>>> a) keep the current drivers/soundwire/Kconfig contents and
>>>    change all the 'select SOUNDWIRE_foo' to 'depends on'.
>> Current patch already using 'depends on SOUNDWIRE_AMD" for
>> SND_SOC_SOF_AMD_SOUNDWIRE Kconfig option.
> Correct, because this is the Kconfig option that actually
> controls whether sound/soc/sof/amd/acp-common.c calls into
> the soundwire-amd module.
>
>> Still we couldn't see SND_SOC_SOF_AMD_ACP63 Kconfig option
>> is enabled.
> I need more information here. Do you have additional
> patches on top of what is in today's linux-next?
> I have it enabled on my build here.
Sorry, it's my bad. My local patches created the problem.
Validated patch on our side. It's working fine.

>
>       Arnd