--- Begin Message ---
- To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>, vkoul@xxxxxxxxxx
- Subject: Re: [PATCH V6 8/8] soundwire: amd: add pm_prepare callback and pm ops support
- From: "Mukunda,Vijendar" <vijendar.mukunda@xxxxxxx>
- Date: Wed, 8 Mar 2023 20:35:43 +0530
- Cc: alsa-devel@xxxxxxxxxxxxxxxx, Basavaraj.Hiregoudar@xxxxxxx, Sunil-kumar.Dommati@xxxxxxx, Mario.Limonciello@xxxxxxx, amadeuszx.slawinski@xxxxxxxxxxxxxxx, Mastan.Katragadda@xxxxxxx, Arungopal.kondaveeti@xxxxxxx, claudiu.beznea@xxxxxxxxxxxxx, Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>, Sanyog Kale <sanyog.r.kale@xxxxxxxxx>, open list <linux-kernel@xxxxxxxxxxxxxxx>
- In-reply-to: <dfb09d78-0620-e535-08b6-894554201ead@linux.intel.com>
- References: <20230307133135.545952-1-Vijendar.Mukunda@amd.com> <20230307133135.545952-9-Vijendar.Mukunda@amd.com> <4330af6a-ce97-53ed-f675-6d3d0ac8f32f@linux.intel.com> <d5a75826-d762-27fc-5820-6826debdecd9@amd.com> <9399110b-bbba-f96e-85ef-a317e8f4d518@linux.intel.com> <4cbbff8a-c596-e9cc-a6cf-6f8b66607505@amd.com> <85aba51e-1feb-5eb0-2e21-b714e217f9e4@linux.intel.com> <2e629a29-093e-46e9-2329-0d5afc916ee4@amd.com> <dfb09d78-0620-e535-08b6-894554201ead@linux.intel.com>
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1
On 08/03/23 19:53, Pierre-Louis Bossart wrote: >>>> device_for_each_child() will invoke amd_resume_child_device() function callback >>>> for each device which will try to resume the child device in this case. >>>> By definition, device_for_each_child() Iterate over @parent's child devices, >>>> and invokes the callback for each. We check the return of amd_resume_child_device() >>>> each time. >>>> If it returns anything other than 0, we break out and return that value. >>>> >>>> In current scenario, As AMP codec is not in runtime suspend state, >>>> pm_request_resume() will return a value as 1. This will break the >>>> sequence for resuming rest of the child devices(JACK codec in our case). >>> Well, yes, now that makes sense, thanks for the details. >>> >>> I think the reason why we didn't see the problem with the Intel code is >>> that both amplifiers are on the same dailink, so they are by >>> construction either both suspended or both active. We never had >>> different types of devices on the same link. >>> >>> I would however suggest this simpler alternative, where only negative >>> return values are returned: >>> >>> ret = pm_request_resume(dev); >>> if (ret < 0) { >>> dev_err(dev, "pm_request_resume failed: %d\n", ret); >>> return ret; >>> } >>> return 0; >>> >>> this would work just fine, no? Sorry its my bad. This would work fine. We will fix it and respin the patch series. >>> No, As explained, pm_request_resume() return value is 1 for active device. >>>> As mentioned in an earlier thread, there are two possible solutions. >>>> 1. check pm runtime suspend state and return 0 if it is not suspended >>>> 2. simply always return 0 for amd_resume_child_device() function callback. >>>> >>>> We opted first one as solution. >>> My suggestion looks like your option 2. It's cleaner IMHO. >> To use option 2, we need to respin the patch series. >> Is it okay if we fix it as supplement patch? > I would vote for re-spinning a new version and ask others to review.
--- End Message ---
