On 7.5.2017 14:42, Jonathan Cameron wrote: > On 05/05/17 12:19, Mikko Koivunen wrote: >> Set sensor measurement off after probe fail in pm_runtime_set_active() or >> iio_device_register(). Without this change sensor measurement stays on >> even though probe fails on these calls. >> >> This is maybe rare case, but causes constant power drain without any >> benefits when it happens. Power drain is 20-500uA, typically 180uA. >> >> Signed-off-by: Mikko Koivunen <mikko.koivunen@xxxxxxxxxxxxxxxxx> > Looks good to me, but I will be looking for Acks from Daniel on this > set. Daniel let me know if you want to pass on responsibility for the > driver (perhaps Mikko if willing?) > > one tiny point inline. >> --- >> Sensor measurement is turned on rpr0521_init() so in probe it should be >> turned off in error cases after that. With CONFIG_PM it's probably >> unnecessary for iio_device_register()-fail, but without CONFIG_PM it is >> needed. Writing power off twice when CONFIG_PM enabled is ok. >> >> drivers/iio/light/rpr0521.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c >> index 03504f6..5d077f4 100644 >> --- a/drivers/iio/light/rpr0521.c >> +++ b/drivers/iio/light/rpr0521.c >> @@ -516,13 +516,26 @@ static int rpr0521_probe(struct i2c_client *client, >> >> ret = pm_runtime_set_active(&client->dev); >> if (ret < 0) >> - return ret; >> + goto err_poweroff; >> >> pm_runtime_enable(&client->dev); >> pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS); >> pm_runtime_use_autosuspend(&client->dev); >> >> - return iio_device_register(indio_dev); >> + ret = iio_device_register(indio_dev); >> + if (ret) >> + goto err_pm_disable; >> + >> + return 0; >> + >> +err_pm_disable: >> + pm_runtime_disable(&client->dev); >> + pm_runtime_set_suspended(&client->dev); >> + pm_runtime_put_noidle(&client->dev); >> +err_poweroff: >> + (void) rpr0521_poweroff(data); > shouldn't need the void cast. It is there because discarding the return value is done on purpose, not by accident. Should I still remove it? >> + >> + return ret; >> } >> >> static int rpr0521_remove(struct i2c_client *client) >> > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
