Dne Ne 22. srpna 2010 22:30:10 Guennadi Liakhovetski napsal(a):
On Sun, 22 Aug 2010, Janusz Krzysztofik wrote:
Hi Guennadi, Thanks for your review time.
Sunday 22 August 2010 18:40:13 Guennadi Liakhovetski wrote:
On Sun, 18 Jul 2010, Janusz Krzysztofik wrote:
This patch provides a V4L2 SoC Camera driver for OV6650 camera sensor, found on OMAP1 SoC based Amstrad Delta videophone.
Have you also had a look at drivers/media/video/gspca/sonixb.c - in also supports ov6650 among other sensors.
Yes, I have, but given up for now since:
- the gspca seems using the sensor in "Bayer 8 BGGR" mode only, which I
was
not even able to select using mplayer to test my drivers with,
- not all settings used there are clear for me, so I've decided to
revisit
them later, when I first get a stable, even if not perfect, working driver version accepted, instead of following them blindly.
But good that you've looked at it.
- unsigned char data[2] = { reg, val };
- struct i2c_msg msg = {
.addr = client->addr,.flags = 0,.len = 2,.buf = data,- };
- ret = i2c_transfer(client->adapter, &msg, 1);
- if (ret < 0) {
dev_err(&client->dev, "Failed writing register 0x%02x!
\n", reg);
return ret;- }
- msleep(1);
Hm, interesting... Is this really needed?
If you mean msleep(1) - yes, the sensor didn't work correctly for me without that msleep().
Yes, I meant msleep(1).
Doesn't surprise me at all actually. Check how this is done on ov9640 and also NOTE I had to use gpio-spi module instead of pxa-spi to get the communication running at all ... might be your case.
Cheers
I you mean reading the register back - I've not tried without, will do.
Ok.
- case V4L2_CID_HUE_AUTO:
if (ctrl->value) {ret = ov6650_reg_rmw(client, REG_HUE,SET_HUE(DEF_HUE),
SET_HUE(HUE_MASK));
if (ret)break;priv->hue = DEF_HUE;} else {ret = ov6650_reg_rmw(client, REG_HUE, HUE_EN,
0);
}if (ret)break;priv->hue_auto = ctrl->value;Hm, sorry, don't understand. If the user sets auto-hue to ON, you set the hue enable bit and hue value to default.
No, I reset the hue enable bit here, which I understand is used for applying a non-default hue value if set rather than enabling auto hue. Maybe my understanding of this bit function is wrong.
If the user sets auto-hue to OFF, you just set the hue enable bit on and don't change the value. Does ov6650 actually support auto-hue?
All I was able to find out was the HUE register bits described like this:
Bit[7:6]: Reserved Bit[5]: Hue Enable Bit[4:0]: Hue setting
and the register default value: 0x10.
What do you think the bit[5] meaning is?
Well, from how I interpret, what you say, I think, there is no auto-hue implemented by this sensor, at least, not by this register. Maybe drop auto-hue support completely? It seems to me just a manual hue value can be set.
+/* select nearest higher resolution for capture */ +static void ov6650_res_roundup(u32 *width, u32 *height) +{
- int i;
- enum { QCIF, CIF };
- int res_x[] = { 176, 352 };
- int res_y[] = { 144, 288 };
- for (i = 0; i < ARRAY_SIZE(res_x); i++) {
if (res_x[i] >= *width && res_y[i] >= *height) {*width = res_x[i];*height = res_y[i];return;}- }
- *width = res_x[CIF];
- *height = res_y[CIF];
+}
This can be replaced by a version of
http://www.spinics.net/lists/linux-media/msg21893.html
when it is fixed and accepted;) I'll try to send an updated version of that patch tomorrow.
Fine, I'll use this instead of my dirty workarounds.
/me has to update that patch... Will try to do that asap.
+/* program default register values */ +static int ov6650_prog_dflt(struct i2c_client *client) +{
- int i, ret;
- dev_dbg(&client->dev, "reinitializing\n");
- for (i = 0; i < ARRAY_SIZE(ov6650_regs_dflt); i++) {
ret = ov6650_reg_write(client, ov6650_regs_dflt[i].reg,
ov6650_regs_dflt[i].val);
if (ret)return ret;- }
Hm, please, don't. I generally don't like such register - value array magic for a number of reasons, and in your case it's just one (!) register write operation - please, remove this array and just write the register explicitly.
OK (with a reservation that I can probably end up with more than just one, non-default settings written explicitly).
Thas's ok. I find a sequence of explicit register writes nicer, e.g., because then you can insert comments / delays / meaningful debugging / other branching between them. Pushing an array of register values to the hardware looks like "no idea what this stuff is good for, I just copied this from vendor's example / sniffed from the hardware..."
- ret = ov6650_reg_write(client, REG_HSTRT, hstrt);
- if (!ret)
ret = ov6650_reg_write(client, REG_HSTOP, hstop);- if (!ret)
ret = ov6650_reg_write(client, REG_VSTRT, vstrt);- if (!ret)
ret = ov6650_reg_write(client, REG_VSTOP, vstop);Are cropping and scaling on this camera absolutely independent? I.e., you can set any output format (CIF or QCIF) and it will just scale whatever rectangle has been configured? And the other way round - you set arbitrary cropping and output format stays the same?
I believe it works like I have put it here, but will try to recheck to make sure. Simply using v4l2-debug for this seems insufficient, since changing a frame format on the fly will get DMA out of sync immediately. What tool or utility could you advise for testing?
firstly, soc-camera is quite restrictive about s_crop ATM: it disallows changes to the cropping sizes (only position can be changed). Whereby, now that I think about it again, perhaps this wasn't a very good idea: effectively this kills live zooming. Maybe we can lift that restriction again. In any case, I don't know any existing programs, that can stream video and simultaneously allow the user to issue crop and scale commands. I just hacked mplayer and gstreamer for that. Or, to test changing left and top offsets, I had mplayer running and issued crops and scales from another window with a command-line tool like v4l2-dbg.
+static struct v4l2_subdev_video_ops ov6650_video_ops = {
- .s_stream = ov6650_s_stream,
- .s_mbus_fmt = ov6650_s_fmt,
- .try_mbus_fmt = ov6650_try_fmt,
Please, implement.g_mbus_fmt.
OK (in addition to what I've already implemented, I guess).
Of course
Thanks Guennadi
Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html