[PATCH 1/2] regulator: core: Add new notification for enabling of regulator

[PATCH 1/2] regulator: core: Add new notification for enabling of regulator

[Harald Geyer]

This is useful for devices, which need some time to start up, to help
the drivers track how long the supply has been up already. Ie whether
it can safely talk to the HW or needs to wait.

Signed-off-by: Harald Geyer <harald-95f8Dae0BrPYtjvyW6yDsg@public.gmane.org>
---
If this change gets accepted soon, it can go in via the regulator tree
and everything will be in place for patch 2/2 to go in via iio when I'm
done with longterm tests.

 drivers/regulator/core.c           | 2 ++
 include/linux/regulator/consumer.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 04baac9..6b9bb1b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2162,6 +2162,8 @@ static int _regulator_enable(struct regulator_dev *rdev)
 			if (ret < 0)
 				return ret;
 
+			_notifier_call_chain(rdev, REGULATOR_EVENT_ENABLE,
+					     NULL);
 		} else if (ret < 0) {
 			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
 			return ret;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ea0fffa..df176d7 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -119,6 +119,7 @@ struct regmap;
 #define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE	0x200
 #define REGULATOR_EVENT_PRE_DISABLE		0x400
 #define REGULATOR_EVENT_ABORT_DISABLE		0x800
+#define REGULATOR_EVENT_ENABLE			0x1000
 
 /*
  * Regulator errors that can be queried using regulator_get_error_flags
-- 
2.1.4

[PATCH 1/2] regulator: core: Add new notification for enabling of regulator

[Harald Geyer]

This is useful for devices, which need some time to start up, to help
the drivers track how long the supply has been up already. Ie whether
it can safely talk to the HW or needs to wait.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
If this change gets accepted soon, it can go in via the regulator tree
and everything will be in place for patch 2/2 to go in via iio when I'm
done with longterm tests.

 drivers/regulator/core.c           | 2 ++
 include/linux/regulator/consumer.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 04baac9..6b9bb1b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2162,6 +2162,8 @@ static int _regulator_enable(struct regulator_dev *rdev)
 			if (ret < 0)
 				return ret;
 
+			_notifier_call_chain(rdev, REGULATOR_EVENT_ENABLE,
+					     NULL);
 		} else if (ret < 0) {
 			rdev_err(rdev, "is_enabled() failed: %d\n", ret);
 			return ret;
diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index ea0fffa..df176d7 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -119,6 +119,7 @@ struct regmap;
 #define REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE	0x200
 #define REGULATOR_EVENT_PRE_DISABLE		0x400
 #define REGULATOR_EVENT_ABORT_DISABLE		0x800
+#define REGULATOR_EVENT_ENABLE			0x1000
 
 /*
  * Regulator errors that can be queried using regulator_get_error_flags
-- 
2.1.4

[PATCH 2/2] [RFC] iio: dht11: Add optional support for supply control via regulator framework

[Harald Geyer]

If a supply is provided via DT, release the supply whenever we don't read
the sensor. Also use notifications to track the actual status of the
supply to ensure that we meet the timing requirements stated in the
datasheet.

This change is motivated by the hope that these sensors will work more
reliably if power-cycled regularly.

Signed-off-by: Harald Geyer <harald-95f8Dae0BrPYtjvyW6yDsg@public.gmane.org>
---
This is an RFC this time, because I want to run extensive long term tests
with DHT11 and DHT22 before officially proposing this for inclusion. I
have learned my lessons with this quirky bits of HW... :(

However since this will take a lot of time (mostly physical, but also
man hours), I'd like to get review comments now instead of risking a
second iteration of long term tests.

 .../devicetree/bindings/iio/humidity/dht11.txt     | 10 ++++
 drivers/iio/humidity/dht11.c                       | 57 ++++++++++++++++++++--
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11.txt b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
index ecc24c19..ab9ea18 100644
--- a/Documentation/devicetree/bindings/iio/humidity/dht11.txt
+++ b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
@@ -6,9 +6,19 @@ Required properties:
     line, see "gpios property" in
     Documentation/devicetree/bindings/gpio/gpio.txt.
 
+Optional properties:
+  - vdd-supply: phandle to the regulator node, for details see
+    Documentation/devicetree/bindings/regulator/regulator.txt
+    On Linux the driver disables the regulator after 4 seconds of
+    inactivity, to save power and to ensure that the hardware is
+    reset regularly, because it was found to randomly stop responding
+    otherwise. However for this to work all other consumers of the
+    regulator must cooperate (disable the regulator when possible too).
+
 Example:
 
 humidity_sensor {
 	compatible = "dht11";
 	gpios = <&gpio0 6 0>;
+	vdd-supply = <&sensor_supply>;
 }
diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 2a22ad9..5bce712 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -34,12 +34,16 @@
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <linux/timekeeping.h>
+#include <linux/regulator/consumer.h>
+#include <linux/notifier.h>
 
 #include <linux/iio/iio.h>
 
 #define DRIVER_NAME	"dht11"
 
 #define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
+#define DHT11_POWER_ON_TIME 2000000000 /* must be less the MAX_INT */
+#define DHT11_POWEROFF_DELAY 4000 /* ms */
 
 #define DHT11_EDGES_PREAMBLE 2
 #define DHT11_BITS_PER_READ 40
@@ -84,6 +88,10 @@ struct dht11 {
 	int				gpio;
 	int				irq;
 
+	struct regulator		*supply;
+	struct notifier_block		nb;
+	s64				timestamp_enabled;
+
 	struct completion		completion;
 	/* The iio sysfs interface doesn't prevent concurrent reads: */
 	struct mutex			lock;
@@ -97,6 +105,21 @@ struct dht11 {
 	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
 };
 
+static int dht11_regulator_update(struct notifier_block *nb,
+				  unsigned long event,
+				  void *ignored)
+{
+	struct dht11 *dht11 = container_of(nb, struct dht11, nb);
+
+	if (event & REGULATOR_EVENT_DISABLE)
+		dht11->timestamp_enabled = -1;
+	else if (event & REGULATOR_EVENT_ENABLE)
+		if (dht11->timestamp_enabled == -1)
+			dht11->timestamp_enabled = ktime_get_boot_ns();
+
+	return NOTIFY_OK;
+}
+
 #ifdef CONFIG_DYNAMIC_DEBUG
 /*
  * dht11_edges_print: show the data as actually received by the
@@ -203,9 +226,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 {
 	struct dht11 *dht11 = iio_priv(iio_dev);
 	int ret, timeres, offset;
+	s64 time;
 
 	mutex_lock(&dht11->lock);
-	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_boot_ns()) {
+	time = ktime_get_boot_ns();
+	if (dht11->timestamp + DHT11_DATA_VALID_TIME < time) {
+		if (dht11->supply) {
+			ret = regulator_enable(dht11->supply);
+			if (ret) {
+				dev_err(dht11->dev, "Can't enable supply\n");
+				goto err_reg;
+			}
+			time -= dht11->timestamp_enabled + DHT11_POWER_ON_TIME;
+			if (time < 0)
+				msleep(((int)(-time)) / 1000000);
+		}
+
 		timeres = ktime_get_resolution_ns();
 		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
 		if (timeres > DHT11_MIN_TIMERES) {
@@ -266,8 +302,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 				break;
 		}
 
+err:
+		if (dht11->supply)
+			regulator_disable_deferred(dht11->supply,
+						   DHT11_POWEROFF_DELAY);
+
 		if (ret)
-			goto err;
+			goto err_reg;
 	}
 
 	ret = IIO_VAL_INT;
@@ -277,7 +318,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		*val = dht11->humidity;
 	else
 		ret = -EINVAL;
-err:
+
+err_reg:
 	dht11->num_edges = -1;
 	mutex_unlock(&dht11->lock);
 	return ret;
@@ -332,6 +374,15 @@ static int dht11_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	dht11->timestamp_enabled = -1;
+	dht11->supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(dht11->supply)) {
+		dht11->supply = NULL;
+	} else {
+		dht11->nb.notifier_call = &dht11_regulator_update;
+		devm_regulator_register_notifier(dht11->supply, &dht11->nb);
+	}
+
 	dht11->timestamp = ktime_get_boot_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
 
-- 
2.1.4

[PATCH 2/2] [RFC] iio: dht11: Add optional support for supply control via regulator framework

[Harald Geyer]

If a supply is provided via DT, release the supply whenever we don't read
the sensor. Also use notifications to track the actual status of the
supply to ensure that we meet the timing requirements stated in the
datasheet.

This change is motivated by the hope that these sensors will work more
reliably if power-cycled regularly.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
This is an RFC this time, because I want to run extensive long term tests
with DHT11 and DHT22 before officially proposing this for inclusion. I
have learned my lessons with this quirky bits of HW... :(

However since this will take a lot of time (mostly physical, but also
man hours), I'd like to get review comments now instead of risking a
second iteration of long term tests.

 .../devicetree/bindings/iio/humidity/dht11.txt     | 10 ++++
 drivers/iio/humidity/dht11.c                       | 57 ++++++++++++++++++++--
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11.txt b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
index ecc24c19..ab9ea18 100644
--- a/Documentation/devicetree/bindings/iio/humidity/dht11.txt
+++ b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
@@ -6,9 +6,19 @@ Required properties:
     line, see "gpios property" in
     Documentation/devicetree/bindings/gpio/gpio.txt.
 
+Optional properties:
+  - vdd-supply: phandle to the regulator node, for details see
+    Documentation/devicetree/bindings/regulator/regulator.txt
+    On Linux the driver disables the regulator after 4 seconds of
+    inactivity, to save power and to ensure that the hardware is
+    reset regularly, because it was found to randomly stop responding
+    otherwise. However for this to work all other consumers of the
+    regulator must cooperate (disable the regulator when possible too).
+
 Example:
 
 humidity_sensor {
 	compatible = "dht11";
 	gpios = <&gpio0 6 0>;
+	vdd-supply = <&sensor_supply>;
 }
diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
index 2a22ad9..5bce712 100644
--- a/drivers/iio/humidity/dht11.c
+++ b/drivers/iio/humidity/dht11.c
@@ -34,12 +34,16 @@
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
 #include <linux/timekeeping.h>
+#include <linux/regulator/consumer.h>
+#include <linux/notifier.h>
 
 #include <linux/iio/iio.h>
 
 #define DRIVER_NAME	"dht11"
 
 #define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
+#define DHT11_POWER_ON_TIME 2000000000 /* must be less the MAX_INT */
+#define DHT11_POWEROFF_DELAY 4000 /* ms */
 
 #define DHT11_EDGES_PREAMBLE 2
 #define DHT11_BITS_PER_READ 40
@@ -84,6 +88,10 @@ struct dht11 {
 	int				gpio;
 	int				irq;
 
+	struct regulator		*supply;
+	struct notifier_block		nb;
+	s64				timestamp_enabled;
+
 	struct completion		completion;
 	/* The iio sysfs interface doesn't prevent concurrent reads: */
 	struct mutex			lock;
@@ -97,6 +105,21 @@ struct dht11 {
 	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
 };
 
+static int dht11_regulator_update(struct notifier_block *nb,
+				  unsigned long event,
+				  void *ignored)
+{
+	struct dht11 *dht11 = container_of(nb, struct dht11, nb);
+
+	if (event & REGULATOR_EVENT_DISABLE)
+		dht11->timestamp_enabled = -1;
+	else if (event & REGULATOR_EVENT_ENABLE)
+		if (dht11->timestamp_enabled == -1)
+			dht11->timestamp_enabled = ktime_get_boot_ns();
+
+	return NOTIFY_OK;
+}
+
 #ifdef CONFIG_DYNAMIC_DEBUG
 /*
  * dht11_edges_print: show the data as actually received by the
@@ -203,9 +226,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 {
 	struct dht11 *dht11 = iio_priv(iio_dev);
 	int ret, timeres, offset;
+	s64 time;
 
 	mutex_lock(&dht11->lock);
-	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_boot_ns()) {
+	time = ktime_get_boot_ns();
+	if (dht11->timestamp + DHT11_DATA_VALID_TIME < time) {
+		if (dht11->supply) {
+			ret = regulator_enable(dht11->supply);
+			if (ret) {
+				dev_err(dht11->dev, "Can't enable supply\n");
+				goto err_reg;
+			}
+			time -= dht11->timestamp_enabled + DHT11_POWER_ON_TIME;
+			if (time < 0)
+				msleep(((int)(-time)) / 1000000);
+		}
+
 		timeres = ktime_get_resolution_ns();
 		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
 		if (timeres > DHT11_MIN_TIMERES) {
@@ -266,8 +302,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 				break;
 		}
 
+err:
+		if (dht11->supply)
+			regulator_disable_deferred(dht11->supply,
+						   DHT11_POWEROFF_DELAY);
+
 		if (ret)
-			goto err;
+			goto err_reg;
 	}
 
 	ret = IIO_VAL_INT;
@@ -277,7 +318,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
 		*val = dht11->humidity;
 	else
 		ret = -EINVAL;
-err:
+
+err_reg:
 	dht11->num_edges = -1;
 	mutex_unlock(&dht11->lock);
 	return ret;
@@ -332,6 +374,15 @@ static int dht11_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	dht11->timestamp_enabled = -1;
+	dht11->supply = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(dht11->supply)) {
+		dht11->supply = NULL;
+	} else {
+		dht11->nb.notifier_call = &dht11_regulator_update;
+		devm_regulator_register_notifier(dht11->supply, &dht11->nb);
+	}
+
 	dht11->timestamp = ktime_get_boot_ns() - DHT11_DATA_VALID_TIME - 1;
 	dht11->num_edges = -1;
 
-- 
2.1.4

Re: [PATCH 2/2] [RFC] iio: dht11: Add optional support for supply control via regulator framework

[Jonathan Cameron]

On 23/02/17 17:06, Harald Geyer wrote:
> If a supply is provided via DT, release the supply whenever we don't read
> the sensor. Also use notifications to track the actual status of the
> supply to ensure that we meet the timing requirements stated in the
> datasheet.
> 
> This change is motivated by the hope that these sensors will work more
> reliably if power-cycled regularly.
> 
> Signed-off-by: Harald Geyer <harald-95f8Dae0BrPYtjvyW6yDsg@public.gmane.org>
I'd certainly rather this was driven by the powersaving argument but I guess
it's an added bonus if we happen to improve on the hardware hangs.

Minor comment inline. Looks good to me.

Jonathan
> ---
> This is an RFC this time, because I want to run extensive long term tests
> with DHT11 and DHT22 before officially proposing this for inclusion. I
> have learned my lessons with this quirky bits of HW... :(
> 
> However since this will take a lot of time (mostly physical, but also
> man hours), I'd like to get review comments now instead of risking a
> second iteration of long term tests.
> 
>  .../devicetree/bindings/iio/humidity/dht11.txt     | 10 ++++
>  drivers/iio/humidity/dht11.c                       | 57 ++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11.txt b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> index ecc24c19..ab9ea18 100644
> --- a/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> @@ -6,9 +6,19 @@ Required properties:
>      line, see "gpios property" in
>      Documentation/devicetree/bindings/gpio/gpio.txt.
>  
> +Optional properties:
> +  - vdd-supply: phandle to the regulator node, for details see
> +    Documentation/devicetree/bindings/regulator/regulator.txt
> +    On Linux the driver disables the regulator after 4 seconds of
> +    inactivity, to save power and to ensure that the hardware is
> +    reset regularly, because it was found to randomly stop responding
> +    otherwise. However for this to work all other consumers of the
> +    regulator must cooperate (disable the regulator when possible too).
> +
>  Example:
>  
>  humidity_sensor {
>  	compatible = "dht11";
>  	gpios = <&gpio0 6 0>;
> +	vdd-supply = <&sensor_supply>;
>  }
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 2a22ad9..5bce712 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -34,12 +34,16 @@
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
>  #include <linux/timekeeping.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/notifier.h>
>  
>  #include <linux/iio/iio.h>
>  
>  #define DRIVER_NAME	"dht11"
>  
>  #define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
> +#define DHT11_POWER_ON_TIME 2000000000 /* must be less the MAX_INT */
> +#define DHT11_POWEROFF_DELAY 4000 /* ms */
>  
>  #define DHT11_EDGES_PREAMBLE 2
>  #define DHT11_BITS_PER_READ 40
> @@ -84,6 +88,10 @@ struct dht11 {
>  	int				gpio;
>  	int				irq;
>  
> +	struct regulator		*supply;
> +	struct notifier_block		nb;
> +	s64				timestamp_enabled;
> +
>  	struct completion		completion;
>  	/* The iio sysfs interface doesn't prevent concurrent reads: */
>  	struct mutex			lock;
> @@ -97,6 +105,21 @@ struct dht11 {
>  	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>  };
>  
> +static int dht11_regulator_update(struct notifier_block *nb,
> +				  unsigned long event,
> +				  void *ignored)
> +{
> +	struct dht11 *dht11 = container_of(nb, struct dht11, nb);
> +
> +	if (event & REGULATOR_EVENT_DISABLE)
> +		dht11->timestamp_enabled = -1;
> +	else if (event & REGULATOR_EVENT_ENABLE)
> +		if (dht11->timestamp_enabled == -1)
This nesting is a bit ugly, why not combine the else if and the if?
> +			dht11->timestamp_enabled = ktime_get_boot_ns();
> +
> +	return NOTIFY_OK;
> +}
> +
>  #ifdef CONFIG_DYNAMIC_DEBUG
>  /*
>   * dht11_edges_print: show the data as actually received by the
> @@ -203,9 +226,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  {
>  	struct dht11 *dht11 = iio_priv(iio_dev);
>  	int ret, timeres, offset;
> +	s64 time;
>  
>  	mutex_lock(&dht11->lock);
> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_boot_ns()) {
> +	time = ktime_get_boot_ns();
> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < time) {
> +		if (dht11->supply) {
> +			ret = regulator_enable(dht11->supply);
> +			if (ret) {
> +				dev_err(dht11->dev, "Can't enable supply\n");
> +				goto err_reg;
> +			}
> +			time -= dht11->timestamp_enabled + DHT11_POWER_ON_TIME;
> +			if (time < 0)
> +				msleep(((int)(-time)) / 1000000);
> +		}
> +
>  		timeres = ktime_get_resolution_ns();
>  		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
>  		if (timeres > DHT11_MIN_TIMERES) {
> @@ -266,8 +302,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  				break;
>  		}
>  
> +err:
> +		if (dht11->supply)
> +			regulator_disable_deferred(dht11->supply,
> +						   DHT11_POWEROFF_DELAY);
> +
>  		if (ret)
> -			goto err;
> +			goto err_reg;
>  	}
>  
>  	ret = IIO_VAL_INT;
> @@ -277,7 +318,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		*val = dht11->humidity;
>  	else
>  		ret = -EINVAL;
> -err:
> +
> +err_reg:
>  	dht11->num_edges = -1;
>  	mutex_unlock(&dht11->lock);
>  	return ret;
> @@ -332,6 +374,15 @@ static int dht11_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	dht11->timestamp_enabled = -1;
> +	dht11->supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(dht11->supply)) {
> +		dht11->supply = NULL;
> +	} else {
> +		dht11->nb.notifier_call = &dht11_regulator_update;
> +		devm_regulator_register_notifier(dht11->supply, &dht11->nb);
> +	}
> +
>  	dht11->timestamp = ktime_get_boot_ns() - DHT11_DATA_VALID_TIME - 1;
>  	dht11->num_edges = -1;
>  
> 

Re: [PATCH 2/2] [RFC] iio: dht11: Add optional support for supply control via regulator framework

[Jonathan Cameron]

On 23/02/17 17:06, Harald Geyer wrote:
> If a supply is provided via DT, release the supply whenever we don't read
> the sensor. Also use notifications to track the actual status of the
> supply to ensure that we meet the timing requirements stated in the
> datasheet.
> 
> This change is motivated by the hope that these sensors will work more
> reliably if power-cycled regularly.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
I'd certainly rather this was driven by the powersaving argument but I guess
it's an added bonus if we happen to improve on the hardware hangs.

Minor comment inline. Looks good to me.

Jonathan
> ---
> This is an RFC this time, because I want to run extensive long term tests
> with DHT11 and DHT22 before officially proposing this for inclusion. I
> have learned my lessons with this quirky bits of HW... :(
> 
> However since this will take a lot of time (mostly physical, but also
> man hours), I'd like to get review comments now instead of risking a
> second iteration of long term tests.
> 
>  .../devicetree/bindings/iio/humidity/dht11.txt     | 10 ++++
>  drivers/iio/humidity/dht11.c                       | 57 ++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11.txt b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> index ecc24c19..ab9ea18 100644
> --- a/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> @@ -6,9 +6,19 @@ Required properties:
>      line, see "gpios property" in
>      Documentation/devicetree/bindings/gpio/gpio.txt.
>  
> +Optional properties:
> +  - vdd-supply: phandle to the regulator node, for details see
> +    Documentation/devicetree/bindings/regulator/regulator.txt
> +    On Linux the driver disables the regulator after 4 seconds of
> +    inactivity, to save power and to ensure that the hardware is
> +    reset regularly, because it was found to randomly stop responding
> +    otherwise. However for this to work all other consumers of the
> +    regulator must cooperate (disable the regulator when possible too).
> +
>  Example:
>  
>  humidity_sensor {
>  	compatible = "dht11";
>  	gpios = <&gpio0 6 0>;
> +	vdd-supply = <&sensor_supply>;
>  }
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> index 2a22ad9..5bce712 100644
> --- a/drivers/iio/humidity/dht11.c
> +++ b/drivers/iio/humidity/dht11.c
> @@ -34,12 +34,16 @@
>  #include <linux/gpio.h>
>  #include <linux/of_gpio.h>
>  #include <linux/timekeeping.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/notifier.h>
>  
>  #include <linux/iio/iio.h>
>  
>  #define DRIVER_NAME	"dht11"
>  
>  #define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
> +#define DHT11_POWER_ON_TIME 2000000000 /* must be less the MAX_INT */
> +#define DHT11_POWEROFF_DELAY 4000 /* ms */
>  
>  #define DHT11_EDGES_PREAMBLE 2
>  #define DHT11_BITS_PER_READ 40
> @@ -84,6 +88,10 @@ struct dht11 {
>  	int				gpio;
>  	int				irq;
>  
> +	struct regulator		*supply;
> +	struct notifier_block		nb;
> +	s64				timestamp_enabled;
> +
>  	struct completion		completion;
>  	/* The iio sysfs interface doesn't prevent concurrent reads: */
>  	struct mutex			lock;
> @@ -97,6 +105,21 @@ struct dht11 {
>  	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>  };
>  
> +static int dht11_regulator_update(struct notifier_block *nb,
> +				  unsigned long event,
> +				  void *ignored)
> +{
> +	struct dht11 *dht11 = container_of(nb, struct dht11, nb);
> +
> +	if (event & REGULATOR_EVENT_DISABLE)
> +		dht11->timestamp_enabled = -1;
> +	else if (event & REGULATOR_EVENT_ENABLE)
> +		if (dht11->timestamp_enabled == -1)
This nesting is a bit ugly, why not combine the else if and the if?
> +			dht11->timestamp_enabled = ktime_get_boot_ns();
> +
> +	return NOTIFY_OK;
> +}
> +
>  #ifdef CONFIG_DYNAMIC_DEBUG
>  /*
>   * dht11_edges_print: show the data as actually received by the
> @@ -203,9 +226,22 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  {
>  	struct dht11 *dht11 = iio_priv(iio_dev);
>  	int ret, timeres, offset;
> +	s64 time;
>  
>  	mutex_lock(&dht11->lock);
> -	if (dht11->timestamp + DHT11_DATA_VALID_TIME < ktime_get_boot_ns()) {
> +	time = ktime_get_boot_ns();
> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < time) {
> +		if (dht11->supply) {
> +			ret = regulator_enable(dht11->supply);
> +			if (ret) {
> +				dev_err(dht11->dev, "Can't enable supply\n");
> +				goto err_reg;
> +			}
> +			time -= dht11->timestamp_enabled + DHT11_POWER_ON_TIME;
> +			if (time < 0)
> +				msleep(((int)(-time)) / 1000000);
> +		}
> +
>  		timeres = ktime_get_resolution_ns();
>  		dev_dbg(dht11->dev, "current timeresolution: %dns\n", timeres);
>  		if (timeres > DHT11_MIN_TIMERES) {
> @@ -266,8 +302,13 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  				break;
>  		}
>  
> +err:
> +		if (dht11->supply)
> +			regulator_disable_deferred(dht11->supply,
> +						   DHT11_POWEROFF_DELAY);
> +
>  		if (ret)
> -			goto err;
> +			goto err_reg;
>  	}
>  
>  	ret = IIO_VAL_INT;
> @@ -277,7 +318,8 @@ static int dht11_read_raw(struct iio_dev *iio_dev,
>  		*val = dht11->humidity;
>  	else
>  		ret = -EINVAL;
> -err:
> +
> +err_reg:
>  	dht11->num_edges = -1;
>  	mutex_unlock(&dht11->lock);
>  	return ret;
> @@ -332,6 +374,15 @@ static int dht11_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	dht11->timestamp_enabled = -1;
> +	dht11->supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(dht11->supply)) {
> +		dht11->supply = NULL;
> +	} else {
> +		dht11->nb.notifier_call = &dht11_regulator_update;
> +		devm_regulator_register_notifier(dht11->supply, &dht11->nb);
> +	}
> +
>  	dht11->timestamp = ktime_get_boot_ns() - DHT11_DATA_VALID_TIME - 1;
>  	dht11->num_edges = -1;
>  
> 

Re: [PATCH 2/2] [RFC] iio: dht11: Add optional support for supply control via regulator framework

[Rob Herring]

On Thu, Feb 23, 2017 at 05:06:53PM +0000, Harald Geyer wrote:
> If a supply is provided via DT, release the supply whenever we don't read
> the sensor. Also use notifications to track the actual status of the
> supply to ensure that we meet the timing requirements stated in the
> datasheet.
> 
> This change is motivated by the hope that these sensors will work more
> reliably if power-cycled regularly.
> 
> Signed-off-by: Harald Geyer <harald-95f8Dae0BrPYtjvyW6yDsg@public.gmane.org>
> ---
> This is an RFC this time, because I want to run extensive long term tests
> with DHT11 and DHT22 before officially proposing this for inclusion. I
> have learned my lessons with this quirky bits of HW... :(
> 
> However since this will take a lot of time (mostly physical, but also
> man hours), I'd like to get review comments now instead of risking a
> second iteration of long term tests.
> 
>  .../devicetree/bindings/iio/humidity/dht11.txt     | 10 ++++

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>  drivers/iio/humidity/dht11.c                       | 57 ++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 3 deletions(-)

Re: [PATCH 2/2] [RFC] iio: dht11: Add optional support for supply control via regulator framework

[Rob Herring]

On Thu, Feb 23, 2017 at 05:06:53PM +0000, Harald Geyer wrote:
> If a supply is provided via DT, release the supply whenever we don't read
> the sensor. Also use notifications to track the actual status of the
> supply to ensure that we meet the timing requirements stated in the
> datasheet.
> 
> This change is motivated by the hope that these sensors will work more
> reliably if power-cycled regularly.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> This is an RFC this time, because I want to run extensive long term tests
> with DHT11 and DHT22 before officially proposing this for inclusion. I
> have learned my lessons with this quirky bits of HW... :(
> 
> However since this will take a lot of time (mostly physical, but also
> man hours), I'd like to get review comments now instead of risking a
> second iteration of long term tests.
> 
>  .../devicetree/bindings/iio/humidity/dht11.txt     | 10 ++++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/iio/humidity/dht11.c                       | 57 ++++++++++++++++++++--
>  2 files changed, 64 insertions(+), 3 deletions(-)