On Sun, Jul 13, 2008 at 09:42:09PM +0200, Erik Andr?n wrote:
> Is the m5602 branch working for you?
Define working. xawtv does get an image, almost completely black
though. If I have enough light on me and crank gimp's brightness and
contrast all the way up I can see that yes it is an image of me.
I coded up a set brightness and contrast controls so as to change the
sensor's 0x7b value, and reinitialize it on the fly. The value 0xff
gives the above description, any other value and it's wierd. It picks
up things that aren't as bright as before, but it seems to have a
threshold, and it reminds me of a night vision quality to it. If
something does saturate the sensor, the lower part of the screen is
somewhat garbled, and doesn't update. I will update to the Lenovo
V200 init and see what happens, I've only tried the default one so
far.
Do you have any USB snoops logs for an Acer Aspire 5102 or related I
could get? I just have Linux installed on the laptop, and don't have
an easy way to generate them myself.
> My current plan has been to develop the "real" driver in the m5602
> branch with all others being experimental and only used to stabilize
> each sensor to the point that they can be merged.
Should I submit patches against the trunk m5602?
> > +++ b/branches/m5602-s5k83a/m5602.c
> > @@ -339,6 +339,7 @@ static void m5602_release_resources(struct m5602_camera
> > *cam)
> > /* up(&m5602_sysfs_lock); */
> >
> > kfree(cam->control_buffer);
> > + cam->control_buffer = NULL;
>
>
> Why should we set this to NULL, AFAICT we never test for this to be NULL.
Code safety, a NULL pointer dereference crashes sooner and more
obviously.
usb_m5602_disconnect calls m5602_release_resources
m5602_release_resources calls kfree(cam->control_buffer);
usb_m5602_disconnect calls kfree(cam)
That's the only code path, and it can't be used after kfree, no chance
of it getting hit. I'll remove it in the next patch.
> > +++ b/branches/m5602-s5k83a/m5602_v4l2.c
> > @@ -166,6 +166,7 @@ static void m5602_release_buffers(struct m5602_camera
> > *cam)
> >
> > /* Free the canvas */
> > kfree(cam->canvas);
> > + cam->canvas = NULL;
>
> Same for this
m5602_release_buffers is called from a few different places, I don't
see an obvious, it can't happen like above. It might be good to leave
it in if there are any invalid uses of it. I'll remove it if you
want.
> > @@ -482,7 +483,10 @@ static int m5602_start_transfer(struct m5602_camera
> > *cam)
> > if (retval) {
> > PDEBUG(DBG_V4L2, "usb_submit_urb failed!
> > %d",
> > retval);
> > - goto free_urbs;
> > + /* Start killing the previous URB. */
> > + for (i--; i>=0; i--)
> > + usb_kill_urb(cam->urb[i]);
> > + goto free_buffers;
>
>
> I see your point here but isn't it more easily read code to do all cleanup
> at the end of the function?
That inner for loop could just be moved down. I just wanted to keep
i in the for loop it was initialized in. i is needed to know which
urbs to kill, we know the failed urb doesn't need killing, or would it
be safe to kill all the urbs?
> Thanks,
> Erik
--
David Fries <[EMAIL PROTECTED]>
http://fries.net/~david/ (PGP encryption key available)
-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08
_______________________________________________
M560x-driver-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/m560x-driver-devel
