Hi Jonathan, On 03.03.24 17:31, Jonathan Cameron wrote: > On Mon, 26 Feb 2024 22:25:55 +0100 > Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote: > >> The HDC3020 sensor carries out periodic measurements during normal >> operation, but as long as the power supply is enabled, it will carry on >> in low-power modes. In order to avoid that and reduce power consumption, >> the device can be switched to Trigger-on Demand mode, and if possible, >> turn off its regulator. >> >> According to the datasheet, the maximum "Power Up Ready" is 5 ms. >> >> Add resume/suspend pm operations to manage measurement mode and >> regulator state. >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> > Hi Javier, > > I think you leave the power on in a bunch of error paths in the probe() > > Thanks, > > Jonathan > > >> --- >> drivers/iio/humidity/hdc3020.c | 89 ++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 73 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c >> index 1e5d0d4797b1..6848be41e1c8 100644 >> --- a/drivers/iio/humidity/hdc3020.c >> +++ b/drivers/iio/humidity/hdc3020.c >> @@ -20,6 +20,8 @@ >> #include <linux/interrupt.h> >> #include <linux/module.h> >> #include <linux/mutex.h> >> +#include <linux/pm.h> >> +#include <linux/regulator/consumer.h> >> #include <linux/units.h> >> >> #include <asm/unaligned.h> >> @@ -68,6 +70,7 @@ >> >> struct hdc3020_data { >> struct i2c_client *client; >> + struct regulator *vdd_supply; >> /* >> * Ensure that the sensor configuration (currently only heater is >> * supported) will not be changed during the process of reading >> @@ -551,9 +554,45 @@ static const struct iio_info hdc3020_info = { >> .write_event_value = hdc3020_write_thresh, >> }; >> >> -static void hdc3020_stop(void *data) >> +static int hdc3020_power_off(struct hdc3020_data *data) >> { >> - hdc3020_exec_cmd((struct hdc3020_data *)data, HDC3020_EXIT_AUTO); >> + hdc3020_exec_cmd(data, HDC3020_EXIT_AUTO); >> + >> + return regulator_disable(data->vdd_supply); >> +} >> + >> +static int hdc3020_power_on(struct hdc3020_data *data) >> +{ >> + int ret; >> + >> + ret = regulator_enable(data->vdd_supply); >> + if (ret) >> + return ret; >> + >> + fsleep(5000); >> + >> + if (data->client->irq) { >> + /* >> + * The alert output is activated by default upon power up, >> + * hardware reset, and soft reset. Clear the status register. >> + */ >> + ret = hdc3020_exec_cmd(data, HDC3020_S_STATUS); >> + if (ret) { >> + hdc3020_power_off(data); >> + return ret; >> + } >> + } >> + >> + ret = hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0); >> + if (ret) >> + hdc3020_power_off(data); >> + >> + return ret; >> +} >> + >> +static void hdc3020_exit(void *data) >> +{ >> + hdc3020_power_off(data); >> } >> >> static int hdc3020_probe(struct i2c_client *client) >> @@ -569,6 +608,8 @@ static int hdc3020_probe(struct i2c_client *client) >> if (!indio_dev) >> return -ENOMEM; >> >> + dev_set_drvdata(&client->dev, (void *)indio_dev); > No need for casting to void * Then I plagiarised the wrong driver :D I will remove it for v3. > >> + >> data = iio_priv(indio_dev); >> data->client = client; >> mutex_init(&data->lock); >> @@ -580,6 +621,16 @@ static int hdc3020_probe(struct i2c_client *client) >> indio_dev->info = &hdc3020_info; >> indio_dev->channels = hdc3020_channels; >> indio_dev->num_channels = ARRAY_SIZE(hdc3020_channels); >> + >> + data->vdd_supply = devm_regulator_get(&client->dev, "vdd"); >> + if (IS_ERR(data->vdd_supply)) >> + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply), >> + "Unable to get VDD regulator\n"); >> + >> + ret = hdc3020_power_on(data); >> + if (ret) >> + return dev_err_probe(&client->dev, ret, "Power on failed\n"); > > Any error after this point needs to power down the regulator and stop the device. > So the devm_add_action_or_reset needs to be here, not down below. > > When adding this sort of automated handling walk through the various paths > to check where they diverge. If you can put the cleanup code right after > what it cleans up, then you get much less divergence where (in this case) > the power gets left on. > If I am not mistaken, the only case where the regulator will not be powered down is if an irq is defined and the threaded irq request fails, because the next action is a call to devm_add_action_or_reset. A single case is still greater than zero, so I will move the devm_add_function_or_reset up to be the next action after powering up. Thanks and best regards, Javier Carrasco