Hi Sakari, Thanks for the review. My comments below. --- ^Divagar >-----Original Message----- >From: Sakari Ailus [mailto:sakari.ailus@xxxxxx] >Sent: Thursday, August 31, 2017 9:07 PM >To: Mohandass, Divagar <divagar.mohandass@xxxxxxxxx> >Cc: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; wsa@xxxxxxxxxxxxx; >devicetree@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux- >kernel@xxxxxxxxxxxxxxx; Mani, Rajmohan <rajmohan.mani@xxxxxxxxx> >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@xxxxxx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
