On Mon, Jun 26, 2017 at 9:39 PM, Grygorii Strashko
[off-list ref] wrote:

On 06/26/2017 11:49 AM, Ulf Hansson wrote:

[cut]

quoted

quoted

quoted

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.

quoted

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