From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mga09.intel.com ([134.134.136.24]:8880 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726872AbeINQzo (ORCPT ); Fri, 14 Sep 2018 12:55:44 -0400 Date: Fri, 14 Sep 2018 14:41:32 +0300 From: Sakari Ailus To: Ping-chung Chen Cc: linux-media@vger.kernel.org, andy.yeh@intel.com, jim.lai@intel.com, tfiga@chromium.org, grundler@chromium.org, rajmohan.mani@intel.com Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver Message-ID: <20180914114131.jqq737k3qug2tdff@paasikivi.fi.intel.com> References: <1533712560-17357-1-git-send-email-ping-chung.chen@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1533712560-17357-1-git-send-email-ping-chung.chen@intel.com> Sender: linux-media-owner@vger.kernel.org List-ID: Hi Ping-chung, My apologies for the late review. On Wed, Aug 08, 2018 at 03:16:00PM +0800, Ping-chung Chen wrote: > From: "Chen, Ping-chung" > > Add a V4L2 sub-device driver for the Sony IMX208 image sensor. > This is a camera sensor using the I2C bus for control and the > CSI-2 bus for data. > > Signed-off-by: Ping-Chung Chen > Reviewed-by: Tomasz Figa > > --- > since v1: > -- Update the function media_entity_pads_init for upstreaming. > -- Change the structure name mutex as imx208_mx. > -- Refine the control flow of test pattern function. > -- vflip/hflip control support (will impact the output bayer order) > -- support 4 bayer orders output (via change v/hflip) > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > -- Simplify error handling in the set_stream function. > since v2: > -- Refine coding style. > -- Fix the if statement to use pm_runtime_get_if_in_use(). > -- Print more error log during error handling. > -- Remove mutex_destroy() from imx208_free_controls(). > -- Add more comments. > since v3: > -- Set explicit indices to link frequencies. > since v4: > -- Fix the wrong index in link_freq_menu_items. > > MAINTAINERS | 7 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/imx208.c | 995 +++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1014 insertions(+) > create mode 100644 drivers/media/i2c/imx208.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index bbd9b9b..896c1df 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -13268,6 +13268,13 @@ S: Maintained > F: drivers/ssb/ > F: include/linux/ssb/ > > +SONY IMX208 SENSOR DRIVER > +M: Sakari Ailus > +L: linux-media@vger.kernel.org > +T: git git://linuxtv.org/media_tree.git > +S: Maintained > +F: drivers/media/i2c/imx208.c > + > SONY IMX258 SENSOR DRIVER > M: Sakari Ailus > L: linux-media@vger.kernel.org > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 8669853..ae11f1e 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -595,6 +595,17 @@ config VIDEO_APTINA_PLL > config VIDEO_SMIAPP_PLL > tristate > > +config VIDEO_IMX208 > + tristate "Sony IMX208 sensor support" > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > + depends on MEDIA_CAMERA_SUPPORT > + ---help--- > + This is a Video4Linux2 sensor driver for the Sony > + IMX208 camera. > + > + To compile this driver as a module, choose M here: the > + module will be called imx208. > + > config VIDEO_IMX258 > tristate "Sony IMX258 sensor support" > depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index 837c428..47604c2 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -104,6 +104,7 @@ obj-$(CONFIG_VIDEO_I2C) += video-i2c.o > obj-$(CONFIG_VIDEO_ML86V7667) += ml86v7667.o > obj-$(CONFIG_VIDEO_OV2659) += ov2659.o > obj-$(CONFIG_VIDEO_TC358743) += tc358743.o > +obj-$(CONFIG_VIDEO_IMX208) += imx208.o > obj-$(CONFIG_VIDEO_IMX258) += imx258.o > obj-$(CONFIG_VIDEO_IMX274) += imx274.o > > diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c > new file mode 100644 > index 0000000..61de0c8 > --- /dev/null > +++ b/drivers/media/i2c/imx208.c > @@ -0,0 +1,995 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (C) 2018 Intel Corporation > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define IMX208_REG_MODE_SELECT 0x0100 > +#define IMX208_MODE_STANDBY 0x00 > +#define IMX208_MODE_STREAMING 0x01 > + > +/* Chip ID */ > +#define IMX208_REG_CHIP_ID 0x0000 > +#define IMX208_CHIP_ID 0x0208 > + > +/* V_TIMING internal */ > +#define IMX208_REG_VTS 0x0340 > +#define IMX208_VTS_60FPS 0x0472 > +#define IMX208_VTS_BINNING 0x0239 > +#define IMX208_VTS_60FPS_MIN 0x0458 > +#define IMX208_VTS_BINNING_MIN 0x0230 > +#define IMX208_VTS_MAX 0xffff > + > +/* HBLANK control - read only */ > +#define IMX208_PPL_384MHZ 2248 > +#define IMX208_PPL_96MHZ 2248 Does this generally depend on the link frequency? > + > +/* Exposure control */ > +#define IMX208_REG_EXPOSURE 0x0202 > +#define IMX208_EXPOSURE_MIN 4 > +#define IMX208_EXPOSURE_STEP 1 > +#define IMX208_EXPOSURE_DEFAULT 0x190 > +#define IMX208_EXPOSURE_MAX 65535 > + > +/* Analog gain control */ > +#define IMX208_REG_ANALOG_GAIN 0x0204 > +#define IMX208_ANA_GAIN_MIN 0 > +#define IMX208_ANA_GAIN_MAX 0x00e0 > +#define IMX208_ANA_GAIN_STEP 1 > +#define IMX208_ANA_GAIN_DEFAULT 0x0 > + > +/* Digital gain control */ > +#define IMX208_REG_GR_DIGITAL_GAIN 0x020e > +#define IMX208_REG_R_DIGITAL_GAIN 0x0210 > +#define IMX208_REG_B_DIGITAL_GAIN 0x0212 > +#define IMX208_REG_GB_DIGITAL_GAIN 0x0214 > +#define IMX208_DGTL_GAIN_MIN 0 > +#define IMX208_DGTL_GAIN_MAX 4096 > +#define IMX208_DGTL_GAIN_DEFAULT 0x100 > +#define IMX208_DGTL_GAIN_STEP 1 > + > +/* Orientation */ > +#define IMX208_REG_ORIENTATION_CONTROL 0x0101 > + > +/* Test Pattern Control */ > +#define IMX208_REG_TEST_PATTERN_MODE 0x0600 > +#define IMX208_TEST_PATTERN_DISABLE 0x0 > +#define IMX208_TEST_PATTERN_SOLID_COLOR 0x1 > +#define IMX208_TEST_PATTERN_COLOR_BARS 0x2 > +#define IMX208_TEST_PATTERN_GREY_COLOR 0x3 > +#define IMX208_TEST_PATTERN_PN9 0x4 > +#define IMX208_TEST_PATTERN_FIX_1 0x100 > +#define IMX208_TEST_PATTERN_FIX_2 0x101 > +#define IMX208_TEST_PATTERN_FIX_3 0x102 > +#define IMX208_TEST_PATTERN_FIX_4 0x103 > +#define IMX208_TEST_PATTERN_FIX_5 0x104 > +#define IMX208_TEST_PATTERN_FIX_6 0x105 > + > +struct imx208_reg { > + u16 address; > + u8 val; > +}; > + > +struct imx208_reg_list { > + u32 num_of_regs; > + const struct imx208_reg *regs; > +}; > + > +/* Link frequency config */ > +struct imx208_link_freq_config { > + u32 pixels_per_line; > + > + /* PLL registers for this link frequency */ > + struct imx208_reg_list reg_list; > +}; > + > +/* Mode : resolution and related config&values */ > +struct imx208_mode { > + /* Frame width */ > + u32 width; > + /* Frame height */ > + u32 height; > + > + /* V-timing */ > + u32 vts_def; > + u32 vts_min; > + > + /* Index of Link frequency config to be used */ > + u32 link_freq_index; > + /* Default register values */ > + struct imx208_reg_list reg_list; > +}; > + > +static const struct imx208_reg pll_ctrl_reg[] = { > + {0x0305, 0x02}, > + {0x0307, 0x50}, > + {0x303C, 0x3C}, > +}; > + > +static const struct imx208_reg mode_1936x1096_60fps_regs[] = { > + {0x0340, 0x04}, > + {0x0341, 0x72}, > + {0x0342, 0x04}, > + {0x0343, 0x64}, > + {0x034C, 0x07}, > + {0x034D, 0x90}, > + {0x034E, 0x04}, > + {0x034F, 0x48}, > + {0x0381, 0x01}, > + {0x0383, 0x01}, > + {0x0385, 0x01}, > + {0x0387, 0x01}, > + {0x3048, 0x00}, > + {0x3050, 0x01}, > + {0x30D5, 0x00}, > + {0x3301, 0x00}, > + {0x3318, 0x62}, > + {0x0202, 0x01}, > + {0x0203, 0x90}, > + {0x0205, 0x00}, > +}; > + > +static const struct imx208_reg mode_968_548_60fps_regs[] = { > + {0x0340, 0x02}, > + {0x0341, 0x39}, > + {0x0342, 0x08}, > + {0x0343, 0xC8}, > + {0x034C, 0x03}, > + {0x034D, 0xC8}, > + {0x034E, 0x02}, > + {0x034F, 0x24}, > + {0x0381, 0x01}, > + {0x0383, 0x03}, > + {0x0385, 0x01}, > + {0x0387, 0x03}, > + {0x3048, 0x01}, > + {0x3050, 0x02}, > + {0x30D5, 0x03}, > + {0x3301, 0x10}, > + {0x3318, 0x75}, > + {0x0202, 0x01}, > + {0x0203, 0x90}, > + {0x0205, 0x00}, > +}; > + > +static const char * const imx208_test_pattern_menu[] = { > + "Disabled", > + "Solid Color", > + "100% Color Bar", > + "Fade to Grey Color Bar", > + "PN9", > + "Fixed Pattern1", > + "Fixed Pattern2", > + "Fixed Pattern3", > + "Fixed Pattern4", > + "Fixed Pattern5", > + "Fixed Pattern6" > +}; > + > +static const int imx208_test_pattern_val[] = { > + IMX208_TEST_PATTERN_DISABLE, > + IMX208_TEST_PATTERN_SOLID_COLOR, > + IMX208_TEST_PATTERN_COLOR_BARS, > + IMX208_TEST_PATTERN_GREY_COLOR, > + IMX208_TEST_PATTERN_PN9, > + IMX208_TEST_PATTERN_FIX_1, > + IMX208_TEST_PATTERN_FIX_2, > + IMX208_TEST_PATTERN_FIX_3, > + IMX208_TEST_PATTERN_FIX_4, > + IMX208_TEST_PATTERN_FIX_5, > + IMX208_TEST_PATTERN_FIX_6, > +}; > + > +/* Configurations for supported link frequencies */ > +#define IMX208_MHZ (1000*1000ULL) > +#define IMX208_LINK_FREQ_384MHZ (384ULL * IMX208_MHZ) > +#define IMX208_LINK_FREQ_96MHZ (96ULL * IMX208_MHZ) You could simply write these as 384000000 and 96000000. > + > +#define IMX208_DATA_RATE_DOUBLE 2 > +#define IMX208_NUM_OF_LANES 2 > +#define IMX208_PIXEL_BITS 10 > + > +enum { > + IMX208_LINK_FREQ_384MHZ_INDEX, > + IMX208_LINK_FREQ_96MHZ_INDEX, > +}; > + > +/* > + * pixel_rate = link_freq * data-rate * nr_of_lanes / bits_per_sample > + * data rate => double data rate; number of lanes => 2; bits per pixel => 10 > + */ > +static u64 link_freq_to_pixel_rate(u64 f) > +{ > + f *= IMX208_DATA_RATE_DOUBLE * IMX208_NUM_OF_LANES; > + do_div(f, IMX208_PIXEL_BITS); > + > + return f; > +} > + > +/* Menu items for LINK_FREQ V4L2 control */ > +static const s64 link_freq_menu_items[] = { > + [IMX208_LINK_FREQ_384MHZ_INDEX] = IMX208_LINK_FREQ_384MHZ, > + [IMX208_LINK_FREQ_96MHZ_INDEX] = IMX208_LINK_FREQ_96MHZ, > +}; > + > +/* Link frequency configs */ > +static const struct imx208_link_freq_config link_freq_configs[] = { > + [IMX208_LINK_FREQ_384MHZ_INDEX] = { > + .pixels_per_line = IMX208_PPL_384MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg), > + .regs = pll_ctrl_reg, > + } > + }, > + [IMX208_LINK_FREQ_96MHZ_INDEX] = { > + .pixels_per_line = IMX208_PPL_96MHZ, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(pll_ctrl_reg), > + .regs = pll_ctrl_reg, > + } > + }, > +}; > + > +/* Mode configs */ > +static const struct imx208_mode supported_modes[] = { > + { > + .width = 1936, > + .height = 1096, > + .vts_def = IMX208_VTS_60FPS, > + .vts_min = IMX208_VTS_60FPS_MIN, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_1936x1096_60fps_regs), > + .regs = mode_1936x1096_60fps_regs, > + }, > + .link_freq_index = IMX208_LINK_FREQ_384MHZ_INDEX, > + }, > + { > + .width = 968, > + .height = 548, > + .vts_def = IMX208_VTS_BINNING, > + .vts_min = IMX208_VTS_BINNING_MIN, > + .reg_list = { > + .num_of_regs = ARRAY_SIZE(mode_968_548_60fps_regs), > + .regs = mode_968_548_60fps_regs, > + }, > + .link_freq_index = IMX208_LINK_FREQ_96MHZ_INDEX, > + }, > +}; > + > +struct imx208 { > + struct v4l2_subdev sd; > + struct media_pad pad; > + > + struct v4l2_ctrl_handler ctrl_handler; > + /* V4L2 Controls */ > + struct v4l2_ctrl *link_freq; > + struct v4l2_ctrl *pixel_rate; > + struct v4l2_ctrl *vblank; > + struct v4l2_ctrl *hblank; > + struct v4l2_ctrl *vflip; > + struct v4l2_ctrl *hflip; > + > + /* Current mode */ > + const struct imx208_mode *cur_mode; > + > + /* > + * Mutex for serialized access: > + * Protect sensor set pad format and start/stop streaming safely. > + * Protect access to sensor v4l2 controls. > + */ > + struct mutex imx208_mx; How about calling it simply e.g. a "mutex"? The struct is already specific to imx208. > + > + /* Streaming on/off */ > + bool streaming; > +}; > + > +static inline struct imx208 *to_imx208(struct v4l2_subdev *_sd) > +{ > + return container_of(_sd, struct imx208, sd); > +} > + > +/* Get bayer order based on flip setting. */ > +static u32 imx208_get_format_code(struct imx208 *imx208) > +{ > + /* > + * Only one bayer order is supported. > + * It depends on the flip settings. > + */ > + static const u32 codes[2][2] = { > + { MEDIA_BUS_FMT_SRGGB10_1X10, MEDIA_BUS_FMT_SGRBG10_1X10, }, > + { MEDIA_BUS_FMT_SGBRG10_1X10, MEDIA_BUS_FMT_SBGGR10_1X10, }, > + }; > + > + return codes[imx208->vflip->val][imx208->hflip->val]; > +} > + > +/* Read registers up to 4 at a time */ > +static int imx208_read_reg(struct imx208 *imx208, u16 reg, u32 len, u32 *val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + struct i2c_msg msgs[2]; > + u8 addr_buf[2] = { reg >> 8, reg & 0xff }; > + u8 data_buf[4] = { 0, }; > + int ret; > + > + if (len > 4) > + return -EINVAL; > + > + /* Write register address */ > + msgs[0].addr = client->addr; > + msgs[0].flags = 0; > + msgs[0].len = ARRAY_SIZE(addr_buf); > + msgs[0].buf = addr_buf; > + > + /* Read data from register */ > + msgs[1].addr = client->addr; > + msgs[1].flags = I2C_M_RD; > + msgs[1].len = len; > + msgs[1].buf = &data_buf[4 - len]; > + > + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > + if (ret != ARRAY_SIZE(msgs)) > + return -EIO; > + > + *val = get_unaligned_be32(data_buf); > + > + return 0; > +} > + > +/* Write registers up to 4 at a time */ > +static int imx208_write_reg(struct imx208 *imx208, u16 reg, u32 len, u32 val) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + u8 buf[6]; > + > + if (len > 4) > + return -EINVAL; > + > + put_unaligned_be16(reg, buf); > + put_unaligned_be32(val << (8 * (4 - len)), buf + 2); > + if (i2c_master_send(client, buf, len + 2) != len + 2) > + return -EIO; > + > + return 0; > +} > + > +/* Write a list of registers */ > +static int imx208_write_regs(struct imx208 *imx208, > + const struct imx208_reg *regs, u32 len) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + unsigned int i; > + int ret; > + > + for (i = 0; i < len; i++) { > + ret = imx208_write_reg(imx208, regs[i].address, 1, > + regs[i].val); > + if (ret) { > + dev_err_ratelimited( > + &client->dev, > + "Failed to write reg 0x%4.4x. error = %d\n", > + regs[i].address, ret); > + > + return ret; > + } > + } > + > + return 0; > +} > + > +/* Open sub-device */ > +static int imx208_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct v4l2_mbus_framefmt *try_fmt = > + v4l2_subdev_get_try_format(sd, fh->pad, 0); > + > + /* Initialize try_fmt */ > + try_fmt->width = supported_modes[0].width; > + try_fmt->height = supported_modes[0].height; > + try_fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10; > + try_fmt->field = V4L2_FIELD_NONE; > + > + return 0; > +} > + > + > +static int imx208_update_digital_gain(struct imx208 *imx208, u32 len, u32 val) > +{ > + int ret; > + > + ret = imx208_write_reg(imx208, IMX208_REG_GR_DIGITAL_GAIN, 2, val); > + if (ret) > + return ret; > + > + ret = imx208_write_reg(imx208, IMX208_REG_GB_DIGITAL_GAIN, 2, val); > + if (ret) > + return ret; > + > + ret = imx208_write_reg(imx208, IMX208_REG_R_DIGITAL_GAIN, 2, val); > + if (ret) > + return ret; > + > + return imx208_write_reg(imx208, IMX208_REG_B_DIGITAL_GAIN, 2, val); > +} > + > +static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct imx208 *imx208 = > + container_of(ctrl->handler, struct imx208, ctrl_handler); > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + int ret; > + > + /* > + * Applying V4L2 control value only happens > + * when power is up for streaming > + */ > + if (!pm_runtime_get_if_in_use(&client->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_ANALOGUE_GAIN: > + ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, > + 2, ctrl->val); > + break; > + case V4L2_CID_EXPOSURE: > + ret = imx208_write_reg(imx208, IMX208_REG_EXPOSURE, > + 2, ctrl->val); > + break; > + case V4L2_CID_DIGITAL_GAIN: > + ret = imx208_update_digital_gain(imx208, 2, ctrl->val); > + break; > + case V4L2_CID_VBLANK: > + /* Update VTS that meets expected vertical blanking */ > + ret = imx208_write_reg(imx208, IMX208_REG_VTS, 2, > + imx208->cur_mode->height + ctrl->val); > + break; > + case V4L2_CID_TEST_PATTERN: > + ret = imx208_write_reg(imx208, IMX208_REG_TEST_PATTERN_MODE, > + 2, imx208_test_pattern_val[ctrl->val]); > + break; > + case V4L2_CID_HFLIP: > + case V4L2_CID_VFLIP: > + ret = imx208_write_reg(imx208, IMX208_REG_ORIENTATION_CONTROL, > + 1, > + imx208->hflip->val | > + imx208->vflip->val << 1); > + break; > + default: > + ret = -EINVAL; > + dev_err(&client->dev, > + "ctrl(id:0x%x,val:0x%x) is not handled\n", > + ctrl->id, ctrl->val); > + break; > + } > + > + pm_runtime_put(&client->dev); > + > + return ret; > +} > + > +static const struct v4l2_ctrl_ops imx208_ctrl_ops = { > + .s_ctrl = imx208_set_ctrl, > +}; > + > +static int imx208_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + struct imx208 *imx208 = to_imx208(sd); > + > + if (code->index > 0) > + return -EINVAL; > + > + code->code = imx208_get_format_code(imx208); > + > + return 0; > +} > + > +static int imx208_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + struct imx208 *imx208 = to_imx208(sd); > + > + if (fse->index >= ARRAY_SIZE(supported_modes)) > + return -EINVAL; > + > + if (fse->code != imx208_get_format_code(imx208)) > + return -EINVAL; > + > + fse->min_width = supported_modes[fse->index].width; > + fse->max_width = fse->min_width; > + fse->min_height = supported_modes[fse->index].height; > + fse->max_height = fse->min_height; > + > + return 0; > +} > + > +static void imx208_mode_to_pad_format(struct imx208 *imx208, > + const struct imx208_mode *mode, > + struct v4l2_subdev_format *fmt) > +{ > + fmt->format.width = mode->width; > + fmt->format.height = mode->height; > + fmt->format.code = imx208_get_format_code(imx208); > + fmt->format.field = V4L2_FIELD_NONE; > +} > + > +static int __imx208_get_pad_format(struct imx208 *imx208, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + fmt->format = *v4l2_subdev_get_try_format(&imx208->sd, cfg, > + fmt->pad); > + else > + imx208_mode_to_pad_format(imx208, imx208->cur_mode, fmt); > + > + return 0; > +} > + > +static int imx208_get_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx208 *imx208 = to_imx208(sd); > + int ret; > + > + mutex_lock(&imx208->imx208_mx); > + ret = __imx208_get_pad_format(imx208, cfg, fmt); > + mutex_unlock(&imx208->imx208_mx); > + > + return ret; > +} > + > +static int imx208_set_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct imx208 *imx208 = to_imx208(sd); > + const struct imx208_mode *mode; > + s32 vblank_def; > + s32 vblank_min; > + s64 h_blank; > + s64 pixel_rate; > + s64 link_freq; > + > + mutex_lock(&imx208->imx208_mx); > + > + fmt->format.code = imx208_get_format_code(imx208); > + mode = v4l2_find_nearest_size( > + supported_modes, ARRAY_SIZE(supported_modes), width, height, > + fmt->format.width, fmt->format.height); > + imx208_mode_to_pad_format(imx208, mode, fmt); > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > + *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > + } else { > + imx208->cur_mode = mode; > + __v4l2_ctrl_s_ctrl(imx208->link_freq, mode->link_freq_index); > + link_freq = link_freq_menu_items[mode->link_freq_index]; Same as on the imx319 driver --- the link frequencies that are available need to reflect what is specified in firmware. > + pixel_rate = link_freq_to_pixel_rate(link_freq); > + __v4l2_ctrl_s_ctrl_int64(imx208->pixel_rate, pixel_rate); > + /* Update limits and set FPS to default */ > + vblank_def = imx208->cur_mode->vts_def - > + imx208->cur_mode->height; > + vblank_min = imx208->cur_mode->vts_min - > + imx208->cur_mode->height; > + __v4l2_ctrl_modify_range( > + imx208->vblank, vblank_min, > + IMX208_VTS_MAX - imx208->cur_mode->height, 1, > + vblank_def); > + __v4l2_ctrl_s_ctrl(imx208->vblank, vblank_def); > + h_blank = > + link_freq_configs[mode->link_freq_index].pixels_per_line > + - imx208->cur_mode->width; > + __v4l2_ctrl_modify_range(imx208->hblank, h_blank, > + h_blank, 1, h_blank); > + } > + > + mutex_unlock(&imx208->imx208_mx); > + > + return 0; > +} > + > +/* Start streaming */ > +static int imx208_start_streaming(struct imx208 *imx208) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + const struct imx208_reg_list *reg_list; > + int ret, link_freq_index; > + > + /* Setup PLL */ > + link_freq_index = imx208->cur_mode->link_freq_index; > + reg_list = &link_freq_configs[link_freq_index].reg_list; > + ret = imx208_write_regs(imx208, reg_list->regs, reg_list->num_of_regs); > + if (ret) { > + dev_err(&client->dev, "%s failed to set plls\n", __func__); > + return ret; > + } > + > + /* Apply default values of current mode */ > + reg_list = &imx208->cur_mode->reg_list; > + ret = imx208_write_regs(imx208, reg_list->regs, reg_list->num_of_regs); > + if (ret) { > + dev_err(&client->dev, "%s failed to set mode\n", __func__); > + return ret; > + } > + > + /* Apply customized values from user */ > + ret = __v4l2_ctrl_handler_setup(imx208->sd.ctrl_handler); > + if (ret) > + return ret; > + > + /* set stream on register */ > + return imx208_write_reg(imx208, IMX208_REG_MODE_SELECT, > + 1, IMX208_MODE_STREAMING); > +} > + > +/* Stop streaming */ > +static int imx208_stop_streaming(struct imx208 *imx208) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + int ret; > + > + /* set stream off register */ > + ret = imx208_write_reg(imx208, IMX208_REG_MODE_SELECT, > + 1, IMX208_MODE_STANDBY); > + if (ret) > + dev_err(&client->dev, "%s failed to set stream\n", __func__); > + > + /* > + * Return success even if it was an error, as there is nothing the > + * caller can do about it. > + */ > + return 0; > +} > + > +static int imx208_set_stream(struct v4l2_subdev *sd, int enable) > +{ > + struct imx208 *imx208 = to_imx208(sd); > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + int ret = 0; > + > + mutex_lock(&imx208->imx208_mx); > + if (imx208->streaming == enable) { > + mutex_unlock(&imx208->imx208_mx); > + return 0; > + } > + > + if (enable) { > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) > + goto err_rpm_put; > + > + /* > + * Apply default & customized values > + * and then start streaming. > + */ > + ret = imx208_start_streaming(imx208); > + if (ret) > + goto err_rpm_put; > + } else { > + imx208_stop_streaming(imx208); > + pm_runtime_put(&client->dev); > + } > + > + imx208->streaming = enable; > + mutex_unlock(&imx208->imx208_mx); > + > + /* vflip and hflip cannot change during streaming */ > + v4l2_ctrl_grab(imx208->vflip, enable); > + v4l2_ctrl_grab(imx208->hflip, enable); Please grab before releasing the lock; use __v4l2_ctrl_grab() here: > + > + return ret; > + > +err_rpm_put: > + pm_runtime_put(&client->dev); > + mutex_unlock(&imx208->imx208_mx); > + > + return ret; > +} > + > +static int __maybe_unused imx208_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx208 *imx208 = to_imx208(sd); > + > + if (imx208->streaming) > + imx208_stop_streaming(imx208); > + > + return 0; > +} > + > +static int __maybe_unused imx208_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx208 *imx208 = to_imx208(sd); > + int ret; > + > + if (imx208->streaming) { > + ret = imx208_start_streaming(imx208); > + if (ret) > + goto error; > + } > + > + return 0; > + > +error: > + imx208_stop_streaming(imx208); > + imx208->streaming = 0; > + > + return ret; > +} > + > +/* Verify chip ID */ > +static int imx208_identify_module(struct imx208 *imx208) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + int ret; > + u32 val; > + > + ret = imx208_read_reg(imx208, IMX208_REG_CHIP_ID, > + 2, &val); Fits on a single line. > + if (ret) { > + dev_err(&client->dev, "failed to read chip id %x\n", > + IMX208_CHIP_ID); > + return ret; > + } > + > + if (val != IMX208_CHIP_ID) { > + dev_err(&client->dev, "chip id mismatch: %x!=%x\n", > + IMX208_CHIP_ID, val); > + return -EIO; > + } > + > + return 0; > +} > + > +static const struct v4l2_subdev_video_ops imx208_video_ops = { > + .s_stream = imx208_set_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops imx208_pad_ops = { > + .enum_mbus_code = imx208_enum_mbus_code, > + .get_fmt = imx208_get_pad_format, > + .set_fmt = imx208_set_pad_format, > + .enum_frame_size = imx208_enum_frame_size, > +}; > + > +static const struct v4l2_subdev_ops imx208_subdev_ops = { > + .video = &imx208_video_ops, > + .pad = &imx208_pad_ops, > +}; > + > +static const struct v4l2_subdev_internal_ops imx208_internal_ops = { > + .open = imx208_open, > +}; > + > +/* Initialize control handlers */ > +static int imx208_init_controls(struct imx208 *imx208) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&imx208->sd); > + struct v4l2_ctrl_handler *ctrl_hdlr = &imx208->ctrl_handler; > + s64 exposure_max; > + s64 vblank_def; > + s64 vblank_min; > + s64 pixel_rate_min; > + s64 pixel_rate_max; > + int ret; > + > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > + if (ret) > + return ret; > + > + mutex_init(&imx208->imx208_mx); > + ctrl_hdlr->lock = &imx208->imx208_mx; > + imx208->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, > + &imx208_ctrl_ops, > + V4L2_CID_LINK_FREQ, > + ARRAY_SIZE(link_freq_menu_items) - 1, > + 0, link_freq_menu_items); > + > + if (imx208->link_freq) > + imx208->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]); > + pixel_rate_min = link_freq_to_pixel_rate( > + link_freq_menu_items[ARRAY_SIZE( > + link_freq_menu_items) - 1]); > + /* By default, PIXEL_RATE is read only */ > + imx208->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, > + V4L2_CID_PIXEL_RATE, > + pixel_rate_min, pixel_rate_max, > + 1, pixel_rate_max); > + > + vblank_def = imx208->cur_mode->vts_def - imx208->cur_mode->height; > + vblank_min = imx208->cur_mode->vts_min - imx208->cur_mode->height; > + imx208->vblank = v4l2_ctrl_new_std( > + ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_VBLANK, > + vblank_min, > + IMX208_VTS_MAX - imx208->cur_mode->height, 1, > + vblank_def); > + > + imx208->hblank = v4l2_ctrl_new_std( > + ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_HBLANK, > + IMX208_PPL_384MHZ - imx208->cur_mode->width, > + IMX208_PPL_384MHZ - imx208->cur_mode->width, > + 1, > + IMX208_PPL_384MHZ - imx208->cur_mode->width); > + > + if (imx208->hblank) > + imx208->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY; > + > + exposure_max = imx208->cur_mode->vts_def - 8; > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_EXPOSURE, > + IMX208_EXPOSURE_MIN, IMX208_EXPOSURE_MAX, > + IMX208_EXPOSURE_STEP, IMX208_EXPOSURE_DEFAULT); > + > + imx208->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, > + V4L2_CID_HFLIP, 0, 1, 1, 0); > + imx208->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, > + V4L2_CID_VFLIP, 0, 1, 1, 0); > + > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_ANALOGUE_GAIN, > + IMX208_ANA_GAIN_MIN, IMX208_ANA_GAIN_MAX, > + IMX208_ANA_GAIN_STEP, IMX208_ANA_GAIN_DEFAULT); > + > + v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_DIGITAL_GAIN, > + IMX208_DGTL_GAIN_MIN, IMX208_DGTL_GAIN_MAX, > + IMX208_DGTL_GAIN_STEP, > + IMX208_DGTL_GAIN_DEFAULT); > + > + v4l2_ctrl_new_std_menu_items(ctrl_hdlr, &imx208_ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(imx208_test_pattern_menu) - 1, > + 0, 0, imx208_test_pattern_menu); > + > + if (ctrl_hdlr->error) { > + ret = ctrl_hdlr->error; > + dev_err(&client->dev, "%s control init failed (%d)\n", > + __func__, ret); > + goto error; > + } > + > + imx208->sd.ctrl_handler = ctrl_hdlr; > + > + return 0; > + > +error: > + v4l2_ctrl_handler_free(ctrl_hdlr); > + mutex_destroy(&imx208->imx208_mx); > + > + return ret; > +} > + > +static void imx208_free_controls(struct imx208 *imx208) > +{ > + v4l2_ctrl_handler_free(imx208->sd.ctrl_handler); > +} > + > +static int imx208_probe(struct i2c_client *client) > +{ > + struct imx208 *imx208; > + int ret; > + u32 val = 0; > + > + device_property_read_u32(&client->dev, "clock-frequency", &val); > + if (val != 19200000) { > + dev_err(&client->dev, > + "Unsupported clock-frequency %u. Expected 19200000.\n", > + val); > + return -EINVAL; > + } > + > + imx208 = devm_kzalloc(&client->dev, sizeof(*imx208), GFP_KERNEL); > + if (!imx208) > + return -ENOMEM; > + > + /* Initialize subdev */ > + v4l2_i2c_subdev_init(&imx208->sd, client, &imx208_subdev_ops); > + > + /* Check module identity */ > + ret = imx208_identify_module(imx208); > + if (ret) { > + dev_err(&client->dev, "failed to find sensor: %d", ret); > + goto error_probe; > + } > + > + /* Set default mode to max resolution */ > + imx208->cur_mode = &supported_modes[0]; > + > + ret = imx208_init_controls(imx208); > + if (ret) { > + dev_err(&client->dev, "failed to init controls: %d", ret); > + goto error_probe; > + } > + > + /* Initialize subdev */ > + imx208->sd.internal_ops = &imx208_internal_ops; > + imx208->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + imx208->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + /* Initialize source pad */ > + imx208->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&imx208->sd.entity, 1, &imx208->pad); > + if (ret) { > + dev_err(&client->dev, "%s failed:%d\n", __func__, ret); > + goto error_handler_free; > + } > + > + ret = v4l2_async_register_subdev_sensor_common(&imx208->sd); > + if (ret < 0) > + goto error_media_entity; > + > + pm_runtime_set_active(&client->dev); > + pm_runtime_enable(&client->dev); > + pm_runtime_idle(&client->dev); > + > + return 0; > + > +error_media_entity: > + media_entity_cleanup(&imx208->sd.entity); > + > +error_handler_free: > + imx208_free_controls(imx208); > + > +error_probe: > + mutex_destroy(&imx208->imx208_mx); > + > + return ret; > +} > + > +static int imx208_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx208 *imx208 = to_imx208(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + imx208_free_controls(imx208); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_set_suspended(&client->dev); > + > + mutex_destroy(&imx208->imx208_mx); > + > + return 0; > +} > + > +static const struct dev_pm_ops imx208_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(imx208_suspend, imx208_resume) > +}; > + > +#ifdef CONFIG_ACPI > +static const struct acpi_device_id imx208_acpi_ids[] = { > + { "INT3478" }, > + { /* sentinel */ } > +}; > + > +MODULE_DEVICE_TABLE(acpi, imx208_acpi_ids); > +#endif > + > +static struct i2c_driver imx208_i2c_driver = { > + .driver = { > + .name = "imx208", > + .pm = &imx208_pm_ops, > + .acpi_match_table = ACPI_PTR(imx208_acpi_ids), > + }, > + .probe_new = imx208_probe, > + .remove = imx208_remove, > +}; > + > +module_i2c_driver(imx208_i2c_driver); > + > +MODULE_AUTHOR("Yeh, Andy "); > +MODULE_AUTHOR("Chen, Ping-chung "); > +MODULE_DESCRIPTION("Sony IMX208 sensor driver"); > +MODULE_LICENSE("GPL v2"); -- Kind regards, Sakari Ailus sakari.ailus@linux.intel.com