[PATCH v5] media: imx208: Add imx208 camera sensor driver

[PATCH v5] media: imx208: Add imx208 camera sensor driver

[Ping-chung Chen]

From: "Chen, Ping-chung" <ping-chung.chen@intel.com>

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 <ping-chung.chen@intel.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>

---
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 <sakari.ailus@linux.intel.com>
+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 <sakari.ailus@linux.intel.com>
 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 <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <asm/unaligned.h>
+
+#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
+
+/* 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)
+
+#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;
+
+	/* 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];
+		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);
+
+	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);
+	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 <andy.yeh@intel.com>");
+MODULE_AUTHOR("Chen, Ping-chung <ping-chung.chen@intel.com>");
+MODULE_DESCRIPTION("Sony IMX208 sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

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" <ping-chung.chen@intel.com>
> 
> 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 <ping-chung.chen@intel.com>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> 
> ---
> 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 <sakari.ailus@linux.intel.com>
> +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 <sakari.ailus@linux.intel.com>
>  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 <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <asm/unaligned.h>
> +
> +#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:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=unlocked-ctrl-grab>

> +
> +	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 <andy.yeh@intel.com>");
> +MODULE_AUTHOR("Chen, Ping-chung <ping-chung.chen@intel.com>");
> +MODULE_DESCRIPTION("Sony IMX208 sensor driver");
> +MODULE_LICENSE("GPL v2");

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Grant Grundler]

On Fri, Sep 14, 2018 at 4:41 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Ping-chung,
>
> My apologies for the late review.

Yeah...I had the impression this was already accepted. Though it
should be straight forward to fix up additional things as normal
patches.

[sorry pruning heavily]
...
> > +/* HBLANK control - read only */
> > +#define IMX208_PPL_384MHZ            2248
> > +#define IMX208_PPL_96MHZ             2248
>
> Does this generally depend on the link frequency?

This was discussed in earlier patch version: in a nutshell, yes.

...
> > +/* 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.

The original code did that. I agree IMX208_MHZ makes this much easier to read.

...
> > +     /* 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.

I specifically asked the code not use "mutex" because trying to find
this specific use of "mutex" with cscope (ctags) is impossible.

Defining "mutex" in multiple name spaces is asking for trouble even
though technically it's "safe" to do.

...
> > +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.

<Someone needs to comment here.>  :)

...
> > +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:
>
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=unlocked-ctrl-grab>

Is the current implementation not correct or is this just the
preferred way to "grab"?

(And thanks for pointing at the patch which adds the new "API")

(and I'm ignoring the remaining nit on the assumption it can be
addressed in the next patch)

cheers,
grant

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

Hi Grant,

On Mon, Sep 17, 2018 at 03:52:30PM -0700, Grant Grundler wrote:
> On Fri, Sep 14, 2018 at 4:41 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Ping-chung,
> >
> > My apologies for the late review.
> 
> Yeah...I had the impression this was already accepted. Though it
> should be straight forward to fix up additional things as normal
> patches.

The remaining issues are rather small and there's still time to get the
driver to v4.20, so I see no need to postpone these either.

> 
> [sorry pruning heavily]
> ...
> > > +/* HBLANK control - read only */
> > > +#define IMX208_PPL_384MHZ            2248
> > > +#define IMX208_PPL_96MHZ             2248
> >
> > Does this generally depend on the link frequency?
> 
> This was discussed in earlier patch version: in a nutshell, yes.
> 
> ...
> > > +/* 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.
> 
> The original code did that. I agree IMX208_MHZ makes this much easier to read.

It is not customary to add driver specific defines for that sort of things;
mostly if you need a plain number you do write a plain number. A sort of an
exception are the SZ_* macros.

The above breaks grep, too.

> 
> ...
> > > +     /* 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.
> 
> I specifically asked the code not use "mutex" because trying to find
> this specific use of "mutex" with cscope (ctags) is impossible.

The mutex is local to the driver, and in this case also to the file.
Mutexes are commonly called either "mutex" or "lock".

> 
> Defining "mutex" in multiple name spaces is asking for trouble even
> though technically it's "safe" to do.
> 
> ...
> > > +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.
> 
> <Someone needs to comment here.>  :)
> 
> ...
> > > +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:
> >
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=unlocked-ctrl-grab>
> 
> Is the current implementation not correct or is this just the
> preferred way to "grab"?

The problem with the above is that the controls have not been grabbed
before the lock is released, therefore allowing them to be changed just
after starting streaming.

> (And thanks for pointing at the patch which adds the new "API")
> 
> (and I'm ignoring the remaining nit on the assumption it can be
> addressed in the next patch)

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Tomasz Figa]

[+Laurent and Sylwester]

On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
<ping-chung.chen@intel.com> wrote:
[snip]
> +
> +/* 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
> +
[snip]
> +/* Initialize control handlers */
> +static int imx208_init_controls(struct imx208 *imx208)
> +{
[snip]
> +       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);

We have a problem here. The sensor supports only a discrete range of
values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
fixed point). This makes it possible for the userspace to set values
that are not allowed by the sensor specification and also leaves no
way to enumerate the supported values.

I can see two solutions here:

1) Define the control range from 0 to 4 and treat it as an exponent of
2, so that the value for the sensor becomes (1 << val) * 256.
(Suggested by Sakari offline.)

This approach has the problem of losing the original unit (and scale)
of the value.

2) Use an integer menu control, which reports only the supported
discrete values - {1, 2, 4, 8, 16}.

With this approach, userspace can enumerate the real gain values, but
we would either need to introduce a new control (e.g.
V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
register V4L2_CID_DIGITAL_GAIN as an integer menu.

Any opinions or better ideas?

Best regards,
Tomasz

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Grant Grundler]

[resend in plain text - sorry!]

On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa <tfiga@chromium.org> wrote:
>
> [+Laurent and Sylwester]
>
> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
> <ping-chung.chen@intel.com> wrote:
> [snip]
> > +
> > +/* 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
> > +
> [snip]
> > +/* Initialize control handlers */
> > +static int imx208_init_controls(struct imx208 *imx208)
> > +{
> [snip]
> > +       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);
>
> We have a problem here. The sensor supports only a discrete range of
> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> fixed point). This makes it possible for the userspace to set values
> that are not allowed by the sensor specification and also leaves no
> way to enumerate the supported values.
>
> I can see two solutions here:
>
> 1) Define the control range from 0 to 4 and treat it as an exponent of
> 2, so that the value for the sensor becomes (1 << val) * 256.
> (Suggested by Sakari offline.)
>
> This approach has the problem of losing the original unit (and scale)
> of the value.

Exactly - will users be confused by this? If we have to explain it,
probably not the best choice.

>
> 2) Use an integer menu control, which reports only the supported
> discrete values - {1, 2, 4, 8, 16}.
>
> With this approach, userspace can enumerate the real gain values, but
> we would either need to introduce a new control (e.g.
> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> register V4L2_CID_DIGITAL_GAIN as an integer menu.
>
> Any opinions or better ideas?

My $0.02: leave the user UI alone - let users specify/select anything
in the range the normal API or UI allows. But have sensor specific
code map all values in that range to values the sensor supports. Users
will notice how it works when they play with it.  One can "adjust" the
mapping so it "feels right".

cheers,
grant
>
> Best regards,
> Tomasz

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sylwester Nawrocki]

On 09/20/2018 06:49 PM, Grant Grundler wrote:
> On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa <tfiga@chromium.org> wrote:
>> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
>> <ping-chung.chen@intel.com> wrote:

>>> +/* Digital gain control */

>>> +#define IMX208_DGTL_GAIN_MIN           0
>>> +#define IMX208_DGTL_GAIN_MAX           4096
>>> +#define IMX208_DGTL_GAIN_DEFAULT       0x100
>>> +#define IMX208_DGTL_GAIN_STEP           1

>>> +/* Initialize control handlers */
>>> +static int imx208_init_controls(struct imx208 *imx208)
>>> +{
>> [snip]
>>> +       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);
>>
>> We have a problem here. The sensor supports only a discrete range of
>> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
>> fixed point). This makes it possible for the userspace to set values
>> that are not allowed by the sensor specification and also leaves no
>> way to enumerate the supported values.

The driver could always adjust the value in set_ctrl callback so invalid
settings are not allowed.

I'm not sure if it's best approach but I once did something similar for
the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
for fractional part. The driver reports values multiplied by 16. See 
ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
Total Gain to Control Bit Correlation" in the OV9650 datasheet for details. 
The integer menu control just seemed not suitable for 2^10 values. 
Now the gain control has range 16...1984 out of which only 1024 values 
are valid. It might not be best approach for a GUI but at least the driver 
exposes mapping of all valid values, which could be enumerated with 
VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific 
user space code.  

>> I can see two solutions here:
>>
>> 1) Define the control range from 0 to 4 and treat it as an exponent of
>> 2, so that the value for the sensor becomes (1 << val) * 256.
>> (Suggested by Sakari offline.)
>>
>> This approach has the problem of losing the original unit (and scale)
>> of the value.
> 
> Exactly - will users be confused by this? If we have to explain it,
> probably not the best choice.
> 
>>
>> 2) Use an integer menu control, which reports only the supported
>> discrete values - {1, 2, 4, 8, 16}.
>>
>> With this approach, userspace can enumerate the real gain values, but
>> we would either need to introduce a new control (e.g.
>> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
>> register V4L2_CID_DIGITAL_GAIN as an integer menu.
>>
>> Any opinions or better ideas?
> 
> My $0.02: leave the user UI alone - let users specify/select anything
> in the range the normal API or UI allows. But have sensor specific
> code map all values in that range to values the sensor supports. Users
> will notice how it works when they play with it.  One can "adjust" the
> mapping so it "feels right".

--
Regards,
Sylwester

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

Hi Tomasz,

On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> [+Laurent and Sylwester]
> 
> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen
> <ping-chung.chen@intel.com> wrote:
> [snip]
> > +
> > +/* 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
> > +
> [snip]
> > +/* Initialize control handlers */
> > +static int imx208_init_controls(struct imx208 *imx208)
> > +{
> [snip]
> > +       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);
> 
> We have a problem here. The sensor supports only a discrete range of
> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> fixed point). This makes it possible for the userspace to set values
> that are not allowed by the sensor specification and also leaves no
> way to enumerate the supported values.
> 
> I can see two solutions here:
> 
> 1) Define the control range from 0 to 4 and treat it as an exponent of
> 2, so that the value for the sensor becomes (1 << val) * 256.
> (Suggested by Sakari offline.)
> 
> This approach has the problem of losing the original unit (and scale)
> of the value.

I'd like to add that this is not a property of the proposed solution.

Rather, the above needs to be accompanied by additional information
provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
other information such as whether the control is linear or exponential (as
in this case).

> 
> 2) Use an integer menu control, which reports only the supported
> discrete values - {1, 2, 4, 8, 16}.
> 
> With this approach, userspace can enumerate the real gain values, but
> we would either need to introduce a new control (e.g.
> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> register V4L2_CID_DIGITAL_GAIN as an integer menu.

New controls in V4L2 are, for the most part, created when there's something
new to control. The documentation of some controls (similar to e.g. gain)
documents a unit as well as a prefix but that's the case only because
there's been no way to tell the unit or prefix otherwise in the API.

An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
sure how they came to be though. An accident is a possibility as far as I
see.

Controls that have a documented unit use that unit --- as long as that's
the unit used by the hardware. If it's not, it tends to be that another
unit is used but the user space has currently no way of knowing this. And
the digital gain control is no exception to this.

So if we want to improve the user space's ability to be informed how the
control values reflect to device configuration, we do need to provide more
information to the user space. One way to do that would be through
VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
just for purposes such as this one.

> 
> Any opinions or better ideas?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Laurent Pinchart]

Hello,

(CC'ing Helmut Grohne)

On Thursday, 20 September 2018 23:16:47 EEST Sylwester Nawrocki wrote:
> On 09/20/2018 06:49 PM, Grant Grundler wrote:
> > On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
> >> On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> >>> +/* Digital gain control */
> >>> 
> >>> +#define IMX208_DGTL_GAIN_MIN           0
> >>> +#define IMX208_DGTL_GAIN_MAX           4096
> >>> +#define IMX208_DGTL_GAIN_DEFAULT       0x100
> >>> +#define IMX208_DGTL_GAIN_STEP           1
> >>> 
> >>> +/* Initialize control handlers */
> >>> +static int imx208_init_controls(struct imx208 *imx208)
> >>> +{
> >> 
> >> [snip]
> >> 
> >>> +       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);
> >> 
> >> We have a problem here. The sensor supports only a discrete range of
> >> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> >> fixed point). This makes it possible for the userspace to set values
> >> that are not allowed by the sensor specification and also leaves no
> >> way to enumerate the supported values.
> 
> The driver could always adjust the value in set_ctrl callback so invalid
> settings are not allowed.
> 
> I'm not sure if it's best approach but I once did something similar for
> the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
> for fractional part. The driver reports values multiplied by 16. See
> ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
> Total Gain to Control Bit Correlation" in the OV9650 datasheet for details.
> The integer menu control just seemed not suitable for 2^10 values.

I've had a similar discussion on IRC recently with Helmut, who posted a nice 
summary of the problem on the mailing list (see https://www.mail-archive.com/
linux-media@vger.kernel.org/msg134502.html). This is a known issue, and while 
I proposed the same approach, I understand that in some cases userspace may 
need to know exactly what values are acceptable. In such a case, however, I 
would expect userspace to have knowledge of the particular sensor model, so 
the information may not need to come from the kernel.

> Now the gain control has range 16...1984 out of which only 1024 values
> are valid. It might not be best approach for a GUI but at least the driver
> exposes mapping of all valid values, which could be enumerated with
> VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific
> user space code.

That would be ~2000 ioctl calls, I don't think that's very practical :-S

> >> I can see two solutions here:
> >> 
> >> 1) Define the control range from 0 to 4 and treat it as an exponent of
> >> 2, so that the value for the sensor becomes (1 << val) * 256.
> >> (Suggested by Sakari offline.)
> >> 
> >> This approach has the problem of losing the original unit (and scale)
> >> of the value.
> > 
> > Exactly - will users be confused by this? If we have to explain it,
> > probably not the best choice.
> > 
> >> 2) Use an integer menu control, which reports only the supported
> >> discrete values - {1, 2, 4, 8, 16}.
> >> 
> >> With this approach, userspace can enumerate the real gain values, but
> >> we would either need to introduce a new control (e.g.
> >> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> >> register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >> 
> >> Any opinions or better ideas?
> > 
> > My $0.02: leave the user UI alone - let users specify/select anything
> > in the range the normal API or UI allows. But have sensor specific
> > code map all values in that range to values the sensor supports. Users
> > will notice how it works when they play with it.  One can "adjust" the
> > mapping so it "feels right".

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Laurent Pinchart]

Hi Sakari,

On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > [snip]
> > 
> > > +
> > > +/* 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
> > > +
> > 
> > [snip]
> > 
> > > +/* Initialize control handlers */
> > > +static int imx208_init_controls(struct imx208 *imx208)
> > > +{
> > 
> > [snip]
> > 
> > > +       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);
> > 
> > We have a problem here. The sensor supports only a discrete range of
> > values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> > fixed point). This makes it possible for the userspace to set values
> > that are not allowed by the sensor specification and also leaves no
> > way to enumerate the supported values.
> > 
> > I can see two solutions here:
> > 
> > 1) Define the control range from 0 to 4 and treat it as an exponent of
> > 2, so that the value for the sensor becomes (1 << val) * 256.
> > (Suggested by Sakari offline.)
> > 
> > This approach has the problem of losing the original unit (and scale)
> > of the value.
> 
> I'd like to add that this is not a property of the proposed solution.
> 
> Rather, the above needs to be accompanied by additional information
> provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
> other information such as whether the control is linear or exponential (as
> in this case).
> 
> > 2) Use an integer menu control, which reports only the supported
> > discrete values - {1, 2, 4, 8, 16}.
> > 
> > With this approach, userspace can enumerate the real gain values, but
> > we would either need to introduce a new control (e.g.
> > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> 
> New controls in V4L2 are, for the most part, created when there's something
> new to control. The documentation of some controls (similar to e.g. gain)
> documents a unit as well as a prefix but that's the case only because
> there's been no way to tell the unit or prefix otherwise in the API.
> 
> An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
> sure how they came to be though. An accident is a possibility as far as I
> see.

If I remember correctly I introduced the absolute variant for the UVC driver 
(even though git blame points to Brandon Philips). I don't really remember why 
though.

> Controls that have a documented unit use that unit --- as long as that's
> the unit used by the hardware. If it's not, it tends to be that another
> unit is used but the user space has currently no way of knowing this. And
> the digital gain control is no exception to this.
> 
> So if we want to improve the user space's ability to be informed how the
> control values reflect to device configuration, we do need to provide more
> information to the user space. One way to do that would be through
> VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
> just for purposes such as this one.

I don't think we can come up with a good way to expose arbitrary mathematical 
formulas through an ioctl. In my opinion the question is how far we want to 
go, how precise we need to be.

> > Any opinions or better ideas?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Ricardo Ribalda Delgado]

HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* 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
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */
> > > > +static int imx208_init_controls(struct imx208 *imx208)
> > > > +{
> > >
> > > [snip]
> > >
> > > > +       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);
> > >
> > > We have a problem here. The sensor supports only a discrete range of
> > > values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> > > fixed point). This makes it possible for the userspace to set values
> > > that are not allowed by the sensor specification and also leaves no
> > > way to enumerate the supported values.
> > >
> > > I can see two solutions here:
> > >
> > > 1) Define the control range from 0 to 4 and treat it as an exponent of
> > > 2, so that the value for the sensor becomes (1 << val) * 256.
> > > (Suggested by Sakari offline.)
> > >
> > > This approach has the problem of losing the original unit (and scale)
> > > of the value.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as well as
> > other information such as whether the control is linear or exponential (as
> > in this case).
> >
> > > 2) Use an integer menu control, which reports only the supported
> > > discrete values - {1, 2, 4, 8, 16}.
> > >
> > > With this approach, userspace can enumerate the real gain values, but
> > > we would either need to introduce a new control (e.g.
> > > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> > > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >
> > New controls in V4L2 are, for the most part, created when there's something
> > new to control. The documentation of some controls (similar to e.g. gain)
> > documents a unit as well as a prefix but that's the case only because
> > there's been no way to tell the unit or prefix otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not entirely
> > sure how they came to be though. An accident is a possibility as far as I
> > see.
>
> If I remember correctly I introduced the absolute variant for the UVC driver
> (even though git blame points to Brandon Philips). I don't really remember why
> though.
>
> > Controls that have a documented unit use that unit --- as long as that's
> > the unit used by the hardware. If it's not, it tends to be that another
> > unit is used but the user space has currently no way of knowing this. And
> > the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how the
> > control values reflect to device configuration, we do need to provide more
> > information to the user space. One way to do that would be through
> > VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved fields ---
> > just for purposes such as this one.
>
> I don't think we can come up with a good way to expose arbitrary mathematical
> formulas through an ioctl. In my opinion the question is how far we want to
> go, how precise we need to be.
>
> > > Any opinions or better ideas?

My 0.02 DKK.  On a similar situation, where userspace was running a
close loop calibration:

We have implemented two extra control: eposure_next exposure_pre that
tell us which one is the next value. Perhaps we could embebed such
functionality in QUERY_EXT_CTRL.

Cheers


>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


-- 
Ricardo Ribalda

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Chen, Ping-chung]

Hi Sakari,

>-----Original Message-----
>From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@gmail.com] 
>Sent: Friday, September 21, 2018 5:55 AM

>HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* 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
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */ static int 
> > > > +imx208_init_controls(struct imx208 *imx208) {
> > >
> > > [snip]
> > >
> > > > +       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);
> > >
> > > We have a problem here. The sensor supports only a discrete range 
> > > of values here - {1, 2, 4, 8, 16} (multiplied by 256, since the 
> > > value is fixed point). This makes it possible for the userspace to 
> > > set values that are not allowed by the sensor specification and 
> > > also leaves no way to enumerate the supported values.
> > >
> > > I can see two solutions here:
> > >
> > > 1) Define the control range from 0 to 4 and treat it as an 
> > > exponent of 2, so that the value for the sensor becomes (1 << val) * 256.
> > > (Suggested by Sakari offline.)
> > >
> > > This approach has the problem of losing the original unit (and 
> > > scale) of the value.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information 
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as 
> > well as other information such as whether the control is linear or 
> > exponential (as in this case).
> >
> > > 2) Use an integer menu control, which reports only the supported 
> > > discrete values - {1, 2, 4, 8, 16}.
> > >
> > > With this approach, userspace can enumerate the real gain values, 
> > > but we would either need to introduce a new control (e.g.
> > > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and 
> > > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >
> > New controls in V4L2 are, for the most part, created when there's 
> > something new to control. The documentation of some controls 
> > (similar to e.g. gain) documents a unit as well as a prefix but 
> > that's the case only because there's been no way to tell the unit or prefix otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not 
> > entirely sure how they came to be though. An accident is a 
> > possibility as far as I see.
>
> If I remember correctly I introduced the absolute variant for the UVC 
> driver (even though git blame points to Brandon Philips). I don't 
> really remember why though.
>
> > Controls that have a documented unit use that unit --- as long as 
> > that's the unit used by the hardware. If it's not, it tends to be 
> > that another unit is used but the user space has currently no way of 
> > knowing this. And the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how 
> > the control values reflect to device configuration, we do need to 
> > provide more information to the user space. One way to do that would 
> > be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of 
> > reserved fields --- just for purposes such as this one.
>
> I don't think we can come up with a good way to expose arbitrary 
> mathematical formulas through an ioctl. In my opinion the question is 
> how far we want to go, how precise we need to be.
>
> > > Any opinions or better ideas?

>My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:

>We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.

>Cheers

How about creating an additional control to handle the case of V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of setting total_gain=AGxDG.

In the case of V4L2_CID_ANALOGUE_GAIN and V4L2_CID_EXPOSURE of imx208_set_ctrl(), it is no need to do any change for WA. The platform which has the feature of IPS gain can go this way as well. And in the case of V4L2_CID_GAIN in imx208_set_ctrl(), the imx208 drivers takes the total_gain from ctrl->val, and the WA for AG&DG re-calculation can be implemented here.

We only need add one flag in camera profiles to notify HAL to select V4L2_CID_GAIN: <discreteDigitalGain value="true"/>

In HAL:
Get3AResult(&3A);
if (discreteDigitalGain)
	total_gain = 3A.ag;
	ioctl(fd, V4L2_CID_GAIN, total_gain);
else {
	ioctl(fd, V4L2_CID_ANALOGUE_GAIN, 3A.ag);
	ioctl(fd, V4L2_CID_DIGITAL_GAIN, 3A.dg);
}

Thanks,
PC Chen
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


>--
>Ricardo Ribalda

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Chen, Ping-chung]

Typo.

-----Original Message-----
From: Chen, Ping-chung 
Sent: Friday, September 21, 2018 3:07 PM
To: 'Ricardo Ribalda Delgado' <ricardo.ribalda@gmail.com>; Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>; tfiga@chromium.org; Sakari Ailus <sakari.ailus@linux.intel.com>; sylwester.nawrocki@gmail.com; linux-media <linux-media@vger.kernel.org>; Yeh, Andy <andy.yeh@intel.com>; Lai, Jim <jim.lai@intel.com>; grundler@chromium.org; Mani, Rajmohan <rajmohan.mani@intel.com>
Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

Hi Sakari,

>-----Original Message-----
>From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@gmail.com]
>Sent: Friday, September 21, 2018 5:55 AM

>HI
On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Sakari,
>
> On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > [snip]
> > >
> > > > +
> > > > +/* 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
> > > > +
> > >
> > > [snip]
> > >
> > > > +/* Initialize control handlers */ static int 
> > > > +imx208_init_controls(struct imx208 *imx208) {
> > >
> > > [snip]
> > >
> > > > +       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);
> > >
> > > We have a problem here. The sensor supports only a discrete range 
> > > of values here - {1, 2, 4, 8, 16} (multiplied by 256, since the 
> > > value is fixed point). This makes it possible for the userspace to 
> > > set values that are not allowed by the sensor specification and 
> > > also leaves no way to enumerate the supported values.
> > >
> > > I can see two solutions here:
> > >
> > > 1) Define the control range from 0 to 4 and treat it as an 
> > > exponent of 2, so that the value for the sensor becomes (1 << val) * 256.
> > > (Suggested by Sakari offline.)
> > >
> > > This approach has the problem of losing the original unit (and
> > > scale) of the value.
> >
> > I'd like to add that this is not a property of the proposed solution.
> >
> > Rather, the above needs to be accompanied by additional information 
> > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as 
> > well as other information such as whether the control is linear or 
> > exponential (as in this case).
> >
> > > 2) Use an integer menu control, which reports only the supported 
> > > discrete values - {1, 2, 4, 8, 16}.
> > >
> > > With this approach, userspace can enumerate the real gain values, 
> > > but we would either need to introduce a new control (e.g.
> > > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and 
> > > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >
> > New controls in V4L2 are, for the most part, created when there's 
> > something new to control. The documentation of some controls 
> > (similar to e.g. gain) documents a unit as well as a prefix but 
> > that's the case only because there's been no way to tell the unit or prefix otherwise in the API.
> >
> > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not 
> > entirely sure how they came to be though. An accident is a 
> > possibility as far as I see.
>
> If I remember correctly I introduced the absolute variant for the UVC 
> driver (even though git blame points to Brandon Philips). I don't 
> really remember why though.
>
> > Controls that have a documented unit use that unit --- as long as 
> > that's the unit used by the hardware. If it's not, it tends to be 
> > that another unit is used but the user space has currently no way of 
> > knowing this. And the digital gain control is no exception to this.
> >
> > So if we want to improve the user space's ability to be informed how 
> > the control values reflect to device configuration, we do need to 
> > provide more information to the user space. One way to do that would 
> > be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of 
> > reserved fields --- just for purposes such as this one.
>
> I don't think we can come up with a good way to expose arbitrary 
> mathematical formulas through an ioctl. In my opinion the question is 
> how far we want to go, how precise we need to be.
>
> > > Any opinions or better ideas?

>My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:

>We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.

>Cheers

How about creating an additional control to handle the case of V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of setting total_gain=AGxDG.

In the case of V4L2_CID_ANALOGUE_GAIN and V4L2_CID_EXPOSURE of imx208_set_ctrl(), it is no need to do any change for WA. The platform which has the feature of IPS gain can go this way as well. And in the case of V4L2_CID_GAIN in imx208_set_ctrl(), the imx208 drivers takes the total_gain from ctrl->val, and the WA for AG&DG re-calculation can be implemented here.

We only need add one flag in camera profiles to notify HAL to select V4L2_CID_GAIN: <discreteDigitalGain value="true"/>

In HAL:
Get3AResult(&3A);
if (discreteDigitalGain) {
	total_gain = 3A.ag;
	ioctl(fd, V4L2_CID_GAIN, total_gain);
}
else {
	ioctl(fd, V4L2_CID_ANALOGUE_GAIN, 3A.ag);
	ioctl(fd, V4L2_CID_DIGITAL_GAIN, 3A.dg); 
}

Thanks,
PC Chen
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>


>--
>Ricardo Ribalda

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Helmut Grohne]

On Thu, Sep 20, 2018 at 11:00:26PM +0200, Laurent Pinchart wrote:
> On Thursday, 20 September 2018 23:16:47 EEST Sylwester Nawrocki wrote:
> > On 09/20/2018 06:49 PM, Grant Grundler wrote:
> > > On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
> > >> We have a problem here. The sensor supports only a discrete range of
> > >> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> > >> fixed point). This makes it possible for the userspace to set values
> > >> that are not allowed by the sensor specification and also leaves no
> > >> way to enumerate the supported values.

I'm not sure I understand this correctly. Tomasz appears to imply here
that the number of actually supported gain values is 5.

> > I'm not sure if it's best approach but I once did something similar for
> > the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
> > for fractional part. The driver reports values multiplied by 16. See
> > ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
> > Total Gain to Control Bit Correlation" in the OV9650 datasheet for details.
> > The integer menu control just seemed not suitable for 2^10 values.

As long as values scale linearly (as is the case for fixed point
numbers) that seems like a reasonable approach to me. That assumption
appears to not hold for imx208 where the values scale exponentially.

> I've had a similar discussion on IRC recently with Helmut, who posted a nice 
> summary of the problem on the mailing list (see https://www.mail-archive.com/
> linux-media@vger.kernel.org/msg134502.html). This is a known issue, and while 
> I proposed the same approach, I understand that in some cases userspace may 
> need to know exactly what values are acceptable. In such a case, however, I 
> would expect userspace to have knowledge of the particular sensor model, so 
> the information may not need to come from the kernel.

A big reason to use V4L2 is to abstract hardware. Having to know what
sensor model you use runs counter that goal. Indeed, gain (whether
digital or analogue) can be abstracted in principle. I see little reason
to not do that.

> > Now the gain control has range 16...1984 out of which only 1024 values
> > are valid. It might not be best approach for a GUI but at least the driver
> > exposes mapping of all valid values, which could be enumerated with
> > VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-specific
> > user space code.

If you have very many values, a reasonable compromise could be reducing
the precision. If you try to represent a floating point number, values
with higher exponent will have larger "gaps" when represented as
integers for v4l2. A compromise could be increasing the step size and
thus removing a few of the gains with lower exponent.

> > >> I can see two solutions here:
> > >> 
> > >> 1) Define the control range from 0 to 4 and treat it as an exponent of
> > >> 2, so that the value for the sensor becomes (1 << val) * 256.
> > >> (Suggested by Sakari offline.)
> > >> 
> > >> This approach has the problem of losing the original unit (and scale)
> > >> of the value.

This approach is the one where users will need to know which sensor they
talk to. The one where the hardware abstraction happens in userspace.
Can we please not do that?

> > >> 2) Use an integer menu control, which reports only the supported
> > >> discrete values - {1, 2, 4, 8, 16}.

That's what I ended up doing, though I kinda deferred the problem as I
started using V4L2_CID_ISO_SENSITIVITY, which is an integer menu but not
exactly the right one. My choice will backfire once I submit the driver
though that'll still take a little while.

> > >> With this approach, userspace can enumerate the real gain values, but
> > >> we would either need to introduce a new control (e.g.
> > >> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> > >> register V4L2_CID_DIGITAL_GAIN as an integer menu.
> > >> 
> > >> Any opinions or better ideas?

Abusing the specification sounds like it would break userspace. I'd
avoid doing that.

We do have a history of adding "duplicate" CIDs for gaining a better
specification. For instance, there is V4L2_CID_EXPOSURE (unspecified)
and then there came V4L2_CID_EXPOSURE_ABSOLUTE (unit 100 µs).
V4L2_CID_GAIN got split into V4L2_CID_ANALOGUE_GAIN and
V4L2_CID_DIGITAL_GAIN. Further splitting that into integer menu variants
of analogue and digital gain seems reasonable to me.

If doing so, I suggest using the following rules (up to discussion):
 * Drivers must not provide both the integer menu and an integer control
   for a single gain.
 * Define which value means "no amplification". For
   V4L2_CID_DIGITAL_GAIN this was defined as 0x100. Keeping that could
   be reasonable, but V4L2_CID_ANALOGUE_GAIN presently leavs that
   undefined.
 * If a gain is linear, use the integer control.
 * If it is non-linear and has fewer than X (1025?) values, use the
   integer menu.
 * Otherwise use the integer control. Either increase step or choose the
   best supported value approximating the user request.

I think that precise rules help both driver writers and users of these
controls.

If an integer menu for V4L2_CID_ANALOGUE_GAIN is being added, I will use
it.

> > > My $0.02: leave the user UI alone - let users specify/select anything
> > > in the range the normal API or UI allows. But have sensor specific
> > > code map all values in that range to values the sensor supports. Users
> > > will notice how it works when they play with it.  One can "adjust" the
> > > mapping so it "feels right".

Actual users trying to fiddle with these values are not the only
consumers. Beware that algorithms may want to do so as well and
algorithms want to know which values are valid in advance.

Hope this helps. It might be raising more questions beyond the scope of
imx208 though.

Helmut

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

Hi Ping-chung,

On Fri, Sep 21, 2018 at 07:08:10AM +0000, Chen, Ping-chung wrote:
> Typo.
> 
> -----Original Message-----
> From: Chen, Ping-chung 
> Sent: Friday, September 21, 2018 3:07 PM
> To: 'Ricardo Ribalda Delgado' <ricardo.ribalda@gmail.com>; Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>; tfiga@chromium.org; Sakari Ailus <sakari.ailus@linux.intel.com>; sylwester.nawrocki@gmail.com; linux-media <linux-media@vger.kernel.org>; Yeh, Andy <andy.yeh@intel.com>; Lai, Jim <jim.lai@intel.com>; grundler@chromium.org; Mani, Rajmohan <rajmohan.mani@intel.com>
> Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
> 
> Hi Sakari,
> 
> >-----Original Message-----
> >From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@gmail.com]
> >Sent: Friday, September 21, 2018 5:55 AM
> 
> >HI
> On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Sakari,
> >
> > On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > > [snip]
> > > >
> > > > > +
> > > > > +/* 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
> > > > > +
> > > >
> > > > [snip]
> > > >
> > > > > +/* Initialize control handlers */ static int 
> > > > > +imx208_init_controls(struct imx208 *imx208) {
> > > >
> > > > [snip]
> > > >
> > > > > +       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);
> > > >
> > > > We have a problem here. The sensor supports only a discrete range 
> > > > of values here - {1, 2, 4, 8, 16} (multiplied by 256, since the 
> > > > value is fixed point). This makes it possible for the userspace to 
> > > > set values that are not allowed by the sensor specification and 
> > > > also leaves no way to enumerate the supported values.
> > > >
> > > > I can see two solutions here:
> > > >
> > > > 1) Define the control range from 0 to 4 and treat it as an 
> > > > exponent of 2, so that the value for the sensor becomes (1 << val) * 256.
> > > > (Suggested by Sakari offline.)
> > > >
> > > > This approach has the problem of losing the original unit (and
> > > > scale) of the value.
> > >
> > > I'd like to add that this is not a property of the proposed solution.
> > >
> > > Rather, the above needs to be accompanied by additional information 
> > > provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, prefix as 
> > > well as other information such as whether the control is linear or 
> > > exponential (as in this case).
> > >
> > > > 2) Use an integer menu control, which reports only the supported 
> > > > discrete values - {1, 2, 4, 8, 16}.
> > > >
> > > > With this approach, userspace can enumerate the real gain values, 
> > > > but we would either need to introduce a new control (e.g.
> > > > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and 
> > > > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> > >
> > > New controls in V4L2 are, for the most part, created when there's 
> > > something new to control. The documentation of some controls 
> > > (similar to e.g. gain) documents a unit as well as a prefix but 
> > > that's the case only because there's been no way to tell the unit or prefix otherwise in the API.
> > >
> > > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not 
> > > entirely sure how they came to be though. An accident is a 
> > > possibility as far as I see.
> >
> > If I remember correctly I introduced the absolute variant for the UVC 
> > driver (even though git blame points to Brandon Philips). I don't 
> > really remember why though.
> >
> > > Controls that have a documented unit use that unit --- as long as 
> > > that's the unit used by the hardware. If it's not, it tends to be 
> > > that another unit is used but the user space has currently no way of 
> > > knowing this. And the digital gain control is no exception to this.
> > >
> > > So if we want to improve the user space's ability to be informed how 
> > > the control values reflect to device configuration, we do need to 
> > > provide more information to the user space. One way to do that would 
> > > be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of 
> > > reserved fields --- just for purposes such as this one.
> >
> > I don't think we can come up with a good way to expose arbitrary 
> > mathematical formulas through an ioctl. In my opinion the question is 
> > how far we want to go, how precise we need to be.
> >
> > > > Any opinions or better ideas?
> 
> >My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:
> 
> >We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.
> 
> >Cheers
> 
> How about creating an additional control to handle the case of
> V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately for
> the general sensor usage by V4L2_CID_ANALOGUE_GAIN and
> V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of setting
> total_gain=AGxDG.

How do you update the two other controls if the user updates the
V4L2_CID_GAIN control?

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Chen, Ping-chung]

Hi Sakari,

>-----Original Message-----
>From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com] 
>Sent: Tuesday, September 25, 2018 5:25 PM
>To: Chen, Ping-chung <ping-chung.chen@intel.com>
>Cc: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>; Laurent Pinchart 
>Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

>Hi Ping-chung,

>On Fri, Sep 21, 2018 at 07:08:10AM +0000, Chen, Ping-chung wrote:
> Typo.
> 
> -----Original Message-----
> From: Chen, Ping-chung
> Sent: Friday, September 21, 2018 3:07 PM
> To: 'Ricardo Ribalda Delgado' <ricardo.ribalda@gmail.com>; Laurent 
> Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>; tfiga@chromium.org; Sakari 
> Ailus <sakari.ailus@linux.intel.com>; sylwester.nawrocki@gmail.com; 
> linux-media <linux-media@vger.kernel.org>; Yeh, Andy 
> <andy.yeh@intel.com>; Lai, Jim <jim.lai@intel.com>; 
> grundler@chromium.org; Mani, Rajmohan <rajmohan.mani@intel.com>
> Subject: RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver
> 
> Hi Sakari,
> 
> >-----Original Message-----
> >From: Ricardo Ribalda Delgado [mailto:ricardo.ribalda@gmail.com]
> >Sent: Friday, September 21, 2018 5:55 AM
> 
> >HI
> On Thu, Sep 20, 2018 at 11:13 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Sakari,
> >
> > On Thursday, 20 September 2018 23:56:59 EEST Sakari Ailus wrote:
> > > On Thu, Sep 20, 2018 at 05:51:55PM +0900, Tomasz Figa wrote:
> > > > On Wed, Aug 8, 2018 at 4:08 PM Ping-chung Chen wrote:
> > > > [snip]
> > > >
> > > > > +
> > > > > +/* 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
> > > > > +
> > > >
> > > > [snip]
> > > >
> > > > > +/* Initialize control handlers */ static int 
> > > > > +imx208_init_controls(struct imx208 *imx208) {
> > > >
> > > > [snip]
> > > >
> > > > > +       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);
> > > >
> > > > We have a problem here. The sensor supports only a discrete 
> > > > range of values here - {1, 2, 4, 8, 16} (multiplied by 256, 
> > > > since the value is fixed point). This makes it possible for the 
> > > > userspace to set values that are not allowed by the sensor 
> > > > specification and also leaves no way to enumerate the supported values.
> > > >
> > > > I can see two solutions here:
> > > >
> > > > 1) Define the control range from 0 to 4 and treat it as an 
> > > > exponent of 2, so that the value for the sensor becomes (1 << val) * 256.
> > > > (Suggested by Sakari offline.)
> > > >
> > > > This approach has the problem of losing the original unit (and
> > > > scale) of the value.
> > >
> > > I'd like to add that this is not a property of the proposed solution.
> > >
> > > Rather, the above needs to be accompanied by additional 
> > > information provided through VIDIOC_QUERY_EXT_CTRL, i.e. the unit, 
> > > prefix as well as other information such as whether the control is 
> > > linear or exponential (as in this case).
> > >
> > > > 2) Use an integer menu control, which reports only the supported 
> > > > discrete values - {1, 2, 4, 8, 16}.
> > > >
> > > > With this approach, userspace can enumerate the real gain 
> > > > values, but we would either need to introduce a new control (e.g.
> > > > V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and 
> > > > register V4L2_CID_DIGITAL_GAIN as an integer menu.
> > >
> > > New controls in V4L2 are, for the most part, created when there's 
> > > something new to control. The documentation of some controls 
> > > (similar to e.g. gain) documents a unit as well as a prefix but 
> > > that's the case only because there's been no way to tell the unit or prefix otherwise in the API.
> > >
> > > An exception to this are EXPOSURE and EXPOSURE_ABSOLUTE. I'm not 
> > > entirely sure how they came to be though. An accident is a 
> > > possibility as far as I see.
> >
> > If I remember correctly I introduced the absolute variant for the 
> > UVC driver (even though git blame points to Brandon Philips). I 
> > don't really remember why though.
> >
> > > Controls that have a documented unit use that unit --- as long as 
> > > that's the unit used by the hardware. If it's not, it tends to be 
> > > that another unit is used but the user space has currently no way 
> > > of knowing this. And the digital gain control is no exception to this.
> > >
> > > So if we want to improve the user space's ability to be informed 
> > > how the control values reflect to device configuration, we do need 
> > > to provide more information to the user space. One way to do that 
> > > would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has 
> > > plenty of reserved fields --- just for purposes such as this one.
> >
> > I don't think we can come up with a good way to expose arbitrary 
> > mathematical formulas through an ioctl. In my opinion the question 
> > is how far we want to go, how precise we need to be.
> >
> > > > Any opinions or better ideas?
> 
> >My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:
> 
> >We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.
> 
> >Cheers
> 
> How about creating an additional control to handle the case of 
> V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately 
> for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and 
> V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of 
> setting total_gain=AGxDG.

>How do you update the two other controls if the user updates the V4L2_CID_GAIN control?

In imx208 driver:

Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global structure imx208.
static int imx208_init_controls(struct imx208 *imx208) {
Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN, ...);
Imx208->analog_gain = v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);

static int imx208_set_ctrl(struct v4l2_ctrl *ctrl)
{
...
	case V4L2_CID_ANALOGUE_GAIN:
		ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, ctrl->val);
		break;
	case V4L2_CID_DIGITAL_GAIN:
		ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
		break;
	case V4L2_CID_ GAIN:
		ret = imx208_update_gain(imx208, 2, ctrl->val);  // total gain
		break;
}

Then the implementation of imx208_update_gain():
static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val)
{
	digital_gain = (val - 1) / ag_max;
	digital_gain = map_to_real_DG(digital_gain);  		// map to 1x, 2x, 4x, 8x, 16x
	digital_gain_code = digital_gain << 8			//  DGx256 for DG_code
	ret = imx208_update_digital_gain(imx208, 2, digital_gain_code); 
	imx208->digital_gain->val = digital_gain_code;
	analog_gain = val/digital_gain;
	analog_gain_code = SMIA_AG_to_Code(analog_gain);  // AG = 256/(256-ag_code)
	ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, analog_gain_code);
	imx208->digital_gain->val = analog_gain_code;

	return ret;
}

Thanks,
PC Chen
>--
>Sakari Ailus
>sakari.ailus@linux.intel.com

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

Hi Ping-chung,

On Tue, Sep 25, 2018 at 10:17:48AM +0000, Chen, Ping-chung wrote:
...
> > > > Controls that have a documented unit use that unit --- as long as 
> > > > that's the unit used by the hardware. If it's not, it tends to be 
> > > > that another unit is used but the user space has currently no way 
> > > > of knowing this. And the digital gain control is no exception to this.
> > > >
> > > > So if we want to improve the user space's ability to be informed 
> > > > how the control values reflect to device configuration, we do need 
> > > > to provide more information to the user space. One way to do that 
> > > > would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has 
> > > > plenty of reserved fields --- just for purposes such as this one.
> > >
> > > I don't think we can come up with a good way to expose arbitrary 
> > > mathematical formulas through an ioctl. In my opinion the question 
> > > is how far we want to go, how precise we need to be.
> > >
> > > > > Any opinions or better ideas?
> > 
> > >My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:
> > 
> > >We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.
> > 
> > >Cheers
> > 
> > How about creating an additional control to handle the case of 
> > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately 
> > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and 
> > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of 
> > setting total_gain=AGxDG.
> 
> >How do you update the two other controls if the user updates the V4L2_CID_GAIN control?
> 
> In imx208 driver:
> 
> Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global structure imx208.
> static int imx208_init_controls(struct imx208 *imx208) {
> Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN, ...);
> Imx208->analog_gain = v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
> 
> static int imx208_set_ctrl(struct v4l2_ctrl *ctrl)
> {
> ...
> 	case V4L2_CID_ANALOGUE_GAIN:
> 		ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, ctrl->val);
> 		break;
> 	case V4L2_CID_DIGITAL_GAIN:
> 		ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> 		break;
> 	case V4L2_CID_ GAIN:
> 		ret = imx208_update_gain(imx208, 2, ctrl->val);  // total gain
> 		break;
> }
> 
> Then the implementation of imx208_update_gain():
> static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val)
> {
> 	digital_gain = (val - 1) / ag_max;
> 	digital_gain = map_to_real_DG(digital_gain);  		// map to 1x, 2x, 4x, 8x, 16x
> 	digital_gain_code = digital_gain << 8			//  DGx256 for DG_code
> 	ret = imx208_update_digital_gain(imx208, 2, digital_gain_code); 
> 	imx208->digital_gain->val = digital_gain_code;
> 	analog_gain = val/digital_gain;
> 	analog_gain_code = SMIA_AG_to_Code(analog_gain);  // AG = 256/(256-ag_code)
> 	ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, analog_gain_code);
> 	imx208->digital_gain->val = analog_gain_code;

How about putting this piece of code to the user space instead?

Some work would be needed to generalise it in order for it to work on other
sensors that do need digital gain applied, too --- assuming it'd be
combined with the TRY_EXT_CTRLS rounding flags.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Chen, Ping-chung]

Hi Sakari,

>-----Original Message-----
>From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com] 
>Sent: Wednesday, September 26, 2018 5:55 AM

>Hi Ping-chung,

>On Tue, Sep 25, 2018 at 10:17:48AM +0000, Chen, Ping-chung wrote:
>...
> > > > Controls that have a documented unit use that unit --- as long 
> > > > as that's the unit used by the hardware. If it's not, it tends 
> > > > to be that another unit is used but the user space has currently 
> > > > no way of knowing this. And the digital gain control is no exception to this.
> > > >
> > > > So if we want to improve the user space's ability to be informed 
> > > > how the control values reflect to device configuration, we do 
> > > > need to provide more information to the user space. One way to 
> > > > do that would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct 
> > > > has plenty of reserved fields --- just for purposes such as this one.
> > >
> > > I don't think we can come up with a good way to expose arbitrary 
> > > mathematical formulas through an ioctl. In my opinion the question 
> > > is how far we want to go, how precise we need to be.
> > >
> > > > > Any opinions or better ideas?
> > 
> > >My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:
> > 
> > >We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.
> > 
> > >Cheers
> > 
> > How about creating an additional control to handle the case of 
> > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately 
> > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and 
> > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of 
> > setting total_gain=AGxDG.
> 
> >How do you update the two other controls if the user updates the V4L2_CID_GAIN control?
> 
> In imx208 driver:
> 
> Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global structure imx208.
> static int imx208_init_controls(struct imx208 *imx208) {
> Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN, 
> Imx208->...); analog_gain = 
> Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
> 
> static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
> 	case V4L2_CID_ANALOGUE_GAIN:
> 		ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, ctrl->val);
> 		break;
> 	case V4L2_CID_DIGITAL_GAIN:
> 		ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> 		break;
> 	case V4L2_CID_ GAIN:
> 		ret = imx208_update_gain(imx208, 2, ctrl->val);  // total gain
> 		break;
> }
> 
> Then the implementation of imx208_update_gain():
> static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val) 
> {
> 	digital_gain = (val - 1) / ag_max;
> 	digital_gain = map_to_real_DG(digital_gain);  		// map to 1x, 2x, 4x, 8x, 16x
> 	digital_gain_code = digital_gain << 8			//  DGx256 for DG_code
> 	ret = imx208_update_digital_gain(imx208, 2, digital_gain_code); 
> 	imx208->digital_gain->val = digital_gain_code;
> 	analog_gain = val/digital_gain;
> 	analog_gain_code = SMIA_AG_to_Code(analog_gain);  // AG = 256/(256-ag_code)
> 	ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, analog_gain_code);
> 	imx208->digital_gain->val = analog_gain_code;

>How about putting this piece of code to the user space instead?

>Some work would be needed to generalise it in order for it to work on other sensors that do need >digital gain applied, too --- assuming it'd be combined with the TRY_EXT_CTRLS rounding flags.

There might be many kinds of discrete DG formats. For imx208, DG=2^n, but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL needs to cover all cases, kernel will have to update more information to this control.
Another problem is should HAL take over the SMIA calculation? If so, kernel will also need to update SMIA parameters to user space (or create an addition filed for SMIA in the configuration XML file).

Thanks,
PC Chen

>--
>Kind regards,

>Sakari Ailus
>sakari.ailus@linux.intel.com

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

Hi Ping-chung,

On Wed, Sep 26, 2018 at 02:27:01AM +0000, Chen, Ping-chung wrote:
> Hi Sakari,
> 
> >-----Original Message-----
> >From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com] 
> >Sent: Wednesday, September 26, 2018 5:55 AM
> 
> >Hi Ping-chung,
> 
> >On Tue, Sep 25, 2018 at 10:17:48AM +0000, Chen, Ping-chung wrote:
> >...
> > > > > Controls that have a documented unit use that unit --- as long 
> > > > > as that's the unit used by the hardware. If it's not, it tends 
> > > > > to be that another unit is used but the user space has currently 
> > > > > no way of knowing this. And the digital gain control is no exception to this.
> > > > >
> > > > > So if we want to improve the user space's ability to be informed 
> > > > > how the control values reflect to device configuration, we do 
> > > > > need to provide more information to the user space. One way to 
> > > > > do that would be through VIDIOC_QUERY_EXT_CTRL. The IOCTL struct 
> > > > > has plenty of reserved fields --- just for purposes such as this one.
> > > >
> > > > I don't think we can come up with a good way to expose arbitrary 
> > > > mathematical formulas through an ioctl. In my opinion the question 
> > > > is how far we want to go, how precise we need to be.
> > > >
> > > > > > Any opinions or better ideas?
> > > 
> > > >My 0.02 DKK.  On a similar situation, where userspace was running a close loop calibration:
> > > 
> > > >We have implemented two extra control: eposure_next exposure_pre that tell us which one is the next value. Perhaps we could embebed such functionality in QUERY_EXT_CTRL.
> > > 
> > > >Cheers
> > > 
> > > How about creating an additional control to handle the case of 
> > > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG separately 
> > > for the general sensor usage by V4L2_CID_ANALOGUE_GAIN and 
> > > V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition of 
> > > setting total_gain=AGxDG.
> > 
> > >How do you update the two other controls if the user updates the V4L2_CID_GAIN control?
> > 
> > In imx208 driver:
> > 
> > Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global structure imx208.
> > static int imx208_init_controls(struct imx208 *imx208) {
> > Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN, 
> > Imx208->...); analog_gain = 
> > Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
> > 
> > static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
> > 	case V4L2_CID_ANALOGUE_GAIN:
> > 		ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, ctrl->val);
> > 		break;
> > 	case V4L2_CID_DIGITAL_GAIN:
> > 		ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
> > 		break;
> > 	case V4L2_CID_ GAIN:
> > 		ret = imx208_update_gain(imx208, 2, ctrl->val);  // total gain
> > 		break;
> > }
> > 
> > Then the implementation of imx208_update_gain():
> > static int imx208_update_gain(struct imx208 *imx208, u32 len, u32 val) 
> > {
> > 	digital_gain = (val - 1) / ag_max;
> > 	digital_gain = map_to_real_DG(digital_gain);  		// map to 1x, 2x, 4x, 8x, 16x
> > 	digital_gain_code = digital_gain << 8			//  DGx256 for DG_code
> > 	ret = imx208_update_digital_gain(imx208, 2, digital_gain_code); 
> > 	imx208->digital_gain->val = digital_gain_code;
> > 	analog_gain = val/digital_gain;
> > 	analog_gain_code = SMIA_AG_to_Code(analog_gain);  // AG = 256/(256-ag_code)
> > 	ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2, analog_gain_code);
> > 	imx208->digital_gain->val = analog_gain_code;
> 
> >How about putting this piece of code to the user space instead?
> 
> >Some work would be needed to generalise it in order for it to work on other sensors that do need >digital gain applied, too --- assuming it'd be combined with the TRY_EXT_CTRLS rounding flags.
> 
> There might be many kinds of discrete DG formats. For imx208, DG=2^n, but
> for other sensors, DG could be 2*n, 5*n, or other styles. If HAL needs to

I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
multiplying the value by a 16-bit number with a 8-bit fractional part. The
imx208 apparently lacks the multiplication and only has the bit shift.

Usually there's some sort of technical reason for the choice of the digital
gain implementation and therefore I expect at least the vast majority of
the implementations to be either of the two.

> cover all cases, kernel will have to update more information to this
> control. Another problem is should HAL take over the SMIA calculation? If
> so, kernel will also need to update SMIA parameters to user space (or
> create an addition filed for SMIA in the configuration XML file).

The parameters for the analogue gain model should come from the driver,
yes. We do not have controls for that purpose but they can (and should) be
added.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Yeh, Andy]

Hi Sakari, PC,

>-----Original Message-----
>From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com]
>Sent: Wednesday, September 26, 2018 6:12 PM
>To: Chen, Ping-chung <ping-chung.chen@intel.com>
>Cc: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>; Laurent Pinchart
><laurent.pinchart@ideasonboard.com>; Sakari Ailus <sakari.ailus@iki.fi>;
>tfiga@chromium.org; sylwester.nawrocki@gmail.com; linux-media <linux-
>media@vger.kernel.org>; Yeh, Andy <andy.yeh@intel.com>; Lai, Jim
><jim.lai@intel.com>; grundler@chromium.org; Mani, Rajmohan
><rajmohan.mani@intel.com>
>Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver
>
>Hi Ping-chung,
>
>On Wed, Sep 26, 2018 at 02:27:01AM +0000, Chen, Ping-chung wrote:
>> Hi Sakari,
>>
>> >-----Original Message-----
>> >From: Sakari Ailus [mailto:sakari.ailus@linux.intel.com]
>> >Sent: Wednesday, September 26, 2018 5:55 AM
>>
>> >Hi Ping-chung,
>>
>> >On Tue, Sep 25, 2018 at 10:17:48AM +0000, Chen, Ping-chung wrote:
>> >...
>> > > > > Controls that have a documented unit use that unit --- as long
>> > > > > as that's the unit used by the hardware. If it's not, it tends
>> > > > > to be that another unit is used but the user space has
>> > > > > currently no way of knowing this. And the digital gain control is no
>exception to this.
>> > > > >
>> > > > > So if we want to improve the user space's ability to be
>> > > > > informed how the control values reflect to device
>> > > > > configuration, we do need to provide more information to the
>> > > > > user space. One way to do that would be through
>> > > > > VIDIOC_QUERY_EXT_CTRL. The IOCTL struct has plenty of reserved
>fields --- just for purposes such as this one.
>> > > >
>> > > > I don't think we can come up with a good way to expose arbitrary
>> > > > mathematical formulas through an ioctl. In my opinion the
>> > > > question is how far we want to go, how precise we need to be.
>> > > >
>> > > > > > Any opinions or better ideas?
>> > >
>> > > >My 0.02 DKK.  On a similar situation, where userspace was running a
>close loop calibration:
>> > >
>> > > >We have implemented two extra control: eposure_next exposure_pre
>that tell us which one is the next value. Perhaps we could embebed such
>functionality in QUERY_EXT_CTRL.
>> > >
>> > > >Cheers
>> > >
>> > > How about creating an additional control to handle the case of
>> > > V4L2_CID_GAIN in the imx208 driver? HAL can set AG and DG
>> > > separately for the general sensor usage by V4L2_CID_ANALOGUE_GAIN
>> > > and V4L2_CID_DIGITAL_GAIN but call V4L2_CID_GAIN for the condition
>> > > of setting total_gain=AGxDG.
>> >
>> > >How do you update the two other controls if the user updates the
>V4L2_CID_GAIN control?
>> >
>> > In imx208 driver:
>> >
>> > Add two pointers *digital_gain and *analog_gain of v4l2_ctrl in the global
>structure imx208.
>> > static int imx208_init_controls(struct imx208 *imx208) {
>> > Imx208->digital_gain = v4l2_ctrl_new_std(..., V4L2_CID_DIGITAL_GAIN,
>> > Imx208->...); analog_gain =
>> > Imx208->v4l2_ctrl_new_std(...,V4L2_CID_ANALOGUE_GAIN, ...);
>> >
>> > static int imx208_set_ctrl(struct v4l2_ctrl *ctrl) { ...
>> > 	case V4L2_CID_ANALOGUE_GAIN:
>> > 		ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
>ctrl->val);
>> > 		break;
>> > 	case V4L2_CID_DIGITAL_GAIN:
>> > 		ret = imx208_update_digital_gain(imx208, 2, ctrl->val);
>> > 		break;
>> > 	case V4L2_CID_ GAIN:
>> > 		ret = imx208_update_gain(imx208, 2, ctrl->val);  // total gain
>> > 		break;
>> > }
>> >
>> > Then the implementation of imx208_update_gain():
>> > static int imx208_update_gain(struct imx208 *imx208, u32 len, u32
>> > val) {
>> > 	digital_gain = (val - 1) / ag_max;
>> > 	digital_gain = map_to_real_DG(digital_gain);  		// map to 1x,
>2x, 4x, 8x, 16x
>> > 	digital_gain_code = digital_gain << 8			//  DGx256 for
>DG_code
>> > 	ret = imx208_update_digital_gain(imx208, 2, digital_gain_code);
>> > 	imx208->digital_gain->val = digital_gain_code;
>> > 	analog_gain = val/digital_gain;
>> > 	analog_gain_code = SMIA_AG_to_Code(analog_gain);  // AG =
>256/(256-ag_code)
>> > 	ret = imx208_write_reg(imx208, IMX208_REG_ANALOG_GAIN, 2,
>analog_gain_code);
>> > 	imx208->digital_gain->val = analog_gain_code;
>>
>> >How about putting this piece of code to the user space instead?
>>
>> >Some work would be needed to generalise it in order for it to work on other
>sensors that do need >digital gain applied, too --- assuming it'd be combined
>with the TRY_EXT_CTRLS rounding flags.
>>
>> There might be many kinds of discrete DG formats. For imx208, DG=2^n,
>> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL
>> needs to
>
>I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
>multiplying the value by a 16-bit number with a 8-bit fractional part. The
>imx208 apparently lacks the multiplication and only has the bit shift.
>
>Usually there's some sort of technical reason for the choice of the digital gain
>implementation and therefore I expect at least the vast majority of the
>implementations to be either of the two.

We shall ensure the expansibility of this architecture to include other kind of styles in the future. Is this API design architecture-wise ok?

>
>> cover all cases, kernel will have to update more information to this
>> control. Another problem is should HAL take over the SMIA calculation?
>> If so, kernel will also need to update SMIA parameters to user space
>> (or create an addition filed for SMIA in the configuration XML file).
>
>The parameters for the analogue gain model should come from the driver, yes.
>We do not have controls for that purpose but they can (and should) be added.
>

How about still follow PC's proposal to implement in XML? It was in IQ tuning file before which is in userspace. Even I proposed to PC to study with ICG SW team whether this info could be retrieved from 3A algorithm.

>--
>Regards,
>
>Sakari Ailus
>sakari.ailus@linux.intel.com

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Chen, Ping-chung]

Hi,

>-----Original Message-----
>From: Yeh, Andy 
>Sent: Wednesday, September 26, 2018 11:19 PM
>To: Sakari Ailus <sakari.ailus@linux.intel.com>; Chen, Ping-chung <ping-chung.chen@intel.com>

>Hi Sakari, PC,

>sensors that do need >digital gain applied, too --- assuming it'd be 
>combined with the TRY_EXT_CTRLS rounding flags.
>>
>> There might be many kinds of discrete DG formats. For imx208, DG=2^n, 
>> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL 
>> needs to
>
>I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
>multiplying the value by a 16-bit number with a 8-bit fractional part. 
>The
>imx208 apparently lacks the multiplication and only has the bit shift.
>
>Usually there's some sort of technical reason for the choice of the 
>digital gain implementation and therefore I expect at least the vast 
>majority of the implementations to be either of the two.

>We shall ensure the expansibility of this architecture to include other kind of styles in the future. Is this API design architecture-wise ok?

Indeed. Seems it is hard to cover all rules and HAL needs complex flow to judge the DG value.
Hi Sakari, could you provide an example that how HAL uses the modified interface to set available digital gain?

>
>> cover all cases, kernel will have to update more information to this 
>> control. Another problem is should HAL take over the SMIA calculation?
>> If so, kernel will also need to update SMIA parameters to user space 
>> (or create an addition filed for SMIA in the configuration XML file).
>
>The parameters for the analogue gain model should come from the driver, yes.
>We do not have controls for that purpose but they can (and should) be added.
>

>How about still follow PC's proposal to implement in XML? It was in IQ tuning file before which is in userspace. Even I proposed to PC to study with ICG SW team whether this info could be retrieved from 3A algorithm.

Hi Andy, because we has to use total gain instead of AG in 3A for the WA, our tuning data of imx208 will not include SMIA of AG anymore. 
So HAL has no way to retrieve correct SMIA parameters of AG from 3A.

Thanks,
PC Chen

>--
>Regards,
>
>Sakari Ailus
>sakari.ailus@linux.intel.com

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Laurent Pinchart]

Hi Helmut,

On Friday, 21 September 2018 10:23:37 EEST Helmut Grohne wrote:
> On Thu, Sep 20, 2018 at 11:00:26PM +0200, Laurent Pinchart wrote:
> > On Thursday, 20 September 2018 23:16:47 EEST Sylwester Nawrocki wrote:
> >> On 09/20/2018 06:49 PM, Grant Grundler wrote:
> >>> On Thu, Sep 20, 2018 at 1:52 AM Tomasz Figa wrote:
> >>>> We have a problem here. The sensor supports only a discrete range of
> >>>> values here - {1, 2, 4, 8, 16} (multiplied by 256, since the value is
> >>>> fixed point). This makes it possible for the userspace to set values
> >>>> that are not allowed by the sensor specification and also leaves no
> >>>> way to enumerate the supported values.
> 
> I'm not sure I understand this correctly. Tomasz appears to imply here
> that the number of actually supported gain values is 5.
> 
> >> I'm not sure if it's best approach but I once did something similar for
> >> the ov9650 sensor. The gain was fixed point 10-bits value with 4 bits
> >> for fractional part. The driver reports values multiplied by 16. See
> >> ov965x_set_gain() function in drivers/media/i2c/ov9650.c and "Table 4-1.
> >> Total Gain to Control Bit Correlation" in the OV9650 datasheet for
> >> details. The integer menu control just seemed not suitable for 2^10
> >> values.
> 
> As long as values scale linearly (as is the case for fixed point
> numbers) that seems like a reasonable approach to me. That assumption
> appears to not hold for imx208 where the values scale exponentially.

And it's often neither. For instance the MT9P031 has three gain stages. 
Quoting the driver:

                /* Gain is controlled by 2 analog stages and a digital stage.
                 * Valid values for the 3 stages are
                 *
                 * Stage                Min     Max     Step
                 * ------------------------------------------
                 * First analog stage   x1      x2      1
                 * Second analog stage  x1      x4      0.125
                 * Digital stage        x1      x16     0.125

The resulting gain is the product of the three.

> > I've had a similar discussion on IRC recently with Helmut, who posted a
> > nice summary of the problem on the mailing list (see
> > https://www.mail-archive.com/
> > linux-media@vger.kernel.org/msg134502.html). This is a known issue, and
> > while I proposed the same approach, I understand that in some cases
> > userspace may need to know exactly what values are acceptable. In such a
> > case, however, I would expect userspace to have knowledge of the
> > particular sensor model, so the information may not need to come from the
> > kernel.
> 
> A big reason to use V4L2 is to abstract hardware. Having to know what
> sensor model you use runs counter that goal. Indeed, gain (whether
> digital or analogue) can be abstracted in principle. I see little reason
> to not do that.

The purpose of V4L2 is to expose the features of the hardware device to 
userspace, and we try to do so in an as much abstract as possible way. 100% 
abstraction isn't feasible, there will always be small device-specific details 
that will break whatever abstraction we design. We should thus strive for a 
good balance, abstracting the most common types of features, while avoiding 
over-engineering an API that would then become unusable (both because it would 
be complex to use by applications, and implemented in subtly differently buggy 
ways by drivers).

I don't think we'll reach an agreement here if we don't start talking about 
real use cases. Would you have some to share ?

> >> Now the gain control has range 16...1984 out of which only 1024 values
> >> are valid. It might not be best approach for a GUI but at least the
> >> driver exposes mapping of all valid values, which could be enumerated
> >> with VIDIOC_TRY_EXT_CTRLS if required, without a need for a driver-
> >> specific user space code.
> 
> If you have very many values, a reasonable compromise could be reducing
> the precision. If you try to represent a floating point number, values
> with higher exponent will have larger "gaps" when represented as
> integers for v4l2. A compromise could be increasing the step size and
> thus removing a few of the gains with lower exponent.

What if an application needs the precision ? Reducing it can cause issues in 
the 3A algorithms, including closed-loops stability problems. I think we 
should expose the device features with as much precision and coverage as 
possible. Hardcoding use case assumptions in the kernel drives us into a wall 
sooner or later.

> >>>> I can see two solutions here:
> >>>> 
> >>>> 1) Define the control range from 0 to 4 and treat it as an exponent
> >>>> of 2, so that the value for the sensor becomes (1 << val) * 256.
> >>>> (Suggested by Sakari offline.)
> >>>> 
> >>>> This approach has the problem of losing the original unit (and scale)
> >>>> of the value.
> 
> This approach is the one where users will need to know which sensor they
> talk to. The one where the hardware abstraction happens in userspace.
> Can we please not do that?

Let's talk about it based on real use cases.

> >>>> 2) Use an integer menu control, which reports only the supported
> >>>> discrete values - {1, 2, 4, 8, 16}.
> 
> That's what I ended up doing, though I kinda deferred the problem as I
> started using V4L2_CID_ISO_SENSITIVITY, which is an integer menu but not
> exactly the right one. My choice will backfire once I submit the driver
> though that'll still take a little while.
> 
> >>>> With this approach, userspace can enumerate the real gain values, but
> >>>> we would either need to introduce a new control (e.g.
> >>>> V4L2_CID_DIGITAL_GAIN_DISCRETE) or abuse the specification and
> >>>> register V4L2_CID_DIGITAL_GAIN as an integer menu.
> >>>> 
> >>>> Any opinions or better ideas?
> 
> Abusing the specification sounds like it would break userspace. I'd
> avoid doing that.
> 
> We do have a history of adding "duplicate" CIDs for gaining a better
> specification. For instance, there is V4L2_CID_EXPOSURE (unspecified)
> and then there came V4L2_CID_EXPOSURE_ABSOLUTE (unit 100 µs).
> V4L2_CID_GAIN got split into V4L2_CID_ANALOGUE_GAIN and
> V4L2_CID_DIGITAL_GAIN. Further splitting that into integer menu variants
> of analogue and digital gain seems reasonable to me.
> 
> If doing so, I suggest using the following rules (up to discussion):
>  * Drivers must not provide both the integer menu and an integer control
>    for a single gain.
>  * Define which value means "no amplification". For
>    V4L2_CID_DIGITAL_GAIN this was defined as 0x100. Keeping that could
>    be reasonable, but V4L2_CID_ANALOGUE_GAIN presently leavs that
>    undefined.

This could be reported as the control's default value.

>  * If a gain is linear, use the integer control.
>  * If it is non-linear and has fewer than X (1025?) values, use the
>    integer menu.

1024 ioctl calls to query the menu values ? :-( We need a better API than 
that. I'm also concerned that it wouldn't be very usable by userspace. Having 
a list of supported values is one thing, making efficient use of it is 
another. Again, use cases :-)

>  * Otherwise use the integer control. Either increase step or choose the
>    best supported value approximating the user request.
> 
> I think that precise rules help both driver writers and users of these
> controls.
> 
> If an integer menu for V4L2_CID_ANALOGUE_GAIN is being added, I will use
> it.

In this particular case I think splitting analog and digital gain makes sense. 
They are very different beasts at the hardware level, and need to be 
controlled independently.

> >>> My $0.02: leave the user UI alone - let users specify/select anything
> >>> in the range the normal API or UI allows. But have sensor specific
> >>> code map all values in that range to values the sensor supports. Users
> >>> will notice how it works when they play with it.  One can "adjust" the
> >>> mapping so it "feels right".
> 
> Actual users trying to fiddle with these values are not the only
> consumers. Beware that algorithms may want to do so as well and
> algorithms want to know which values are valid in advance.

I expect many algorithms to need a mathematical view of the valid values, not 
just a list. What particular algorithms do you have in mind ?

> Hope this helps. It might be raising more questions beyond the scope of
> imx208 though.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Helmut Grohne]

Hi Laurent,

On Fri, Sep 28, 2018 at 03:49:38PM +0200, Laurent Pinchart wrote:
> I don't think we'll reach an agreement here if we don't start talking about 
> real use cases. Would you have some to share ?

Fair enough, but at that point, we very much disconnect from the imx208
in the subject.

I'm working with a stereo camera. In that setup, you have two image
sensors and infer a three dimensional structure from images captured at
equal time points. For that to happen, it is important that the image
sensors use the same settings. In particular, settings such as expoure
and gain must match. That in turn means that you cannot use the
automatic exposure mode that comes with your image sensor.

This is one reason for implementing exposure control outside of the
image sensor. Typically you can categorize your parameters into three
classes that affect the brightness of an image: aperture, shutter speed
and some kind of gain. If you know the units of these parameters, you
can estimate the effect of changing them on the resulting image.

The algorithm for controlling brightness can be quite generic if you
know the units. V4L2_CID_EXPOSURE_ABSOLUTE is given in 100 µs. That
tends to work well. Typically, you prefer increasing exposure over
increasing gain to avoid noise. Similarly, you prefer increasing
analogue gain over increasing digital gain. On the other hand, there is
a limit on exposure to avoid motion blur. If an algorithm knows valid
values for these parameters, it can produce settings independently of
what concrete image sensor you use.

> > >>>> I can see two solutions here:
> > >>>> 
> > >>>> 1) Define the control range from 0 to 4 and treat it as an exponent
> > >>>> of 2, so that the value for the sensor becomes (1 << val) * 256.
> > >>>> (Suggested by Sakari offline.)
> > >>>> 
> > >>>> This approach has the problem of losing the original unit (and scale)
> > >>>> of the value.
> > 
> > This approach is the one where users will need to know which sensor they
> > talk to. The one where the hardware abstraction happens in userspace.
> > Can we please not do that?
> 
> Let's talk about it based on real use cases.

So if you change your gain from 0 to 1, your image becomes roughly twice
as bright. In the typical scenario that's too bright, so when increasing
gain, you simultaneously decrease something else (e.g. exposure). But if
you don't know the effect of your gain change, you cannot tell how much
your exposure needs to be reduced. The only way out here is just doing
it and reducing exposure afterwards. Users tend to not like the
flickering resulting from this approach.

> >  * If it is non-linear and has fewer than X (1025?) values, use the
> >    integer menu.
> 
> 1024 ioctl calls to query the menu values ? :-( We need a better API than 
> that. I'm also concerned that it wouldn't be very usable by userspace. Having 
> a list of supported values is one thing, making efficient use of it is 
> another. Again, use cases :-)

You only need to query it once, but I'm not opposed to a better API
either. I just don't think it is that important or urgent.

> I expect many algorithms to need a mathematical view of the valid values, not 
> just a list. What particular algorithms do you have in mind ?

A very simple algorithm could go like this:
 * Assume that exposure time and gain have a linear effect on the
   brightness of a captured image. This tends to not hold exactly, but
   close enough.
 * Compare brightness of the previous frame with a target value.
 * Compute the product of current exposure time and gain. Adapt the
   product according to the brightness error.
 * Distribute this product to exposure time and gain such that exposure
   time is maximal below a user-defined limit and that gain is one of
   the valid values.

All you need to know for using this besides V4L2_CID_EXPOSURE_ABSOLUTE,
is the valid gain values.

Now I wonder, does this help in reaching a conclusion about whether
querying valid gain values is a relevant use case?

Helmut

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Philippe De Muyter]

Hi,

On Mon, Oct 01, 2018 at 12:50:02PM +0200, Helmut Grohne wrote:
> Hi Laurent,
> 
> On Fri, Sep 28, 2018 at 03:49:38PM +0200, Laurent Pinchart wrote:
> > I don't think we'll reach an agreement here if we don't start talking about 
> > real use cases. Would you have some to share ?
> 
> Fair enough, but at that point, we very much disconnect from the imx208
> in the subject.
> 
> I'm working with a stereo camera. In that setup, you have two image
> sensors and infer a three dimensional structure from images captured at
> equal time points. For that to happen, it is important that the image
> sensors use the same settings. In particular, settings such as expoure
> and gain must match. That in turn means that you cannot use the
> automatic exposure mode that comes with your image sensor.
> 
> This is one reason for implementing exposure control outside of the
> image sensor. Typically you can categorize your parameters into three
> classes that affect the brightness of an image: aperture, shutter speed
> and some kind of gain. If you know the units of these parameters, you
> can estimate the effect of changing them on the resulting image.
> 
> The algorithm for controlling brightness can be quite generic if you
> know the units. V4L2_CID_EXPOSURE_ABSOLUTE is given in 100 µs. That
> tends to work well. Typically, you prefer increasing exposure over
> increasing gain to avoid noise. Similarly, you prefer increasing
> analogue gain over increasing digital gain. On the other hand, there is
> a limit on exposure to avoid motion blur. If an algorithm knows valid
> values for these parameters, it can produce settings independently of
> what concrete image sensor you use.
> 
> > > >>>> I can see two solutions here:
> > > >>>> 
> > > >>>> 1) Define the control range from 0 to 4 and treat it as an exponent
> > > >>>> of 2, so that the value for the sensor becomes (1 << val) * 256.
> > > >>>> (Suggested by Sakari offline.)
> > > >>>> 
> > > >>>> This approach has the problem of losing the original unit (and scale)
> > > >>>> of the value.
> > > 
> > > This approach is the one where users will need to know which sensor they
> > > talk to. The one where the hardware abstraction happens in userspace.
> > > Can we please not do that?
> > 
> > Let's talk about it based on real use cases.
> 
> So if you change your gain from 0 to 1, your image becomes roughly twice
> as bright. In the typical scenario that's too bright, so when increasing
> gain, you simultaneously decrease something else (e.g. exposure). But if
> you don't know the effect of your gain change, you cannot tell how much
> your exposure needs to be reduced. The only way out here is just doing
> it and reducing exposure afterwards. Users tend to not like the
> flickering resulting from this approach.
> 
> > >  * If it is non-linear and has fewer than X (1025?) values, use the
> > >    integer menu.
> > 
> > 1024 ioctl calls to query the menu values ? :-( We need a better API than 
> > that. I'm also concerned that it wouldn't be very usable by userspace. Having 
> > a list of supported values is one thing, making efficient use of it is 
> > another. Again, use cases :-)
> 
> You only need to query it once, but I'm not opposed to a better API
> either. I just don't think it is that important or urgent.
> 
> > I expect many algorithms to need a mathematical view of the valid values, not 
> > just a list. What particular algorithms do you have in mind ?
> 
> A very simple algorithm could go like this:
>  * Assume that exposure time and gain have a linear effect on the
>    brightness of a captured image. This tends to not hold exactly, but
>    close enough.
>  * Compare brightness of the previous frame with a target value.
>  * Compute the product of current exposure time and gain. Adapt the
>    product according to the brightness error.
>  * Distribute this product to exposure time and gain such that exposure
>    time is maximal below a user-defined limit and that gain is one of
>    the valid values.
> 
> All you need to know for using this besides V4L2_CID_EXPOSURE_ABSOLUTE,
> is the valid gain values.
> 

I agree with Helmut, and have exactly the same problem, even without stereo
involved.

But : V4L2_CID_EXPOSURE_ABSOLUTE unit is too high for the sensors I use,
which provides a exposure time of 15,625 us, that I often need to use.

one of my sensor provides an analog gain which can take the values 1, 1.5,
2, 3, 4, 6 and 8 and a digital gain which is a floating point number
with 2 bits for the exponent and 6 bits for the mantissa, giving values
from 1 to (1 + 63/64) * (1, 2, 4 or 8).  I currently combine them and
indicate the unit in the name of the control "gain (64th)", but a more
robust solution would be welcome.

the other sensor provides an analog gain which is expressed in tenth of dB
(cB ?) from 0 to 480. Clearly this is difficult to comunicate to the user app
using the current definition of V4L2_CID_GAIN.  I think I'd need to be able
to express that the gain of this sensor is exponential and that the unit is
0.1 dB.  If we admit that exponential gains will probably be expressed in dB,
that leaves us with the same problem as above : express that the unit is
not 1, but 0.1.  We could also define a V4L2_CID_EXPONENTIAL_GAIN, with unit
0.01 dB.

Best regards

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Sakari Ailus]

Hi Ping-chung,

On Thu, Sep 27, 2018 at 03:19:07AM +0000, Chen, Ping-chung wrote:
> Hi,
> 
> >-----Original Message-----
> >From: Yeh, Andy 
> >Sent: Wednesday, September 26, 2018 11:19 PM
> >To: Sakari Ailus <sakari.ailus@linux.intel.com>; Chen, Ping-chung <ping-chung.chen@intel.com>
> 
> >Hi Sakari, PC,
> 
> >sensors that do need >digital gain applied, too --- assuming it'd be 
> >combined with the TRY_EXT_CTRLS rounding flags.
> >>
> >> There might be many kinds of discrete DG formats. For imx208, DG=2^n, 
> >> but for other sensors, DG could be 2*n, 5*n, or other styles. If HAL 
> >> needs to
> >
> >I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
> >multiplying the value by a 16-bit number with a 8-bit fractional part. 
> >The
> >imx208 apparently lacks the multiplication and only has the bit shift.
> >
> >Usually there's some sort of technical reason for the choice of the 
> >digital gain implementation and therefore I expect at least the vast 
> >majority of the implementations to be either of the two.
> 
> >We shall ensure the expansibility of this architecture to include other kind of styles in the future. Is this API design architecture-wise ok?
> 
> Indeed. Seems it is hard to cover all rules and HAL needs complex flow to
> judge the DG value. Hi Sakari, could you provide an example that how HAL
> uses the modified interface to set available digital gain?

It'll require more user space code no matter how you'd implement this.

Thinking this again, I don't think you'd be doing harm by resorting to an
integer menu here. It'll take some more time to get a decent API to provide
information on the units etc. to the user space.

> >> cover all cases, kernel will have to update more information to this 
> >> control. Another problem is should HAL take over the SMIA calculation?
> >> If so, kernel will also need to update SMIA parameters to user space 
> >> (or create an addition filed for SMIA in the configuration XML file).
> >
> >The parameters for the analogue gain model should come from the driver, yes.
> >We do not have controls for that purpose but they can (and should) be added.
> >
> 
> >How about still follow PC's proposal to implement in XML? It was in IQ tuning file before which is in userspace. Even I proposed to PC to study with ICG SW team whether this info could be retrieved from 3A algorithm.
> 
> Hi Andy, because we has to use total gain instead of AG in 3A for the WA, our tuning data of imx208 will not include SMIA of AG anymore. 
> So HAL has no way to retrieve correct SMIA parameters of AG from 3A.

Ideally the driver would be able to provide enough information here to the
user space to work with it. This needs improvement going forward, but in a
way that is generic enough.

-- 
Kind regards.

Sakari Ailus
sakari.ailus@linux.intel.com

RE: [PATCH v5] media: imx208: Add imx208 camera sensor driver

[Tu, ShawnX]

Hi Sakari,

In patch v6, it added the code for DG v4l2 control with integer menu type.

IMX208 only has 5 steps (1x, 2x, 4x, 8x, 16x) of discrete digital gain.
The imx208 driver includes the menu with 5 steps of DG and needs to notify
HAL to select a suitable digital gain value from the menu by 3A result.

Share the chromium link for reference.
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1297101

Thanks,
Shawn
-----Original Message-----
From: Sakari Ailus <sakari.ailus@linux.intel.com> 
Sent: Thursday, October 4, 2018 11:58 PM
To: Chen, Ping-chung <ping-chung.chen@intel.com>
Cc: Yeh, Andy <andy.yeh@intel.com>; Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>; Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Sakari Ailus <sakari.ailus@iki.fi>; tfiga@chromium.org; sylwester.nawrocki@gmail.com; linux-media <linux-media@vger.kernel.org>; Lai, Jim <jim.lai@intel.com>; grundler@chromium.org; Mani, Rajmohan <rajmohan.mani@intel.com>
Subject: Re: [PATCH v5] media: imx208: Add imx208 camera sensor driver

Hi Ping-chung,

On Thu, Sep 27, 2018 at 03:19:07AM +0000, Chen, Ping-chung wrote:
> Hi,
> 
> >-----Original Message-----
> >From: Yeh, Andy
> >Sent: Wednesday, September 26, 2018 11:19 PM
> >To: Sakari Ailus <sakari.ailus@linux.intel.com>; Chen, Ping-chung 
> ><ping-chung.chen@intel.com>
> 
> >Hi Sakari, PC,
> 
> >sensors that do need >digital gain applied, too --- assuming it'd be 
> >combined with the TRY_EXT_CTRLS rounding flags.
> >>
> >> There might be many kinds of discrete DG formats. For imx208, 
> >> DG=2^n, but for other sensors, DG could be 2*n, 5*n, or other 
> >> styles. If HAL needs to
> >
> >I guess the most common is multiplication and a bit shift (by e.g. 8), e.g.
> >multiplying the value by a 16-bit number with a 8-bit fractional part. 
> >The
> >imx208 apparently lacks the multiplication and only has the bit shift.
> >
> >Usually there's some sort of technical reason for the choice of the 
> >digital gain implementation and therefore I expect at least the vast 
> >majority of the implementations to be either of the two.
> 
> >We shall ensure the expansibility of this architecture to include other kind of styles in the future. Is this API design architecture-wise ok?
> 
> Indeed. Seems it is hard to cover all rules and HAL needs complex flow 
> to judge the DG value. Hi Sakari, could you provide an example that 
> how HAL uses the modified interface to set available digital gain?

It'll require more user space code no matter how you'd implement this.

Thinking this again, I don't think you'd be doing harm by resorting to an integer menu here. It'll take some more time to get a decent API to provide information on the units etc. to the user space.

> >> cover all cases, kernel will have to update more information to 
> >> this control. Another problem is should HAL take over the SMIA calculation?
> >> If so, kernel will also need to update SMIA parameters to user 
> >> space (or create an addition filed for SMIA in the configuration XML file).
> >
> >The parameters for the analogue gain model should come from the driver, yes.
> >We do not have controls for that purpose but they can (and should) be added.
> >
> 
> >How about still follow PC's proposal to implement in XML? It was in IQ tuning file before which is in userspace. Even I proposed to PC to study with ICG SW team whether this info could be retrieved from 3A algorithm.
> 
> Hi Andy, because we has to use total gain instead of AG in 3A for the WA, our tuning data of imx208 will not include SMIA of AG anymore. 
> So HAL has no way to retrieve correct SMIA parameters of AG from 3A.

Ideally the driver would be able to provide enough information here to the user space to work with it. This needs improvement going forward, but in a way that is generic enough.

--
Kind regards.

Sakari Ailus
sakari.ailus@linux.intel.com