Messages in this thread Patch in this message | | | From | "Mohandass, Divagar" <> | Subject | RE: [PATCH v3 3/3] eeprom: at24: enable runtime pm support | Date | Wed, 30 Aug 2017 17:04:21 +0000 |
| |
Hi Sakari,
Thanks for the review. My comments below.
--- ^Divagar
>-----Original Message----- >From: Sakari Ailus [mailto:sakari.ailus@iki.fi] >Sent: Wednesday, August 30, 2017 6:11 PM >To: Mohandass, Divagar <divagar.mohandass@intel.com> >Cc: 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; Mani, Rajmohan <rajmohan.mani@intel.com> >Subject: Re: [PATCH v3 3/3] eeprom: at24: enable runtime pm support > >On Wed, Aug 30, 2017 at 12:32:07PM +0000, Mohandass, Divagar wrote: >> >> @@ -743,6 +770,15 @@ 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); >> >> + err = pm_runtime_set_active(&client->dev); >> >> + if (err < 0) >> >> + goto err_clients; >> >> + >> >> + pm_runtime_enable(&client->dev); >> >> + pm_runtime_put(&client->dev); >> >> + >> > >> >You're just about to perform a read here. I believe you should move >> >the last put after that. >> >> At the end of at24_read we are performing a pm_runtime_put, still we need >this change ? > >True, so this isn't an actual problem. > >It'll still power the chip down when you're about to need it, so it'd make sense >to perform the check before pm_runtime_put(). > >I might move the runtime PM setup after the check altogether.
Ok, I will move the pm_runtime_put() after the check and publish the v4. Moving the PM setup altogether below, will introduce more error handling in read call.
> >-- >Sakari Ailus >e-mail: sakari.ailus@iki.fi
| |