[PATCH 2/2] iio: distance: add devantech us ranger srf04

[PATCH 2/2] iio: distance: add devantech us ranger srf04

[Andreas Klinger]

This patch adds support for the ultrasonic ranger srf04 of devantech.

This device is measuring the distance of objects in a range between 1 cm
and 3 meters and a theoretical resolution of 3 mm.

There are two GPIOs used:
  - trigger: set when the measurement should start
  - echo: set when the ultrasonic wave is sent out and reset when the echo
    is recognized; needs to be an interrupt input

The time between setting and resetting the echo pin is the time the
waveform needed for one round trip. This time is recorded in the interrupt
handler.

The distance is calculated in the read function by using the ultrasonic
speed at 20 degrees celsius which is about 343 m/s.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 MAINTAINERS                    |   6 +
 drivers/iio/proximity/Kconfig  |  11 ++
 drivers/iio/proximity/Makefile |   1 +
 drivers/iio/proximity/srf04.c  | 269 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 287 insertions(+)
 create mode 100644 drivers/iio/proximity/srf04.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fa6af014fd72..ba979cc48fd8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3469,6 +3469,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
 S:	Maintained
 F:	drivers/usb/dwc3/
 
+DEVANTECH SRF ULTRASONIC RANGER IIO DRIVER
+M:	Andreas Klinger <ak@it-klinger.de>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/proximity/srf*.c
+
 DEVICE COREDUMP (DEV_COREDUMP)
 M:	Johannes Berg <johannes@sipsolutions.net>
 L:	linux-kernel@vger.kernel.org
diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
index ef4c73db5b53..0bcbc66eae6e 100644
--- a/drivers/iio/proximity/Kconfig
+++ b/drivers/iio/proximity/Kconfig
@@ -32,6 +32,17 @@ config LIDAR_LITE_V2
 	  To compile this driver as a module, choose M here: the
 	  module will be called pulsedlight-lite-v2
 
+config SRF04
+	tristate "Devantech SRF04 ultrasonic ranger sensor"
+	depends on GPIOLIB
+	help
+	  Say Y here to build a driver for Devantech SRF04 ultrasonic
+	  ranger sensor. This driver can be used to measure the distance
+	  of objects. It is using two GPIOs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called srf04.
+
 config SX9500
 	tristate "SX9500 Semtech proximity sensor"
 	select IIO_BUFFER
diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
index 9aadd9a8ee99..97c046c1dfc5 100644
--- a/drivers/iio/proximity/Makefile
+++ b/drivers/iio/proximity/Makefile
@@ -5,4 +5,5 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AS3935)		+= as3935.o
 obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
+obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c
new file mode 100644
index 000000000000..f458c3d9084b
--- /dev/null
+++ b/drivers/iio/proximity/srf04.c
@@ -0,0 +1,269 @@
+/*
+ * SRF04: ultrasonic sensor for distance measuring by using GPIOs
+ *
+ * Copyright (c) 2017 Andreas Klinger <ak@it-klinger.de>
+ *
+ * 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.
+ *
+ * For details about the device see:
+ * http://www.robot-electronics.co.uk/htm/srf04tech.htm
+ */
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/bitops.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/sched.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+struct srf04_data {
+	struct device		*dev;
+	struct gpio_desc	*gpiod_trig;
+	struct gpio_desc	*gpiod_echo;
+	struct mutex		lock;
+	int			irqnr;
+	ktime_t			ts_rising;
+	ktime_t			ts_falling;
+	struct completion	rising;
+	struct completion	falling;
+};
+
+static irqreturn_t srf04_handle_irq(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct srf04_data *data = iio_priv(indio_dev);
+
+	if (gpiod_get_value(data->gpiod_echo)) {
+		data->ts_rising = ktime_get();
+		complete(&data->rising);
+	} else {
+		data->ts_falling = ktime_get();
+		complete(&data->falling);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int srf04_read(struct srf04_data *data)
+{
+	int ret;
+	ktime_t ktime_dt;
+	u64 dt_ns;
+	u32 time_ns;
+	u32 distance_mm;
+
+	mutex_lock(&data->lock);
+
+	reinit_completion(&data->rising);
+	reinit_completion(&data->falling);
+
+	gpiod_set_value(data->gpiod_trig, 1);
+	udelay(10);
+	gpiod_set_value(data->gpiod_trig, 0);
+
+	mutex_unlock(&data->lock);
+
+	/* it cannot take more than 20 ms */
+	ret = wait_for_completion_killable_timeout(&data->rising, HZ/50);
+	if (ret < 0)
+		return -ETIMEDOUT;
+
+	ret = wait_for_completion_killable_timeout(&data->falling, HZ/50);
+	if (ret < 0)
+		return -ETIMEDOUT;
+
+	ktime_dt = ktime_sub(data->ts_falling, data->ts_rising);
+
+	dt_ns = ktime_to_ns(ktime_dt);
+	/*
+	 * measuring more than 3 meters is beyond the posibilities of
+	 * the sensor
+	 */
+	if (dt_ns > 8750000) {
+		return -EFAULT;
+	}
+	time_ns = dt_ns;
+
+	/*
+	 * the speed as function of the temperature is approximately:
+	 * speed = 331,5 + 0,6 * Temp
+	 *   with Temp in °C
+	 *   and speed in m/s
+	 *
+	 * use 343 m/s as ultrasonic speed at 20 °C here in absence of the
+	 * temperature
+	 *
+	 * therefore:
+	 * distance = time / 10^6 * 343 / 2
+	 *   with time in ns
+	 *   and distance in mm (one way)
+	 *
+	 * because we limit to 3 meters the multiplication with 343 just
+	 * fits into 32 bit
+	 */
+	distance_mm = time_ns * 343 / 2000000;
+
+	dev_info (data->dev, "ns: %llu, dist: %d\n", dt_ns, distance_mm);
+
+	return distance_mm;
+}
+
+static int srf04_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *channel, int *val,
+			    int *val2, long mask)
+{
+	struct srf04_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (channel->type != IIO_DISTANCE)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = srf04_read(data);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		/*
+		 * theoretical maximum resolution is 3 mm
+		 * 1 LSB is 1 mm
+		 */
+		*val = 0;
+		*val2 = 1000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info srf04_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= srf04_read_raw,
+};
+
+static const struct iio_chan_spec srf04_chan_spec[] = {
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate =
+				BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int srf04_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct srf04_data *data = NULL;
+	struct iio_dev *indio_dev;
+	int ret = 0;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct srf04_data));
+	if (!indio_dev) {
+		dev_err(dev, "failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+
+	mutex_init(&data->lock);
+	init_completion(&data->rising);
+	init_completion(&data->falling);
+
+	data->gpiod_trig = devm_gpiod_get(dev, "trig", GPIOD_OUT_LOW);
+	if (IS_ERR(data->gpiod_trig)) {
+		dev_err(dev, "failed to get trig-gpiod: err=%ld\n",
+					PTR_ERR(data->gpiod_trig));
+		return PTR_ERR(data->gpiod_trig);
+	}
+
+	data->gpiod_echo = devm_gpiod_get(dev, "echo", GPIOD_IN);
+	if (IS_ERR(data->gpiod_echo)) {
+		dev_err(dev, "failed to get echo-gpiod: err=%ld\n",
+					PTR_ERR(data->gpiod_echo));
+		return PTR_ERR(data->gpiod_echo);
+	}
+
+	if (gpiod_cansleep(data->gpiod_echo)) {
+		dev_err(data->dev, "cansleep-GPIOs not supported\n");
+		return -ENODEV;
+	}
+
+	data->irqnr = gpiod_to_irq(data->gpiod_echo);
+	if (data->irqnr < 0) {
+		dev_err(data->dev, "gpiod_to_irq: %d\n", ret);
+		return ret;
+	}
+
+	ret = request_irq(data->irqnr, srf04_handle_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+			pdev->name, indio_dev);
+	if (ret < 0) {
+		dev_err(data->dev, "request_irq: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	indio_dev->name = "srf04";
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->info = &srf04_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = srf04_chan_spec;
+	indio_dev->num_channels = ARRAY_SIZE(srf04_chan_spec);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int srf04_remove(struct platform_device *pdev)
+{
+	struct srf04_data *data = NULL;
+	struct iio_dev *indio_dev;
+
+	indio_dev = platform_get_drvdata(pdev);
+	data = iio_priv(indio_dev);
+
+	free_irq(data->irqnr, indio_dev);
+
+	return 0;
+}
+
+static const struct of_device_id of_srf04_match[] = {
+	{ .compatible = "devantech,srf04", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_srf04_match);
+
+static struct platform_driver srf04_driver = {
+	.probe		= srf04_probe,
+	.remove		= srf04_remove,
+	.driver		= {
+		.name		= "srf04-gpio",
+		.of_match_table	= of_srf04_match,
+	},
+};
+
+module_platform_driver(srf04_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("SRF04 ultrasonic sensor for distance measuring using GPIOs");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:srf04");
-- 
2.1.4

Re: [PATCH 2/2] iio: distance: add devantech us ranger srf04

[Peter Meerwald-Stadler]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 11000 bytes --]


> This patch adds support for the ultrasonic ranger srf04 of devantech.

comments below
 
> This device is measuring the distance of objects in a range between 1 cm
> and 3 meters and a theoretical resolution of 3 mm.
> 
> There are two GPIOs used:
>   - trigger: set when the measurement should start
>   - echo: set when the ultrasonic wave is sent out and reset when the echo
>     is recognized; needs to be an interrupt input
> 
> The time between setting and resetting the echo pin is the time the
> waveform needed for one round trip. This time is recorded in the interrupt
> handler.
> 
> The distance is calculated in the read function by using the ultrasonic
> speed at 20 degrees celsius which is about 343 m/s.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  MAINTAINERS                    |   6 +
>  drivers/iio/proximity/Kconfig  |  11 ++
>  drivers/iio/proximity/Makefile |   1 +
>  drivers/iio/proximity/srf04.c  | 269 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 287 insertions(+)
>  create mode 100644 drivers/iio/proximity/srf04.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fa6af014fd72..ba979cc48fd8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3469,6 +3469,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
>  S:	Maintained
>  F:	drivers/usb/dwc3/
>  
> +DEVANTECH SRF ULTRASONIC RANGER IIO DRIVER
> +M:	Andreas Klinger <ak@it-klinger.de>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	drivers/iio/proximity/srf*.c
> +
>  DEVICE COREDUMP (DEV_COREDUMP)
>  M:	Johannes Berg <johannes@sipsolutions.net>
>  L:	linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
> index ef4c73db5b53..0bcbc66eae6e 100644
> --- a/drivers/iio/proximity/Kconfig
> +++ b/drivers/iio/proximity/Kconfig
> @@ -32,6 +32,17 @@ config LIDAR_LITE_V2
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called pulsedlight-lite-v2
>  
> +config SRF04
> +	tristate "Devantech SRF04 ultrasonic ranger sensor"
> +	depends on GPIOLIB
> +	help
> +	  Say Y here to build a driver for Devantech SRF04 ultrasonic
> +	  ranger sensor. This driver can be used to measure the distance
> +	  of objects. It is using two GPIOs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called srf04.
> +
>  config SX9500
>  	tristate "SX9500 Semtech proximity sensor"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
> index 9aadd9a8ee99..97c046c1dfc5 100644
> --- a/drivers/iio/proximity/Makefile
> +++ b/drivers/iio/proximity/Makefile
> @@ -5,4 +5,5 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AS3935)		+= as3935.o
>  obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
> +obj-$(CONFIG_SRF04)		+= srf04.o
>  obj-$(CONFIG_SX9500)		+= sx9500.o
> diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c
> new file mode 100644
> index 000000000000..f458c3d9084b
> --- /dev/null
> +++ b/drivers/iio/proximity/srf04.c
> @@ -0,0 +1,269 @@
> +/*
> + * SRF04: ultrasonic sensor for distance measuring by using GPIOs
> + *
> + * Copyright (c) 2017 Andreas Klinger <ak@it-klinger.de>
> + *
> + * 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.
> + *
> + * For details about the device see:
> + * http://www.robot-electronics.co.uk/htm/srf04tech.htm
> + */
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/sched.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +struct srf04_data {
> +	struct device		*dev;
> +	struct gpio_desc	*gpiod_trig;
> +	struct gpio_desc	*gpiod_echo;
> +	struct mutex		lock;
> +	int			irqnr;
> +	ktime_t			ts_rising;
> +	ktime_t			ts_falling;
> +	struct completion	rising;
> +	struct completion	falling;
> +};
> +
> +static irqreturn_t srf04_handle_irq(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +	struct srf04_data *data = iio_priv(indio_dev);
> +
> +	if (gpiod_get_value(data->gpiod_echo)) {
> +		data->ts_rising = ktime_get();
> +		complete(&data->rising);
> +	} else {
> +		data->ts_falling = ktime_get();
> +		complete(&data->falling);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int srf04_read(struct srf04_data *data)
> +{
> +	int ret;
> +	ktime_t ktime_dt;
> +	u64 dt_ns;
> +	u32 time_ns;
> +	u32 distance_mm;
> +
> +	mutex_lock(&data->lock);
> +
> +	reinit_completion(&data->rising);
> +	reinit_completion(&data->falling);
> +
> +	gpiod_set_value(data->gpiod_trig, 1);
> +	udelay(10);
> +	gpiod_set_value(data->gpiod_trig, 0);
> +
> +	mutex_unlock(&data->lock);
> +
> +	/* it cannot take more than 20 ms */
> +	ret = wait_for_completion_killable_timeout(&data->rising, HZ/50);
> +	if (ret < 0)
> +		return -ETIMEDOUT;
> +
> +	ret = wait_for_completion_killable_timeout(&data->falling, HZ/50);
> +	if (ret < 0)
> +		return -ETIMEDOUT;
> +
> +	ktime_dt = ktime_sub(data->ts_falling, data->ts_rising);
> +
> +	dt_ns = ktime_to_ns(ktime_dt);
> +	/*
> +	 * measuring more than 3 meters is beyond the posibilities of

possibilities, maybe better capabilities

> +	 * the sensor
> +	 */
> +	if (dt_ns > 8750000) {

magic value?
parenthesis {} not needed

> +		return -EFAULT;
> +	}
> +	time_ns = dt_ns;
> +
> +	/*
> +	 * the speed as function of the temperature is approximately:
> +	 * speed = 331,5 + 0,6 * Temp
> +	 *   with Temp in °C
> +	 *   and speed in m/s
> +	 *
> +	 * use 343 m/s as ultrasonic speed at 20 °C here in absence of the
> +	 * temperature
> +	 *
> +	 * therefore:
> +	 * distance = time / 10^6 * 343 / 2
> +	 *   with time in ns
> +	 *   and distance in mm (one way)
> +	 *
> +	 * because we limit to 3 meters the multiplication with 343 just
> +	 * fits into 32 bit
> +	 */
> +	distance_mm = time_ns * 343 / 2000000;
> +
> +	dev_info (data->dev, "ns: %llu, dist: %d\n", dt_ns, distance_mm);

this should be dbg I think; and no space after dev_info

> +
> +	return distance_mm;
> +}
> +
> +static int srf04_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +	struct srf04_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (channel->type != IIO_DISTANCE)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = srf04_read(data);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		/*
> +		 * theoretical maximum resolution is 3 mm
> +		 * 1 LSB is 1 mm
> +		 */
> +		*val = 0;
> +		*val2 = 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info srf04_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= srf04_read_raw,
> +};
> +
> +static const struct iio_chan_spec srf04_chan_spec[] = {
> +	{
> +		.type = IIO_DISTANCE,
> +		.info_mask_separate =
> +				BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +static int srf04_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct srf04_data *data = NULL;
> +	struct iio_dev *indio_dev;
> +	int ret = 0;

initialization not needed

> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct srf04_data));
> +	if (!indio_dev) {
> +		dev_err(dev, "failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +
> +	mutex_init(&data->lock);
> +	init_completion(&data->rising);
> +	init_completion(&data->falling);
> +
> +	data->gpiod_trig = devm_gpiod_get(dev, "trig", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->gpiod_trig)) {
> +		dev_err(dev, "failed to get trig-gpiod: err=%ld\n",
> +					PTR_ERR(data->gpiod_trig));
> +		return PTR_ERR(data->gpiod_trig);
> +	}
> +
> +	data->gpiod_echo = devm_gpiod_get(dev, "echo", GPIOD_IN);
> +	if (IS_ERR(data->gpiod_echo)) {
> +		dev_err(dev, "failed to get echo-gpiod: err=%ld\n",
> +					PTR_ERR(data->gpiod_echo));
> +		return PTR_ERR(data->gpiod_echo);
> +	}
> +
> +	if (gpiod_cansleep(data->gpiod_echo)) {
> +		dev_err(data->dev, "cansleep-GPIOs not supported\n");
> +		return -ENODEV;
> +	}
> +
> +	data->irqnr = gpiod_to_irq(data->gpiod_echo);
> +	if (data->irqnr < 0) {
> +		dev_err(data->dev, "gpiod_to_irq: %d\n", ret);

ret? should be data->irqnr?

> +		return ret;
> +	}
> +
> +	ret = request_irq(data->irqnr, srf04_handle_irq,

could use devm_

> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +			pdev->name, indio_dev);
> +	if (ret < 0) {
> +		dev_err(data->dev, "request_irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = "srf04";
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->info = &srf04_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = srf04_chan_spec;
> +	indio_dev->num_channels = ARRAY_SIZE(srf04_chan_spec);
> +
> +	return devm_iio_device_register(dev, indio_dev);

you can't use devm_iio_device_register() if you need/have a _remove()

> +}
> +
> +static int srf04_remove(struct platform_device *pdev)
> +{

struct iio_dev *indio_dev = platform_get_drvdata(pdev);
struct srf04_data *data = iio_priv(indio_dev); // (but not needed anymore)

> +	struct srf04_data *data = NULL;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = platform_get_drvdata(pdev);
> +	data = iio_priv(indio_dev);
> +
> +	free_irq(data->irqnr, indio_dev);

I suggest to use in _probe()
devm_request_irq()
iio_device_register()

and in _remove() just
iio_device_unregister()

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_srf04_match[] = {
> +	{ .compatible = "devantech,srf04", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, of_srf04_match);
> +
> +static struct platform_driver srf04_driver = {
> +	.probe		= srf04_probe,
> +	.remove		= srf04_remove,
> +	.driver		= {
> +		.name		= "srf04-gpio",
> +		.of_match_table	= of_srf04_match,
> +	},
> +};
> +
> +module_platform_driver(srf04_driver);
> +
> +MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
> +MODULE_DESCRIPTION("SRF04 ultrasonic sensor for distance measuring using GPIOs");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:srf04");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 2/2] iio: distance: add devantech us ranger srf04

[Lars-Peter Clausen]

On 01/29/2017 12:58 AM, Andreas Klinger wrote:
> This patch adds support for the ultrasonic ranger srf04 of devantech.

Thanks for the patch. Looks mostly good, a few small comments inline.

> diff --git a/drivers/iio/proximity/srf04.c b/drivers/iio/proximity/srf04.c
> new file mode 100644
> index 000000000000..f458c3d9084b
> --- /dev/null
> +++ b/drivers/iio/proximity/srf04.c
> @@ -0,0 +1,269 @@
[...]
> +#include <linux/err.h>
> +#include <linux/gpio.h>

Your driver is only a GPIO consumer, so the above include should not be
necessary.

> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>

As far as I can see nothing from bitops.h is used in this driver.

> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/sched.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
[...]	
> +static int srf04_read(struct srf04_data *data)
> +{
> +	int ret;
> +	ktime_t ktime_dt;
> +	u64 dt_ns;
> +	u32 time_ns;
> +	u32 distance_mm;
> +
> +	mutex_lock(&data->lock);

Maybe a stupid question, but what does the lock protect?

> +
> +	reinit_completion(&data->rising);
> +	reinit_completion(&data->falling);
> +
> +	gpiod_set_value(data->gpiod_trig, 1);
> +	udelay(10);
> +	gpiod_set_value(data->gpiod_trig, 0);
> +
> +	mutex_unlock(&data->lock);
> +
> +	/* it cannot take more than 20 ms */
> +	ret = wait_for_completion_killable_timeout(&data->rising, HZ/50);
> +	if (ret < 0)

In case of a timeout wait_for_... will return 0. In case of an interrupt
(kill event) a negative value. In the later case we should typically
propagate the error rather than replacing it.

> +		return -ETIMEDOUT;
> +
> +	ret = wait_for_completion_killable_timeout(&data->falling, HZ/50);
> +	if (ret < 0)
> +		return -ETIMEDOUT;
> +
> +	ktime_dt = ktime_sub(data->ts_falling, data->ts_rising);
> +
> +	dt_ns = ktime_to_ns(ktime_dt);
> +	/*
> +	 * measuring more than 3 meters is beyond the posibilities of
> +	 * the sensor
> +	 */
> +	if (dt_ns > 8750000) {
> +		return -EFAULT;

EFAULT means that "An invalid user space address was specified for an
argument". EIO is usually used to indicate that there was a communication
issue with the device.

> +	}
> +	time_ns = dt_ns;
> +
> +	/*
> +	 * the speed as function of the temperature is approximately:
> +	 * speed = 331,5 + 0,6 * Temp
> +	 *   with Temp in °C
> +	 *   and speed in m/s
> +	 *
> +	 * use 343 m/s as ultrasonic speed at 20 °C here in absence of the
> +	 * temperature
> +	 *
> +	 * therefore:
> +	 * distance = time / 10^6 * 343 / 2
> +	 *   with time in ns
> +	 *   and distance in mm (one way)
> +	 *
> +	 * because we limit to 3 meters the multiplication with 343 just
> +	 * fits into 32 bit
> +	 */
> +	distance_mm = time_ns * 343 / 2000000;
> +
> +	dev_info (data->dev, "ns: %llu, dist: %d\n", dt_ns, distance_mm);

dev_dbg probably.

> +
> +	return distance_mm;
> +}
> +
> +static int srf04_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)

I know lots of drivers use the name 'mask' here, but that is a relict from a
long long time ago and the implementation was changed and the old name it is
not really appropriate anymore. The recommendation is to use 'info' for new
drivers.

> +{
[...]
> +static int srf04_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct srf04_data *data = NULL;

The NULL initialization is not really necessary. The variable is never used
until it is initialized again a few lines below. The compiler will just
remove this.

> +	struct iio_dev *indio_dev;
> +	int ret = 0;

Same here.

> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(struct srf04_data));
> +	if (!indio_dev) {
> +		dev_err(dev, "failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +
> +	mutex_init(&data->lock);
> +	init_completion(&data->rising);
> +	init_completion(&data->falling);
> +
> +	data->gpiod_trig = devm_gpiod_get(dev, "trig", GPIOD_OUT_LOW);
> +	if (IS_ERR(data->gpiod_trig)) {
> +		dev_err(dev, "failed to get trig-gpiod: err=%ld\n",
> +					PTR_ERR(data->gpiod_trig));
> +		return PTR_ERR(data->gpiod_trig);
> +	}
> +
> +	data->gpiod_echo = devm_gpiod_get(dev, "echo", GPIOD_IN);
> +	if (IS_ERR(data->gpiod_echo)) {
> +		dev_err(dev, "failed to get echo-gpiod: err=%ld\n",
> +					PTR_ERR(data->gpiod_echo));
> +		return PTR_ERR(data->gpiod_echo);
> +	}
> +
> +	if (gpiod_cansleep(data->gpiod_echo)) {
> +		dev_err(data->dev, "cansleep-GPIOs not supported\n");
> +		return -ENODEV;
> +	}
> +
> +	data->irqnr = gpiod_to_irq(data->gpiod_echo);
> +	if (data->irqnr < 0) {
> +		dev_err(data->dev, "gpiod_to_irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = request_irq(data->irqnr, srf04_handle_irq,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +			pdev->name, indio_dev);
> +	if (ret < 0) {
> +		dev_err(data->dev, "request_irq: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +
> +	indio_dev->name = "srf04";
> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->info = &srf04_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = srf04_chans_spec;
> +	indio_dev->num_channels = ARRAY_SIZE(srf04_chan_spec);
> +
> +	return devm_iio_device_register(dev, indio_dev);

If this returns an error the interrupt needs to be freed.

> +}
> +
> +static int srf04_remove(struct platform_device *pdev)
> +{
> +	struct srf04_data *data = NULL;
> +	struct iio_dev *indio_dev;
> +
> +	indio_dev = platform_get_drvdata(pdev);
> +	data = iio_priv(indio_dev);

I'd write this as


	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
	struct srf04_data *data = iio_priv(indio_dev)

Looks a bit cleaner in my opinion.

> +
> +	free_irq(data->irqnr, indio_dev);
> +
> +	return 0;
> +}
[...]