Messages in this thread Patch in this message | | | From | "Mohandass, Divagar" <> | Subject | RE: [PATCH v4 3/3] eeprom: at24: enable runtime pm support | Date | Fri, 1 Sep 2017 18:26:47 +0000 |
| |
Hi Sakari,
Thanks for the review. My comments below.
--- ^Divagar
>-----Original Message----- >From: Sakari Ailus [mailto:sakari.ailus@iki.fi] >Sent: Thursday, August 31, 2017 9:07 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 v4 3/3] eeprom: at24: enable runtime pm support > >On Thu, Aug 31, 2017 at 11:24:38AM +0000, Mohandass, Divagar wrote: >> >> @@ -743,6 +770,14 @@ 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; >> > >> >Btw. I don't think pm_runtime_set_active() can fail here. In other >> >words it'd be fine to ignore the return value. >> > >> >> Ack >> >> >> >> + >> >> + pm_runtime_enable(&client->dev); >> >> + >> >> /* >> >> * Perform a one-byte test read to verify that the >> >> * chip is functional. >> >> @@ -753,6 +788,8 @@ static int at24_probe(struct i2c_client >> >> *client, const >> >struct i2c_device_id *id) >> >> goto err_clients; >> > >> >I suppose the runtime PM state is re-initialised for a device when a >> >driver is probed, but it'd still be nice to decrement the use count if this >fails. >> >> Ack >> >> >You should also disable PM runtime if probe fails and set the device >> >suspended again. >> > >> >Same for other error cases. I think you'll need a new label. >> > >> >> Can I disable PM runtime and set suspend in the 'err_clients' label itself ? > >Disable, yes, but the get and put calls need to be balanced.
We are performing pm_runtime_put after the first read check and in the error condition, so PM runtime disable alone should be sufficient in the 'err_clients' label. I think it is balanced, your comments ?
> >-- >Sakari Ailus >e-mail: sakari.ailus@iki.fi
| |