On Mon, Jun 26, 2017 at 11:11 PM, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote: > 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 I meant ->resume, sorry. > callback of the driver instead of the ->complete one, in which case > the ACPI path would not be affected by this change. Thanks, Rafael
