On Tue, 31 May 2016 23:05:57 +0200, johannes@xxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxxxx>
>
> 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....
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@xxxxxxxxxxxxxxxxx
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
