From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752024AbdJCOX4 (ORCPT ); Tue, 3 Oct 2017 10:23:56 -0400 Received: from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:50490 "EHLO hillosipuli.retiisi.org.uk" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751156AbdJCOXx (ORCPT ); Tue, 3 Oct 2017 10:23:53 -0400 Date: Tue, 3 Oct 2017 17:23:50 +0300 From: "sakari.ailus@iki.fi" To: Tomasz Figa Cc: "Mani, Rajmohan" , "Mohandass, Divagar" , "robh+dt@kernel.org" , "mark.rutland@arm.com" , "wsa@the-dreams.de" , "devicetree@vger.kernel.org" , "linux-i2c@vger.kernel.org" , "linux-kernel@vger.kernel.org" , mika.westerberg@linux.intel.com, linux-pm@vger.kernel.org Subject: Re: [PATCH v6 3/3] eeprom: at24: enable runtime pm support Message-ID: <20171003142349.6uxwfime32dwzxwk@valkosipuli.retiisi.org.uk> References: <1504520928-5191-1-git-send-email-divagar.mohandass@intel.com> <1504520928-5191-4-git-send-email-divagar.mohandass@intel.com> <6F87890CF0F5204F892DEA1EF0D77A5972FC1278@FMSMSX114.amr.corp.intel.com> <20170920084519.asyfjxo6upt36woz@valkosipuli.retiisi.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170920084519.asyfjxo6upt36woz@valkosipuli.retiisi.org.uk> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Sep 20, 2017 at 11:45:20AM +0300, sakari.ailus@iki.fi wrote: > > >> @@ -743,11 +770,17 @@ static int at24_probe(struct i2c_client *client, const > > >> struct i2c_device_id *id) > > >> > > >> i2c_set_clientdata(client, at24); > > >> > > >> + /* enable runtime pm */ > > >> + pm_runtime_get_noresume(&client->dev); > > >> + pm_runtime_set_active(&client->dev); > > >> + pm_runtime_enable(&client->dev); > > > > Do we need this get_noresume/set_active dance? I remember it was for > > some reason needed for PCI devices, but I don't see why for I2C > > anything else than just pm_runtime_enable() would be necessary. > > You specifically do not need (all) this for PCI devices, but AFAIU for I²C > devices you do. The runtime PM status of a device is disabled by default > and the use count is zero, but on ACPI based systems the device is still > powered on. > > > > > Also, we enable runtime PM, but we don't provide any callbacks. If > > there is no callback in any level of the hierarchy, NULL would be > > returned in [3], making [2] return -ENOSYS and [1] fail. The behavior > > depends on subsystem and whether the device is attached to a > > pm_domain. In our particular case I'd guess the device would be in an > > ACPI pm_domain and that would work, but the driver is generic and must > > work in any cases. > > Agreed. I looked at the code and what actually happens here is the runtime_suspend and runtime_resume callbacks aren't set is that the first pm_runtime_put() call itself succeeds because checking the the runtime_suspend callback will be done in the work queue function. This leaves the device in RPM_ACTIVE state, which I don't think is a problem since the driver did not have explicit functions to control the device power state. Further pm_runtime_put() and pm_runtime_get() calls will succeed because the device is in RPM_ACTIVE state. So I see no reason to set the callbacks if they would not actually control regulators, clocks or GPIOs required by the device. Cc linux-pm. -- Kind regards, Sakari Ailus e-mail: sakari.ailus@iki.fi