[PATCH 1/9] i2c: designware: Fix system suspend

[Rafael J. Wysocki]

On Mon, Jun 26, 2017 at 11:11 PM, Rafael J. Wysocki <rafael at kernel.org> wrote:
> On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
> <grygorii.strashko at ti.com> 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