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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
>
> 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@xxxxxxxxx>");
>> +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@xxxxxxxxxxxxxxx
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux