[{"id":1759146,"web_url":"http://patchwork.ozlabs.org/comment/1759146/","msgid":"<20170829085848.atlfarekyw4h43cl@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-08-29T08:58:49","subject":"Re: [PATCH v2 1/2] media:imx274 device tree binding file","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Sören,\n\nOn Mon, Aug 28, 2017 at 06:40:25PM -0700, Soren Brinkmann wrote:\n> From: Leon Luo <leonl@leopardimaging.com>\n> \n> The binding file for imx274 CMOS sensor V4l2 driver\n> \n> Signed-off-by: Leon Luo <leonl@leopardimaging.com>\n> Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>\n> ---\n>  .../devicetree/bindings/media/i2c/imx274.txt       | 32 ++++++++++++++++++++++\n>  1 file changed, 32 insertions(+)\n>  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx274.txt\n> \n> diff --git a/Documentation/devicetree/bindings/media/i2c/imx274.txt b/Documentation/devicetree/bindings/media/i2c/imx274.txt\n> new file mode 100644\n> index 000000000000..9154666d1149\n> --- /dev/null\n> +++ b/Documentation/devicetree/bindings/media/i2c/imx274.txt\n> @@ -0,0 +1,32 @@\n> +* Sony 1/2.5-Inch 8.51Mp CMOS Digital Image Sensor\n> +\n> +The Sony imx274 is a 1/2.5-inch CMOS active pixel digital image sensor with\n> +an active array size of 3864H x 2202V. It is programmable through I2C\n> +interface. The I2C address is fixed to 0x1a as per sensor data sheet.\n> +Image data is sent through MIPI CSI-2, which is configured as 4 lanes\n> +at 1440 Mbps.\n> +\n> +\n> +Required Properties:\n> +- compatible: value should be \"sony,imx274\" for imx274 sensor\n\n\"reg\"\n\n> +\n> +Optional Properties:\n> +- reset-gpios: Sensor reset GPIO\n> +\n> +For further reading on port node refer to\n> +Documentation/devicetree/bindings/media/video-interfaces.txt.\n> +\n> +Example:\n> +\timx274: sensor@1a{\n\nAny use for the reference? If not, I'd omit it. For the endpoint you'll\nneed it.\n\n> +\t\tcompatible = \"sony,imx274\";\n> +\t\treg = <0x1a>;\n> +\t\t#address-cells = <1>;\n> +\t\t#size-cells = <0>;\n> +\t\treset-gpios = <&gpio_sensor 0 0>;\n> +\t\tport@0 {\n\n\"@0\" is redundant here, as is reg below.\n\n> +\t\t\treg = <0>;\n> +\t\t\tsensor_out: endpoint {\n> +\t\t\t\tremote-endpoint = <&csiss_in>;\n> +\t\t\t};\n> +\t\t};\n> +\t};","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhMyJ0wqgz9t0F\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tTue, 29 Aug 2017 18:59:20 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751449AbdH2I66 (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 29 Aug 2017 04:58:58 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:49542 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1750909AbdH2I6w (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 29 Aug 2017 04:58:52 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id 1041F600E3;\n\tTue, 29 Aug 2017 11:58:50 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from <sakke@valkosipuli.retiisi.org.uk>)\n\tid 1dmcMD-0001Gz-DF; Tue, 29 Aug 2017 11:58:49 +0300"],"Date":"Tue, 29 Aug 2017 11:58:49 +0300","From":"Sakari Ailus <sakari.ailus@iki.fi>","To":"Soren Brinkmann <soren.brinkmann@xilinx.com>","Cc":"mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,\n\thans.verkuil@cisco.com, sakari.ailus@linux.intel.com,\n\tlinux-media@vger.kernel.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, Leon Luo <leonl@leopardimaging.com>","Subject":"Re: [PATCH v2 1/2] media:imx274 device tree binding file","Message-ID":"<20170829085848.atlfarekyw4h43cl@valkosipuli.retiisi.org.uk>","References":"<20170829014026.26551-1-soren.brinkmann@xilinx.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20170829014026.26551-1-soren.brinkmann@xilinx.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1759165,"web_url":"http://patchwork.ozlabs.org/comment/1759165/","msgid":"<20170829093501.ijsnmzyl6d7xbpxc@valkosipuli.retiisi.org.uk>","list_archive_url":null,"date":"2017-08-29T09:35:01","subject":"Re: [PATCH v2 2/2] media:imx274 V4l2 driver for Sony imx274 CMOS\n\tsensor","submitter":{"id":1593,"url":"http://patchwork.ozlabs.org/api/people/1593/","name":"Sakari Ailus","email":"sakari.ailus@iki.fi"},"content":"Hi Sören,\n\nThanks for the patch. Some comments below.\n\nOn Mon, Aug 28, 2017 at 06:40:26PM -0700, Soren Brinkmann wrote:\n> From: Leon Luo <leonl@leopardimaging.com>\n> \n> The imx274 is a Sony CMOS image sensor that has 1/2.5 image size.\n> It supports up to 3840x2160 (4K) 60fps, 1080p 120fps. The interface\n> is 4-lane MIPI running at 1.44Gbps each.\n\n\"MIPI CSI-2\" I believe.\n\n> \n> This driver has been tested on Xilinx ZCU102 platform with a Leopard\n> LI-IMX274MIPI-FMC camera board.\n> \n> Support for the following features:\n> -Resolutions: 3840x2160, 1920x1080, 1280x720\n> -Frame rate: 3840x2160 : 5 – 60fps\n>             1920x1080 : 5 – 120fps\n>             1280x720 : 5 – 120fps\n> -Exposure time: 16 – (frame interval) micro-seconds\n> -Gain: 1x - 180x\n> -VFLIP: enable/disable\n> -Test pattern: 12 test patterns\n> \n> Signed-off-by: Leon Luo <leonl@leopardimaging.com>\n> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>\n> ---\n> v2:\n>  - Fix Kconfig to not remove existing options\n> ---\n>  drivers/media/i2c/Kconfig  |    7 +\n>  drivers/media/i2c/Makefile |    1 +\n>  drivers/media/i2c/imx274.c | 1842 ++++++++++++++++++++++++++++++++++++++++++++\n>  3 files changed, 1850 insertions(+)\n>  create mode 100644 drivers/media/i2c/imx274.c\n> \n> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig\n> index 94153895fcd4..ad2e70a02363 100644\n> --- a/drivers/media/i2c/Kconfig\n> +++ b/drivers/media/i2c/Kconfig\n> @@ -547,6 +547,13 @@ config VIDEO_APTINA_PLL\n>  config VIDEO_SMIAPP_PLL\n>  \ttristate\n>  \n> +config VIDEO_IMX274\n> +\ttristate \"Sony IMX274 sensor support\"\n> +\tdepends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API\n> +\t---help---\n> +\t  This is a V4L2 sensor-level driver for the Sony IMX274\n> +\t  CMOS image sensor.\n> +\n>  config VIDEO_OV2640\n>  \ttristate \"OmniVision OV2640 sensor support\"\n>  \tdepends on VIDEO_V4L2 && I2C\n> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile\n> index c843c181dfb9..f8d57e453936 100644\n> --- a/drivers/media/i2c/Makefile\n> +++ b/drivers/media/i2c/Makefile\n> @@ -92,5 +92,6 @@ obj-$(CONFIG_VIDEO_IR_I2C)  += ir-kbd-i2c.o\n>  obj-$(CONFIG_VIDEO_ML86V7667)\t+= ml86v7667.o\n>  obj-$(CONFIG_VIDEO_OV2659)\t+= ov2659.o\n>  obj-$(CONFIG_VIDEO_TC358743)\t+= tc358743.o\n> +obj-$(CONFIG_VIDEO_IMX274)\t+= imx274.o\n>  \n>  obj-$(CONFIG_SDR_MAX2175) += max2175.o\n> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c\n> new file mode 100644\n> index 000000000000..fcbb5ad2763c\n> --- /dev/null\n> +++ b/drivers/media/i2c/imx274.c\n> @@ -0,0 +1,1842 @@\n> +/*\n> + * imx274.c - IMX274 CMOS Image Sensor driver\n> + *\n> + * Copyright (C) 2017, Leopard Imaging, Inc.\n> + *\n> + * Leon Luo <leonl@leopardimaging.com>\n> + * Edwin Zou <edwinz@leopardimaging.com>\n> + *\n> + * This program is free software; you can redistribute it and/or modify it\n> + * under the terms and conditions of the GNU General Public License,\n> + * version 2, as published by the Free Software Foundation.\n> + *\n> + * This program is distributed in the hope it will be useful, but WITHOUT\n> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or\n> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for\n> + * more details.\n> + *\n> + * You should have received a copy of the GNU General Public License\n> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.\n> + */\n> +\n> +#include <linux/debugfs.h>\n> +#include <linux/delay.h>\n> +#include <linux/gpio.h>\n> +#include <linux/gpio/consumer.h>\n> +#include <linux/i2c.h>\n> +#include <linux/kernel.h>\n> +#include <linux/media.h>\n> +#include <linux/module.h>\n> +#include <linux/mutex.h>\n> +#include <linux/of.h>\n> +#include <linux/of_device.h>\n> +#include <linux/of_gpio.h>\n> +#include <linux/ratelimit.h>\n> +#include <linux/regmap.h>\n> +#include <linux/seq_file.h>\n> +#include <linux/slab.h>\n> +#include <linux/string.h>\n> +#include <linux/uaccess.h>\n> +#include <linux/videodev2.h>\n> +#include <media/media-entity.h>\n> +#include <media/v4l2-ctrls.h>\n> +#include <media/v4l2-device.h>\n> +#include <media/v4l2-event.h>\n> +#include <media/v4l2-image-sizes.h>\n> +#include <media/v4l2-mediabus.h>\n> +#include <media/v4l2-subdev.h>\n\nYou include a lot of headers directly. Do you need them all? Such as\nlinux/debugfs.h or linux/uaccess.h.\n\n> +\n> +static int debug;\n> +module_param(debug, int, 0644);\n> +MODULE_PARM_DESC(debug, \"Debug level (0-2)\");\n\nPlease rely on dynamic debug instead, see\nDocumentation/admin-guide/dynamic-debug-howto.rst .\n\n> +\n> +/*\n> + * See \"SHR, SVR Setting\" in datasheet\n> + */\n> +#define IMX274_DEFAULT_FRAME_LENGTH\t\t(4550)\n> +#define IMX274_MAX_FRAME_LENGTH\t\t\t(0x000fffff)\n> +\n> +/*\n> + * See \"Frame Rate Adjustment\" in datasheet\n> + */\n> +#define IMX274_PIXCLK_CONST1\t\t\t(72000000)\n> +#define IMX274_PIXCLK_CONST2\t\t\t(1000000)\n> +\n> +/*\n> + * The input gain is shifted by IMX274_GAIN_SHIFT to get\n> + * decimal number. The real gain is\n> + * (float)input_gain_value / (1 << IMX274_GAIN_SHIFT)\n> + */\n> +#define IMX274_GAIN_SHIFT\t\t\t(8)\n> +#define IMX274_GAIN_SHIFT_MASK\t\t\t((1 << IMX274_GAIN_SHIFT) - 1)\n> +\n> +/*\n> + * See \"Analog Gain\" and \"Digital Gain\" in datasheet\n> + * min gain is 1X\n> + * max gain is calculated based on IMX274_GAIN_REG_MAX\n> + */\n> +#define IMX274_GAIN_REG_MAX\t\t\t(1957)\n> +#define IMX274_MIN_GAIN\t\t\t\t(0x01 << IMX274_GAIN_SHIFT)\n> +#define IMX274_MAX_ANALOG_GAIN\t\t\t((2048 << IMX274_GAIN_SHIFT)\\\n> +\t\t\t\t\t/ (2048 - IMX274_GAIN_REG_MAX))\n> +#define IMX274_MAX_DIGITAL_GAIN\t\t\t(8)\n> +#define IMX274_DEF_GAIN\t\t\t\t(20 << IMX274_GAIN_SHIFT)\n> +#define IMX274_GAIN_CONST\t\t\t(2048) /* for gain formula */\n> +\n> +/*\n> + * 1 line time in us = (HMAX / 72), minimal is 4 lines\n> + */\n> +#define IMX274_MIN_EXPOSURE_TIME\t\t(4 * 260 / 72)\n> +\n> +#define IMX274_DEFAULT_MODE\t\t\tIMX274_MODE_3840X2160\n> +#define IMX274_MAX_WIDTH\t\t\t(3840)\n> +#define IMX274_MAX_HEIGHT\t\t\t(2160)\n> +#define IMX274_MAX_FRAME_RATE\t\t\t(120)\n> +#define IMX274_MIN_FRAME_RATE\t\t\t(5)\n> +#define IMX274_DEF_FRAME_RATE\t\t\t(60)\n> +\n> +/*\n> + * register SHR is limited to (SVR value + 1) x VMAX value - 4\n> + */\n> +#define IMX274_SHR_LIMIT_CONST\t\t\t(4)\n> +\n> +/*\n> + * Constants for sensor reset delay\n> + */\n> +#define IMX274_RESET_DELAY1\t\t\t(2000)\n> +#define IMX274_RESET_DELAY2\t\t\t(2200)\n> +\n> +/*\n> + * shift and mask constants\n> + */\n> +#define IMX274_SHIFT_8_BITS\t\t\t(8)\n> +#define IMX274_SHIFT_16_BITS\t\t\t(16)\n> +#define IMX274_MASK_LSB_2_BITS\t\t\t(0x03)\n> +#define IMX274_MASK_LSB_3_BITS\t\t\t(0x07)\n> +#define IMX274_MASK_LSB_4_BITS\t\t\t(0x0f)\n> +#define IMX274_MASK_LSB_8_BITS\t\t\t(0x00ff)\n> +\n> +#define DRIVER_NAME \"IMX274\"\n> +\n> +/*\n> + * IMX274 register definitions\n> + */\n> +#define IMX274_FRAME_LENGTH_ADDR_1\t\t0x30FA /* VMAX, MSB */\n> +#define IMX274_FRAME_LENGTH_ADDR_2\t\t0x30F9 /* VMAX */\n> +#define IMX274_FRAME_LENGTH_ADDR_3\t\t0x30F8 /* VMAX, LSB */\n> +#define IMX274_SVR_REG_MSB\t\t\t0x300F /* SVR */\n> +#define IMX274_SVR_REG_LSB\t\t\t0x300E /* SVR */\n> +#define IMX274_HMAX_REG_MSB\t\t\t0x30F7 /* HMAX */\n> +#define IMX274_HMAX_REG_LSB\t\t\t0x30F6 /* HMAX */\n> +#define IMX274_COARSE_TIME_ADDR_MSB\t\t0x300D /* SHR */\n> +#define IMX274_COARSE_TIME_ADDR_LSB\t\t0x300C /* SHR */\n> +#define IMX274_ANALOG_GAIN_ADDR_LSB\t\t0x300A /* ANALOG GAIN LSB */\n> +#define IMX274_ANALOG_GAIN_ADDR_MSB\t\t0x300B /* ANALOG GAIN MSB */\n> +#define IMX274_DIGITAL_GAIN_REG\t\t\t0x3012 /* Digital Gain */\n> +#define IMX274_VFLIP_REG\t\t\t0x301A /* VERTICAL FLIP */\n> +#define IMX274_STANDBY_REG\t\t\t0x3000 /* STANDBY */\n> +\n> +#define IMX274_TABLE_WAIT_MS\t\t\t0\n> +#define IMX274_TABLE_END\t\t\t1\n> +\n> +/*\n> + * imx274 I2C operation related structure\n> + */\n> +struct reg_8 {\n> +\tu16 addr;\n> +\tu8 val;\n> +};\n> +\n> +#define imx274_reg struct reg_8\n\nPlease don't. Use the struct definition instead.\n\n> +\n> +static struct regmap_config imx274_regmap_config = {\n\nconst?\n\n> +\t.reg_bits = 16,\n> +\t.val_bits = 8,\n> +\t.cache_type = REGCACHE_RBTREE,\n> +};\n> +\n> +/*\n> + * imx274 format related structure\n> + */\n> +struct imx274_frmfmt {\n> +\tu32 mbus_code;\n> +\tenum v4l2_colorspace colorspace;\n> +\tstruct v4l2_frmsize_discrete size;\n> +\tint mode;\n\nPlease use e.g. unsigned int consistently. Elsewhere you have e.g. int /\nu32.\n\n> +};\n> +\n> +/*\n> + * imx274 test pattern related structure\n> + */\n> +enum {\n> +\tTEST_PATTERN_DISABLED = 0,\n> +\tTEST_PATTERN_ALL_000H,\n> +\tTEST_PATTERN_ALL_FFFH,\n> +\tTEST_PATTERN_ALL_555H,\n> +\tTEST_PATTERN_ALL_AAAH,\n> +\tTEST_PATTERN_VSP_5AH, /* VERTICAL STRIPE PATTERN 555H/AAAH */\n> +\tTEST_PATTERN_VSP_A5H, /* VERTICAL STRIPE PATTERN AAAH/555H */\n> +\tTEST_PATTERN_VSP_05H, /* VERTICAL STRIPE PATTERN 000H/555H */\n> +\tTEST_PATTERN_VSP_50H, /* VERTICAL STRIPE PATTERN 555H/000H */\n> +\tTEST_PATTERN_VSP_0FH, /* VERTICAL STRIPE PATTERN 000H/FFFH */\n> +\tTEST_PATTERN_VSP_F0H, /* VERTICAL STRIPE PATTERN FFFH/000H */\n> +\tTEST_PATTERN_H_COLOR_BARS,\n> +\tTEST_PATTERN_V_COLOR_BARS,\n> +};\n> +\n> +static const char * const tp_qmenu[] = {\n> +\t\"Disabled\",\n> +\t\"All 000h Pattern\",\n> +\t\"All FFFh Pattern\",\n> +\t\"All 555h Pattern\",\n> +\t\"All AAAh Pattern\",\n> +\t\"Vertical Stripe (555h / AAAh)\",\n> +\t\"Vertical Stripe (AAAh / 555h)\",\n> +\t\"Vertical Stripe (000h / 555h)\",\n> +\t\"Vertical Stripe (555h / 000h)\",\n> +\t\"Vertical Stripe (000h / FFFh)\",\n> +\t\"Vertical Stripe (FFFh / 000h)\",\n> +\t\"Horizontal Color Bars\",\n> +\t\"Vertical Color Bars\",\n> +};\n> +\n> +/*\n> + * All-pixel scan mode (10-bit)\n> + * imx274 mode1(refer to datasheet) register configuration with\n> + * 3840x2160 resolution, raw10 data and mipi four lane output\n> + */\n> +static const imx274_reg imx274_mode1_3840x2160_raw10[] = {\n> +\t{0x3004, 0x01},\n> +\t{0x3005, 0x01},\n> +\t{0x3006, 0x00},\n> +\t{0x3007, 0x02},\n> +\n> +\t{0x3018, 0xA2}, /* output XVS, HVS */\n> +\n> +\t{0x306B, 0x05},\n> +\t{0x30E2, 0x01},\n> +\t{0x30F6, 0x07}, /* HMAX, 263 */\n> +\t{0x30F7, 0x01}, /* HMAX */\n> +\n> +\t{0x30dd, 0x01}, /* crop to 2160 */\n> +\t{0x30de, 0x06},\n> +\t{0x30df, 0x00},\n> +\t{0x30e0, 0x12},\n> +\t{0x30e1, 0x00},\n> +\t{0x3037, 0x01}, /* to crop to 3840 */\n> +\t{0x3038, 0x0c},\n> +\t{0x3039, 0x00},\n> +\t{0x303a, 0x0c},\n> +\t{0x303b, 0x0f},\n> +\n> +\t{0x30EE, 0x01},\n> +\t{0x3130, 0x86},\n> +\t{0x3131, 0x08},\n> +\t{0x3132, 0x7E},\n> +\t{0x3133, 0x08},\n> +\t{0x3342, 0x0A},\n> +\t{0x3343, 0x00},\n> +\t{0x3344, 0x16},\n> +\t{0x3345, 0x00},\n> +\t{0x33A6, 0x01},\n> +\t{0x3528, 0x0E},\n> +\t{0x3554, 0x1F},\n> +\t{0x3555, 0x01},\n> +\t{0x3556, 0x01},\n> +\t{0x3557, 0x01},\n> +\t{0x3558, 0x01},\n> +\t{0x3559, 0x00},\n> +\t{0x355A, 0x00},\n> +\t{0x35BA, 0x0E},\n> +\t{0x366A, 0x1B},\n> +\t{0x366B, 0x1A},\n> +\t{0x366C, 0x19},\n> +\t{0x366D, 0x17},\n> +\t{0x3A41, 0x08},\n> +\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * Horizontal/vertical 2/2-line binning\n> + * (Horizontal and vertical weightedbinning, 10-bit)\n> + * imx274 mode3(refer to datasheet) register configuration with\n> + * 1920x1080 resolution, raw10 data and mipi four lane output\n> + */\n> +static const imx274_reg imx274_mode3_1920x1080_raw10[] = {\n> +\t{0x3004, 0x02},\n> +\t{0x3005, 0x21},\n> +\t{0x3006, 0x00},\n> +\t{0x3007, 0x11},\n> +\n> +\t{0x3018, 0xA2}, /* output XVS, HVS */\n> +\n> +\t{0x306B, 0x05},\n> +\t{0x30E2, 0x02},\n> +\n> +\t{0x30F6, 0x04}, /* HMAX, 260 */\n> +\t{0x30F7, 0x01}, /* HMAX */\n> +\n> +\t{0x30dd, 0x01}, /* to crop to 1920x1080 */\n> +\t{0x30de, 0x05},\n> +\t{0x30df, 0x00},\n> +\t{0x30e0, 0x04},\n> +\t{0x30e1, 0x00},\n> +\t{0x3037, 0x01},\n> +\t{0x3038, 0x0c},\n> +\t{0x3039, 0x00},\n> +\t{0x303a, 0x0c},\n> +\t{0x303b, 0x0f},\n> +\n> +\t{0x30EE, 0x01},\n> +\t{0x3130, 0x4E},\n> +\t{0x3131, 0x04},\n> +\t{0x3132, 0x46},\n> +\t{0x3133, 0x04},\n> +\t{0x3342, 0x0A},\n> +\t{0x3343, 0x00},\n> +\t{0x3344, 0x1A},\n> +\t{0x3345, 0x00},\n> +\t{0x33A6, 0x01},\n> +\t{0x3528, 0x0E},\n> +\t{0x3554, 0x00},\n> +\t{0x3555, 0x01},\n> +\t{0x3556, 0x01},\n> +\t{0x3557, 0x01},\n> +\t{0x3558, 0x01},\n> +\t{0x3559, 0x00},\n> +\t{0x355A, 0x00},\n> +\t{0x35BA, 0x0E},\n> +\t{0x366A, 0x1B},\n> +\t{0x366B, 0x1A},\n> +\t{0x366C, 0x19},\n> +\t{0x366D, 0x17},\n> +\t{0x3A41, 0x08},\n> +\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * Vertical 2/3 subsampling binning horizontal 3 binning\n> + * imx274 mode5(refer to datasheet) register configuration with\n> + * 1280x720 resolution, raw10 data and mipi four lane output\n> + */\n> +static const imx274_reg imx274_mode5_1280x720_raw10[] = {\n> +\t{0x3004, 0x03},\n> +\t{0x3005, 0x31},\n> +\t{0x3006, 0x00},\n> +\t{0x3007, 0x09},\n> +\n> +\t{0x3018, 0xA2}, /* output XVS, HVS */\n> +\n> +\t{0x306B, 0x05},\n> +\t{0x30E2, 0x03},\n> +\n> +\t{0x30F6, 0x04}, /* HMAX, 260 */\n> +\t{0x30F7, 0x01}, /* HMAX */\n> +\n> +\t{0x30DD, 0x01},\n> +\t{0x30DE, 0x07},\n> +\t{0x30DF, 0x00},\n> +\t{0x40E0, 0x04},\n> +\t{0x30E1, 0x00},\n> +\t{0x3030, 0xD4},\n> +\t{0x3031, 0x02},\n> +\t{0x3032, 0xD0},\n> +\t{0x3033, 0x02},\n> +\n> +\t{0x30EE, 0x01},\n> +\t{0x3130, 0xE2},\n> +\t{0x3131, 0x02},\n> +\t{0x3132, 0xDE},\n> +\t{0x3133, 0x02},\n> +\t{0x3342, 0x0A},\n> +\t{0x3343, 0x00},\n> +\t{0x3344, 0x1B},\n> +\t{0x3345, 0x00},\n> +\t{0x33A6, 0x01},\n> +\t{0x3528, 0x0E},\n> +\t{0x3554, 0x00},\n> +\t{0x3555, 0x01},\n> +\t{0x3556, 0x01},\n> +\t{0x3557, 0x01},\n> +\t{0x3558, 0x01},\n> +\t{0x3559, 0x00},\n> +\t{0x355A, 0x00},\n> +\t{0x35BA, 0x0E},\n> +\t{0x366A, 0x1B},\n> +\t{0x366B, 0x19},\n> +\t{0x366C, 0x17},\n> +\t{0x366D, 0x17},\n> +\t{0x3A41, 0x04},\n> +\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 first step register configuration for\n> + * starting stream\n> + */\n> +static const imx274_reg imx274_start_1[] = {\n> +\t{IMX274_STANDBY_REG, 0x12},\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 second step register configuration for\n> + * starting stream\n> + */\n> +static const imx274_reg imx274_start_2[] = {\n> +\t{0x3120, 0xF0}, /* clock settings */\n> +\t{0x3121, 0x00}, /* clock settings */\n> +\t{0x3122, 0x02}, /* clock settings */\n> +\t{0x3129, 0x9C}, /* clock settings */\n> +\t{0x312A, 0x02}, /* clock settings */\n> +\t{0x312D, 0x02}, /* clock settings */\n> +\n> +\t{0x310B, 0x00},\n> +\n> +\t/* PLSTMG */\n> +\t{0x304C, 0x00}, /* PLSTMG01 */\n> +\t{0x304D, 0x03},\n> +\t{0x331C, 0x1A},\n> +\t{0x331D, 0x00},\n> +\t{0x3502, 0x02},\n> +\t{0x3529, 0x0E},\n> +\t{0x352A, 0x0E},\n> +\t{0x352B, 0x0E},\n> +\t{0x3538, 0x0E},\n> +\t{0x3539, 0x0E},\n> +\t{0x3553, 0x00},\n> +\t{0x357D, 0x05},\n> +\t{0x357F, 0x05},\n> +\t{0x3581, 0x04},\n> +\t{0x3583, 0x76},\n> +\t{0x3587, 0x01},\n> +\t{0x35BB, 0x0E},\n> +\t{0x35BC, 0x0E},\n> +\t{0x35BD, 0x0E},\n> +\t{0x35BE, 0x0E},\n> +\t{0x35BF, 0x0E},\n> +\t{0x366E, 0x00},\n> +\t{0x366F, 0x00},\n> +\t{0x3670, 0x00},\n> +\t{0x3671, 0x00},\n> +\n> +\t/* PSMIPI */\n> +\t{0x3304, 0x32}, /* PSMIPI1 */\n> +\t{0x3305, 0x00},\n> +\t{0x3306, 0x32},\n> +\t{0x3307, 0x00},\n> +\t{0x3590, 0x32},\n> +\t{0x3591, 0x00},\n> +\t{0x3686, 0x32},\n> +\t{0x3687, 0x00},\n> +\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 third step register configuration for\n> + * starting stream\n> + */\n> +static const imx274_reg imx274_start_3[] = {\n> +\t{IMX274_STANDBY_REG, 0x00},\n> +\t{0x303E, 0x02}, /* SYS_MODE = 2 */\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 forth step register configuration for\n> + * starting stream\n> + */\n> +static const imx274_reg imx274_start_4[] = {\n> +\t{0x30F4, 0x00},\n> +\t{0x3018, 0xA2}, /* XHS VHS OUTUPT */\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 register configuration for stoping stream\n> + */\n> +static const imx274_reg imx274_stop[] = {\n> +\t{IMX274_STANDBY_REG, 0x01},\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 disable test pattern register configuration\n> + */\n> +static const imx274_reg imx274_tp_disabled[] = {\n> +\t{0x303C, 0x00},\n> +\t{0x377F, 0x00},\n> +\t{0x3781, 0x00},\n> +\t{0x370B, 0x00},\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 test pattern register configuration\n> + * reg 0x303D defines the test pattern modes\n> + */\n> +static imx274_reg imx274_tp_regs[] = {\n\nconst? Same for other static variables, except the i2c_driver struct.\n\n> +\t{0x303D, 0x00},\n> +\t{0x303C, 0x11},\n> +\t{0x370E, 0x01},\n> +\t{0x377F, 0x01},\n> +\t{0x3781, 0x01},\n> +\t{0x370B, 0x11},\n> +\t{IMX274_TABLE_END, 0x00}\n> +};\n> +\n> +/*\n> + * imx274 mode related structure\n> + */\n> +enum {\n> +\tIMX274_MODE_3840X2160,\n> +\tIMX274_MODE_1920X1080,\n> +\tIMX274_MODE_1280X720,\n> +\n> +\tIMX274_MODE_START_STREAM_1,\n> +\tIMX274_MODE_START_STREAM_2,\n> +\tIMX274_MODE_START_STREAM_3,\n> +\tIMX274_MODE_START_STREAM_4,\n> +\tIMX274_MODE_STOP_STREAM\n> +};\n> +\n> +static const imx274_reg *mode_table[] = {\n> +\t[IMX274_MODE_3840X2160]\t\t= imx274_mode1_3840x2160_raw10,\n> +\t[IMX274_MODE_1920X1080]\t\t= imx274_mode3_1920x1080_raw10,\n> +\t[IMX274_MODE_1280X720]\t\t= imx274_mode5_1280x720_raw10,\n> +\n> +\t[IMX274_MODE_START_STREAM_1]\t= imx274_start_1,\n> +\t[IMX274_MODE_START_STREAM_2]\t= imx274_start_2,\n> +\t[IMX274_MODE_START_STREAM_3]\t= imx274_start_3,\n> +\t[IMX274_MODE_START_STREAM_4]\t= imx274_start_4,\n> +\t[IMX274_MODE_STOP_STREAM]\t= imx274_stop,\n> +};\n> +\n> +/*\n> + * imx274 format related structure\n> + */\n> +static const struct imx274_frmfmt imx274_formats[] = {\n> +\t{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {3840, 2160},\n> +\t\tIMX274_MODE_3840X2160},\n> +\t{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1920, 1080},\n> +\t\tIMX274_MODE_1920X1080},\n> +\t{MEDIA_BUS_FMT_SRGGB10_1X10, V4L2_COLORSPACE_SRGB, {1280, 720},\n> +\t\tIMX274_MODE_1280X720},\n> +};\n> +\n> +/*\n> + * minimal frame length for each mode\n> + * refer to datasheet section \"Frame Rate Adjustment (CSI-2)\"\n> + */\n> +static int min_frame_len[] = {\n> +\t4550, /* mode 1, 4K */\n> +\t2310, /* mode 3, 1080p */\n> +\t2310 /* mode 5, 720p */\n> +};\n> +\n> +/*\n> + * minimal numbers of SHR register\n> + * refer to datasheet table \"Shutter Setting (CSI-2)\"\n> + */\n> +static int min_SHR[] = {\n> +\t12, /* mode 1, 4K */\n> +\t8, /* mode 3, 1080p */\n> +\t8 /* mode 5, 720p */\n> +};\n> +\n> +static int max_frame_rate[] = {\n> +\t60, /* mode 1 , 4K */\n> +\t120, /* mode 3, 1080p */\n> +\t120 /* mode 5, 720p */\n> +};\n> +\n> +/*\n> + * Number of clocks per internal offset period\n> + * a constant based on mode\n> + * refer to section \"Integration Time in Each Readout Drive Mode (CSI-2)\"\n> + * in the datasheet\n> + * for the implemented 3 modes, it happens to be the same number\n> + */\n> +static const int nocpiop[] = {\n> +\t112, /* mode 1 , 4K */\n> +\t112, /* mode 3, 1080p */\n> +\t112 /* mode 5, 720p */\n> +};\n> +\n> +/*\n> + * struct imx274_ctrls - imx274 ctrl structure\n> + * @handler: V4L2 ctrl handler structure\n> + * @exposure: Pointer to expsure ctrl structure\n> + * @gain: Pointer to gain ctrl structure\n> + * @vflip: Pointer to vflip ctrl structure\n> + * @test_pattern: Pointer to test pattern ctrl structure\n> + */\n> +struct imx274_ctrls {\n> +\tstruct v4l2_ctrl_handler handler;\n> +\tstruct v4l2_ctrl *exposure;\n> +\tstruct v4l2_ctrl *gain;\n> +\tstruct v4l2_ctrl *vflip;\n> +\tstruct v4l2_ctrl *test_pattern;\n> +};\n> +\n> +/*\n> + * struct stim274 - imx274 device structure\n> + * @sd: V4L2 subdevice structure\n> + * @pd: Media pad structure\n> + * @client: Pointer to I2C client\n> + * @ctrls: imx274 control structure\n> + * @format: V4L2 media bus frame format structure\n> + * @frame_rate: V4L2 frame rate structure\n> + * @regmap: Pointer to regmap structure\n> + * @reset_gpio: Pointer to reset gpio\n> + * @lock: Mutex structure\n> + * @mode_index: Resolution mode index\n> + */\n> +struct stimx274 {\n> +\tstruct v4l2_subdev sd;\n> +\tstruct media_pad pad;\n> +\tstruct i2c_client *client;\n> +\tstruct imx274_ctrls ctrls;\n> +\tstruct v4l2_mbus_framefmt format;\n> +\tstruct v4l2_fract frame_interval;\n> +\tstruct regmap *regmap;\n> +\tstruct gpio_desc *reset_gpio;\n> +\tstruct mutex lock; /* mutex lock for operations */\n> +\tu32 mode_index;\n> +};\n> +\n> +/*\n> + * Function declaration\n> + */\n> +static int imx274_set_gain(struct stimx274 *priv, s64 val);\n> +static int imx274_set_exposure(struct stimx274 *priv, s64 val);\n> +static int imx274_set_vflip(struct stimx274 *priv, int val);\n> +static int imx274_set_test_pattern(struct stimx274 *priv, int val);\n> +static int imx274_set_frame_interval(struct stimx274 *priv,\n> +\t\t\t\t     struct v4l2_fract frame_interval);\n> +\n> +static inline void msleep_range(unsigned int delay_base)\n> +{\n> +\tusleep_range(delay_base * 1000, delay_base * 1000 + 500);\n> +}\n> +\n> +/*\n> + * v4l2_ctrl and v4l2_subdev related operations\n> + */\n> +static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)\n> +{\n> +\treturn &container_of(ctrl->handler,\n> +\t\t\t     struct stimx274, ctrls.handler)->sd;\n> +}\n> +\n> +static inline struct stimx274 *to_imx274(struct v4l2_subdev *sd)\n> +{\n> +\treturn container_of(sd, struct stimx274, sd);\n> +}\n> +\n> +/*\n> + * imx274_regmap_util_write_table_8 - Function for writing register table\n> + * @regmap: Pointer to device reg map structure\n> + * @table: Table containing register values\n> + * @wait_ms_addr: Flag for performing delay\n> + * @end_addr: Flag for incating end of table\n> + *\n> + * This is used to write register table into sensor's reg map.\n> + *\n> + * Return: 0 on success, errors otherwise\n> + */\n> +static int imx274_regmap_util_write_table_8(struct regmap *regmap,\n> +\t\t\t\t\t    const struct reg_8 table[],\n> +\t\t\t\t\t    u16 wait_ms_addr, u16 end_addr)\n> +{\n> +\tint err;\n> +\tconst struct reg_8 *next;\n> +\tu8 val;\n> +\n> +\tint range_start = -1;\n> +\tint range_count = 0;\n> +\tu8 range_vals[16];\n> +\tint max_range_vals = ARRAY_SIZE(range_vals);\n> +\n> +\tfor (next = table;; next++) {\n> +\t\tif ((next->addr != range_start + range_count) ||\n> +\t\t    (next->addr == end_addr) ||\n> +\t\t    (next->addr == wait_ms_addr) ||\n> +\t\t    (range_count == max_range_vals)) {\n> +\t\t\tif (range_count == 1)\n> +\t\t\t\terr = regmap_write(regmap,\n> +\t\t\t\t\t\t   range_start, range_vals[0]);\n> +\t\t\telse if (range_count > 1)\n> +\t\t\t\terr = regmap_bulk_write(regmap, range_start,\n> +\t\t\t\t\t\t\t&range_vals[0],\n> +\t\t\t\t\t\t\trange_count);\n> +\n> +\t\t\tif (err)\n> +\t\t\t\treturn err;\n> +\n> +\t\t\trange_start = -1;\n> +\t\t\trange_count = 0;\n> +\n> +\t\t\t/* Handle special address values */\n> +\t\t\tif (next->addr == end_addr)\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tif (next->addr == wait_ms_addr) {\n> +\t\t\t\tmsleep_range(next->val);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tval = next->val;\n> +\n> +\t\tif (range_start == -1)\n> +\t\t\trange_start = next->addr;\n> +\n> +\t\trange_vals[range_count++] = val;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> +static inline int imx274_read_reg(struct stimx274 *priv, u16 addr, u8 *val)\n> +{\n> +\tint err;\n> +\n> +\terr = regmap_read(priv->regmap, addr, (unsigned int *)val);\n> +\tif (err)\n> +\t\tv4l2_err(&priv->sd,\n> +\t\t\t \"%s : i2c read failed, addr = %x\\n\", __func__, addr);\n> +\telse\n> +\t\tv4l2_dbg(2, debug, &priv->sd,\n> +\t\t\t \"%s : addr 0x%x, val=0x%x\\n\", __func__,\n> +\t\t\t addr, *val);\n> +\treturn err;\n> +}\n> +\n> +static inline int imx274_write_reg(struct stimx274 *priv, u16 addr, u8 val)\n> +{\n> +\tint err;\n> +\n> +\terr = regmap_write(priv->regmap, addr, val);\n> +\tif (err)\n> +\t\tv4l2_err(&priv->sd,\n> +\t\t\t \"%s : i2c write failed, %x = %x\\n\", __func__,\n> +\t\t\t addr, val);\n> +\telse\n> +\t\tv4l2_dbg(2, debug, &priv->sd,\n> +\t\t\t \"%s : addr 0x%x, val=0x%x\\n\", __func__,\n> +\t\t\t addr, val);\n> +\treturn err;\n> +}\n> +\n> +static int imx274_write_table(struct stimx274 *priv, const imx274_reg table[])\n> +{\n> +\treturn imx274_regmap_util_write_table_8(priv->regmap,\n> +\t\ttable, IMX274_TABLE_WAIT_MS, IMX274_TABLE_END);\n> +}\n> +\n> +/*\n> + * imx274_start_stream - Function for starting stream per mode index\n> + * @priv: Pointer to device structure\n> + * @mode: Mode index value\n> + *\n> + * This is used to start steam per mode index.\n> + * mode = 0, start stream for sensor Mode 1: 4K/raw10\n> + * mode = 1, start stream for sensor Mode 3: 1080p/raw10\n> + * mode = 2, start stream for sensor Mode 5: 720p/raw10\n> + *\n> + * Return: 0 on success, errors otherwise\n> + */\n> +static int imx274_start_stream(struct stimx274 *priv, int mode)\n> +{\n> +\tint err = 0;\n> +\n> +\terr = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_1]);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\terr = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_2]);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\terr = imx274_write_table(priv, mode_table[mode]);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\t/*\n> +\t * Refer to \"Standby Cancel Sequence when using CSI-2\" in\n> +\t * imx274 datasheet, it should wait 10ms or more here.\n> +\t * give it 1 extra ms for margin\n> +\t */\n> +\tmsleep_range(11);\n> +\terr = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_3]);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\t/*\n> +\t * Refer to \"Standby Cancel Sequence when using CSI-2\" in\n> +\t * imx274 datasheet, it should wait 7ms or more here.\n> +\t * give it 1 extra ms for margin\n> +\t */\n> +\tmsleep_range(8);\n> +\terr = imx274_write_table(priv, mode_table[IMX274_MODE_START_STREAM_4]);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd, \"%s : finished\\n\", __func__);\n\nThese look like something that was quite possibly useful during development\ntime, but I'd remove them now.\n\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * imx274_reset - Function called to reset the sensor\n> + * @priv: Pointer to device structure\n> + * @rst: Input value for determining the sensor's end state after reset\n> + *\n> + * Set the senor in reset and then\n> + * if rst = 0, keep it in reset;\n> + * if rst = 1, bring it out of reset.\n> + *\n> + */\n> +static void imx274_reset(struct stimx274 *priv, int rst)\n> +{\n> +\tgpiod_set_value_cansleep(priv->reset_gpio, 0);\n> +\tusleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);\n> +\tgpiod_set_value_cansleep(priv->reset_gpio, !!rst);\n> +\tusleep_range(IMX274_RESET_DELAY1, IMX274_RESET_DELAY2);\n> +}\n> +\n> +/**\n> + * imx274_s_ctrl - This is used to set the imx274 V4L2 controls\n> + * @ctrl: V4L2 control to be set\n> + *\n> + * This function is used to set the V4L2 controls for the imx274 sensor.\n> + *\n> + * Return: 0 on success, errors otherwise\n> + */\n> +static int imx274_s_ctrl(struct v4l2_ctrl *ctrl)\n> +{\n> +\tstruct v4l2_subdev *sd = ctrl_to_sd(ctrl);\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\tint ret = -EINVAL;\n> +\n> +\tv4l2_dbg(1, debug, &imx274->sd,\n> +\t\t \"%s : s_ctrl: %s, value: %d\\n\", __func__,\n> +\t\tctrl->name, ctrl->val);\n> +\n> +\tmutex_lock(&imx274->lock);\n\nYou could assigne the same lock to the control handler and omit explicit locking\nhere. See e.g. the smiapp-core.c for an example.\n\n> +\n> +\tswitch (ctrl->id) {\n> +\tcase V4L2_CID_EXPOSURE:\n> +\t\tv4l2_dbg(1, debug, &imx274->sd,\n> +\t\t\t \"%s : set V4L2_CID_EXPOSURE\\n\", __func__);\n> +\t\tret = imx274_set_exposure(imx274, ctrl->val);\n> +\t\tbreak;\n> +\n> +\tcase V4L2_CID_GAIN:\n> +\t\tv4l2_dbg(1, debug, &imx274->sd,\n> +\t\t\t \"%s : set V4L2_CID_GAIN\\n\", __func__);\n> +\t\tret = imx274_set_gain(imx274, ctrl->val);\n> +\t\tbreak;\n> +\n> +\tcase V4L2_CID_VFLIP:\n> +\t\tv4l2_dbg(1, debug, &imx274->sd,\n> +\t\t\t \"%s : set V4L2_CID_VFLIP\\n\", __func__);\n> +\t\tret = imx274_set_vflip(imx274, ctrl->val);\n> +\t\tbreak;\n> +\n> +\tcase V4L2_CID_TEST_PATTERN:\n> +\t\tv4l2_dbg(1, debug, &imx274->sd,\n> +\t\t\t \"%s : set V4L2_CID_TEST_PATTERN\\n\", __func__);\n> +\t\tret = imx274_set_test_pattern(imx274, ctrl->val);\n> +\t\tbreak;\n> +\t}\n> +\n> +\tmutex_unlock(&imx274->lock);\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * imx274_get_fmt - Get the pad format\n> + * @sd: Pointer to V4L2 Sub device structure\n> + * @cfg: Pointer to sub device pad information structure\n> + * @fmt: Pointer to pad level media bus format\n> + *\n> + * This function is used to get the pad format information.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_get_fmt(struct v4l2_subdev *sd,\n> +\t\t\t  struct v4l2_subdev_pad_config *cfg,\n> +\t\t\t  struct v4l2_subdev_format *fmt)\n> +{\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\n> +\tif (fmt->pad)\n> +\t\treturn -EINVAL;\n> +\n> +\tfmt->format = imx274->format;\n\nYou should acquire the mutex here.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * imx274_set_fmt - This is used to set the pad format\n> + * @sd: Pointer to V4L2 Sub device structure\n> + * @cfg: Pointer to sub device pad information structure\n> + * @format: Pointer to pad level media bus format\n> + *\n> + * This function is used to set the pad format.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_fmt(struct v4l2_subdev *sd,\n> +\t\t\t  struct v4l2_subdev_pad_config *cfg,\n> +\t\t\t  struct v4l2_subdev_format *format)\n> +{\n> +\tstruct v4l2_mbus_framefmt *fmt = &format->format;\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\tstruct i2c_client *client = imx274->client;\n> +\tint index;\n> +\n> +\tv4l2_dbg(1, debug, client,\n> +\t\t \"%s: width = %d height = %d code = %d mbus_code = %d\\n\",\n> +\t\t __func__, fmt->width, fmt->height, fmt->code,\n> +\t\t imx274_formats[imx274->mode_index].mbus_code);\n> +\n> +\tif (format->pad)\n> +\t\treturn -EINVAL;\n\nIf you expose just a single pad, pad will be always zero.\n\n> +\n> +\tmutex_lock(&imx274->lock);\n> +\n> +\tfor (index = 0; index < ARRAY_SIZE(imx274_formats); index++) {\n> +\t\tif (imx274_formats[index].size.width == fmt->width &&\n> +\t\t    imx274_formats[index].size.height == fmt->height)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (index >= ARRAY_SIZE(imx274_formats)) {\n> +\t\t/* default to first format */\n> +\t\tindex = 0;\n> +\t}\n> +\n> +\timx274->mode_index = index;\n> +\n> +\tif (fmt->width > IMX274_MAX_WIDTH)\n> +\t\tfmt->width = IMX274_MAX_WIDTH;\n> +\tif (fmt->height > IMX274_MAX_HEIGHT)\n> +\t\tfmt->height = IMX274_MAX_HEIGHT;\n> +\tfmt->width = fmt->width & (~IMX274_MASK_LSB_2_BITS);\n> +\tfmt->height = fmt->height & (~IMX274_MASK_LSB_2_BITS);\n> +\tfmt->field = V4L2_FIELD_NONE;\n> +\n> +\tif (format->which == V4L2_SUBDEV_FORMAT_TRY)\n> +\t\tcfg->try_fmt = *fmt;\n> +\telse\n> +\t\timx274->format = *fmt;\n> +\n> +\tmutex_unlock(&imx274->lock);\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * imx274_g_frame_interval - Get the frame interval\n> + * @sd: Pointer to V4L2 Sub device structure\n> + * @fi: Pointer to V4l2 Sub device frame interval structure\n> + *\n> + * This function is used to get the frame interval.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_g_frame_interval(struct v4l2_subdev *sd,\n> +\t\t\t\t   struct v4l2_subdev_frame_interval *fi)\n> +{\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\n> +\tfi->interval = imx274->frame_interval;\n> +\tv4l2_dbg(1, debug, &imx274->sd, \"%s frame rate = %d / %d\\n\",\n> +\t\t __func__, imx274->frame_interval.numerator,\n> +\t\timx274->frame_interval.denominator);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * imx274_s_frame_interval - Set the frame interval\n> + * @sd: Pointer to V4L2 Sub device structure\n> + * @fi: Pointer to V4l2 Sub device frame interval structure\n> + *\n> + * This function is used to set the frame intervavl.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_s_frame_interval(struct v4l2_subdev *sd,\n> +\t\t\t\t   struct v4l2_subdev_frame_interval *fi)\n> +{\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\tstruct v4l2_ctrl *ctrl = imx274->ctrls.exposure;\n> +\tint min, max, def;\n> +\tint ret;\n> +\n> +\tmutex_lock(&imx274->lock);\n> +\tret = imx274_set_frame_interval(imx274, fi->interval);\n> +\tmutex_unlock(&imx274->lock);\n> +\n> +\tif (!ret) {\n> +\t\t/*\n> +\t\t * exposure time range is decided by frame interval\n> +\t\t * need to update it after frame interal changes\n> +\t\t */\n> +\t\tmin = IMX274_MIN_EXPOSURE_TIME;\n> +\t\tmax = fi->interval.numerator * 1000000\n> +\t\t\t/ fi->interval.denominator;\n> +\t\tdef = max;\n> +\t\tif (v4l2_ctrl_modify_range(ctrl, min, max, 1, def))\n\nI'd keep the mutex locked. You can use __v4l2_ctrl_modify_range() here.\n\n> +\t\t\tv4l2_err(sd, \"Exposure ctrl range update failed\\n\");\n> +\n> +\t\t/* update exposure time accordingly */\n> +\t\tmutex_lock(&imx274->lock);\n> +\t\timx274_set_exposure(imx274, imx274->ctrls.exposure->val);\n> +\t\tmutex_unlock(&imx274->lock);\n> +\n> +\t\tv4l2_dbg(1, debug, &imx274->sd, \"set frame interval to %uus\\n\",\n> +\t\t\t fi->interval.numerator * 1000000\n> +\t\t\t / fi->interval.denominator);\n> +\t}\n> +\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * imx274_load_default - load default control values\n> + * @priv: Pointer to device structure\n> + *\n> + * Return: 0 on success, errors otherwise\n> + */\n> +static int imx274_load_default(struct stimx274 *priv)\n> +{\n> +\tstruct v4l2_control control;\n> +\tint ret;\n> +\n> +\t/* load default control values */\n> +\tpriv->frame_interval.numerator = 1;\n> +\tpriv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;\n> +\tpriv->ctrls.exposure->val = 1000000 / IMX274_DEF_FRAME_RATE;\n> +\tpriv->ctrls.gain->val = IMX274_DEF_GAIN;\n> +\tpriv->ctrls.vflip->val = 0;\n> +\tpriv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;\n> +\n> +\t/* update frame rate */\n> +\tret = imx274_set_frame_interval(priv,\n> +\t\t\t\t\tpriv->frame_interval);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* update exposure time */\n> +\tcontrol.id = V4L2_CID_EXPOSURE;\n> +\tcontrol.value = priv->ctrls.exposure->val;\n> +\tret = v4l2_s_ctrl(NULL, &priv->ctrls.handler, &control);\n\nv4l2_ctrl_s_ctrl(), please.\n\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* update gain */\n> +\tcontrol.id = V4L2_CID_GAIN;\n> +\tcontrol.value = priv->ctrls.gain->val;\n> +\tret = v4l2_s_ctrl(NULL, &priv->ctrls.handler, &control);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* update vflip */\n> +\tcontrol.id = V4L2_CID_VFLIP;\n> +\tcontrol.value = priv->ctrls.vflip->val;\n> +\tret = v4l2_s_ctrl(NULL, &priv->ctrls.handler, &control);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * imx274_s_stream - It is used to start/stop the streaming.\n> + * @sd: V4L2 Sub device\n> + * @on: Flag (True / False)\n> + *\n> + * This function controls the start or stop of streaming for the\n> + * imx274 sensor.\n> + *\n> + * Return: 0 on success, errors otherwise\n> + */\n> +static int imx274_s_stream(struct v4l2_subdev *sd, int on)\n> +{\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\tstruct v4l2_control control;\n> +\tint ret = 0;\n> +\n> +\tv4l2_dbg(1, debug, &imx274->sd, \"%s : %s, mode index = %d\\n\", __func__,\n> +\t\t on ? \"Stream Start\" : \"Stream Stop\", imx274->mode_index);\n> +\n> +\tmutex_lock(&imx274->lock);\n> +\n> +\tif (on) {\n> +\t\t/* start stream */\n> +\t\tret = imx274_start_stream(imx274, imx274->mode_index);\n> +\t\tif (ret)\n> +\t\t\tgoto fail;\n> +\n> +\t\t/*\n> +\t\t * update frame rate & expsoure. if the last mode is different,\n> +\t\t * HMAX could be changed. As the result, frame rate & exposure\n> +\t\t * are changed.\n> +\t\t * gain is not affected.\n> +\t\t */\n\nShouldn't you do these before starting streaming?\n\nI don't know your hardware but usually this is what you want to do.\n\n> +\t\tret = imx274_set_frame_interval(imx274,\n> +\t\t\t\t\t\timx274->frame_interval);\n> +\t\tif (ret)\n> +\t\t\tgoto fail;\n> +\n> +\t\tmutex_unlock(&imx274->lock);\n> +\n> +\t\tcontrol.id = V4L2_CID_EXPOSURE;\n> +\t\tcontrol.value = imx274->ctrls.exposure->val;\n> +\t\tret = v4l2_s_ctrl(NULL, &imx274->ctrls.handler, &control);\n> +\t\tif (ret)\n> +\t\t\tgoto fail;\n> +\n> +\t\tmutex_lock(&imx274->lock);\n> +\n> +\t} else {\n> +\t\t/* stop stream */\n> +\t\tret = imx274_write_table(imx274,\n> +\t\t\t\t\t mode_table[IMX274_MODE_STOP_STREAM]);\n> +\t\tif (ret)\n> +\t\t\tgoto fail;\n> +\t}\n> +\n> +\tmutex_unlock(&imx274->lock);\n> +\tv4l2_dbg(1, debug, &imx274->sd,\n> +\t\t \"%s : Done: mode = %d\\n\", __func__, imx274->mode_index);\n> +\treturn 0;\n> +\n> +fail:\n> +\tmutex_unlock(&imx274->lock);\n> +\tv4l2_err(&imx274->sd, \"s_stream failed\\n\");\n> +\treturn ret;\n> +}\n> +\n> +static inline void imx274_calculate_frame_length_regs(imx274_reg *regs,\n\nYou use this once. Please either move the code where it's used or\nexplicitly denote there will be three entries. Same below.\n\n> +\t\t\t\t\t\t      u32 frame_length)\n> +{\n> +\tregs->addr = IMX274_FRAME_LENGTH_ADDR_1;\n> +\tregs->val = (frame_length >> IMX274_SHIFT_16_BITS)\n> +\t\t\t& IMX274_MASK_LSB_4_BITS;\n> +\t(regs + 1)->addr = IMX274_FRAME_LENGTH_ADDR_2;\n> +\t(regs + 1)->val = (frame_length >> IMX274_SHIFT_8_BITS)\n> +\t\t\t& IMX274_MASK_LSB_8_BITS;\n> +\t(regs + 2)->addr = IMX274_FRAME_LENGTH_ADDR_3;\n> +\t(regs + 2)->val = (frame_length) & IMX274_MASK_LSB_8_BITS;\n> +}\n> +\n> +static inline void imx274_calculate_coarse_time_regs(imx274_reg *regs,\n> +\t\t\t\t\t\t     u32 coarse_time)\n> +{\n> +\tregs->addr = IMX274_COARSE_TIME_ADDR_MSB;\n> +\tregs->val = (coarse_time >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_8_BITS;\n> +\t(regs + 1)->addr = IMX274_COARSE_TIME_ADDR_LSB;\n> +\t(regs + 1)->val = (coarse_time) & IMX274_MASK_LSB_8_BITS;\n> +}\n> +\n> +static inline void imx274_calculate_gain_regs(imx274_reg *regs, u16 gain)\n> +{\n> +\tregs->addr = IMX274_ANALOG_GAIN_ADDR_MSB;\n> +\tregs->val = (gain >> IMX274_SHIFT_8_BITS) & IMX274_MASK_LSB_3_BITS;\n> +\n> +\t(regs + 1)->addr = IMX274_ANALOG_GAIN_ADDR_LSB;\n> +\t(regs + 1)->val = (gain) & IMX274_MASK_LSB_8_BITS;\n> +}\n> +\n> +/*\n> + * imx274_get_frame_length - Function for obtaining current frame length\n> + * @priv: Pointer to device structure\n> + * @val: Pointer to obainted value\n> + *\n> + * frame_length = vmax x (svr + 1), in unit of hmax.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_get_frame_length(struct stimx274 *priv, s64 *val)\n> +{\n> +\tint err;\n> +\tu16 svr;\n> +\tu32 vmax;\n> +\tu8 reg_val[3];\n> +\n> +\t/* svr */\n> +\terr = imx274_read_reg(priv, IMX274_SVR_REG_LSB, &reg_val[0]);\n> +\terr |= imx274_read_reg(priv, IMX274_SVR_REG_MSB, &reg_val[1]);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\tsvr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];\n> +\n> +\t/* vmax */\n> +\terr = imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_3, &reg_val[0]);\n> +\terr |= imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_2, &reg_val[1]);\n> +\terr |= imx274_read_reg(priv, IMX274_FRAME_LENGTH_ADDR_1, &reg_val[2]);\n\nPlease don't use bit-wise or on integer error codes.\n\n> +\tif (err)\n> +\t\tgoto fail;\n> +\tvmax = ((reg_val[2] & IMX274_MASK_LSB_3_BITS) << IMX274_SHIFT_16_BITS)\n> +\t\t+ (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];\n> +\n> +\t*val = vmax * (svr + 1);\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +static int imx274_clamp_coarse_time(struct stimx274 *priv, s64 *val,\n> +\t\t\t\t    s64 *frame_length)\n> +{\n> +\tint err;\n> +\n> +\terr = imx274_get_frame_length(priv, frame_length);\n> +\tif (err)\n> +\t\treturn err;\n> +\n> +\tif (*frame_length < min_frame_len[priv->mode_index])\n> +\t\t*frame_length = min_frame_len[priv->mode_index];\n> +\n> +\t*val = *frame_length - *val; /* convert to raw shr */\n> +\tif (*val > *frame_length - IMX274_SHR_LIMIT_CONST)\n> +\t\t*val = *frame_length - IMX274_SHR_LIMIT_CONST;\n> +\telse if (*val < min_SHR[priv->mode_index])\n> +\t\t*val = min_SHR[priv->mode_index];\n> +\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * imx274_set_digital gain - Function called when setting digital gain\n> + * @priv: Pointer to device structure\n> + * @dgain: Value of digital gain.\n> + *\n> + * Digital gain has only 4 steps: 1x, 2x, 4x, and 8x\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_digital_gain(struct stimx274 *priv, u32 dgain)\n> +{\n> +\tint ret;\n> +\tu8 reg_val;\n> +\n> +\tswitch (dgain) {\n> +\tcase 1:\n> +\t\treg_val = 0;\n> +\t\tbreak;\n> +\tcase 2:\n> +\t\treg_val = 1;\n> +\t\tbreak;\n> +\tcase 4:\n> +\t\treg_val = 2;\n> +\t\tbreak;\n> +\tcase 8:\n> +\t\treg_val = 3;\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treturn -EINVAL;\n\nPlease set the closest value instead of returning an error. That's more\nuser friendly. You could use fls().\n\n> +\t}\n> +\n> +\tret = imx274_write_reg(priv, IMX274_DIGITAL_GAIN_REG,\n> +\t\t\t       reg_val & IMX274_MASK_LSB_4_BITS);\n> +\treturn ret;\n> +}\n> +\n> +/*\n> + * imx274_set_gain - Function called when setting gain\n> + * @priv: Pointer to device structure\n> + * @val: Value of gain. the real value = val << IMX274_GAIN_SHIFT;\n> + *\n> + * Set the gain based on input value.\n> + * The caller should hold the mutex lock imx274->lock if necessary\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_gain(struct stimx274 *priv, s64 val)\n> +{\n> +\timx274_reg reg_list[2];\n> +\tint err;\n> +\tu32 gain, analog_gain, digital_gain, gain_reg;\n> +\tint i;\n> +\n> +\tgain = (u32)(val);\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd,\n> +\t\t \"%s : input gain = %d.%d\\n\", __func__,\n> +\t\t gain >> IMX274_GAIN_SHIFT,\n> +\t\t ((gain & IMX274_GAIN_SHIFT_MASK) * 100) >> IMX274_GAIN_SHIFT);\n> +\n> +\tif (gain > IMX274_MAX_DIGITAL_GAIN * IMX274_MAX_ANALOG_GAIN)\n> +\t\tgain = IMX274_MAX_DIGITAL_GAIN * IMX274_MAX_ANALOG_GAIN;\n> +\telse if (gain < IMX274_MIN_GAIN)\n> +\t\tgain = IMX274_MIN_GAIN;\n> +\n> +\tif (gain <= IMX274_MAX_ANALOG_GAIN)\n> +\t\tdigital_gain = 1;\n> +\telse if (gain <= IMX274_MAX_ANALOG_GAIN * 2)\n> +\t\tdigital_gain = 2;\n> +\telse if (gain <= IMX274_MAX_ANALOG_GAIN * 4)\n> +\t\tdigital_gain = 4;\n> +\telse\n> +\t\tdigital_gain = IMX274_MAX_DIGITAL_GAIN;\n> +\n> +\tanalog_gain = gain / digital_gain;\n> +\n> +\tv4l2_dbg(2, debug, &priv->sd,\n> +\t\t \"%s : digital gain = %d, analog gain = %d.%d\\n\",\n> +\t\t __func__, digital_gain, analog_gain >> IMX274_GAIN_SHIFT,\n> +\t\t ((analog_gain & IMX274_GAIN_SHIFT_MASK) * 100)\n> +\t\t >> IMX274_GAIN_SHIFT);\n> +\n> +\terr = imx274_set_digital_gain(priv, digital_gain);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\n> +\t/* convert to register value, refer to imx274 datasheet */\n> +\tgain_reg = (u32)IMX274_GAIN_CONST -\n> +\t\t(IMX274_GAIN_CONST << IMX274_GAIN_SHIFT) / analog_gain;\n> +\tif (gain_reg > IMX274_GAIN_REG_MAX)\n> +\t\tgain_reg = IMX274_GAIN_REG_MAX;\n> +\n> +\timx274_calculate_gain_regs(reg_list, (u16)gain_reg);\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(reg_list); i++) {\n> +\t\terr = imx274_write_reg(priv, reg_list[i].addr,\n> +\t\t\t\t       reg_list[i].val);\n> +\t\tif (err)\n> +\t\t\tgoto fail;\n> +\t}\n> +\n> +\t/* convert register value back to gain value */\n> +\tpriv->ctrls.gain->val = (IMX274_GAIN_CONST << IMX274_GAIN_SHIFT)\n> +\t\t\t\t/ (IMX274_GAIN_CONST - gain_reg)\n> +\t\t\t\t* digital_gain;\n\nPlease pass the struct v4l2_ctrl pointer and use that instead. It'd be much\ncleaner.\n\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd,\n> +\t\t \"%s : GAIN control success, gain_reg = %d, new gain = %d\\n\",\n> +\t\t __func__, gain_reg, priv->ctrls.gain->val);\n> +\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * imx274_set_coarse_time - Function called when setting SHR value\n> + * @priv: Pointer to device structure\n> + * @val: Value for exposure time in number of line_length, or [HMAX]\n> + *\n> + * Set SHR value based on input value.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_coarse_time(struct stimx274 *priv, s64 *val)\n> +{\n> +\timx274_reg reg_list[2];\n> +\tint err;\n> +\ts64 coarse_time, frame_length;\n> +\tint i;\n> +\n> +\tcoarse_time = *val;\n> +\n> +\t/* convert exposure_time to appropriate SHR value */\n> +\terr = imx274_clamp_coarse_time(priv, &coarse_time, &frame_length);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\n> +\t/* prepare SHR registers */\n> +\timx274_calculate_coarse_time_regs(reg_list, coarse_time);\n> +\n> +\t/* write to SHR registers */\n> +\tfor (i = 0; i < ARRAY_SIZE(reg_list); i++) {\n> +\t\terr = imx274_write_reg(priv, reg_list[i].addr,\n> +\t\t\t\t       reg_list[i].val);\n> +\t\tif (err)\n> +\t\t\tgoto fail;\n> +\t}\n> +\n> +\t*val = frame_length - coarse_time;\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * imx274_set_exposure - Function called when setting exposure time\n> + * @priv: Pointer to device structure\n> + * @val: Variable for exposure time, in the unit of micro-second\n> + *\n> + * Set exposure time based on input value.\n> + * The caller should hold the mutex lock imx274->lock if necessary\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_exposure(struct stimx274 *priv, s64 val)\n> +{\n> +\tint err;\n> +\tu16 hmax;\n> +\tu8 reg_val[2];\n> +\ts64 coarse_time; /* exposure time in unit of line (HMAX)*/\n> +\n> +\t/* step 1: convert input exposure_time (val) into number of 1[HMAX] */\n> +\n> +\t/* obtain HMAX value */\n> +\terr = imx274_read_reg(priv, IMX274_HMAX_REG_LSB, &reg_val[0]);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\terr = imx274_read_reg(priv, IMX274_HMAX_REG_MSB, &reg_val[1]);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\thmax = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];\n> +\tif (hmax == 0) {\n> +\t\terr = -EINVAL;\n> +\t\tgoto fail;\n> +\t}\n> +\n> +\tcoarse_time = (IMX274_PIXCLK_CONST1 * val / IMX274_PIXCLK_CONST2\n> +\t\t\t- nocpiop[priv->mode_index]) / hmax;\n> +\n> +\tif (coarse_time < 0)\n> +\t\tcoarse_time = 0;\n> +\n> +\t/* step 2: convert exposure_time into SHR value */\n> +\n> +\t/* set SHR */\n> +\terr = imx274_set_coarse_time(priv, &coarse_time);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\n> +\tpriv->ctrls.exposure->val =\n> +\t\t\t(coarse_time * hmax + nocpiop[priv->mode_index])\n> +\t\t\t* IMX274_PIXCLK_CONST2 / IMX274_PIXCLK_CONST1;\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd,\n> +\t\t \"%s : EXPOSURE control success\\n\", __func__);\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * imx274_set_vflip - Function called when setting vertical flip\n> + * @priv: Pointer to device structure\n> + * @val: Value for vflip setting\n> + *\n> + * Set vertical flip based on input value.\n> + * val = 0: normal, no vertical flip\n> + * val = 1: vertical flip enabled\n> + * The caller should hold the mutex lock imx274->lock if necessary\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_vflip(struct stimx274 *priv, int val)\n> +{\n> +\tint err;\n> +\n> +\terr = imx274_write_reg(priv, IMX274_VFLIP_REG, val);\n> +\tif (err) {\n> +\t\tv4l2_err(&priv->sd, \"VFILP control error\\n\");\n> +\t\treturn err;\n> +\t}\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd,\n> +\t\t \"%s : VFLIP control success\\n\", __func__);\n> +\tpriv->ctrls.vflip->val = val;\n\nWhy?\n\n> +\treturn 0;\n> +}\n> +\n> +/*\n> + * imx274_set_test_pattern - Function called when setting test pattern\n> + * @priv: Pointer to device structure\n> + * @val: Variable for test pattern\n> + *\n> + * Set to different test patterns based on input value.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_test_pattern(struct stimx274 *priv, int val)\n> +{\n> +\tint err = 0;\n> +\n> +\tif (val == TEST_PATTERN_DISABLED) {\n> +\t\terr = imx274_write_table(priv, imx274_tp_disabled);\n> +\t} else if (val <= TEST_PATTERN_V_COLOR_BARS) {\n> +\t\timx274_tp_regs[0].val = val - 1;\n> +\t\terr = imx274_write_table(priv, imx274_tp_regs);\n> +\t} else {\n> +\t\tv4l2_err(&priv->sd, \"TEST PATTERN control our of range\\n\");\n\nWouldn't this be a driver bug?\n\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tif (err)\n> +\t\tgoto fail;\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd,\n> +\t\t \"%s : TEST PATTERN control success\\n\", __func__);\n> +\n> +\tpriv->ctrls.test_pattern->val = val;\n\nDitto.\n\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * imx274_set_frame_length - Function called when setting frame length\n> + * @priv: Pointer to device structure\n> + * @val: Variable for frame length (= VMAX, i.e. vertical drive period length)\n> + *\n> + * Set frame length based on input value.\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_frame_length(struct stimx274 *priv, u32 val)\n> +{\n> +\timx274_reg reg_list[3];\n> +\tint err;\n> +\tu32 frame_length;\n> +\tint i;\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd, \"%s : input length = %d\\n\",\n> +\t\t __func__, val);\n> +\n> +\tframe_length = (u32)val;\n> +\n> +\timx274_calculate_frame_length_regs(reg_list, frame_length);\n> +\tfor (i = 0; i < ARRAY_SIZE(reg_list); i++) {\n> +\t\terr = imx274_write_reg(priv, reg_list[i].addr,\n> +\t\t\t\t       reg_list[i].val);\n> +\t\tif (err)\n> +\t\t\tgoto fail;\n> +\t}\n> +\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * imx274_set_frame_interval - Function called when setting frame interval\n> + * @priv: Pointer to device structure\n> + * @frame_interval: Variable for frame interval\n> + *\n> + * Change frame interval by updating VMAX value\n> + * The caller should hold the mutex lock imx274->lock if necessary\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_set_frame_interval(struct stimx274 *priv,\n> +\t\t\t\t     struct v4l2_fract frame_interval)\n> +{\n> +\tint err;\n> +\ts64 frame_length, req_frame_rate;\n> +\tu16 svr;\n> +\tu16 hmax;\n> +\tu8 reg_val[2];\n> +\n> +\tv4l2_dbg(1, debug, &priv->sd, \"%s: input frame interval = %d / %d\",\n> +\t\t __func__, frame_interval.numerator,\n> +\t\t frame_interval.denominator);\n> +\n> +\tif (frame_interval.denominator == 0) {\n> +\t\terr = -EINVAL;\n> +\t\tgoto fail;\n> +\t}\n> +\n> +\treq_frame_rate = (u64)(frame_interval.denominator\n> +\t\t\t\t/ frame_interval.numerator);\n> +\n> +\t/* boundary check */\n> +\tif (req_frame_rate > max_frame_rate[priv->mode_index]) {\n> +\t\tframe_interval.numerator = 1;\n> +\t\tframe_interval.denominator =\n> +\t\t\t\t\tmax_frame_rate[priv->mode_index];\n> +\t} else if (req_frame_rate < IMX274_MIN_FRAME_RATE) {\n> +\t\tframe_interval.numerator = 1;\n> +\t\tframe_interval.denominator = IMX274_MIN_FRAME_RATE;\n> +\t}\n> +\n> +\t/*\n> +\t * VMAX = 1/frame_rate x 72M / (SVR+1) / HMAX\n> +\t * frame_length (i.e. VMAX) = (frame_interval) x 72M /(SVR+1) / HMAX\n> +\t */\n> +\n> +\t/* SVR */\n> +\terr = imx274_read_reg(priv, IMX274_SVR_REG_LSB, &reg_val[0]);\n> +\terr |= imx274_read_reg(priv, IMX274_SVR_REG_MSB, &reg_val[1]);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\tsvr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];\n> +\tv4l2_dbg(2, debug, &priv->sd,\n> +\t\t \"%s : register SVR = %d\\n\", __func__, svr);\n> +\n> +\t/* HMAX */\n> +\terr = imx274_read_reg(priv, IMX274_HMAX_REG_LSB, &reg_val[0]);\n> +\terr |= imx274_read_reg(priv, IMX274_HMAX_REG_MSB, &reg_val[1]);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\thmax = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];\n> +\tv4l2_dbg(2, debug, &priv->sd,\n> +\t\t \"%s : register HMAX = %d\\n\", __func__, hmax);\n> +\n> +\tframe_length = IMX274_PIXCLK_CONST1 / (svr + 1) / hmax\n> +\t\t\t\t\t* frame_interval.numerator\n> +\t\t\t\t\t/ frame_interval.denominator;\n> +\n> +\terr = imx274_set_frame_length(priv, frame_length);\n> +\tif (err)\n> +\t\tgoto fail;\n> +\n> +\tpriv->frame_interval = frame_interval;\n> +\treturn 0;\n> +\n> +fail:\n> +\tv4l2_err(&priv->sd, \"%s error = %d\\n\", __func__, err);\n> +\treturn err;\n> +}\n> +\n> +/*\n> + * imx274_open - Called on v4l2_open()\n> + * @sd: Pointer to V4L2 sub device structure\n> + * @fh: Pointer to V4L2 File handle\n> + *\n> + * This function is called on v4l2_open().\n> + *\n> + * Return: 0 on success\n> + */\n> +static int imx274_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)\n> +{\n> +\treturn 0;\n> +}\n> +\n> +static int imx274_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)\n> +{\n> +\treturn 0;\n> +}\n\nNo need for empty open/close callbacks.\n\n> +\n> +static const struct v4l2_subdev_pad_ops imx274_pad_ops = {\n> +\t.get_fmt = imx274_get_fmt,\n> +\t.set_fmt = imx274_set_fmt,\n> +};\n> +\n> +static const struct v4l2_subdev_video_ops imx274_video_ops = {\n> +\t.g_frame_interval = imx274_g_frame_interval,\n> +\t.s_frame_interval = imx274_s_frame_interval,\n> +\t.s_stream = imx274_s_stream,\n> +};\n> +\n> +static const struct v4l2_subdev_internal_ops imx274_subdev_internal_ops = {\n> +\t.open = imx274_open,\n> +\t.close = imx274_close\n> +};\n> +\n> +static const struct v4l2_subdev_core_ops imx274_core_ops = {\n> +};\n> +\n\nNo need for empty core ops.\n\n> +static const struct v4l2_subdev_ops imx274_subdev_ops = {\n> +\t.core = &imx274_core_ops,\n> +\t.pad = &imx274_pad_ops,\n> +\t.video = &imx274_video_ops,\n> +};\n> +\n> +static const struct v4l2_ctrl_ops imx274_ctrl_ops = {\n> +\t.s_ctrl\t= imx274_s_ctrl,\n> +};\n> +\n> +static const struct of_device_id imx274_of_id_table[] = {\n> +\t{ .compatible = \"sony,imx274\" },\n> +\t{ }\n> +};\n> +\n> +MODULE_DEVICE_TABLE(of, imx274_of_id_table);\n> +static const struct i2c_device_id imx274_id[] = {\n> +\t{ \"IMX274\", 0 },\n> +\t{ }\n> +};\n> +MODULE_DEVICE_TABLE(i2c, imx274_id);\n> +\n> +static int imx274_probe(struct i2c_client *client,\n> +\t\t\tconst struct i2c_device_id *id)\n> +{\n> +\tstruct v4l2_subdev *sd;\n> +\tstruct stimx274 *imx274;\n> +\tint ret;\n> +\n> +\t/* initialize imx274 */\n> +\timx274 = devm_kzalloc(&client->dev, sizeof(*imx274), GFP_KERNEL);\n> +\tif (!imx274)\n> +\t\treturn -ENOMEM;\n> +\n> +\tmutex_init(&imx274->lock);\n> +\n> +\t/* initialize regmap */\n> +\timx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);\n> +\tif (IS_ERR(imx274->regmap)) {\n> +\t\tdev_err(&client->dev,\n> +\t\t\t\"regmap init failed: %ld\\n\", PTR_ERR(imx274->regmap));\n\nmutex_destroy(). You might want to add more labeles for error handling.\n\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\t/* initialize subdevice */\n> +\timx274->client = client;\n> +\tsd = &imx274->sd;\n> +\tv4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);\n> +\tstrlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));\n> +\tsd->internal_ops = &imx274_subdev_internal_ops;\n> +\tsd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;\n> +\n> +\t/* initialize subdev media pad */\n> +\timx274->pad.flags = MEDIA_PAD_FL_SOURCE;\n> +\tsd->entity.function = MEDIA_ENT_F_CAM_SENSOR;\n> +\tret = media_entity_pads_init(&sd->entity, 1, &imx274->pad);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev,\n> +\t\t\t\"%s : media entity init Failed %d\\n\", __func__, ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* initialize sensor reset gpio */\n> +\timx274->reset_gpio = devm_gpiod_get_optional(&client->dev, \"reset\",\n> +\t\t\t\t\t\t     GPIOD_OUT_HIGH);\n> +\tif (IS_ERR(imx274->reset_gpio)) {\n> +\t\tif (PTR_ERR(imx274->reset_gpio) != -EPROBE_DEFER)\n> +\t\t\tdev_err(&client->dev, \"Reset GPIO not setup in DT\");\n> +\t\treturn PTR_ERR(imx274->reset_gpio);\n> +\t}\n> +\n> +\t/* pull sensor out of reset */\n> +\timx274_reset(imx274, 1);\n> +\n> +\t/* initialize controls */\n> +\tret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 2);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev,\n> +\t\t\t\"%s : ctrl handler init Failed\\n\", __func__);\n> +\t\tgoto err_me;\n> +\t}\n> +\n> +\t/* add new controls */\n> +\timx274->ctrls.test_pattern = v4l2_ctrl_new_std_menu_items(\n> +\t\t&imx274->ctrls.handler, &imx274_ctrl_ops,\n> +\t\tV4L2_CID_TEST_PATTERN,\n> +\t\tARRAY_SIZE(tp_qmenu) - 1, 0, 0, tp_qmenu);\n> +\n> +\timx274->ctrls.gain = v4l2_ctrl_new_std(&imx274->ctrls.handler,\n> +\t\t&imx274_ctrl_ops,\n> +\t\tV4L2_CID_GAIN, IMX274_MIN_GAIN,\n> +\t\tIMX274_MAX_DIGITAL_GAIN * IMX274_MAX_ANALOG_GAIN, 1,\n> +\t\tIMX274_DEF_GAIN);\n> +\n> +\timx274->ctrls.exposure = v4l2_ctrl_new_std(&imx274->ctrls.handler,\n> +\t\t&imx274_ctrl_ops,\n> +\t\tV4L2_CID_EXPOSURE, IMX274_MIN_EXPOSURE_TIME,\n> +\t\t1000000 / IMX274_DEF_FRAME_RATE, 1,\n> +\t\t1000000 / IMX274_DEF_FRAME_RATE);\n> +\n> +\timx274->ctrls.vflip = v4l2_ctrl_new_std(&imx274->ctrls.handler,\n> +\t\t&imx274_ctrl_ops,\n> +\t\tV4L2_CID_VFLIP, 0, 1, 1, 0);\n> +\n> +\timx274->sd.ctrl_handler = &imx274->ctrls.handler;\n> +\tif (imx274->ctrls.handler.error) {\n> +\t\tret = imx274->ctrls.handler.error;\n> +\t\tgoto err_ctrls;\n> +\t}\n> +\n> +\t/* setup default controls */\n> +\tret = v4l2_ctrl_handler_setup(&imx274->ctrls.handler);\n> +\tif (ret) {\n> +\t\tdev_err(&client->dev,\n> +\t\t\t\"Error %d setup default controls\\n\", ret);\n> +\t\tgoto err_ctrls;\n> +\t}\n> +\n> +\t/* initialize format */\n> +\timx274->mode_index = IMX274_MODE_3840X2160;\n> +\timx274->format.width = imx274_formats[0].size.width;\n> +\timx274->format.height = imx274_formats[0].size.height;\n> +\timx274->format.field = V4L2_FIELD_NONE;\n> +\timx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;\n> +\timx274->format.colorspace = V4L2_COLORSPACE_SRGB;\n> +\timx274->frame_interval.numerator = 1;\n> +\timx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;\n> +\n> +\t/* register subdevice */\n> +\tret = v4l2_async_register_subdev(sd);\n> +\tif (ret < 0) {\n> +\t\tdev_err(&client->dev,\n> +\t\t\t\"%s : v4l2_async_register_subdev failed %d\\n\",\n> +\t\t\t__func__, ret);\n> +\t\tgoto err_ctrls;\n> +\t}\n> +\n> +\t/* load default control values */\n> +\tret = imx274_load_default(imx274);\n> +\tif (ret)\n\nv4l2_async_unregister_subdev();\n\n> +\t\tgoto err_ctrls;\n> +\n> +\tv4l2_info(sd, \"imx274 : imx274 probe success !\\n\");\n> +\treturn 0;\n> +\n> +err_ctrls:\n> +\tv4l2_ctrl_handler_free(sd->ctrl_handler);\n> +err_me:\n> +\tmedia_entity_cleanup(&sd->entity);\n> +\tmutex_destroy(&imx274->lock);\n> +\treturn ret;\n> +}\n> +\n> +static int imx274_remove(struct i2c_client *client)\n> +{\n> +\tint ret;\n> +\tstruct v4l2_subdev *sd = i2c_get_clientdata(client);\n> +\tstruct stimx274 *imx274 = to_imx274(sd);\n> +\n> +\t/* stop stream */\n> +\tret = imx274_write_table(imx274,\n> +\t\t\t\t mode_table[IMX274_MODE_STOP_STREAM]);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tv4l2_device_unregister_subdev(sd);\n\nInstead: v4l2_async_unregister_subdev().\n\n> +\tv4l2_ctrl_handler_free(sd->ctrl_handler);\n> +\tmedia_entity_cleanup(&sd->entity);\n> +\tmutex_destroy(&imx274->lock);\n> +\treturn 0;\n> +}\n> +\n> +static struct i2c_driver imx274_i2c_driver = {\n> +\t.driver = {\n> +\t\t.name\t= DRIVER_NAME,\n> +\t\t.of_match_table\t= imx274_of_id_table,\n> +\t},\n> +\t.probe\t\t= imx274_probe,\n> +\t.remove\t\t= imx274_remove,\n> +\t.id_table\t= imx274_id,\n> +};\n> +\n> +module_i2c_driver(imx274_i2c_driver);\n> +\n> +MODULE_AUTHOR(\"Leon Luo <leonl@leopardimaging.com>\");\n> +MODULE_DESCRIPTION(\"IMX274 CMOS Image Sensor driver\");\n> +MODULE_LICENSE(\"GPL v2\");","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xhNlf3sjcz9t33\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tTue, 29 Aug 2017 19:35:10 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751231AbdH2JfI (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 29 Aug 2017 05:35:08 -0400","from nblzone-211-213.nblnetworks.fi ([83.145.211.213]:49908 \"EHLO\n\thillosipuli.retiisi.org.uk\" rhost-flags-OK-OK-OK-FAIL)\n\tby vger.kernel.org with ESMTP id S1751227AbdH2JfF (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 29 Aug 2017 05:35:05 -0400","from valkosipuli.localdomain (valkosipuli.retiisi.org.uk\n\t[IPv6:2001:1bc8:1a6:d3d5::80:2])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby hillosipuli.retiisi.org.uk (Postfix) with ESMTPS id 27124600E9;\n\tTue, 29 Aug 2017 12:35:02 +0300 (EEST)","from sakke by valkosipuli.localdomain with local (Exim 4.89)\n\t(envelope-from <sakke@valkosipuli.retiisi.org.uk>)\n\tid 1dmcvF-0001HJ-L1; Tue, 29 Aug 2017 12:35:01 +0300"],"Date":"Tue, 29 Aug 2017 12:35:01 +0300","From":"Sakari Ailus <sakari.ailus@iki.fi>","To":"Soren Brinkmann <soren.brinkmann@xilinx.com>","Cc":"mchehab@kernel.org, robh+dt@kernel.org, mark.rutland@arm.com,\n\thans.verkuil@cisco.com, sakari.ailus@linux.intel.com,\n\tlinux-media@vger.kernel.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, Leon Luo <leonl@leopardimaging.com>","Subject":"Re: [PATCH v2 2/2] media:imx274 V4l2 driver for Sony imx274 CMOS\n\tsensor","Message-ID":"<20170829093501.ijsnmzyl6d7xbpxc@valkosipuli.retiisi.org.uk>","References":"<20170829014026.26551-1-soren.brinkmann@xilinx.com>\n\t<20170829014026.26551-2-soren.brinkmann@xilinx.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20170829014026.26551-2-soren.brinkmann@xilinx.com>","User-Agent":"NeoMutt/20170113 (1.7.2)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}}]