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

  This patch provides a V4L2 SoC Camera driver for OV6650 camera sensor, found 
on OMAP1 SoC based Amstrad Delta videophone.

Since I have no experience with camera sensors, and the sensor documentation I 
was able to find was not very comprehensive, I left most settings at their 
default (reset) values, except for:
- those required for proper mediabus parameters and picture format setup,
- those used by controls.
Resulting picture quality is far from perfect, but better than nothing.

In order to be able to get / set the sensor frame rate from userspace, I 
decided to provide two not yet SoC camera supported operations, g_parm and 
s_parm. These can be used after applying patch 4/6 from this series, 
"SoC Camera: add support for g_parm / s_parm operations".

Created and tested against linux-2.6.36-rc3 on Amstrad Delta.

Signed-off-by: Janusz Krzysztofik <jkrzyszt@tis.icnet.pl>
---
v1->v2 changes:

requested by Guennadi Liakhovetski (thanks!):
- include <linux/bitops.h> if using BIT() macro,
- sort headers alphabetically,
- don't mix tabs with spaces (preferred) when separating symbols from 
  #define keywords,
- drop unused NUM_REGS definition,
- optimize SET_SAT() and SAT_MASK macros,
- reuse no longer needed function argument instead of declaring a new local 
  variable,
- don't touch auto controls when changing their correspondig manual settings, 
  and vice versa,
- drop probably unsupported auto-hue control,
- initialize sensor by writing registers explicitly instead of using a "magic" 
  initialization array,
- avoid gotos, don't use them other than in failure cases,
- make pclk computation more readable,
- implement g_mbus_fmt() callback,
- correct a few obvious mistakes,
- remove a few extra whitespaces,

suggested by Ralph Corderoy (thanks!):
- use one common format when hex printing register addresses and values,
- optimize if(ret) vs. if(!ret) constructs usage,
- replace a few if-else constructs with more compact, conditional 
  expression based, when translating controls to register bits,
- optimize ov6650_res_roundup(),
- drop redundant cast of index from ov6650_enum_fmt(),
- use variable identifiers rather than their types as sizeof() arguments,

other:
- disable band filter, auto exposure control seems working more effectively 
  without it,
- refresh against linux-2.6.36-rc3.


 drivers/media/video/Kconfig     |    6
 drivers/media/video/Makefile    |    1
 drivers/media/video/ov6650.c    | 1242 ++++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-chip-ident.h |    1
 4 files changed, 1250 insertions(+)


--
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
  

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?...

> +
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed writing register 0x%02x!\n", reg);
> +		return ret;
> +	}
> +	return 0;
> +}

...

> +/* set the format we will capture in */
> +static int ov6650_s_fmt(struct v4l2_subdev *sd,
> +			struct v4l2_mbus_framefmt *mf)
> +{
> +	struct i2c_client *client = sd->priv;
> +	struct soc_camera_device *icd	= client->dev.platform_data;
> +	struct soc_camera_sense *sense = icd->sense;
> +	struct ov6650 *priv = to_ov6650(client);
> +	enum v4l2_mbus_pixelcode code = mf->code;
> +	unsigned long pclk;
> +	u8 coma_set = 0, coma_mask = 0, coml_set = 0, coml_mask = 0;
> +	u8 clkrc, clkrc_div;
> +	int ret;
> +
> +	/* select color matrix configuration for given color encoding */
> +	switch (code) {
> +	case V4L2_MBUS_FMT_GREY8_1X8:
> +		dev_dbg(&client->dev, "pixel format GREY8_1X8\n");
> +		coma_set |= COMA_BW;
> +		coma_mask |= COMA_RGB | COMA_WORD_SWAP | COMA_BYTE_SWAP;
> +		break;
> +	case V4L2_MBUS_FMT_YUYV8_2X8:
> +		dev_dbg(&client->dev, "pixel format YUYV8_2X8_LE\n");
> +		coma_set |= COMA_WORD_SWAP;
> +		coma_mask |= COMA_RGB | COMA_BW | COMA_BYTE_SWAP;
> +		break;
> +	case V4L2_MBUS_FMT_YVYU8_2X8:
> +		dev_dbg(&client->dev, "pixel format YVYU8_2X8_LE (untested)\n");
> +		coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP |
> +				COMA_BYTE_SWAP;
> +		break;
> +	case V4L2_MBUS_FMT_UYVY8_2X8:
> +		dev_dbg(&client->dev, "pixel format YUYV8_2X8_BE\n");
> +		if (mf->width == W_CIF) {
> +			coma_set |= COMA_BYTE_SWAP | COMA_WORD_SWAP;
> +			coma_mask |= COMA_RGB | COMA_BW;
> +		} else {
> +			coma_set |= COMA_BYTE_SWAP;
> +			coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP;
> +		}
> +		break;
> +	case V4L2_MBUS_FMT_VYUY8_2X8:
> +		dev_dbg(&client->dev, "pixel format YVYU8_2X8_BE (untested)\n");
> +		if (mf->width == W_CIF) {
> +			coma_set |= COMA_BYTE_SWAP;
> +			coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP;
> +		} else {
> +			coma_set |= COMA_BYTE_SWAP | COMA_WORD_SWAP;
> +			coma_mask |= COMA_RGB | COMA_BW;
> +		}
> +		break;
> +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> +		dev_dbg(&client->dev, "pixel format SBGGR8_1X8 (untested)\n");
> +		coma_set |= COMA_RAW_RGB | COMA_RGB;
> +		coma_mask |= COMA_BW | COMA_BYTE_SWAP | COMA_WORD_SWAP;
> +		break;
> +	default:
> +		dev_err(&client->dev, "Pixel format not handled: 0x%x\n", code);
> +		return -EINVAL;
> +	}
> +	priv->code = code;
> +
> +	if ((code == V4L2_MBUS_FMT_GREY8_1X8) ||
> +			(code == V4L2_MBUS_FMT_SBGGR8_1X8)) {

Superfluous parenthesis

> +		coml_mask |= COML_ONE_CHANNEL;
> +		priv->pclk_max = 4000000;
> +	} else {
> +		coml_set |= COML_ONE_CHANNEL;
> +		priv->pclk_max = 8000000;
> +	}

coml_mask and coml_set are only set here and only used once below, so, 
dropping initialisation to 0 in variable definitions and just doing

+	if (code == V4L2_MBUS_FMT_GREY8_1X8 ||
+			code == V4L2_MBUS_FMT_SBGGR8_1X8) {
+		coml_mask = COML_ONE_CHANNEL;
+		coml_set = 0;
+		priv->pclk_max = 4000000;
+	} else {
+		coml_mask = 0;
+		coml_set = COML_ONE_CHANNEL;
+		priv->pclk_max = 8000000;
+	}

would work too.

> +
> +	if (code == V4L2_MBUS_FMT_SBGGR8_1X8)
> +		priv->colorspace = V4L2_COLORSPACE_SRGB;
> +	else
> +		priv->colorspace = V4L2_COLORSPACE_JPEG;
> +
> +	/*
> +	 * Select register configuration for given resolution.
> +	 * To be replaced with a common function that does it, once available.
> +	 */
> +	ov6650_res_roundup(&mf->width, &mf->height);
> +
> +	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;
> +	}
> +
> +	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? But 
which of them is stored in your priv->rect? Is this the input window 
(cropping) or the output one (scaling)? You overwrite it in .s_fmt and 
.s_crop...

> +
> +	if (priv->timeperframe.numerator && priv->timeperframe.denominator)
> +		pclk = priv->pclk_max * priv->timeperframe.denominator /
> +				(FRAME_RATE_MAX * priv->timeperframe.numerator);
> +	else
> +		pclk = priv->pclk_max;
> +
> +	if (sense) {
> +		if (sense->master_clock == 8000000) {
> +			dev_dbg(&client->dev, "8MHz input clock\n");
> +			clkrc = CLKRC_6MHz;
> +		} else if (sense->master_clock == 12000000) {
> +			dev_dbg(&client->dev, "12MHz input clock\n");
> +			clkrc = CLKRC_12MHz;
> +		} else if (sense->master_clock == 16000000) {
> +			dev_dbg(&client->dev, "16MHz input clock\n");
> +			clkrc = CLKRC_16MHz;
> +		} else if (sense->master_clock == 24000000) {
> +			dev_dbg(&client->dev, "24MHz input clock\n");
> +			clkrc = CLKRC_24MHz;
> +		} else {
> +			dev_err(&client->dev,
> +				"unspported input clock, check platform data\n");
> +			return -EINVAL;
> +		}
> +		priv->pclk_limit = sense->pixel_clock_max;
> +		if (priv->pclk_limit && (priv->pclk_limit < pclk))

Don't think the compiler would complain without the internal parenthesis 
here?

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
  

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?

> > +
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "Failed writing register 0x%02x!\n", reg);
> > +		return ret;
> > +	}
> > +	return 0;
> > +}
>
> ...
>
> > +/* set the format we will capture in */
> > +static int ov6650_s_fmt(struct v4l2_subdev *sd,
> > +			struct v4l2_mbus_framefmt *mf)
> > +{
> > +	struct i2c_client *client = sd->priv;
> > +	struct soc_camera_device *icd	= client->dev.platform_data;
> > +	struct soc_camera_sense *sense = icd->sense;
> > +	struct ov6650 *priv = to_ov6650(client);
> > +	enum v4l2_mbus_pixelcode code = mf->code;
> > +	unsigned long pclk;
> > +	u8 coma_set = 0, coma_mask = 0, coml_set = 0, coml_mask = 0;
> > +	u8 clkrc, clkrc_div;
> > +	int ret;
> > +
> > +	/* select color matrix configuration for given color encoding */
> > +	switch (code) {
> > +	case V4L2_MBUS_FMT_GREY8_1X8:
> > +		dev_dbg(&client->dev, "pixel format GREY8_1X8\n");
> > +		coma_set |= COMA_BW;
> > +		coma_mask |= COMA_RGB | COMA_WORD_SWAP | COMA_BYTE_SWAP;
> > +		break;
> > +	case V4L2_MBUS_FMT_YUYV8_2X8:
> > +		dev_dbg(&client->dev, "pixel format YUYV8_2X8_LE\n");
> > +		coma_set |= COMA_WORD_SWAP;
> > +		coma_mask |= COMA_RGB | COMA_BW | COMA_BYTE_SWAP;
> > +		break;
> > +	case V4L2_MBUS_FMT_YVYU8_2X8:
> > +		dev_dbg(&client->dev, "pixel format YVYU8_2X8_LE (untested)\n");
> > +		coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP |
> > +				COMA_BYTE_SWAP;
> > +		break;
> > +	case V4L2_MBUS_FMT_UYVY8_2X8:
> > +		dev_dbg(&client->dev, "pixel format YUYV8_2X8_BE\n");
> > +		if (mf->width == W_CIF) {
> > +			coma_set |= COMA_BYTE_SWAP | COMA_WORD_SWAP;
> > +			coma_mask |= COMA_RGB | COMA_BW;
> > +		} else {
> > +			coma_set |= COMA_BYTE_SWAP;
> > +			coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP;
> > +		}
> > +		break;
> > +	case V4L2_MBUS_FMT_VYUY8_2X8:
> > +		dev_dbg(&client->dev, "pixel format YVYU8_2X8_BE (untested)\n");
> > +		if (mf->width == W_CIF) {
> > +			coma_set |= COMA_BYTE_SWAP;
> > +			coma_mask |= COMA_RGB | COMA_BW | COMA_WORD_SWAP;
> > +		} else {
> > +			coma_set |= COMA_BYTE_SWAP | COMA_WORD_SWAP;
> > +			coma_mask |= COMA_RGB | COMA_BW;
> > +		}
> > +		break;
> > +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> > +		dev_dbg(&client->dev, "pixel format SBGGR8_1X8 (untested)\n");
> > +		coma_set |= COMA_RAW_RGB | COMA_RGB;
> > +		coma_mask |= COMA_BW | COMA_BYTE_SWAP | COMA_WORD_SWAP;
> > +		break;
> > +	default:
> > +		dev_err(&client->dev, "Pixel format not handled: 0x%x\n", code);
> > +		return -EINVAL;
> > +	}
> > +	priv->code = code;
> > +
> > +	if ((code == V4L2_MBUS_FMT_GREY8_1X8) ||
> > +			(code == V4L2_MBUS_FMT_SBGGR8_1X8)) {
>
> Superfluous parenthesis

Will be dropped.

> > +		coml_mask |= COML_ONE_CHANNEL;
> > +		priv->pclk_max = 4000000;
> > +	} else {
> > +		coml_set |= COML_ONE_CHANNEL;
> > +		priv->pclk_max = 8000000;
> > +	}
>
> coml_mask and coml_set are only set here and only used once below, so,
> dropping initialisation to 0 in variable definitions and just doing
>
> +	if (code == V4L2_MBUS_FMT_GREY8_1X8 ||
> +			code == V4L2_MBUS_FMT_SBGGR8_1X8) {
> +		coml_mask = COML_ONE_CHANNEL;
> +		coml_set = 0;
> +		priv->pclk_max = 4000000;
> +	} else {
> +		coml_mask = 0;
> +		coml_set = COML_ONE_CHANNEL;
> +		priv->pclk_max = 8000000;
> +	}
>
> would work too.

OK, I'll use your prefered pattern.

> > +
> > +	if (code == V4L2_MBUS_FMT_SBGGR8_1X8)
> > +		priv->colorspace = V4L2_COLORSPACE_SRGB;
> > +	else
> > +		priv->colorspace = V4L2_COLORSPACE_JPEG;
> > +
> > +	/*
> > +	 * Select register configuration for given resolution.
> > +	 * To be replaced with a common function that does it, once available.
> > +	 */
> > +	ov6650_res_roundup(&mf->width, &mf->height);
> > +
> > +	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;
> > +	}
> > +
> > +	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 (priv->timeperframe.numerator && priv->timeperframe.denominator)
> > +		pclk = priv->pclk_max * priv->timeperframe.denominator /
> > +				(FRAME_RATE_MAX * priv->timeperframe.numerator);
> > +	else
> > +		pclk = priv->pclk_max;
> > +
> > +	if (sense) {
> > +		if (sense->master_clock == 8000000) {
> > +			dev_dbg(&client->dev, "8MHz input clock\n");
> > +			clkrc = CLKRC_6MHz;
> > +		} else if (sense->master_clock == 12000000) {
> > +			dev_dbg(&client->dev, "12MHz input clock\n");
> > +			clkrc = CLKRC_12MHz;
> > +		} else if (sense->master_clock == 16000000) {
> > +			dev_dbg(&client->dev, "16MHz input clock\n");
> > +			clkrc = CLKRC_16MHz;
> > +		} else if (sense->master_clock == 24000000) {
> > +			dev_dbg(&client->dev, "24MHz input clock\n");
> > +			clkrc = CLKRC_24MHz;
> > +		} else {
> > +			dev_err(&client->dev,
> > +				"unspported input clock, check platform data\n");
> > +			return -EINVAL;
> > +		}
> > +		priv->pclk_limit = sense->pixel_clock_max;
> > +		if (priv->pclk_limit && (priv->pclk_limit < pclk))
>
> Don't think the compiler would complain without the internal parenthesis
> here?

OK, I'll drop them.

Thanks,
Janusz

> 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


--
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
  

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.

> > > +	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, whereas COMA and COML select 
whether to scale it to a CIF or to a QCIF output. 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.


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.

...and, please, replace

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

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, but let's say, you fix it to always return CIF.

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.

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. Then, without 
changing COMA and COML you assume, that the output size changed equally, 
because that's what you return in g_fmt.

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?;)

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
  

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
--
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
  

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:
> > > > > +	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).

Yes, sorry, forgot to mention that.

> > 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.

> > 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.

...and for the ->qcif member.

> > 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.

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.

> 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?

that's exactly the purpose, I think.

> > 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.

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?" 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.

> > 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.

Yes, if you cannot handle smaller values.

> > 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.

Yes, now that I know, that that flag is actually a scale-down-by-2 switch.

> > 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?

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?

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
  

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

On Fri, 24 Sep 2010, Janusz Krzysztofik wrote:

> 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?

ok, so, it means to which output window the _complete_ sensor area is 
mapped. And if you get a smaller sensor rectangle, you get a smaller 
output image, right? Ok, then you can just leave it as is.

> ...
> > > > 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()?

Good idea!:)

> ...
> > > > 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.

Good! Looking forward:)

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