Re: [PATCH 2/4] iio: chemical: add driver for dsm501/ppd42ns particle sensors

[Jonathan Cameron]

On 06/02/17 06:52, Matt Ranostay wrote:
> 
>> On Feb 5, 2017, at 08:24, Tomasz Duszynski <tduszyns@xxxxxxxxx> wrote:
>>
>> Thanks for review!
>>
>>> On Sun, Feb 05, 2017 at 04:19:47PM +0100, Peter Meerwald-Stadler wrote:
>>>
>>>> This patch adds support for dsm501 and ppd42ns particle sensors.
>>>
>>> quick comments below
>>> G
>>>> Both sensors work on the same principle. Heater (resistor) heats up air
>>>> in sensor chamber which induces upward flow. Particles convect up through
>>>> a light beam provided by internal infra-red LED. Light scattered by
>>>> particles is picked by photodiode and internal electronics outputs PWM
>>>> signal.
>>>>
>>>> Measuring low time occupancy of the signal during 30s measurement period
>>>> particles number in cubic meter can be computed.
>>>>
> 
> This a SI unit?
> 
> Also this maybe could be better off in drivers/iio/light... as much I
> like to have other people in drivers/iio/chemical ð
Hmm. It's an odd one certainly.  Bit like the pulse sensors that also
use something derived from light in a non trivial way.

I'm not sure, but my gut feeling is to go with use case and short
of having a new particle sensing category I think chemical is
probably just about the best place for this one

Anyhow, interesting device.  Another one where it sort of feels
like it ought to be tied to dedicated pulse counter hardware
but I guess it's slow enough that this works.

Jonathan
>>>> Signed-off-by: Tomasz Duszynski <tduszyns@xxxxxxxxx>
>>>> ---
>>>> drivers/iio/chemical/Kconfig  |  10 ++
>>>> drivers/iio/chemical/Makefile |   1 +
>>>> drivers/iio/chemical/dsm501.c | 231 ++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 242 insertions(+)
>>>> create mode 100644 drivers/iio/chemical/dsm501.c
>>>>
>>>> diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
>>>> index cea7f9857a1f..986d612aa77f 100644
>>>> --- a/drivers/iio/chemical/Kconfig
>>>> +++ b/drivers/iio/chemical/Kconfig
>>>> @@ -21,6 +21,16 @@ config ATLAS_PH_SENSOR
>>>>     To compile this driver as module, choose M here: the
>>>>     module will be called atlas-ph-sensor.
>>>>
>>>> +config DSM501
>>>> +    tristate "Samyoung DSM501 particle sensor"
>>>> +    depends on GPIOLIB
>>>> +    help
>>>> +     Say yes here to build support for the Samyoung DSM501
>>>> +     particle sensor.
>>>> +
>>>> +     To compile this driver as a module, choose M here: the module
>>>> +     will be called dsm501.
>>>> +
>>>> config IAQCORE
>>>>    tristate "AMS iAQ-Core VOC sensors"
>>>>    depends on I2C
>>>> diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
>>>> index b02202b41289..76f50ff8ba7d 100644
>>>> --- a/drivers/iio/chemical/Makefile
>>>> +++ b/drivers/iio/chemical/Makefile
>>>> @@ -4,5 +4,6 @@
>>>>
>>>> # When adding new entries keep the list in alphabetical order
>>>> obj-$(CONFIG_ATLAS_PH_SENSOR)    += atlas-ph-sensor.o
>>>> +obj-$(CONFIG_DSM501)        += dsm501.o
>>>> obj-$(CONFIG_IAQCORE)        += ams-iaq-core.o
>>>> obj-$(CONFIG_VZ89X)        += vz89x.o
>>>> diff --git a/drivers/iio/chemical/dsm501.c b/drivers/iio/chemical/dsm501.c
>>>> new file mode 100644
>>>> index 000000000000..013f6b3bfd48
>>>> --- /dev/null
>>>> +++ b/drivers/iio/chemical/dsm501.c
>>>> @@ -0,0 +1,231 @@
>>>> +/*
>>>> + * Samyoung DSM501 particle sensor driver
>>>> + *
>>>> + * Copyright (C) 2017 Tomasz Duszynski <tduszyns@xxxxxxxxx>
>>>> + *
>>>> + * Datasheets:
>>>> + *  http://www.samyoungsnc.com/products/3-1%20Specification%20DSM501.pdf
>>>> + *  http://wiki.timelab.org/images/f/f9/PPD42NS.pdf
>>>> + *
>>>> + * 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/delay.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/timekeeping.h>
>>>> +
>>>> +#define DSM501_DRV_NAME "dsm501"
>>>> +#define    DSM501_IRQ_NAME "dsm501_irq"
>>>
>>> replace tab by space
>> Ack.
>>>
>>>> +
>>>> +#define DSM501_DEFAULT_MEASUREMENT_TIME 30 /* seconds */
>>>> +
>>>> +struct dsm501_data {
>>>> +    ktime_t ts;
>>>> +    ktime_t low_time;
>>>> +    ktime_t meas_time;
>>>> +
>>>> +    int irq;
>>>> +    struct gpio_desc *gpio;
>>>> +
>>>> +    struct mutex lock;
>>>> +
>>>> +    int (*number_concentration)(struct dsm501_data *data);
>>>> +};
>>>> +
>>>> +/*
>>>> + * Series of data points in Fig. 8-3 (Low Ratio vs Particle)
>>>> + * can be approximated by following polynomials:
>>>> + *
>>>> + * p(r) = 0 (undefined) for r < 4
>>>> + * p(r) = 2353564.2r - 4373814.7 for 4 <= r < 20
>>>> + * p(r) = 4788112.4r - 53581390 for r >= 20
>>>> + *
>>>> + * Note: Result is in pcs/m3. To convert to pcs/0.01cf multiply
>>>> + *     by 0.0002831685.
>>>
>>> is cf needed?
>> No. I've put that sidenote so that others looking at figures in
>> datahseets won't scratch their heads why units here are pcs/m3 and
>> pcs/0.01cf there. Anyway I am happy to drop that part.
>>>
>>>> + */
>>>> +static int dsm501_number_concentartion(struct dsm501_data *data)
>>>> +{
>>>> +    s64 retval = 0, r = div64_s64(ktime_to_ns(data->low_time) * 100,
>>>> +                      ktime_to_ns(data->meas_time));
>>>> +
>>>> +    if (r >= 4 && r < 20)
>>>> +        retval = 23535642 * r - 43738147;
>>>> +    else if (r >= 20)
>>>> +        retval = 47881124 * r - 535813900;
>>>> +
>>>> +    return div_s64(retval, 10);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Series of data points in Fig. 2 (Lo Pulse Occupancy Time vs Concentration)
>>>> + * can be approximated by following polynomial:
>>>> + *
>>>> + * p(r) = 3844.2r^3 - 16201.3r^2 + 1848746.1r + 52497.2
>>>> + *
>>>> + * Note: Result is in pcs/m3. To convert to pcs/0.01cf multiply
>>>> + *     by 0.0002831685.
>>>> + */
>>>> +static int ppd42ns_number_concentration(struct dsm501_data *data)
>>>> +{
>>>> +    s64 retval, r3, r2, r = div64_s64(ktime_to_ns(data->low_time) * 100,
>>>> +                      ktime_to_ns(data->meas_time));
>>>> +
>>>> +    r2 = r * r;
>>>> +    r3 = r2 * r;
>>>> +
>>>> +    retval = 38442 * r3;
>>>> +    retval -= 162013 * r2;
>>>> +    retval += 18487461 * r;
>>>> +    retval += 524972;
>>>> +
>>>> +    return div_s64(retval, 10);
>>>> +}
>>>> +
>>>> +static irqreturn_t dsm501_irq(int irq, void *dev_id)
>>>> +{
>>>> +    struct dsm501_data *data = iio_priv(dev_id);
>>>> +    int val = gpiod_get_value(data->gpio);
>>>> +    ktime_t dt, ts = ktime_get();
>>>> +
>>>> +    if (ktime_to_ns(data->ts) == 0) {
>>>> +        data->ts = ts;
>>>> +        data->low_time = ktime_set(0, 0);
>>>> +    }
>>>> +
>>>> +    if (val) {
>>>> +        dt = ktime_sub(ts, data->ts);
>>>> +        data->low_time = ktime_add(data->low_time, dt);
>>>> +    } else {
>>>> +        data->ts = ts;
>>>> +    }
>>>
>>> {} needed?
>> According to CodingStyle yes. {} could be dropped if there was single
>> statement under if (val) { ...
>>>
>>>> +
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int dsm501_read_raw(struct iio_dev *indio_dev,
>>>> +               struct iio_chan_spec const *chan, int *val,
>>>> +               int *val2, long mask)
>>>> +{
>>>> +    struct dsm501_data *data = iio_priv(indio_dev);
>>>> +    struct device *dev = indio_dev->dev.parent;
>>>> +    unsigned long irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;
>>>> +    int ret;
>>>> +
>>>> +
>>>> +    switch (mask) {
>>>> +    case IIO_CHAN_INFO_PROCESSED:
>>>> +        mutex_lock(&data->lock);
>>>> +        data->ts = ktime_set(0, 0);
>>>> +
>>>> +        ret = devm_request_irq(dev, data->irq, dsm501_irq, irqflags,
>>>> +                       DSM501_IRQ_NAME, indio_dev);
>>>
>>> why devm_()? if the irq is explicitly freed below?
>>> requesting the irq for every measurement seems weird even if it only
>>> happens every 30s
>> Ack.
>>>
>>>> +        if (ret) {
>>>> +            dev_err(dev, "Failed to request interrupt %d\n", data->irq);
>>>> +            mutex_unlock(&data->lock);
>>>> +            return ret;
>>>> +        }
>>>> +
>>>> +        msleep_interruptible(ktime_to_ms(data->meas_time));
>>>> +        devm_free_irq(dev, data->irq, indio_dev);
>>>> +
>>>> +        *val = data->number_concentration(data);
>>>> +        mutex_unlock(&data->lock);
>>>> +
>>>> +        return IIO_VAL_INT;
>>>> +    default:
>>>> +        return -EINVAL;
>>>> +    }
>>>> +}
>>>> +
>>>> +static const struct iio_info dsm501_info = {
>>>> +    .driver_module    = THIS_MODULE,
>>>> +    .read_raw = dsm501_read_raw,
>>>> +};
>>>> +
>>>> +static const struct iio_chan_spec dsm501_channels[] = {
>>>> +    {
>>>> +        .type = IIO_NUMBERCONCENTRATION,
>>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
>>>> +    },
>>>> +};
>>>> +
>>>> +static int dsm501_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct dsm501_data *data;
>>>> +    struct iio_dev *indio_dev;
>>>> +    struct device *dev = &pdev->dev;
>>>> +
>>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*data));
>>>> +    if (!indio_dev)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    data = iio_priv(indio_dev);
>>>> +    platform_set_drvdata(pdev, indio_dev);
>>>> +
>>>> +    data->gpio = devm_gpiod_get_index(dev, NULL, 0, GPIOD_IN);
>>>> +    if (IS_ERR(data->gpio)) {
>>>> +        dev_err(dev, "Failed to get GPIO\n");
>>>> +        return PTR_ERR(data->gpio);
>>>> +    }
>>>> +
>>>> +    data->irq = gpiod_to_irq(data->gpio);
>>>> +    if (data->irq < 0) {
>>>> +        dev_err(dev, "GPIO has no interrupt\n");
>>>> +        return data->irq;
>>>> +    }
>>>> +
>>>> +    data->meas_time = ktime_set(DSM501_DEFAULT_MEASUREMENT_TIME, 0);
>>>> +    data->number_concentration = of_device_get_match_data(dev);
>>>> +    mutex_init(&data->lock);
>>>> +
>>>> +    indio_dev->name = DSM501_DRV_NAME;
>>>> +    indio_dev->dev.parent = dev;
>>>> +    indio_dev->info = &dsm501_info;
>>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +    indio_dev->channels = dsm501_channels;
>>>> +    indio_dev->num_channels = ARRAY_SIZE(dsm501_channels);
>>>> +
>>>> +    return devm_iio_device_register(&pdev->dev, indio_dev);
>>>> +}
>>>> +
>>>> +static const struct of_device_id dsm501_id[] = {
>>>> +    {
>>>> +        .compatible = "samyoung,dsm501",
>>>> +        .data = dsm501_number_concentartion,
>>>
>>> type: concentration
>> Are you refering to function naming here?  *_number_concentration was named
>> after returned physical quantity.
>>>
>>>> +    },
>>>> +    {
>>>> +        .compatible = "shinyei,ppd42ns",
>>>> +        .data = ppd42ns_number_concentration,
>>>> +    },
>>>> +    { }
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, dsm501_id);
>>>> +
>>>> +static struct platform_driver dsm501_driver = {
>>>> +    .driver    = {
>>>> +        .name = DSM501_DRV_NAME,
>>>> +        .of_match_table = of_match_ptr(dsm501_id)
>>>> +    },
>>>> +    .probe = dsm501_probe
>>>> +};
>>>> +module_platform_driver(dsm501_driver);
>>>> +
>>>> +MODULE_AUTHOR("Tomasz Duszynski <tduszyns@xxxxxxxxx>");
>>>> +MODULE_DESCRIPTION("Samyoung DSM501 particle sensor driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>> --
>>>
>>> Peter Meerwald-Stadler
>>> +43-664-2444418 (mobile)
>> --
>> 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
>