[PATCH] HC-SR04 ultrasonic ranger IIO driver

[Valdis.Kletnieks at vt.edu]

On Tue, 31 May 2016 23:05:57 +0200, johannes at johannesthoma.com said:

Looks good overall, far from the ugliest driver I've seen.  I spotted one
locking bug, and a few small typos etc, noted inline...

> From: Johannes Thoma <johannes at johannesthoma.com>
>
> The HC-SR04 is an ultrasonic distance sensor attached to two GPIO
> pins. The driver based on Industrial I/O (iio) subsystem and is

        The driver is based on the Industrial...


> + * To configure a device do a
> + *
> + *    mkdir /config/iio/triggers/hc-sr04/sensor0
> + *
> + * (you need to mount configfs to /config first)

Most distros seem to be using /sys/kernel/config as the mount point for this...


> + * Then you can measure distance with:
> + *
> + *    cat /sys/devices/trigger0/measure

What are the units of the returned value? Inches? Hundredths of an inch?
inches.hundredths?   Other?

(Yes, I looked at the datasheet.. and your driver source is more helpful
than the sheed :)


> +	struct gpio_desc *echo_desc;
> +		/* Used to measure length of ECHO signal */

I was going to say "comments on same line", but that would result in *long*
lines, this is better....


> +static int do_measurement(struct hc_sr04 *device,
> +			  long long *usecs_elapsed)
> +{
(...)
> +	if (!mutex_trylock(&device->measurement_mutex))
> +		return -EBUSY;

OK... this is a potential problem, because...

> +	irq = gpiod_to_irq(device->echo_desc);
> +	if (irq < 0)
> +		return -EIO;

Here you do a 'return' without unlocking.  This should probably be:

         if (irq < 0) {
		ret = -EIO;
		goto out_mutex;
	}

I admit not knowing the GPIO or IIO stuff well enough to comment on those
details, but I didn't see anything obviously insane either....