[{"id":11748,"web_url":"https://patchwork.libcamera.org/comment/11748/","msgid":"<20200731114439.GN13316@paasikivi.fi.intel.com>","date":"2020-07-31T11:44:39","subject":"Re: [libcamera-devel] [PATCH 15/25] media: ov5647: Break out format\n\thandling","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Jacopo,\n\nBtw. I've bounced the two DT binding patches to DT list + Rob. Please\nremember that in v2 if there are changes.\n\nOn Tue, Jun 23, 2020 at 06:42:24PM +0200, Jacopo Mondi wrote:\n> Break format handling out from the main driver structure.\n> \n> This commit prepares for the introduction of more sensor formats and\n> resolutions by instrumenting the existing operation to work on multiple\n> modes instead of assuming a single one supported.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> drivers/media/i2c/ov5647.c | 84 +++++++++++++++++++++++++++-----------\n> 1 file changed, 61 insertions(+), 23 deletions(-)\n> \n> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c\n> index 03f4f1a257ecd..a801ed0249aad 100644\n> --- a/drivers/media/i2c/ov5647.c\n> +++ b/drivers/media/i2c/ov5647.c\n> @@ -84,18 +84,28 @@ struct regval_list {\n> \tu8 data;\n> };\n> \n> +struct ov5647_mode {\n> +\tstruct v4l2_mbus_framefmt\tformat;\n> +\tstruct regval_list\t\t*reg_list;\n> +\tunsigned int\t\t\tnum_regs;\n> +};\n> +\n> +struct ov5647_format_list {\n> +\tunsigned int\t\t\tmbus_code;\n> +\tstruct ov5647_mode\t\t*modes;\n> +\tunsigned int\t\t\tnum_modes;\n> +};\n> +\n> struct ov5647 {\n> \tstruct v4l2_subdev\t\tsd;\n> \tstruct media_pad\t\tpad;\n> \tstruct mutex\t\t\tlock;\n> -\tstruct v4l2_mbus_framefmt\tformat;\n> -\tunsigned int\t\t\twidth;\n> -\tunsigned int\t\t\theight;\n> \tint\t\t\t\tpower_count;\n> \tstruct clk\t\t\t*xclk;\n> \tstruct gpio_desc\t\t*pwdn;\n> \tbool\t\t\t\tclock_ncont;\n> \tstruct v4l2_ctrl_handler\tctrls;\n> +\tstruct ov5647_mode\t\t*mode;\n> };\n> \n> static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)\n> @@ -205,6 +215,33 @@ static struct regval_list ov5647_640x480[] = {\n> \t{0x0100, 0x01},\n> };\n> \n> +static struct ov5647_mode ov5647_8bit_modes[] = {\n\nconst\n\n> +\t{\n> +\t\t.format\t= {\n> +\t\t\t.code\t\t= MEDIA_BUS_FMT_SBGGR8_1X8,\n> +\t\t\t.colorspace\t= V4L2_COLORSPACE_SRGB,\n> +\t\t\t.field\t\t= V4L2_FIELD_NONE,\n> +\t\t\t.width\t\t= 640,\n> +\t\t\t.height\t\t= 480\n> +\t\t},\n> +\t\t.reg_list\t= ov5647_640x480,\n> +\t\t.num_regs\t= ARRAY_SIZE(ov5647_640x480)\n> +\t},\n> +};\n> +\n> +static const struct ov5647_format_list ov5647_formats[] = {\n> +\t{\n> +\t\t.mbus_code\t= MEDIA_BUS_FMT_SBGGR8_1X8,\n> +\t\t.modes\t\t= ov5647_8bit_modes,\n> +\t\t.num_modes\t= ARRAY_SIZE(ov5647_8bit_modes),\n> +\t},\n> +};\n> +\n> +#define OV5647_NUM_FORMATS\t(ARRAY_SIZE(ov5647_formats))\n\nPlease use ARRAY_SIZE() directly, you don't need a macro here.\n\n> +\n> +#define OV5647_DEFAULT_MODE\t(&ov5647_formats[0].modes[0])\n> +#define OV5647_DEFAULT_FORMAT\t(ov5647_formats[0].modes[0].format)\n> +\n> static int ov5647_write(struct v4l2_subdev *sd, u16 reg, u8 val)\n> {\n> \tunsigned char data[3] = { reg >> 8, reg & 0xff, val};\n> @@ -282,8 +319,7 @@ static int ov5647_set_mode(struct v4l2_subdev *sd)\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> -\tret = ov5647_write_array(sd, ov5647_640x480,\n> -\t\t\t\t ARRAY_SIZE(ov5647_640x480));\n> +\tret = ov5647_write_array(sd, sensor->mode->reg_list, sensor->mode->num_regs);\n\nNo need for a line over 80.\n\n> \tif (ret < 0) {\n> \t\tdev_err(&client->dev, \"write sensor default regs error\\n\");\n> \t\treturn ret;\n> @@ -494,10 +530,10 @@ static int ov5647_enum_mbus_code(struct v4l2_subdev *sd,\n> \t\t\t\t struct v4l2_subdev_pad_config *cfg,\n> \t\t\t\t struct v4l2_subdev_mbus_code_enum *code)\n> {\n> -\tif (code->index > 0)\n> +\tif (code->index >= OV5647_NUM_FORMATS)\n> \t\treturn -EINVAL;\n> \n> -\tcode->code = MEDIA_BUS_FMT_SBGGR8_1X8;\n> +\tcode->code = ov5647_formats[code->index].mbus_code;\n> \n> \treturn 0;\n> }\n> @@ -506,16 +542,24 @@ static int ov5647_enum_frame_size(struct v4l2_subdev *sd,\n> \t\t\t\t struct v4l2_subdev_pad_config *cfg,\n> \t\t\t\t struct v4l2_subdev_frame_size_enum *fse)\n> {\n> -\tif (fse->index)\n> +\tconst struct v4l2_mbus_framefmt *fmt;\n> +\tunsigned int i = 0;\n> +\n> +\tfor (; i < OV5647_NUM_FORMATS; ++i) {\n> +\t\tif (ov5647_formats[i].mbus_code == fse->code)\n> +\t\t\tbreak;\n> +\t}\n> +\tif (i == OV5647_NUM_FORMATS)\n> \t\treturn -EINVAL;\n> \n> -\tif (fse->code != MEDIA_BUS_FMT_SBGGR8_1X8)\n> +\tif (fse->index >= ov5647_formats[i].num_modes)\n> \t\treturn -EINVAL;\n> \n> -\tfse->min_width = 640;\n> -\tfse->max_width = 640;\n> -\tfse->min_height = 480;\n> -\tfse->max_height = 480;\n> +\tfmt = &ov5647_formats[i].modes[fse->index].format;\n> +\tfse->min_width = fmt->width;\n> +\tfse->max_width = fmt->width;\n> +\tfse->min_height = fmt->height;\n> +\tfse->max_height = fmt->height;\n> \n> \treturn 0;\n> }\n> @@ -528,11 +572,7 @@ static int ov5647_set_get_fmt(struct v4l2_subdev *sd,\n> \n> \t/* Only one format is supported, so return that. */\n> \tmemset(fmt, 0, sizeof(*fmt));\n> -\tfmt->code = MEDIA_BUS_FMT_SBGGR8_1X8;\n> -\tfmt->colorspace = V4L2_COLORSPACE_SRGB;\n> -\tfmt->field = V4L2_FIELD_NONE;\n> -\tfmt->width = 640;\n> -\tfmt->height = 480;\n> +\t*fmt = OV5647_DEFAULT_FORMAT;\n> \n> \treturn 0;\n> }\n> @@ -591,11 +631,7 @@ static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)\n> \tcrop->width = OV5647_WINDOW_WIDTH_DEF;\n> \tcrop->height = OV5647_WINDOW_HEIGHT_DEF;\n> \n> -\tformat->code = MEDIA_BUS_FMT_SBGGR8_1X8;\n> -\tformat->width = 640;\n> -\tformat->height = 480;\n> -\tformat->field = V4L2_FIELD_NONE;\n> -\tformat->colorspace = V4L2_COLORSPACE_SRGB;\n> +\t*format = OV5647_DEFAULT_FORMAT;\n> \n> \treturn 0;\n> }\n> @@ -813,6 +849,8 @@ static int ov5647_probe(struct i2c_client *client)\n> \n> \tmutex_init(&sensor->lock);\n> \n> +\tsensor->mode = OV5647_DEFAULT_MODE;\n\nYou could do this without the macro, too.\n\n> +\n> \tret = ov5647_init_controls(sensor);\n> \tif (ret)\n> \t\tgoto mutex_destroy;","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D9853BD86F\n\tfor ;\n\tFri, 31 Jul 2020 12:01:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B595E61988;\n\tFri, 31 Jul 2020 14:01:29 +0200 (CEST)","from mga17.intel.com (mga17.intel.com [192.55.52.151])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AEEE60396\n\tfor ;\n\tFri, 31 Jul 2020 13:44:47 +0200 (CEST)","from orsmga003.jf.intel.com ([10.7.209.27])\n\tby fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t31 Jul 2020 04:44:46 -0700","from paasikivi.fi.intel.com ([10.237.72.42])\n\tby orsmga003-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Jul 2020 04:44:42 -0700","by paasikivi.fi.intel.com (Postfix, from userid 1000)\n\tid CD0E920722; Fri, 31 Jul 2020 14:44:39 +0300 (EEST)"],"IronPort-SDR":["YELH7K3EN5kzwYE6Nc5ywNwCJpMQ/NPoiPUxPU2UWMu4UGu03E/7X8tac4ANpr+IwxIeO6lUND\n\toK++F82CaiBw==","nW0vU/U/0td0BoGSqQsk5m8/w0z2OBQrZ7gc0mJ14YrW83ND3Mzw541HUBA6OihIHFh+HwpDQS\n\tRutUV8DVSRBg=="],"X-IronPort-AV":["E=McAfee;i=\"6000,8403,9698\"; a=\"131833044\"","E=Sophos;i=\"5.75,418,1589266800\"; d=\"scan'208\";a=\"131833044\"","E=Sophos;i=\"5.75,418,1589266800\"; d=\"scan'208\";a=\"287160277\""],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","Date":"Fri, 31 Jul 2020 14:44:39 +0300","From":"Sakari Ailus ","To":"Jacopo Mondi ","Message-ID":"<20200731114439.GN13316@paasikivi.fi.intel.com>","References":"<20200623100815.10674-1-jacopo@jmondi.org>\n\t<20200623164224.44476-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200623164224.44476-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","X-Mailman-Approved-At":"Fri, 31 Jul 2020 14:01:28 +0200","Subject":"Re: [libcamera-devel] [PATCH 15/25] media: ov5647: Break out format\n\thandling","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"andrew_gabbasov@mentor.com, linux-media@vger.kernel.org,\n\tmrodin@de.adit-jv.com, erosca@de.adit-jv.com, mripard@kernel.org,\n\troman.kovalivskyi@globallogic.com, libcamera-devel@lists.libcamera.org,\n\tdave.stevenson@raspberrypi.org, hugues.fruchet@st.com,\n\tmchehab@kernel.org, aford173@gmail.com, sudipi@jp.adit-jv.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]