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.
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/790183
Title:
[138a:0011] Fingerprint reader Validity Sensors not recognized
To manage notifications about this bug go to:
https://bugs.launchpad.net/libfprint/+bug/790183/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
