Re: [PATCH v2] Add support for M5MO-LS 8 Mega Pixel camera

[Sylwester Nawrocki <snjw23@gmail.com>]

HeungJun,

as I didn't really get a chance to review this patch in its current
form before, please let me now add some further comments.  

On 01/22/2011 11:12 AM, Kim, HeungJun wrote:
> Hello,
> 
> This is second patch of I2C/V4L2 subdev driver for M5MOLS 8 Mega Pixel camera
> sensor using MIPI interface.
> 
> The first version patch is here:
> http://www.spinics.net/lists/linux-media/msg26246.html
> 
> And the first verion's issues was corrected referencing with quick reviews of
> Hans Verkuil and Sylwester Nawrocki.
> 
> The main issues is below:
> 1. remove I2C function macro, and use static inline for type-safe.
> 2. use the v4l2 control framework documented at v4l2-control.txt.
> 3. Add regulator enable/disable functions
> 4. fix any coding problems
> 
> The driver supports currently that:
> 1. Preview enables on each resolution and format code V4L2_MBUS_FMT_VYUY8_2X8.
> 2. The 5 kind of control was working well.
> 
> This driver is tested on s5pc210 board using s5p-fimc driver.
> 
> Thanks to any ideas.
> 
> Regrads,
> 	Heungjun Kim
> 
> Signed-off-by: Heungjun Kim<riverful.kim@samsung.com>
> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> ---
>   drivers/media/video/Kconfig                  |    2 +
>   drivers/media/video/Makefile                 |    1 +
>   drivers/media/video/m5mols/Kconfig           |    6 +
>   drivers/media/video/m5mols/Makefile          |    3 +
>   drivers/media/video/m5mols/m5mols.h          |  257 ++++++++
>   drivers/media/video/m5mols/m5mols_controls.c |  173 +++++
>   drivers/media/video/m5mols/m5mols_core.c     |  898 ++++++++++++++++++++++++++
>   drivers/media/video/m5mols/m5mols_reg.h      |  103 +++
>   8 files changed, 1443 insertions(+), 0 deletions(-)
>   create mode 100644 drivers/media/video/m5mols/Kconfig
>   create mode 100644 drivers/media/video/m5mols/Makefile
>   create mode 100644 drivers/media/video/m5mols/m5mols.h
>   create mode 100644 drivers/media/video/m5mols/m5mols_controls.c
>   create mode 100644 drivers/media/video/m5mols/m5mols_core.c
>   create mode 100644 drivers/media/video/m5mols/m5mols_reg.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index ce3555a..fd3130a 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -746,6 +746,8 @@ config VIDEO_NOON010PC30
>   	---help---
>   	  This driver supports NOON010PC30 CIF camera from Siliconfile
> 
> +source "drivers/media/video/m5mols/Kconfig"
> +
>   config SOC_CAMERA
>   	tristate "SoC camera support"
>   	depends on VIDEO_V4L2&&  HAS_DMA&&  I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 251b7ca..adb9361 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
>   obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
>   obj-$(CONFIG_VIDEO_SR030PC30)	+= sr030pc30.o
>   obj-$(CONFIG_VIDEO_NOON010PC30)	+= noon010pc30.o
> +obj-$(CONFIG_VIDEO_M5MOLS)	+= m5mols/
> 
>   obj-$(CONFIG_SOC_CAMERA_IMX074)		+= imx074.o
>   obj-$(CONFIG_SOC_CAMERA_MT9M001)	+= mt9m001.o
> diff --git a/drivers/media/video/m5mols/Kconfig b/drivers/media/video/m5mols/Kconfig
> new file mode 100644
> index 0000000..387425b
> --- /dev/null
> +++ b/drivers/media/video/m5mols/Kconfig
> @@ -0,0 +1,6 @@
> +config VIDEO_M5MOLS
> +	tristate "Fujitsu M5MO-LS 8MP sensor support"
> +	depends on I2C&&  VIDEO_V4L2
> +	---help---
> +	  This driver supports Fujitsu M5MO-LS camera sensor with ISP
> +
> diff --git a/drivers/media/video/m5mols/Makefile b/drivers/media/video/m5mols/Makefile
> new file mode 100644
> index 0000000..b5d19bf
> --- /dev/null
> +++ b/drivers/media/video/m5mols/Makefile
> @@ -0,0 +1,3 @@
> +m5mols-objs	:= m5mols_core.o m5mols_controls.o
> +
> +obj-$(CONFIG_VIDEO_M5MOLS)		+= m5mols.o
> diff --git a/drivers/media/video/m5mols/m5mols.h b/drivers/media/video/m5mols/m5mols.h
> new file mode 100644
> index 0000000..eaeb7ca
> --- /dev/null
> +++ b/drivers/media/video/m5mols/m5mols.h
> @@ -0,0 +1,257 @@
> +/*
> + * Header for M5MOLS 8M Pixel camera sensor with ISP
> + *
> + * Copyright (C) 2011 Samsung Electronics Co., Ltd
> + * Author: HeungJun Kim, riverful.kim@samsung.com
> + *
> + * Copyright (C) 2009 Samsung Electronics Co., Ltd
> + * Author: Dongsoo Nathaniel Kim, dongsoo45.kim@samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef M5MOLS_H
> +#define M5MOLS_H
> +
> +#include<media/v4l2-subdev.h>
> +#include "m5mols_reg.h"
> +
> +#define v4l2msg(fmt, arg...)	do {				\
> +	v4l2_dbg(1, m5mols_debug,&info->sd, fmt, ## arg);	\
> +} while (0)
> +
> +extern int m5mols_debug;
> +
> +enum m5mols_mode {
> +	MODE_SYSINIT,
> +	MODE_PARMSET,
> +	MODE_MONITOR,
> +	MODE_UNKNOWN,
> +};
> +
> +enum m5mols_i2c_size {
> +	I2C_8BIT	= 1,
> +	I2C_16BIT	= 2,
> +	I2C_32BIT	= 4,
> +	I2C_MAX		= 4,
> +};
> +
> +enum m5mols_fps {
> +	M5MOLS_FPS_AUTO	= 0,
> +	M5MOLS_FPS_10	= 10,
> +	M5MOLS_FPS_12	= 12,
> +	M5MOLS_FPS_15	= 15,
> +	M5MOLS_FPS_20	= 20,
> +	M5MOLS_FPS_21	= 21,
> +	M5MOLS_FPS_22	= 22,
> +	M5MOLS_FPS_23	= 23,
> +	M5MOLS_FPS_24	= 24,
> +	M5MOLS_FPS_30	= 30,
> +	M5MOLS_FPS_MAX	= M5MOLS_FPS_30,
> +};
> +
> +enum m5mols_res_type {
> +	M5MOLS_RES_MON,
> +	/* It's not supported below yet. */
> +	M5MOLS_RES_PREVIEW,
> +	M5MOLS_RES_THUMB,
> +	M5MOLS_RES_CAPTURE,
> +	M5MOLS_RES_UNKNOWN,
> +};

Would be better to have ne empty here.

> +struct m5mols_resolution {
> +	u8			value;
> +	enum m5mols_res_type	type;
> +	u16			width;
> +	u16			height;
> +};

Ditto.
> +struct m5mols_format {
> +	enum v4l2_mbus_pixelcode code;
> +	enum v4l2_colorspace colorspace;
> +};
> +
> +struct m5mols_control {
> +	u32	id;
> +	s32	min;
> +	s32	max;
> +	u32	step;
> +	s32	def;
> +};
> +
> +struct m5mols_version {
> +	u8	ctm_code;	/* customer code */
> +	u8	pj_code;	/* project code */
> +	u16	fw;		/* firmware version */
> +	u16	hw;		/* hardware version */
> +	u16	parm;		/* parameter version */
> +	u16	awb;		/* AWB version */
> +};
> +
> +struct m5mols_info {
> +	struct v4l2_subdev		sd;
> +	struct v4l2_mbus_framefmt	fmt;
> +	struct v4l2_fract		tpf;
> +
> +	struct v4l2_ctrl_handler	handle;
> +	struct {
> +		/* support only AE of the Monitor Mode in this version */
> +		struct v4l2_ctrl	*autoexposure;
> +		struct v4l2_ctrl	*exposure;
> +	};
> +	struct v4l2_ctrl		*autowb;
> +	struct v4l2_ctrl		*colorfx;
> +	struct v4l2_ctrl		*saturation;
> +
> +	enum m5mols_mode		mode;
> +	enum m5mols_mode		mode_backup;
> +
> +	struct m5mols_version		ver;
> +	int				gpio_nrst;
> +	int				supply_size;
> +	struct regulator_bulk_data	*supply;
> +	bool				power;
> +	int (*set_power)(struct device *dev, int on);
> +};
> +
> +/* control functions */
> +int m5mols_set_ctrl(struct v4l2_ctrl *ctrl);
> +
> +/* I2C functions - referenced by below I2C helper functions */
> +int m5mols_read_reg(struct v4l2_subdev *sd, enum m5mols_i2c_size size,
> +		u8 category, u8 cmd, u32 *val);
> +int m5mols_write_reg(struct v4l2_subdev *sd, enum m5mols_i2c_size size,
> +		u8 category, u8 cmd, u32 val);
> +int m5mols_check_busy(struct v4l2_subdev *sd,
> +		u8 category, u8 cmd, u32 value);
> +int m5mols_set_mode(struct v4l2_subdev *sd, enum m5mols_mode mode);
> +
> +/*
> + * helper functions
> + */
> +static inline struct m5mols_info *to_m5mols(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct m5mols_info, sd);
> +}
> +
> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return&container_of(ctrl->handler, struct m5mols_info, handle)->sd;
> +}
> +
> +static inline bool is_streaming(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	return (info->mode == MODE_MONITOR) ? true : false;

Just do:
	return info->mode == MODE_MONITOR;
> +}
> +
> +static inline bool is_stoped(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	return (info->mode != MODE_MONITOR) ? true : false;

	return info->mode != MODE_MONITOR;
> +}
> +
> +static inline bool is_powerup(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	return (info->power == true) ? true : false;

"return info->power;" is very enough..

> +}
> +
> +static inline bool is_powerdown(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	return (info->power == false) ? true : false;

	return !info->power;

> +}
> +
> +static inline int m5mols_set_mode_backup(struct v4l2_subdev *sd,
> +		enum m5mols_mode mode)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);

Would be nice to have an empty line here.
> +	info->mode_backup = info->mode;
> +	return m5mols_set_mode(sd, mode);
> +}
> +
> +static inline int m5mols_set_mode_restore(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	int ret;

Ditto.
> +	ret = m5mols_set_mode(sd, info->mode_backup);
> +	if (!ret)
> +		info->mode = info->mode_backup;
> +	return ret;
> +}
> +
> +static inline int __must_check i2c_w8_system(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_8BIT, CAT_SYSTEM, cmd, val);
> +}
> +
> +static inline int __must_check i2c_w8_param(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_8BIT, CAT_PARAM, cmd, val);
> +}
> +
> +static inline int __must_check i2c_w8_mon(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_8BIT, CAT_MON, cmd, val);
> +}
> +
> +static inline int __must_check i2c_w8_ae(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_8BIT, CAT_AE, cmd, val);
> +}
> +
> +static inline int __must_check i2c_w16_ae(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_16BIT, CAT_AE, cmd, val);
> +}
> +
> +static inline int __must_check i2c_w8_wb(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_8BIT, CAT_WB, cmd, val);
> +}
> +
> +static inline int __must_check i2c_w8_flash(struct v4l2_subdev *sd,
> +		u8 cmd, u32 val)
> +{
> +	return m5mols_write_reg(sd, I2C_8BIT, CAT_FLASH, cmd, val);
> +}
> +
> +static inline int __must_check i2c_r8_system(struct v4l2_subdev *sd,
> +		u8 cmd, u32 *val)
> +{
> +	return m5mols_read_reg(sd, I2C_8BIT, CAT_SYSTEM, cmd, val);
> +}
> +
> +static inline int __must_check i2c_r8_param(struct v4l2_subdev *sd,
> +		u8 cmd, u32 *val)
> +{
> +	return m5mols_read_reg(sd, I2C_8BIT, CAT_PARAM, cmd, val);
> +}
> +
> +static inline int __must_check i2c_r8_mon(struct v4l2_subdev *sd,
> +		u8 cmd, u32 *val)
> +{
> +	return m5mols_read_reg(sd, I2C_8BIT, CAT_MON, cmd, val);
> +}
> +
> +static inline int __must_check i2c_r8_ae(struct v4l2_subdev *sd,
> +		u8 cmd, u32 *val)
> +{
> +	return m5mols_read_reg(sd, I2C_8BIT, CAT_AE, cmd, val);
> +}
> +
> +static inline int __must_check i2c_r16_ae(struct v4l2_subdev *sd,
> +		u8 cmd, u32 *val)
> +{
> +	return m5mols_read_reg(sd, I2C_16BIT, CAT_AE, cmd, val);
> +}
> +
> +#endif	/* M5MOLS_H */
> diff --git a/drivers/media/video/m5mols/m5mols_controls.c b/drivers/media/video/m5mols/m5mols_controls.c
> new file mode 100644
> index 0000000..ad148f8
> --- /dev/null
> +++ b/drivers/media/video/m5mols/m5mols_controls.c
> @@ -0,0 +1,173 @@
> +/*
> + * Controls for M5MOLS 8M Pixel camera sensor with ISP
> + *
> + * Copyright (C) 2011 Samsung Electronics Co., Ltd
> + * Author: HeungJun Kim, riverful.kim@samsung.com
> + *
> + * Copyright (C) 2009 Samsung Electronics Co., Ltd
> + * Author: Dongsoo Nathaniel Kim, dongsoo45.kim@samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include<linux/i2c.h>
> +#include<linux/videodev2.h>
> +#include<media/v4l2-ctrls.h>
> +
> +#include "m5mols.h"
> +#include "m5mols_reg.h"
> +
> +static int m5mols_set_ae_lock(struct m5mols_info *info, bool lock)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	static u8 m5mols_ae_lock[] = {		/* false:unlock, true:lock */
> +		[false]		= 0x00,		/* AE unlock */
> +		[true]		= 0x01,		/* AE lock */
> +	};
> +
> +	return i2c_w8_ae(sd, CAT3_AE_LOCK, m5mols_ae_lock[lock]);

How about just doing: 
	return i2c_w8_ae(sd, CAT3_AE_LOCK, (u8)lock);

And then there is no need for the array.
> +}
> +
> +static int m5mols_set_awb_lock(struct m5mols_info *info, bool lock)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	static u8 m5mols_awb_lock[] = {		/* false:unlock, true:lock */
> +		[false]		= 0x00,		/* AWB unlock */
> +		[true]		= 0x01,		/* AWB lock */
> +	};
> +
> +	return i2c_w8_wb(sd, CAT6_AWB_LOCK, m5mols_awb_lock[lock]);

	return i2c_w8_wb(sd, CAT6_AWB_LOCK, (u8)lock);
> +}
> +
> +static int m5mols_wb_mode(struct m5mols_info *info, struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	static u8 m5mols_wb_auto[] = { 0x1, 0x2, };	/* 0:Auto , 1:Manual */
> +	int ret = -EINVAL;

No need for EINVAL. And perhaps some argument check is desired here.

> +
> +	ret = m5mols_set_awb_lock(info, false);
> +	if (!ret)
> +		ret = i2c_w8_wb(sd, CAT6_AWB_MODE, m5mols_wb_auto[ctrl->val]);

	ret = i2c_w8_wb(sd, CAT6_AWB_MODE, ctrl->val + 1);

wouldn't be sufficient?
> +
> +	return ret;
> +}
> +
> +static int m5mols_exposure_mode(struct m5mols_info *info,
> +		struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	static u8 m5mols_ae_mode[] = {
> +		[V4L2_EXPOSURE_MANUAL]	= 0x00,	/* AE off */
> +		[V4L2_EXPOSURE_AUTO]	= 0x01,	/* Exposure all blocks */
> +	};
> +	int ret = -EINVAL;

No need for initalization and missing ctrl->val check.

> +
> +	ret = m5mols_set_ae_lock(info, false);
> +	if (!ret)
> +		ret = i2c_w8_ae(sd, CAT3_AE_MODE, m5mols_ae_mode[ctrl->val]);
> +
> +	return ret;
> +}
> +
> +static int m5mols_exposure(struct m5mols_info *info)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +
> +	return i2c_w16_ae(sd, CAT3_MANUAL_GAIN_MON_1, info->exposure->val);
> +}
> +
> +static int m5mols_set_saturation(struct m5mols_info *info,
> +		struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	static u8 m5mols_chroma_lvl[] = {
> +		0x1c, 0x3e, 0x5f, 0x80, 0xa1, 0xc2, 0xe4,
> +	};
> +	int ret = -EINVAL;

No need to initialize "ret" and I would definitely be checking
ctrl->val's value before indexing an array with it.. 

> +
> +	ret = i2c_w8_mon(sd, CAT2_CHROMA_LVL, m5mols_chroma_lvl[ctrl->val]);
> +	if (!ret)
> +		ret = i2c_w8_mon(sd, CAT2_CHROMA_EN, true);
> +
> +	return ret;
> +}
> +
> +static int m5mols_set_colorfx(struct m5mols_info *info, struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	static u8 m5mols_effects_gamma[] = {	/* cat 1: Effects */
> +		[V4L2_COLORFX_NEGATIVE]		= 0x01,
> +		[V4L2_COLORFX_EMBOSS]		= 0x06,
> +		[V4L2_COLORFX_SKETCH]		= 0x07,
> +	};
> +	static u8 m5mols_cfixb_chroma[] = {	/* cat 2: Cr for effect */
> +		[V4L2_COLORFX_BW]		= 0x0,
> +		[V4L2_COLORFX_SEPIA]		= 0xd8,
> +		[V4L2_COLORFX_SKY_BLUE]		= 0x40,
> +		[V4L2_COLORFX_GRASS_GREEN]	= 0xe0,
> +	};
> +	static u8 m5mols_cfixr_chroma[] = {	/* cat 2: Cb for effect */
> +		[V4L2_COLORFX_BW]		= 0x0,
> +		[V4L2_COLORFX_SEPIA]		= 0x18,
> +		[V4L2_COLORFX_SKY_BLUE]		= 0x00,
> +		[V4L2_COLORFX_GRASS_GREEN]	= 0xe0,
> +	};
> +	int ret = -EINVAL;
> +
> +	switch (ctrl->val) {
> +	case V4L2_COLORFX_NONE:
> +		if (!ret)
> +			ret = i2c_w8_mon(sd, CAT2_COLOR_EFFECT, false);

Isn't it an unreachable code?

> +		return ret;
> +	case V4L2_COLORFX_BW:		/* chroma: Gray */
> +	case V4L2_COLORFX_SEPIA:	/* chroma: Sepia */
> +	case V4L2_COLORFX_SKY_BLUE:	/* chroma: Blue */
> +	case V4L2_COLORFX_GRASS_GREEN:	/* chroma: Green */
> +		ret = i2c_w8_mon(sd, CAT2_CFIXB,
> +				m5mols_cfixb_chroma[ctrl->val]);
> +		if (!ret)
> +			ret = i2c_w8_mon(sd, CAT2_CFIXR,
> +					m5mols_cfixr_chroma[ctrl->val]);
> +		if (!ret)
> +			ret = i2c_w8_mon(sd, CAT2_COLOR_EFFECT, true);
> +		return ret;
> +	case V4L2_COLORFX_NEGATIVE:	/* gamma: Negative */
> +	case V4L2_COLORFX_EMBOSS:	/* gamma: Emboss */
> +	case V4L2_COLORFX_SKETCH:	/* gamma: Outline */
> +		ret = i2c_w8_param(sd, CAT1_EFFECT,
> +				m5mols_effects_gamma[ctrl->val]);
> +		if (!ret)
> +			ret = i2c_w8_mon(sd, CAT2_COLOR_EFFECT, true);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +int m5mols_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	struct m5mols_info *info = to_m5mols(sd);
> +	int ret = 0;

A superfluous initialization.

> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_EXPOSURE_AUTO:
> +		if (!ctrl->has_new)
> +			ctrl->val = V4L2_EXPOSURE_MANUAL;
> +		ret = m5mols_exposure_mode(info, ctrl);
> +		if (!ret&&  ctrl->val == V4L2_EXPOSURE_MANUAL)
> +			ret = m5mols_exposure(info);
> +		return ret;
> +	case V4L2_CID_AUTO_WHITE_BALANCE:
> +		return m5mols_wb_mode(info, ctrl);
> +	case V4L2_CID_SATURATION:
> +		return m5mols_set_saturation(info, ctrl);
> +	case V4L2_CID_COLORFX:
> +		return m5mols_set_colorfx(info, ctrl);
> +	}
> +
> +	return -EINVAL;
> +}
> diff --git a/drivers/media/video/m5mols/m5mols_core.c b/drivers/media/video/m5mols/m5mols_core.c
> new file mode 100644
> index 0000000..3f25ae4
> --- /dev/null
> +++ b/drivers/media/video/m5mols/m5mols_core.c
> @@ -0,0 +1,898 @@
> +/*
> + * Driver for M5MOLS 8M Pixel camera sensor with ISP
> + *
> + * Copyright (C) 2011 Samsung Electronics Co., Ltd
> + * Author: HeungJun Kim, riverful.kim@samsung.com
> + *
> + * Copyright (C) 2009 Samsung Electronics Co., Ltd
> + * Author: Dongsoo Nathaniel Kim, dongsoo45.kim@samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include<linux/i2c.h>
> +#include<linux/irq.h>
> +#include<linux/interrupt.h>
> +#include<linux/delay.h>
> +#include<linux/version.h>
> +#include<linux/gpio.h>
> +#include<linux/regulator/consumer.h>
> +
> +#include<linux/videodev2.h>
> +#include<media/v4l2-ctrls.h>
> +#include<media/v4l2-device.h>
> +#include<media/v4l2-subdev.h>
> +#include<media/m5mols.h>

It looks like the driver's public header file is missing in the patch,
include/media/m5mols.h?

..and the copiler is not very happy about that:

  CC      drivers/media/video/m5mols/m5mols_core.o
drivers/media/video/m5mols/m5mols_core.c:28: fatal error: media/m5mols.h: No such file or directory
compilation terminated.

> +
> +#include "m5mols.h"
> +#include "m5mols_reg.h"
> +
> +int m5mols_debug;
> +
> +module_param(m5mols_debug, int, 0644);
> +
> +#define MOD_NAME		"M5MOLS"
> +#define M5MOLS_I2C_CHECK_RETRY	50
> +
> +/* M5MOLS mode */
> +static u8 m5mols_reg_mode[] = {
> +	[MODE_SYSINIT]		= 0x00,
> +	[MODE_PARMSET]		= 0x01,
> +	[MODE_MONITOR]		= 0x02,
> +	[MODE_UNKNOWN]		= 0xff,
> +};
> +
> +/* M5MOLS regulator consumer names */
> +static const char *supply_names[] = {
> +	/* The DEFAULT names of power are referenced with M5MO datasheet. */
> +	"core",		/* core power - 1.2v, generally at the M5MOLS */
> +	"d_sensor",	/* sensor power 1 - 1.8v */
> +	"dig_18",	/* digital power 1 - 1.8v */
> +	"dig_28",	/* digital power 2 - 2.8v */
> +	"a_sensor",	/* analog power */
> +	"dig_12",	/* digital power 3 - 1.2v */
> +};
> +
> +/* M5MOLS default format (codes, sizes, preset values) */
> +static const struct v4l2_mbus_framefmt default_fmt = {
> +	.width		= 1920,
> +	.height		= 1080,
> +	.code		= V4L2_MBUS_FMT_VYUY8_2X8,
> +	.field		= V4L2_FIELD_NONE,
> +	.colorspace	= V4L2_COLORSPACE_JPEG,
> +};
> +static const struct m5mols_format m5mols_formats[] = {
> +	{
> +		.code		= V4L2_MBUS_FMT_VYUY8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +	},
> +};
> +static const struct m5mols_resolution m5mols_resolutions[] = {
> +	/* monitor size */
> +	{ 0x01, M5MOLS_RES_MON, 128, 96 },	/* SUB-QCIF */
> +	{ 0x03, M5MOLS_RES_MON, 160, 120 },	/* QQVGA */
> +	{ 0x05, M5MOLS_RES_MON, 176, 144 },	/* QCIF */
> +	{ 0x06, M5MOLS_RES_MON, 176, 176 },	/* 176*176 */
> +	{ 0x08, M5MOLS_RES_MON, 240, 320 },	/* 1 QVGA */
> +	{ 0x09, M5MOLS_RES_MON, 320, 240 },	/* QVGA */
> +	{ 0x0c, M5MOLS_RES_MON, 240, 400 },	/* l WQVGA */
> +	{ 0x0d, M5MOLS_RES_MON, 400, 240 },	/* WQVGA */
> +	{ 0x0e, M5MOLS_RES_MON, 352, 288 },	/* CIF */
> +	{ 0x13, M5MOLS_RES_MON, 480, 360 },	/* 480*360 */
> +	{ 0x15, M5MOLS_RES_MON, 640, 360 },	/* qHD */
> +	{ 0x17, M5MOLS_RES_MON, 640, 480 },	/* VGA */
> +	{ 0x18, M5MOLS_RES_MON, 720, 480 },	/* 720x480 */
> +	{ 0x1a, M5MOLS_RES_MON, 800, 480 },	/* WVGA */
> +	{ 0x1f, M5MOLS_RES_MON, 800, 600 },	/* SVGA */
> +	{ 0x21, M5MOLS_RES_MON, 1280, 720 },	/* HD */
> +	{ 0x25, M5MOLS_RES_MON, 1920, 1080 },	/* 1080p */
> +	{ 0x29, M5MOLS_RES_MON, 3264, 2448 },	/* 8M (2.63fps@3264*2448) */
> +	{ 0x30, M5MOLS_RES_MON, 320, 240 },	/* 60fps for slow motion */
> +	{ 0x31, M5MOLS_RES_MON, 320, 240 },	/* 120fps for slow motion */
> +	{ 0x39, M5MOLS_RES_MON, 800, 602 },	/* AHS_MON debug */
> +};
> +
> +/* M5MOLS default FPS */
> +static const struct v4l2_fract default_fps = {
> +	.numerator		= 1,
> +	.denominator		= M5MOLS_FPS_AUTO,
> +};

Can you separate those with an empty line too?

> +static u8 m5mols_reg_fps[] = {
> +	[M5MOLS_FPS_AUTO]	= 0x01,
> +	[M5MOLS_FPS_10]		= 0x05,
> +	[M5MOLS_FPS_12]		= 0x04,
> +	[M5MOLS_FPS_15]		= 0x03,
> +	[M5MOLS_FPS_20]		= 0x08,
> +	[M5MOLS_FPS_21]		= 0x09,
> +	[M5MOLS_FPS_22]		= 0x0a,
> +	[M5MOLS_FPS_23]		= 0x0b,
> +	[M5MOLS_FPS_24]		= 0x07,
> +	[M5MOLS_FPS_30]		= 0x02,
> +};
> +
> +static u32 m5mols_swap_byte(u8 *data, enum m5mols_i2c_size size)
> +{
> +	if (size == I2C_8BIT)
> +		return *data;
> +	else if (size == I2C_16BIT)
> +		return be16_to_cpu(*((u16 *)data));
> +	else
> +		return be32_to_cpu(*((u32 *)data));
> +}
> +
> +/*
> + * m5mols_read_reg/m5mols_write_reg - handle sensor's I2C communications.
> + *
> + * The I2C command packet of M5MOLS is made up 3 kinds of I2C bytes(category,
> + * command, bytes). Reference m5mols.h.
> + *
> + * The packet is needed 2, when M5MOLS is read through I2C.
> + * The packet is needed 1, when M5MOLS is written through I2C.
> + *
> + * I2C packet common order(including both reading/writing)
> + *   1st : size (data size + 4)
> + *   2nd : READ/WRITE (R - 0x01, W - 0x02)
> + *   3rd : Category
> + *   4th : Command
> + *
> + * I2C packet order for READING operation
> + *   5th : data real size for reading
> + *   And, read another I2C packet again, until data size.
> + *
> + * I2C packet order for WRITING operation
> + *   5th to 8th: an actual data to write
> + */
> +
> +#define M5MOLS_BYTE_READ	0x01
> +#define M5MOLS_BYTE_WRITE	0x02
> +
> +int m5mols_read_reg(struct v4l2_subdev *sd,
> +		enum m5mols_i2c_size size,
> +		u8 category, u8 cmd, u32 *val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct i2c_msg msg[1];
> +	u8 wbuf[5], rbuf[I2C_MAX + 1];
> +	int ret;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	if (size != I2C_8BIT&&  size != I2C_16BIT&&  size != I2C_32BIT)
> +		return -EINVAL;
> +
> +	/* 1st I2C operation, for writing info to read. */
> +	msg->addr = client->addr;
> +	msg->flags = 0;
> +	msg->len = 5;		/* 1(cmd size per bytes) + 4 */
> +	msg->buf = wbuf;
> +	wbuf[0] = 5;		/* 1(cmd size per bytes) + 4 */
> +	wbuf[1] = M5MOLS_BYTE_READ;
> +	wbuf[2] = category;
> +	wbuf[3] = cmd;
> +	wbuf[4] = size;
> +
> +	ret = i2c_transfer(client->adapter, msg, 1);
> +	if (ret<  0) {
> +		dev_err(&client->dev, "failed READ-1[%d] at "
> +				"cat[%02x] cmd[%02x]\n",
> +				size, category, cmd);
> +		return ret;
> +	}
> +
> +	/* 2nd I2C operation, for reading data. */
> +	msg->addr = client->addr;
> +	msg->flags = I2C_M_RD;
> +	msg->len = size + 1;
> +	msg->buf = rbuf;
> +
> +	ret = i2c_transfer(client->adapter, msg, 1);
> +	if (ret<  0) {
> +		dev_err(&client->dev, "failed READ-2[%d] at "
> +				"cat[%02x] cmd[%02x]\n",
> +				size, category, cmd);
> +		return ret;
> +	}
> +
> +	*val = m5mols_swap_byte(&rbuf[1], size);
> +
> +	mdelay(15);	/* must be for stabilization */

I'm strongly suggesting to try to replace the busy waiting mdelay
with usleep_range here, which is more appropriate for obtaining
delays of this order. It looks a bit wasteful to hog CPU for such
a long time.

> +
> +	return 0;
> +}
> +
> +int m5mols_write_reg(struct v4l2_subdev *sd,
> +		enum m5mols_i2c_size size,
> +		u8 category, u8 cmd, u32 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct device *cdev =&client->dev;
> +	struct i2c_msg msg[1];
> +	u8 wbuf[I2C_MAX + 4];
> +	u32 *buf = (u32 *)&wbuf[4];
> +	int ret;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	if (size != I2C_8BIT&&  size != I2C_16BIT&&  size != I2C_32BIT) {
> +		dev_err(cdev, "Wrong data size\n");
> +		return -EINVAL;
> +	}
> +
> +	msg->addr = client->addr;
> +	msg->flags = 0;
> +	msg->len = size + 4;
> +	msg->buf = wbuf;
> +	wbuf[0] = size + 4;
> +	wbuf[1] = M5MOLS_BYTE_WRITE;
> +	wbuf[2] = category;
> +	wbuf[3] = cmd;
> +
> +	*buf = m5mols_swap_byte((u8 *)&val, size);
> +
> +	ret = i2c_transfer(client->adapter, msg, 1);
> +	if (ret<  0) {
> +		dev_err(&client->dev, "failed WRITE[%d] at "
> +				"cat[%02x] cmd[%02x], ret %d\n",
> +				size, msg->buf[2], msg->buf[3], ret);
> +		return ret;
> +	}
> +
> +	mdelay(15);	/* must be for stabilization */

Ditto.
> +
> +	return 0;
> +}
> +
> +int m5mols_check_busy(struct v4l2_subdev *sd,
> +		u8 category, u8 cmd, u32 value)
> +{
> +	u32 busy, i;
> +	int ret;
> +
> +	for (i = 0; i<  M5MOLS_I2C_CHECK_RETRY; i++) {
> +		ret = m5mols_read_reg(sd, I2C_8BIT, category, cmd,&busy);
> +		if (ret<  0)
> +			return ret;
> +
> +		if (busy == value)	/* bingo */
> +			return 0;
> +
> +		mdelay(1);
> +	}
> +
> +	return -EBUSY;
> +}
> +
> +/*
> + * m5mols_set_mode - change and set mode of M5MOLS.
> + *
> + * This driver supports now only 3 modes(System, Monitor, Parameter).
> + */
> +int m5mols_set_mode(struct v4l2_subdev *sd, enum m5mols_mode mode)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct device *cdev =&client->dev;
> +	const char *m5mols_str_mode[] = {
> +		"System initialization",
> +		"Parameter setting",
> +		"Monitor setting",
> +		"Unknown",
> +	};
> +	int ret = 0;
> +
> +	if (mode<  MODE_SYSINIT || mode>  MODE_UNKNOWN)
> +		return -EINVAL;
> +
> +	ret = i2c_w8_system(sd, CAT0_SYSMODE, m5mols_reg_mode[mode]);
> +	if (!ret)
> +		ret = m5mols_check_busy(sd, CAT_SYSTEM, CAT0_SYSMODE,
> +				m5mols_reg_mode[mode]);
> +	if (ret<  0)
> +		return ret;
> +
> +	info->mode = m5mols_reg_mode[mode];
> +	dev_dbg(cdev, " mode: %s\n", m5mols_str_mode[mode]);
> +
> +	return ret;
> +}
> +
> +/*
> + * get_version - get M5MOLS sensor versions.
> + */
> +static int get_version(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	union {
> +		struct m5mols_version	ver;
> +		u8			bytes[10];
> +	} value;
> +	int ret, i;
> +
> +	for (i = CAT0_CUSTOMER_CODE; i<= CAT0_VERSION_AWB_L; i++) {
> +		ret = i2c_r8_system(sd, i, (u32 *)&value.bytes[i]);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	info->ver = value.ver;
> +
> +	info->ver.fw = be16_to_cpu(info->ver.fw);
> +	info->ver.hw = be16_to_cpu(info->ver.hw);
> +	info->ver.parm = be16_to_cpu(info->ver.parm);
> +	info->ver.awb = be16_to_cpu(info->ver.awb);
> +
> +	return ret;
> +}
> +
> +static void m5mols_show_version(struct v4l2_subdev *sd)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct device *dev =&client->dev;
> +	struct m5mols_info *info = to_m5mols(sd);
> +
> +	dev_info(dev, "customer code\t0x%02x\n", info->ver.ctm_code);
> +	dev_info(dev, "project code\t0x%02x\n", info->ver.pj_code);
> +	dev_info(dev, "firmware version\t0x%04x\n", info->ver.fw);
> +	dev_info(dev, "hardware version\t0x%04x\n", info->ver.hw);
> +	dev_info(dev, "parameter version\t0x%04x\n", info->ver.parm);
> +	dev_info(dev, "AWB version\t0x%04x\n", info->ver.awb);
> +}
> +
> +/*
> + * get_res_preset - find out M5MOLS register value from requested resolution.
> + *
> + * @width: requested width
> + * @height: requested height
> + * @type: requested type of each modes. It supports only monitor mode now.
> + */
> +static int get_res_preset(struct v4l2_subdev *sd, u16 width, u16 height,
> +		enum m5mols_res_type type)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	int i;
> +
> +	for (i = 0; i<  ARRAY_SIZE(m5mols_resolutions); i++) {
> +		if ((m5mols_resolutions[i].type == type)&&
> +			(m5mols_resolutions[i].width == width)&&
> +			(m5mols_resolutions[i].height == height))
> +			break;
> +	}
> +
> +	if (i>= ARRAY_SIZE(m5mols_resolutions)) {
> +		v4l2msg("no matching resolution\n");
> +		return -EINVAL;
> +	}
> +
> +	return m5mols_resolutions[i].value;
> +}
> +
> +/*
> + * get_fps - calc&  check FPS from v4l2_captureparm, if FPS is adequate, set.
> + *
> + * In M5MOLS case, the denominator means FPS. The each value of numerator and
> + * denominator should not be minus. If numerator is 0, it sets AUTO FPS. If
> + * numerator is not 1, it recalculates denominator. After it checks, the
> + * denominator is set to timeperframe.denominator, and used by FPS.
> + */
> +static int get_fps(struct v4l2_subdev *sd,
> +		struct v4l2_captureparm *parm)
> +{
> +	int numerator = parm->timeperframe.numerator;
> +	int denominator = parm->timeperframe.denominator;
> +
> +	/* The denominator should be +, except 0. The numerator shoud be +. */
> +	if (numerator<  0 || denominator<= 0)
> +		return -EINVAL;
> +
> +	/* The numerator is 0, return auto fps. */
> +	if (numerator == 0) {
> +		parm->timeperframe.denominator = M5MOLS_FPS_AUTO;
> +		return 0;
> +	}
> +
> +	/* calc FPS(not time per frame) per 1 numerator */
> +	denominator = denominator / numerator;
> +
> +	if (denominator<  M5MOLS_FPS_AUTO || denominator>  M5MOLS_FPS_MAX)
> +		return -EINVAL;
> +
> +	if (!m5mols_reg_fps[denominator])
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int m5mols_g_mbus_fmt(struct v4l2_subdev *sd,
> +		struct v4l2_mbus_framefmt *ffmt)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +
> +	*ffmt = info->fmt;
> +
> +	return 0;
> +}
> +
> +static int m5mols_s_mbus_fmt(struct v4l2_subdev *sd,
> +		struct v4l2_mbus_framefmt *ffmt)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	int size;
> +	int ret = -EINVAL;
> +
> +	size = get_res_preset(sd, ffmt->width, ffmt->height,
> +			M5MOLS_RES_MON);
> +	if (size<  0)
> +		return -EINVAL;
> +
> +	ret = m5mols_set_mode(sd, MODE_PARMSET);
> +	if (!ret)
> +		ret = i2c_w8_param(sd, CAT1_MONITOR_SIZE, (u8)size);
> +	if (!ret) {
> +		info->fmt = default_fmt;
> +		info->fmt.width = ffmt->width;
> +		info->fmt.height = ffmt->height;
> +
> +		*ffmt = info->fmt;
> +	}
> +
> +	return ret;
> +}
> +
> +static int m5mols_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +			      enum v4l2_mbus_pixelcode *code)
> +{
> +	if (!code || index>= ARRAY_SIZE(m5mols_formats))
> +		return -EINVAL;
> +
> +	*code = m5mols_formats[index].code;
> +
> +	return 0;
> +}
> +
> +static int m5mols_g_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	struct v4l2_captureparm *cp =&parms->parm.capture;
> +
> +	if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE&&
> +			parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		return -EINVAL;
> +
> +	cp->capability = V4L2_CAP_TIMEPERFRAME;
> +	cp->timeperframe = info->tpf;
> +
> +	return 0;
> +}
> +
> +static int m5mols_s_parm(struct v4l2_subdev *sd, struct v4l2_streamparm *parms)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	struct v4l2_captureparm *cp =&parms->parm.capture;
> +	int ret = -EINVAL;
> +
> +	if (parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE&&
> +			parms->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +		return -EINVAL;
> +
> +	ret = m5mols_set_mode_backup(sd, MODE_PARMSET);
> +	if (!ret)
> +		ret = get_fps(sd, cp);	/* set right FPS to denominator. */
> +	if (!ret)
> +		ret = i2c_w8_param(sd, CAT1_MONITOR_FPS,
> +				m5mols_reg_fps[cp->timeperframe.denominator]);
> +	if (!ret)
> +		ret = m5mols_set_mode_restore(sd);
> +	if (!ret) {
> +		cp->capability = V4L2_CAP_TIMEPERFRAME;
> +		info->tpf = cp->timeperframe;
> +	}
> +
> +	v4l2msg("denominator: %d / numerator: %d.\n",
> +		cp->timeperframe.denominator, cp->timeperframe.numerator);
> +
> +	return ret;
> +}
> +
> +static int m5mols_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	if (enable) {
> +		if (is_stoped(sd))
> +			return m5mols_set_mode(sd, MODE_MONITOR);
> +		else
> +			return -EINVAL;
> +	} else {
> +		if (is_streaming(sd))
> +			return m5mols_set_mode(sd, MODE_PARMSET);
> +		else
> +			return -EINVAL;
> +	}
> +}
> +
> +static const struct v4l2_subdev_video_ops m5mols_video_ops = {
> +	.g_mbus_fmt		= m5mols_g_mbus_fmt,
> +	.s_mbus_fmt		= m5mols_s_mbus_fmt,
> +	.enum_mbus_fmt		= m5mols_enum_mbus_fmt,
> +	.g_parm			= m5mols_g_parm,
> +	.s_parm			= m5mols_s_parm,
> +	.s_stream		= m5mols_s_stream,
> +};
> +
> +static int m5mols_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	int ret = 0;

No need to initialize ret to 0.
> +
> +	ret = m5mols_set_mode_backup(sd, MODE_PARMSET);
> +	if (!ret)
> +		ret = m5mols_set_ctrl(ctrl);
> +	if (!ret)
> +		ret = m5mols_set_mode_restore(sd);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops m5mols_ctrl_ops = {
> +	.s_ctrl = m5mols_s_ctrl,
> +};
> +
> +
> +/*
> + * m5mols_sensor_power - handle sensor power up/down.
> + *
> + * @enable: If it is true, power up. If is not, power down.
> + */
> +static int m5mols_sensor_power(struct m5mols_info *info, bool enable)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	struct i2c_client *c = v4l2_get_subdevdata(sd);
> +	int ret = 0;

Ditto.
> +
> +	if (enable) {
> +		if (!is_powerdown(sd))
> +			return 0;
> +
> +		if (gpio_is_valid(info->gpio_nrst))
> +			gpio_set_value(info->gpio_nrst, 0);
> +
> +		if (info->set_power) {
> +			ret = info->set_power(&c->dev, 1);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		ret = regulator_bulk_enable(info->supply_size,
> +				info->supply);
> +		if (ret)
> +			return ret;
> +
> +		if (gpio_is_valid(info->gpio_nrst)) {
> +			gpio_set_value(info->gpio_nrst, 0);
> +			msleep(100);
> +			gpio_set_value(info->gpio_nrst, 1);
> +			msleep(100);
> +		}
> +
> +		info->power = true;
> +	} else {
> +		if (!is_powerup(sd))
> +			return 0;
> +
> +		if (gpio_is_valid(info->gpio_nrst)) {
> +			gpio_set_value(info->gpio_nrst, 0);
> +			msleep(100);
> +		}
> +
> +		ret = regulator_bulk_disable(info->supply_size,
> +				info->supply);
> +		if (ret)
> +			return ret;
> +
> +		if (info->set_power) {
> +			ret = info->set_power(&c->dev, 0);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (gpio_is_valid(info->gpio_nrst))
> +			gpio_set_value(info->gpio_nrst, 0);
> +
> +		info->power = false;
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * m5mols_sensor_armboot - booting M5MOLS internal ARM core-controller.
> + *
> + * It makes to ready M5MOLS for I2C&  MIPI interface. After it's powered up,
> + * it activates if it gets armboot command for I2C interface. After getting
> + * cmd, it must wait about least 500ms referenced by M5MOLS datasheet.
> + */
> +static int m5mols_sensor_armboot(struct v4l2_subdev *sd)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	static u8 m5mols_mipi_value = 0x02;
> +	int ret = -EINVAL;

Superfluous initialization.

> +
> +	/* 1. ARM booting */
> +	ret = i2c_w8_flash(sd, CAT0_INT_ROOTEN, true);
> +	if (ret<  0)
> +		return ret;
> +
> +	msleep(500);
> +	dev_dbg(&client->dev, "Success ARM Booting\n");
> +
> +	ret = m5mols_set_mode(sd, MODE_PARMSET);
> +	if (!ret)
> +		ret = get_version(sd);
> +	if (!ret)
> +		ret = i2c_w8_param(sd, CAT1_DATA_INTERFACE, m5mols_mipi_value);
> +
> +	m5mols_show_version(sd);
> +
> +	return ret;
> +}
> +
> +/*
> + * m5mols_init_controls - initialization using v4l2_ctrl.
> + */
> +static int m5mols_init_controls(struct m5mols_info *info)
> +{
> +	struct v4l2_subdev *sd =&info->sd;
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	int ret = -EINVAL;

Ditto.

> +	u16 max_ex_mon;
> +
> +	/* check minimum&  maximum of M5MOLS controls */
> +	ret = i2c_r16_ae(sd, CAT3_MAX_GAIN_MON_1, (u32 *)&max_ex_mon);
> +	if (!ret)
> +		return ret;
> +
> +	/* set the controls using v4l2 control frameworks */
> +	v4l2_ctrl_handler_init(&info->handle, 5);
> +
> +	info->colorfx = v4l2_ctrl_new_std_menu(&info->handle,
> +			&m5mols_ctrl_ops, V4L2_CID_COLORFX,
> +			9, 1, V4L2_COLORFX_NONE);
> +	info->autoexposure = v4l2_ctrl_new_std_menu(&info->handle,
> +			&m5mols_ctrl_ops, V4L2_CID_EXPOSURE_AUTO,
> +			1, 0, V4L2_EXPOSURE_AUTO);
> +	info->exposure = v4l2_ctrl_new_std(&info->handle,
> +			&m5mols_ctrl_ops, V4L2_CID_EXPOSURE,
> +			0, max_ex_mon, 1, (int)max_ex_mon/2);
> +	info->autowb = v4l2_ctrl_new_std(&info->handle,
> +			&m5mols_ctrl_ops, V4L2_CID_AUTO_WHITE_BALANCE,
> +			0, 1, 1, 1);
> +	info->saturation = v4l2_ctrl_new_std(&info->handle,
> +			&m5mols_ctrl_ops, V4L2_CID_SATURATION,
> +			0, 6, 1, 3);
> +
> +	sd->ctrl_handler =&info->handle;
> +	if (info->handle.error) {
> +		dev_err(&client->dev, "Failed to init controls, %d\n", ret);
> +		v4l2_ctrl_handler_free(&info->handle);
> +		return info->handle.error;
> +	}
> +
> +	v4l2_ctrl_cluster(2,&info->autoexposure);
> +	v4l2_ctrl_handler_setup(&info->handle);
> +
> +	return 0;
> +}
> +
> +/*
> + * m5mols_setup_default - set default size&  fps in the monitor mode.
> + */
> +static int m5mols_setup_default(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	int value;
> +	int ret = -EINVAL;
> +
> +	value = get_res_preset(sd, default_fmt.width, default_fmt.height,
> +			M5MOLS_RES_MON);
> +	if (value>= 0)
> +		ret = i2c_w8_param(sd, CAT1_MONITOR_SIZE, (u8)value);
> +	if (!ret)
> +		ret = i2c_w8_param(sd, CAT1_MONITOR_FPS,
> +			m5mols_reg_fps[default_fps.denominator]);
> +	if (!ret)
> +		ret = m5mols_init_controls(info);
> +	if (!ret) {
> +		info->fmt = default_fmt;
> +		info->tpf = default_fps;
> +
> +		ret = 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static int m5mols_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +	int ret = 0;

Ditto.
> +
> +	if (on) {
> +		ret = m5mols_sensor_power(info, true);
> +		if (!ret)
> +			ret = m5mols_sensor_armboot(sd);
> +		if (!ret)
> +			ret = m5mols_setup_default(sd);
> +	} else {
> +		ret = m5mols_sensor_power(info, false);
> +	}
> +
> +	return ret;
> +}
> +
> +static int m5mols_log_status(struct v4l2_subdev *sd)
> +{
> +	struct m5mols_info *info = to_m5mols(sd);
> +
> +	v4l2_ctrl_handler_log_status(&info->handle, sd->name);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops m5mols_core_ops = {
> +	.s_power		= m5mols_s_power,
> +	.g_ctrl			= v4l2_subdev_g_ctrl,
> +	.s_ctrl			= v4l2_subdev_s_ctrl,
> +	.queryctrl		= v4l2_subdev_queryctrl,
> +	.querymenu		= v4l2_subdev_querymenu,
> +	.g_ext_ctrls		= v4l2_subdev_g_ext_ctrls,
> +	.try_ext_ctrls		= v4l2_subdev_try_ext_ctrls,
> +	.s_ext_ctrls		= v4l2_subdev_s_ext_ctrls,
> +	.log_status		= m5mols_log_status,
> +};
> +
> +static const struct v4l2_subdev_ops m5mols_ops = {
> +	.core	=&m5mols_core_ops,
> +	.video	=&m5mols_video_ops,
> +};
> +
> +static int m5mols_get_gpio(struct m5mols_info *info,
> +	const struct m5mols_platform_data *pdata)
> +{
> +
> +	if (!gpio_is_valid(pdata->gpio_nrst))
> +		return -EINVAL;
> +
> +	if (gpio_request(pdata->gpio_nrst, "M5MOLS-NRST"))
> +		return -EINVAL;
> +
> +	info->gpio_nrst = pdata->gpio_nrst;
> +	gpio_direction_output(info->gpio_nrst, 0);
> +	gpio_export(info->gpio_nrst, 0);
> +
> +	return 0;
> +}
> +
> +static int m5mols_get_regulators(struct m5mols_info *info,
> +		const struct m5mols_platform_data *pdata,
> +		struct i2c_client *c)
> +{
> +	int i = 0;
> +
> +	info->supply = kzalloc(sizeof(struct regulator_bulk_data) *
> +			ARRAY_SIZE(supply_names), GFP_KERNEL);

Isn't it more clean to just embed

struct regulator_bulk_data supply[ARRAY_SIZE(supply_names)];

in struct m5mols_info and do not bother with dynamic allocation?

> +	if (!info->supply)
> +		return -ENOMEM;
> +
> +	info->supply_size = ARRAY_SIZE(supply_names);
> +	for (i = 0; i<  info->supply_size; i++)
> +		info->supply[i].supply = supply_names[i];
> +
> +	return regulator_bulk_get(&c->dev, info->supply_size, info->supply);
> +}
> +
> +static int m5mols_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	const struct m5mols_platform_data *pdata =
> +		client->dev.platform_data;
> +	struct m5mols_info *info;
> +	struct v4l2_subdev *sd;
> +	int ret = 0;
> +
> +	if (pdata == NULL) {
> +		dev_err(&client->dev, "No platform data\n");
> +		return -EIO;
> +	}
> +
> +	info = kzalloc(sizeof(struct m5mols_info), GFP_KERNEL);
> +	if (info == NULL) {
> +		dev_err(&client->dev, "Failed to allocate info\n");
> +		return -ENOMEM;
> +	}
> +
> +	info->set_power = pdata->set_power;
> +
> +	ret = m5mols_get_gpio(info, pdata);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to set gpio, %d\n", ret);
> +		goto out_gpio;
> +	}
> +
> +	ret = m5mols_get_regulators(info, pdata, client);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to get regulators, %d\n", ret);
> +		goto out_reg;
> +	}
> +
> +	sd =&info->sd;
> +	strlcpy(sd->name, MOD_NAME, sizeof(sd->name));
> +	v4l2_i2c_subdev_init(sd, client,&m5mols_ops);
> +
> +	v4l2msg("probed m5mols driver.\n");
> +
> +	return 0;
> +
> +out_reg:
> +	regulator_bulk_free(info->supply_size, info->supply);
> +	kfree(info->supply);
> +out_gpio:
> +	if (gpio_is_valid(info->gpio_nrst))
> +		gpio_free(info->gpio_nrst);
> +	kfree(info);
> +
> +	return ret;
> +}
> +
> +static int m5mols_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct m5mols_info *info = to_m5mols(sd);
> +
> +	v4l2_device_unregister_subdev(sd);
> +	v4l2_ctrl_handler_free(&info->handle);
> +
> +	regulator_bulk_free(info->supply_size, info->supply);
> +	if (gpio_is_valid(info->gpio_nrst))
> +		gpio_free(info->gpio_nrst);
> +
> +	kfree(info->supply);
> +	kfree(info);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id m5mols_id[] = {
> +	{ MOD_NAME, 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, m5mols_id);
> +
> +static struct i2c_driver m5mols_i2c_driver = {
> +	.driver = {
> +		.name	= MOD_NAME,
> +	},
> +	.probe		= m5mols_probe,
> +	.remove		= m5mols_remove,
> +	.id_table	= m5mols_id,
> +};
> +
> +static int __init m5mols_mod_init(void)
> +{
> +	return i2c_add_driver(&m5mols_i2c_driver);
> +}
> +
> +static void __exit m5mols_mod_exit(void)
> +{
> +	i2c_del_driver(&m5mols_i2c_driver);
> +}
> +
> +module_init(m5mols_mod_init);
> +module_exit(m5mols_mod_exit);
> +
> +MODULE_AUTHOR("HeungJun Kim<riverful.kim@samsung.com>");
> +MODULE_AUTHOR("Dongsoo Kim<dongsoo45.kim@samsung.com>");
> +MODULE_DESCRIPTION("Fujitsu M5MOLS 8M Pixel camera sensor with ISP driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/video/m5mols/m5mols_reg.h b/drivers/media/video/m5mols/m5mols_reg.h
> new file mode 100644
> index 0000000..3e48a12
> --- /dev/null
> +++ b/drivers/media/video/m5mols/m5mols_reg.h
> @@ -0,0 +1,103 @@
> +/*
> + * Register map for M5MOLS 8M Pixel camera sensor with ISP
> + *
> + * Copyright (C) 2010 Samsung Electronics Co., Ltd
> + * Author: HeungJun Kim, riverful.kim@samsung.com
> + *
> + * Copyright (C) 2009 Samsung Electronics Co., Ltd
> + * Author: Dongsoo Nathaniel Kim, dongsoo45.kim@samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#ifndef M5MOLS_REG_H
> +#define M5MOLS_REG_H
> +
> +/*
> + * Category section register
> + *
> + * The category means a kind of command set. Including category section,
> + * all defined categories in this version supports only, as you see below:
> + */
> +#define CAT_SYSTEM		0x00
> +#define CAT_PARAM		0x01
> +#define CAT_MON			0x02
> +#define CAT_AE			0x03
> +#define CAT_WB			0x06
> +#define CAT_FLASH		0x0f	/* related with FW, Verions, booting */
> +
> +/*
> + * Category 0 - System
> + *
> + * This category supports FW version, managing mode, even interrupt.
> + */
> +#define CAT0_CUSTOMER_CODE	0x00
> +#define CAT0_PJ_CODE		0x01
> +#define CAT0_VERSION_FW_H	0x02
> +#define CAT0_VERSION_FW_L	0x03
> +#define CAT0_VERSION_HW_H	0x04
> +#define CAT0_VERSION_HW_L	0x05
> +#define CAT0_VERSION_PARM_H	0x06
> +#define CAT0_VERSION_PARM_L	0x07
> +#define CAT0_VERSION_AWB_H	0x08
> +#define CAT0_VERSION_AWB_L	0x09
> +#define CAT0_SYSMODE		0x0b
> +#define CAT0_INT_ROOTEN		0x12
> +
> +/*
> + * category 1 - Parameter mode
> + *
> + * This category is dealing with almost camera vendor. In spite of that,
> + * It's a register to be able to detailed value for whole camera syste.
> + * The key parameter like a resolution, FPS, data interface connecting
> + * with Mobile AP, even effects.
> + */
> +#define CAT1_DATA_INTERFACE	0x00
> +#define CAT1_MONITOR_SIZE	0x01
> +#define CAT1_MONITOR_FPS	0x02
> +#define CAT1_EFFECT		0x0b
> +
> +/*
> + * Category 2 - Monitor mode
> + *
> + * This category supports only monitoring mode. The monitoring mode means,
> + * similar to preview. It supports like a YUYV format. At the capture mode,
> + * it is handled like a JPEG&  RAW formats.
> + */
> +#define CAT2_CFIXB		0x09
> +#define CAT2_CFIXR		0x0a
> +#define CAT2_COLOR_EFFECT	0x0b
> +#define CAT2_CHROMA_LVL		0x0f
> +#define CAT2_CHROMA_EN		0x10
> +
> +/*
> + * Category 3 - Auto Exposure
> + *
> + * Currently, it supports only gain value with monitor mode. This device
> + * is able to support Shutter, Gain(similar with Aperture), Flicker, at
> + * monitor mode&  capture mode both.
> + */
> +#define CAT3_AE_LOCK		0x00
> +#define CAT3_AE_MODE		0x01
> +#define CAT3_MANUAL_GAIN_MON_1	0x12	/* upper byte */
> +#define CAT3_MANUAL_GAIN_MON_2	0x13	/* lower byte */
> +#define CAT3_MANUAL_SHUT_MON_1	0x14
> +#define CAT3_MANUAL_SHUT_MON_2	0x15
> +#define CAT3_MAX_SHUT_MON_1	0x16
> +#define CAT3_MAX_SHUT_MON_2	0x17
> +#define CAT3_MAX_GAIN_MON_1	0x1a
> +#define CAT3_MAX_GAIN_MON_2	0x1b
> +
> +/*
> + * Category 6 - White Balance
> + *
> + * Currently, it supports only auto white balance.
> + */
> +#define CAT6_AWB_LOCK		0x00
> +#define CAT6_AWB_MODE		0x02
> +#define CAT6_AWB_MANUAL		0x03
> +
> +#endif	/* M5MOLS_REG_H */


Regards,
Sylwester