[PATCH 0/5] Improvements to IMX290 CMOS driver

[PATCH 0/5] Improvements to IMX290 CMOS driver

[Manivannan Sadhasivam]

Hello,

This patchset adds improvements to the existing IMX290 CMOS driver from
Sony. The major changes are adding 2 lane support, test pattern generation,
RAW12 mode support and configurable link frequency & pixel rate.

The link frequency & pixel rate combinations depend on various factors like
lane count, resolution and image format as per the datasheet.

Thanks,
Mani

Manivannan Sadhasivam (5):
  media: i2c: imx290: Add support for 2 data lanes
  media: i2c: imx290: Add support for test pattern generation
  media: i2c: imx290: Add RAW12 mode support
  media: i2c: imx290: Add support to enumerate all frame sizes
  media: i2c: imx290: Add configurable link frequency and pixel rate

 drivers/media/i2c/imx290.c | 328 +++++++++++++++++++++++++++++++------
 1 file changed, 277 insertions(+), 51 deletions(-)

-- 
2.17.1

[PATCH 1/5] media: i2c: imx290: Add support for 2 data lanes

[Manivannan Sadhasivam]

The IMX290 sensor can output frames with 2/4 CSI2 data lanes. This commit
adds support for 2 lane mode in addition to the 4 lane and also
configuring the data lane settings in the driver based on system
configuration.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 130 ++++++++++++++++++++++++++++++++++---
 1 file changed, 121 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index f7678e5a5d87..1d49910937fb 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -25,7 +25,18 @@
 #define IMX290_STANDBY 0x3000
 #define IMX290_REGHOLD 0x3001
 #define IMX290_XMSTA 0x3002
+#define IMX290_FR_FDG_SEL 0x3009
 #define IMX290_GAIN 0x3014
+#define IMX290_HMAX_LOW 0x301c
+#define IMX290_HMAX_HIGH 0x301d
+#define IMX290_PHY_LANE_NUM 0x3407
+#define IMX290_CSI_LANE_MODE 0x3443
+
+/* HMAX fields */
+#define IMX290_HMAX_2_1920 0x1130
+#define IMX290_HMAX_4_1920 0x0898
+#define IMX290_HMAX_2_720 0x19C8
+#define IMX290_HMAX_4_720 0x0CE4
 
 #define IMX290_DEFAULT_LINK_FREQ 445500000
 
@@ -56,6 +67,7 @@ struct imx290 {
 	struct device *dev;
 	struct clk *xclk;
 	struct regmap *regmap;
+	int nlanes;
 
 	struct v4l2_subdev sd;
 	struct v4l2_fwnode_endpoint ep;
@@ -89,14 +101,11 @@ static const struct regmap_config imx290_regmap_config = {
 
 static const struct imx290_regval imx290_global_init_settings[] = {
 	{ 0x3007, 0x00 },
-	{ 0x3009, 0x00 },
 	{ 0x3018, 0x65 },
 	{ 0x3019, 0x04 },
 	{ 0x301a, 0x00 },
-	{ 0x3443, 0x03 },
 	{ 0x3444, 0x20 },
 	{ 0x3445, 0x25 },
-	{ 0x3407, 0x03 },
 	{ 0x303a, 0x0c },
 	{ 0x3040, 0x00 },
 	{ 0x3041, 0x00 },
@@ -169,7 +178,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
 	{ 0x3164, 0x1a },
 	{ 0x3480, 0x49 },
 	/* data rate settings */
-	{ 0x3009, 0x01 },
 	{ 0x3405, 0x10 },
 	{ 0x3446, 0x57 },
 	{ 0x3447, 0x00 },
@@ -187,8 +195,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
 	{ 0x3453, 0x00 },
 	{ 0x3454, 0x17 },
 	{ 0x3455, 0x00 },
-	{ 0x301c, 0x98 },
-	{ 0x301d, 0x08 },
 };
 
 static const struct imx290_regval imx290_720p_settings[] = {
@@ -210,7 +216,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
 	{ 0x3164, 0x1a },
 	{ 0x3480, 0x49 },
 	/* data rate settings */
-	{ 0x3009, 0x01 },
 	{ 0x3405, 0x10 },
 	{ 0x3446, 0x4f },
 	{ 0x3447, 0x00 },
@@ -228,8 +233,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
 	{ 0x3453, 0x00 },
 	{ 0x3454, 0x17 },
 	{ 0x3455, 0x00 },
-	{ 0x301c, 0xe4 },
-	{ 0x301d, 0x0c },
 };
 
 static const struct imx290_regval imx290_10bit_settings[] = {
@@ -522,6 +525,25 @@ static int imx290_write_current_format(struct imx290 *imx290,
 	return 0;
 }
 
+static int imx290_set_hmax(struct imx290 *imx290, u32 val)
+{
+	int ret;
+
+	ret = imx290_write_reg(imx290, IMX290_HMAX_LOW, (val & 0xff));
+	if (ret) {
+		dev_err(imx290->dev, "Error setting HMAX register\n");
+		return ret;
+	}
+
+	ret = imx290_write_reg(imx290, IMX290_HMAX_HIGH, ((val >> 8) & 0xff));
+	if (ret) {
+		dev_err(imx290->dev, "Error setting HMAX register\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 /* Start streaming */
 static int imx290_start_streaming(struct imx290 *imx290)
 {
@@ -551,6 +573,40 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
+	switch (imx290->nlanes) {
+	case 2:
+		if (imx290->current_mode->width == 1920) {
+			ret = imx290_set_hmax(imx290, IMX290_HMAX_2_1920);
+			if (ret < 0)
+				return ret;
+		} else {
+			ret = imx290_set_hmax(imx290, IMX290_HMAX_2_720);
+			if (ret < 0)
+				return ret;
+		}
+
+		break;
+	case 4:
+		if (imx290->current_mode->width == 1920) {
+			ret = imx290_set_hmax(imx290, IMX290_HMAX_4_1920);
+			if (ret < 0)
+				return ret;
+		} else {
+			ret = imx290_set_hmax(imx290, IMX290_HMAX_4_720);
+			if (ret < 0)
+				return ret;
+		}
+
+		break;
+	default:
+		/*
+		 * We should never hit this since the data lane count is
+		 * validated in probe itself
+		 */
+		dev_err(imx290->dev, "Lane configuration not supported\n");
+		return -EINVAL;
+	}
+
 	/* Apply customized values from user */
 	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
 	if (ret) {
@@ -607,6 +663,49 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
 				       imx290->supplies);
 }
 
+static int imx290_set_data_lanes(struct imx290 *imx290)
+{
+	int ret = 0, laneval, frsel;
+
+	switch (imx290->nlanes) {
+	case 2:
+		laneval = 0x01;
+		frsel = 0x02;
+		break;
+	case 4:
+		laneval = 0x03;
+		frsel = 0x01;
+		break;
+	default:
+		/*
+		 * We should never hit this since the data lane count is
+		 * validated in probe itself
+		 */
+		dev_err(imx290->dev, "Lane configuration not supported\n");
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
+	if (ret) {
+		dev_err(imx290->dev, "Error setting Physical Lane number register\n");
+		goto exit;
+	}
+
+	ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
+	if (ret) {
+		dev_err(imx290->dev, "Error setting CSI Lane mode register\n");
+		goto exit;
+	}
+
+	ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
+	if (ret)
+		dev_err(imx290->dev, "Error setting FR/FDG SEL register\n");
+
+exit:
+	return ret;
+}
+
 static int imx290_power_on(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
@@ -703,6 +802,16 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_err;
 	}
 
+	/* Get number of data lanes */
+	imx290->nlanes = imx290->ep.bus.mipi_csi2.num_data_lanes;
+	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
+		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
+		ret = -EINVAL;
+		goto free_err;
+	}
+
+	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
+
 	if (!imx290->ep.nr_of_link_frequencies) {
 		dev_err(dev, "link-frequency property not found in DT\n");
 		ret = -EINVAL;
@@ -822,6 +931,9 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_entity;
 	}
 
+	/* Set data lane count */
+	imx290_set_data_lanes(imx290);
+
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	pm_runtime_idle(dev);
-- 
2.17.1

[PATCH 2/5] media: i2c: imx290: Add support for test pattern generation

[Manivannan Sadhasivam]

Add support for generating following test patterns by IMX290:

* Sequence Pattern 1
* Horizontal Color-bar Chart
* Vertical Color-bar Chart
* Sequence Pattern 2
* Gradation Pattern 1
* Gradation Pattern 2
* 000/555h Toggle Pattern

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 41 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 1d49910937fb..e218c959a729 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -26,12 +26,19 @@
 #define IMX290_REGHOLD 0x3001
 #define IMX290_XMSTA 0x3002
 #define IMX290_FR_FDG_SEL 0x3009
+#define IMX290_BLKLEVEL_LOW 0x300a
+#define IMX290_BLKLEVEL_HIGH 0x300b
 #define IMX290_GAIN 0x3014
 #define IMX290_HMAX_LOW 0x301c
 #define IMX290_HMAX_HIGH 0x301d
+#define IMX290_PGCTRL 0x308c
 #define IMX290_PHY_LANE_NUM 0x3407
 #define IMX290_CSI_LANE_MODE 0x3443
 
+#define IMX290_PGCTRL_REGEN BIT(0)
+#define IMX290_PGCTRL_THRU BIT(1)
+#define IMX290_PGCTRL_MODE(n) ((n) << 4)
+
 /* HMAX fields */
 #define IMX290_HMAX_2_1920 0x1130
 #define IMX290_HMAX_4_1920 0x0898
@@ -99,6 +106,17 @@ static const struct regmap_config imx290_regmap_config = {
 	.cache_type = REGCACHE_RBTREE,
 };
 
+static const char * const imx290_test_pattern_menu[] = {
+	"Disabled",
+	"Sequence Pattern 1",
+	"Horizontal Color-bar Chart",
+	"Vertical Color-bar Chart",
+	"Sequence Pattern 2",
+	"Gradation Pattern 1",
+	"Gradation Pattern 2",
+	"000/555h Toggle Pattern",
+};
+
 static const struct imx290_regval imx290_global_init_settings[] = {
 	{ 0x3007, 0x00 },
 	{ 0x3018, 0x65 },
@@ -394,6 +412,22 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_GAIN:
 		ret = imx290_set_gain(imx290, ctrl->val);
 		break;
+	case V4L2_CID_TEST_PATTERN:
+		if (ctrl->val) {
+			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x00);
+			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
+			mdelay(10);
+			imx290_write_reg(imx290, IMX290_PGCTRL,
+					 (u8)(IMX290_PGCTRL_REGEN |
+					 IMX290_PGCTRL_THRU |
+					 IMX290_PGCTRL_MODE(ctrl->val)));
+		} else {
+			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
+			mdelay(10);
+			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
+			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -878,7 +912,7 @@ static int imx290_probe(struct i2c_client *client)
 
 	mutex_init(&imx290->lock);
 
-	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
+	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
 
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_GAIN, 0, 72, 1, 0);
@@ -896,6 +930,11 @@ static int imx290_probe(struct i2c_client *client)
 					       INT_MAX, 1,
 					       imx290_modes[0].pixel_rate);
 
+	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
+				     0, 0, imx290_test_pattern_menu);
+
 	imx290->sd.ctrl_handler = &imx290->ctrls;
 
 	if (imx290->ctrls.error) {
-- 
2.17.1

[PATCH 3/5] media: i2c: imx290: Add RAW12 mode support

[Manivannan Sadhasivam]

IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and
12 bit formats. Since the driver already supports RAW10 mode, let's add
the missing RAW12 mode as well.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index e218c959a729..d5bb3a59ac46 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -75,6 +75,7 @@ struct imx290 {
 	struct clk *xclk;
 	struct regmap *regmap;
 	int nlanes;
+	u8 bpp;
 
 	struct v4l2_subdev sd;
 	struct v4l2_fwnode_endpoint ep;
@@ -98,6 +99,7 @@ struct imx290_pixfmt {
 
 static const struct imx290_pixfmt imx290_formats[] = {
 	{ MEDIA_BUS_FMT_SRGGB10_1X10 },
+	{ MEDIA_BUS_FMT_SRGGB12_1X12 },
 };
 
 static const struct regmap_config imx290_regmap_config = {
@@ -265,6 +267,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {
 	{ 0x300b, 0x00},
 };
 
+static const struct imx290_regval imx290_12bit_settings[] = {
+	{ 0x3005, 0x01 },
+	{ 0x3046, 0x01 },
+	{ 0x3129, 0x00 },
+	{ 0x317c, 0x00 },
+	{ 0x31ec, 0x0e },
+	{ 0x3441, 0x0c },
+	{ 0x3442, 0x0c },
+	{ 0x300a, 0xf0 },
+	{ 0x300b, 0x00 },
+};
+
 /* supported link frequencies */
 static const s64 imx290_link_freq[] = {
 	IMX290_DEFAULT_LINK_FREQ,
@@ -550,6 +564,21 @@ static int imx290_write_current_format(struct imx290 *imx290,
 			dev_err(imx290->dev, "Could not set format registers\n");
 			return ret;
 		}
+
+		imx290->bpp = 10;
+
+		break;
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
+						ARRAY_SIZE(
+							imx290_12bit_settings));
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set format registers\n");
+			return ret;
+		}
+
+		imx290->bpp = 12;
+
 		break;
 	default:
 		dev_err(imx290->dev, "Unknown pixel format\n");
@@ -910,6 +939,9 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_err;
 	}
 
+	/* Default bits per pixel value */
+	imx290->bpp = 10;
+
 	mutex_init(&imx290->lock);
 
 	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
-- 
2.17.1

[PATCH 4/5] media: i2c: imx290: Add support to enumerate all frame sizes

[Manivannan Sadhasivam]

Add support to enumerate all frame sizes supported by IMX290. This is
required for using with userspace tools such as libcamera.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index d5bb3a59ac46..f26c4a0ee0a0 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -468,6 +468,25 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int imx290_enum_frame_size(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	if ((fse->code != imx290_formats[0].code) &&
+	    (fse->code != imx290_formats[1].code))
+		return -EINVAL;
+
+	if (fse->index >= ARRAY_SIZE(imx290_modes))
+		return -EINVAL;
+
+	fse->min_width = imx290_modes[fse->index].width;
+	fse->max_width = imx290_modes[fse->index].width;
+	fse->min_height = imx290_modes[fse->index].height;
+	fse->max_height = imx290_modes[fse->index].height;
+
+	return 0;
+}
+
 static int imx290_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_pad_config *cfg,
 			  struct v4l2_subdev_format *fmt)
@@ -820,6 +839,7 @@ static const struct v4l2_subdev_video_ops imx290_video_ops = {
 static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
 	.init_cfg = imx290_entity_init_cfg,
 	.enum_mbus_code = imx290_enum_mbus_code,
+	.enum_frame_size = imx290_enum_frame_size,
 	.get_fmt = imx290_get_fmt,
 	.set_fmt = imx290_set_fmt,
 };
-- 
2.17.1

[PATCH 5/5] media: i2c: imx290: Add configurable link frequency and pixel rate

[Manivannan Sadhasivam]

IMX290 operates with multiple link frequency and pixel rate combinations.
The initial driver used a single setting for both but since we now have
the lane count support in place, let's add configurable link frequency
and pixel rate.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 155 +++++++++++++++++++++----------------
 1 file changed, 89 insertions(+), 66 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index f26c4a0ee0a0..d794ade26609 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -45,8 +45,6 @@
 #define IMX290_HMAX_2_720 0x19C8
 #define IMX290_HMAX_4_720 0x0CE4
 
-#define IMX290_DEFAULT_LINK_FREQ 445500000
-
 static const char * const imx290_supply_name[] = {
 	"vdda",
 	"vddd",
@@ -63,8 +61,6 @@ struct imx290_regval {
 struct imx290_mode {
 	u32 width;
 	u32 height;
-	u32 pixel_rate;
-	u32 link_freq_index;
 
 	const struct imx290_regval *data;
 	u32 data_size;
@@ -281,7 +277,10 @@ static const struct imx290_regval imx290_12bit_settings[] = {
 
 /* supported link frequencies */
 static const s64 imx290_link_freq[] = {
-	IMX290_DEFAULT_LINK_FREQ,
+	891000000, /* 1920x1080 -  2 lane */
+	445500000, /* 1920x1080 -  4 lane */
+	594000000, /* 1280x720  -  2 lane */
+	297000000, /* 1280x720  -  4 lane */
 };
 
 /* Mode configs */
@@ -291,16 +290,12 @@ static const struct imx290_mode imx290_modes[] = {
 		.height = 1080,
 		.data = imx290_1080p_settings,
 		.data_size = ARRAY_SIZE(imx290_1080p_settings),
-		.pixel_rate = 178200000,
-		.link_freq_index = 0,
 	},
 	{
 		.width = 1280,
 		.height = 720,
 		.data = imx290_720p_settings,
 		.data_size = ARRAY_SIZE(imx290_720p_settings),
-		.pixel_rate = 178200000,
-		.link_freq_index = 0,
 	},
 };
 
@@ -509,6 +504,73 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static s64 imx290_get_link_freq_index(struct imx290 *imx290)
+{
+	const struct imx290_mode *cur_mode = imx290->current_mode;
+	u8 index;
+
+	if (cur_mode->width == 1920)
+		index = imx290->nlanes / 4;
+	else
+		index = (imx290->nlanes / 4) + 2;
+
+	return index;
+}
+
+static s64 imx290_get_link_freq(struct imx290 *imx290)
+{
+	u8 index = imx290_get_link_freq_index(imx290);
+
+	return imx290_link_freq[index];
+}
+
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
+{
+	s64 link_freq = imx290_get_link_freq(imx290);
+	u8 nlanes = imx290->nlanes;
+
+	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+	return (link_freq * 2 * nlanes / imx290->bpp);
+}
+
+static int imx290_write_current_format(struct imx290 *imx290,
+				       struct v4l2_mbus_framefmt *format)
+{
+	int ret;
+
+	switch (format->code) {
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
+						ARRAY_SIZE(
+							imx290_10bit_settings));
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set format registers\n");
+			return ret;
+		}
+
+		imx290->bpp = 10;
+
+		break;
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
+						ARRAY_SIZE(
+							imx290_12bit_settings));
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set format registers\n");
+			return ret;
+		}
+
+		imx290->bpp = 12;
+
+		break;
+	default:
+		dev_err(imx290->dev, "Unknown pixel format\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int imx290_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_pad_config *cfg,
 		      struct v4l2_subdev_format *fmt)
@@ -517,6 +579,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	const struct imx290_mode *mode;
 	struct v4l2_mbus_framefmt *format;
 	unsigned int i;
+	int ret = 0;
 
 	mutex_lock(&imx290->lock);
 
@@ -542,17 +605,27 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 		format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
 	} else {
 		format = &imx290->current_format;
-		__v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
-		__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, mode->pixel_rate);
-
 		imx290->current_mode = mode;
+
+		/* Set current frame format */
+		ret = imx290_write_current_format(imx290, &fmt->format);
+		if (ret < 0) {
+			dev_err(imx290->dev, "Could not set frame format\n");
+			goto err_out;
+		}
+
+		__v4l2_ctrl_s_ctrl(imx290->link_freq,
+				   imx290_get_link_freq_index(imx290));
+		__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
+					 imx290_calc_pixel_rate(imx290));
 	}
 
 	*format = fmt->format;
 
+err_out:
 	mutex_unlock(&imx290->lock);
 
-	return 0;
+	return ret;
 }
 
 static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
@@ -569,44 +642,6 @@ static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static int imx290_write_current_format(struct imx290 *imx290,
-				       struct v4l2_mbus_framefmt *format)
-{
-	int ret;
-
-	switch (format->code) {
-	case MEDIA_BUS_FMT_SRGGB10_1X10:
-		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
-						ARRAY_SIZE(
-							imx290_10bit_settings));
-		if (ret < 0) {
-			dev_err(imx290->dev, "Could not set format registers\n");
-			return ret;
-		}
-
-		imx290->bpp = 10;
-
-		break;
-	case MEDIA_BUS_FMT_SRGGB12_1X12:
-		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
-						ARRAY_SIZE(
-							imx290_12bit_settings));
-		if (ret < 0) {
-			dev_err(imx290->dev, "Could not set format registers\n");
-			return ret;
-		}
-
-		imx290->bpp = 12;
-
-		break;
-	default:
-		dev_err(imx290->dev, "Unknown pixel format\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int imx290_set_hmax(struct imx290 *imx290, u32 val)
 {
 	int ret;
@@ -640,13 +675,6 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
-	/* Set current frame format */
-	ret = imx290_write_current_format(imx290, &imx290->current_format);
-	if (ret < 0) {
-		dev_err(imx290->dev, "Could not set frame format\n");
-		return ret;
-	}
-
 	/* Apply default values of current mode */
 	ret = imx290_set_register_array(imx290, imx290->current_mode->data,
 					imx290->current_mode->data_size);
@@ -901,12 +929,6 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_err;
 	}
 
-	if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {
-		dev_err(dev, "Unsupported link frequency\n");
-		ret = -EINVAL;
-		goto free_err;
-	}
-
 	/* Only CSI2 is supported for now */
 	if (imx290->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
 		dev_err(dev, "Unsupported bus type, should be CSI2\n");
@@ -973,14 +995,15 @@ static int imx290_probe(struct i2c_client *client)
 				       &imx290_ctrl_ops,
 				       V4L2_CID_LINK_FREQ,
 				       ARRAY_SIZE(imx290_link_freq) - 1,
-				       0, imx290_link_freq);
+				       (imx290->nlanes / 4),
+				       imx290_link_freq);
 	if (imx290->link_freq)
 		imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
 	imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 					       V4L2_CID_PIXEL_RATE, 1,
 					       INT_MAX, 1,
-					       imx290_modes[0].pixel_rate);
+					       imx290_calc_pixel_rate(imx290));
 
 	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
 				     V4L2_CID_TEST_PATTERN,
-- 
2.17.1

Re: [PATCH 3/5] media: i2c: imx290: Add RAW12 mode support

[Fabio Estevam]

Hi Manivannan,

On Fri, Nov 29, 2019 at 4:07 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
               }
> +
> +               imx290->bpp = 10;
> +
> +               break;
> +       case MEDIA_BUS_FMT_SRGGB12_1X12:
> +               ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> +                                               ARRAY_SIZE(
> +                                                       imx290_12bit_settings));

Could you please write the ARRAY_SIZE and its parameter in the same line?

It would improve readability.

Thanks

Re: [PATCH 3/5] media: i2c: imx290: Add RAW12 mode support

[Manivannan Sadhasivam]

Hi Fabio,

On Fri, Nov 29, 2019 at 04:49:25PM -0300, Fabio Estevam wrote:
> Hi Manivannan,
> 
> On Fri, Nov 29, 2019 at 4:07 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
>                }
> > +
> > +               imx290->bpp = 10;
> > +
> > +               break;
> > +       case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +               ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> > +                                               ARRAY_SIZE(
> > +                                                       imx290_12bit_settings));
> 
> Could you please write the ARRAY_SIZE and its parameter in the same line?
> 
> It would improve readability.
> 

I don't favor this change but Sakari did this to supress the checkpatch
warning while applying my initial patch, so now I did this here itself
to maintain the uniformity.

Thanks,
Mani

> Thanks

Re: [PATCH 1/5] media: i2c: imx290: Add support for 2 data lanes

[Sakari Ailus]

Hi Manivannan,

Thanks for the patchset.

On Sat, Nov 30, 2019 at 12:35:37AM +0530, Manivannan Sadhasivam wrote:
> The IMX290 sensor can output frames with 2/4 CSI2 data lanes. This commit
> adds support for 2 lane mode in addition to the 4 lane and also
> configuring the data lane settings in the driver based on system
> configuration.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 130 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 121 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index f7678e5a5d87..1d49910937fb 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -25,7 +25,18 @@
>  #define IMX290_STANDBY 0x3000
>  #define IMX290_REGHOLD 0x3001
>  #define IMX290_XMSTA 0x3002
> +#define IMX290_FR_FDG_SEL 0x3009
>  #define IMX290_GAIN 0x3014
> +#define IMX290_HMAX_LOW 0x301c
> +#define IMX290_HMAX_HIGH 0x301d
> +#define IMX290_PHY_LANE_NUM 0x3407
> +#define IMX290_CSI_LANE_MODE 0x3443
> +
> +/* HMAX fields */
> +#define IMX290_HMAX_2_1920 0x1130
> +#define IMX290_HMAX_4_1920 0x0898
> +#define IMX290_HMAX_2_720 0x19C8
> +#define IMX290_HMAX_4_720 0x0CE4
>  
>  #define IMX290_DEFAULT_LINK_FREQ 445500000
>  
> @@ -56,6 +67,7 @@ struct imx290 {
>  	struct device *dev;
>  	struct clk *xclk;
>  	struct regmap *regmap;
> +	int nlanes;

You're using u8 for another small (unsigned) integer later. How about u8
here?

>  
>  	struct v4l2_subdev sd;
>  	struct v4l2_fwnode_endpoint ep;
> @@ -89,14 +101,11 @@ static const struct regmap_config imx290_regmap_config = {
>  
>  static const struct imx290_regval imx290_global_init_settings[] = {
>  	{ 0x3007, 0x00 },
> -	{ 0x3009, 0x00 },
>  	{ 0x3018, 0x65 },
>  	{ 0x3019, 0x04 },
>  	{ 0x301a, 0x00 },
> -	{ 0x3443, 0x03 },
>  	{ 0x3444, 0x20 },
>  	{ 0x3445, 0x25 },
> -	{ 0x3407, 0x03 },
>  	{ 0x303a, 0x0c },
>  	{ 0x3040, 0x00 },
>  	{ 0x3041, 0x00 },
> @@ -169,7 +178,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
>  	{ 0x3164, 0x1a },
>  	{ 0x3480, 0x49 },
>  	/* data rate settings */
> -	{ 0x3009, 0x01 },
>  	{ 0x3405, 0x10 },
>  	{ 0x3446, 0x57 },
>  	{ 0x3447, 0x00 },
> @@ -187,8 +195,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
>  	{ 0x3453, 0x00 },
>  	{ 0x3454, 0x17 },
>  	{ 0x3455, 0x00 },
> -	{ 0x301c, 0x98 },
> -	{ 0x301d, 0x08 },
>  };
>  
>  static const struct imx290_regval imx290_720p_settings[] = {
> @@ -210,7 +216,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
>  	{ 0x3164, 0x1a },
>  	{ 0x3480, 0x49 },
>  	/* data rate settings */
> -	{ 0x3009, 0x01 },
>  	{ 0x3405, 0x10 },
>  	{ 0x3446, 0x4f },
>  	{ 0x3447, 0x00 },
> @@ -228,8 +233,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
>  	{ 0x3453, 0x00 },
>  	{ 0x3454, 0x17 },
>  	{ 0x3455, 0x00 },
> -	{ 0x301c, 0xe4 },
> -	{ 0x301d, 0x0c },
>  };
>  
>  static const struct imx290_regval imx290_10bit_settings[] = {
> @@ -522,6 +525,25 @@ static int imx290_write_current_format(struct imx290 *imx290,
>  	return 0;
>  }
>  
> +static int imx290_set_hmax(struct imx290 *imx290, u32 val)
> +{
> +	int ret;
> +
> +	ret = imx290_write_reg(imx290, IMX290_HMAX_LOW, (val & 0xff));
> +	if (ret) {
> +		dev_err(imx290->dev, "Error setting HMAX register\n");
> +		return ret;
> +	}
> +
> +	ret = imx290_write_reg(imx290, IMX290_HMAX_HIGH, ((val >> 8) & 0xff));
> +	if (ret) {
> +		dev_err(imx290->dev, "Error setting HMAX register\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /* Start streaming */
>  static int imx290_start_streaming(struct imx290 *imx290)
>  {
> @@ -551,6 +573,40 @@ static int imx290_start_streaming(struct imx290 *imx290)
>  		return ret;
>  	}
>  
> +	switch (imx290->nlanes) {
> +	case 2:
> +		if (imx290->current_mode->width == 1920) {
> +			ret = imx290_set_hmax(imx290, IMX290_HMAX_2_1920);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = imx290_set_hmax(imx290, IMX290_HMAX_2_720);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		break;
> +	case 4:
> +		if (imx290->current_mode->width == 1920) {
> +			ret = imx290_set_hmax(imx290, IMX290_HMAX_4_1920);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = imx290_set_hmax(imx290, IMX290_HMAX_4_720);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		break;
> +	default:
> +		/*
> +		 * We should never hit this since the data lane count is
> +		 * validated in probe itself
> +		 */
> +		dev_err(imx290->dev, "Lane configuration not supported\n");
> +		return -EINVAL;
> +	}
> +
>  	/* Apply customized values from user */
>  	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
>  	if (ret) {
> @@ -607,6 +663,49 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
>  				       imx290->supplies);
>  }
>  
> +static int imx290_set_data_lanes(struct imx290 *imx290)
> +{
> +	int ret = 0, laneval, frsel;
> +
> +	switch (imx290->nlanes) {
> +	case 2:
> +		laneval = 0x01;
> +		frsel = 0x02;
> +		break;
> +	case 4:
> +		laneval = 0x03;
> +		frsel = 0x01;
> +		break;
> +	default:
> +		/*
> +		 * We should never hit this since the data lane count is
> +		 * validated in probe itself
> +		 */
> +		dev_err(imx290->dev, "Lane configuration not supported\n");
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
> +	if (ret) {
> +		dev_err(imx290->dev, "Error setting Physical Lane number register\n");
> +		goto exit;
> +	}
> +
> +	ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
> +	if (ret) {
> +		dev_err(imx290->dev, "Error setting CSI Lane mode register\n");
> +		goto exit;
> +	}
> +
> +	ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
> +	if (ret)
> +		dev_err(imx290->dev, "Error setting FR/FDG SEL register\n");
> +
> +exit:
> +	return ret;
> +}
> +
>  static int imx290_power_on(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> @@ -703,6 +802,16 @@ static int imx290_probe(struct i2c_client *client)
>  		goto free_err;
>  	}
>  
> +	/* Get number of data lanes */
> +	imx290->nlanes = imx290->ep.bus.mipi_csi2.num_data_lanes;
> +	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
> +		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
> +		ret = -EINVAL;
> +		goto free_err;
> +	}
> +
> +	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
> +
>  	if (!imx290->ep.nr_of_link_frequencies) {
>  		dev_err(dev, "link-frequency property not found in DT\n");
>  		ret = -EINVAL;
> @@ -822,6 +931,9 @@ static int imx290_probe(struct i2c_client *client)
>  		goto free_entity;
>  	}
>  
> +	/* Set data lane count */
> +	imx290_set_data_lanes(imx290);

The sensor is likely (but not necessarily) about to be powered off here.
Wouldn't this also belong to be put to the power on sequence?

> +
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
>  	pm_runtime_idle(dev);

-- 
Regards,

Sakari Ailus

Re: [PATCH 2/5] media: i2c: imx290: Add support for test pattern generation

[Sakari Ailus]

Hi Manivannan,

On Sat, Nov 30, 2019 at 12:35:38AM +0530, Manivannan Sadhasivam wrote:
> Add support for generating following test patterns by IMX290:
> 
> * Sequence Pattern 1
> * Horizontal Color-bar Chart
> * Vertical Color-bar Chart
> * Sequence Pattern 2
> * Gradation Pattern 1
> * Gradation Pattern 2
> * 000/555h Toggle Pattern
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 41 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 1d49910937fb..e218c959a729 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -26,12 +26,19 @@
>  #define IMX290_REGHOLD 0x3001
>  #define IMX290_XMSTA 0x3002
>  #define IMX290_FR_FDG_SEL 0x3009
> +#define IMX290_BLKLEVEL_LOW 0x300a
> +#define IMX290_BLKLEVEL_HIGH 0x300b
>  #define IMX290_GAIN 0x3014
>  #define IMX290_HMAX_LOW 0x301c
>  #define IMX290_HMAX_HIGH 0x301d
> +#define IMX290_PGCTRL 0x308c
>  #define IMX290_PHY_LANE_NUM 0x3407
>  #define IMX290_CSI_LANE_MODE 0x3443
>  
> +#define IMX290_PGCTRL_REGEN BIT(0)
> +#define IMX290_PGCTRL_THRU BIT(1)
> +#define IMX290_PGCTRL_MODE(n) ((n) << 4)
> +
>  /* HMAX fields */
>  #define IMX290_HMAX_2_1920 0x1130
>  #define IMX290_HMAX_4_1920 0x0898
> @@ -99,6 +106,17 @@ static const struct regmap_config imx290_regmap_config = {
>  	.cache_type = REGCACHE_RBTREE,
>  };
>  
> +static const char * const imx290_test_pattern_menu[] = {
> +	"Disabled",
> +	"Sequence Pattern 1",
> +	"Horizontal Color-bar Chart",
> +	"Vertical Color-bar Chart",
> +	"Sequence Pattern 2",
> +	"Gradation Pattern 1",
> +	"Gradation Pattern 2",
> +	"000/555h Toggle Pattern",
> +};
> +
>  static const struct imx290_regval imx290_global_init_settings[] = {
>  	{ 0x3007, 0x00 },
>  	{ 0x3018, 0x65 },
> @@ -394,6 +412,22 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>  	case V4L2_CID_GAIN:
>  		ret = imx290_set_gain(imx290, ctrl->val);
>  		break;
> +	case V4L2_CID_TEST_PATTERN:
> +		if (ctrl->val) {
> +			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x00);
> +			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
> +			mdelay(10);

Any particular reason for a busy loop instead of sleeping? Same below.

> +			imx290_write_reg(imx290, IMX290_PGCTRL,
> +					 (u8)(IMX290_PGCTRL_REGEN |
> +					 IMX290_PGCTRL_THRU |
> +					 IMX290_PGCTRL_MODE(ctrl->val)));
> +		} else {
> +			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
> +			mdelay(10);
> +			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
> +			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
> +		}
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -878,7 +912,7 @@ static int imx290_probe(struct i2c_client *client)
>  
>  	mutex_init(&imx290->lock);
>  
> -	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
> +	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
>  
>  	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  			  V4L2_CID_GAIN, 0, 72, 1, 0);
> @@ -896,6 +930,11 @@ static int imx290_probe(struct i2c_client *client)
>  					       INT_MAX, 1,
>  					       imx290_modes[0].pixel_rate);
>  
> +	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
> +				     0, 0, imx290_test_pattern_menu);
> +
>  	imx290->sd.ctrl_handler = &imx290->ctrls;
>  
>  	if (imx290->ctrls.error) {

-- 
Sakari Ailus

Re: [PATCH 3/5] media: i2c: imx290: Add RAW12 mode support

[Sakari Ailus]

Hi Manivannan,

On Sat, Nov 30, 2019 at 12:35:39AM +0530, Manivannan Sadhasivam wrote:
> IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and
> 12 bit formats. Since the driver already supports RAW10 mode, let's add
> the missing RAW12 mode as well.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index e218c959a729..d5bb3a59ac46 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -75,6 +75,7 @@ struct imx290 {
>  	struct clk *xclk;
>  	struct regmap *regmap;
>  	int nlanes;
> +	u8 bpp;
>  
>  	struct v4l2_subdev sd;
>  	struct v4l2_fwnode_endpoint ep;
> @@ -98,6 +99,7 @@ struct imx290_pixfmt {
>  
>  static const struct imx290_pixfmt imx290_formats[] = {
>  	{ MEDIA_BUS_FMT_SRGGB10_1X10 },
> +	{ MEDIA_BUS_FMT_SRGGB12_1X12 },
>  };
>  
>  static const struct regmap_config imx290_regmap_config = {
> @@ -265,6 +267,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {
>  	{ 0x300b, 0x00},
>  };
>  
> +static const struct imx290_regval imx290_12bit_settings[] = {
> +	{ 0x3005, 0x01 },
> +	{ 0x3046, 0x01 },
> +	{ 0x3129, 0x00 },
> +	{ 0x317c, 0x00 },
> +	{ 0x31ec, 0x0e },
> +	{ 0x3441, 0x0c },
> +	{ 0x3442, 0x0c },
> +	{ 0x300a, 0xf0 },
> +	{ 0x300b, 0x00 },
> +};
> +
>  /* supported link frequencies */
>  static const s64 imx290_link_freq[] = {
>  	IMX290_DEFAULT_LINK_FREQ,
> @@ -550,6 +564,21 @@ static int imx290_write_current_format(struct imx290 *imx290,
>  			dev_err(imx290->dev, "Could not set format registers\n");
>  			return ret;
>  		}
> +
> +		imx290->bpp = 10;
> +
> +		break;
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> +						ARRAY_SIZE(
> +							imx290_12bit_settings));
> +		if (ret < 0) {
> +			dev_err(imx290->dev, "Could not set format registers\n");
> +			return ret;
> +		}
> +
> +		imx290->bpp = 12;
> +
>  		break;
>  	default:
>  		dev_err(imx290->dev, "Unknown pixel format\n");
> @@ -910,6 +939,9 @@ static int imx290_probe(struct i2c_client *client)
>  		goto free_err;
>  	}
>  
> +	/* Default bits per pixel value */
> +	imx290->bpp = 10;

Where is the format being initialised at the moment? Nowhere?

If that is the case, I think it should be fixed before this patch.

> +
>  	mutex_init(&imx290->lock);
>  
>  	v4l2_ctrl_handler_init(&imx290->ctrls, 4);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 4/5] media: i2c: imx290: Add support to enumerate all frame sizes

[Sakari Ailus]

On Sat, Nov 30, 2019 at 12:35:40AM +0530, Manivannan Sadhasivam wrote:
> Add support to enumerate all frame sizes supported by IMX290. This is
> required for using with userspace tools such as libcamera.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index d5bb3a59ac46..f26c4a0ee0a0 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -468,6 +468,25 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int imx290_enum_frame_size(struct v4l2_subdev *subdev,
> +				  struct v4l2_subdev_pad_config *cfg,
> +				  struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	if ((fse->code != imx290_formats[0].code) &&
> +	    (fse->code != imx290_formats[1].code))

Please use a loop over imx290_formats instead.

> +		return -EINVAL;
> +
> +	if (fse->index >= ARRAY_SIZE(imx290_modes))
> +		return -EINVAL;
> +
> +	fse->min_width = imx290_modes[fse->index].width;
> +	fse->max_width = imx290_modes[fse->index].width;
> +	fse->min_height = imx290_modes[fse->index].height;
> +	fse->max_height = imx290_modes[fse->index].height;
> +
> +	return 0;
> +}
> +
>  static int imx290_get_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_pad_config *cfg,
>  			  struct v4l2_subdev_format *fmt)
> @@ -820,6 +839,7 @@ static const struct v4l2_subdev_video_ops imx290_video_ops = {
>  static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
>  	.init_cfg = imx290_entity_init_cfg,
>  	.enum_mbus_code = imx290_enum_mbus_code,
> +	.enum_frame_size = imx290_enum_frame_size,
>  	.get_fmt = imx290_get_fmt,
>  	.set_fmt = imx290_set_fmt,
>  };

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/5] media: i2c: imx290: Add support for 2 data lanes

[Manivannan Sadhasivam]

Hi Sakari,

On Tue, Dec 03, 2019 at 10:45:20AM +0200, Sakari Ailus wrote:
> Hi Manivannan,
> 
> Thanks for the patchset.
> 
> On Sat, Nov 30, 2019 at 12:35:37AM +0530, Manivannan Sadhasivam wrote:
> > The IMX290 sensor can output frames with 2/4 CSI2 data lanes. This commit
> > adds support for 2 lane mode in addition to the 4 lane and also
> > configuring the data lane settings in the driver based on system
> > configuration.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/media/i2c/imx290.c | 130 ++++++++++++++++++++++++++++++++++---
> >  1 file changed, 121 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index f7678e5a5d87..1d49910937fb 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -25,7 +25,18 @@
> >  #define IMX290_STANDBY 0x3000
> >  #define IMX290_REGHOLD 0x3001
> >  #define IMX290_XMSTA 0x3002
> > +#define IMX290_FR_FDG_SEL 0x3009
> >  #define IMX290_GAIN 0x3014
> > +#define IMX290_HMAX_LOW 0x301c
> > +#define IMX290_HMAX_HIGH 0x301d
> > +#define IMX290_PHY_LANE_NUM 0x3407
> > +#define IMX290_CSI_LANE_MODE 0x3443
> > +
> > +/* HMAX fields */
> > +#define IMX290_HMAX_2_1920 0x1130
> > +#define IMX290_HMAX_4_1920 0x0898
> > +#define IMX290_HMAX_2_720 0x19C8
> > +#define IMX290_HMAX_4_720 0x0CE4
> >  
> >  #define IMX290_DEFAULT_LINK_FREQ 445500000
> >  
> > @@ -56,6 +67,7 @@ struct imx290 {
> >  	struct device *dev;
> >  	struct clk *xclk;
> >  	struct regmap *regmap;
> > +	int nlanes;
> 
> You're using u8 for another small (unsigned) integer later. How about u8
> here?
> 

Yeah, this is a left over. I was initially trying to use some error
checking with this! Will change it to u8.

> >  
> >  	struct v4l2_subdev sd;
> >  	struct v4l2_fwnode_endpoint ep;
> > @@ -89,14 +101,11 @@ static const struct regmap_config imx290_regmap_config = {
> >  
> >  static const struct imx290_regval imx290_global_init_settings[] = {
> >  	{ 0x3007, 0x00 },
> > -	{ 0x3009, 0x00 },
> >  	{ 0x3018, 0x65 },
> >  	{ 0x3019, 0x04 },
> >  	{ 0x301a, 0x00 },
> > -	{ 0x3443, 0x03 },
> >  	{ 0x3444, 0x20 },
> >  	{ 0x3445, 0x25 },
> > -	{ 0x3407, 0x03 },
> >  	{ 0x303a, 0x0c },
> >  	{ 0x3040, 0x00 },
> >  	{ 0x3041, 0x00 },
> > @@ -169,7 +178,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
> >  	{ 0x3164, 0x1a },
> >  	{ 0x3480, 0x49 },
> >  	/* data rate settings */
> > -	{ 0x3009, 0x01 },
> >  	{ 0x3405, 0x10 },
> >  	{ 0x3446, 0x57 },
> >  	{ 0x3447, 0x00 },
> > @@ -187,8 +195,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
> >  	{ 0x3453, 0x00 },
> >  	{ 0x3454, 0x17 },
> >  	{ 0x3455, 0x00 },
> > -	{ 0x301c, 0x98 },
> > -	{ 0x301d, 0x08 },
> >  };
> >  
> >  static const struct imx290_regval imx290_720p_settings[] = {
> > @@ -210,7 +216,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
> >  	{ 0x3164, 0x1a },
> >  	{ 0x3480, 0x49 },
> >  	/* data rate settings */
> > -	{ 0x3009, 0x01 },
> >  	{ 0x3405, 0x10 },
> >  	{ 0x3446, 0x4f },
> >  	{ 0x3447, 0x00 },
> > @@ -228,8 +233,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
> >  	{ 0x3453, 0x00 },
> >  	{ 0x3454, 0x17 },
> >  	{ 0x3455, 0x00 },
> > -	{ 0x301c, 0xe4 },
> > -	{ 0x301d, 0x0c },
> >  };
> >  
> >  static const struct imx290_regval imx290_10bit_settings[] = {
> > @@ -522,6 +525,25 @@ static int imx290_write_current_format(struct imx290 *imx290,
> >  	return 0;
> >  }
> >  
> > +static int imx290_set_hmax(struct imx290 *imx290, u32 val)
> > +{
> > +	int ret;
> > +
> > +	ret = imx290_write_reg(imx290, IMX290_HMAX_LOW, (val & 0xff));
> > +	if (ret) {
> > +		dev_err(imx290->dev, "Error setting HMAX register\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = imx290_write_reg(imx290, IMX290_HMAX_HIGH, ((val >> 8) & 0xff));
> > +	if (ret) {
> > +		dev_err(imx290->dev, "Error setting HMAX register\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /* Start streaming */
> >  static int imx290_start_streaming(struct imx290 *imx290)
> >  {
> > @@ -551,6 +573,40 @@ static int imx290_start_streaming(struct imx290 *imx290)
> >  		return ret;
> >  	}
> >  
> > +	switch (imx290->nlanes) {
> > +	case 2:
> > +		if (imx290->current_mode->width == 1920) {
> > +			ret = imx290_set_hmax(imx290, IMX290_HMAX_2_1920);
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			ret = imx290_set_hmax(imx290, IMX290_HMAX_2_720);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		break;
> > +	case 4:
> > +		if (imx290->current_mode->width == 1920) {
> > +			ret = imx290_set_hmax(imx290, IMX290_HMAX_4_1920);
> > +			if (ret < 0)
> > +				return ret;
> > +		} else {
> > +			ret = imx290_set_hmax(imx290, IMX290_HMAX_4_720);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		break;
> > +	default:
> > +		/*
> > +		 * We should never hit this since the data lane count is
> > +		 * validated in probe itself
> > +		 */
> > +		dev_err(imx290->dev, "Lane configuration not supported\n");
> > +		return -EINVAL;
> > +	}
> > +
> >  	/* Apply customized values from user */
> >  	ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
> >  	if (ret) {
> > @@ -607,6 +663,49 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
> >  				       imx290->supplies);
> >  }
> >  
> > +static int imx290_set_data_lanes(struct imx290 *imx290)
> > +{
> > +	int ret = 0, laneval, frsel;
> > +
> > +	switch (imx290->nlanes) {
> > +	case 2:
> > +		laneval = 0x01;
> > +		frsel = 0x02;
> > +		break;
> > +	case 4:
> > +		laneval = 0x03;
> > +		frsel = 0x01;
> > +		break;
> > +	default:
> > +		/*
> > +		 * We should never hit this since the data lane count is
> > +		 * validated in probe itself
> > +		 */
> > +		dev_err(imx290->dev, "Lane configuration not supported\n");
> > +		ret = -EINVAL;
> > +		goto exit;
> > +	}
> > +
> > +	ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
> > +	if (ret) {
> > +		dev_err(imx290->dev, "Error setting Physical Lane number register\n");
> > +		goto exit;
> > +	}
> > +
> > +	ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
> > +	if (ret) {
> > +		dev_err(imx290->dev, "Error setting CSI Lane mode register\n");
> > +		goto exit;
> > +	}
> > +
> > +	ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
> > +	if (ret)
> > +		dev_err(imx290->dev, "Error setting FR/FDG SEL register\n");
> > +
> > +exit:
> > +	return ret;
> > +}
> > +
> >  static int imx290_power_on(struct device *dev)
> >  {
> >  	struct i2c_client *client = to_i2c_client(dev);
> > @@ -703,6 +802,16 @@ static int imx290_probe(struct i2c_client *client)
> >  		goto free_err;
> >  	}
> >  
> > +	/* Get number of data lanes */
> > +	imx290->nlanes = imx290->ep.bus.mipi_csi2.num_data_lanes;
> > +	if (imx290->nlanes != 2 && imx290->nlanes != 4) {
> > +		dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
> > +		ret = -EINVAL;
> > +		goto free_err;
> > +	}
> > +
> > +	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
> > +
> >  	if (!imx290->ep.nr_of_link_frequencies) {
> >  		dev_err(dev, "link-frequency property not found in DT\n");
> >  		ret = -EINVAL;
> > @@ -822,6 +931,9 @@ static int imx290_probe(struct i2c_client *client)
> >  		goto free_entity;
> >  	}
> >  
> > +	/* Set data lane count */
> > +	imx290_set_data_lanes(imx290);
> 
> The sensor is likely (but not necessarily) about to be powered off here.
> Wouldn't this also belong to be put to the power on sequence?
> 

Agree, will add.

Thanks,
Mani

> > +
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> >  	pm_runtime_idle(dev);
> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH 2/5] media: i2c: imx290: Add support for test pattern generation

[Manivannan Sadhasivam]

Hi Sakari,

On Tue, Dec 03, 2019 at 10:48:50AM +0200, Sakari Ailus wrote:
> Hi Manivannan,
> 
> On Sat, Nov 30, 2019 at 12:35:38AM +0530, Manivannan Sadhasivam wrote:
> > Add support for generating following test patterns by IMX290:
> > 
> > * Sequence Pattern 1
> > * Horizontal Color-bar Chart
> > * Vertical Color-bar Chart
> > * Sequence Pattern 2
> > * Gradation Pattern 1
> > * Gradation Pattern 2
> > * 000/555h Toggle Pattern
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/media/i2c/imx290.c | 41 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 1d49910937fb..e218c959a729 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -26,12 +26,19 @@
> >  #define IMX290_REGHOLD 0x3001
> >  #define IMX290_XMSTA 0x3002
> >  #define IMX290_FR_FDG_SEL 0x3009
> > +#define IMX290_BLKLEVEL_LOW 0x300a
> > +#define IMX290_BLKLEVEL_HIGH 0x300b
> >  #define IMX290_GAIN 0x3014
> >  #define IMX290_HMAX_LOW 0x301c
> >  #define IMX290_HMAX_HIGH 0x301d
> > +#define IMX290_PGCTRL 0x308c
> >  #define IMX290_PHY_LANE_NUM 0x3407
> >  #define IMX290_CSI_LANE_MODE 0x3443
> >  
> > +#define IMX290_PGCTRL_REGEN BIT(0)
> > +#define IMX290_PGCTRL_THRU BIT(1)
> > +#define IMX290_PGCTRL_MODE(n) ((n) << 4)
> > +
> >  /* HMAX fields */
> >  #define IMX290_HMAX_2_1920 0x1130
> >  #define IMX290_HMAX_4_1920 0x0898
> > @@ -99,6 +106,17 @@ static const struct regmap_config imx290_regmap_config = {
> >  	.cache_type = REGCACHE_RBTREE,
> >  };
> >  
> > +static const char * const imx290_test_pattern_menu[] = {
> > +	"Disabled",
> > +	"Sequence Pattern 1",
> > +	"Horizontal Color-bar Chart",
> > +	"Vertical Color-bar Chart",
> > +	"Sequence Pattern 2",
> > +	"Gradation Pattern 1",
> > +	"Gradation Pattern 2",
> > +	"000/555h Toggle Pattern",
> > +};
> > +
> >  static const struct imx290_regval imx290_global_init_settings[] = {
> >  	{ 0x3007, 0x00 },
> >  	{ 0x3018, 0x65 },
> > @@ -394,6 +412,22 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> >  	case V4L2_CID_GAIN:
> >  		ret = imx290_set_gain(imx290, ctrl->val);
> >  		break;
> > +	case V4L2_CID_TEST_PATTERN:
> > +		if (ctrl->val) {
> > +			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x00);
> > +			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
> > +			mdelay(10);
> 
> Any particular reason for a busy loop instead of sleeping? Same below.
> 

Nothing. I should've used msleep() here... Will change it.

Thanks,
Mani

> > +			imx290_write_reg(imx290, IMX290_PGCTRL,
> > +					 (u8)(IMX290_PGCTRL_REGEN |
> > +					 IMX290_PGCTRL_THRU |
> > +					 IMX290_PGCTRL_MODE(ctrl->val)));
> > +		} else {
> > +			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
> > +			mdelay(10);
> > +			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
> > +			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
> > +		}
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  		break;
> > @@ -878,7 +912,7 @@ static int imx290_probe(struct i2c_client *client)
> >  
> >  	mutex_init(&imx290->lock);
> >  
> > -	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
> > +	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
> >  
> >  	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> >  			  V4L2_CID_GAIN, 0, 72, 1, 0);
> > @@ -896,6 +930,11 @@ static int imx290_probe(struct i2c_client *client)
> >  					       INT_MAX, 1,
> >  					       imx290_modes[0].pixel_rate);
> >  
> > +	v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
> > +				     V4L2_CID_TEST_PATTERN,
> > +				     ARRAY_SIZE(imx290_test_pattern_menu) - 1,
> > +				     0, 0, imx290_test_pattern_menu);
> > +
> >  	imx290->sd.ctrl_handler = &imx290->ctrls;
> >  
> >  	if (imx290->ctrls.error) {
> 
> -- 
> Sakari Ailus

Re: [PATCH 3/5] media: i2c: imx290: Add RAW12 mode support

[Manivannan Sadhasivam]

Hi Sakari,

On Tue, Dec 03, 2019 at 10:54:17AM +0200, Sakari Ailus wrote:
> Hi Manivannan,
> 
> On Sat, Nov 30, 2019 at 12:35:39AM +0530, Manivannan Sadhasivam wrote:
> > IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and
> > 12 bit formats. Since the driver already supports RAW10 mode, let's add
> > the missing RAW12 mode as well.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index e218c959a729..d5bb3a59ac46 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -75,6 +75,7 @@ struct imx290 {
> >  	struct clk *xclk;
> >  	struct regmap *regmap;
> >  	int nlanes;
> > +	u8 bpp;
> >  
> >  	struct v4l2_subdev sd;
> >  	struct v4l2_fwnode_endpoint ep;
> > @@ -98,6 +99,7 @@ struct imx290_pixfmt {
> >  
> >  static const struct imx290_pixfmt imx290_formats[] = {
> >  	{ MEDIA_BUS_FMT_SRGGB10_1X10 },
> > +	{ MEDIA_BUS_FMT_SRGGB12_1X12 },
> >  };
> >  
> >  static const struct regmap_config imx290_regmap_config = {
> > @@ -265,6 +267,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {
> >  	{ 0x300b, 0x00},
> >  };
> >  
> > +static const struct imx290_regval imx290_12bit_settings[] = {
> > +	{ 0x3005, 0x01 },
> > +	{ 0x3046, 0x01 },
> > +	{ 0x3129, 0x00 },
> > +	{ 0x317c, 0x00 },
> > +	{ 0x31ec, 0x0e },
> > +	{ 0x3441, 0x0c },
> > +	{ 0x3442, 0x0c },
> > +	{ 0x300a, 0xf0 },
> > +	{ 0x300b, 0x00 },
> > +};
> > +
> >  /* supported link frequencies */
> >  static const s64 imx290_link_freq[] = {
> >  	IMX290_DEFAULT_LINK_FREQ,
> > @@ -550,6 +564,21 @@ static int imx290_write_current_format(struct imx290 *imx290,
> >  			dev_err(imx290->dev, "Could not set format registers\n");
> >  			return ret;
> >  		}
> > +
> > +		imx290->bpp = 10;
> > +
> > +		break;
> > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> > +						ARRAY_SIZE(
> > +							imx290_12bit_settings));
> > +		if (ret < 0) {
> > +			dev_err(imx290->dev, "Could not set format registers\n");
> > +			return ret;
> > +		}
> > +
> > +		imx290->bpp = 12;
> > +
> >  		break;
> >  	default:
> >  		dev_err(imx290->dev, "Unknown pixel format\n");
> > @@ -910,6 +939,9 @@ static int imx290_probe(struct i2c_client *client)
> >  		goto free_err;
> >  	}
> >  
> > +	/* Default bits per pixel value */
> > +	imx290->bpp = 10;
> 
> Where is the format being initialised at the moment? Nowhere?
> 
> If that is the case, I think it should be fixed before this patch.
> 

Sorry, I don't quite understand what you're suggesting here. The bpp
is initialised because that's the default bit value at power up and
this value is being used below in imx290_calc_pixel_rate(). I'm not sure
why we need to initialise the format before set_fmt!

Thanks,
Mani

> > +
> >  	mutex_init(&imx290->lock);
> >  
> >  	v4l2_ctrl_handler_init(&imx290->ctrls, 4);
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

Re: [PATCH 4/5] media: i2c: imx290: Add support to enumerate all frame sizes

[Manivannan Sadhasivam]

Hi Sakari,

On Tue, Dec 03, 2019 at 10:56:04AM +0200, Sakari Ailus wrote:
> On Sat, Nov 30, 2019 at 12:35:40AM +0530, Manivannan Sadhasivam wrote:
> > Add support to enumerate all frame sizes supported by IMX290. This is
> > required for using with userspace tools such as libcamera.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/media/i2c/imx290.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index d5bb3a59ac46..f26c4a0ee0a0 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -468,6 +468,25 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >  
> > +static int imx290_enum_frame_size(struct v4l2_subdev *subdev,
> > +				  struct v4l2_subdev_pad_config *cfg,
> > +				  struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > +	if ((fse->code != imx290_formats[0].code) &&
> > +	    (fse->code != imx290_formats[1].code))
> 
> Please use a loop over imx290_formats instead.
> 

May I know why? What benefit does it provide over current method?

Thanks,
Mani

> > +		return -EINVAL;
> > +
> > +	if (fse->index >= ARRAY_SIZE(imx290_modes))
> > +		return -EINVAL;
> > +
> > +	fse->min_width = imx290_modes[fse->index].width;
> > +	fse->max_width = imx290_modes[fse->index].width;
> > +	fse->min_height = imx290_modes[fse->index].height;
> > +	fse->max_height = imx290_modes[fse->index].height;
> > +
> > +	return 0;
> > +}
> > +
> >  static int imx290_get_fmt(struct v4l2_subdev *sd,
> >  			  struct v4l2_subdev_pad_config *cfg,
> >  			  struct v4l2_subdev_format *fmt)
> > @@ -820,6 +839,7 @@ static const struct v4l2_subdev_video_ops imx290_video_ops = {
> >  static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
> >  	.init_cfg = imx290_entity_init_cfg,
> >  	.enum_mbus_code = imx290_enum_mbus_code,
> > +	.enum_frame_size = imx290_enum_frame_size,
> >  	.get_fmt = imx290_get_fmt,
> >  	.set_fmt = imx290_set_fmt,
> >  };
> 
> -- 
> Regards,
> 
> Sakari Ailus

Re: [PATCH 4/5] media: i2c: imx290: Add support to enumerate all frame sizes

[Manivannan Sadhasivam]

On Sun, Dec 15, 2019 at 03:52:37PM -0800, karthik poduval wrote:
> What if someone adds RAW8 or RAW14 formats in future the enum frame sizes
> code doesn't have to be patched again if written using a loop on formats
> array.
> 

Please don't top post :)

IMX290 only supports RAW10 and RAW12 formats. And I don't think this driver
can handle any other CMOS sensors from Sony, so looping over imx290_formats
seems unnecessary to me.

Thanks,
Mani

> On Sun, Dec 15, 2019, 9:49 AM Manivannan Sadhasivam <
> manivannan.sadhasivam@linaro.org> wrote:
> 
> > Hi Sakari,
> >
> > On Tue, Dec 03, 2019 at 10:56:04AM +0200, Sakari Ailus wrote:
> > > On Sat, Nov 30, 2019 at 12:35:40AM +0530, Manivannan Sadhasivam wrote:
> > > > Add support to enumerate all frame sizes supported by IMX290. This is
> > > > required for using with userspace tools such as libcamera.
> > > >
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org
> > >
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 20 ++++++++++++++++++++
> > > >  1 file changed, 20 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index d5bb3a59ac46..f26c4a0ee0a0 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -468,6 +468,25 @@ static int imx290_enum_mbus_code(struct
> > v4l2_subdev *sd,
> > > >     return 0;
> > > >  }
> > > >
> > > > +static int imx290_enum_frame_size(struct v4l2_subdev *subdev,
> > > > +                             struct v4l2_subdev_pad_config *cfg,
> > > > +                             struct v4l2_subdev_frame_size_enum *fse)
> > > > +{
> > > > +   if ((fse->code != imx290_formats[0].code) &&
> > > > +       (fse->code != imx290_formats[1].code))
> > >
> > > Please use a loop over imx290_formats instead.
> > >
> >
> > May I know why? What benefit does it provide over current method?
> >
> > Thanks,
> > Mani
> >
> > > > +           return -EINVAL;
> > > > +
> > > > +   if (fse->index >= ARRAY_SIZE(imx290_modes))
> > > > +           return -EINVAL;
> > > > +
> > > > +   fse->min_width = imx290_modes[fse->index].width;
> > > > +   fse->max_width = imx290_modes[fse->index].width;
> > > > +   fse->min_height = imx290_modes[fse->index].height;
> > > > +   fse->max_height = imx290_modes[fse->index].height;
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > >  static int imx290_get_fmt(struct v4l2_subdev *sd,
> > > >                       struct v4l2_subdev_pad_config *cfg,
> > > >                       struct v4l2_subdev_format *fmt)
> > > > @@ -820,6 +839,7 @@ static const struct v4l2_subdev_video_ops
> > imx290_video_ops = {
> > > >  static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
> > > >     .init_cfg = imx290_entity_init_cfg,
> > > >     .enum_mbus_code = imx290_enum_mbus_code,
> > > > +   .enum_frame_size = imx290_enum_frame_size,
> > > >     .get_fmt = imx290_get_fmt,
> > > >     .set_fmt = imx290_set_fmt,
> > > >  };
> > >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus
> >

Re: [PATCH 3/5] media: i2c: imx290: Add RAW12 mode support

[Sakari Ailus]

Hi Manivannan,

On Sun, Dec 15, 2019 at 11:16:06PM +0530, Manivannan Sadhasivam wrote:
> Hi Sakari,
> 
> On Tue, Dec 03, 2019 at 10:54:17AM +0200, Sakari Ailus wrote:
> > Hi Manivannan,
> > 
> > On Sat, Nov 30, 2019 at 12:35:39AM +0530, Manivannan Sadhasivam wrote:
> > > IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and
> > > 12 bit formats. Since the driver already supports RAW10 mode, let's add
> > > the missing RAW12 mode as well.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/media/i2c/imx290.c | 32 ++++++++++++++++++++++++++++++++
> > >  1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index e218c959a729..d5bb3a59ac46 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -75,6 +75,7 @@ struct imx290 {
> > >  	struct clk *xclk;
> > >  	struct regmap *regmap;
> > >  	int nlanes;
> > > +	u8 bpp;
> > >  
> > >  	struct v4l2_subdev sd;
> > >  	struct v4l2_fwnode_endpoint ep;
> > > @@ -98,6 +99,7 @@ struct imx290_pixfmt {
> > >  
> > >  static const struct imx290_pixfmt imx290_formats[] = {
> > >  	{ MEDIA_BUS_FMT_SRGGB10_1X10 },
> > > +	{ MEDIA_BUS_FMT_SRGGB12_1X12 },
> > >  };
> > >  
> > >  static const struct regmap_config imx290_regmap_config = {
> > > @@ -265,6 +267,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {
> > >  	{ 0x300b, 0x00},
> > >  };
> > >  
> > > +static const struct imx290_regval imx290_12bit_settings[] = {
> > > +	{ 0x3005, 0x01 },
> > > +	{ 0x3046, 0x01 },
> > > +	{ 0x3129, 0x00 },
> > > +	{ 0x317c, 0x00 },
> > > +	{ 0x31ec, 0x0e },
> > > +	{ 0x3441, 0x0c },
> > > +	{ 0x3442, 0x0c },
> > > +	{ 0x300a, 0xf0 },
> > > +	{ 0x300b, 0x00 },
> > > +};
> > > +
> > >  /* supported link frequencies */
> > >  static const s64 imx290_link_freq[] = {
> > >  	IMX290_DEFAULT_LINK_FREQ,
> > > @@ -550,6 +564,21 @@ static int imx290_write_current_format(struct imx290 *imx290,
> > >  			dev_err(imx290->dev, "Could not set format registers\n");
> > >  			return ret;
> > >  		}
> > > +
> > > +		imx290->bpp = 10;
> > > +
> > > +		break;
> > > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > > +		ret = imx290_set_register_array(imx290, imx290_12bit_settings,
> > > +						ARRAY_SIZE(
> > > +							imx290_12bit_settings));
> > > +		if (ret < 0) {
> > > +			dev_err(imx290->dev, "Could not set format registers\n");
> > > +			return ret;
> > > +		}
> > > +
> > > +		imx290->bpp = 12;
> > > +
> > >  		break;
> > >  	default:
> > >  		dev_err(imx290->dev, "Unknown pixel format\n");
> > > @@ -910,6 +939,9 @@ static int imx290_probe(struct i2c_client *client)
> > >  		goto free_err;
> > >  	}
> > >  
> > > +	/* Default bits per pixel value */
> > > +	imx290->bpp = 10;
> > 
> > Where is the format being initialised at the moment? Nowhere?
> > 
> > If that is the case, I think it should be fixed before this patch.
> > 
> 
> Sorry, I don't quite understand what you're suggesting here. The bpp
> is initialised because that's the default bit value at power up and
> this value is being used below in imx290_calc_pixel_rate(). I'm not sure
> why we need to initialise the format before set_fmt!

Alternatively set_fmt needs to be called then.

It looks like you can call VIDIOC_SUBDEV_G_FMT without the format being
initialised first, and if that's the case, then it's a driver bug. I don't
have the sensor so I can't test it.

-- 
Sakari Ailus