All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 2/2 v2] [media] Add v4l2 subdev driver for NOON010PC30L image sensor
       [not found] ` <1291638298-31926-3-git-send-email-s.nawrocki@samsung.com>
@ 2010-12-20 13:01   ` Hans Verkuil
  2010-12-20 23:32     ` Sylwester Nawrocki
  0 siblings, 1 reply; 2+ messages in thread
From: Hans Verkuil @ 2010-12-20 13:01 UTC (permalink / raw)
  To: Sylwester Nawrocki; +Cc: linux-media, m.szyprowski, kyungmin.park

Hi Sylwester,

Here is a quick review of this second patch version.

Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl>

On Monday, December 06, 2010 13:24:58 Sylwester Nawrocki wrote:
> Add I2C/V4L2 subdev driver for Siliconfile NOON010PC30 CIF camera.
> The driver implements basic functionality, i.e. CIF/QCIF/QQCIF
> resolution and color format selection, automatic/manual color
> balance control. Other functions like cropping, rotation/flip,
> exposure etc. can be easily implemented if needed.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/Kconfig       |    6 +
>  drivers/media/video/Makefile      |    1 +
>  drivers/media/video/noon010pc30.c |  805 +++++++++++++++++++++++++++++++++++++
>  include/media/noon010pc30.h       |   28 ++
>  4 files changed, 840 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/noon010pc30.c
>  create mode 100644 include/media/noon010pc30.h
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index da08267..1f4b418 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -732,6 +732,12 @@ config VIDEO_VIA_CAMERA
>  	   Chrome9 chipsets.  Currently only tested on OLPC xo-1.5 systems
>  	   with ov7670 sensors.
>  
> +config VIDEO_NOON010PC30
> +	tristate "NOON010PC30 CIF camera sensor support"
> +	depends on I2C && VIDEO_V4L2
> +	---help---
> +	  This driver supports NOON010PC30 CIF camera from Siliconfile
> +
>  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 482f14b..fcc0644 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
>  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_SOC_CAMERA_IMX074)		+= imx074.o
>  obj-$(CONFIG_SOC_CAMERA_MT9M001)	+= mt9m001.o
> diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
> new file mode 100644
> index 0000000..22eead3
> --- /dev/null
> +++ b/drivers/media/video/noon010pc30.c
> @@ -0,0 +1,805 @@
> +/*
> + * Driver for SiliconFile NOON010PC30 CIF (1/11") Image Sensor with ISP
> + *
> + * Copyright (C) 2010 Samsung Electronics
> + * Contact: Sylwester Nawrocki, <s.nawrocki@samsung.com>
> + *
> + * Initial register configuration based on a driver authored by
> + * HeungJun Kim <riverful.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 vergsion.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
> +#include <media/noon010pc30.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-mediabus.h>
> +#include <media/v4l2-subdev.h>
> +
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "Enable module debug trace. Set to 1 to enable.");
> +
> +#define MODULE_NAME		"NOON010PC30"
> +#define NOON010_NUM_SUPPLIES	3
> +
> +/*
> + * Register offsets within a page
> + * b15..b8 - page id, b7..b0 - register address
> + */
> +#define POWER_CTRL_REG		0x0001
> +#define PAGEMODE_REG		0x03
> +#define DEVICE_ID_REG		0x0004
> +#define NOON010PC30_ID		0x86
> +#define VDO_CTL_REG(n)		(0x0010 + (n))
> +#define SYNC_CTL_REG		0x0012
> +/* Window size and position */
> +#define WIN_ROWH_REG		0x0013
> +#define WIN_ROWL_REG		0x0014
> +#define WIN_COLH_REG		0x0015
> +#define WIN_COLL_REG		0x0016
> +#define WIN_HEIGHTH_REG		0x0017
> +#define WIN_HEIGHTL_REG		0x0018
> +#define WIN_WIDTHH_REG		0x0019
> +#define WIN_WIDTHL_REG		0x001A
> +#define HBLANKH_REG		0x001B
> +#define HBLANKL_REG		0x001C
> +#define VSYNCH_REG		0x001D
> +#define VSYNCL_REG		0x001E
> +/* VSYNC control */
> +#define VS_CTL_REG(n)		(0x00A1 + (n))
> +/* page 1 */
> +#define ISP_CTL_REG(n)		(0x0110 + (n))
> +#define YOFS_REG		0x0119
> +#define DARK_YOFS_REG		0x011A
> +#define SAT_CTL_REG		0x0120
> +#define BSAT_REG		0x0121
> +#define RSAT_REG		0x0122
> +/* Color correction */
> +#define CMC_CTL_REG		0x0130
> +#define CMC_OFSGH_REG		0x0133
> +#define CMC_OFSGL_REG		0x0135
> +#define CMC_SIGN_REG		0x0136
> +#define CMC_GOFS_REG		0x0137
> +#define CMC_COEF_REG(n)		(0x0138 + (n))
> +#define CMC_OFS_REG(n)		(0x0141 + (n))
> +/* Gamma correction */
> +#define GMA_CTL_REG		0x0160
> +#define GMA_COEF_REG(n)		(0x0161 + (n))
> +/* Lens Shading */
> +#define LENS_CTRL_REG		0x01D0
> +#define LENS_XCEN_REG		0x01D1
> +#define LENS_YCEN_REG		0x01D2
> +#define LENS_RC_REG		0x01D3
> +#define LENS_GC_REG		0x01D4
> +#define LENS_BC_REG		0x01D5
> +#define L_AGON_REG		0x01D6
> +#define L_AGOFF_REG		0x01D7
> +/* Page 3 - Auto Exposure */
> +#define AE_CTL_REG(n)		(0x0310 + (n))
> +#define AE_CTL9_REG		0x032C
> +#define AE_CTL10_REG		0x032D
> +#define AE_YLVL_REG		0x031C
> +#define AE_YTH_REG(n)		(0x031D + (n))
> +#define AE_WGT_REG		0x0326
> +#define EXP_TIMEH_REG		0x0333
> +#define EXP_TIMEM_REG		0x0334
> +#define EXP_TIMEL_REG		0x0335
> +#define EXP_MMINH_REG		0x0336
> +#define EXP_MMINL_REG		0x0337
> +#define EXP_MMAXH_REG		0x0338
> +#define EXP_MMAXM_REG		0x0339
> +#define EXP_MMAXL_REG		0x033A
> +/* Page 4 - Auto White Balance */
> +#define AWB_CTL_REG(n)		(0x0410 + (n))
> +#define AWB_ENABE		0x80
> +#define AWB_WGHT_REG		0x0419
> +#define BGAIN_PAR_REG(n)	(0x044F + (n))
> +/* Manual white balance, when AWB_CTL2[0]=1 */
> +#define MWB_RGAIN_REG		0x0466
> +#define MWB_BGAIN_REG		0x0467
> +
> +/* The token to mark an array end */
> +#define REG_TERM		0xFFFF
> +
> +struct noon010_format {
> +	enum v4l2_mbus_pixelcode code;
> +	enum v4l2_colorspace colorspace;
> +	u16 ispctl1_reg;
> +};
> +
> +struct noon010_frmsize {
> +	u16 width;
> +	u16 height;
> +	int vid_ctl1;
> +};
> +
> +struct noon010_info {
> +	struct v4l2_subdev sd;
> +	struct v4l2_ctrl_handler hdl;
> +	struct v4l2_ctrl *awb;
> +	struct v4l2_ctrl *red_bal;
> +	struct v4l2_ctrl *blue_bal;

As mentioned earlier, these three pointers are no longer used.

> +
> +	const struct noon010pc30_platform_data *pdata;
> +	const struct noon010_format *curr_fmt;
> +	const struct noon010_frmsize *curr_win;
> +	unsigned int hflip:1;
> +	unsigned int vflip:1;
> +	unsigned int power:1;
> +	u8 i2c_reg_page;
> +	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
> +	u32 gpio_nreset;
> +	u32 gpio_nstby;
> +};
> +
> +struct i2c_regval {
> +	u16 addr;
> +	u16 val;
> +};
> +
> +static const char *supply_name[NOON010_NUM_SUPPLIES] = {
> +	"vdd_core", "vddio", "vdda"
> +};
> +
> +/* Supported resolutions. */
> +static const struct noon010_frmsize noon010_sizes[] = {
> +	{
> +		.width		= 352,
> +		.height		= 288,
> +		.vid_ctl1	= 0,
> +	}, {
> +		.width		= 176,
> +		.height		= 144,
> +		.vid_ctl1	= 0x10,
> +	}, {
> +		.width		= 88,
> +		.height		= 72,
> +		.vid_ctl1	= 0x20,
> +	},
> +};
> +
> +/* Supported pixel formats. */
> +static const struct noon010_format noon010_formats[] = {
> +	{
> +		.code		= V4L2_MBUS_FMT_YUYV8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +		.ispctl1_reg	= 0x03,
> +	}, {
> +		.code		= V4L2_MBUS_FMT_YVYU8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +		.ispctl1_reg	= 0x02,
> +	}, {
> +		.code		= V4L2_MBUS_FMT_VYUY8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +		.ispctl1_reg	= 0,
> +	}, {
> +		.code		= V4L2_MBUS_FMT_UYVY8_2X8,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +		.ispctl1_reg	= 0x01,
> +	}, {
> +		.code		= V4L2_MBUS_FMT_RGB565_2X8_BE,
> +		.colorspace	= V4L2_COLORSPACE_JPEG,
> +		.ispctl1_reg	= 0x40,
> +	},
> +};
> +
> +static const struct i2c_regval noon010_base_regs[] = {
> +	{ WIN_COLL_REG,		0x06 },	{ HBLANKL_REG,		0x7C },
> +	/* Color corection and saturation */
> +	{ ISP_CTL_REG(0),	0x30 }, { ISP_CTL_REG(2),	0x30 },
> +	{ YOFS_REG,		0x80 }, { DARK_YOFS_REG,	0x04 },
> +	{ SAT_CTL_REG,		0x1F }, { BSAT_REG,		0x90 },
> +	{ CMC_CTL_REG,		0x0F }, { CMC_OFSGH_REG,	0x3C },
> +	{ CMC_OFSGL_REG,	0x2C }, { CMC_SIGN_REG,		0x3F },
> +	{ CMC_COEF_REG(0),	0x79 }, { CMC_OFS_REG(0),	0x00 },
> +	{ CMC_COEF_REG(1),	0x39 }, { CMC_OFS_REG(1),	0x00 },
> +	{ CMC_COEF_REG(2),	0x00 }, { CMC_OFS_REG(2),	0x00 },
> +	{ CMC_COEF_REG(3),	0x11 }, { CMC_OFS_REG(3),	0x8B },
> +	{ CMC_COEF_REG(4),	0x65 }, { CMC_OFS_REG(4),	0x07 },
> +	{ CMC_COEF_REG(5),	0x14 }, { CMC_OFS_REG(5),	0x04 },
> +	{ CMC_COEF_REG(6),	0x01 }, { CMC_OFS_REG(6),	0x9C },
> +	{ CMC_COEF_REG(7),	0x33 }, { CMC_OFS_REG(7),	0x89 },
> +	{ CMC_COEF_REG(8),	0x74 }, { CMC_OFS_REG(8),	0x25 },
> +	/* Automatic white balance */
> +	{ AWB_CTL_REG(0),	0x78 }, { AWB_CTL_REG(1),	0x2E },
> +	{ AWB_CTL_REG(2),	0x20 }, { AWB_CTL_REG(3),	0x85 },
> +	/* Auto exposure */
> +	{ AE_CTL_REG(0),	0xDC }, { AE_CTL_REG(1),	0x81 },
> +	{ AE_CTL_REG(2),	0x30 }, { AE_CTL_REG(3),	0xA5 },
> +	{ AE_CTL_REG(4),	0x40 }, { AE_CTL_REG(5),	0x51 },
> +	{ AE_CTL_REG(6),	0x33 }, { AE_CTL_REG(7),	0x7E },
> +	{ AE_CTL9_REG,		0x00 }, { AE_CTL10_REG,		0x02 },
> +	{ AE_YLVL_REG,		0x44 },	{ AE_YTH_REG(0),	0x34 },
> +	{ AE_YTH_REG(1),	0x30 },	{ AE_WGT_REG,		0xD5 },
> +	/* Lens shading compensation */
> +	{ LENS_CTRL_REG,	0x01 }, { LENS_XCEN_REG,	0x80 },
> +	{ LENS_YCEN_REG,	0x70 }, { LENS_RC_REG,		0x53 },
> +	{ LENS_GC_REG,		0x40 }, { LENS_BC_REG,		0x3E },
> +	{ REG_TERM,		0 },
> +};
> +
> +static inline struct noon010_info *to_noon010(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct noon010_info, sd);
> +}
> +
> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct noon010_info, hdl)->sd;
> +}
> +
> +static inline int set_i2c_page(struct noon010_info *info,
> +			       struct i2c_client *client, unsigned int reg)
> +{
> +	u32 page = reg >> 8 & 0xFF;
> +	int ret = 0;
> +
> +	if (info->i2c_reg_page != page && (reg & 0xFF) != 0x03) {
> +		ret = i2c_smbus_write_byte_data(client, PAGEMODE_REG, page);
> +		if (!ret)
> +			info->i2c_reg_page = page;
> +	}
> +	return ret;
> +}
> +
> +static int cam_i2c_read(struct v4l2_subdev *sd, u32 reg_addr)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	int ret = set_i2c_page(info, client, reg_addr);

Swap the two lines above. Looks better that way with all the local
variables before the empty line.

> +	if (ret)
> +		return ret;
> +	return i2c_smbus_read_byte_data(client, reg_addr & 0xFF);
> +}
> +
> +static int cam_i2c_write(struct v4l2_subdev *sd, u32 reg_addr, u32 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	int ret = set_i2c_page(info, client, reg_addr);

ditto

> +	if (ret)
> +		return ret;
> +	return i2c_smbus_write_byte_data(client, reg_addr & 0xFF, val);
> +}
> +
> +static inline int noon010_bulk_write_reg(struct v4l2_subdev *sd,
> +					 const struct i2c_regval *msg)
> +{
> +	while (msg->addr != REG_TERM) {
> +		int ret = cam_i2c_write(sd, msg->addr, msg->val);

add empty line.

> +		if (ret)
> +			return ret;
> +		msg++;
> +	}
> +	return 0;
> +}
> +
> +/* Device reset and sleep mode control */
> +static int noon010_power_ctrl(struct v4l2_subdev *sd, bool reset, bool sleep)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +	u8 reg = sleep ? 0xF1 : 0xF0;
> +	int ret = 0;
> +
> +	if (reset)
> +		ret = cam_i2c_write(sd, POWER_CTRL_REG, reg | 0x02);
> +	if (!ret) {
> +		ret = cam_i2c_write(sd, POWER_CTRL_REG, reg);
> +		if (reset && !ret)
> +			info->i2c_reg_page = -1;
> +	}
> +	return ret;
> +}
> +
> +/* Automatic white balance control */
> +static int noon010_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
> +{
> +	int ret;
> +
> +	ret = cam_i2c_write(sd, AWB_CTL_REG(1), on ? 0x2E : 0x2F);
> +	if (!ret)
> +		ret = cam_i2c_write(sd, AWB_CTL_REG(0), on ? 0xFB : 0x7B);
> +	return ret;
> +}
> +
> +static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +	int reg, ret;
> +
> +	reg = cam_i2c_read(sd, VDO_CTL_REG(1));
> +	if (reg < 0)
> +		return reg;
> +
> +	reg &= 0x7C;
> +	if (hflip)
> +		reg |= 0x01;
> +	if (vflip)
> +		reg |= 0x02;
> +
> +	ret = cam_i2c_write(sd, VDO_CTL_REG(1), reg | 0x80);
> +	if (!ret) {
> +		info->hflip = hflip;
> +		info->vflip = vflip;
> +	}
> +	return ret;
> +}
> +
> +/* Configure resolution and color format */
> +static int noon010_set_params(struct v4l2_subdev *sd)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +	int ret;
> +
> +	if (!info->curr_win)
> +		return -EINVAL;
> +
> +	ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1);
> +
> +	if (!ret && info->curr_fmt)
> +		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
> +				info->curr_fmt->ispctl1_reg);
> +	return ret;
> +}
> +
> +/* Find nearest matching image pixel size. */
> +static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
> +{
> +	unsigned int min_err = ~0;
> +	int i = ARRAY_SIZE(noon010_sizes);
> +	const struct noon010_frmsize *fsize = &noon010_sizes[0],
> +		*match = NULL;
> +
> +	while (i--) {
> +		int err = abs(fsize->width - mf->width)
> +				+ abs(fsize->height - mf->height);

Empty line.

> +		if (err < min_err) {
> +			min_err = err;
> +			match = fsize;
> +		}
> +		fsize++;
> +	}
> +	if (match) {
> +		mf->width  = match->width;
> +		mf->height = match->height;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int power_enable(struct noon010_info *info)
> +{
> +	int ret;
> +
> +	if (info->power) {
> +		v4l2_info(&info->sd, "%s: sensor is already on\n", __func__);
> +		return 0;
> +	}
> +
> +	if (gpio_is_valid(info->gpio_nstby))
> +		gpio_set_value(info->gpio_nstby, 0);
> +
> +	if (gpio_is_valid(info->gpio_nreset))
> +		gpio_set_value(info->gpio_nreset, 0);
> +
> +	ret = regulator_bulk_enable(NOON010_NUM_SUPPLIES, info->supply);
> +	if (ret)
> +		return ret;
> +
> +	if (gpio_is_valid(info->gpio_nreset)) {
> +		msleep(50);
> +		gpio_set_value(info->gpio_nreset, 1);
> +	}
> +	if (gpio_is_valid(info->gpio_nstby)) {
> +		udelay(1000);
> +		gpio_set_value(info->gpio_nstby, 1);
> +	}
> +	if (gpio_is_valid(info->gpio_nreset)) {
> +		udelay(1000);
> +		gpio_set_value(info->gpio_nreset, 0);
> +		msleep(100);
> +		gpio_set_value(info->gpio_nreset, 1);
> +		msleep(20);
> +	}
> +	info->power = 1;
> +
> +	v4l2_dbg(1, debug, &info->sd,  "%s: sensor is on\n", __func__);
> +	return 0;
> +}
> +
> +static int power_disable(struct noon010_info *info)
> +{
> +	int ret;
> +
> +	if (!info->power) {
> +		v4l2_info(&info->sd, "%s: sensor is already off\n", __func__);
> +		return 0;
> +	}
> +
> +	ret = regulator_bulk_disable(NOON010_NUM_SUPPLIES, info->supply);
> +	if (ret)
> +		return ret;
> +
> +	if (gpio_is_valid(info->gpio_nstby))
> +		gpio_set_value(info->gpio_nstby, 0);
> +
> +	if (gpio_is_valid(info->gpio_nreset))
> +		gpio_set_value(info->gpio_nreset, 0);
> +
> +	info->power = 0;
> +
> +	v4l2_dbg(1, debug, &info->sd,  "%s: sensor is off\n", __func__);
> +
> +	return 0;
> +}
> +
> +static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	int ret;
> +
> +	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
> +		 __func__, ctrl->id, ctrl->val);
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_AUTO_WHITE_BALANCE:
> +		ret = noon010_enable_autowhitebalance(sd, ctrl->val);

Just do:

		return noon010_enable_autowhitebalance(sd, ctrl->val);

Ditto for the lines below. This removes the need for the ret variable.

> +		break;
> +	case V4L2_CID_BLUE_BALANCE:
> +		ret = cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
> +		break;
> +	case V4L2_CID_RED_BALANCE:
> +		ret = cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int noon010_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
> +			    enum v4l2_mbus_pixelcode *code)
> +{
> +	if (!code || index >= ARRAY_SIZE(noon010_formats))
> +		return -EINVAL;
> +
> +	*code = noon010_formats[index].code;
> +	return 0;
> +}
> +
> +static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +	int ret;
> +
> +	if (!mf)
> +		return -EINVAL;
> +
> +	if (!info->curr_win || !info->curr_fmt) {
> +		ret = noon010_set_params(sd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	mf->width	= info->curr_win->width;
> +	mf->height	= info->curr_win->height;
> +	mf->code	= info->curr_fmt->code;
> +	mf->colorspace	= info->curr_fmt->colorspace;
> +	mf->field	= V4L2_FIELD_NONE;
> +
> +	return 0;
> +}
> +
> +/* Return nearest media bus frame format. */
> +static const struct noon010_format *try_fmt(struct v4l2_subdev *sd,
> +					    struct v4l2_mbus_framefmt *mf)
> +{
> +	int i = ARRAY_SIZE(noon010_formats);
> +
> +	noon010_try_frame_size(mf);
> +
> +	while (i--)
> +		if (mf->code == noon010_formats[i].code)
> +			break;
> +
> +	mf->code = noon010_formats[i].code;
> +
> +	return &noon010_formats[i];
> +}
> +
> +static int noon010_try_fmt(struct v4l2_subdev *sd,
> +			   struct v4l2_mbus_framefmt *mf)
> +{
> +	if (!sd || !mf)
> +		return -EINVAL;
> +
> +	try_fmt(sd, mf);
> +	return 0;
> +}
> +
> +static int noon010_s_fmt(struct v4l2_subdev *sd,
> +			 struct v4l2_mbus_framefmt *mf)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	if (!sd || !mf)
> +		return -EINVAL;
> +
> +	info->curr_fmt = try_fmt(sd, mf);
> +
> +	return noon010_set_params(sd);
> +}
> +
> +static int noon010_base_config(struct v4l2_subdev *sd)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +	int ret;
> +
> +	ret = noon010_bulk_write_reg(sd, noon010_base_regs);
> +	if (!ret) {
> +		info->curr_fmt = &noon010_formats[0];
> +		info->curr_win = &noon010_sizes[0];
> +		ret = noon010_set_params(sd);
> +	}
> +	if (!ret)
> +		ret = noon010_set_flip(sd, 1, 0);
> +	if (!ret)
> +		ret = noon010_power_ctrl(sd, false, false);
> +
> +	/* sync the handler and the registers state */
> +	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
> +	return ret;
> +}
> +
> +static int noon010_s_config(struct v4l2_subdev *sd,
> +			    int irq, void *platform_data)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	info->pdata = platform_data;
> +	return 0;
> +}

Why do you need this? This shouldn't be used in new drivers. It's for
backwards compatibility with older kernels and it should probably be
removed altogether.

> +static int noon010_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	return 0;
> +}

This doesn't do anything, so you can just drop this function.

> +static int noon010_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +	const struct noon010pc30_platform_data *pdata = info->pdata;
> +	int ret = 0;
> +
> +	if (WARN(pdata == NULL, "No platform data!\n"))
> +		return -ENOMEM;
> +
> +	if (on) {
> +		noon010_power_ctrl(sd, false, true);
> +		ret = power_enable(info);
> +		if (ret)
> +			return ret;
> +		ret = noon010_base_config(sd);
> +	} else {
> +		ret = power_disable(info);
> +		info->curr_win = NULL;
> +		info->curr_fmt = NULL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int noon010_g_chip_ident(struct v4l2_subdev *sd,
> +				struct v4l2_dbg_chip_ident *chip)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	return v4l2_chip_ident_i2c_client(client, chip,
> +					  V4L2_IDENT_NOON010PC30, 0);
> +}
> +
> +static int noon010_log_status(struct v4l2_subdev *sd)
> +{
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	v4l2_ctrl_handler_log_status(&info->hdl, sd->name);
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops noon010_ctrl_ops = {
> +	.s_ctrl = noon010_s_ctrl,
> +};
> +
> +static const struct v4l2_subdev_core_ops noon010_core_ops = {
> +	.g_chip_ident	= noon010_g_chip_ident,
> +	.s_config	= noon010_s_config,
> +	.s_power	= noon010_s_power,
> +	.g_ctrl		= v4l2_subdev_g_ctrl,
> +	.s_ctrl		= v4l2_subdev_s_ctrl,
> +	.queryctrl	= v4l2_subdev_queryctrl,
> +	.querymenu	= v4l2_subdev_querymenu,

Also add:

        .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	= noon010_log_status,
> +};
> +
> +static const struct v4l2_subdev_video_ops noon010_video_ops = {
> +	.s_stream	= noon010_s_stream,
> +	.g_mbus_fmt	= noon010_g_fmt,
> +	.s_mbus_fmt	= noon010_s_fmt,
> +	.try_mbus_fmt	= noon010_try_fmt,
> +	.enum_mbus_fmt	= noon010_enum_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops noon010_ops = {
> +	.core	= &noon010_core_ops,
> +	.video	= &noon010_video_ops,
> +};
> +
> +/* Return 0 if NOON010PC30L sensor type was detected or -ENODEV otherwise. */
> +static int noon010_detect(struct i2c_client *client, struct noon010_info *info)
> +{
> +	int ret;
> +
> +	ret = power_enable(info);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, DEVICE_ID_REG);
> +	if (ret < 0)
> +		dev_err(&client->dev, "I2C read failed: 0x%X\n", ret);
> +
> +	power_disable(info);
> +
> +	return ret == NOON010PC30_ID ? 0 : -ENODEV;
> +}
> +
> +static int noon010_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct noon010_info *info;
> +	struct v4l2_subdev *sd;
> +	const struct noon010pc30_platform_data *pdata
> +		= client->dev.platform_data;
> +	int ret;
> +	int i;
> +
> +	if (!pdata) {
> +		dev_err(&client->dev, "No platform data!\n");
> +		return -EIO;
> +	}
> +
> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	sd = &info->sd;
> +	strcpy(sd->name, MODULE_NAME);

strlcpy

> +	v4l2_i2c_subdev_init(sd, client, &noon010_ops);
> +
> +	v4l2_ctrl_handler_init(&info->hdl, 3);
> +
> +	v4l2_ctrl_new_std(&info->hdl, &noon010_ctrl_ops,
> +			  V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
> +	v4l2_ctrl_new_std(&info->hdl, &noon010_ctrl_ops,
> +			  V4L2_CID_RED_BALANCE, 0, 127, 1, 64);
> +	v4l2_ctrl_new_std(&info->hdl, &noon010_ctrl_ops,
> +			  V4L2_CID_BLUE_BALANCE, 0, 127, 1, 64);
> +
> +	sd->ctrl_handler = &info->hdl;
> +
> +	ret = info->hdl.error;
> +	if (ret) {
> +		v4l2_ctrl_handler_free(&info->hdl);

The v4l2_ctrl_handler_free call should be part of the 'exit' labels at the end...

> +		goto np_err;
> +	}
> +
> +	info->pdata		= client->dev.platform_data;
> +	info->i2c_reg_page	= -1;
> +	info->gpio_nreset	= -EINVAL;
> +	info->gpio_nstby	= -EINVAL;
> +
> +	if (gpio_is_valid(pdata->gpio_nreset)) {
> +		ret = gpio_request(pdata->gpio_nreset, "NOON010PC30 NRST");
> +		if (ret) {
> +			dev_err(&client->dev, "GPIO request error: %d\n", ret);
> +			goto np_err;

...because otherwise it will never be freed here and in the other exit paths
below.

> +		}
> +		info->gpio_nreset = pdata->gpio_nreset;
> +		gpio_direction_output(info->gpio_nreset, 0);
> +		gpio_export(info->gpio_nreset, 0);
> +	}
> +
> +	if (gpio_is_valid(pdata->gpio_nstby)) {
> +		ret = gpio_request(pdata->gpio_nstby, "NOON010PC30 NSTBY");
> +		if (ret) {
> +			dev_err(&client->dev, "GPIO request error: %d\n", ret);
> +			goto gpio_stby_err;
> +		}
> +		info->gpio_nstby = pdata->gpio_nstby;
> +		gpio_direction_output(info->gpio_nstby, 0);
> +		gpio_export(info->gpio_nstby, 0);
> +	}
> +
> +	for (i = 0; i < NOON010_NUM_SUPPLIES; i++)
> +		info->supply[i].supply = supply_name[i];
> +
> +	ret = regulator_bulk_get(&client->dev, NOON010_NUM_SUPPLIES,
> +			info->supply);

ret is assigned but not checked?

> +	ret = noon010_detect(client, info);
> +	if (!ret)
> +		return 0;
> +
> +	/* the sensor detection failed */
> +	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
> +	if (gpio_is_valid(info->gpio_nstby))
> +		gpio_free(info->gpio_nstby);
> +gpio_stby_err:
> +	if (gpio_is_valid(info->gpio_nreset))
> +		gpio_free(info->gpio_nreset);
> +np_err:
> +	v4l2_device_unregister_subdev(sd);
> +	kfree(info);
> +	return ret;
> +}
> +
> +static int noon010_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct noon010_info *info = to_noon010(sd);
> +
> +	v4l2_device_unregister_subdev(sd);

Missing v4l2_ctrl_handler_free.

> +	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
> +	if (gpio_is_valid(info->gpio_nreset))
> +		gpio_free(info->gpio_nreset);
> +	if (gpio_is_valid(info->gpio_nstby))
> +		gpio_free(info->gpio_nstby);
> +	kfree(info);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id noon010_id[] = {
> +	{ MODULE_NAME, 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, noon010_id);
> +
> +
> +static struct i2c_driver noon010_i2c_driver = {
> +	.driver = {
> +		.name = MODULE_NAME
> +	},
> +	.probe		= noon010_probe,
> +	.remove		= noon010_remove,
> +	.id_table	= noon010_id,
> +};
> +
> +static int __init noon010_init(void)
> +{
> +	return i2c_add_driver(&noon010_i2c_driver);
> +}
> +
> +static void __exit noon010_exit(void)
> +{
> +	i2c_del_driver(&noon010_i2c_driver);
> +}
> +
> +module_init(noon010_init);
> +module_exit(noon010_exit);
> +
> +MODULE_DESCRIPTION("Siliconfile NOON010PC30 camera driver");
> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@samsung.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/media/noon010pc30.h b/include/media/noon010pc30.h
> new file mode 100644
> index 0000000..58eafee
> --- /dev/null
> +++ b/include/media/noon010pc30.h
> @@ -0,0 +1,28 @@
> +/*
> + * Driver header for NOON010PC30L camera sensor chip.
> + *
> + * Copyright (c) 2010 Samsung Electronics, Co. Ltd
> + * Contact: Sylwester Nawrocki <s.nawrocki@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 NOON010PC30_H
> +#define NOON010PC30_H
> +
> +/**
> + * @clk_rate: the clock frequency in Hz
> + * @gpio_nreset: GPIO driving nRESET pin
> + * @gpio_nstby: GPIO driving nSTBY pin
> + */
> +
> +struct noon010pc30_platform_data {
> +	unsigned long clk_rate;
> +	int gpio_nreset;
> +	int gpio_nstby;
> +};
> +
> +#endif /* NOON010PC30_H */
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH 2/2 v2] [media] Add v4l2 subdev driver for NOON010PC30L image sensor
  2010-12-20 13:01   ` [PATCH 2/2 v2] [media] Add v4l2 subdev driver for NOON010PC30L image sensor Hans Verkuil
@ 2010-12-20 23:32     ` Sylwester Nawrocki
  0 siblings, 0 replies; 2+ messages in thread
From: Sylwester Nawrocki @ 2010-12-20 23:32 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Sylwester Nawrocki, linux-media, m.szyprowski, kyungmin.park


Hi Hans,

Thanks for this quick albeit helpful review.
I had no objections to your comments so they all
are considered in the patch version 3.

On 12/20/2010 02:01 PM, Hans Verkuil wrote:
> Hi Sylwester,
>
> Here is a quick review of this second patch version.
>
> Reviewed-by: Hans Verkuil<hverkuil@xs4all.nl>
>
> On Monday, December 06, 2010 13:24:58 Sylwester Nawrocki wrote:
>> Add I2C/V4L2 subdev driver for Siliconfile NOON010PC30 CIF camera.
>> The driver implements basic functionality, i.e. CIF/QCIF/QQCIF
>> resolution and color format selection, automatic/manual color
>> balance control. Other functions like cropping, rotation/flip,
>> exposure etc. can be easily implemented if needed.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   drivers/media/video/Kconfig       |    6 +
>>   drivers/media/video/Makefile      |    1 +
>>   drivers/media/video/noon010pc30.c |  805 +++++++++++++++++++++++++++++++++++++
>>   include/media/noon010pc30.h       |   28 ++
>>   4 files changed, 840 insertions(+), 0 deletions(-)
>>   create mode 100644 drivers/media/video/noon010pc30.c
>>   create mode 100644 include/media/noon010pc30.h
>>
>> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
>> index da08267..1f4b418 100644
>> --- a/drivers/media/video/Kconfig
>> +++ b/drivers/media/video/Kconfig
>> @@ -732,6 +732,12 @@ config VIDEO_VIA_CAMERA
>>   	   Chrome9 chipsets.  Currently only tested on OLPC xo-1.5 systems
>>   	   with ov7670 sensors.
>>
>> +config VIDEO_NOON010PC30
>> +	tristate "NOON010PC30 CIF camera sensor support"
>> +	depends on I2C&&  VIDEO_V4L2
>> +	---help---
>> +	  This driver supports NOON010PC30 CIF camera from Siliconfile
>> +
>>   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 482f14b..fcc0644 100644
>> --- a/drivers/media/video/Makefile
>> +++ b/drivers/media/video/Makefile
>> @@ -72,6 +72,7 @@ obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
>>   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_SOC_CAMERA_IMX074)		+= imx074.o
>>   obj-$(CONFIG_SOC_CAMERA_MT9M001)	+= mt9m001.o
>> diff --git a/drivers/media/video/noon010pc30.c b/drivers/media/video/noon010pc30.c
>> new file mode 100644
>> index 0000000..22eead3
>> --- /dev/null
>> +++ b/drivers/media/video/noon010pc30.c
>> @@ -0,0 +1,805 @@
>> +/*
>> + * Driver for SiliconFile NOON010PC30 CIF (1/11") Image Sensor with ISP
>> + *
>> + * Copyright (C) 2010 Samsung Electronics
>> + * Contact: Sylwester Nawrocki,<s.nawrocki@samsung.com>
>> + *
>> + * Initial register configuration based on a driver authored by
>> + * HeungJun Kim<riverful.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 vergsion.
>> + */
>> +
>> +#include<linux/delay.h>
>> +#include<linux/gpio.h>
>> +#include<linux/i2c.h>
>> +#include<linux/slab.h>
>> +#include<linux/regulator/consumer.h>
>> +#include<media/noon010pc30.h>
>> +#include<media/v4l2-chip-ident.h>
>> +#include<linux/videodev2.h>
>> +#include<media/v4l2-ctrls.h>
>> +#include<media/v4l2-device.h>
>> +#include<media/v4l2-mediabus.h>
>> +#include<media/v4l2-subdev.h>
>> +
>> +static int debug;
>> +module_param(debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "Enable module debug trace. Set to 1 to enable.");
>> +
>> +#define MODULE_NAME		"NOON010PC30"
>> +#define NOON010_NUM_SUPPLIES	3
>> +
>> +/*
>> + * Register offsets within a page
>> + * b15..b8 - page id, b7..b0 - register address
>> + */
>> +#define POWER_CTRL_REG		0x0001
>> +#define PAGEMODE_REG		0x03
>> +#define DEVICE_ID_REG		0x0004
>> +#define NOON010PC30_ID		0x86
>> +#define VDO_CTL_REG(n)		(0x0010 + (n))
>> +#define SYNC_CTL_REG		0x0012
>> +/* Window size and position */
>> +#define WIN_ROWH_REG		0x0013
>> +#define WIN_ROWL_REG		0x0014
>> +#define WIN_COLH_REG		0x0015
>> +#define WIN_COLL_REG		0x0016
>> +#define WIN_HEIGHTH_REG		0x0017
>> +#define WIN_HEIGHTL_REG		0x0018
>> +#define WIN_WIDTHH_REG		0x0019
>> +#define WIN_WIDTHL_REG		0x001A
>> +#define HBLANKH_REG		0x001B
>> +#define HBLANKL_REG		0x001C
>> +#define VSYNCH_REG		0x001D
>> +#define VSYNCL_REG		0x001E
>> +/* VSYNC control */
>> +#define VS_CTL_REG(n)		(0x00A1 + (n))
>> +/* page 1 */
>> +#define ISP_CTL_REG(n)		(0x0110 + (n))
>> +#define YOFS_REG		0x0119
>> +#define DARK_YOFS_REG		0x011A
>> +#define SAT_CTL_REG		0x0120
>> +#define BSAT_REG		0x0121
>> +#define RSAT_REG		0x0122
>> +/* Color correction */
>> +#define CMC_CTL_REG		0x0130
>> +#define CMC_OFSGH_REG		0x0133
>> +#define CMC_OFSGL_REG		0x0135
>> +#define CMC_SIGN_REG		0x0136
>> +#define CMC_GOFS_REG		0x0137
>> +#define CMC_COEF_REG(n)		(0x0138 + (n))
>> +#define CMC_OFS_REG(n)		(0x0141 + (n))
>> +/* Gamma correction */
>> +#define GMA_CTL_REG		0x0160
>> +#define GMA_COEF_REG(n)		(0x0161 + (n))
>> +/* Lens Shading */
>> +#define LENS_CTRL_REG		0x01D0
>> +#define LENS_XCEN_REG		0x01D1
>> +#define LENS_YCEN_REG		0x01D2
>> +#define LENS_RC_REG		0x01D3
>> +#define LENS_GC_REG		0x01D4
>> +#define LENS_BC_REG		0x01D5
>> +#define L_AGON_REG		0x01D6
>> +#define L_AGOFF_REG		0x01D7
>> +/* Page 3 - Auto Exposure */
>> +#define AE_CTL_REG(n)		(0x0310 + (n))
>> +#define AE_CTL9_REG		0x032C
>> +#define AE_CTL10_REG		0x032D
>> +#define AE_YLVL_REG		0x031C
>> +#define AE_YTH_REG(n)		(0x031D + (n))
>> +#define AE_WGT_REG		0x0326
>> +#define EXP_TIMEH_REG		0x0333
>> +#define EXP_TIMEM_REG		0x0334
>> +#define EXP_TIMEL_REG		0x0335
>> +#define EXP_MMINH_REG		0x0336
>> +#define EXP_MMINL_REG		0x0337
>> +#define EXP_MMAXH_REG		0x0338
>> +#define EXP_MMAXM_REG		0x0339
>> +#define EXP_MMAXL_REG		0x033A
>> +/* Page 4 - Auto White Balance */
>> +#define AWB_CTL_REG(n)		(0x0410 + (n))
>> +#define AWB_ENABE		0x80
>> +#define AWB_WGHT_REG		0x0419
>> +#define BGAIN_PAR_REG(n)	(0x044F + (n))
>> +/* Manual white balance, when AWB_CTL2[0]=1 */
>> +#define MWB_RGAIN_REG		0x0466
>> +#define MWB_BGAIN_REG		0x0467
>> +
>> +/* The token to mark an array end */
>> +#define REG_TERM		0xFFFF
>> +
>> +struct noon010_format {
>> +	enum v4l2_mbus_pixelcode code;
>> +	enum v4l2_colorspace colorspace;
>> +	u16 ispctl1_reg;
>> +};
>> +
>> +struct noon010_frmsize {
>> +	u16 width;
>> +	u16 height;
>> +	int vid_ctl1;
>> +};
>> +
>> +struct noon010_info {
>> +	struct v4l2_subdev sd;
>> +	struct v4l2_ctrl_handler hdl;
>> +	struct v4l2_ctrl *awb;
>> +	struct v4l2_ctrl *red_bal;
>> +	struct v4l2_ctrl *blue_bal;
>
> As mentioned earlier, these three pointers are no longer used.
>
>> +
>> +	const struct noon010pc30_platform_data *pdata;
>> +	const struct noon010_format *curr_fmt;
>> +	const struct noon010_frmsize *curr_win;
>> +	unsigned int hflip:1;
>> +	unsigned int vflip:1;
>> +	unsigned int power:1;
>> +	u8 i2c_reg_page;
>> +	struct regulator_bulk_data supply[NOON010_NUM_SUPPLIES];
>> +	u32 gpio_nreset;
>> +	u32 gpio_nstby;
>> +};
>> +
>> +struct i2c_regval {
>> +	u16 addr;
>> +	u16 val;
>> +};
>> +
>> +static const char *supply_name[NOON010_NUM_SUPPLIES] = {
>> +	"vdd_core", "vddio", "vdda"
>> +};
>> +
>> +/* Supported resolutions. */
>> +static const struct noon010_frmsize noon010_sizes[] = {
>> +	{
>> +		.width		= 352,
>> +		.height		= 288,
>> +		.vid_ctl1	= 0,
>> +	}, {
>> +		.width		= 176,
>> +		.height		= 144,
>> +		.vid_ctl1	= 0x10,
>> +	}, {
>> +		.width		= 88,
>> +		.height		= 72,
>> +		.vid_ctl1	= 0x20,
>> +	},
>> +};
>> +
>> +/* Supported pixel formats. */
>> +static const struct noon010_format noon010_formats[] = {
>> +	{
>> +		.code		= V4L2_MBUS_FMT_YUYV8_2X8,
>> +		.colorspace	= V4L2_COLORSPACE_JPEG,
>> +		.ispctl1_reg	= 0x03,
>> +	}, {
>> +		.code		= V4L2_MBUS_FMT_YVYU8_2X8,
>> +		.colorspace	= V4L2_COLORSPACE_JPEG,
>> +		.ispctl1_reg	= 0x02,
>> +	}, {
>> +		.code		= V4L2_MBUS_FMT_VYUY8_2X8,
>> +		.colorspace	= V4L2_COLORSPACE_JPEG,
>> +		.ispctl1_reg	= 0,
>> +	}, {
>> +		.code		= V4L2_MBUS_FMT_UYVY8_2X8,
>> +		.colorspace	= V4L2_COLORSPACE_JPEG,
>> +		.ispctl1_reg	= 0x01,
>> +	}, {
>> +		.code		= V4L2_MBUS_FMT_RGB565_2X8_BE,
>> +		.colorspace	= V4L2_COLORSPACE_JPEG,
>> +		.ispctl1_reg	= 0x40,
>> +	},
>> +};
>> +
>> +static const struct i2c_regval noon010_base_regs[] = {
>> +	{ WIN_COLL_REG,		0x06 },	{ HBLANKL_REG,		0x7C },
>> +	/* Color corection and saturation */
>> +	{ ISP_CTL_REG(0),	0x30 }, { ISP_CTL_REG(2),	0x30 },
>> +	{ YOFS_REG,		0x80 }, { DARK_YOFS_REG,	0x04 },
>> +	{ SAT_CTL_REG,		0x1F }, { BSAT_REG,		0x90 },
>> +	{ CMC_CTL_REG,		0x0F }, { CMC_OFSGH_REG,	0x3C },
>> +	{ CMC_OFSGL_REG,	0x2C }, { CMC_SIGN_REG,		0x3F },
>> +	{ CMC_COEF_REG(0),	0x79 }, { CMC_OFS_REG(0),	0x00 },
>> +	{ CMC_COEF_REG(1),	0x39 }, { CMC_OFS_REG(1),	0x00 },
>> +	{ CMC_COEF_REG(2),	0x00 }, { CMC_OFS_REG(2),	0x00 },
>> +	{ CMC_COEF_REG(3),	0x11 }, { CMC_OFS_REG(3),	0x8B },
>> +	{ CMC_COEF_REG(4),	0x65 }, { CMC_OFS_REG(4),	0x07 },
>> +	{ CMC_COEF_REG(5),	0x14 }, { CMC_OFS_REG(5),	0x04 },
>> +	{ CMC_COEF_REG(6),	0x01 }, { CMC_OFS_REG(6),	0x9C },
>> +	{ CMC_COEF_REG(7),	0x33 }, { CMC_OFS_REG(7),	0x89 },
>> +	{ CMC_COEF_REG(8),	0x74 }, { CMC_OFS_REG(8),	0x25 },
>> +	/* Automatic white balance */
>> +	{ AWB_CTL_REG(0),	0x78 }, { AWB_CTL_REG(1),	0x2E },
>> +	{ AWB_CTL_REG(2),	0x20 }, { AWB_CTL_REG(3),	0x85 },
>> +	/* Auto exposure */
>> +	{ AE_CTL_REG(0),	0xDC }, { AE_CTL_REG(1),	0x81 },
>> +	{ AE_CTL_REG(2),	0x30 }, { AE_CTL_REG(3),	0xA5 },
>> +	{ AE_CTL_REG(4),	0x40 }, { AE_CTL_REG(5),	0x51 },
>> +	{ AE_CTL_REG(6),	0x33 }, { AE_CTL_REG(7),	0x7E },
>> +	{ AE_CTL9_REG,		0x00 }, { AE_CTL10_REG,		0x02 },
>> +	{ AE_YLVL_REG,		0x44 },	{ AE_YTH_REG(0),	0x34 },
>> +	{ AE_YTH_REG(1),	0x30 },	{ AE_WGT_REG,		0xD5 },
>> +	/* Lens shading compensation */
>> +	{ LENS_CTRL_REG,	0x01 }, { LENS_XCEN_REG,	0x80 },
>> +	{ LENS_YCEN_REG,	0x70 }, { LENS_RC_REG,		0x53 },
>> +	{ LENS_GC_REG,		0x40 }, { LENS_BC_REG,		0x3E },
>> +	{ REG_TERM,		0 },
>> +};
>> +
>> +static inline struct noon010_info *to_noon010(struct v4l2_subdev *sd)
>> +{
>> +	return container_of(sd, struct noon010_info, sd);
>> +}
>> +
>> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
>> +{
>> +	return&container_of(ctrl->handler, struct noon010_info, hdl)->sd;
>> +}
>> +
>> +static inline int set_i2c_page(struct noon010_info *info,
>> +			       struct i2c_client *client, unsigned int reg)
>> +{
>> +	u32 page = reg>>  8&  0xFF;
>> +	int ret = 0;
>> +
>> +	if (info->i2c_reg_page != page&&  (reg&  0xFF) != 0x03) {
>> +		ret = i2c_smbus_write_byte_data(client, PAGEMODE_REG, page);
>> +		if (!ret)
>> +			info->i2c_reg_page = page;
>> +	}
>> +	return ret;
>> +}
>> +
>> +static int cam_i2c_read(struct v4l2_subdev *sd, u32 reg_addr)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	int ret = set_i2c_page(info, client, reg_addr);
>
> Swap the two lines above. Looks better that way with all the local
> variables before the empty line.
>
>> +	if (ret)
>> +		return ret;
>> +	return i2c_smbus_read_byte_data(client, reg_addr&  0xFF);
>> +}
>> +
>> +static int cam_i2c_write(struct v4l2_subdev *sd, u32 reg_addr, u32 val)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	int ret = set_i2c_page(info, client, reg_addr);
>
> ditto
>
>> +	if (ret)
>> +		return ret;
>> +	return i2c_smbus_write_byte_data(client, reg_addr&  0xFF, val);
>> +}
>> +
>> +static inline int noon010_bulk_write_reg(struct v4l2_subdev *sd,
>> +					 const struct i2c_regval *msg)
>> +{
>> +	while (msg->addr != REG_TERM) {
>> +		int ret = cam_i2c_write(sd, msg->addr, msg->val);
>
> add empty line.
>
>> +		if (ret)
>> +			return ret;
>> +		msg++;
>> +	}
>> +	return 0;
>> +}
>> +
>> +/* Device reset and sleep mode control */
>> +static int noon010_power_ctrl(struct v4l2_subdev *sd, bool reset, bool sleep)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +	u8 reg = sleep ? 0xF1 : 0xF0;
>> +	int ret = 0;
>> +
>> +	if (reset)
>> +		ret = cam_i2c_write(sd, POWER_CTRL_REG, reg | 0x02);
>> +	if (!ret) {
>> +		ret = cam_i2c_write(sd, POWER_CTRL_REG, reg);
>> +		if (reset&&  !ret)
>> +			info->i2c_reg_page = -1;
>> +	}
>> +	return ret;
>> +}
>> +
>> +/* Automatic white balance control */
>> +static int noon010_enable_autowhitebalance(struct v4l2_subdev *sd, int on)
>> +{
>> +	int ret;
>> +
>> +	ret = cam_i2c_write(sd, AWB_CTL_REG(1), on ? 0x2E : 0x2F);
>> +	if (!ret)
>> +		ret = cam_i2c_write(sd, AWB_CTL_REG(0), on ? 0xFB : 0x7B);
>> +	return ret;
>> +}
>> +
>> +static int noon010_set_flip(struct v4l2_subdev *sd, int hflip, int vflip)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +	int reg, ret;
>> +
>> +	reg = cam_i2c_read(sd, VDO_CTL_REG(1));
>> +	if (reg<  0)
>> +		return reg;
>> +
>> +	reg&= 0x7C;
>> +	if (hflip)
>> +		reg |= 0x01;
>> +	if (vflip)
>> +		reg |= 0x02;
>> +
>> +	ret = cam_i2c_write(sd, VDO_CTL_REG(1), reg | 0x80);
>> +	if (!ret) {
>> +		info->hflip = hflip;
>> +		info->vflip = vflip;
>> +	}
>> +	return ret;
>> +}
>> +
>> +/* Configure resolution and color format */
>> +static int noon010_set_params(struct v4l2_subdev *sd)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +	int ret;
>> +
>> +	if (!info->curr_win)
>> +		return -EINVAL;
>> +
>> +	ret = cam_i2c_write(sd, VDO_CTL_REG(0), info->curr_win->vid_ctl1);
>> +
>> +	if (!ret&&  info->curr_fmt)
>> +		ret = cam_i2c_write(sd, ISP_CTL_REG(0),
>> +				info->curr_fmt->ispctl1_reg);
>> +	return ret;
>> +}
>> +
>> +/* Find nearest matching image pixel size. */
>> +static int noon010_try_frame_size(struct v4l2_mbus_framefmt *mf)
>> +{
>> +	unsigned int min_err = ~0;
>> +	int i = ARRAY_SIZE(noon010_sizes);
>> +	const struct noon010_frmsize *fsize =&noon010_sizes[0],
>> +		*match = NULL;
>> +
>> +	while (i--) {
>> +		int err = abs(fsize->width - mf->width)
>> +				+ abs(fsize->height - mf->height);
>
> Empty line.
>
>> +		if (err<  min_err) {
>> +			min_err = err;
>> +			match = fsize;
>> +		}
>> +		fsize++;
>> +	}
>> +	if (match) {
>> +		mf->width  = match->width;
>> +		mf->height = match->height;
>> +		return 0;
>> +	}
>> +	return -EINVAL;
>> +}
>> +
>> +static int power_enable(struct noon010_info *info)
>> +{
>> +	int ret;
>> +
>> +	if (info->power) {
>> +		v4l2_info(&info->sd, "%s: sensor is already on\n", __func__);
>> +		return 0;
>> +	}
>> +
>> +	if (gpio_is_valid(info->gpio_nstby))
>> +		gpio_set_value(info->gpio_nstby, 0);
>> +
>> +	if (gpio_is_valid(info->gpio_nreset))
>> +		gpio_set_value(info->gpio_nreset, 0);
>> +
>> +	ret = regulator_bulk_enable(NOON010_NUM_SUPPLIES, info->supply);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (gpio_is_valid(info->gpio_nreset)) {
>> +		msleep(50);
>> +		gpio_set_value(info->gpio_nreset, 1);
>> +	}
>> +	if (gpio_is_valid(info->gpio_nstby)) {
>> +		udelay(1000);
>> +		gpio_set_value(info->gpio_nstby, 1);
>> +	}
>> +	if (gpio_is_valid(info->gpio_nreset)) {
>> +		udelay(1000);
>> +		gpio_set_value(info->gpio_nreset, 0);
>> +		msleep(100);
>> +		gpio_set_value(info->gpio_nreset, 1);
>> +		msleep(20);
>> +	}
>> +	info->power = 1;
>> +
>> +	v4l2_dbg(1, debug,&info->sd,  "%s: sensor is on\n", __func__);
>> +	return 0;
>> +}
>> +
>> +static int power_disable(struct noon010_info *info)
>> +{
>> +	int ret;
>> +
>> +	if (!info->power) {
>> +		v4l2_info(&info->sd, "%s: sensor is already off\n", __func__);
>> +		return 0;
>> +	}
>> +
>> +	ret = regulator_bulk_disable(NOON010_NUM_SUPPLIES, info->supply);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (gpio_is_valid(info->gpio_nstby))
>> +		gpio_set_value(info->gpio_nstby, 0);
>> +
>> +	if (gpio_is_valid(info->gpio_nreset))
>> +		gpio_set_value(info->gpio_nreset, 0);
>> +
>> +	info->power = 0;
>> +
>> +	v4l2_dbg(1, debug,&info->sd,  "%s: sensor is off\n", __func__);
>> +
>> +	return 0;
>> +}
>> +
>> +static int noon010_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct v4l2_subdev *sd = to_sd(ctrl);
>> +	int ret;
>> +
>> +	v4l2_dbg(1, debug, sd, "%s: ctrl_id: %d, value: %d\n",
>> +		 __func__, ctrl->id, ctrl->val);
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_AUTO_WHITE_BALANCE:
>> +		ret = noon010_enable_autowhitebalance(sd, ctrl->val);
>
> Just do:
>
> 		return noon010_enable_autowhitebalance(sd, ctrl->val);
>
> Ditto for the lines below. This removes the need for the ret variable.

Yes, that's much better.

>
>> +		break;
>> +	case V4L2_CID_BLUE_BALANCE:
>> +		ret = cam_i2c_write(sd, MWB_BGAIN_REG, ctrl->val);
>> +		break;
>> +	case V4L2_CID_RED_BALANCE:
>> +		ret = cam_i2c_write(sd, MWB_RGAIN_REG, ctrl->val);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int noon010_enum_fmt(struct v4l2_subdev *sd, unsigned int index,
>> +			    enum v4l2_mbus_pixelcode *code)
>> +{
>> +	if (!code || index>= ARRAY_SIZE(noon010_formats))
>> +		return -EINVAL;
>> +
>> +	*code = noon010_formats[index].code;
>> +	return 0;
>> +}
>> +
>> +static int noon010_g_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +	int ret;
>> +
>> +	if (!mf)
>> +		return -EINVAL;
>> +
>> +	if (!info->curr_win || !info->curr_fmt) {
>> +		ret = noon010_set_params(sd);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	mf->width	= info->curr_win->width;
>> +	mf->height	= info->curr_win->height;
>> +	mf->code	= info->curr_fmt->code;
>> +	mf->colorspace	= info->curr_fmt->colorspace;
>> +	mf->field	= V4L2_FIELD_NONE;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Return nearest media bus frame format. */
>> +static const struct noon010_format *try_fmt(struct v4l2_subdev *sd,
>> +					    struct v4l2_mbus_framefmt *mf)
>> +{
>> +	int i = ARRAY_SIZE(noon010_formats);
>> +
>> +	noon010_try_frame_size(mf);
>> +
>> +	while (i--)
>> +		if (mf->code == noon010_formats[i].code)
>> +			break;
>> +
>> +	mf->code = noon010_formats[i].code;
>> +
>> +	return&noon010_formats[i];
>> +}
>> +
>> +static int noon010_try_fmt(struct v4l2_subdev *sd,
>> +			   struct v4l2_mbus_framefmt *mf)
>> +{
>> +	if (!sd || !mf)
>> +		return -EINVAL;
>> +
>> +	try_fmt(sd, mf);
>> +	return 0;
>> +}
>> +
>> +static int noon010_s_fmt(struct v4l2_subdev *sd,
>> +			 struct v4l2_mbus_framefmt *mf)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	if (!sd || !mf)
>> +		return -EINVAL;
>> +
>> +	info->curr_fmt = try_fmt(sd, mf);
>> +
>> +	return noon010_set_params(sd);
>> +}
>> +
>> +static int noon010_base_config(struct v4l2_subdev *sd)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +	int ret;
>> +
>> +	ret = noon010_bulk_write_reg(sd, noon010_base_regs);
>> +	if (!ret) {
>> +		info->curr_fmt =&noon010_formats[0];
>> +		info->curr_win =&noon010_sizes[0];
>> +		ret = noon010_set_params(sd);
>> +	}
>> +	if (!ret)
>> +		ret = noon010_set_flip(sd, 1, 0);
>> +	if (!ret)
>> +		ret = noon010_power_ctrl(sd, false, false);
>> +
>> +	/* sync the handler and the registers state */
>> +	v4l2_ctrl_handler_setup(&to_noon010(sd)->hdl);
>> +	return ret;
>> +}
>> +
>> +static int noon010_s_config(struct v4l2_subdev *sd,
>> +			    int irq, void *platform_data)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	info->pdata = platform_data;
>> +	return 0;
>> +}
>
> Why do you need this? This shouldn't be used in new drivers. It's for
> backwards compatibility with older kernels and it should probably be
> removed altogether.

I do not need it for anything special. Just don't know why I was
thinking it is needed for v4l2_new_i2c_subdev_board() to work.

>
>> +static int noon010_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +	return 0;
>> +}
>
> This doesn't do anything, so you can just drop this function.

Thanks for pointing this out. Just added a check for -ENOIOCTLCMD
in the host driver so it doesn't fail when s_stream is null.

>
>> +static int noon010_s_power(struct v4l2_subdev *sd, int on)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +	const struct noon010pc30_platform_data *pdata = info->pdata;
>> +	int ret = 0;
>> +
>> +	if (WARN(pdata == NULL, "No platform data!\n"))
>> +		return -ENOMEM;
>> +
>> +	if (on) {
>> +		noon010_power_ctrl(sd, false, true);
>> +		ret = power_enable(info);
>> +		if (ret)
>> +			return ret;
>> +		ret = noon010_base_config(sd);
>> +	} else {
>> +		ret = power_disable(info);
>> +		info->curr_win = NULL;
>> +		info->curr_fmt = NULL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int noon010_g_chip_ident(struct v4l2_subdev *sd,
>> +				struct v4l2_dbg_chip_ident *chip)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +	return v4l2_chip_ident_i2c_client(client, chip,
>> +					  V4L2_IDENT_NOON010PC30, 0);
>> +}
>> +
>> +static int noon010_log_status(struct v4l2_subdev *sd)
>> +{
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	v4l2_ctrl_handler_log_status(&info->hdl, sd->name);
>> +	return 0;
>> +}
>> +
>> +static const struct v4l2_ctrl_ops noon010_ctrl_ops = {
>> +	.s_ctrl = noon010_s_ctrl,
>> +};
>> +
>> +static const struct v4l2_subdev_core_ops noon010_core_ops = {
>> +	.g_chip_ident	= noon010_g_chip_ident,
>> +	.s_config	= noon010_s_config,
>> +	.s_power	= noon010_s_power,
>> +	.g_ctrl		= v4l2_subdev_g_ctrl,
>> +	.s_ctrl		= v4l2_subdev_s_ctrl,
>> +	.queryctrl	= v4l2_subdev_queryctrl,
>> +	.querymenu	= v4l2_subdev_querymenu,
>
> Also add:
>
>          .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	= noon010_log_status,
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops noon010_video_ops = {
>> +	.s_stream	= noon010_s_stream,
>> +	.g_mbus_fmt	= noon010_g_fmt,
>> +	.s_mbus_fmt	= noon010_s_fmt,
>> +	.try_mbus_fmt	= noon010_try_fmt,
>> +	.enum_mbus_fmt	= noon010_enum_fmt,
>> +};
>> +
>> +static const struct v4l2_subdev_ops noon010_ops = {
>> +	.core	=&noon010_core_ops,
>> +	.video	=&noon010_video_ops,
>> +};
>> +
>> +/* Return 0 if NOON010PC30L sensor type was detected or -ENODEV otherwise. */
>> +static int noon010_detect(struct i2c_client *client, struct noon010_info *info)
>> +{
>> +	int ret;
>> +
>> +	ret = power_enable(info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(client, DEVICE_ID_REG);
>> +	if (ret<  0)
>> +		dev_err(&client->dev, "I2C read failed: 0x%X\n", ret);
>> +
>> +	power_disable(info);
>> +
>> +	return ret == NOON010PC30_ID ? 0 : -ENODEV;
>> +}
>> +
>> +static int noon010_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct noon010_info *info;
>> +	struct v4l2_subdev *sd;
>> +	const struct noon010pc30_platform_data *pdata
>> +		= client->dev.platform_data;
>> +	int ret;
>> +	int i;
>> +
>> +	if (!pdata) {
>> +		dev_err(&client->dev, "No platform data!\n");
>> +		return -EIO;
>> +	}
>> +
>> +	info = kzalloc(sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	sd =&info->sd;
>> +	strcpy(sd->name, MODULE_NAME);
>
> strlcpy
>
>> +	v4l2_i2c_subdev_init(sd, client,&noon010_ops);
>> +
>> +	v4l2_ctrl_handler_init(&info->hdl, 3);
>> +
>> +	v4l2_ctrl_new_std(&info->hdl,&noon010_ctrl_ops,
>> +			  V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
>> +	v4l2_ctrl_new_std(&info->hdl,&noon010_ctrl_ops,
>> +			  V4L2_CID_RED_BALANCE, 0, 127, 1, 64);
>> +	v4l2_ctrl_new_std(&info->hdl,&noon010_ctrl_ops,
>> +			  V4L2_CID_BLUE_BALANCE, 0, 127, 1, 64);
>> +
>> +	sd->ctrl_handler =&info->hdl;
>> +
>> +	ret = info->hdl.error;
>> +	if (ret) {
>> +		v4l2_ctrl_handler_free(&info->hdl);
>
> The v4l2_ctrl_handler_free call should be part of the 'exit' labels at the end...

Oh, that's bad :/ Thanks.

>
>> +		goto np_err;
>> +	}
>> +
>> +	info->pdata		= client->dev.platform_data;
>> +	info->i2c_reg_page	= -1;
>> +	info->gpio_nreset	= -EINVAL;
>> +	info->gpio_nstby	= -EINVAL;
>> +
>> +	if (gpio_is_valid(pdata->gpio_nreset)) {
>> +		ret = gpio_request(pdata->gpio_nreset, "NOON010PC30 NRST");
>> +		if (ret) {
>> +			dev_err(&client->dev, "GPIO request error: %d\n", ret);
>> +			goto np_err;
>
> ...because otherwise it will never be freed here and in the other exit paths
> below.
>
>> +		}
>> +		info->gpio_nreset = pdata->gpio_nreset;
>> +		gpio_direction_output(info->gpio_nreset, 0);
>> +		gpio_export(info->gpio_nreset, 0);
>> +	}
>> +
>> +	if (gpio_is_valid(pdata->gpio_nstby)) {
>> +		ret = gpio_request(pdata->gpio_nstby, "NOON010PC30 NSTBY");
>> +		if (ret) {
>> +			dev_err(&client->dev, "GPIO request error: %d\n", ret);
>> +			goto gpio_stby_err;
>> +		}
>> +		info->gpio_nstby = pdata->gpio_nstby;
>> +		gpio_direction_output(info->gpio_nstby, 0);
>> +		gpio_export(info->gpio_nstby, 0);
>> +	}
>> +
>> +	for (i = 0; i<  NOON010_NUM_SUPPLIES; i++)
>> +		info->supply[i].supply = supply_name[i];
>> +
>> +	ret = regulator_bulk_get(&client->dev, NOON010_NUM_SUPPLIES,
>> +			info->supply);
>
> ret is assigned but not checked?

Hmm, that must have been two lines deleted too much ;)

>
>> +	ret = noon010_detect(client, info);
>> +	if (!ret)
>> +		return 0;
>> +
>> +	/* the sensor detection failed */
>> +	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
>> +	if (gpio_is_valid(info->gpio_nstby))
>> +		gpio_free(info->gpio_nstby);
>> +gpio_stby_err:
>> +	if (gpio_is_valid(info->gpio_nreset))
>> +		gpio_free(info->gpio_nreset);
>> +np_err:
>> +	v4l2_device_unregister_subdev(sd);
>> +	kfree(info);
>> +	return ret;
>> +}
>> +
>> +static int noon010_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct noon010_info *info = to_noon010(sd);
>> +
>> +	v4l2_device_unregister_subdev(sd);
>
> Missing v4l2_ctrl_handler_free.
>
>> +	regulator_bulk_free(NOON010_NUM_SUPPLIES, info->supply);
>> +	if (gpio_is_valid(info->gpio_nreset))
>> +		gpio_free(info->gpio_nreset);
>> +	if (gpio_is_valid(info->gpio_nstby))
>> +		gpio_free(info->gpio_nstby);
>> +	kfree(info);
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id noon010_id[] = {
>> +	{ MODULE_NAME, 0 },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, noon010_id);
>> +
>> +
>> +static struct i2c_driver noon010_i2c_driver = {
>> +	.driver = {
>> +		.name = MODULE_NAME
>> +	},
>> +	.probe		= noon010_probe,
>> +	.remove		= noon010_remove,
>> +	.id_table	= noon010_id,
>> +};
>> +
>> +static int __init noon010_init(void)
>> +{
>> +	return i2c_add_driver(&noon010_i2c_driver);
>> +}
>> +
>> +static void __exit noon010_exit(void)
>> +{
>> +	i2c_del_driver(&noon010_i2c_driver);
>> +}
>> +
>> +module_init(noon010_init);
>> +module_exit(noon010_exit);
>> +
>> +MODULE_DESCRIPTION("Siliconfile NOON010PC30 camera driver");
>> +MODULE_AUTHOR("Sylwester Nawrocki<s.nawrocki@samsung.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/media/noon010pc30.h b/include/media/noon010pc30.h
>> new file mode 100644
>> index 0000000..58eafee
>> --- /dev/null
>> +++ b/include/media/noon010pc30.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * Driver header for NOON010PC30L camera sensor chip.
>> + *
>> + * Copyright (c) 2010 Samsung Electronics, Co. Ltd
>> + * Contact: Sylwester Nawrocki<s.nawrocki@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 NOON010PC30_H
>> +#define NOON010PC30_H
>> +
>> +/**
>> + * @clk_rate: the clock frequency in Hz
>> + * @gpio_nreset: GPIO driving nRESET pin
>> + * @gpio_nstby: GPIO driving nSTBY pin
>> + */
>> +
>> +struct noon010pc30_platform_data {
>> +	unsigned long clk_rate;
>> +	int gpio_nreset;
>> +	int gpio_nstby;
>> +};
>> +
>> +#endif /* NOON010PC30_H */
>>
>
> Regards,
>
> 	Hans
>


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2010-12-20 23:32 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <1291638298-31926-1-git-send-email-s.nawrocki@samsung.com>
     [not found] ` <1291638298-31926-3-git-send-email-s.nawrocki@samsung.com>
2010-12-20 13:01   ` [PATCH 2/2 v2] [media] Add v4l2 subdev driver for NOON010PC30L image sensor Hans Verkuil
2010-12-20 23:32     ` Sylwester Nawrocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.