[{"id":1759943,"web_url":"http://patchwork.ozlabs.org/comment/1759943/","msgid":"<20170830075329.cyhzitpfwojq3aox@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-08-30T07:53:30","subject":"Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Divagar,\n\nThanks for the update. A few more comments below.\n\nOn Wed, Aug 30, 2017 at 09:41:06AM +0530, Divagar Mohandass wrote:\n> Currently the device is kept in D0, there is an opportunity\n> to save power by enabling runtime pm.\n> \n> Device can be daisy chained from PMIC and we can't rely on I2C core\n> for auto resume/suspend. Driver will decide when to resume/suspend.\n> \n> Signed-off-by: Divagar Mohandass \n> ---\n> drivers/misc/eeprom/at24.c | 39 +++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 39 insertions(+)\n> \n> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c\n> index 2199c42..a670814 100644\n> --- a/drivers/misc/eeprom/at24.c\n> +++ b/drivers/misc/eeprom/at24.c\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> /*\n> * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.\n> @@ -501,11 +502,22 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,\n> static int at24_read(void *priv, unsigned int off, void *val, size_t count)\n> {\n> \tstruct at24_data *at24 = priv;\n> +\tstruct i2c_client *client;\n> \tchar *buf = val;\n> +\tint ret;\n> \n> \tif (unlikely(!count))\n> \t\treturn count;\n> \n> +\tclient = at24_translate_offset(at24, &off);\n> +\n> +\tret = pm_runtime_get_sync(&client->dev);\n> +\tif (ret < 0) {\n> +\t\tpm_runtime_put_noidle(&client->dev);\n> +\t\tpm_runtime_put(&client->dev);\n\nTwo puts are too much here. How about dropping this one?\n\n> +\t\treturn ret;\n> +\t}\n> +\n> \t/*\n> \t * Read data from chip, protecting against concurrent updates\n> \t * from this host, but not from other I2C masters.\n\nIf an error happens between the two chunks, you'll need pm_runtime_put(),\ntoo.\n\n> @@ -527,17 +539,30 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)\n> \n> \tmutex_unlock(&at24->lock);\n> \n> +\tpm_runtime_put(&client->dev);\n> +\n> \treturn 0;\n> }\n> \n> static int at24_write(void *priv, unsigned int off, void *val, size_t count)\n> {\n> \tstruct at24_data *at24 = priv;\n> +\tstruct i2c_client *client;\n> \tchar *buf = val;\n> +\tint ret;\n> \n> \tif (unlikely(!count))\n> \t\treturn -EINVAL;\n> \n> +\tclient = at24_translate_offset(at24, &off);\n> +\n> +\tret = pm_runtime_get_sync(&client->dev);\n> +\tif (ret < 0) {\n> +\t\tpm_runtime_put_noidle(&client->dev);\n> +\t\tpm_runtime_put(&client->dev);\n\nSame here.\n\n> +\t\treturn ret;\n> +\t}\n> +\n> \t/*\n> \t * Write data to chip, protecting against concurrent updates\n> \t * from this host, but not from other I2C masters.\n\nDitto.\n\n> @@ -559,6 +584,8 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)\n> \n> \tmutex_unlock(&at24->lock);\n> \n> +\tpm_runtime_put(&client->dev);\n> +\n> \treturn 0;\n> }\n> \n> @@ -743,6 +770,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)\n> \n> \ti2c_set_clientdata(client, at24);\n> \n> +\t/* enable runtime pm */\n> +\tpm_runtime_get_noresume(&client->dev);\n> +\terr = pm_runtime_set_active(&client->dev);\n> +\tif (err < 0)\n> +\t\tgoto err_clients;\n> +\n> +\tpm_runtime_enable(&client->dev);\n> +\tpm_runtime_put(&client->dev);\n> +\n\nYou're just about to perform a read here. I believe you should move the\nlast put after that.\n\n> \t/*\n> \t * Perform a one-byte test read to verify that the\n> \t * chip is functional.\n> @@ -810,6 +846,9 @@ static int at24_remove(struct i2c_client *client)\n> \tfor (i = 1; i < at24->num_addresses; i++)\n> \t\ti2c_unregister_device(at24->client[i]);\n> \n> +\tpm_runtime_disable(&client->dev);\n> +\tpm_runtime_set_suspended(&client->dev);\n> +\n> \treturn 0;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhySD1RkDz9sQl\n\tfor ;\n\tWed, 30 Aug 2017 17:53:48 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751343AbdH3Hxe (ORCPT );\n\tWed, 30 Aug 2017 03:53:34 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:33740 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1751302AbdH3Hxd (ORCPT\n\t); Wed, 30 Aug 2017 03:53:33 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id 29911600C3;\n\tWed, 30 Aug 2017 10:53:31 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dmxoY-0001Sl-CC; Wed, 30 Aug 2017 10:53:30 +0300"],"Date":"Wed, 30 Aug 2017 10:53:30 +0300","From":"Sakari Ailus ","To":"Divagar Mohandass ","Cc":"robh+dt@kernel.org, mark.rutland@arm.com, wsa@the-dreams.de,\n\tdevicetree@vger.kernel.org, linux-i2c@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, rajmohan.mani@intel.com","Subject":"Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","Message-ID":"<20170830075329.cyhzitpfwojq3aox@valkosipuli.retiisi.org.uk>","References":"<1504066266-30051-1-git-send-email-divagar.mohandass@intel.com>\n\t<1504066266-30051-4-git-send-email-divagar.mohandass@intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1504066266-30051-4-git-send-email-divagar.mohandass@intel.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760116,"web_url":"http://patchwork.ozlabs.org/comment/1760116/","msgid":"<7B8CE47BD58441468D2BB13285B50E6031DE621E@BGSMSX107.gar.corp.intel.com>","list_archive_url":null,"date":"2017-08-30T12:32:07","subject":"RE: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","submitter":{"id":72054,"url":"http://patchwork.ozlabs.org/api/people/72054/","name":"Divagar Mohandass","email":"divagar.mohandass@intel.com"},"content":"Hi Sakari,\n\nThanks for your time.\nMy comments below.\n\n---\n^Divagar\n\n>-----Original Message-----\n>From: Sakari Ailus [mailto:sakari.ailus@iki.fi]\n>Sent: Wednesday, August 30, 2017 1:24 PM\n>To: Mohandass, Divagar \n>Cc: robh+dt@kernel.org; mark.rutland@arm.com; wsa@the-dreams.de;\n>devicetree@vger.kernel.org; linux-i2c@vger.kernel.org; linux-\n>kernel@vger.kernel.org; Mani, Rajmohan \n>Subject: Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support\n>\n>Hi Divagar,\n>\n>Thanks for the update. A few more comments below.\n>\n>On Wed, Aug 30, 2017 at 09:41:06AM +0530, Divagar Mohandass wrote:\n>> Currently the device is kept in D0, there is an opportunity to save\n>> power by enabling runtime pm.\n>>\n>> Device can be daisy chained from PMIC and we can't rely on I2C core\n>> for auto resume/suspend. Driver will decide when to resume/suspend.\n>>\n>> Signed-off-by: Divagar Mohandass \n>> ---\n>> drivers/misc/eeprom/at24.c | 39\n>> +++++++++++++++++++++++++++++++++++++++\n>> 1 file changed, 39 insertions(+)\n>>\n>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c\n>> index 2199c42..a670814 100644\n>> --- a/drivers/misc/eeprom/at24.c\n>> +++ b/drivers/misc/eeprom/at24.c\n>> @@ -24,6 +24,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>>\n>> /*\n>> * I2C EEPROMs from most vendors are inexpensive and mostly\n>interchangeable.\n>> @@ -501,11 +502,22 @@ static ssize_t at24_eeprom_write_i2c(struct\n>> at24_data *at24, const char *buf, static int at24_read(void *priv,\n>> unsigned int off, void *val, size_t count) {\n>> \tstruct at24_data *at24 = priv;\n>> +\tstruct i2c_client *client;\n>> \tchar *buf = val;\n>> +\tint ret;\n>>\n>> \tif (unlikely(!count))\n>> \t\treturn count;\n>>\n>> +\tclient = at24_translate_offset(at24, &off);\n>> +\n>> +\tret = pm_runtime_get_sync(&client->dev);\n>> +\tif (ret < 0) {\n>> +\t\tpm_runtime_put_noidle(&client->dev);\n>> +\t\tpm_runtime_put(&client->dev);\n>\n>Two puts are too much here. How about dropping this one?\n\nAck\nWill fix in next version.\n\n>\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> \t/*\n>> \t * Read data from chip, protecting against concurrent updates\n>> \t * from this host, but not from other I2C masters.\n>\n>If an error happens between the two chunks, you'll need pm_runtime_put(),\n>too.\n>\n\nAck\n\n>> @@ -527,17 +539,30 @@ static int at24_read(void *priv, unsigned int\n>> off, void *val, size_t count)\n>>\n>> \tmutex_unlock(&at24->lock);\n>>\n>> +\tpm_runtime_put(&client->dev);\n>> +\n>> \treturn 0;\n>> }\n>>\n>> static int at24_write(void *priv, unsigned int off, void *val, size_t\n>> count) {\n>> \tstruct at24_data *at24 = priv;\n>> +\tstruct i2c_client *client;\n>> \tchar *buf = val;\n>> +\tint ret;\n>>\n>> \tif (unlikely(!count))\n>> \t\treturn -EINVAL;\n>>\n>> +\tclient = at24_translate_offset(at24, &off);\n>> +\n>> +\tret = pm_runtime_get_sync(&client->dev);\n>> +\tif (ret < 0) {\n>> +\t\tpm_runtime_put_noidle(&client->dev);\n>> +\t\tpm_runtime_put(&client->dev);\n>\n>Same here.\n>\n\nAck\n\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> \t/*\n>> \t * Write data to chip, protecting against concurrent updates\n>> \t * from this host, but not from other I2C masters.\n>\n>Ditto.\n\nAck\n\n>\n>> @@ -559,6 +584,8 @@ static int at24_write(void *priv, unsigned int\n>> off, void *val, size_t count)\n>>\n>> \tmutex_unlock(&at24->lock);\n>>\n>> +\tpm_runtime_put(&client->dev);\n>> +\n>> \treturn 0;\n>> }\n>>\n>> @@ -743,6 +770,15 @@ static int at24_probe(struct i2c_client *client,\n>> const struct i2c_device_id *id)\n>>\n>> \ti2c_set_clientdata(client, at24);\n>>\n>> +\t/* enable runtime pm */\n>> +\tpm_runtime_get_noresume(&client->dev);\n>> +\terr = pm_runtime_set_active(&client->dev);\n>> +\tif (err < 0)\n>> +\t\tgoto err_clients;\n>> +\n>> +\tpm_runtime_enable(&client->dev);\n>> +\tpm_runtime_put(&client->dev);\n>> +\n>\n>You're just about to perform a read here. I believe you should move the last\n>put after that.\n\nAt the end of at24_read we are performing a pm_runtime_put, still we need this change ?\n\n>\n>> \t/*\n>> \t * Perform a one-byte test read to verify that the\n>> \t * chip is functional.\n>> @@ -810,6 +846,9 @@ static int at24_remove(struct i2c_client *client)\n>> \tfor (i = 1; i < at24->num_addresses; i++)\n>> \t\ti2c_unregister_device(at24->client[i]);\n>>\n>> +\tpm_runtime_disable(&client->dev);\n>> +\tpm_runtime_set_suspended(&client->dev);\n>> +\n>> \treturn 0;\n>> }\n>>\n>\n>--\n>Regards,\n>\n>Sakari Ailus\n>e-mail: sakari.ailus@iki.fi","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xj4dX5hwzz9s7F\n\tfor ;\n\tWed, 30 Aug 2017 22:32:16 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751351AbdH3McP convert rfc822-to-8bit (ORCPT\n\t);\n\tWed, 30 Aug 2017 08:32:15 -0400","from mga11.intel.com ([192.55.52.93]:22375 \"EHLO mga11.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751318AbdH3McO (ORCPT );\n\tWed, 30 Aug 2017 08:32:14 -0400","from orsmga001.jf.intel.com ([10.7.209.18])\n\tby fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t30 Aug 2017 05:32:13 -0700","from fmsmsx104.amr.corp.intel.com ([10.18.124.202])\n\tby orsmga001.jf.intel.com with ESMTP; 30 Aug 2017 05:32:13 -0700","from fmsmsx153.amr.corp.intel.com (10.18.125.6) by\n\tfmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP\n\tServer (TLS) id 14.3.319.2; Wed, 30 Aug 2017 05:32:12 -0700","from bgsmsx102.gar.corp.intel.com (10.223.4.172) by\n\tFMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server\n\t(TLS) id 14.3.319.2; Wed, 30 Aug 2017 05:32:12 -0700","from BGSMSX107.gar.corp.intel.com ([169.254.9.43]) by\n\tBGSMSX102.gar.corp.intel.com ([169.254.2.11]) with mapi id\n\t14.03.0319.002; Wed, 30 Aug 2017 18:02:09 +0530"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,448,1498546800\"; d=\"scan'208\";a=\"1167573888\"","From":"\"Mohandass, Divagar\" ","To":"Sakari Ailus ","CC":"\"robh+dt@kernel.org\" ,\n\t\"mark.rutland@arm.com\" ,\n\t\"wsa@the-dreams.de\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"linux-i2c@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"Mani, Rajmohan\" ","Subject":"RE: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","Thread-Topic":"[PATCH v3 3/3] eeprom: at24: enable runtime pm support","Thread-Index":"AQHTIUYgIm5yxjRWak2Glo+47F6ElqKcK2QAgACh8CA=","Date":"Wed, 30 Aug 2017 12:32:07 +0000","Message-ID":"<7B8CE47BD58441468D2BB13285B50E6031DE621E@BGSMSX107.gar.corp.intel.com>","References":"<1504066266-30051-1-git-send-email-divagar.mohandass@intel.com>\n\t<1504066266-30051-4-git-send-email-divagar.mohandass@intel.com>\n\t<20170830075329.cyhzitpfwojq3aox@valkosipuli.retiisi.org.uk>","In-Reply-To":"<20170830075329.cyhzitpfwojq3aox@valkosipuli.retiisi.org.uk>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","dlp-product":"dlpe-windows","dlp-version":"11.0.0.116","dlp-reaction":"no-action","x-originating-ip":"[10.223.10.10]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760124,"web_url":"http://patchwork.ozlabs.org/comment/1760124/","msgid":"<20170830124122.3oipo4ykpzmkzdy2@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-08-30T12:41:23","subject":"Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"On Wed, Aug 30, 2017 at 12:32:07PM +0000, Mohandass, Divagar wrote:\n> >> @@ -743,6 +770,15 @@ static int at24_probe(struct i2c_client *client,\n> >> const struct i2c_device_id *id)\n> >>\n> >> \ti2c_set_clientdata(client, at24);\n> >>\n> >> +\t/* enable runtime pm */\n> >> +\tpm_runtime_get_noresume(&client->dev);\n> >> +\terr = pm_runtime_set_active(&client->dev);\n> >> +\tif (err < 0)\n> >> +\t\tgoto err_clients;\n> >> +\n> >> +\tpm_runtime_enable(&client->dev);\n> >> +\tpm_runtime_put(&client->dev);\n> >> +\n> >\n> >You're just about to perform a read here. I believe you should move the last\n> >put after that.\n> \n> At the end of at24_read we are performing a pm_runtime_put, still we need this change ?\n\nTrue, so this isn't an actual problem.\n\nIt'll still power the chip down when you're about to need it, so it'd make\nsense to perform the check before pm_runtime_put().\n\nI might move the runtime PM setup after the check altogether.","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xj4rN1Rn3z9s7F\n\tfor ;\n\tWed, 30 Aug 2017 22:41:40 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751332AbdH3Ml1 (ORCPT );\n\tWed, 30 Aug 2017 08:41:27 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:36524 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1751302AbdH3Ml0 (ORCPT\n\t); Wed, 30 Aug 2017 08:41:26 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id ED5A3600C3;\n\tWed, 30 Aug 2017 15:41:23 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from )\n\tid 1dn2J9-0001U0-FV; Wed, 30 Aug 2017 15:41:23 +0300"],"Date":"Wed, 30 Aug 2017 15:41:23 +0300","From":"Sakari Ailus ","To":"\"Mohandass, Divagar\" ","Cc":"\"robh+dt@kernel.org\" ,\n\t\"mark.rutland@arm.com\" ,\n\t\"wsa@the-dreams.de\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"linux-i2c@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"Mani, Rajmohan\" ","Subject":"Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","Message-ID":"<20170830124122.3oipo4ykpzmkzdy2@valkosipuli.retiisi.org.uk>","References":"<1504066266-30051-1-git-send-email-divagar.mohandass@intel.com>\n\t<1504066266-30051-4-git-send-email-divagar.mohandass@intel.com>\n\t<20170830075329.cyhzitpfwojq3aox@valkosipuli.retiisi.org.uk>\n\t<7B8CE47BD58441468D2BB13285B50E6031DE621E@BGSMSX107.gar.corp.intel.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<7B8CE47BD58441468D2BB13285B50E6031DE621E@BGSMSX107.gar.corp.intel.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}},{"id":1760344,"web_url":"http://patchwork.ozlabs.org/comment/1760344/","msgid":"<7B8CE47BD58441468D2BB13285B50E6031DE62E9@BGSMSX107.gar.corp.intel.com>","list_archive_url":null,"date":"2017-08-30T17:04:21","subject":"RE: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","submitter":{"id":72054,"url":"http://patchwork.ozlabs.org/api/people/72054/","name":"Divagar Mohandass","email":"divagar.mohandass@intel.com"},"content":"Hi Sakari,\n\nThanks for the review.\nMy comments below.\n\n---\n^Divagar\n\n>-----Original Message-----\n>From: Sakari Ailus [mailto:sakari.ailus@iki.fi]\n>Sent: Wednesday, August 30, 2017 6:11 PM\n>To: Mohandass, Divagar \n>Cc: robh+dt@kernel.org; mark.rutland@arm.com; wsa@the-dreams.de;\n>devicetree@vger.kernel.org; linux-i2c@vger.kernel.org; linux-\n>kernel@vger.kernel.org; Mani, Rajmohan \n>Subject: Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support\n>\n>On Wed, Aug 30, 2017 at 12:32:07PM +0000, Mohandass, Divagar wrote:\n>> >> @@ -743,6 +770,15 @@ static int at24_probe(struct i2c_client\n>> >> *client, const struct i2c_device_id *id)\n>> >>\n>> >> \ti2c_set_clientdata(client, at24);\n>> >>\n>> >> +\t/* enable runtime pm */\n>> >> +\tpm_runtime_get_noresume(&client->dev);\n>> >> +\terr = pm_runtime_set_active(&client->dev);\n>> >> +\tif (err < 0)\n>> >> +\t\tgoto err_clients;\n>> >> +\n>> >> +\tpm_runtime_enable(&client->dev);\n>> >> +\tpm_runtime_put(&client->dev);\n>> >> +\n>> >\n>> >You're just about to perform a read here. I believe you should move\n>> >the last put after that.\n>>\n>> At the end of at24_read we are performing a pm_runtime_put, still we need\n>this change ?\n>\n>True, so this isn't an actual problem.\n>\n>It'll still power the chip down when you're about to need it, so it'd make sense\n>to perform the check before pm_runtime_put().\n>\n>I might move the runtime PM setup after the check altogether.\n\nOk, I will move the pm_runtime_put() after the check and publish the v4.\nMoving the PM setup altogether below, will introduce more error handling in read call.\n\n>\n>--\n>Sakari Ailus\n>e-mail: sakari.ailus@iki.fi","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=linux-i2c-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xjBl935Hyz9sPt\n\tfor ;\n\tThu, 31 Aug 2017 03:07:33 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752023AbdH3RHa convert rfc822-to-8bit (ORCPT\n\t);\n\tWed, 30 Aug 2017 13:07:30 -0400","from mga05.intel.com ([192.55.52.43]:60428 \"EHLO mga05.intel.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751920AbdH3RH3 (ORCPT );\n\tWed, 30 Aug 2017 13:07:29 -0400","from fmsmga001.fm.intel.com ([10.253.24.23])\n\tby fmsmga105.fm.intel.com with ESMTP; 30 Aug 2017 10:07:29 -0700","from fmsmsx103.amr.corp.intel.com ([10.18.124.201])\n\tby fmsmga001.fm.intel.com with ESMTP; 30 Aug 2017 10:07:28 -0700","from fmsmsx156.amr.corp.intel.com (10.18.116.74) by\n\tFMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP\n\tServer (TLS) id 14.3.319.2; Wed, 30 Aug 2017 10:07:26 -0700","from bgsmsx105.gar.corp.intel.com (10.223.43.197) by\n\tfmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP\n\tServer (TLS) id 14.3.319.2; Wed, 30 Aug 2017 10:06:59 -0700","from BGSMSX107.gar.corp.intel.com ([169.254.9.43]) by\n\tBGSMSX105.gar.corp.intel.com ([169.254.3.58]) with mapi id\n\t14.03.0319.002; Wed, 30 Aug 2017 22:34:22 +0530"],"X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.41,449,1498546800\"; d=\"scan'208\";a=\"1189883404\"","From":"\"Mohandass, Divagar\" ","To":"Sakari Ailus ","CC":"\"robh+dt@kernel.org\" ,\n\t\"mark.rutland@arm.com\" ,\n\t\"wsa@the-dreams.de\" ,\n\t\"devicetree@vger.kernel.org\" ,\n\t\"linux-i2c@vger.kernel.org\" ,\n\t\"linux-kernel@vger.kernel.org\" ,\n\t\"Mani, Rajmohan\" ","Subject":"RE: [PATCH v3 3/3] eeprom: at24: enable runtime pm support","Thread-Topic":"[PATCH v3 3/3] eeprom: at24: enable runtime pm support","Thread-Index":"AQHTIUYgIm5yxjRWak2Glo+47F6ElqKcK2QAgACh8CD//65/gIAAh3ag","Date":"Wed, 30 Aug 2017 17:04:21 +0000","Message-ID":"<7B8CE47BD58441468D2BB13285B50E6031DE62E9@BGSMSX107.gar.corp.intel.com>","References":"<1504066266-30051-1-git-send-email-divagar.mohandass@intel.com>\n\t<1504066266-30051-4-git-send-email-divagar.mohandass@intel.com>\n\t<20170830075329.cyhzitpfwojq3aox@valkosipuli.retiisi.org.uk>\n\t<7B8CE47BD58441468D2BB13285B50E6031DE621E@BGSMSX107.gar.corp.intel.com>\n\t<20170830124122.3oipo4ykpzmkzdy2@valkosipuli.retiisi.org.uk>","In-Reply-To":"<20170830124122.3oipo4ykpzmkzdy2@valkosipuli.retiisi.org.uk>","Accept-Language":"en-US","Content-Language":"en-US","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","dlp-product":"dlpe-windows","dlp-version":"11.0.0.116","dlp-reaction":"no-action","x-originating-ip":"[10.223.10.10]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"8BIT","MIME-Version":"1.0","Sender":"linux-i2c-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"linux-i2c@vger.kernel.org"}}]