From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-bw0-f45.google.com ([209.85.214.45]:38920 "EHLO mail-bw0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933273Ab0LTXcs (ORCPT ); Mon, 20 Dec 2010 18:32:48 -0500 Received: by bwz16 with SMTP id 16so3929618bwz.4 for ; Mon, 20 Dec 2010 15:32:46 -0800 (PST) Message-ID: <4D0FE79A.9020506@gmail.com> Date: Tue, 21 Dec 2010 00:32:42 +0100 From: Sylwester Nawrocki MIME-Version: 1.0 To: Hans Verkuil CC: Sylwester Nawrocki , linux-media@vger.kernel.org, m.szyprowski@samsung.com, kyungmin.park@samsung.com Subject: Re: [PATCH 2/2 v2] [media] Add v4l2 subdev driver for NOON010PC30L image sensor References: <1291638298-31926-1-git-send-email-s.nawrocki@samsung.com> <1291638298-31926-3-git-send-email-s.nawrocki@samsung.com> <201012201401.09935.hverkuil@xs4all.nl> In-Reply-To: <201012201401.09935.hverkuil@xs4all.nl> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-ID: Sender: Mauro Carvalho Chehab 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 > > 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 >> Signed-off-by: Kyungmin Park >> --- >> 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, >> + * >> + * Initial register configuration based on a driver authored by >> + * HeungJun Kim. >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +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"); >> +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 >> + * >> + * 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 >