Messages in this thread | | | Subject | Re: [PATCH 2/2] iio: distance: add devantech us ranger srf04 | From | Lars-Peter Clausen <> | Date | Sun, 29 Jan 2017 12:43:46 +0100 |
| |
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; > +} [...]
| |