Comment 60 for bug 790183

Comment on attachment 82310
0001-lib-Add-VFS5011-driver.patch

Review of attachment 82310:
-----------------------------------------------------------------

::: libfprint/drivers/vfs5011.c
@@ +33,5 @@
> + }
> + fp_dbg(s);
> +#endif
> +}
> +

Please drop all debugging stuff from driver. Only debug logs via fp_dbg are allowed.

@@ +321,5 @@
> +static void median_filter(int *data, int size, int filtersize)
> +{
> + int i;
> + int *result = (int *)malloc(size*sizeof(int));
> + int *sortbuf = (int *)malloc(filtersize*sizeof(int));

Please use g_malloc or g_malloc0 here.

@@ +329,5 @@
> + if (i1 < 0)
> + i1 = 0;
> + if (i2 >= size)
> + i2 = size-1;
> + memmove(sortbuf, data+i1, (i2-i1+1)*sizeof(int));

g_memmove()

@@ +330,5 @@
> + i1 = 0;
> + if (i2 >= size)
> + i2 = size-1;
> + memmove(sortbuf, data+i1, (i2-i1+1)*sizeof(int));
> + qsort(sortbuf, i2-i1+1, sizeof(int), cmpint);

I wonder if there's glib wrapper for that one...

@@ +364,5 @@
> + };
> + int i;
> + float y = 0.0;
> + int line_ind = 0;
> + int *offsets = (int *)malloc(input_lines * sizeof(int));

g_malloc()

@@ +372,5 @@
> +
> + if (debugpath != NULL) {
> + sprintf(name, "%s/offsets%d.dat", debugpath, (int)time(NULL));
> + debugfile = fopen(name, "wb");
> + }

Drop debug stuff except logs

@@ +945,5 @@
> + array_n_elements(vfs5011_initialization);
> + data->init_sequence.actions = vfs5011_initialization;
> + data->init_sequence.device = dev;
> + data->init_sequence.receive_buf =
> + malloc(VFS5011_RECEIVE_BUF_SIZE);

g_malloc()

@@ +989,5 @@
> + int r = libusb_reset_device(dev->udev);
> + if (r != 0) {
> + fp_err("Failed to reset the device");
> + return r;
> + }

Is it necessary? Btw, doc says that handle may be invalid after reset, and you're reusing same handle later.

@@ +1000,5 @@
> +
> + struct fpi_ssm *ssm;
> + ssm = fpi_ssm_new(dev->dev, open_loop, DEV_OPEN_NUM_STATES);
> + ssm->priv = dev;
> + fpi_ssm_start(ssm, open_loop_complete);

And no one waits for this ssm (and async USB reqs) to complete here. It's a race condition I've described earlier. Please move device initialization into dev_activate().

@@ +1035,5 @@
> +}
> +
> +static void dev_deactivate(struct fp_img_dev *dev)
> +{
> + fpi_imgdev_deactivate_complete(dev);

How do you ensure that _all_ transfers are completed now? fpi_imgdev_deactivate_complete should be called only after deactivation has been completed. For this driver "cancel" button in fprint_demo won't work.