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
