From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ulf Hansson Subject: Re: [PATCH 03/12] PM: i2c-designware-platdrv: Use DPM_FLAG_SMART_PREPARE Date: Mon, 23 Oct 2017 18:57:42 +0200 Message-ID: References: <3806130.B2KCK0tvef@aspire.rjw.lan> <4017204.UeppN1BopX@aspire.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: Received: from mail-io0-f196.google.com ([209.85.223.196]:43647 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751704AbdJWQ5n (ORCPT ); Mon, 23 Oct 2017 12:57:43 -0400 Received: by mail-io0-f196.google.com with SMTP id 134so20844839ioo.0 for ; Mon, 23 Oct 2017 09:57:43 -0700 (PDT) In-Reply-To: <4017204.UeppN1BopX@aspire.rjw.lan> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: "Rafael J. Wysocki" Cc: Linux PM , Bjorn Helgaas , Alan Stern , Greg Kroah-Hartman , LKML , Linux ACPI , Linux PCI , Linux Documentation , Mika Westerberg , Andy Shevchenko , Kevin Hilman , Wolfram Sang , "linux-i2c@vger.kernel.org" , Lee Jones On 16 October 2017 at 03:29, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > Modify i2c-designware-platdrv to set DPM_FLAG_SMART_PREPARE for its > devices and return 0 from the system suspend ->prepare callback > if the device has an ACPI companion object in order to tell the PM > core and middle layers to avoid skipping system suspend/resume > callbacks for the device in that case (which may be problematic, > because the device may be accessed during suspend and resume of > other devices via I2C operation regions then). > > Also the pm_runtime_suspended() check in dw_i2c_plat_prepare() > is not necessary any more, because the core does it when setting > power.direct_complete for the device, so drop it. > > Signed-off-by: Rafael J. Wysocki > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > =================================================================== > --- linux-pm.orig/drivers/i2c/busses/i2c-designware-platdrv.c > +++ linux-pm/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -370,6 +370,8 @@ static int dw_i2c_plat_probe(struct plat > ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev)); > adap->dev.of_node = pdev->dev.of_node; > > + dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE); > + > /* The code below assumes runtime PM to be disabled. */ > WARN_ON(pm_runtime_enabled(&pdev->dev)); > > @@ -433,7 +435,13 @@ MODULE_DEVICE_TABLE(of, dw_i2c_of_match) > #ifdef CONFIG_PM_SLEEP > static int dw_i2c_plat_prepare(struct device *dev) > { > - return pm_runtime_suspended(dev); > + /* > + * If the ACPI companion device object is present for this device, it > + * may be accessed during suspend and resume of other devices via I2C > + * operation regions, so tell the PM core and middle layers to avoid > + * skipping system suspend/resume callbacks for it in that case. > + */ The above scenario can also happens for non-acpi companion devices. That makes this comment a bit confusing to me. > + return !has_acpi_companion(dev); I understand it still works by always returning 1 for the non-acpi case, because the PM core deals with it for the direct_complete path. However it looks rather odd, especially due to the comment above. Perhaps returning pm_runtime_suspended() in the other case make this more clear? Or perhaps clarifying the comment somehow? :-) > } > > static void dw_i2c_plat_complete(struct device *dev) > > Kind regards Uffe