This series adds power management for the hdc3020 humidity and
temperature sensor as well as control over the reset signal the device
provides.
The hdc3020 carries out measurements automatically, which is not
necessary in low-power modes. Furthermore, if the low-power
configuration turns off the device, proper initialization is required to
account for the setup times and initial status register value.
This device provides an active low reset signal that must be handled if
connected. This signal can be used by the driver to keep the device
under minimal power consumption during low-power modes if the power
supply stays active.
This series uses char-misc-next as basis to include the last additions
to the driver to handle events [1] as well as the fix to include the
entries in the Makefile and Kconfig files [2].
[1] https://lore.kernel.org/linux-iio/[email protected]/
[2] https://lore.kernel.org/linux-iio/[email protected]/
Signed-off-by: Javier Carrasco <[email protected]>
---
Changes in v2:
- Trigger power off sequence if the power on sequence fails.
- Check return value of hdc3020_power_on() in the probe.
- Remove type casting for void pointer.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Javier Carrasco (3):
iio: humidity: hdc3020: add power management
dt-bindings: iio: humidity: hdc3020: add reset-gpios
iio: humidity: hdc3020: add reset management
.../bindings/iio/humidity/ti,hdc3020.yaml | 5 +
drivers/iio/humidity/hdc3020.c | 105 +++++++++++++++++----
2 files changed, 94 insertions(+), 16 deletions(-)
---
base-commit: d4551c189d6e6a3fcf7f625bd4b273e770fad35a
change-id: 20240217-hdc3020-pm-177983de3cab
Best regards,
--
Javier Carrasco <[email protected]>
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 <[email protected]>
---
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);
+
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");
+
if (client->irq) {
ret = devm_request_threaded_irq(&client->dev, client->irq,
NULL, hdc3020_interrupt_handler,
@@ -588,22 +639,9 @@ static int hdc3020_probe(struct i2c_client *client)
if (ret)
return dev_err_probe(&client->dev, ret,
"Failed to request IRQ\n");
-
- /*
- * 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)
- return ret;
}
- ret = hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0);
- if (ret)
- return dev_err_probe(&client->dev, ret,
- "Unable to set up measurement\n");
-
- ret = devm_add_action_or_reset(&data->client->dev, hdc3020_stop, data);
+ ret = devm_add_action_or_reset(&data->client->dev, hdc3020_exit, data);
if (ret)
return ret;
@@ -614,6 +652,24 @@ static int hdc3020_probe(struct i2c_client *client)
return 0;
}
+static int hdc3020_suspend(struct device *dev)
+{
+ struct iio_dev *iio_dev = dev_get_drvdata(dev);
+ struct hdc3020_data *data = iio_priv(iio_dev);
+
+ return hdc3020_power_off(data);
+}
+
+static int hdc3020_resume(struct device *dev)
+{
+ struct iio_dev *iio_dev = dev_get_drvdata(dev);
+ struct hdc3020_data *data = iio_priv(iio_dev);
+
+ return hdc3020_power_on(data);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(hdc3020_pm_ops, hdc3020_suspend, hdc3020_resume);
+
static const struct i2c_device_id hdc3020_id[] = {
{ "hdc3020" },
{ "hdc3021" },
@@ -633,6 +689,7 @@ MODULE_DEVICE_TABLE(of, hdc3020_dt_ids);
static struct i2c_driver hdc3020_driver = {
.driver = {
.name = "hdc3020",
+ .pm = pm_sleep_ptr(&hdc3020_pm_ops),
.of_match_table = hdc3020_dt_ids,
},
.probe = hdc3020_probe,
--
2.40.1
The HDC3020 provides an active low reset signal that is still not
described in the bindings.
Add reset-gpios to the bindings and the example.
Signed-off-by: Javier Carrasco <[email protected]>
---
Documentation/devicetree/bindings/iio/humidity/ti,hdc3020.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/humidity/ti,hdc3020.yaml b/Documentation/devicetree/bindings/iio/humidity/ti,hdc3020.yaml
index 8b5dedd1a598..b375d307513f 100644
--- a/Documentation/devicetree/bindings/iio/humidity/ti,hdc3020.yaml
+++ b/Documentation/devicetree/bindings/iio/humidity/ti,hdc3020.yaml
@@ -34,6 +34,9 @@ properties:
reg:
maxItems: 1
+ reset-gpios:
+ maxItems: 1
+
required:
- compatible
- reg
@@ -43,6 +46,7 @@ additionalProperties: false
examples:
- |
+ #include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/interrupt-controller/irq.h>
i2c {
#address-cells = <1>;
@@ -54,5 +58,6 @@ examples:
vdd-supply = <&vcc_3v3>;
interrupt-parent = <&gpio3>;
interrupts = <23 IRQ_TYPE_EDGE_RISING>;
+ reset-gpios = <&gpio3 27 GPIO_ACTIVE_LOW>;
};
};
--
2.40.1
The HDC3020 provides an active low reset signal that must be handled if
connected. Asserting this signal turns the device into Trigger-on Demand
measurement mode, reducing its power consumption when no measurements
are required like in low-power modes.
According to the datasheet, the longest "Reset Ready" is 3 ms, which is
only taken into account if the reset signal is defined.
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/iio/humidity/hdc3020.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
index 6848be41e1c8..aa477b5e583e 100644
--- a/drivers/iio/humidity/hdc3020.c
+++ b/drivers/iio/humidity/hdc3020.c
@@ -15,6 +15,7 @@
#include <linux/cleanup.h>
#include <linux/crc8.h>
#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
#include <linux/i2c.h>
#include <linux/init.h>
#include <linux/interrupt.h>
@@ -70,6 +71,7 @@
struct hdc3020_data {
struct i2c_client *client;
+ struct gpio_desc *reset_gpio;
struct regulator *vdd_supply;
/*
* Ensure that the sensor configuration (currently only heater is
@@ -558,6 +560,9 @@ static int hdc3020_power_off(struct hdc3020_data *data)
{
hdc3020_exec_cmd(data, HDC3020_EXIT_AUTO);
+ if (data->reset_gpio)
+ gpiod_set_value_cansleep(data->reset_gpio, 1);
+
return regulator_disable(data->vdd_supply);
}
@@ -571,6 +576,11 @@ static int hdc3020_power_on(struct hdc3020_data *data)
fsleep(5000);
+ if (data->reset_gpio) {
+ gpiod_set_value_cansleep(data->reset_gpio, 0);
+ fsleep(3000);
+ }
+
if (data->client->irq) {
/*
* The alert output is activated by default upon power up,
@@ -627,6 +637,12 @@ static int hdc3020_probe(struct i2c_client *client)
return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
"Unable to get VDD regulator\n");
+ data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(data->reset_gpio))
+ return dev_err_probe(&client->dev, PTR_ERR(data->reset_gpio),
+ "Cannot get reset GPIO\n");
+
ret = hdc3020_power_on(data);
if (ret)
return dev_err_probe(&client->dev, ret, "Power on failed\n");
--
2.40.1
On 26/02/2024 22:25, Javier Carrasco wrote:
> The HDC3020 provides an active low reset signal that is still not
> described in the bindings.
>
> Add reset-gpios to the bindings and the example.
>
> Signed-off-by: Javier Carrasco <[email protected]>
This is a friendly reminder during the review process.
It looks like you received a tag and forgot to add it.
If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
If a tag was not added on purpose, please state why and what changed.
Best regards,
Krzysztof
On Mon, 26 Feb 2024 22:25:55 +0100
Javier Carrasco <[email protected]> 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 <[email protected]>
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 *
> +
> 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 (client->irq) {
> ret = devm_request_threaded_irq(&client->dev, client->irq,
> NULL, hdc3020_interrupt_handler,
> @@ -588,22 +639,9 @@ static int hdc3020_probe(struct i2c_client *client)
> if (ret)
> return dev_err_probe(&client->dev, ret,
> "Failed to request IRQ\n");
> -
> - /*
> - * 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)
> - return ret;
> }
>
> - ret = hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0);
> - if (ret)
> - return dev_err_probe(&client->dev, ret,
> - "Unable to set up measurement\n");
> -
> - ret = devm_add_action_or_reset(&data->client->dev, hdc3020_stop, data);
> + ret = devm_add_action_or_reset(&data->client->dev, hdc3020_exit, data);
> if (ret)
> return ret;
>
> @@ -614,6 +652,24 @@ static int hdc3020_probe(struct i2c_client *client)
> return 0;
> }
>
> +static int hdc3020_suspend(struct device *dev)
> +{
> + struct iio_dev *iio_dev = dev_get_drvdata(dev);
> + struct hdc3020_data *data = iio_priv(iio_dev);
> +
> + return hdc3020_power_off(data);
> +}
> +
> +static int hdc3020_resume(struct device *dev)
> +{
> + struct iio_dev *iio_dev = dev_get_drvdata(dev);
> + struct hdc3020_data *data = iio_priv(iio_dev);
> +
> + return hdc3020_power_on(data);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(hdc3020_pm_ops, hdc3020_suspend, hdc3020_resume);
> +
> static const struct i2c_device_id hdc3020_id[] = {
> { "hdc3020" },
> { "hdc3021" },
> @@ -633,6 +689,7 @@ MODULE_DEVICE_TABLE(of, hdc3020_dt_ids);
> static struct i2c_driver hdc3020_driver = {
> .driver = {
> .name = "hdc3020",
> + .pm = pm_sleep_ptr(&hdc3020_pm_ops),
> .of_match_table = hdc3020_dt_ids,
> },
> .probe = hdc3020_probe,
>
Hi Jonathan,
On 03.03.24 17:31, Jonathan Cameron wrote:
> On Mon, 26 Feb 2024 22:25:55 +0100
> Javier Carrasco <[email protected]> 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 <[email protected]>
> 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