Re: [PATCH v2 1/3] iio: humidity: hdc3020: add power management

From: Javier Carrasco <javier.carrasco.cruz@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>, Li peiyu <579lpy@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v2 1/3] iio: humidity: hdc3020: add power management
Date: Sun, 3 Mar 2024 21:05:29 +0100	[thread overview]
Message-ID: <fc81f249-7996-48f4-8573-5e20509c796c@gmail.com> (raw)
In-Reply-To: <20240303163106.25dbf4e5@jic23-huawei>

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@gmail.com> 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@gmail.com>
> 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