[PATCH 2/2] iio: Add new driver dht11-gpio

[PATCH 2/2] iio: Add new driver dht11-gpio

[Harald Geyer]

This driver handles DHT11 and DHT22 sensors.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Now this is tested with both DHT11 and DHT22 hardware, so I'm asking
for full review this time. As this hadn't had a thorough review yet,
I don't list changes to the last version posted.

Thanks,
Harald

 .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
 drivers/iio/Kconfig                                |    1 +
 drivers/iio/Makefile                               |    1 +
 drivers/iio/humidity/Kconfig                       |   14 +
 drivers/iio/humidity/Makefile                      |    5 +
 drivers/iio/humidity/dht11-gpio.c                  |  332 ++++++++++++++++++++
 6 files changed, 367 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
 create mode 100644 drivers/iio/humidity/Kconfig
 create mode 100644 drivers/iio/humidity/Makefile
 create mode 100644 drivers/iio/humidity/dht11-gpio.c

diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
new file mode 100644
index 0000000..c91bb27
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
@@ -0,0 +1,14 @@
+* DHT11 humidity/temperature sensor (and compatibles like DHT22)
+
+Required properties:
+  - compatible: Should be "dht11-gpio"
+  - gpios: Should specify the GPIO connected to the sensors data
+    line, see "gpios property" in
+    Documentation/devicetree/bindings/gpio/gpio.txt.
+
+Example:
+
+humidity_sensor {
+	compatible = "dht11-gpio";
+	gpios = <&gpio0 6 0>;
+}
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 90cf0cd..a5ed882 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
 source "drivers/iio/dac/Kconfig"
 source "drivers/iio/frequency/Kconfig"
 source "drivers/iio/gyro/Kconfig"
+source "drivers/iio/humidity/Kconfig"
 source "drivers/iio/imu/Kconfig"
 source "drivers/iio/light/Kconfig"
 source "drivers/iio/magnetometer/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index bcf7e9e..b3b5096 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -18,6 +18,7 @@ obj-y += common/
 obj-y += dac/
 obj-y += gyro/
 obj-y += frequency/
+obj-y += humidity/
 obj-y += imu/
 obj-y += light/
 obj-y += magnetometer/
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
new file mode 100644
index 0000000..aa7e481
--- /dev/null
+++ b/drivers/iio/humidity/Kconfig
@@ -0,0 +1,14 @@
+#
+# humidity sensor drivers
+#
+menu "Humidity sensors"
+
+config DHT11_GPIO
+	tristate "DHT11 (and compatible sensors) driver"
+	help
+	  This driver supports reading data via a single interrupts
+	  generating GPIO line. Currently tested are DHT11 and DHT22.
+	  Other sensors should work as well as long as they speak the
+	  same protocol.
+
+endmenu
diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
new file mode 100644
index 0000000..5e8226f
--- /dev/null
+++ b/drivers/iio/humidity/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for IIO humidity sensor drivers
+#
+
+obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11-gpio.c
new file mode 100644
index 0000000..fcbfa3e
--- /dev/null
+++ b/drivers/iio/humidity/dht11-gpio.c
@@ -0,0 +1,332 @@
+/*
+ * DHT11/DHT22 bit banging GPIO driver
+ *
+ * Copyright (c) Harald Geyer <harald@ccbib.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/wait.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#include <linux/iio/iio.h>
+
+#define DRIVER_NAME	"dht11-gpio"
+
+#define DATA_VALID_TIME	2000000000 /* 2s in ns */
+
+#define EDGES_PREAMBLE	4
+#define BITS_PER_READ	40
+#define EDGES_PER_READ	(2*BITS_PER_READ + EDGES_PREAMBLE + 1)
+
+/* Data transmission timing (nano seconds) */
+#define STARTUP		18    /* ms */
+#define SENSOR_RESPONSE	80000
+#define START_BIT	50000
+#define DATA_BIT_LOW	27000
+#define DATA_BIT_HIGH	70000
+
+/* TODO?: Support systems without DT? */
+
+struct dht11_gpio {
+	struct device			*dev;
+
+	int				gpio;
+	int				irq;
+
+	struct completion		completion;
+
+	s64				timestamp;
+	int				temperature;
+	int				humidity;
+
+	/* num_edges: -1 means "no transmission in progress" */
+	int				num_edges;
+	struct {s64 ts; int value; }	edges[EDGES_PER_READ];
+};
+
+/*
+ * dht11_edges_print: show the data as actually received by the
+ *                    driver.
+ * This is dead code, keeping it for now just in case somebody needs
+ * it for porting the driver to new sensor HW, etc.
+ *
+static void dht11_edges_print(struct dht11_gpio *dht11)
+{
+	int i;
+
+	for (i = 1; i < dht11->num_edges; ++i) {
+		pr_err("dht11: %d: %lld ns %s\n", i,
+			dht11->edges[i].ts - dht11->edges[i-1].ts,
+			dht11->edges[i-1].value ? "high" : "low");
+	}
+}
+*/
+
+static unsigned char dht11_gpio_decode_byte(int *timing, int threshold)
+{
+	unsigned char ret = 0;
+	int i;
+
+	for (i = 0; i < 8; ++i) {
+		ret <<= 1;
+		if (timing[i] >= threshold)
+			++ret;
+	}
+
+	return ret;
+}
+
+static int dht11_gpio_decode(struct dht11_gpio *dht11, int offset)
+{
+	int i, t, timing[BITS_PER_READ], threshold, timeres = SENSOR_RESPONSE;
+	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
+
+	/* Calculate timestamp resolution */
+	for (i = 0; i < dht11->num_edges; ++i) {
+		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
+		if (t > 0 && t < timeres)
+			timeres = t;
+	}
+	if (2*timeres > DATA_BIT_HIGH) {
+		pr_err("dht11-gpio: timeresolution %d too bad for decoding\n",
+			timeres);
+		return -EIO;
+	}
+	threshold = DATA_BIT_HIGH/timeres;
+	if (DATA_BIT_LOW/timeres + 1 >= threshold)
+		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
+
+	/* scale down with timeres and check validity */
+	for (i = 0; i < BITS_PER_READ; ++i) {
+		t = dht11->edges[offset + 2*i + 2].ts -
+			dht11->edges[offset + 2*i + 1].ts;
+		if (!dht11->edges[offset + 2*i + 1].value)
+			return -EIO; /* lost synchronisation */
+		timing[i] = t / timeres;
+	}
+
+	hum_int = dht11_gpio_decode_byte(timing, threshold);
+	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
+	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
+	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
+	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
+
+	if (((hum_int + hum_dec + temp_int + temp_dec) & 0x00ff) != checksum)
+		return -EIO;
+
+	dht11->timestamp = iio_get_time_ns();
+	if (hum_int < 20) {  /* DHT22 */
+		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
+					((temp_int & 0x80) ? -100 : 100);
+		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
+	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
+		dht11->temperature = temp_int * 1000;
+		dht11->humidity = hum_int * 1000;
+	} else {
+		dev_err(dht11->dev,
+			"Don't know how to decode data: %d %d %d %d\n",
+			hum_int, hum_dec, temp_int, temp_dec);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int *val, int *val2, long m)
+{
+	struct dht11_gpio *dht11 = iio_priv(iio_dev);
+	int ret = 0;
+
+	if (dht11->timestamp + DATA_VALID_TIME < iio_get_time_ns()) {
+		INIT_COMPLETION(dht11->completion);
+
+		dht11->num_edges = 0;
+		ret = gpio_direction_output(dht11->gpio, 0);
+		if (ret)
+			goto err;
+		msleep(STARTUP);
+		ret = gpio_direction_input(dht11->gpio);
+		if (ret)
+			goto err;
+
+		ret = wait_for_completion_killable_timeout(&dht11->completion,
+								 HZ);
+		if (ret == 0 && dht11->num_edges < EDGES_PER_READ - 1) {
+			dev_err(&iio_dev->dev,
+					"Only %d signal edges detected\n",
+					dht11->num_edges);
+			ret = -ETIMEDOUT;
+		}
+		if (ret < 0)
+			goto err;
+
+		ret = dht11_gpio_decode(dht11,
+				dht11->num_edges == EDGES_PER_READ ?
+					EDGES_PREAMBLE : EDGES_PREAMBLE - 2);
+		if (ret)
+			goto err;
+	}
+
+	ret = IIO_VAL_INT;
+	if (chan->channel == 0)
+		*val = dht11->temperature;
+	else if (chan->channel == 1)
+		*val = dht11->humidity;
+	else
+		ret = -EINVAL;
+err:
+	dht11->num_edges = -1;
+	return ret;
+}
+
+static const struct iio_info dht11_gpio_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= dht11_gpio_read_raw,
+};
+
+/*
+ * IRQ handler called on GPIO edges
+*/
+static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
+{
+	struct iio_dev *iio = data;
+	struct dht11_gpio *dht11 = iio_priv(iio);
+
+	/* TODO: Consider making the handler safe for IRQ sharing */
+	if (dht11->num_edges < EDGES_PER_READ && dht11->num_edges >= 0) {
+		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
+		dht11->edges[dht11->num_edges++].value =
+						gpio_get_value(dht11->gpio);
+
+		if (dht11->num_edges >= EDGES_PER_READ)
+			complete(&dht11->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
+	{ .type = IIO_TEMP, .channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
+	{ .type = IIO_HUMIDITYRELATIVE, .channel = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+}
+};
+
+static const struct of_device_id dht11_gpio_dt_ids[] = {
+	{ .compatible = "dht11-gpio", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
+
+static int dht11_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct dht11_gpio *dht11;
+	struct iio_dev *iio;
+	int ret = 0;
+
+	/* Allocate the IIO device. */
+	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
+	if (!iio) {
+		dev_err(dev, "Failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	dht11 = iio_priv(iio);
+	dht11->dev = dev;
+
+	dht11->gpio = ret = of_get_gpio(node, 0);
+	if (ret < 0)
+		return ret;
+	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
+	if (ret)
+		return ret;
+
+	dht11->irq = gpio_to_irq(dht11->gpio);
+	if (dht11->irq < 0) {
+		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
+		return -EINVAL;
+	}
+	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				pdev->name, iio);
+	if (ret)
+		return ret;
+
+	dht11->timestamp = iio_get_time_ns() - DATA_VALID_TIME - 1;
+	dht11->num_edges = -1;
+
+	platform_set_drvdata(pdev, iio);
+
+	init_completion(&dht11->completion);
+	iio->name = pdev->name;
+	iio->dev.parent = &pdev->dev;
+	iio->info = &dht11_gpio_iio_info;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->channels = dht11_gpio_chan_spec;
+	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
+
+	/* Register IIO device. */
+	ret = iio_device_register(iio);
+	if (ret) {
+		dev_err(dev, "Failed to register IIO device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dht11_gpio_remove(struct platform_device *pdev)
+{
+	struct iio_dev *iio = platform_get_drvdata(pdev);
+
+	iio_device_unregister(iio);
+
+	return 0;
+}
+
+static struct platform_driver dht11_gpio_driver = {
+	.driver = {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = dht11_gpio_dt_ids,
+	},
+	.probe  = dht11_gpio_probe,
+	.remove = dht11_gpio_remove,
+};
+
+module_platform_driver(dht11_gpio_driver);
+
+MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
+MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
+MODULE_LICENSE("GPL v2");
+
-- 
1.7.2.5

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Peter Meerwald]

> This driver handles DHT11 and DHT22 sensors.

comments inline, mostly nitpicking
 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> Now this is tested with both DHT11 and DHT22 hardware, so I'm asking
> for full review this time. As this hadn't had a thorough review yet,
> I don't list changes to the last version posted.
> 
> Thanks,
> Harald
> 
>  .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
>  drivers/iio/Kconfig                                |    1 +
>  drivers/iio/Makefile                               |    1 +
>  drivers/iio/humidity/Kconfig                       |   14 +
>  drivers/iio/humidity/Makefile                      |    5 +
>  drivers/iio/humidity/dht11-gpio.c                  |  332 ++++++++++++++++++++
>  6 files changed, 367 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>  create mode 100644 drivers/iio/humidity/Kconfig
>  create mode 100644 drivers/iio/humidity/Makefile
>  create mode 100644 drivers/iio/humidity/dht11-gpio.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
> new file mode 100644
> index 0000000..c91bb27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
> @@ -0,0 +1,14 @@
> +* DHT11 humidity/temperature sensor (and compatibles like DHT22)
> +
> +Required properties:
> +  - compatible: Should be "dht11-gpio"
> +  - gpios: Should specify the GPIO connected to the sensors data

sensor's

> +    line, see "gpios property" in
> +    Documentation/devicetree/bindings/gpio/gpio.txt.
> +
> +Example:
> +
> +humidity_sensor {
> +	compatible = "dht11-gpio";
> +	gpios = <&gpio0 6 0>;
> +}
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 90cf0cd..a5ed882 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
>  source "drivers/iio/dac/Kconfig"
>  source "drivers/iio/frequency/Kconfig"
>  source "drivers/iio/gyro/Kconfig"
> +source "drivers/iio/humidity/Kconfig"
>  source "drivers/iio/imu/Kconfig"
>  source "drivers/iio/light/Kconfig"
>  source "drivers/iio/magnetometer/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index bcf7e9e..b3b5096 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -18,6 +18,7 @@ obj-y += common/
>  obj-y += dac/
>  obj-y += gyro/
>  obj-y += frequency/
> +obj-y += humidity/
>  obj-y += imu/
>  obj-y += light/
>  obj-y += magnetometer/
> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> new file mode 100644
> index 0000000..aa7e481
> --- /dev/null
> +++ b/drivers/iio/humidity/Kconfig
> @@ -0,0 +1,14 @@
> +#
> +# humidity sensor drivers
> +#
> +menu "Humidity sensors"
> +
> +config DHT11_GPIO
> +	tristate "DHT11 (and compatible sensors) driver"
> +	help
> +	  This driver supports reading data via a single interrupts

interrupt

> +	  generating GPIO line. Currently tested are DHT11 and DHT22.
> +	  Other sensors should work as well as long as they speak the
> +	  same protocol.
> +
> +endmenu
> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
> new file mode 100644
> index 0000000..5e8226f
> --- /dev/null
> +++ b/drivers/iio/humidity/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for IIO humidity sensor drivers
> +#
> +
> +obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
> diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11-gpio.c
> new file mode 100644
> index 0000000..fcbfa3e
> --- /dev/null
> +++ b/drivers/iio/humidity/dht11-gpio.c
> @@ -0,0 +1,332 @@
> +/*
> + * DHT11/DHT22 bit banging GPIO driver
> + *
> + * Copyright (c) Harald Geyer <harald@ccbib.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/wait.h>
> +#include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define DRIVER_NAME	"dht11-gpio"
> +
> +#define DATA_VALID_TIME	2000000000 /* 2s in ns */

prefix with DHT11_

> +
> +#define EDGES_PREAMBLE	4
> +#define BITS_PER_READ	40
> +#define EDGES_PER_READ	(2*BITS_PER_READ + EDGES_PREAMBLE + 1)
> +
> +/* Data transmission timing (nano seconds) */
> +#define STARTUP		18    /* ms */

just a line above nano seconds were promised, and now this: ms?! :)

> +#define SENSOR_RESPONSE	80000
> +#define START_BIT	50000
> +#define DATA_BIT_LOW	27000
> +#define DATA_BIT_HIGH	70000
> +
> +/* TODO?: Support systems without DT? */
> +
> +struct dht11_gpio {
> +	struct device			*dev;
> +
> +	int				gpio;
> +	int				irq;
> +
> +	struct completion		completion;
> +
> +	s64				timestamp;
> +	int				temperature;
> +	int				humidity;
> +
> +	/* num_edges: -1 means "no transmission in progress" */
> +	int				num_edges;
> +	struct {s64 ts; int value; }	edges[EDGES_PER_READ];
> +};
> +
> +/*
> + * dht11_edges_print: show the data as actually received by the
> + *                    driver.
> + * This is dead code, keeping it for now just in case somebody needs
> + * it for porting the driver to new sensor HW, etc.
> + *
> +static void dht11_edges_print(struct dht11_gpio *dht11)
> +{
> +	int i;
> +
> +	for (i = 1; i < dht11->num_edges; ++i) {
> +		pr_err("dht11: %d: %lld ns %s\n", i,

inconsistent driver name; dht11-gpio was used before

> +			dht11->edges[i].ts - dht11->edges[i-1].ts,
> +			dht11->edges[i-1].value ? "high" : "low");
> +	}
> +}
> +*/
> +
> +static unsigned char dht11_gpio_decode_byte(int *timing, int threshold)
> +{
> +	unsigned char ret = 0;
> +	int i;
> +
> +	for (i = 0; i < 8; ++i) {
> +		ret <<= 1;
> +		if (timing[i] >= threshold)
> +			++ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int dht11_gpio_decode(struct dht11_gpio *dht11, int offset)
> +{
> +	int i, t, timing[BITS_PER_READ], threshold, timeres = SENSOR_RESPONSE;
> +	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
> +
> +	/* Calculate timestamp resolution */
> +	for (i = 0; i < dht11->num_edges; ++i) {
> +		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> +		if (t > 0 && t < timeres)
> +			timeres = t;
> +	}
> +	if (2*timeres > DATA_BIT_HIGH) {
> +		pr_err("dht11-gpio: timeresolution %d too bad for decoding\n",
> +			timeres);
> +		return -EIO;
> +	}
> +	threshold = DATA_BIT_HIGH/timeres;
> +	if (DATA_BIT_LOW/timeres + 1 >= threshold)
> +		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
> +
> +	/* scale down with timeres and check validity */
> +	for (i = 0; i < BITS_PER_READ; ++i) {
> +		t = dht11->edges[offset + 2*i + 2].ts -
> +			dht11->edges[offset + 2*i + 1].ts;
> +		if (!dht11->edges[offset + 2*i + 1].value)
> +			return -EIO; /* lost synchronisation */
> +		timing[i] = t / timeres;

inconsistent whitespace around / operator

> +	}
> +
> +	hum_int = dht11_gpio_decode_byte(timing, threshold);
> +	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
> +	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
> +	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
> +	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
> +
> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0x00ff) != checksum)

maybe 0xff instead of 0x00ff is clearer

> +		return -EIO;
> +
> +	dht11->timestamp = iio_get_time_ns();
> +	if (hum_int < 20) {  /* DHT22 */
> +		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
> +					((temp_int & 0x80) ? -100 : 100);
> +		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
> +	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */

extra space before comment

> +		dht11->temperature = temp_int * 1000;
> +		dht11->humidity = hum_int * 1000;
> +	} else {
> +		dev_err(dht11->dev,
> +			"Don't know how to decode data: %d %d %d %d\n",
> +			hum_int, hum_dec, temp_int, temp_dec);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
> +			const struct iio_chan_spec *chan,
> +			int *val, int *val2, long m)
> +{
> +	struct dht11_gpio *dht11 = iio_priv(iio_dev);
> +	int ret = 0;

the initialization of ret is not used; don't or init with -EINVAL to save 
an assignment

> +
> +	if (dht11->timestamp + DATA_VALID_TIME < iio_get_time_ns()) {
> +		INIT_COMPLETION(dht11->completion);
> +
> +		dht11->num_edges = 0;
> +		ret = gpio_direction_output(dht11->gpio, 0);
> +		if (ret)
> +			goto err;
> +		msleep(STARTUP);
> +		ret = gpio_direction_input(dht11->gpio);
> +		if (ret)
> +			goto err;
> +
> +		ret = wait_for_completion_killable_timeout(&dht11->completion,
> +								 HZ);
> +		if (ret == 0 && dht11->num_edges < EDGES_PER_READ - 1) {
> +			dev_err(&iio_dev->dev,
> +					"Only %d signal edges detected\n",
> +					dht11->num_edges);
> +			ret = -ETIMEDOUT;
> +		}
> +		if (ret < 0)
> +			goto err;
> +
> +		ret = dht11_gpio_decode(dht11,
> +				dht11->num_edges == EDGES_PER_READ ?
> +					EDGES_PREAMBLE : EDGES_PREAMBLE - 2);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = IIO_VAL_INT;
> +	if (chan->channel == 0)
> +		*val = dht11->temperature;
> +	else if (chan->channel == 1)
> +		*val = dht11->humidity;

use channel type to discriminate channels

> +	else
> +		ret = -EINVAL;
> +err:
> +	dht11->num_edges = -1;
> +	return ret;
> +}
> +
> +static const struct iio_info dht11_gpio_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= dht11_gpio_read_raw,
> +};
> +
> +/*
> + * IRQ handler called on GPIO edges
> +*/
> +static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
> +{
> +	struct iio_dev *iio = data;
> +	struct dht11_gpio *dht11 = iio_priv(iio);
> +
> +	/* TODO: Consider making the handler safe for IRQ sharing */
> +	if (dht11->num_edges < EDGES_PER_READ && dht11->num_edges >= 0) {
> +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> +		dht11->edges[dht11->num_edges++].value =
> +						gpio_get_value(dht11->gpio);
> +
> +		if (dht11->num_edges >= EDGES_PER_READ)
> +			complete(&dht11->completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
> +	{ .type = IIO_TEMP, .channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> +	{ .type = IIO_HUMIDITYRELATIVE, .channel = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),

.channel is not needed; there aren't multiple channels of the same type

> +}

weird indentation level

> +};
> +
> +static const struct of_device_id dht11_gpio_dt_ids[] = {
> +	{ .compatible = "dht11-gpio", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
> +
> +static int dht11_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct dht11_gpio *dht11;
> +	struct iio_dev *iio;
> +	int ret = 0;

don't initialize ret

> +
> +	/* Allocate the IIO device. */

obvious comment

> +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> +	if (!iio) {
> +		dev_err(dev, "Failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	dht11 = iio_priv(iio);
> +	dht11->dev = dev;
> +
> +	dht11->gpio = ret = of_get_gpio(node, 0);
> +	if (ret < 0)
> +		return ret;
> +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
> +	if (ret)
> +		return ret;
> +
> +	dht11->irq = gpio_to_irq(dht11->gpio);
> +	if (dht11->irq < 0) {
> +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
> +		return -EINVAL;
> +	}
> +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				pdev->name, iio);
> +	if (ret)
> +		return ret;
> +
> +	dht11->timestamp = iio_get_time_ns() - DATA_VALID_TIME - 1;
> +	dht11->num_edges = -1;
> +
> +	platform_set_drvdata(pdev, iio);
> +
> +	init_completion(&dht11->completion);
> +	iio->name = pdev->name;
> +	iio->dev.parent = &pdev->dev;
> +	iio->info = &dht11_gpio_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = dht11_gpio_chan_spec;
> +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
> +
> +	/* Register IIO device. */

the function name is pretty obvious, no comment needed

> +	ret = iio_device_register(iio);

maybe just return here, the extra output is not needed

> +	if (ret) {
> +		dev_err(dev, "Failed to register IIO device\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dht11_gpio_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *iio = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(iio);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver dht11_gpio_driver = {
> +	.driver = {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +		.of_match_table = dht11_gpio_dt_ids,
> +	},
> +	.probe  = dht11_gpio_probe,
> +	.remove = dht11_gpio_remove,
> +};
> +
> +module_platform_driver(dht11_gpio_driver);
> +
> +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
> +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
> +MODULE_LICENSE("GPL v2");
> +
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Peter Meerwald]

> > +config DHT11_GPIO
> > +	tristate "DHT11 (and compatible sensors) driver"
> > +	help
> > +	  This driver supports reading data via a single interrupts

should probably depend on GENERIC_GPIO?

p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Harald Geyer]

> > This driver handles DHT11 and DHT22 sensors.
> 
> comments inline, mostly nitpicking

Thanks for the review.

> > Signed-off-by: Harald Geyer <harald@ccbib.org>
> > ---
> > Now this is tested with both DHT11 and DHT22 hardware, so I'm asking
> > for full review this time. As this hadn't had a thorough review yet,
> > I don't list changes to the last version posted.
> > 
> > Thanks,
> > Harald
> > 
> >  .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
> >  drivers/iio/Kconfig                                |    1 +
> >  drivers/iio/Makefile                               |    1 +
> >  drivers/iio/humidity/Kconfig                       |   14 +
> >  drivers/iio/humidity/Makefile                      |    5 +
> >  drivers/iio/humidity/dht11-gpio.c                  |  332 ++++++++++++++++
> ++++
> >  6 files changed, 367 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gp
> io.txt
> >  create mode 100644 drivers/iio/humidity/Kconfig
> >  create mode 100644 drivers/iio/humidity/Makefile
> >  create mode 100644 drivers/iio/humidity/dht11-gpio.c
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt 
> b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
> > new file mode 100644
> > index 0000000..c91bb27
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
> > @@ -0,0 +1,14 @@
> > +* DHT11 humidity/temperature sensor (and compatibles like DHT22)
> > +
> > +Required properties:
> > +  - compatible: Should be "dht11-gpio"
> > +  - gpios: Should specify the GPIO connected to the sensors data
> 
> sensor's
> 
> > +    line, see "gpios property" in
> > +    Documentation/devicetree/bindings/gpio/gpio.txt.
> > +
> > +Example:
> > +
> > +humidity_sensor {
> > +	compatible = "dht11-gpio";
> > +	gpios = <&gpio0 6 0>;
> > +}
> > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> > index 90cf0cd..a5ed882 100644
> > --- a/drivers/iio/Kconfig
> > +++ b/drivers/iio/Kconfig
> > @@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
> >  source "drivers/iio/dac/Kconfig"
> >  source "drivers/iio/frequency/Kconfig"
> >  source "drivers/iio/gyro/Kconfig"
> > +source "drivers/iio/humidity/Kconfig"
> >  source "drivers/iio/imu/Kconfig"
> >  source "drivers/iio/light/Kconfig"
> >  source "drivers/iio/magnetometer/Kconfig"
> > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> > index bcf7e9e..b3b5096 100644
> > --- a/drivers/iio/Makefile
> > +++ b/drivers/iio/Makefile
> > @@ -18,6 +18,7 @@ obj-y += common/
> >  obj-y += dac/
> >  obj-y += gyro/
> >  obj-y += frequency/
> > +obj-y += humidity/
> >  obj-y += imu/
> >  obj-y += light/
> >  obj-y += magnetometer/
> > diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> > new file mode 100644
> > index 0000000..aa7e481
> > --- /dev/null
> > +++ b/drivers/iio/humidity/Kconfig
> > @@ -0,0 +1,14 @@
> > +#
> > +# humidity sensor drivers
> > +#
> > +menu "Humidity sensors"
> > +
> > +config DHT11_GPIO
> > +	tristate "DHT11 (and compatible sensors) driver"
> > +	help
> > +	  This driver supports reading data via a single interrupts
> 
> interrupt
> 
> > +	  generating GPIO line. Currently tested are DHT11 and DHT22.
> > +	  Other sensors should work as well as long as they speak the
> > +	  same protocol.
> > +
> > +endmenu
> > diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
> > new file mode 100644
> > index 0000000..5e8226f
> > --- /dev/null
> > +++ b/drivers/iio/humidity/Makefile
> > @@ -0,0 +1,5 @@
> > +#
> > +# Makefile for IIO humidity sensor drivers
> > +#
> > +
> > +obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
> > diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11
> -gpio.c
> > new file mode 100644
> > index 0000000..fcbfa3e
> > --- /dev/null
> > +++ b/drivers/iio/humidity/dht11-gpio.c
> > @@ -0,0 +1,332 @@
> > +/*
> > + * DHT11/DHT22 bit banging GPIO driver
> > + *
> > + * Copyright (c) Harald Geyer <harald@ccbib.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/printk.h>
> > +#include <linux/slab.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/wait.h>
> > +#include <linux/bitops.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/of_gpio.h>
> > +
> > +#include <linux/iio/iio.h>
> > +
> > +#define DRIVER_NAME	"dht11-gpio"
> > +
> > +#define DATA_VALID_TIME	2000000000 /* 2s in ns */
> 
> prefix with DHT11_

Do you think this is needed, even as all these defines are
local to this file? (It would force rather unpleasant line
breaks...) Is there some general rule when to prefix and
when not to?
 
> > +
> > +#define EDGES_PREAMBLE	4
> > +#define BITS_PER_READ	40
> > +#define EDGES_PER_READ	(2*BITS_PER_READ + EDGES_PREAMBLE + 1)
> > +
> > +/* Data transmission timing (nano seconds) */
> > +#define STARTUP		18    /* ms */
> 
> just a line above nano seconds were promised, and now this: ms?! :)

Sure, the exception from the rule is noted as end of line comment.
Is there some better way to do this?

> > +#define SENSOR_RESPONSE	80000
> > +#define START_BIT	50000
> > +#define DATA_BIT_LOW	27000
> > +#define DATA_BIT_HIGH	70000
> > +
> > +/* TODO?: Support systems without DT? */
> > +
> > +struct dht11_gpio {
> > +	struct device			*dev;
> > +
> > +	int				gpio;
> > +	int				irq;
> > +
> > +	struct completion		completion;
> > +
> > +	s64				timestamp;
> > +	int				temperature;
> > +	int				humidity;
> > +
> > +	/* num_edges: -1 means "no transmission in progress" */
> > +	int				num_edges;
> > +	struct {s64 ts; int value; }	edges[EDGES_PER_READ];
> > +};
> > +
> > +/*
> > + * dht11_edges_print: show the data as actually received by the
> > + *                    driver.
> > + * This is dead code, keeping it for now just in case somebody needs
> > + * it for porting the driver to new sensor HW, etc.
> > + *
> > +static void dht11_edges_print(struct dht11_gpio *dht11)
> > +{
> > +	int i;
> > +
> > +	for (i = 1; i < dht11->num_edges; ++i) {
> > +		pr_err("dht11: %d: %lld ns %s\n", i,
> 
> inconsistent driver name; dht11-gpio was used before
> 
> > +			dht11->edges[i].ts - dht11->edges[i-1].ts,
> > +			dht11->edges[i-1].value ? "high" : "low");
> > +	}
> > +}
> > +*/
> > +
> > +static unsigned char dht11_gpio_decode_byte(int *timing, int threshold)
> > +{
> > +	unsigned char ret = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < 8; ++i) {
> > +		ret <<= 1;
> > +		if (timing[i] >= threshold)
> > +			++ret;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int dht11_gpio_decode(struct dht11_gpio *dht11, int offset)
> > +{
> > +	int i, t, timing[BITS_PER_READ], threshold, timeres = SENSOR_RESPONSE;
> > +	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
> > +
> > +	/* Calculate timestamp resolution */
> > +	for (i = 0; i < dht11->num_edges; ++i) {
> > +		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> > +		if (t > 0 && t < timeres)
> > +			timeres = t;
> > +	}
> > +	if (2*timeres > DATA_BIT_HIGH) {
> > +		pr_err("dht11-gpio: timeresolution %d too bad for decoding\n",
> > +			timeres);
> > +		return -EIO;
> > +	}
> > +	threshold = DATA_BIT_HIGH/timeres;

I'm inclined to change this to:
	threshold = DATA_BIT_HIGH / timeres;

> > +	if (DATA_BIT_LOW/timeres + 1 >= threshold)
> > +		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
> > +
> > +	/* scale down with timeres and check validity */
> > +	for (i = 0; i < BITS_PER_READ; ++i) {
> > +		t = dht11->edges[offset + 2*i + 2].ts -
> > +			dht11->edges[offset + 2*i + 1].ts;
> > +		if (!dht11->edges[offset + 2*i + 1].value)
> > +			return -EIO; /* lost synchronisation */
> > +		timing[i] = t / timeres;

and leave this as is.

> inconsistent whitespace around / operator

Would that be consistent enough? (The rule being, that a / operator
on it's own gets spaces, but the same operator in a statement
together with operators with lower binding power gets none.)

> > +	}
> > +
> > +	hum_int = dht11_gpio_decode_byte(timing, threshold);
> > +	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
> > +	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
> > +	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
> > +	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
> > +
> > +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0x00ff) != checksum)
> 
> maybe 0xff instead of 0x00ff is clearer

Ok, I'm happy either way.

> > +		return -EIO;
> > +
> > +	dht11->timestamp = iio_get_time_ns();
> > +	if (hum_int < 20) {  /* DHT22 */
> > +		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
> > +					((temp_int & 0x80) ? -100 : 100);
> > +		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
> > +	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
> 
> extra space before comment

Actually I think that code is much more readable if end of line
comments are visually separated by two or more spaces (and
check_patch.pl is happy with that). I see now, that I have not
been entirely consistent and would rather add extra spaces rather
then remove them. What do you think?
 
> > +		dht11->temperature = temp_int * 1000;
> > +		dht11->humidity = hum_int * 1000;
> > +	} else {
> > +		dev_err(dht11->dev,
> > +			"Don't know how to decode data: %d %d %d %d\n",
> > +			hum_int, hum_dec, temp_int, temp_dec);
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
> > +			const struct iio_chan_spec *chan,
> > +			int *val, int *val2, long m)
> > +{
> > +	struct dht11_gpio *dht11 = iio_priv(iio_dev);
> > +	int ret = 0;
> 
> the initialization of ret is not used; don't or init with -EINVAL to save 
> an assignment
> 
> > +
> > +	if (dht11->timestamp + DATA_VALID_TIME < iio_get_time_ns()) {
> > +		INIT_COMPLETION(dht11->completion);
> > +
> > +		dht11->num_edges = 0;
> > +		ret = gpio_direction_output(dht11->gpio, 0);
> > +		if (ret)
> > +			goto err;
> > +		msleep(STARTUP);
> > +		ret = gpio_direction_input(dht11->gpio);
> > +		if (ret)
> > +			goto err;
> > +
> > +		ret = wait_for_completion_killable_timeout(&dht11->completion,
> > +								 HZ);
> > +		if (ret == 0 && dht11->num_edges < EDGES_PER_READ - 1) {
> > +			dev_err(&iio_dev->dev,
> > +					"Only %d signal edges detected\n",
> > +					dht11->num_edges);
> > +			ret = -ETIMEDOUT;
> > +		}
> > +		if (ret < 0)
> > +			goto err;
> > +
> > +		ret = dht11_gpio_decode(dht11,
> > +				dht11->num_edges == EDGES_PER_READ ?
> > +					EDGES_PREAMBLE : EDGES_PREAMBLE - 2);
> > +		if (ret)
> > +			goto err;
> > +	}
> > +
> > +	ret = IIO_VAL_INT;
> > +	if (chan->channel == 0)
> > +		*val = dht11->temperature;
> > +	else if (chan->channel == 1)
> > +		*val = dht11->humidity;
> 
> use channel type to discriminate channels

Yes, very good point.

I agree with the rest of the comments. Thanks for the review.
Harald
 
> > +	else
> > +		ret = -EINVAL;
> > +err:
> > +	dht11->num_edges = -1;
> > +	return ret;
> > +}
> > +
> > +static const struct iio_info dht11_gpio_iio_info = {
> > +	.driver_module		= THIS_MODULE,
> > +	.read_raw		= dht11_gpio_read_raw,
> > +};
> > +
> > +/*
> > + * IRQ handler called on GPIO edges
> > +*/
> > +static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
> > +{
> > +	struct iio_dev *iio = data;
> > +	struct dht11_gpio *dht11 = iio_priv(iio);
> > +
> > +	/* TODO: Consider making the handler safe for IRQ sharing */
> > +	if (dht11->num_edges < EDGES_PER_READ && dht11->num_edges >= 0) {
> > +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> > +		dht11->edges[dht11->num_edges++].value =
> > +						gpio_get_value(dht11->gpio);
> > +
> > +		if (dht11->num_edges >= EDGES_PER_READ)
> > +			complete(&dht11->completion);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
> > +	{ .type = IIO_TEMP, .channel = 0,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> > +	{ .type = IIO_HUMIDITYRELATIVE, .channel = 1,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> 
> .channel is not needed; there aren't multiple channels of the same type
> 
> > +}
> 
> weird indentation level
> 
> > +};
> > +
> > +static const struct of_device_id dht11_gpio_dt_ids[] = {
> > +	{ .compatible = "dht11-gpio", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
> > +
> > +static int dht11_gpio_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node = dev->of_node;
> > +	struct dht11_gpio *dht11;
> > +	struct iio_dev *iio;
> > +	int ret = 0;
> 
> don't initialize ret
> 
> > +
> > +	/* Allocate the IIO device. */
> 
> obvious comment
> 
> > +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> > +	if (!iio) {
> > +		dev_err(dev, "Failed to allocate IIO device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	dht11 = iio_priv(iio);
> > +	dht11->dev = dev;
> > +
> > +	dht11->gpio = ret = of_get_gpio(node, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dht11->irq = gpio_to_irq(dht11->gpio);
> > +	if (dht11->irq < 0) {
> > +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
> > +		return -EINVAL;
> > +	}
> > +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
> > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > +				pdev->name, iio);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dht11->timestamp = iio_get_time_ns() - DATA_VALID_TIME - 1;
> > +	dht11->num_edges = -1;
> > +
> > +	platform_set_drvdata(pdev, iio);
> > +
> > +	init_completion(&dht11->completion);
> > +	iio->name = pdev->name;
> > +	iio->dev.parent = &pdev->dev;
> > +	iio->info = &dht11_gpio_iio_info;
> > +	iio->modes = INDIO_DIRECT_MODE;
> > +	iio->channels = dht11_gpio_chan_spec;
> > +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
> > +
> > +	/* Register IIO device. */
> 
> the function name is pretty obvious, no comment needed
> 
> > +	ret = iio_device_register(iio);
> 
> maybe just return here, the extra output is not needed
> 
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register IIO device\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dht11_gpio_remove(struct platform_device *pdev)
> > +{
> > +	struct iio_dev *iio = platform_get_drvdata(pdev);
> > +
> > +	iio_device_unregister(iio);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver dht11_gpio_driver = {
> > +	.driver = {
> > +		.name	= DRIVER_NAME,
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = dht11_gpio_dt_ids,
> > +	},
> > +	.probe  = dht11_gpio_probe,
> > +	.remove = dht11_gpio_remove,
> > +};
> > +
> > +module_platform_driver(dht11_gpio_driver);
> > +
> > +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
> > +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > +
> > 
> 
> -- 
> 
> Peter Meerwald
> +43-664-2444418 (mobile)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Peter Meerwald]

> > prefix with DHT11_
 
> Do you think this is needed, even as all these defines are
> local to this file? (It would force rather unpleasant line
> breaks...) Is there some general rule when to prefix and
> when not to?

breakage occurs when some of your include file #define e.g. START_BIT or 
STARTUP in the future; I think the rule for iio is to prefix everything
  
> > > +#define EDGES_PREAMBLE	4
> > > +#define BITS_PER_READ	40
> > > +#define EDGES_PER_READ	(2*BITS_PER_READ + EDGES_PREAMBLE + 1)
> > > +
> > > +/* Data transmission timing (nano seconds) */
> > > +#define STARTUP		18    /* ms */
> > 
> > just a line above nano seconds were promised, and now this: ms?! :)
> 
> Sure, the exception from the rule is noted as end of line comment.
> Is there some better way to do this?

STARTUP is used exactly once; I'd just drop the #define and use the 
constant directly -- you'll get a checkpatch warning for msleep(18) though

and startup is not transmission timing, right?

> > > +#define SENSOR_RESPONSE	80000
> > > +#define START_BIT	50000
> > > +#define DATA_BIT_LOW	27000
> > > +#define DATA_BIT_HIGH	70000
> > > +
> > > +/* TODO?: Support systems without DT? */
> > > +
> > > +struct dht11_gpio {
> > > +	struct device			*dev;
> > > +
> > > +	int				gpio;
> > > +	int				irq;
> > > +
> > > +	struct completion		completion;
> > > +
> > > +	s64				timestamp;
> > > +	int				temperature;
> > > +	int				humidity;
> > > +
> > > +	/* num_edges: -1 means "no transmission in progress" */
> > > +	int				num_edges;
> > > +	struct {s64 ts; int value; }	edges[EDGES_PER_READ];
> > > +};
> > > +
> > > +/*
> > > + * dht11_edges_print: show the data as actually received by the
> > > + *                    driver.
> > > + * This is dead code, keeping it for now just in case somebody needs
> > > + * it for porting the driver to new sensor HW, etc.
> > > + *
> > > +static void dht11_edges_print(struct dht11_gpio *dht11)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 1; i < dht11->num_edges; ++i) {
> > > +		pr_err("dht11: %d: %lld ns %s\n", i,
> > 
> > inconsistent driver name; dht11-gpio was used before
> > 
> > > +			dht11->edges[i].ts - dht11->edges[i-1].ts,
> > > +			dht11->edges[i-1].value ? "high" : "low");
> > > +	}
> > > +}
> > > +*/
> > > +
> > > +static unsigned char dht11_gpio_decode_byte(int *timing, int threshold)
> > > +{
> > > +	unsigned char ret = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < 8; ++i) {
> > > +		ret <<= 1;
> > > +		if (timing[i] >= threshold)
> > > +			++ret;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int dht11_gpio_decode(struct dht11_gpio *dht11, int offset)
> > > +{
> > > +	int i, t, timing[BITS_PER_READ], threshold, timeres = SENSOR_RESPONSE;
> > > +	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
> > > +
> > > +	/* Calculate timestamp resolution */
> > > +	for (i = 0; i < dht11->num_edges; ++i) {
> > > +		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> > > +		if (t > 0 && t < timeres)
> > > +			timeres = t;
> > > +	}
> > > +	if (2*timeres > DATA_BIT_HIGH) {
> > > +		pr_err("dht11-gpio: timeresolution %d too bad for decoding\n",
> > > +			timeres);
> > > +		return -EIO;
> > > +	}
> > > +	threshold = DATA_BIT_HIGH/timeres;
> 
> I'm inclined to change this to:
> 	threshold = DATA_BIT_HIGH / timeres;

this is what I had in mind
 
> > > +	if (DATA_BIT_LOW/timeres + 1 >= threshold)
> > > +		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
> > > +
> > > +	/* scale down with timeres and check validity */
> > > +	for (i = 0; i < BITS_PER_READ; ++i) {
> > > +		t = dht11->edges[offset + 2*i + 2].ts -
> > > +			dht11->edges[offset + 2*i + 1].ts;
> > > +		if (!dht11->edges[offset + 2*i + 1].value)
> > > +			return -EIO; /* lost synchronisation */
> > > +		timing[i] = t / timeres;
> 
> and leave this as is.
> 
> > inconsistent whitespace around / operator
> 
> Would that be consistent enough? (The rule being, that a / operator
> on it's own gets spaces, but the same operator in a statement
> together with operators with lower binding power gets none.)
> 
> > > +	}
> > > +
> > > +	hum_int = dht11_gpio_decode_byte(timing, threshold);
> > > +	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
> > > +	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
> > > +	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
> > > +	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
> > > +
> > > +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0x00ff) != checksum)
> > 
> > maybe 0xff instead of 0x00ff is clearer
> 
> Ok, I'm happy either way.
> 
> > > +		return -EIO;
> > > +
> > > +	dht11->timestamp = iio_get_time_ns();
> > > +	if (hum_int < 20) {  /* DHT22 */
> > > +		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
> > > +					((temp_int & 0x80) ? -100 : 100);
> > > +		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
> > > +	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
> > 
> > extra space before comment
> 
> Actually I think that code is much more readable if end of line
> comments are visually separated by two or more spaces (and
> check_patch.pl is happy with that). I see now, that I have not
> been entirely consistent and would rather add extra spaces rather
> then remove them. What do you think?

I'd use just one space, just a matter of taste; no strong feelings :)
  
> > > +		dht11->temperature = temp_int * 1000;
> > > +		dht11->humidity = hum_int * 1000;
> > > +	} else {
> > > +		dev_err(dht11->dev,
> > > +			"Don't know how to decode data: %d %d %d %d\n",
> > > +			hum_int, hum_dec, temp_int, temp_dec);
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
> > > +			const struct iio_chan_spec *chan,
> > > +			int *val, int *val2, long m)
> > > +{
> > > +	struct dht11_gpio *dht11 = iio_priv(iio_dev);
> > > +	int ret = 0;
> > 
> > the initialization of ret is not used; don't or init with -EINVAL to save 
> > an assignment
> > 
> > > +
> > > +	if (dht11->timestamp + DATA_VALID_TIME < iio_get_time_ns()) {
> > > +		INIT_COMPLETION(dht11->completion);
> > > +
> > > +		dht11->num_edges = 0;
> > > +		ret = gpio_direction_output(dht11->gpio, 0);
> > > +		if (ret)
> > > +			goto err;
> > > +		msleep(STARTUP);
> > > +		ret = gpio_direction_input(dht11->gpio);
> > > +		if (ret)
> > > +			goto err;
> > > +
> > > +		ret = wait_for_completion_killable_timeout(&dht11->completion,
> > > +								 HZ);
> > > +		if (ret == 0 && dht11->num_edges < EDGES_PER_READ - 1) {
> > > +			dev_err(&iio_dev->dev,
> > > +					"Only %d signal edges detected\n",
> > > +					dht11->num_edges);
> > > +			ret = -ETIMEDOUT;
> > > +		}
> > > +		if (ret < 0)
> > > +			goto err;
> > > +
> > > +		ret = dht11_gpio_decode(dht11,
> > > +				dht11->num_edges == EDGES_PER_READ ?
> > > +					EDGES_PREAMBLE : EDGES_PREAMBLE - 2);
> > > +		if (ret)
> > > +			goto err;
> > > +	}
> > > +
> > > +	ret = IIO_VAL_INT;
> > > +	if (chan->channel == 0)
> > > +		*val = dht11->temperature;
> > > +	else if (chan->channel == 1)
> > > +		*val = dht11->humidity;
> > 
> > use channel type to discriminate channels
> 
> Yes, very good point.
> 
> I agree with the rest of the comments. Thanks for the review.
> Harald
>  
> > > +	else
> > > +		ret = -EINVAL;
> > > +err:
> > > +	dht11->num_edges = -1;
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct iio_info dht11_gpio_iio_info = {
> > > +	.driver_module		= THIS_MODULE,
> > > +	.read_raw		= dht11_gpio_read_raw,
> > > +};
> > > +
> > > +/*
> > > + * IRQ handler called on GPIO edges
> > > +*/
> > > +static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
> > > +{
> > > +	struct iio_dev *iio = data;
> > > +	struct dht11_gpio *dht11 = iio_priv(iio);
> > > +
> > > +	/* TODO: Consider making the handler safe for IRQ sharing */
> > > +	if (dht11->num_edges < EDGES_PER_READ && dht11->num_edges >= 0) {
> > > +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> > > +		dht11->edges[dht11->num_edges++].value =
> > > +						gpio_get_value(dht11->gpio);
> > > +
> > > +		if (dht11->num_edges >= EDGES_PER_READ)
> > > +			complete(&dht11->completion);
> > > +	}
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
> > > +	{ .type = IIO_TEMP, .channel = 0,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> > > +	{ .type = IIO_HUMIDITYRELATIVE, .channel = 1,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > 
> > .channel is not needed; there aren't multiple channels of the same type
> > 
> > > +}
> > 
> > weird indentation level
> > 
> > > +};
> > > +
> > > +static const struct of_device_id dht11_gpio_dt_ids[] = {
> > > +	{ .compatible = "dht11-gpio", },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
> > > +
> > > +static int dht11_gpio_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct device_node *node = dev->of_node;
> > > +	struct dht11_gpio *dht11;
> > > +	struct iio_dev *iio;
> > > +	int ret = 0;
> > 
> > don't initialize ret
> > 
> > > +
> > > +	/* Allocate the IIO device. */
> > 
> > obvious comment
> > 
> > > +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> > > +	if (!iio) {
> > > +		dev_err(dev, "Failed to allocate IIO device\n");
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	dht11 = iio_priv(iio);
> > > +	dht11->dev = dev;
> > > +
> > > +	dht11->gpio = ret = of_get_gpio(node, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dht11->irq = gpio_to_irq(dht11->gpio);
> > > +	if (dht11->irq < 0) {
> > > +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
> > > +		return -EINVAL;
> > > +	}
> > > +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
> > > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> > > +				pdev->name, iio);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	dht11->timestamp = iio_get_time_ns() - DATA_VALID_TIME - 1;
> > > +	dht11->num_edges = -1;
> > > +
> > > +	platform_set_drvdata(pdev, iio);
> > > +
> > > +	init_completion(&dht11->completion);
> > > +	iio->name = pdev->name;
> > > +	iio->dev.parent = &pdev->dev;
> > > +	iio->info = &dht11_gpio_iio_info;
> > > +	iio->modes = INDIO_DIRECT_MODE;
> > > +	iio->channels = dht11_gpio_chan_spec;
> > > +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
> > > +
> > > +	/* Register IIO device. */
> > 
> > the function name is pretty obvious, no comment needed
> > 
> > > +	ret = iio_device_register(iio);
> > 
> > maybe just return here, the extra output is not needed
> > 
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to register IIO device\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int dht11_gpio_remove(struct platform_device *pdev)
> > > +{
> > > +	struct iio_dev *iio = platform_get_drvdata(pdev);
> > > +
> > > +	iio_device_unregister(iio);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver dht11_gpio_driver = {
> > > +	.driver = {
> > > +		.name	= DRIVER_NAME,
> > > +		.owner	= THIS_MODULE,
> > > +		.of_match_table = dht11_gpio_dt_ids,
> > > +	},
> > > +	.probe  = dht11_gpio_probe,
> > > +	.remove = dht11_gpio_remove,
> > > +};
> > > +
> > > +module_platform_driver(dht11_gpio_driver);
> > > +
> > > +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
> > > +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> > > +
> > > 
> > 
> > -- 
> > 
> > Peter Meerwald
> > +43-664-2444418 (mobile)
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Harald Geyer]

Replying to both of your mails in one message here:

> > > +config DHT11_GPIO
> > > +   tristate "DHT11 (and compatible sensors) driver"
> > > +   help
> > > +     This driver supports reading data via a single interrupts
>
> should probably depend on GENERIC_GPIO?

After thinking about this a bit, I believe a dependency on
OF_GPIO is necessary. Maybe also add an explicit dependency
on GPIOLIB?

I believe GPIO_GENERIC ist not a dependency as it seems to
mostly be support code for other gpio controler drivers.
Actually, no other drivers like leds-gpio depend on it.
 
> > > prefix with DHT11_
>  
> > Do you think this is needed, even as all these defines are
> > local to this file? (It would force rather unpleasant line
> > breaks...) Is there some general rule when to prefix and
> > when not to?
> 
> breakage occurs when some of your include file #define e.g. START_BIT or 
> STARTUP in the future; I think the rule for iio is to prefix everything

Ok.
   
> > > > +#define EDGES_PREAMBLE	4
> > > > +#define BITS_PER_READ	40
> > > > +#define EDGES_PER_READ	(2*BITS_PER_READ + EDGES_PREAMBLE + 1)
> > > > +
> > > > +/* Data transmission timing (nano seconds) */
> > > > +#define STARTUP		18    /* ms */
> > > 
> > > just a line above nano seconds were promised, and now this: ms?! :)
> > 
> > Sure, the exception from the rule is noted as end of line comment.
> > Is there some better way to do this?
> 
> STARTUP is used exactly once; I'd just drop the #define and use the 
> constant directly -- you'll get a checkpatch warning for msleep(18) though
> 
> and startup is not transmission timing, right?

Actually it is in the sense that we need to pull down the gpio line
for at least this time to have the sensor start a transmission.
Maybe STARTTRANSMISSION is a better name than STARTUP.
 
It's not as critical as the other timing constants, but it is still
used whenever the driver wants any new data from the hardware.

Thanks for your help,
Harald

> > > > +#define SENSOR_RESPONSE	80000
> > > > +#define START_BIT	50000
> > > > +#define DATA_BIT_LOW	27000
> > > > +#define DATA_BIT_HIGH	70000
> > > > +
> > > > +/* TODO?: Support systems without DT? */
> > > > +
> > > > +struct dht11_gpio {
> > > > +	struct device			*dev;
> > > > +
> > > > +	int				gpio;
> > > > +	int				irq;
> > > > +
> > > > +	struct completion		completion;
> > > > +
> > > > +	s64				timestamp;
> > > > +	int				temperature;
> > > > +	int				humidity;
> > > > +
> > > > +	/* num_edges: -1 means "no transmission in progress" */
> > > > +	int				num_edges;
> > > > +	struct {s64 ts; int value; }	edges[EDGES_PER_READ];
> > > > +};
> > > > +
> > > > +/*
> > > > + * dht11_edges_print: show the data as actually received by the
> > > > + *                    driver.
> > > > + * This is dead code, keeping it for now just in case somebody needs
> > > > + * it for porting the driver to new sensor HW, etc.
> > > > + *
> > > > +static void dht11_edges_print(struct dht11_gpio *dht11)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	for (i = 1; i < dht11->num_edges; ++i) {
> > > > +		pr_err("dht11: %d: %lld ns %s\n", i,
> > > 
> > > inconsistent driver name; dht11-gpio was used before
> > > 
> > > > +			dht11->edges[i].ts - dht11->edges[i-1].ts,
> > > > +			dht11->edges[i-1].value ? "high" : "low");
> > > > +	}
> > > > +}
> > > > +*/
> > > > +
> > > > +static unsigned char dht11_gpio_decode_byte(int *timing, int threshold
> )
> > > > +{
> > > > +	unsigned char ret = 0;
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < 8; ++i) {
> > > > +		ret <<= 1;
> > > > +		if (timing[i] >= threshold)
> > > > +			++ret;
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int dht11_gpio_decode(struct dht11_gpio *dht11, int offset)
> > > > +{
> > > > +	int i, t, timing[BITS_PER_READ], threshold, timeres = SENSOR_RE
> SPONSE;
> > > > +	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
> > > > +
> > > > +	/* Calculate timestamp resolution */
> > > > +	for (i = 0; i < dht11->num_edges; ++i) {
> > > > +		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> > > > +		if (t > 0 && t < timeres)
> > > > +			timeres = t;
> > > > +	}
> > > > +	if (2*timeres > DATA_BIT_HIGH) {
> > > > +		pr_err("dht11-gpio: timeresolution %d too bad for decod
> ing\n",
> > > > +			timeres);
> > > > +		return -EIO;
> > > > +	}
> > > > +	threshold = DATA_BIT_HIGH/timeres;
> > 
> > I'm inclined to change this to:
> > 	threshold = DATA_BIT_HIGH / timeres;
> 
> this is what I had in mind
>  
> > > > +	if (DATA_BIT_LOW/timeres + 1 >= threshold)
> > > > +		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
> > > > +
> > > > +	/* scale down with timeres and check validity */
> > > > +	for (i = 0; i < BITS_PER_READ; ++i) {
> > > > +		t = dht11->edges[offset + 2*i + 2].ts -
> > > > +			dht11->edges[offset + 2*i + 1].ts;
> > > > +		if (!dht11->edges[offset + 2*i + 1].value)
> > > > +			return -EIO; /* lost synchronisation */
> > > > +		timing[i] = t / timeres;
> > 
> > and leave this as is.
> > 
> > > inconsistent whitespace around / operator
> > 
> > Would that be consistent enough? (The rule being, that a / operator
> > on it's own gets spaces, but the same operator in a statement
> > together with operators with lower binding power gets none.)
> > 
> > > > +	}
> > > > +
> > > > +	hum_int = dht11_gpio_decode_byte(timing, threshold);
> > > > +	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
> > > > +	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
> > > > +	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
> > > > +	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
> > > > +
> > > > +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0x00ff) != che
> cksum)
> > > 
> > > maybe 0xff instead of 0x00ff is clearer
> > 
> > Ok, I'm happy either way.
> > 
> > > > +		return -EIO;
> > > > +
> > > > +	dht11->timestamp = iio_get_time_ns();
> > > > +	if (hum_int < 20) {  /* DHT22 */
> > > > +		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_d
> ec) *
> > > > +					((temp_int & 0x80) ? -100 : 100
> );
> > > > +		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
> > > > +	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
> > > 
> > > extra space before comment
> > 
> > Actually I think that code is much more readable if end of line
> > comments are visually separated by two or more spaces (and
> > check_patch.pl is happy with that). I see now, that I have not
> > been entirely consistent and would rather add extra spaces rather
> > then remove them. What do you think?
> 
> I'd use just one space, just a matter of taste; no strong feelings :)
>   
> > > > +		dht11->temperature = temp_int * 1000;
> > > > +		dht11->humidity = hum_int * 1000;
> > > > +	} else {
> > > > +		dev_err(dht11->dev,
> > > > +			"Don't know how to decode data: %d %d %d %d\n",
> > > > +			hum_int, hum_dec, temp_int, temp_dec);
> > > > +		return -EIO;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
> > > > +			const struct iio_chan_spec *chan,
> > > > +			int *val, int *val2, long m)
> > > > +{
> > > > +	struct dht11_gpio *dht11 = iio_priv(iio_dev);
> > > > +	int ret = 0;
> > > 
> > > the initialization of ret is not used; don't or init with -EINVAL to save
>  
> > > an assignment
> > > 
> > > > +
> > > > +	if (dht11->timestamp + DATA_VALID_TIME < iio_get_time_ns()) {
> > > > +		INIT_COMPLETION(dht11->completion);
> > > > +
> > > > +		dht11->num_edges = 0;
> > > > +		ret = gpio_direction_output(dht11->gpio, 0);
> > > > +		if (ret)
> > > > +			goto err;
> > > > +		msleep(STARTUP);
> > > > +		ret = gpio_direction_input(dht11->gpio);
> > > > +		if (ret)
> > > > +			goto err;
> > > > +
> > > > +		ret = wait_for_completion_killable_timeout(&dht11->comp
> letion,
> > > > +								 HZ);
> > > > +		if (ret == 0 && dht11->num_edges < EDGES_PER_READ - 1) 
> {
> > > > +			dev_err(&iio_dev->dev,
> > > > +					"Only %d signal edges detected\
> n",
> > > > +					dht11->num_edges);
> > > > +			ret = -ETIMEDOUT;
> > > > +		}
> > > > +		if (ret < 0)
> > > > +			goto err;
> > > > +
> > > > +		ret = dht11_gpio_decode(dht11,
> > > > +				dht11->num_edges == EDGES_PER_READ ?
> > > > +					EDGES_PREAMBLE : EDGES_PREAMBLE
>  - 2);
> > > > +		if (ret)
> > > > +			goto err;
> > > > +	}
> > > > +
> > > > +	ret = IIO_VAL_INT;
> > > > +	if (chan->channel == 0)
> > > > +		*val = dht11->temperature;
> > > > +	else if (chan->channel == 1)
> > > > +		*val = dht11->humidity;
> > > 
> > > use channel type to discriminate channels
> > 
> > Yes, very good point.
> > 
> > I agree with the rest of the comments. Thanks for the review.
> > Harald
> >  
> > > > +	else
> > > > +		ret = -EINVAL;
> > > > +err:
> > > > +	dht11->num_edges = -1;
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static const struct iio_info dht11_gpio_iio_info = {
> > > > +	.driver_module		= THIS_MODULE,
> > > > +	.read_raw		= dht11_gpio_read_raw,
> > > > +};
> > > > +
> > > > +/*
> > > > + * IRQ handler called on GPIO edges
> > > > +*/
> > > > +static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
> > > > +{
> > > > +	struct iio_dev *iio = data;
> > > > +	struct dht11_gpio *dht11 = iio_priv(iio);
> > > > +
> > > > +	/* TODO: Consider making the handler safe for IRQ sharing */
> > > > +	if (dht11->num_edges < EDGES_PER_READ && dht11->num_edges >= 0)
>  {
> > > > +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> > > > +		dht11->edges[dht11->num_edges++].value =
> > > > +						gpio_get_value(dht11->g
> pio);
> > > > +
> > > > +		if (dht11->num_edges >= EDGES_PER_READ)
> > > > +			complete(&dht11->completion);
> > > > +	}
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
> > > > +	{ .type = IIO_TEMP, .channel = 0,
> > > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> > > > +	{ .type = IIO_HUMIDITYRELATIVE, .channel = 1,
> > > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > > 
> > > .channel is not needed; there aren't multiple channels of the same type
> > > 
> > > > +}
> > > 
> > > weird indentation level
> > > 
> > > > +};
> > > > +
> > > > +static const struct of_device_id dht11_gpio_dt_ids[] = {
> > > > +	{ .compatible = "dht11-gpio", },
> > > > +	{ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
> > > > +
> > > > +static int dht11_gpio_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct device_node *node = dev->of_node;
> > > > +	struct dht11_gpio *dht11;
> > > > +	struct iio_dev *iio;
> > > > +	int ret = 0;
> > > 
> > > don't initialize ret
> > > 
> > > > +
> > > > +	/* Allocate the IIO device. */
> > > 
> > > obvious comment
> > > 
> > > > +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> > > > +	if (!iio) {
> > > > +		dev_err(dev, "Failed to allocate IIO device\n");
> > > > +		return -ENOMEM;
> > > > +	}
> > > > +
> > > > +	dht11 = iio_priv(iio);
> > > > +	dht11->dev = dev;
> > > > +
> > > > +	dht11->gpio = ret = of_get_gpio(node, 0);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->n
> ame);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	dht11->irq = gpio_to_irq(dht11->gpio);
> > > > +	if (dht11->irq < 0) {
> > > > +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio)
> ;
> > > > +		return -EINVAL;
> > > > +	}
> > > > +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
> > > > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALL
> ING,
> > > > +				pdev->name, iio);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	dht11->timestamp = iio_get_time_ns() - DATA_VALID_TIME - 1;
> > > > +	dht11->num_edges = -1;
> > > > +
> > > > +	platform_set_drvdata(pdev, iio);
> > > > +
> > > > +	init_completion(&dht11->completion);
> > > > +	iio->name = pdev->name;
> > > > +	iio->dev.parent = &pdev->dev;
> > > > +	iio->info = &dht11_gpio_iio_info;
> > > > +	iio->modes = INDIO_DIRECT_MODE;
> > > > +	iio->channels = dht11_gpio_chan_spec;
> > > > +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
> > > > +
> > > > +	/* Register IIO device. */
> > > 
> > > the function name is pretty obvious, no comment needed
> > > 
> > > > +	ret = iio_device_register(iio);
> > > 
> > > maybe just return here, the extra output is not needed
> > > 
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to register IIO device\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dht11_gpio_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct iio_dev *iio = platform_get_drvdata(pdev);
> > > > +
> > > > +	iio_device_unregister(iio);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static struct platform_driver dht11_gpio_driver = {
> > > > +	.driver = {
> > > > +		.name	= DRIVER_NAME,
> > > > +		.owner	= THIS_MODULE,
> > > > +		.of_match_table = dht11_gpio_dt_ids,
> > > > +	},
> > > > +	.probe  = dht11_gpio_probe,
> > > > +	.remove = dht11_gpio_remove,
> > > > +};
> > > > +
> > > > +module_platform_driver(dht11_gpio_driver);
> > > > +
> > > > +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
> > > > +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
> > > > +MODULE_LICENSE("GPL v2");
> > > > +
> > > > 
> > > 
> > > -- 
> > > 
> > > Peter Meerwald
> > > +43-664-2444418 (mobile)
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> -- 
> 
> Peter Meerwald
> +43-664-2444418 (mobile)

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Jonathan Cameron]

Peter Meerwald <pmeerw@pmeerw.net> wrote:
>
>> > prefix with DHT11_
> 
>> Do you think this is needed, even as all these defines are
>> local to this file? (It would force rather unpleasant line
>> breaks...) Is there some general rule when to prefix and
>> when not to?
>
>breakage occurs when some of your include file #define e.g. START_BIT
>or 
>STARTUP in the future; I think the rule for iio is to prefix everything
Yes it is. Best way to avoid pain for me :)

Line length limit is not rigid so don't worry if it is obviously stupid to break a line. If in doubt keep to the limit though!
>  
>> > > +#define EDGES_PREAMBLE	4
>> > > +#define BITS_PER_READ	40
>> > > +#define EDGES_PER_READ	(2*BITS_PER_READ + EDGES_PREAMBLE + 1)
>> > > +
>> > > +/* Data transmission timing (nano seconds) */
>> > > +#define STARTUP		18    /* ms */
>> > 
>> > just a line above nano seconds were promised, and now this: ms?! :)
>> 
>> Sure, the exception from the rule is noted as end of line comment.
>> Is there some better way to do this?
>
>STARTUP is used exactly once; I'd just drop the #define and use the 
>constant directly -- you'll get a checkpatch warning for msleep(18)
>though
>
>and startup is not transmission timing, right?
>
>> > > +#define SENSOR_RESPONSE	80000
>> > > +#define START_BIT	50000
>> > > +#define DATA_BIT_LOW	27000
>> > > +#define DATA_BIT_HIGH	70000
>> > > +
>> > > +/* TODO?: Support systems without DT? */
>> > > +
>> > > +struct dht11_gpio {
>> > > +	struct device			*dev;
>> > > +
>> > > +	int				gpio;
>> > > +	int				irq;
>> > > +
>> > > +	struct completion		completion;
>> > > +
>> > > +	s64				timestamp;
>> > > +	int				temperature;
>> > > +	int				humidity;
>> > > +
>> > > +	/* num_edges: -1 means "no transmission in progress" */
>> > > +	int				num_edges;
>> > > +	struct {s64 ts; int value; }	edges[EDGES_PER_READ];
>> > > +};
>> > > +
>> > > +/*
>> > > + * dht11_edges_print: show the data as actually received by the
>> > > + *                    driver.
>> > > + * This is dead code, keeping it for now just in case somebody
>needs
>> > > + * it for porting the driver to new sensor HW, etc.
>> > > + *
>> > > +static void dht11_edges_print(struct dht11_gpio *dht11)
>> > > +{
>> > > +	int i;
>> > > +
>> > > +	for (i = 1; i < dht11->num_edges; ++i) {
>> > > +		pr_err("dht11: %d: %lld ns %s\n", i,
>> > 
>> > inconsistent driver name; dht11-gpio was used before
>> > 
>> > > +			dht11->edges[i].ts - dht11->edges[i-1].ts,
>> > > +			dht11->edges[i-1].value ? "high" : "low");
>> > > +	}
>> > > +}
>> > > +*/
>> > > +
>> > > +static unsigned char dht11_gpio_decode_byte(int *timing, int
>threshold)
>> > > +{
>> > > +	unsigned char ret = 0;
>> > > +	int i;
>> > > +
>> > > +	for (i = 0; i < 8; ++i) {
>> > > +		ret <<= 1;
>> > > +		if (timing[i] >= threshold)
>> > > +			++ret;
>> > > +	}
>> > > +
>> > > +	return ret;
>> > > +}
>> > > +
>> > > +static int dht11_gpio_decode(struct dht11_gpio *dht11, int
>offset)
>> > > +{
>> > > +	int i, t, timing[BITS_PER_READ], threshold, timeres =
>SENSOR_RESPONSE;
>> > > +	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
>> > > +
>> > > +	/* Calculate timestamp resolution */
>> > > +	for (i = 0; i < dht11->num_edges; ++i) {
>> > > +		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
>> > > +		if (t > 0 && t < timeres)
>> > > +			timeres = t;
>> > > +	}
>> > > +	if (2*timeres > DATA_BIT_HIGH) {
>> > > +		pr_err("dht11-gpio: timeresolution %d too bad for decoding\n",
>> > > +			timeres);
>> > > +		return -EIO;
>> > > +	}
>> > > +	threshold = DATA_BIT_HIGH/timeres;
>> 
>> I'm inclined to change this to:
>> 	threshold = DATA_BIT_HIGH / timeres;
>
>this is what I had in mind
> 
>> > > +	if (DATA_BIT_LOW/timeres + 1 >= threshold)
>> > > +		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
>> > > +
>> > > +	/* scale down with timeres and check validity */
>> > > +	for (i = 0; i < BITS_PER_READ; ++i) {
>> > > +		t = dht11->edges[offset + 2*i + 2].ts -
>> > > +			dht11->edges[offset + 2*i + 1].ts;
>> > > +		if (!dht11->edges[offset + 2*i + 1].value)
>> > > +			return -EIO; /* lost synchronisation */
>> > > +		timing[i] = t / timeres;
>> 
>> and leave this as is.
>> 
>> > inconsistent whitespace around / operator
>> 
>> Would that be consistent enough? (The rule being, that a / operator
>> on it's own gets spaces, but the same operator in a statement
>> together with operators with lower binding power gets none.)
>> 
>> > > +	}
>> > > +
>> > > +	hum_int = dht11_gpio_decode_byte(timing, threshold);
>> > > +	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
>> > > +	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
>> > > +	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
>> > > +	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
>> > > +
>> > > +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0x00ff) !=
>checksum)
>> > 
>> > maybe 0xff instead of 0x00ff is clearer
>> 
>> Ok, I'm happy either way.
>> 
>> > > +		return -EIO;
>> > > +
>> > > +	dht11->timestamp = iio_get_time_ns();
>> > > +	if (hum_int < 20) {  /* DHT22 */
>> > > +		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
>> > > +					((temp_int & 0x80) ? -100 : 100);
>> > > +		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
>> > > +	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
>> > 
>> > extra space before comment
>> 
>> Actually I think that code is much more readable if end of line
>> comments are visually separated by two or more spaces (and
>> check_patch.pl is happy with that). I see now, that I have not
>> been entirely consistent and would rather add extra spaces rather
>> then remove them. What do you think?
>
>I'd use just one space, just a matter of taste; no strong feelings :)
>  
>> > > +		dht11->temperature = temp_int * 1000;
>> > > +		dht11->humidity = hum_int * 1000;
>> > > +	} else {
>> > > +		dev_err(dht11->dev,
>> > > +			"Don't know how to decode data: %d %d %d %d\n",
>> > > +			hum_int, hum_dec, temp_int, temp_dec);
>> > > +		return -EIO;
>> > > +	}
>> > > +
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
>> > > +			const struct iio_chan_spec *chan,
>> > > +			int *val, int *val2, long m)
>> > > +{
>> > > +	struct dht11_gpio *dht11 = iio_priv(iio_dev);
>> > > +	int ret = 0;
>> > 
>> > the initialization of ret is not used; don't or init with -EINVAL
>to save 
>> > an assignment
>> > 
>> > > +
>> > > +	if (dht11->timestamp + DATA_VALID_TIME < iio_get_time_ns()) {
>> > > +		INIT_COMPLETION(dht11->completion);
>> > > +
>> > > +		dht11->num_edges = 0;
>> > > +		ret = gpio_direction_output(dht11->gpio, 0);
>> > > +		if (ret)
>> > > +			goto err;
>> > > +		msleep(STARTUP);
>> > > +		ret = gpio_direction_input(dht11->gpio);
>> > > +		if (ret)
>> > > +			goto err;
>> > > +
>> > > +		ret = wait_for_completion_killable_timeout(&dht11->completion,
>> > > +								 HZ);
>> > > +		if (ret == 0 && dht11->num_edges < EDGES_PER_READ - 1) {
>> > > +			dev_err(&iio_dev->dev,
>> > > +					"Only %d signal edges detected\n",
>> > > +					dht11->num_edges);
>> > > +			ret = -ETIMEDOUT;
>> > > +		}
>> > > +		if (ret < 0)
>> > > +			goto err;
>> > > +
>> > > +		ret = dht11_gpio_decode(dht11,
>> > > +				dht11->num_edges == EDGES_PER_READ ?
>> > > +					EDGES_PREAMBLE : EDGES_PREAMBLE - 2);
>> > > +		if (ret)
>> > > +			goto err;
>> > > +	}
>> > > +
>> > > +	ret = IIO_VAL_INT;
>> > > +	if (chan->channel == 0)
>> > > +		*val = dht11->temperature;
>> > > +	else if (chan->channel == 1)
>> > > +		*val = dht11->humidity;
>> > 
>> > use channel type to discriminate channels
>> 
>> Yes, very good point.
>> 
>> I agree with the rest of the comments. Thanks for the review.
>> Harald
>>  
>> > > +	else
>> > > +		ret = -EINVAL;
>> > > +err:
>> > > +	dht11->num_edges = -1;
>> > > +	return ret;
>> > > +}
>> > > +
>> > > +static const struct iio_info dht11_gpio_iio_info = {
>> > > +	.driver_module		= THIS_MODULE,
>> > > +	.read_raw		= dht11_gpio_read_raw,
>> > > +};
>> > > +
>> > > +/*
>> > > + * IRQ handler called on GPIO edges
>> > > +*/
>> > > +static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
>> > > +{
>> > > +	struct iio_dev *iio = data;
>> > > +	struct dht11_gpio *dht11 = iio_priv(iio);
>> > > +
>> > > +	/* TODO: Consider making the handler safe for IRQ sharing */
>> > > +	if (dht11->num_edges < EDGES_PER_READ && dht11->num_edges >= 0)
>{
>> > > +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
>> > > +		dht11->edges[dht11->num_edges++].value =
>> > > +						gpio_get_value(dht11->gpio);
>> > > +
>> > > +		if (dht11->num_edges >= EDGES_PER_READ)
>> > > +			complete(&dht11->completion);
>> > > +	}
>> > > +
>> > > +	return IRQ_HANDLED;
>> > > +}
>> > > +
>> > > +static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
>> > > +	{ .type = IIO_TEMP, .channel = 0,
>> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
>> > > +	{ .type = IIO_HUMIDITYRELATIVE, .channel = 1,
>> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>> > 
>> > .channel is not needed; there aren't multiple channels of the same
>type
>> > 
>> > > +}
>> > 
>> > weird indentation level
>> > 
>> > > +};
>> > > +
>> > > +static const struct of_device_id dht11_gpio_dt_ids[] = {
>> > > +	{ .compatible = "dht11-gpio", },
>> > > +	{ }
>> > > +};
>> > > +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
>> > > +
>> > > +static int dht11_gpio_probe(struct platform_device *pdev)
>> > > +{
>> > > +	struct device *dev = &pdev->dev;
>> > > +	struct device_node *node = dev->of_node;
>> > > +	struct dht11_gpio *dht11;
>> > > +	struct iio_dev *iio;
>> > > +	int ret = 0;
>> > 
>> > don't initialize ret
>> > 
>> > > +
>> > > +	/* Allocate the IIO device. */
>> > 
>> > obvious comment
>> > 
>> > > +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
>> > > +	if (!iio) {
>> > > +		dev_err(dev, "Failed to allocate IIO device\n");
>> > > +		return -ENOMEM;
>> > > +	}
>> > > +
>> > > +	dht11 = iio_priv(iio);
>> > > +	dht11->dev = dev;
>> > > +
>> > > +	dht11->gpio = ret = of_get_gpio(node, 0);
>> > > +	if (ret < 0)
>> > > +		return ret;
>> > > +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN,
>pdev->name);
>> > > +	if (ret)
>> > > +		return ret;
>> > > +
>> > > +	dht11->irq = gpio_to_irq(dht11->gpio);
>> > > +	if (dht11->irq < 0) {
>> > > +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
>> > > +		return -EINVAL;
>> > > +	}
>> > > +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
>> > > +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> > > +				pdev->name, iio);
>> > > +	if (ret)
>> > > +		return ret;
>> > > +
>> > > +	dht11->timestamp = iio_get_time_ns() - DATA_VALID_TIME - 1;
>> > > +	dht11->num_edges = -1;
>> > > +
>> > > +	platform_set_drvdata(pdev, iio);
>> > > +
>> > > +	init_completion(&dht11->completion);
>> > > +	iio->name = pdev->name;
>> > > +	iio->dev.parent = &pdev->dev;
>> > > +	iio->info = &dht11_gpio_iio_info;
>> > > +	iio->modes = INDIO_DIRECT_MODE;
>> > > +	iio->channels = dht11_gpio_chan_spec;
>> > > +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
>> > > +
>> > > +	/* Register IIO device. */
>> > 
>> > the function name is pretty obvious, no comment needed
>> > 
>> > > +	ret = iio_device_register(iio);
>> > 
>> > maybe just return here, the extra output is not needed
>> > 
>> > > +	if (ret) {
>> > > +		dev_err(dev, "Failed to register IIO device\n");
>> > > +		return ret;
>> > > +	}
>> > > +
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static int dht11_gpio_remove(struct platform_device *pdev)
>> > > +{
>> > > +	struct iio_dev *iio = platform_get_drvdata(pdev);
>> > > +
>> > > +	iio_device_unregister(iio);
>> > > +
>> > > +	return 0;
>> > > +}
>> > > +
>> > > +static struct platform_driver dht11_gpio_driver = {
>> > > +	.driver = {
>> > > +		.name	= DRIVER_NAME,
>> > > +		.owner	= THIS_MODULE,
>> > > +		.of_match_table = dht11_gpio_dt_ids,
>> > > +	},
>> > > +	.probe  = dht11_gpio_probe,
>> > > +	.remove = dht11_gpio_remove,
>> > > +};
>> > > +
>> > > +module_platform_driver(dht11_gpio_driver);
>> > > +
>> > > +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
>> > > +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
>> > > +MODULE_LICENSE("GPL v2");
>> > > +
>> > > 
>> > 
>> > -- 
>> > 
>> > Peter Meerwald
>> > +43-664-2444418 (mobile)
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe
>linux-iio" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/2] iio: Add new driver dht11-gpio

[Lars-Peter Clausen]

On 10/22/2013 11:28 PM, Harald Geyer wrote:
> Replying to both of your mails in one message here:
> 
>>>> +config DHT11_GPIO
>>>> +   tristate "DHT11 (and compatible sensors) driver"
>>>> +   help
>>>> +     This driver supports reading data via a single interrupts
>>
>> should probably depend on GENERIC_GPIO?
> 
> After thinking about this a bit, I believe a dependency on
> OF_GPIO is necessary. Maybe also add an explicit dependency
> on GPIOLIB?

The of_gpio calls are stubbed out if OF_GPIO is not selected. So there is no
need to add the dependency and not having the dependency makes compile
testing easier on platforms which e.g. do not support OF. But the
GENERIC_GPIO symbol got removed recently, since everybody is using GPIOLIB
these days. So that's the right symbol to depend on.

- Lars

[PATCH v2 2/2] iio: Add new driver dht11-gpio

[Harald Geyer]

This driver handles DHT11 and DHT22 sensors.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
changes since v1 (tested on DHT11):
Add dependency on GPIOLIB
Prefix all local preprocessor macros with DHT11_
Rename STARTUP to START_TRANSMISSION
Remove leading zeros
Simplify channel disambiguation
Remove obvious comments
Make whitespace more consistent
Strip unnecessary output and simplify error handling
Fix spelling errors

The v1 patch has been tested with DHT11 and DHT22 hardware.

Thanks,
Harald

 .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
 drivers/iio/Kconfig                                |    1 +
 drivers/iio/Makefile                               |    1 +
 drivers/iio/humidity/Kconfig                       |   15 +
 drivers/iio/humidity/Makefile                      |    5 +
 drivers/iio/humidity/dht11-gpio.c                  |  325 ++++++++++++++++++++
 6 files changed, 361 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
 create mode 100644 drivers/iio/humidity/Kconfig
 create mode 100644 drivers/iio/humidity/Makefile
 create mode 100644 drivers/iio/humidity/dht11-gpio.c

diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
new file mode 100644
index 0000000..ee8fa04
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
@@ -0,0 +1,14 @@
+* DHT11 humidity/temperature sensor (and compatibles like DHT22)
+
+Required properties:
+  - compatible: Should be "dht11-gpio"
+  - gpios: Should specify the GPIO connected to the sensor's data
+    line, see "gpios property" in
+    Documentation/devicetree/bindings/gpio/gpio.txt.
+
+Example:
+
+humidity_sensor {
+	compatible = "dht11-gpio";
+	gpios = <&gpio0 6 0>;
+}
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 90cf0cd..a5ed882 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
 source "drivers/iio/dac/Kconfig"
 source "drivers/iio/frequency/Kconfig"
 source "drivers/iio/gyro/Kconfig"
+source "drivers/iio/humidity/Kconfig"
 source "drivers/iio/imu/Kconfig"
 source "drivers/iio/light/Kconfig"
 source "drivers/iio/magnetometer/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index bcf7e9e..b3b5096 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -18,6 +18,7 @@ obj-y += common/
 obj-y += dac/
 obj-y += gyro/
 obj-y += frequency/
+obj-y += humidity/
 obj-y += imu/
 obj-y += light/
 obj-y += magnetometer/
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
new file mode 100644
index 0000000..bbc44f2
--- /dev/null
+++ b/drivers/iio/humidity/Kconfig
@@ -0,0 +1,15 @@
+#
+# humidity sensor drivers
+#
+menu "Humidity sensors"
+
+config DHT11_GPIO
+	tristate "DHT11 (and compatible sensors) driver"
+	depends on GPIOLIB
+	help
+	  This driver supports reading data via a single interrupt
+	  generating GPIO line. Currently tested are DHT11 and DHT22.
+	  Other sensors should work as well as long as they speak the
+	  same protocol.
+
+endmenu
diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
new file mode 100644
index 0000000..5e8226f
--- /dev/null
+++ b/drivers/iio/humidity/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for IIO humidity sensor drivers
+#
+
+obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11-gpio.c
new file mode 100644
index 0000000..3bb4d81
--- /dev/null
+++ b/drivers/iio/humidity/dht11-gpio.c
@@ -0,0 +1,325 @@
+/*
+ * DHT11/DHT22 bit banging GPIO driver
+ *
+ * Copyright (c) Harald Geyer <harald@ccbib.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/wait.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#include <linux/iio/iio.h>
+
+#define DRIVER_NAME	"dht11-gpio"
+
+#define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
+
+#define DHT11_EDGES_PREAMBLE 4
+#define DHT11_BITS_PER_READ 40
+#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
+
+/* Data transmission timing (nano seconds) */
+#define DHT11_START_TRANSMISSION	18  /* ms */
+#define DHT11_SENSOR_RESPONSE	80000
+#define DHT11_START_BIT		50000
+#define DHT11_DATA_BIT_LOW	27000
+#define DHT11_DATA_BIT_HIGH	70000
+
+/* TODO?: Support systems without DT? */
+
+struct dht11_gpio {
+	struct device			*dev;
+
+	int				gpio;
+	int				irq;
+
+	struct completion		completion;
+
+	s64				timestamp;
+	int				temperature;
+	int				humidity;
+
+	/* num_edges: -1 means "no transmission in progress" */
+	int				num_edges;
+	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
+};
+
+/*
+ * dht11_edges_print: show the data as actually received by the
+ *                    driver.
+ * This is dead code, keeping it for now just in case somebody needs
+ * it for porting the driver to new sensor HW, etc.
+ *
+static void dht11_edges_print(struct dht11_gpio *dht11)
+{
+	int i;
+
+	for (i = 1; i < dht11->num_edges; ++i) {
+		pr_err("dht11-gpio: %d: %lld ns %s\n", i,
+			dht11->edges[i].ts - dht11->edges[i-1].ts,
+			dht11->edges[i-1].value ? "high" : "low");
+	}
+}
+*/
+
+static unsigned char dht11_gpio_decode_byte(int *timing, int threshold)
+{
+	unsigned char ret = 0;
+	int i;
+
+	for (i = 0; i < 8; ++i) {
+		ret <<= 1;
+		if (timing[i] >= threshold)
+			++ret;
+	}
+
+	return ret;
+}
+
+static int dht11_gpio_decode(struct dht11_gpio *dht11, int offset)
+{
+	int i, t, timing[DHT11_BITS_PER_READ], threshold,
+		timeres = DHT11_SENSOR_RESPONSE;
+	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
+
+	/* Calculate timestamp resolution */
+	for (i = 0; i < dht11->num_edges; ++i) {
+		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
+		if (t > 0 && t < timeres)
+			timeres = t;
+	}
+	if (2*timeres > DHT11_DATA_BIT_HIGH) {
+		pr_err("dht11-gpio: timeresolution %d too bad for decoding\n",
+			timeres);
+		return -EIO;
+	}
+	threshold = DHT11_DATA_BIT_HIGH / timeres;
+	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
+		pr_err("dht11-gpio: WARNING: decoding ambiguous\n");
+
+	/* scale down with timeres and check validity */
+	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
+		t = dht11->edges[offset + 2*i + 2].ts -
+			dht11->edges[offset + 2*i + 1].ts;
+		if (!dht11->edges[offset + 2*i + 1].value)
+			return -EIO;  /* lost synchronisation */
+		timing[i] = t / timeres;
+	}
+
+	hum_int = dht11_gpio_decode_byte(timing, threshold);
+	hum_dec = dht11_gpio_decode_byte(&timing[8], threshold);
+	temp_int = dht11_gpio_decode_byte(&timing[16], threshold);
+	temp_dec = dht11_gpio_decode_byte(&timing[24], threshold);
+	checksum = dht11_gpio_decode_byte(&timing[32], threshold);
+
+	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+		return -EIO;
+
+	dht11->timestamp = iio_get_time_ns();
+	if (hum_int < 20) {  /* DHT22 */
+		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
+					((temp_int & 0x80) ? -100 : 100);
+		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
+	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
+		dht11->temperature = temp_int * 1000;
+		dht11->humidity = hum_int * 1000;
+	} else {
+		dev_err(dht11->dev,
+			"Don't know how to decode data: %d %d %d %d\n",
+			hum_int, hum_dec, temp_int, temp_dec);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int dht11_gpio_read_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int *val, int *val2, long m)
+{
+	struct dht11_gpio *dht11 = iio_priv(iio_dev);
+	int ret;
+
+	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
+		INIT_COMPLETION(dht11->completion);
+
+		dht11->num_edges = 0;
+		ret = gpio_direction_output(dht11->gpio, 0);
+		if (ret)
+			goto err;
+		msleep(DHT11_START_TRANSMISSION);
+		ret = gpio_direction_input(dht11->gpio);
+		if (ret)
+			goto err;
+
+		ret = wait_for_completion_killable_timeout(&dht11->completion,
+								 HZ);
+		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
+			dev_err(&iio_dev->dev,
+					"Only %d signal edges detected\n",
+					dht11->num_edges);
+			ret = -ETIMEDOUT;
+		}
+		if (ret < 0)
+			goto err;
+
+		ret = dht11_gpio_decode(dht11,
+				dht11->num_edges == DHT11_EDGES_PER_READ ?
+					DHT11_EDGES_PREAMBLE :
+					DHT11_EDGES_PREAMBLE - 2);
+		if (ret)
+			goto err;
+	}
+
+	ret = IIO_VAL_INT;
+	if (chan->type == IIO_TEMP)
+		*val = dht11->temperature;
+	else if (chan->type == IIO_HUMIDITYRELATIVE)
+		*val = dht11->humidity;
+	else
+		ret = -EINVAL;
+err:
+	dht11->num_edges = -1;
+	return ret;
+}
+
+static const struct iio_info dht11_gpio_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= dht11_gpio_read_raw,
+};
+
+/*
+ * IRQ handler called on GPIO edges
+*/
+static irqreturn_t dht11_gpio_handle_irq(int irq, void *data)
+{
+	struct iio_dev *iio = data;
+	struct dht11_gpio *dht11 = iio_priv(iio);
+
+	/* TODO: Consider making the handler safe for IRQ sharing */
+	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
+		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
+		dht11->edges[dht11->num_edges++].value =
+						gpio_get_value(dht11->gpio);
+
+		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
+			complete(&dht11->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_chan_spec dht11_gpio_chan_spec[] = {
+	{ .type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
+	{ .type = IIO_HUMIDITYRELATIVE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), }
+};
+
+static const struct of_device_id dht11_gpio_dt_ids[] = {
+	{ .compatible = "dht11-gpio", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
+
+static int dht11_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct dht11_gpio *dht11;
+	struct iio_dev *iio;
+	int ret;
+
+	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
+	if (!iio) {
+		dev_err(dev, "Failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	dht11 = iio_priv(iio);
+	dht11->dev = dev;
+
+	dht11->gpio = ret = of_get_gpio(node, 0);
+	if (ret < 0)
+		return ret;
+	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
+	if (ret)
+		return ret;
+
+	dht11->irq = gpio_to_irq(dht11->gpio);
+	if (dht11->irq < 0) {
+		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
+		return -EINVAL;
+	}
+	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				pdev->name, iio);
+	if (ret)
+		return ret;
+
+	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
+	dht11->num_edges = -1;
+
+	platform_set_drvdata(pdev, iio);
+
+	init_completion(&dht11->completion);
+	iio->name = pdev->name;
+	iio->dev.parent = &pdev->dev;
+	iio->info = &dht11_gpio_iio_info;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->channels = dht11_gpio_chan_spec;
+	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
+
+	return iio_device_register(iio);
+}
+
+static int dht11_gpio_remove(struct platform_device *pdev)
+{
+	struct iio_dev *iio = platform_get_drvdata(pdev);
+
+	iio_device_unregister(iio);
+
+	return 0;
+}
+
+static struct platform_driver dht11_gpio_driver = {
+	.driver = {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = dht11_gpio_dt_ids,
+	},
+	.probe  = dht11_gpio_probe,
+	.remove = dht11_gpio_remove,
+};
+
+module_platform_driver(dht11_gpio_driver);
+
+MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
+MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
+MODULE_LICENSE("GPL v2");
+
-- 
1.7.2.5

Re: [PATCH v2 2/2] iio: Add new driver dht11-gpio

[Jonathan Cameron]

On 10/25/13 12:47, Harald Geyer wrote:
> This driver handles DHT11 and DHT22 sensors.
>
> Signed-off-by: Harald Geyer <harald@ccbib.org>

Hi Harald,

I'm afraid I completely missed your posting of this version 2.

Do feel free to pester me if I fail to say anything about a patch
for a couple of weeks.

Anyhow
> ---
> changes since v1 (tested on DHT11):
> Add dependency on GPIOLIB
> Prefix all local preprocessor macros with DHT11_
> Rename STARTUP to START_TRANSMISSION
> Remove leading zeros
> Simplify channel disambiguation
> Remove obvious comments
> Make whitespace more consistent
> Strip unnecessary output and simplify error handling
> Fix spelling errors
>
> The v1 patch has been tested with DHT11 and DHT22 hardware.
There are a few redundant bits below that want to be dropped.

The only real issue I have otherwise is with the naming.
I would expect *-gpio to be a driver providing gpios?
Just go with dht11.  This applies to the bindings as well.

Also note that policy is now to cc the device tree list for
all patches changing or introducing bindings.

I've cc'd them on this reply.

Thanks,

Jonathan
>
> Thanks,
> Harald
>
>  .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
>  drivers/iio/Kconfig                                |    1 +
>  drivers/iio/Makefile                               |    1 +
>  drivers/iio/humidity/Kconfig                       |   15 +
>  drivers/iio/humidity/Makefile                      |    5 +
>  drivers/iio/humidity/dht11-gpio.c                  |  325 ++++++++++++++++++++
>  6 files changed, 361 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>  create mode 100644 drivers/iio/humidity/Kconfig
>  create mode 100644 drivers/iio/humidity/Makefile
>  create mode 100644 drivers/iio/humidity/dht11-gpio.c
>
> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
> new file mode 100644
> index 0000000..ee8fa04
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
> @@ -0,0 +1,14 @@
> +* DHT11 humidity/temperature sensor (and compatibles like DHT22)
> +
> +Required properties:
> +  - compatible: Should be "dht11-gpio"
> +  - gpios: Should specify the GPIO connected to the sensor's data
> +    line, see "gpios property" in
> +    Documentation/devicetree/bindings/gpio/gpio.txt.
> +
> +Example:
> +
> +humidity_sensor {
> +	compatible = "dht11-gpio";
> +	gpios = <&gpio0 6 0>;
> +}
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 90cf0cd..a5ed882 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
>  source "drivers/iio/dac/Kconfig"
>  source "drivers/iio/frequency/Kconfig"
>  source "drivers/iio/gyro/Kconfig"
> +source "drivers/iio/humidity/Kconfig"
>  source "drivers/iio/imu/Kconfig"
>  source "drivers/iio/light/Kconfig"
>  source "drivers/iio/magnetometer/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index bcf7e9e..b3b5096 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -18,6 +18,7 @@ obj-y += common/
>  obj-y += dac/
>  obj-y += gyro/
>  obj-y += frequency/
> +obj-y += humidity/
>  obj-y += imu/
>  obj-y += light/
>  obj-y += magnetometer/
> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> new file mode 100644
> index 0000000..bbc44f2
> --- /dev/null
> +++ b/drivers/iio/humidity/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# humidity sensor drivers
> +#
> +menu "Humidity sensors"
> +
> +config DHT11_GPIO
The _GPIO to my mind would normally imply that this as a driver
providing GPIOs rather than using them.
> +	tristate "DHT11 (and compatible sensors) driver"
> +	depends on GPIOLIB
> +	help
> +	  This driver supports reading data via a single interrupt
> +	  generating GPIO line. Currently tested are DHT11 and DHT22.
> +	  Other sensors should work as well as long as they speak the
> +	  same protocol.
> +
> +endmenu
> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
> new file mode 100644
> index 0000000..5e8226f
> --- /dev/null
> +++ b/drivers/iio/humidity/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for IIO humidity sensor drivers
> +#
> +
> +obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
> diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11-gpio.c
> new file mode 100644
> index 0000000..3bb4d81
> --- /dev/null
> +++ b/drivers/iio/humidity/dht11-gpio.c
> @@ -0,0 +1,325 @@
...
Don't bother with this comment.  If anyone wants it they can
add support :)
> +/* TODO?: Support systems without DT? */
> +
> +struct dht11_gpio {
> +	struct device			*dev;
> +
> +	int				gpio;
> +	int				irq;
> +
> +	struct completion		completion;
> +
> +	s64				timestamp;
> +	int				temperature;
> +	int				humidity;
> +
> +	/* num_edges: -1 means "no transmission in progress" */
> +	int				num_edges;
> +	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
> +};
> +
This isn't called anywhere and so should be removed from the driver.
> +/*
> + * dht11_edges_print: show the data as actually received by the
> + *                    driver.
> + * This is dead code, keeping it for now just in case somebody needs
> + * it for porting the driver to new sensor HW, etc.
> + *
> +static void dht11_edges_print(struct dht11_gpio *dht11)
> +{
> +	int i;
> +
> +	for (i = 1; i < dht11->num_edges; ++i) {
> +		pr_err("dht11-gpio: %d: %lld ns %s\n", i,
> +			dht11->edges[i].ts - dht11->edges[i-1].ts,
> +			dht11->edges[i-1].value ? "high" : "low");
> +	}
> +}
> +*/
...

> +static const struct of_device_id dht11_gpio_dt_ids[] = {
> +	{ .compatible = "dht11-gpio", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
> +
> +static int dht11_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct dht11_gpio *dht11;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> +	if (!iio) {
> +		dev_err(dev, "Failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	dht11 = iio_priv(iio);
> +	dht11->dev = dev;
> +
> +	dht11->gpio = ret = of_get_gpio(node, 0);
> +	if (ret < 0)
> +		return ret;
> +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
> +	if (ret)
> +		return ret;
> +
> +	dht11->irq = gpio_to_irq(dht11->gpio);
> +	if (dht11->irq < 0) {
> +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
> +		return -EINVAL;
> +	}
> +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				pdev->name, iio);
> +	if (ret)
> +		return ret;
> +
> +	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
> +	dht11->num_edges = -1;
> +
> +	platform_set_drvdata(pdev, iio);
> +
> +	init_completion(&dht11->completion);
> +	iio->name = pdev->name;
> +	iio->dev.parent = &pdev->dev;
> +	iio->info = &dht11_gpio_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = dht11_gpio_chan_spec;
> +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
> +
> +	return iio_device_register(iio);
> +}
> +

Could use the new devm_iio_device_register() call and drop
this boilerplate.

> +static int dht11_gpio_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *iio = platform_get_drvdata(pdev);
> +
> +	iio_device_unregister(iio);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver dht11_gpio_driver = {
> +	.driver = {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +		.of_match_table = dht11_gpio_dt_ids,
> +	},
> +	.probe  = dht11_gpio_probe,
> +	.remove = dht11_gpio_remove,
> +};
> +
> +module_platform_driver(dht11_gpio_driver);
> +
> +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
> +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
> +MODULE_LICENSE("GPL v2");
> +
>

Re: [PATCH v2 2/2] iio: Add new driver dht11-gpio

[Harald Geyer]

Jonathan Cameron writes:
>On 10/25/13 12:47, Harald Geyer wrote:
>> This driver handles DHT11 and DHT22 sensors.
>>
>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>
> Hi Harald,
>
> I'm afraid I completely missed your posting of this version 2.
>
> Do feel free to pester me if I fail to say anything about a patch
> for a couple of weeks.

Well, was going to do so in a few days ... :)

> Anyhow
>> ---
>> changes since v1 (tested on DHT11):
>> Add dependency on GPIOLIB
>> Prefix all local preprocessor macros with DHT11_
>> Rename STARTUP to START_TRANSMISSION
>> Remove leading zeros
>> Simplify channel disambiguation
>> Remove obvious comments
>> Make whitespace more consistent
>> Strip unnecessary output and simplify error handling
>> Fix spelling errors
>>
>> The v1 patch has been tested with DHT11 and DHT22 hardware.
> There are a few redundant bits below that want to be dropped.
>
> The only real issue I have otherwise is with the naming.
> I would expect *-gpio to be a driver providing gpios?

At least there are 'leds-gpio' and 'w1-gpio' already.
I was under the impression that drivers providing gpios
start with 'gpio' like 'gpio-mxs' ...

> Just go with dht11.  This applies to the bindings as well.

I'm happy to grab 'dht11' but am reluctant to do so because
somebody might write a DHT11 driver using some other IO hardware
then gpio. How would this be called then?

> Also note that policy is now to cc the device tree list for
> all patches changing or introducing bindings.
>
> I've cc'd them on this reply.

Thanks.

> Thanks,
>
> Jonathan
>>
>> Thanks,
>> Harald
>>
>>  .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
>>  drivers/iio/Kconfig                                |    1 +
>>  drivers/iio/Makefile                               |    1 +
>>  drivers/iio/humidity/Kconfig                       |   15 +
>>  drivers/iio/humidity/Makefile                      |    5 +
>>  drivers/iio/humidity/dht11-gpio.c                  |  325 ++++++++++++++++++++
>>  6 files changed, 361 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>>  create mode 100644 drivers/iio/humidity/Kconfig
>>  create mode 100644 drivers/iio/humidity/Makefile
>>  create mode 100644 drivers/iio/humidity/dht11-gpio.c
>>
>> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>> new file mode 100644
>> index 0000000..ee8fa04
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>> @@ -0,0 +1,14 @@
>> +* DHT11 humidity/temperature sensor (and compatibles like DHT22)
>> +
>> +Required properties:
>> +  - compatible: Should be "dht11-gpio"
>> +  - gpios: Should specify the GPIO connected to the sensor's data
>> +    line, see "gpios property" in
>> +    Documentation/devicetree/bindings/gpio/gpio.txt.
>> +
>> +Example:
>> +
>> +humidity_sensor {
>> +	compatible = "dht11-gpio";
>> +	gpios = <&gpio0 6 0>;
>> +}
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index 90cf0cd..a5ed882 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
>>  source "drivers/iio/dac/Kconfig"
>>  source "drivers/iio/frequency/Kconfig"
>>  source "drivers/iio/gyro/Kconfig"
>> +source "drivers/iio/humidity/Kconfig"
>>  source "drivers/iio/imu/Kconfig"
>>  source "drivers/iio/light/Kconfig"
>>  source "drivers/iio/magnetometer/Kconfig"
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index bcf7e9e..b3b5096 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -18,6 +18,7 @@ obj-y += common/
>>  obj-y += dac/
>>  obj-y += gyro/
>>  obj-y += frequency/
>> +obj-y += humidity/
>>  obj-y += imu/
>>  obj-y += light/
>>  obj-y += magnetometer/
>> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
>> new file mode 100644
>> index 0000000..bbc44f2
>> --- /dev/null
>> +++ b/drivers/iio/humidity/Kconfig
>> @@ -0,0 +1,15 @@
>> +#
>> +# humidity sensor drivers
>> +#
>> +menu "Humidity sensors"
>> +
>> +config DHT11_GPIO
> The _GPIO to my mind would normally imply that this as a driver
> providing GPIOs rather than using them.

I will change this according to the result of the naming
discussion.

>> +	tristate "DHT11 (and compatible sensors) driver"
>> +	depends on GPIOLIB
>> +	help
>> +	  This driver supports reading data via a single interrupt
>> +	  generating GPIO line. Currently tested are DHT11 and DHT22.
>> +	  Other sensors should work as well as long as they speak the
>> +	  same protocol.
>> +
>> +endmenu
>> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
>> new file mode 100644
>> index 0000000..5e8226f
>> --- /dev/null
>> +++ b/drivers/iio/humidity/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Makefile for IIO humidity sensor drivers
>> +#
>> +
>> +obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
>> diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11-gpio.c
>> new file mode 100644
>> index 0000000..3bb4d81
>> --- /dev/null
>> +++ b/drivers/iio/humidity/dht11-gpio.c
>> @@ -0,0 +1,325 @@
> ...
> Don't bother with this comment.  If anyone wants it they can
> add support :)
>> +/* TODO?: Support systems without DT? */

Ok.

>> +
>> +struct dht11_gpio {
>> +	struct device			*dev;
>> +
>> +	int				gpio;
>> +	int				irq;
>> +
>> +	struct completion		completion;
>> +
>> +	s64				timestamp;
>> +	int				temperature;
>> +	int				humidity;
>> +
>> +	/* num_edges: -1 means "no transmission in progress" */
>> +	int				num_edges;
>> +	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>> +};
>> +
> This isn't called anywhere and so should be removed from the driver.

I will do so, if you think it's more apropriate. I figured that
commenting it out is like removing but retaining a useful comment.
Maybe prefix the whole comment with asterix to make it more obvious
this actually is a comment?

>> +/*
>> + * dht11_edges_print: show the data as actually received by the
>> + *                    driver.
>> + * This is dead code, keeping it for now just in case somebody needs
>> + * it for porting the driver to new sensor HW, etc.
>> + *
>> +static void dht11_edges_print(struct dht11_gpio *dht11)
>> +{
>> +	int i;
>> +
>> +	for (i = 1; i < dht11->num_edges; ++i) {
>> +		pr_err("dht11-gpio: %d: %lld ns %s\n", i,
>> +			dht11->edges[i].ts - dht11->edges[i-1].ts,
>> +			dht11->edges[i-1].value ? "high" : "low");
>> +	}
>> +}
>> +*/
> ...



>> +static const struct of_device_id dht11_gpio_dt_ids[] = {
>> +	{ .compatible = "dht11-gpio", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
>> +
>> +static int dht11_gpio_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *node = dev->of_node;
>> +	struct dht11_gpio *dht11;
>> +	struct iio_dev *iio;
>> +	int ret;
>> +
>> +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
>> +	if (!iio) {
>> +		dev_err(dev, "Failed to allocate IIO device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	dht11 = iio_priv(iio);
>> +	dht11->dev = dev;
>> +
>> +	dht11->gpio = ret = of_get_gpio(node, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dht11->irq = gpio_to_irq(dht11->gpio);
>> +	if (dht11->irq < 0) {
>> +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
>> +		return -EINVAL;
>> +	}
>> +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +				pdev->name, iio);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>> +	dht11->num_edges = -1;
>> +
>> +	platform_set_drvdata(pdev, iio);
>> +
>> +	init_completion(&dht11->completion);
>> +	iio->name = pdev->name;
>> +	iio->dev.parent = &pdev->dev;
>> +	iio->info = &dht11_gpio_iio_info;
>> +	iio->modes = INDIO_DIRECT_MODE;
>> +	iio->channels = dht11_gpio_chan_spec;
>> +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
>> +
>> +	return iio_device_register(iio);
>> +}
>> +
>
> Could use the new devm_iio_device_register() call and drop
> this boilerplate.

Yes, I'll send a v3 with this after the other issues are sorted
out.

Thanks,
Harald

>> +static int dht11_gpio_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *iio = platform_get_drvdata(pdev);
>> +
>> +	iio_device_unregister(iio);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver dht11_gpio_driver = {
>> +	.driver = {
>> +		.name	= DRIVER_NAME,
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = dht11_gpio_dt_ids,
>> +	},
>> +	.probe  = dht11_gpio_probe,
>> +	.remove = dht11_gpio_remove,
>> +};
>> +
>> +module_platform_driver(dht11_gpio_driver);
>> +
>> +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
>> +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
>> +MODULE_LICENSE("GPL v2");
>> +
>>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] iio: Add new driver dht11-gpio

[Jonathan Cameron]

On 11/23/13 17:44, Harald Geyer wrote:
> Jonathan Cameron writes:
>> On 10/25/13 12:47, Harald Geyer wrote:
>>> This driver handles DHT11 and DHT22 sensors.
>>>
>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>>
>> Hi Harald,
>>
>> I'm afraid I completely missed your posting of this version 2.
>>
>> Do feel free to pester me if I fail to say anything about a patch
>> for a couple of weeks.
> 
> Well, was going to do so in a few days ... :)
> 
>> Anyhow
>>> ---
>>> changes since v1 (tested on DHT11):
>>> Add dependency on GPIOLIB
>>> Prefix all local preprocessor macros with DHT11_
>>> Rename STARTUP to START_TRANSMISSION
>>> Remove leading zeros
>>> Simplify channel disambiguation
>>> Remove obvious comments
>>> Make whitespace more consistent
>>> Strip unnecessary output and simplify error handling
>>> Fix spelling errors
>>>
>>> The v1 patch has been tested with DHT11 and DHT22 hardware.
>> There are a few redundant bits below that want to be dropped.
>>
>> The only real issue I have otherwise is with the naming.
>> I would expect *-gpio to be a driver providing gpios?
> 
> At least there are 'leds-gpio' and 'w1-gpio' already.
> I was under the impression that drivers providing gpios
> start with 'gpio' like 'gpio-mxs' ...
Looking at the device tree bindings, this is a mess. Some are gpio-*
and some *-gpio.
> 
>> Just go with dht11.  This applies to the bindings as well.
> 
> I'm happy to grab 'dht11' but am reluctant to do so because
> somebody might write a DHT11 driver using some other IO hardware
> then gpio. How would this be called then?
I would hope that they would rework the driver to support both rather
than starting again.

I would definitely go with just dht11
> 
>> Also note that policy is now to cc the device tree list for
>> all patches changing or introducing bindings.
>>
>> I've cc'd them on this reply.
> 
> Thanks.
> 
>> Thanks,
>>
>> Jonathan
>>>
>>> Thanks,
>>> Harald
>>>
>>>  .../bindings/iio/humidity/dht11-gpio.txt           |   14 +
>>>  drivers/iio/Kconfig                                |    1 +
>>>  drivers/iio/Makefile                               |    1 +
>>>  drivers/iio/humidity/Kconfig                       |   15 +
>>>  drivers/iio/humidity/Makefile                      |    5 +
>>>  drivers/iio/humidity/dht11-gpio.c                  |  325 ++++++++++++++++++++
>>>  6 files changed, 361 insertions(+), 0 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>>>  create mode 100644 drivers/iio/humidity/Kconfig
>>>  create mode 100644 drivers/iio/humidity/Makefile
>>>  create mode 100644 drivers/iio/humidity/dht11-gpio.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>>> new file mode 100644
>>> index 0000000..ee8fa04
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11-gpio.txt
>>> @@ -0,0 +1,14 @@
>>> +* DHT11 humidity/temperature sensor (and compatibles like DHT22)
>>> +
>>> +Required properties:
>>> +  - compatible: Should be "dht11-gpio"
>>> +  - gpios: Should specify the GPIO connected to the sensor's data
>>> +    line, see "gpios property" in
>>> +    Documentation/devicetree/bindings/gpio/gpio.txt.
>>> +
>>> +Example:
>>> +
>>> +humidity_sensor {
>>> +	compatible = "dht11-gpio";
>>> +	gpios = <&gpio0 6 0>;
>>> +}
>>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>>> index 90cf0cd..a5ed882 100644
>>> --- a/drivers/iio/Kconfig
>>> +++ b/drivers/iio/Kconfig
>>> @@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
>>>  source "drivers/iio/dac/Kconfig"
>>>  source "drivers/iio/frequency/Kconfig"
>>>  source "drivers/iio/gyro/Kconfig"
>>> +source "drivers/iio/humidity/Kconfig"
>>>  source "drivers/iio/imu/Kconfig"
>>>  source "drivers/iio/light/Kconfig"
>>>  source "drivers/iio/magnetometer/Kconfig"
>>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>>> index bcf7e9e..b3b5096 100644
>>> --- a/drivers/iio/Makefile
>>> +++ b/drivers/iio/Makefile
>>> @@ -18,6 +18,7 @@ obj-y += common/
>>>  obj-y += dac/
>>>  obj-y += gyro/
>>>  obj-y += frequency/
>>> +obj-y += humidity/
>>>  obj-y += imu/
>>>  obj-y += light/
>>>  obj-y += magnetometer/
>>> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
>>> new file mode 100644
>>> index 0000000..bbc44f2
>>> --- /dev/null
>>> +++ b/drivers/iio/humidity/Kconfig
>>> @@ -0,0 +1,15 @@
>>> +#
>>> +# humidity sensor drivers
>>> +#
>>> +menu "Humidity sensors"
>>> +
>>> +config DHT11_GPIO
>> The _GPIO to my mind would normally imply that this as a driver
>> providing GPIOs rather than using them.
> 
> I will change this according to the result of the naming
> discussion.
> 
>>> +	tristate "DHT11 (and compatible sensors) driver"
>>> +	depends on GPIOLIB
>>> +	help
>>> +	  This driver supports reading data via a single interrupt
>>> +	  generating GPIO line. Currently tested are DHT11 and DHT22.
>>> +	  Other sensors should work as well as long as they speak the
>>> +	  same protocol.
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
>>> new file mode 100644
>>> index 0000000..5e8226f
>>> --- /dev/null
>>> +++ b/drivers/iio/humidity/Makefile
>>> @@ -0,0 +1,5 @@
>>> +#
>>> +# Makefile for IIO humidity sensor drivers
>>> +#
>>> +
>>> +obj-$(CONFIG_DHT11_GPIO) += dht11-gpio.o
>>> diff --git a/drivers/iio/humidity/dht11-gpio.c b/drivers/iio/humidity/dht11-gpio.c
>>> new file mode 100644
>>> index 0000000..3bb4d81
>>> --- /dev/null
>>> +++ b/drivers/iio/humidity/dht11-gpio.c
>>> @@ -0,0 +1,325 @@
>> ...
>> Don't bother with this comment.  If anyone wants it they can
>> add support :)
>>> +/* TODO?: Support systems without DT? */
> 
> Ok.
> 
>>> +
>>> +struct dht11_gpio {
>>> +	struct device			*dev;
>>> +
>>> +	int				gpio;
>>> +	int				irq;
>>> +
>>> +	struct completion		completion;
>>> +
>>> +	s64				timestamp;
>>> +	int				temperature;
>>> +	int				humidity;
>>> +
>>> +	/* num_edges: -1 means "no transmission in progress" */
>>> +	int				num_edges;
>>> +	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
>>> +};
>>> +
>> This isn't called anywhere and so should be removed from the driver.
> 
> I will do so, if you think it's more apropriate. I figured that
> commenting it out is like removing but retaining a useful comment.
> Maybe prefix the whole comment with asterix to make it more obvious
> this actually is a comment?
> 
>>> +/*
>>> + * dht11_edges_print: show the data as actually received by the
>>> + *                    driver.
>>> + * This is dead code, keeping it for now just in case somebody needs
>>> + * it for porting the driver to new sensor HW, etc.
>>> + *
>>> +static void dht11_edges_print(struct dht11_gpio *dht11)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 1; i < dht11->num_edges; ++i) {
>>> +		pr_err("dht11-gpio: %d: %lld ns %s\n", i,
>>> +			dht11->edges[i].ts - dht11->edges[i-1].ts,
>>> +			dht11->edges[i-1].value ? "high" : "low");
>>> +	}
>>> +}
>>> +*/
>> ...
> 
> 
> 
>>> +static const struct of_device_id dht11_gpio_dt_ids[] = {
>>> +	{ .compatible = "dht11-gpio", },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dht11_gpio_dt_ids);
>>> +
>>> +static int dht11_gpio_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct device_node *node = dev->of_node;
>>> +	struct dht11_gpio *dht11;
>>> +	struct iio_dev *iio;
>>> +	int ret;
>>> +
>>> +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
>>> +	if (!iio) {
>>> +		dev_err(dev, "Failed to allocate IIO device\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	dht11 = iio_priv(iio);
>>> +	dht11->dev = dev;
>>> +
>>> +	dht11->gpio = ret = of_get_gpio(node, 0);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	dht11->irq = gpio_to_irq(dht11->gpio);
>>> +	if (dht11->irq < 0) {
>>> +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
>>> +		return -EINVAL;
>>> +	}
>>> +	ret = devm_request_irq(dev, dht11->irq, dht11_gpio_handle_irq,
>>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>>> +				pdev->name, iio);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
>>> +	dht11->num_edges = -1;
>>> +
>>> +	platform_set_drvdata(pdev, iio);
>>> +
>>> +	init_completion(&dht11->completion);
>>> +	iio->name = pdev->name;
>>> +	iio->dev.parent = &pdev->dev;
>>> +	iio->info = &dht11_gpio_iio_info;
>>> +	iio->modes = INDIO_DIRECT_MODE;
>>> +	iio->channels = dht11_gpio_chan_spec;
>>> +	iio->num_channels = ARRAY_SIZE(dht11_gpio_chan_spec);
>>> +
>>> +	return iio_device_register(iio);
>>> +}
>>> +
>>
>> Could use the new devm_iio_device_register() call and drop
>> this boilerplate.
> 
> Yes, I'll send a v3 with this after the other issues are sorted
> out.
> 
> Thanks,
> Harald
> 
>>> +static int dht11_gpio_remove(struct platform_device *pdev)
>>> +{
>>> +	struct iio_dev *iio = platform_get_drvdata(pdev);
>>> +
>>> +	iio_device_unregister(iio);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver dht11_gpio_driver = {
>>> +	.driver = {
>>> +		.name	= DRIVER_NAME,
>>> +		.owner	= THIS_MODULE,
>>> +		.of_match_table = dht11_gpio_dt_ids,
>>> +	},
>>> +	.probe  = dht11_gpio_probe,
>>> +	.remove = dht11_gpio_remove,
>>> +};
>>> +
>>> +module_platform_driver(dht11_gpio_driver);
>>> +
>>> +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
>>> +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/2] iio: Add new driver dht11

[Harald Geyer]

This driver handles DHT11 and DHT22 sensors.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
changes since v2 (tested on DHT22):
Remove some comments
Use devm_iio_device_register()
Rename driver: dht11-gpio -> dht11
Move from INIT_COMPLETION to reinit_completion()

changes since v1 (tested on DHT11):
Add dependency on GPIOLIB
Prefix all local preprocessor macros with DHT11_
Rename STARTUP to START_TRANSMISSION
Remove leading zeros
Simplify channel disambiguation
Remove obvious comments
Make whitespace more consistent
Strip unnecessary output and simplify error handling
Fix spelling errors

The v1 patch has been tested with DHT11 and DHT22 hardware.

Thanks,
Harald

 .../devicetree/bindings/iio/humidity/dht11.txt     |   14 +
 drivers/iio/Kconfig                                |    1 +
 drivers/iio/Makefile                               |    1 +
 drivers/iio/humidity/Kconfig                       |   15 +
 drivers/iio/humidity/Makefile                      |    5 +
 drivers/iio/humidity/dht11.c                       |  295 ++++++++++++++++++++
 6 files changed, 331 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11.txt
 create mode 100644 drivers/iio/humidity/Kconfig
 create mode 100644 drivers/iio/humidity/Makefile
 create mode 100644 drivers/iio/humidity/dht11.c

diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11.txt b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
new file mode 100644
index 0000000..ecc24c19
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
@@ -0,0 +1,14 @@
+* DHT11 humidity/temperature sensor (and compatibles like DHT22)
+
+Required properties:
+  - compatible: Should be "dht11"
+  - gpios: Should specify the GPIO connected to the sensor's data
+    line, see "gpios property" in
+    Documentation/devicetree/bindings/gpio/gpio.txt.
+
+Example:
+
+humidity_sensor {
+	compatible = "dht11";
+	gpios = <&gpio0 6 0>;
+}
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 90cf0cd..a5ed882 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
 source "drivers/iio/dac/Kconfig"
 source "drivers/iio/frequency/Kconfig"
 source "drivers/iio/gyro/Kconfig"
+source "drivers/iio/humidity/Kconfig"
 source "drivers/iio/imu/Kconfig"
 source "drivers/iio/light/Kconfig"
 source "drivers/iio/magnetometer/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index bcf7e9e..b3b5096 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -18,6 +18,7 @@ obj-y += common/
 obj-y += dac/
 obj-y += gyro/
 obj-y += frequency/
+obj-y += humidity/
 obj-y += imu/
 obj-y += light/
 obj-y += magnetometer/
diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
new file mode 100644
index 0000000..463c4d9
--- /dev/null
+++ b/drivers/iio/humidity/Kconfig
@@ -0,0 +1,15 @@
+#
+# humidity sensor drivers
+#
+menu "Humidity sensors"
+
+config DHT11
+	tristate "DHT11 (and compatible sensors) driver"
+	depends on GPIOLIB
+	help
+	  This driver supports reading data via a single interrupt
+	  generating GPIO line. Currently tested are DHT11 and DHT22.
+	  Other sensors should work as well as long as they speak the
+	  same protocol.
+
+endmenu
diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
new file mode 100644
index 0000000..d5d36c0
--- /dev/null
+++ b/drivers/iio/humidity/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for IIO humidity sensor drivers
+#
+
+obj-$(CONFIG_DHT11) += dht11.o
diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
new file mode 100644
index 0000000..248f0da
--- /dev/null
+++ b/drivers/iio/humidity/dht11.c
@@ -0,0 +1,295 @@
+/*
+ * DHT11/DHT22 bit banging GPIO driver
+ *
+ * Copyright (c) Harald Geyer <harald@ccbib.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/printk.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/sysfs.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/wait.h>
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
+#include <linux/iio/iio.h>
+
+#define DRIVER_NAME	"dht11"
+
+#define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
+
+#define DHT11_EDGES_PREAMBLE 4
+#define DHT11_BITS_PER_READ 40
+#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
+
+/* Data transmission timing (nano seconds) */
+#define DHT11_START_TRANSMISSION	18  /* ms */
+#define DHT11_SENSOR_RESPONSE	80000
+#define DHT11_START_BIT		50000
+#define DHT11_DATA_BIT_LOW	27000
+#define DHT11_DATA_BIT_HIGH	70000
+
+struct dht11 {
+	struct device			*dev;
+
+	int				gpio;
+	int				irq;
+
+	struct completion		completion;
+
+	s64				timestamp;
+	int				temperature;
+	int				humidity;
+
+	/* num_edges: -1 means "no transmission in progress" */
+	int				num_edges;
+	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
+};
+
+static unsigned char dht11_decode_byte(int *timing, int threshold)
+{
+	unsigned char ret = 0;
+	int i;
+
+	for (i = 0; i < 8; ++i) {
+		ret <<= 1;
+		if (timing[i] >= threshold)
+			++ret;
+	}
+
+	return ret;
+}
+
+static int dht11_decode(struct dht11 *dht11, int offset)
+{
+	int i, t, timing[DHT11_BITS_PER_READ], threshold,
+		timeres = DHT11_SENSOR_RESPONSE;
+	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
+
+	/* Calculate timestamp resolution */
+	for (i = 0; i < dht11->num_edges; ++i) {
+		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
+		if (t > 0 && t < timeres)
+			timeres = t;
+	}
+	if (2*timeres > DHT11_DATA_BIT_HIGH) {
+		pr_err("dht11: timeresolution %d too bad for decoding\n",
+			timeres);
+		return -EIO;
+	}
+	threshold = DHT11_DATA_BIT_HIGH / timeres;
+	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
+		pr_err("dht11: WARNING: decoding ambiguous\n");
+
+	/* scale down with timeres and check validity */
+	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
+		t = dht11->edges[offset + 2*i + 2].ts -
+			dht11->edges[offset + 2*i + 1].ts;
+		if (!dht11->edges[offset + 2*i + 1].value)
+			return -EIO;  /* lost synchronisation */
+		timing[i] = t / timeres;
+	}
+
+	hum_int = dht11_decode_byte(timing, threshold);
+	hum_dec = dht11_decode_byte(&timing[8], threshold);
+	temp_int = dht11_decode_byte(&timing[16], threshold);
+	temp_dec = dht11_decode_byte(&timing[24], threshold);
+	checksum = dht11_decode_byte(&timing[32], threshold);
+
+	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
+		return -EIO;
+
+	dht11->timestamp = iio_get_time_ns();
+	if (hum_int < 20) {  /* DHT22 */
+		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
+					((temp_int & 0x80) ? -100 : 100);
+		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
+	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
+		dht11->temperature = temp_int * 1000;
+		dht11->humidity = hum_int * 1000;
+	} else {
+		dev_err(dht11->dev,
+			"Don't know how to decode data: %d %d %d %d\n",
+			hum_int, hum_dec, temp_int, temp_dec);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int dht11_read_raw(struct iio_dev *iio_dev,
+			const struct iio_chan_spec *chan,
+			int *val, int *val2, long m)
+{
+	struct dht11 *dht11 = iio_priv(iio_dev);
+	int ret;
+
+	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
+		reinit_completion(&dht11->completion);
+
+		dht11->num_edges = 0;
+		ret = gpio_direction_output(dht11->gpio, 0);
+		if (ret)
+			goto err;
+		msleep(DHT11_START_TRANSMISSION);
+		ret = gpio_direction_input(dht11->gpio);
+		if (ret)
+			goto err;
+
+		ret = wait_for_completion_killable_timeout(&dht11->completion,
+								 HZ);
+		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
+			dev_err(&iio_dev->dev,
+					"Only %d signal edges detected\n",
+					dht11->num_edges);
+			ret = -ETIMEDOUT;
+		}
+		if (ret < 0)
+			goto err;
+
+		ret = dht11_decode(dht11,
+				dht11->num_edges == DHT11_EDGES_PER_READ ?
+					DHT11_EDGES_PREAMBLE :
+					DHT11_EDGES_PREAMBLE - 2);
+		if (ret)
+			goto err;
+	}
+
+	ret = IIO_VAL_INT;
+	if (chan->type == IIO_TEMP)
+		*val = dht11->temperature;
+	else if (chan->type == IIO_HUMIDITYRELATIVE)
+		*val = dht11->humidity;
+	else
+		ret = -EINVAL;
+err:
+	dht11->num_edges = -1;
+	return ret;
+}
+
+static const struct iio_info dht11_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= dht11_read_raw,
+};
+
+/*
+ * IRQ handler called on GPIO edges
+*/
+static irqreturn_t dht11_handle_irq(int irq, void *data)
+{
+	struct iio_dev *iio = data;
+	struct dht11 *dht11 = iio_priv(iio);
+
+	/* TODO: Consider making the handler safe for IRQ sharing */
+	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
+		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
+		dht11->edges[dht11->num_edges++].value =
+						gpio_get_value(dht11->gpio);
+
+		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
+			complete(&dht11->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_chan_spec dht11_chan_spec[] = {
+	{ .type = IIO_TEMP, 
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
+	{ .type = IIO_HUMIDITYRELATIVE, 
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), }
+};
+
+static const struct of_device_id dht11_dt_ids[] = {
+	{ .compatible = "dht11", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, dht11_dt_ids);
+
+static int dht11_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct dht11 *dht11;
+	struct iio_dev *iio;
+	int ret;
+
+	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
+	if (!iio) {
+		dev_err(dev, "Failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	dht11 = iio_priv(iio);
+	dht11->dev = dev;
+
+	dht11->gpio = ret = of_get_gpio(node, 0);
+	if (ret < 0)
+		return ret;
+	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
+	if (ret)
+		return ret;
+
+	dht11->irq = gpio_to_irq(dht11->gpio);
+	if (dht11->irq < 0) {
+		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
+		return -EINVAL;
+	}
+	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				pdev->name, iio);
+	if (ret)
+		return ret;
+
+	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
+	dht11->num_edges = -1;
+
+	platform_set_drvdata(pdev, iio);
+
+	init_completion(&dht11->completion);
+	iio->name = pdev->name;
+	iio->dev.parent = &pdev->dev;
+	iio->info = &dht11_iio_info;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->channels = dht11_chan_spec;
+	iio->num_channels = ARRAY_SIZE(dht11_chan_spec);
+
+	return devm_iio_device_register(dev, iio);
+}
+
+static struct platform_driver dht11_driver = {
+	.driver = {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = dht11_dt_ids,
+	},
+	.probe  = dht11_probe,
+};
+
+module_platform_driver(dht11_driver);
+
+MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
+MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
+MODULE_LICENSE("GPL v2");
+
-- 
1.7.2.5

[PATCH v3 1/2] iio: Add support for humidity sensors

[Harald Geyer]

There are already humidity sensors in the hwmon subsystem,
so we use their unit (milli percent) here as well.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---

No changes since v2 except for
KernelVersion: 3.14

No changes since v1.

 Documentation/ABI/testing/sysfs-bus-iio |   13 +++++++++++++
 drivers/iio/industrialio-core.c         |    1 +
 include/linux/iio/types.h               |    1 +
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index b20e829..25aebeb 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -197,6 +197,19 @@ Description:
 		Raw pressure measurement from channel Y. Units after
 		application of scale and offset are kilopascal.
 
+What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_raw
+KernelVersion:	3.14
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Raw humidity measurement of air. Units after application of
+		scale and offset are milli percent.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_input
+KernelVersion:	3.14
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Scaled humidity measurement in milli percent.
+
 What:		/sys/bus/iio/devices/iio:deviceX/in_accel_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_accel_x_offset
 What:		/sys/bus/iio/devices/iio:deviceX/in_accel_y_offset
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 2fe88c1..acc911a 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -69,6 +69,7 @@ static const char * const iio_chan_type_name_spec[] = {
 	[IIO_ALTVOLTAGE] = "altvoltage",
 	[IIO_CCT] = "cct",
 	[IIO_PRESSURE] = "pressure",
+	[IIO_HUMIDITYRELATIVE] = "humidityrelative",
 };
 
 static const char * const iio_modifier_names[] = {
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 4ac928e..084d882 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -29,6 +29,7 @@ enum iio_chan_type {
 	IIO_ALTVOLTAGE,
 	IIO_CCT,
 	IIO_PRESSURE,
+	IIO_HUMIDITYRELATIVE,
 };
 
 enum iio_modifier {
-- 
1.7.2.5

Re: [PATCH v3 1/2] iio: Add support for humidity sensors

[Jonathan Cameron]

On 12/01/13 15:08, Harald Geyer wrote:
> There are already humidity sensors in the hwmon subsystem,
> so we use their unit (milli percent) here as well.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Applied to the togreg branch of iio.git (push out as testing for the
autobuilders to run against)
> ---
> 
> No changes since v2 except for
> KernelVersion: 3.14
> 
> No changes since v1.
> 
>  Documentation/ABI/testing/sysfs-bus-iio |   13 +++++++++++++
>  drivers/iio/industrialio-core.c         |    1 +
>  include/linux/iio/types.h               |    1 +
>  3 files changed, 15 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index b20e829..25aebeb 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -197,6 +197,19 @@ Description:
>  		Raw pressure measurement from channel Y. Units after
>  		application of scale and offset are kilopascal.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_raw
> +KernelVersion:	3.14
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Raw humidity measurement of air. Units after application of
> +		scale and offset are milli percent.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_humidityrelative_input
> +KernelVersion:	3.14
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Scaled humidity measurement in milli percent.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_accel_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_accel_x_offset
>  What:		/sys/bus/iio/devices/iio:deviceX/in_accel_y_offset
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 2fe88c1..acc911a 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -69,6 +69,7 @@ static const char * const iio_chan_type_name_spec[] = {
>  	[IIO_ALTVOLTAGE] = "altvoltage",
>  	[IIO_CCT] = "cct",
>  	[IIO_PRESSURE] = "pressure",
> +	[IIO_HUMIDITYRELATIVE] = "humidityrelative",
>  };
>  
>  static const char * const iio_modifier_names[] = {
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 4ac928e..084d882 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -29,6 +29,7 @@ enum iio_chan_type {
>  	IIO_ALTVOLTAGE,
>  	IIO_CCT,
>  	IIO_PRESSURE,
> +	IIO_HUMIDITYRELATIVE,
>  };
>  
>  enum iio_modifier {
> 

Re: [PATCH v3 2/2] iio: Add new driver dht11

[Jonathan Cameron]

On 12/01/13 15:04, Harald Geyer wrote:
> This driver handles DHT11 and DHT22 sensors.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
Applied with 2 white space at end of line and one two many blank lines at the end of the
file fixed.

Applied to the togreg branch of iio.git (pushed out for now as testing until the
autobuilders are done).

Thanks for your hard work on this 'interesting' device ;)

Peter/Lars, if you want to add an ack/reviewed by then you have a day or two...

Note given it is a trivial binding and has been posted for a lot more than 3 weeks
I'm happy to take this one without and device tree specific feedback...

Jonathan
> ---
> changes since v2 (tested on DHT22):
> Remove some comments
> Use devm_iio_device_register()
> Rename driver: dht11-gpio -> dht11
> Move from INIT_COMPLETION to reinit_completion()
> 
> changes since v1 (tested on DHT11):
> Add dependency on GPIOLIB
> Prefix all local preprocessor macros with DHT11_
> Rename STARTUP to START_TRANSMISSION
> Remove leading zeros
> Simplify channel disambiguation
> Remove obvious comments
> Make whitespace more consistent
> Strip unnecessary output and simplify error handling
> Fix spelling errors
> 
> The v1 patch has been tested with DHT11 and DHT22 hardware.
> 
> Thanks,
> Harald
> 
>  .../devicetree/bindings/iio/humidity/dht11.txt     |   14 +
>  drivers/iio/Kconfig                                |    1 +
>  drivers/iio/Makefile                               |    1 +
>  drivers/iio/humidity/Kconfig                       |   15 +
>  drivers/iio/humidity/Makefile                      |    5 +
>  drivers/iio/humidity/dht11.c                       |  295 ++++++++++++++++++++
>  6 files changed, 331 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/humidity/dht11.txt
>  create mode 100644 drivers/iio/humidity/Kconfig
>  create mode 100644 drivers/iio/humidity/Makefile
>  create mode 100644 drivers/iio/humidity/dht11.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/humidity/dht11.txt b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> new file mode 100644
> index 0000000..ecc24c19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/humidity/dht11.txt
> @@ -0,0 +1,14 @@
> +* DHT11 humidity/temperature sensor (and compatibles like DHT22)
> +
> +Required properties:
> +  - compatible: Should be "dht11"
> +  - gpios: Should specify the GPIO connected to the sensor's data
> +    line, see "gpios property" in
> +    Documentation/devicetree/bindings/gpio/gpio.txt.
> +
> +Example:
> +
> +humidity_sensor {
> +	compatible = "dht11";
> +	gpios = <&gpio0 6 0>;
> +}
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index 90cf0cd..a5ed882 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -65,6 +65,7 @@ source "drivers/iio/common/Kconfig"
>  source "drivers/iio/dac/Kconfig"
>  source "drivers/iio/frequency/Kconfig"
>  source "drivers/iio/gyro/Kconfig"
> +source "drivers/iio/humidity/Kconfig"
>  source "drivers/iio/imu/Kconfig"
>  source "drivers/iio/light/Kconfig"
>  source "drivers/iio/magnetometer/Kconfig"
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index bcf7e9e..b3b5096 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -18,6 +18,7 @@ obj-y += common/
>  obj-y += dac/
>  obj-y += gyro/
>  obj-y += frequency/
> +obj-y += humidity/
>  obj-y += imu/
>  obj-y += light/
>  obj-y += magnetometer/
> diff --git a/drivers/iio/humidity/Kconfig b/drivers/iio/humidity/Kconfig
> new file mode 100644
> index 0000000..463c4d9
> --- /dev/null
> +++ b/drivers/iio/humidity/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# humidity sensor drivers
> +#
> +menu "Humidity sensors"
> +
> +config DHT11
> +	tristate "DHT11 (and compatible sensors) driver"
> +	depends on GPIOLIB
> +	help
> +	  This driver supports reading data via a single interrupt
> +	  generating GPIO line. Currently tested are DHT11 and DHT22.
> +	  Other sensors should work as well as long as they speak the
> +	  same protocol.
> +
> +endmenu
> diff --git a/drivers/iio/humidity/Makefile b/drivers/iio/humidity/Makefile
> new file mode 100644
> index 0000000..d5d36c0
> --- /dev/null
> +++ b/drivers/iio/humidity/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for IIO humidity sensor drivers
> +#
> +
> +obj-$(CONFIG_DHT11) += dht11.o
> diff --git a/drivers/iio/humidity/dht11.c b/drivers/iio/humidity/dht11.c
> new file mode 100644
> index 0000000..248f0da
> --- /dev/null
> +++ b/drivers/iio/humidity/dht11.c
> @@ -0,0 +1,295 @@
> +/*
> + * DHT11/DHT22 bit banging GPIO driver
> + *
> + * Copyright (c) Harald Geyer <harald@ccbib.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/printk.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/wait.h>
> +#include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define DRIVER_NAME	"dht11"
> +
> +#define DHT11_DATA_VALID_TIME	2000000000  /* 2s in ns */
> +
> +#define DHT11_EDGES_PREAMBLE 4
> +#define DHT11_BITS_PER_READ 40
> +#define DHT11_EDGES_PER_READ (2*DHT11_BITS_PER_READ + DHT11_EDGES_PREAMBLE + 1)
> +
> +/* Data transmission timing (nano seconds) */
> +#define DHT11_START_TRANSMISSION	18  /* ms */
> +#define DHT11_SENSOR_RESPONSE	80000
> +#define DHT11_START_BIT		50000
> +#define DHT11_DATA_BIT_LOW	27000
> +#define DHT11_DATA_BIT_HIGH	70000
> +
> +struct dht11 {
> +	struct device			*dev;
> +
> +	int				gpio;
> +	int				irq;
> +
> +	struct completion		completion;
> +
> +	s64				timestamp;
> +	int				temperature;
> +	int				humidity;
> +
> +	/* num_edges: -1 means "no transmission in progress" */
> +	int				num_edges;
> +	struct {s64 ts; int value; }	edges[DHT11_EDGES_PER_READ];
> +};
> +
> +static unsigned char dht11_decode_byte(int *timing, int threshold)
> +{
> +	unsigned char ret = 0;
> +	int i;
> +
> +	for (i = 0; i < 8; ++i) {
> +		ret <<= 1;
> +		if (timing[i] >= threshold)
> +			++ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int dht11_decode(struct dht11 *dht11, int offset)
> +{
> +	int i, t, timing[DHT11_BITS_PER_READ], threshold,
> +		timeres = DHT11_SENSOR_RESPONSE;
> +	unsigned char temp_int, temp_dec, hum_int, hum_dec, checksum;
> +
> +	/* Calculate timestamp resolution */
> +	for (i = 0; i < dht11->num_edges; ++i) {
> +		t = dht11->edges[i].ts - dht11->edges[i-1].ts;
> +		if (t > 0 && t < timeres)
> +			timeres = t;
> +	}
> +	if (2*timeres > DHT11_DATA_BIT_HIGH) {
> +		pr_err("dht11: timeresolution %d too bad for decoding\n",
> +			timeres);
> +		return -EIO;
> +	}
> +	threshold = DHT11_DATA_BIT_HIGH / timeres;
> +	if (DHT11_DATA_BIT_LOW/timeres + 1 >= threshold)
> +		pr_err("dht11: WARNING: decoding ambiguous\n");
> +
> +	/* scale down with timeres and check validity */
> +	for (i = 0; i < DHT11_BITS_PER_READ; ++i) {
> +		t = dht11->edges[offset + 2*i + 2].ts -
> +			dht11->edges[offset + 2*i + 1].ts;
> +		if (!dht11->edges[offset + 2*i + 1].value)
> +			return -EIO;  /* lost synchronisation */
> +		timing[i] = t / timeres;
> +	}
> +
> +	hum_int = dht11_decode_byte(timing, threshold);
> +	hum_dec = dht11_decode_byte(&timing[8], threshold);
> +	temp_int = dht11_decode_byte(&timing[16], threshold);
> +	temp_dec = dht11_decode_byte(&timing[24], threshold);
> +	checksum = dht11_decode_byte(&timing[32], threshold);
> +
> +	if (((hum_int + hum_dec + temp_int + temp_dec) & 0xff) != checksum)
> +		return -EIO;
> +
> +	dht11->timestamp = iio_get_time_ns();
> +	if (hum_int < 20) {  /* DHT22 */
> +		dht11->temperature = (((temp_int & 0x7f) << 8) + temp_dec) *
> +					((temp_int & 0x80) ? -100 : 100);
> +		dht11->humidity = ((hum_int << 8) + hum_dec) * 100;
> +	} else if (temp_dec == 0 && hum_dec == 0) {  /* DHT11 */
> +		dht11->temperature = temp_int * 1000;
> +		dht11->humidity = hum_int * 1000;
> +	} else {
> +		dev_err(dht11->dev,
> +			"Don't know how to decode data: %d %d %d %d\n",
> +			hum_int, hum_dec, temp_int, temp_dec);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dht11_read_raw(struct iio_dev *iio_dev,
> +			const struct iio_chan_spec *chan,
> +			int *val, int *val2, long m)
> +{
> +	struct dht11 *dht11 = iio_priv(iio_dev);
> +	int ret;
> +
> +	if (dht11->timestamp + DHT11_DATA_VALID_TIME < iio_get_time_ns()) {
> +		reinit_completion(&dht11->completion);
> +
> +		dht11->num_edges = 0;
> +		ret = gpio_direction_output(dht11->gpio, 0);
> +		if (ret)
> +			goto err;
> +		msleep(DHT11_START_TRANSMISSION);
> +		ret = gpio_direction_input(dht11->gpio);
> +		if (ret)
> +			goto err;
> +
> +		ret = wait_for_completion_killable_timeout(&dht11->completion,
> +								 HZ);
> +		if (ret == 0 && dht11->num_edges < DHT11_EDGES_PER_READ - 1) {
> +			dev_err(&iio_dev->dev,
> +					"Only %d signal edges detected\n",
> +					dht11->num_edges);
> +			ret = -ETIMEDOUT;
> +		}
> +		if (ret < 0)
> +			goto err;
> +
> +		ret = dht11_decode(dht11,
> +				dht11->num_edges == DHT11_EDGES_PER_READ ?
> +					DHT11_EDGES_PREAMBLE :
> +					DHT11_EDGES_PREAMBLE - 2);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = IIO_VAL_INT;
> +	if (chan->type == IIO_TEMP)
> +		*val = dht11->temperature;
> +	else if (chan->type == IIO_HUMIDITYRELATIVE)
> +		*val = dht11->humidity;
> +	else
> +		ret = -EINVAL;
> +err:
> +	dht11->num_edges = -1;
> +	return ret;
> +}
> +
> +static const struct iio_info dht11_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= dht11_read_raw,
> +};
> +
> +/*
> + * IRQ handler called on GPIO edges
> +*/
> +static irqreturn_t dht11_handle_irq(int irq, void *data)
> +{
> +	struct iio_dev *iio = data;
> +	struct dht11 *dht11 = iio_priv(iio);
> +
> +	/* TODO: Consider making the handler safe for IRQ sharing */
> +	if (dht11->num_edges < DHT11_EDGES_PER_READ && dht11->num_edges >= 0) {
> +		dht11->edges[dht11->num_edges].ts = iio_get_time_ns();
> +		dht11->edges[dht11->num_edges++].value =
> +						gpio_get_value(dht11->gpio);
> +
> +		if (dht11->num_edges >= DHT11_EDGES_PER_READ)
> +			complete(&dht11->completion);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const struct iio_chan_spec dht11_chan_spec[] = {
> +	{ .type = IIO_TEMP, 
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), },
> +	{ .type = IIO_HUMIDITYRELATIVE, 
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), }
> +};
> +
> +static const struct of_device_id dht11_dt_ids[] = {
> +	{ .compatible = "dht11", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, dht11_dt_ids);
> +
> +static int dht11_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct dht11 *dht11;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*dht11));
> +	if (!iio) {
> +		dev_err(dev, "Failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	dht11 = iio_priv(iio);
> +	dht11->dev = dev;
> +
> +	dht11->gpio = ret = of_get_gpio(node, 0);
> +	if (ret < 0)
> +		return ret;
> +	ret = devm_gpio_request_one(dev, dht11->gpio, GPIOF_IN, pdev->name);
> +	if (ret)
> +		return ret;
> +
> +	dht11->irq = gpio_to_irq(dht11->gpio);
> +	if (dht11->irq < 0) {
> +		dev_err(dev, "GPIO %d has no interrupt\n", dht11->gpio);
> +		return -EINVAL;
> +	}
> +	ret = devm_request_irq(dev, dht11->irq, dht11_handle_irq,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				pdev->name, iio);
> +	if (ret)
> +		return ret;
> +
> +	dht11->timestamp = iio_get_time_ns() - DHT11_DATA_VALID_TIME - 1;
> +	dht11->num_edges = -1;
> +
> +	platform_set_drvdata(pdev, iio);
> +
> +	init_completion(&dht11->completion);
> +	iio->name = pdev->name;
> +	iio->dev.parent = &pdev->dev;
> +	iio->info = &dht11_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = dht11_chan_spec;
> +	iio->num_channels = ARRAY_SIZE(dht11_chan_spec);
> +
> +	return devm_iio_device_register(dev, iio);
> +}
> +
> +static struct platform_driver dht11_driver = {
> +	.driver = {
> +		.name	= DRIVER_NAME,
> +		.owner	= THIS_MODULE,
> +		.of_match_table = dht11_dt_ids,
> +	},
> +	.probe  = dht11_probe,
> +};
> +
> +module_platform_driver(dht11_driver);
> +
> +MODULE_AUTHOR("Harald Geyer <harald@ccbib.org>");
> +MODULE_DESCRIPTION("DHT11 humidity/temperature sensor driver");
> +MODULE_LICENSE("GPL v2");
> +
>