Re: [PATCH v2 3/6] SoC Camera: add driver for OV6650 sensor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Friday 24 September 2010 08:52:32 Guennadi Liakhovetski napisaÅ(a):
> On Fri, 24 Sep 2010, Janusz Krzysztofik wrote:
> > 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):
> > > > > On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:

...
> > > 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).
>
> Ah! Ok, that it would be better to select different names for those bits.

I was trying to keep all names more or less consistent with the wording used 
in the sensor documentation (which doesn't follow the v4l2 specification 
unfortunately :). In this case we have:

COMA  Common Control A
	...
	Bit[5]: Output format â resolution
		0: CIF (352 x 288)
		1: QCIF (176 x 144)

So I could rename it to something like COMA_OUTFMT or COMA_RESOLUTION. Which 
one sounds better for you?

...
> > > 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.
>
> Oh, does it mean all registers are reset to their defaults? That'd be not
> good - no v4l(2) ioctl, AFAIK, should affect parameters, not directly
> related to it. Even closing and reopening the video device node shouldn't
> reset values. So, maybe you should drop that reset completely.

Shouldn't I rather move it over into the ov6650_video_probe()?

...
> > > 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.
>
> Hm, but in your s_fmt you do:
>
> +	switch (mf->width) {
> +	case W_QCIF:
> +		dev_dbg(&client->dev, "resolution QCIF\n");
> +		priv->qcif = 1;
> +		coma_set |= COMA_QCIF;
> +		priv->pclk_max /= 2;
> +		break;
> +	case W_CIF:
> +		dev_dbg(&client->dev, "resolution CIF\n");
> +		priv->qcif = 0;
> +		coma_mask |= COMA_QCIF;
> +		break;
> +	default:
> +		dev_err(&client->dev, "unspported resolution!\n");
> +		return -EINVAL;
> +	}
>
> So, you accept only CIF or QCIF as your output window. Or do you mean a v3
> by your "not any longer?" 

Exactly!

> And yes, you are allowed to change your input 
> sensor window, if that lets you configure your output format more
> precisely. And v.v. The rule is - the most recent command wins.

I see.

...
> No, there's nothing wrong with your sensor:) So, what I would do is:
>
> 1. in your struct ov6650:
>
> +	struct v4l2_rect	rect;		/* sensor cropping window */
> +	bool			half_scale;	/* scale down output by 2 */
>
> 2. in s_crop verify left, width, top, height, program them into the chip
> and store in ->rect
>
> 3. in g_crop just return values from ->rect
>
> 4. in s_fmt you have to select an input rectangle, that would allow you to
> possibly exactly configure the requested output format. Say, if you have a
> 320x240 cropping configured and you get an s_fmt request for 120x90. You
> can either set your input rectangle to 240x180 and scale it down by 2, or
> set the rectangle directly to 120x90. Obviously, it's better to use
> 240x180 and scale down, because that's closer to the current cropping of
> 320x240. So, in s_fmt you select a new input rectangle _closest_ to the
> currently configured one, that would allow you to configure the correct
> output format. Then you set your ->rect with the new values and your
> ->half_scale
>
> 5. in g_fmt you return ->rect scaled with ->half_scale
>
> Makes sense?

Absolutely. Hope to submit v3 soon.

Thanks,
Janusz
--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux