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:
> 1. 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,
> 2. 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).
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
