[E3-hacking] [PATCH v2 3/6] SoC Camera: add driver for OV6650 sensor

Thu Sep 23 23:45:23 BST 2010

Thursday 23 September 2010 18:06:15 Guennadi Liakhovetski napisał(a):
> On Wed, 22 Sep 2010, Janusz Krzysztofik wrote:
> > Wednesday 22 September 2010 11:12:46 Guennadi Liakhovetski napisaÅ‚(a):
> > > Ok, just a couple more comments, all looking quite good so far, if we
> > > get a new version soon enough, we still might manage it for 2.6.37
> > >
> > > On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:
> > >
> > > [snip]
> > >
> > > > +/* write a register */
> > > > +static int ov6650_reg_write(struct i2c_client *client, u8 reg, u8
> > > > val) +{
> > > > +	int ret;
> > > > +	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);
> > > > +	msleep_interruptible(1);
> > >
> > > Why do you want _interruptible here? Firstly it's just 1ms, secondly -
> > > why?...
> >
> > My bad. I didn't verified what a real difference between msleep() and
> > msleep_interruptible() is, only found that msleep_interruptible(1) makes
> > checkpatch.pl more happy than msleep(1), sorry.
> >
> > What I can be sure is that a short delay is required here, otherwise the
> > driver doesn't work correctly. To prevent the checkpatch.pl from
> > complying against msleep(1), I think I could just replace it with
> > msleep(20). What do you think?
>
> oh, no, don't think replacing msleep(1) with msleep(20) just to silence a
> compiler warning is a god idea...;) Well, use a udelay(1000). Or maybe
> try, whether a udelay(100) suffices too.

OK. Thanks for the hints.

> > > > +	priv->rect.left	  = DEF_HSTRT << !priv->qcif;
> > > > +	priv->rect.top	  = DEF_VSTRT << !priv->qcif;
> > > > +	priv->rect.width  = mf->width;
> > > > +	priv->rect.height = mf->height;
> > >
> > > Sorry, don't understand. The sensor can do both - cropping per HSTRT,
> > > HSTOP, VSTRT and VSTOP and scaling per COMA_CIF / COMA_QCIF, right?
> >
> > Right.
> >
> > > But
> > > which of them is stored in your priv->rect? Is this the input window
> > > (cropping) or the output one (scaling)?
> >
> > I'm not sure how I can follow your input/output concept here.
> > Default (reset) values of HSTRT, HSTOP, VSTRT and VSTOP registers are the
> > same for both CIF and QCIF, giving a 176x144 picture area in both cases.
> > Than, when in CIF (reset default) mode, which actual size is double of
> > that (352x288), I scale them by 2 when converting to priv->rect elements.
> >
> > > You overwrite it in .s_fmt and
> > > .s_crop...
> >
> > I added the priv->rect to be returned by g_crop() instead of
> > recalculating it from the register values. Then, I think I have to
> > overwrite it on every geometry change, whether s_crop or s_fmt caused. Am
> > I missing something?
>
> If I understand your sensor correctly, HSTRT etc. registers configure
> pretty much any (input) sensor window, 

Let's say, not exceeding CIF geometry (352x288).

> whereas COMA and COML select 
> whether to scale it to a CIF or to a QCIF output. 

I think the answer is: not exactly. AFAICT, the COMA_QCIF bit selects whether 
to scale it down by 2 (QCIF selection) or not (CIF selection).

> So, these are two 
> different things. Hence your ->rect can hold only one of the two - the
> sensor window or the output image. Since output image has only two options
> - CIF or QCIF, you don't need to store it in rect, you already have
> priv->qcif.

With the above reservation - yes, I could use priv->qcif to scale priv->rect 
down by 2 or not in g_fmt.

> Oh, and one more thing - didn't notice before: in your cropcap you do
>
> +	int shift = !priv->qcif;
> ...
> +	a->bounds.left			= DEF_HSTRT << shift;
> +	a->bounds.top			= DEF_VSTRT << shift;
> +	a->bounds.width			= W_QCIF << shift;
> +	a->bounds.height		= H_QCIF << shift;
>
> Don't think this is right. cropcap shouldn't depend on any dynamic
> (configured by S_FMT) setting, it contains absolute limits of your
> hardware. I know I might have produced a couple of bad examples in the
> past - before I eventually settled with this definition... So, I think,
> it's best to put the full sensor resolution in cropcap.

OK.

> ...and, please, replace
>
> +		priv->qcif = 1;
> with
> +		priv->qcif = true;
> and
> +		priv->qcif = 0;
> with
> +		priv->qcif = false;
> in ov6650_s_fmt().

Sure.

> But I think your driver might have a problem with its cropping / scaling
> handling. Let's see if I understand it right:
>
> 1. as cropcap you currently return QCIF or CIF, depending on the last
> S_FMT, 

Yes.

BTW, my S_FMT always calls ov6650_reset(), which resets the current crop to 
defaults. This behaviour doesn't follow the requirement of this operation 
being done only once, when the driver is first loaded, but not later. Should 
I restore the last crop after every reset? If yes, what is the purpose of 
defrect if applied only at driver first load?

> but let's say, you fix it to always return CIF. 

OK.

> 2. in your s_fmt you accept only two output sizes: CIF or QCIF, that's ok,
> if that's all you can configure with your driver.

Not any longer :). I'm able to configure using current crop geometry only, 
either unscaled or scaled down by 2. I'm not able to configure neither exact 
CIF nor QCIF if my current crop window doesn't match, unless I'm allowed to 
change the crop from here.

> 3. in s_crop you accept anything with left + width <= HSTOP and top +
> height <= VSTOP. This I don't understand. Your HSTOP and VSTOP are fixed
> values, based on QCIF plus some offsets. So, you would accept widths up to
> "a little larger than QCIF width" and similar heights. 

Do you mean I should also verify if (rect->left >= DEF_HSTRT) and (rect->top 
>= DEF_VSTRT)? I should probably.

> Then, without 
> changing COMA and COML you assume, that the output size changed equally,
> because that's what you return in g_fmt.

I think I've verified, to the extent possible using v4l2-dbg, that it works 
like this. If I change the input height by overwriting VSTOP with a slightly 
higher value, the output seems to change proportionally and starts rolling 
down, since the host is not updated to handle the so changed frame size.

> Anyway, I think, there is some misunderstanding of the v4l2 cropping and
> scaling procedures. Please, have a look here:
> http://v4l2spec.bytesex.org/spec/x1904.htm. Do you agree, that what your
> driver is implementing doesn't reflect that correctly?;)

Yes, I do. And I think that that the reason of this misunderstanding is my 
sensor just not matching the v4l2 model with its limited, probably uncommon 
scaling capability, or me still not being able to map the sensor to the v4l2 
model correctly. What do you think?

Thanks,
Janusz