Re: [PATCH] [media] v4l: Add mt9v034 sensor driver

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Enric,

In general this driver looks good to me. However it seems you're
using it together with the omap3isp driver. Likely Laurent and Sakari
may have some comments on that.

Just one thing I'm unsure about, please see below.

On 10/05/2012 12:34 PM, Enric Balletbo i Serra wrote:
> From: Enric Balletbo i Serra<eballetbo@iseebcn.com>
>
> The MT9V034 is a parallel wide VGA sensor from Aptina (formerly Micron)
> controlled through I2C.
>
> The driver creates a V4L2 subdevice. It currently supports binning and
> cropping, and the gain, auto gain, exposure, auto exposure and test
> pattern controls.
>
> The following patch is based on the MT9V032 driver from Laurent Pinchart
> and was tested on IGEP tech based boards with custom expansion board with
> MT9V034 sensor.
>
> Signed-off-by: Enric Balletbo i Serra<eballetbo@iseebcn.com>
> ---
>   drivers/media/i2c/Kconfig   |   10 +
>   drivers/media/i2c/Makefile  |    1 +
>   drivers/media/i2c/mt9v034.c |  834 +++++++++++++++++++++++++++++++++++++++++++
>   include/media/mt9v034.h     |   15 +
>   4 files changed, 860 insertions(+), 0 deletions(-)
>   create mode 100644 drivers/media/i2c/mt9v034.c
>   create mode 100644 include/media/mt9v034.h
...
> +static int mt9v034_enum_frame_size(struct v4l2_subdev *subdev,
> +				   struct v4l2_subdev_fh *fh,
> +				   struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	if (fse->index>= 8 || fse->code != V4L2_MBUS_FMT_SBGGR10_1X10)
> +		return -EINVAL;
> +
> +	fse->min_width = MT9V034_WINDOW_WIDTH_DEF / fse->index;
> +	fse->max_width = fse->min_width;
> +	fse->min_height = MT9V034_WINDOW_HEIGHT_DEF / fse->index;
> +	fse->max_height = fse->min_height;
> +
> +	return 0;
> +}

Have you actually tested VIDIOC_SUBDEV_ENUM_FRAME_SIZE ioctl with the index
field set to 0 ? Shouldn't (fse->index + 1) be used as a denominator 
instead ?

--

Regards,
Sylwester