On Thu, Jun 22, 2017 at 01:31:51AM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 21, 2017 at 9:21 PM, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> > The commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming
> > during system suspend"), may suggest to the PM core to try out the so
> > called direct_complete path for system sleep. In this path, the PM core
> > treats a runtime suspended device as it's already in a proper low power
> > state for system sleep, which makes it skip calling the system sleep
> > callbacks for the device, except for the ->prepare() and the ->complete()
> > callback.
> >
> > Moreover, under certain circumstances the PM core may unset the
> > direct_complete flag for a parent device, in case its child device are
> > being system suspended before. In other words, the PM core doesn't skip
> > calling the system sleep callbacks, no matter if the device is runtime
> > suspended or not.
> >
> > In cases of an i2c slave device, the above situation is triggered.
> > Unfortunate, this also breaks the assumption that the i2c device is always
> > runtime resumed, whenever the dw_i2c_plat_suspend() callback is being
> > invoked, which then leads to a regression.
> >
> > More precisely, dw_i2c_plat_suspend() then calls clk_core_disable() and
> > clk_core_unprepare(), for an already disabled/unprepared clock, leading to
> > complaints about clocks calls being wrongly balanced.
> >
> > In cases when the i2c device is attached to the ACPI PM domain, the problem
> > doesn't occur. That's because ACPI's ->suspend() callback, assigned to
> > acpi_subsys_suspend(), calls pm_runtime_resume() for the i2c device.
>
> Which really is expected to happen, so direct_complete should only be
> used along with the ACPI PM domain in this case.
>
> Moreover, in the ACPI PM domain case acpi_subsys_prepare() is supposed
> to do the right thing without dw_i2c_plat_prepare() and the return
> value of the latter will be ignored anyway, so dw_i2c_plat_prepare()
> will only have effect without ACPI PM domain AFAICS.
>
> It looks like commit 8503ff166504 is entirely misguided.
Indeed it is. At the time I suggested that change I did not really
understand how the direct complete is supposed to be used :-/
Thanks Ulf for taking care of this!
I tested this series on Dell XPS 9350 which has touch panel connected to
I2C and suspend/resume still works fine and I can see the controller
going to D3 when the touch panel is idle.
I can perform more comprehensive testing next week.
