Re: [PATCH RFC] [media] add Aptina mt9m114 HD digital image sensor driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Scott,

Thank you for the patch.

On Friday 18 January 2013 17:00:44 Scott Jiang wrote:
> This driver support parallel data output mode and
> QVGA/VGA/WVGA/720P resolution. You can select YCbCr and RGB565
> output format.

What host bridge do you use this driver with ?

> Signed-off-by: Scott Jiang <scott.jiang.linux@gmail.com>
> ---
>  drivers/media/i2c/Kconfig   |   10 +
>  drivers/media/i2c/Makefile  |    1 +
>  drivers/media/i2c/mt9m114.c | 1055 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1066 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/i2c/mt9m114.c

[snip]


> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> new file mode 100644
> index 0000000..564b711
> --- /dev/null
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -0,0 +1,1055 @@
> +/*
> + * mt9m114.c Aptina MT9M114 sensor driver
> + *
> + * Copyright (c) 2012 Analog Devices Inc.
> + *
> + * refer to: SoC Camera driver by Andrew Chew <achew@nvidia.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.

You can remove the last paragraph, it doesn't bring any legal added value, and 
we don't want to patch every source file in the kernel if the FSF moves :-)

> + */

[snip]

> +struct mt9m114_reg {
> +	u16 reg;
> +	u32 val;
> +	int width;
> +};
> +
> +enum {
> +	MT9M114_QVGA,
> +	MT9M114_VGA,
> +	MT9M114_WVGA,
> +	MT9M114_720P,
> +};

This is the part I don't like. Instead of hardcoding 4 different resolutions 
and using large register address/value tables, you should compute the register 
values from the image size requested by the user.

-- 
Regards,

Laurent Pinchart