[PATCH 0/4] iio: humidity: hdc3020: add power and reset management

[PATCH 0/4] iio: humidity: hdc3020: add power and reset management

[Javier Carrasco]

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 requires 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]. There is still no branch where everything is
included. Therefore, iio/testing has been used as basis and the fix has
been added to the series as it has been applied to iio/fixes-togreg to
make code testing and validation possible.

[1] https://lore.kernel.org/linux-iio/20240214085350.19382-1-dima.fedrau@gmail.com/
[2] https://lore.kernel.org/linux-iio/20240121135123.71506-1-jic23@kernel.org/

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
Javier Carrasco (3):
      iio: humidity: hdc3020: add power management
      dt-bindings: iio: humidity: hdc3020: add reset-gpios
      iio: humidity: hdc3020: add reset management

Jonathan Cameron (1):
      iio: humidity: hdc3020: Add Makefile, Kconfig and MAINTAINERS entry

 .../bindings/iio/humidity/ti,hdc3020.yaml          |  5 ++
 MAINTAINERS                                        |  8 ++
 drivers/iio/humidity/Kconfig                       | 12 +++
 drivers/iio/humidity/Makefile                      |  1 +
 drivers/iio/humidity/hdc3020.c                     | 97 ++++++++++++++++++----
 5 files changed, 107 insertions(+), 16 deletions(-)
---
base-commit: 3cc5ebd3a2d6247aeba81873d6b040d5d87f7db1
change-id: 20240217-hdc3020-pm-177983de3cab

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH 1/4] iio: humidity: hdc3020: Add Makefile, Kconfig and MAINTAINERS entry

[Javier Carrasco]

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Something when wrong when applying the original patch and only the
c file made it in.  Here the rest of the changes are applied.

Fixes: c9180b8e39be ("iio: humidity: Add driver for ti HDC302x humidity sensors")
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
Cc: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Cc: Li peiyu <579lpy@gmail.com>
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 MAINTAINERS                   |  8 ++++++++
 drivers/iio/humidity/Kconfig  | 12 ++++++++++++
 drivers/iio/humidity/Makefile |  1 +
 3 files changed, 21 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6add3fde252d..fde7456438b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22023,6 +22023,14 @@ F:	Documentation/devicetree/bindings/media/i2c/ti,ds90*
 F:	drivers/media/i2c/ds90*
 F:	include/media/i2c/ds90*
 
+TI HDC302X HUMIDITY DRIVER
+M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
+M:	Li peiyu <579lpy@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/humidity/ti,hdc3020.yaml
+F:	drivers/iio/humidity/hdc3020.c
+
 TI ICSSG ETHERNET DRIVER (ICSSG)
 R:	MD Danish Anwar <danishanwar@ti.com>
 R:	Roger Quadros <rogerq@kernel.org>
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
index 2de5494e7c22..b15b7a3b66d5 100644
--- a/drivers/iio/humidity/Kconfig
+++ b/drivers/iio/humidity/Kconfig
@@ -48,6 +48,18 @@ config HDC2010
 	  To compile this driver as a module, choose M here: the module
 	  will be called hdc2010.
 
+config HDC3020
+	tristate "TI HDC3020 relative humidity and temperature sensor"
+	depends on I2C
+	select CRC8
+	help
+	  Say yes here to build support for the Texas Instruments
+	  HDC3020, HDC3021 and HDC3022 relative humidity and temperature
+	  sensors.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called hdc3020.
+
 config HID_SENSOR_HUMIDITY
 	tristate "HID Environmental humidity sensor"
 	depends on HID_SENSOR_HUB
diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
index f19ff3de97c5..5fbeef299f61 100644
--- a/drivers/iio/humidity/Makefile
+++ b/drivers/iio/humidity/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_AM2315) += am2315.o
 obj-$(CONFIG_DHT11) += dht11.o
 obj-$(CONFIG_HDC100X) += hdc100x.o
 obj-$(CONFIG_HDC2010) += hdc2010.o
+obj-$(CONFIG_HDC3020) += hdc3020.o
 obj-$(CONFIG_HID_SENSOR_HUMIDITY) += hid-sensor-humidity.o
 
 hts221-y := hts221_core.o \

-- 
2.40.1

[PATCH 2/4] iio: humidity: hdc3020: add power management

[Javier Carrasco]

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>
---
 drivers/iio/humidity/hdc3020.c | 81 +++++++++++++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
index 11ede97a31d7..0da5c5c41cd2 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,39 @@ static const struct iio_info hdc3020_info = {
 	.write_event_value = hdc3020_write_thresh,
 };
 
-static void hdc3020_stop(void *data)
+static int hdc3020_power_on(struct hdc3020_data *data)
 {
-	hdc3020_exec_cmd((struct hdc3020_data *)data, HDC3020_EXIT_AUTO);
+	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)
+			return ret;
+	}
+
+	return hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0);
+}
+
+static int hdc3020_power_off(struct hdc3020_data *data)
+{
+	hdc3020_exec_cmd(data, HDC3020_EXIT_AUTO);
+
+	return regulator_disable(data->vdd_supply);
+}
+
+static void hdc3020_exit(void *data)
+{
+	hdc3020_power_off((struct hdc3020_data *)data);
 }
 
 static int hdc3020_probe(struct i2c_client *client)
@@ -569,6 +602,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 +615,14 @@ 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");
+
+	hdc3020_power_on(data);
+
 	if (client->irq) {
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
 						NULL, hdc3020_interrupt_handler,
@@ -588,22 +631,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 +644,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 +681,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

Re: [PATCH 2/4] iio: humidity: hdc3020: add power management

[Jonathan Cameron]

On Tue, 20 Feb 2024 22:14:56 +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,

Comments inline. Mainly that you should not have side effects if the power
up fails and you should fail probe.

Thanks,

Jonathan

> ---
>  drivers/iio/humidity/hdc3020.c | 81 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/humidity/hdc3020.c b/drivers/iio/humidity/hdc3020.c
> index 11ede97a31d7..0da5c5c41cd2 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,39 @@ static const struct iio_info hdc3020_info = {
>  	.write_event_value = hdc3020_write_thresh,
>  };
>  
> -static void hdc3020_stop(void *data)
> +static int hdc3020_power_on(struct hdc3020_data *data)
>  {
> -	hdc3020_exec_cmd((struct hdc3020_data *)data, HDC3020_EXIT_AUTO);
> +	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)
> +			return ret;
> +	}
> +
> +	return hdc3020_exec_cmd(data, HDC3020_S_AUTO_10HZ_MOD0);
Expectation of a power on fail, in probe at least is it should cleanup after
itself.  It's messier in resume because there isn't anything sensible to do
about it, but we should keep to the convention of no side effects on
failure.

As such if either of the later parts of this fail, you should power
down the regulator before returning.


> +}
> +
> +static int hdc3020_power_off(struct hdc3020_data *data)
> +{
> +	hdc3020_exec_cmd(data, HDC3020_EXIT_AUTO);
> +
> +	return regulator_disable(data->vdd_supply);
> +}
> +
> +static void hdc3020_exit(void *data)
> +{
> +	hdc3020_power_off((struct hdc3020_data *)data);

Trivial but no need to cast a void * to anything as the C standard says this
is fine as implicit.

>  }
>  
>  static int hdc3020_probe(struct i2c_client *client)
> @@ -569,6 +602,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 +615,14 @@ 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");
> +
> +	hdc3020_power_on(data)

Check return value. We want to fail probe if the power up didn't work!

> +
>  	if (client->irq) {
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
>  						NULL, hdc3020_interrupt_handler,
> @@ -588,22 +631,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 +644,24 @@ static int hdc3020_probe(struct i2c_client *client)
>  	return 0;
>  }
>

[PATCH 3/4] dt-bindings: iio: humidity: hdc3020: add reset-gpios

[Javier Carrasco]

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

Re: [PATCH 3/4] dt-bindings: iio: humidity: hdc3020: add reset-gpios

[Krzysztof Kozlowski]

On 20/02/2024 22:14, 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 <javier.carrasco.cruz@gmail.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 4/4] iio: humidity: hdc3020: add reset management

[Javier Carrasco]

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 <javier.carrasco.cruz@gmail.com>
---
 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 0da5c5c41cd2..bd2f57a7eedc 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
@@ -564,6 +566,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,
@@ -581,6 +588,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);
 }
 
@@ -621,6 +631,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");
+
 	hdc3020_power_on(data);
 
 	if (client->irq) {

-- 
2.40.1

Re: [PATCH 4/4] iio: humidity: hdc3020: add reset management

[Jonathan Cameron]

On Tue, 20 Feb 2024 22:14:58 +0100
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> 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 <javier.carrasco.cruz@gmail.com>
Rest of this series looks good to me.

Hopefully you'll soon have a sensible base and can drop patch 1.
Ideal is Greg takes my pull request shortly and I rebase the tree
asap so that I can get a clean base for a second pull request late
this week.  The aim is 1 week in linux-next (usually in via char-misc
not my tree) before the merge window opens.  So busy week ;)

Thanks,

Jonathan