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

[Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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-95f8Dae0BrPYtjvyW6yDsg@public.gmane.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-95f8Dae0BrPYtjvyW6yDsg@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html