On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko <grygorii.strashko@xxxxxx> wrote: > On 06/26/2017 11:49 AM, Ulf Hansson wrote: [cut] >>>> This makes it worse in that sense, that then you don't even need an >>>> i2c slave to trigger the problem. >>> >>> In that case your changelog is a bit misleading. >> >> Yeah, it is! I didn't realize that until I fully investigated the revert option. >> >>> >>> It looks like the commit in question attempted to fix exactly this >>> issue, but it failed, so it should be replaced with something else >>> which is what your patch is effectively doing. >>> >>> IMO you should describe the original problem, explain why that commit >>> is not sufficient to fix it and then describe the final fix. >>> >>> Anyway, after reading the changelog it should be clear that things >>> were broken before the commit in question. >>> >>> And BTW I'm not really sure how the rest of the series is related to this? >> >> Going back in history, I realize the system sleep support in this >> driver has been broken even before the commit $subject patch intends >> to fix. >> However it has been working fine for the ACPI case, because of how the >> ACPI PM domain manages its devices during system sleep. >> >> The commit in question, adds an improvement to the driver, because it >> enables the direct_complete path. For ACPI, that was already working, >> but not for the other cases. So to be able to support the similar >> improvement as the direct_complete path offers, as that isn't working >> for this driver, I tried out using the runtime PM centric path >> instead. That is what the rest of the changes in this series takes >> care of. >> >> Now, as the system sleep support is broken I wanted to make a simple >> fix for that first, via $subject patch. I guess what makes this a bit >> confusing is that I shouldn't point to a certain commit, but rather >> just add a stable tag and update the changelog accordingly. I agree, modulo the below. > > Wouldn't it fix suspend for this driver if you will just replace > dw_i2c_plat_suspend() with pm_runtime_force_suspend() in SET_SYSTEM_SLEEP_PM_OPS() as > you've done in patch 9? > > And, I think, direct_complete path should still work after this also. That's a good point and I was about to mention that. In any case, even if the pm_runtime_resume() added by the $subject patch is necessary to start with, it could be added to the ->suspend callback of the driver instead of the ->complete one, in which case the ACPI path would not be affected by this change. Thanks, Rafael
