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@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' ...
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@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
