[PATCH v3 00/10] Improvements to IMX290 CMOS driver

[PATCH v3 00/10] Improvements to IMX290 CMOS driver

[Andrey Konovalov]

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

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

Also fixes for the following issues in the existing driver are included:
* the current_format field in the struct imx290 can be used before
  initialization,
* the reset signal to IMX290 isn't handled correctly,
* the bus_type field of v4l2_fwnode_endpoint structure passed as the
  argument to v4l2_fwnode_endpoint_alloc_parse() function is not
  initiaized.

Changes in v3:

* The review comments from Sakari are addressed
  https://lkml.org/lkml/2019/12/19/705
  As a part of those changes:
  . null ptr checks are added to imx290_set_fmt() so that it can be called
    early in the probe() function to set the default format, and to
    initialize imx290->current_mode and imx290->bpp - these last two must be 
    set before imx290_calc_pixel_rate() is called when creating the controls
  . setting imx290->bpp removed from imx290_write_current_format(). Now this
    function only writes to the camera sensor registers. The call to
    imx290_write_current_format() is moved from imx290_set_fmt() back to
    imx290_start_streaming(): imx290_set_fmt() can be called when the sensor
    is powered off, and writes to the sensor registers would fail.
  . in imx290_set_ctrl() in the 12 bpp case the value the BLKLEVEL register
    is restored to when the test pattern is disabled is made consistent with
    imx290_12bit_settings[]
* The "IMX290 sensor driver fixes" patchset included
  https://patchwork.kernel.org/cover/11407347/
* Added a patch to set the bus_type field of v4l2_fwnode_endpoint structure
  before calling v4l2_fwnode_endpoint_alloc_parse()

Andrey Konovalov (4):
  media: i2c: imx290: set the format before VIDIOC_SUBDEV_G_FMT is
    called
  media: i2c: imx290: fix the order of the args in SET_RUNTIME_PM_OPS()
  media: i2c: imx290: fix reset GPIO pin handling
  media: i2c: imx290: set bus_type before calling
    v4l2_fwnode_endpoint_alloc_parse()

Manivannan Sadhasivam (6):
  media: i2c: imx290: Add support for 2 data lanes
  media: i2c: imx290: Add configurable link frequency and pixel rate
  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: Move the settle time delay out of loop

 drivers/media/i2c/imx290.c | 358 ++++++++++++++++++++++++++++++-------
 1 file changed, 297 insertions(+), 61 deletions(-)

-- 
2.17.1

[PATCH v3 01/10] media: i2c: imx290: set the format before VIDIOC_SUBDEV_G_FMT is called

[Andrey Konovalov]

With the current driver 'media-ctl -p' issued right after the imx290 driver
is loaded prints:
pad0: Source
             [fmt:unknown/0x0]

The format value of zero is due to the current_format field of the imx290
struct not being initialized yet.

As imx290_entity_init_cfg() calls imx290_set_fmt(), the current_mode field
is also initialized, so the line which set current_mode to a default value
in driver's probe() function is no longer needed.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index f7678e5a5d87..2d8c38ffe2f0 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -722,9 +722,6 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_err;
 	}
 
-	/* Set default mode to max resolution */
-	imx290->current_mode = &imx290_modes[0];
-
 	/* get system clock (xclk) */
 	imx290->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(imx290->xclk)) {
@@ -809,6 +806,9 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_ctrl;
 	}
 
+	/* Initialize the frame format (this also sets imx290->current_mode) */
+	imx290_entity_init_cfg(&imx290->sd, NULL);
+
 	ret = v4l2_async_register_subdev(&imx290->sd);
 	if (ret < 0) {
 		dev_err(dev, "Could not register v4l2 device\n");
-- 
2.17.1

[PATCH v3 02/10] media: i2c: imx290: fix the order of the args in SET_RUNTIME_PM_OPS()

[Andrey Konovalov]

This macro is defined as SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn),
so imx290_power_off must be the 1st arg, and imx290_power_on the 2nd.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 2d8c38ffe2f0..d0322f9a8856 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -648,7 +648,7 @@ static int imx290_power_off(struct device *dev)
 }
 
 static const struct dev_pm_ops imx290_pm_ops = {
-	SET_RUNTIME_PM_OPS(imx290_power_on, imx290_power_off, NULL)
+	SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
 };
 
 static const struct v4l2_subdev_video_ops imx290_video_ops = {
-- 
2.17.1

[PATCH v3 03/10] media: i2c: imx290: fix reset GPIO pin handling

[Andrey Konovalov]

According to https://www.kernel.org/doc/Documentation/gpio/consumer.txt,

- all of the gpiod_set_value_xxx() functions operate with the *logical* value.
So in imx290_power_on() the reset signal should be cleared/de-asserted with
gpiod_set_value_cansleep(imx290->rst_gpio, 0), and in imx290_power_off() the
value of 1 must be used to apply/assert the reset to the sensor. In the device
tree the reset pin is described as GPIO_ACTIVE_LOW, and gpiod_set_value_xxx()
functions take this into account,

- when devm_gpiod_get_optional() is called with GPIOD_ASIS, the GPIO is not
initialized, and the direction must be set later; using a GPIO
without setting its direction first is illegal and will result in undefined
behavior. Fix this by using GPIOD_OUT_HIGH instead of GPIOD_ASIS (this asserts
the reset signal to the sensor initially).

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/media/i2c/imx290.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index d0322f9a8856..7b1de1f0c8b7 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -628,7 +628,7 @@ static int imx290_power_on(struct device *dev)
 	}
 
 	usleep_range(1, 2);
-	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
+	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
 	usleep_range(30000, 31000);
 
 	return 0;
@@ -641,7 +641,7 @@ static int imx290_power_off(struct device *dev)
 	struct imx290 *imx290 = to_imx290(sd);
 
 	clk_disable_unprepare(imx290->xclk);
-	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
+	gpiod_set_value_cansleep(imx290->rst_gpio, 1);
 	regulator_bulk_disable(IMX290_NUM_SUPPLIES, imx290->supplies);
 
 	return 0;
@@ -757,7 +757,8 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_err;
 	}
 
-	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
+	imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
+						   GPIOD_OUT_HIGH);
 	if (IS_ERR(imx290->rst_gpio)) {
 		dev_err(dev, "Cannot get reset gpio\n");
 		ret = PTR_ERR(imx290->rst_gpio);
-- 
2.17.1

[PATCH v3 04/10] media: i2c: imx290: Add support for 2 data lanes

[Andrey Konovalov]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/imx290.c | 133 ++++++++++++++++++++++++++++++++++---
 1 file changed, 124 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 7b1de1f0c8b7..a361c9ac8bd5 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;
+	u8 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);
@@ -631,6 +730,9 @@ static int imx290_power_on(struct device *dev)
 	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
 	usleep_range(30000, 31000);
 
+	/* Set data lane count */
+	imx290_set_data_lanes(imx290);
+
 	return 0;
 }
 
@@ -703,6 +805,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;
@@ -823,6 +935,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 v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate

[Andrey Konovalov]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/imx290.c | 100 ++++++++++++++++++++++++-------------
 1 file changed, 66 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a361c9ac8bd5..e800557cf423 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -38,8 +38,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",
@@ -56,8 +54,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;
@@ -248,8 +244,13 @@ static const struct imx290_regval imx290_10bit_settings[] = {
 };
 
 /* supported link frequencies */
-static const s64 imx290_link_freq[] = {
-	IMX290_DEFAULT_LINK_FREQ,
+static const s64 imx290_link_freq_2lanes[] = {
+	891000000, /* 1920x1080 -  2 lane */
+	594000000, /* 1280x720  -  2 lane */
+};
+static const s64 imx290_link_freq_4lanes[] = {
+	445500000, /* 1920x1080 -  4 lane */
+	297000000, /* 1280x720  -  4 lane */
 };
 
 /* Mode configs */
@@ -259,16 +260,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,
 	},
 };
 
@@ -442,6 +439,32 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static u8 imx290_get_link_freq_index(struct imx290 *imx290)
+{
+	const struct imx290_mode *cur_mode = imx290->current_mode;
+
+	return (cur_mode->width == 1920) ? 0 : 1;
+}
+
+static s64 imx290_get_link_freq(struct imx290 *imx290)
+{
+	u8 index = imx290_get_link_freq_index(imx290);
+
+	if (imx290->nlanes == 4)
+		return imx290_link_freq_4lanes[index];
+	else
+		return imx290_link_freq_2lanes[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 / 10);
+}
+
 static int imx290_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_pad_config *cfg,
 		      struct v4l2_subdev_format *fmt)
@@ -475,10 +498,14 @@ 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;
+
+		if (imx290->link_freq)
+			__v4l2_ctrl_s_ctrl(imx290->link_freq,
+					   imx290_get_link_freq_index(imx290));
+		if (imx290->pixel_rate)
+			__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
+						 imx290_calc_pixel_rate(imx290));
 	}
 
 	*format = fmt->format;
@@ -502,12 +529,11 @@ 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)
+static int imx290_write_current_format(struct imx290 *imx290)
 {
 	int ret;
 
-	switch (format->code) {
+	switch (imx290->current_format.code) {
 	case MEDIA_BUS_FMT_SRGGB10_1X10:
 		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
 						ARRAY_SIZE(
@@ -558,8 +584,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
-	/* Set current frame format */
-	ret = imx290_write_current_format(imx290, &imx290->current_format);
+	/* Apply the register values related to current frame format */
+	ret = imx290_write_current_format(imx290);
 	if (ret < 0) {
 		dev_err(imx290->dev, "Could not set frame format\n");
 		return ret;
@@ -821,12 +847,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");
@@ -879,23 +899,38 @@ static int imx290_probe(struct i2c_client *client)
 
 	mutex_init(&imx290->lock);
 
+	/*
+	 * Initialize the frame format. In particular, imx290->current_mode
+	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
+	 * below relies on these fields.
+	 */
+	imx290_entity_init_cfg(&imx290->sd, NULL);
+
 	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
 
 	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
 			  V4L2_CID_GAIN, 0, 72, 1, 0);
-	imx290->link_freq =
-		v4l2_ctrl_new_int_menu(&imx290->ctrls,
-				       &imx290_ctrl_ops,
-				       V4L2_CID_LINK_FREQ,
-				       ARRAY_SIZE(imx290_link_freq) - 1,
-				       0, imx290_link_freq);
+	if (imx290->nlanes == 4)
+		imx290->link_freq =
+			v4l2_ctrl_new_int_menu(&imx290->ctrls,
+					       &imx290_ctrl_ops,
+					       V4L2_CID_LINK_FREQ,
+					       ARRAY_SIZE(imx290_link_freq_4lanes) - 1,
+					       0, imx290_link_freq_4lanes);
+	else
+		imx290->link_freq =
+			v4l2_ctrl_new_int_menu(&imx290->ctrls,
+					       &imx290_ctrl_ops,
+					       V4L2_CID_LINK_FREQ,
+					       ARRAY_SIZE(imx290_link_freq_2lanes) - 1,
+					       0, imx290_link_freq_2lanes);
 	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));
 
 	imx290->sd.ctrl_handler = &imx290->ctrls;
 
@@ -919,9 +954,6 @@ static int imx290_probe(struct i2c_client *client)
 		goto free_ctrl;
 	}
 
-	/* Initialize the frame format (this also sets imx290->current_mode) */
-	imx290_entity_init_cfg(&imx290->sd, NULL);
-
 	ret = v4l2_async_register_subdev(&imx290->sd);
 	if (ret < 0) {
 		dev_err(dev, "Could not register v4l2 device\n");
-- 
2.17.1

[PATCH v3 06/10] media: i2c: imx290: Add support for test pattern generation

[Andrey Konovalov]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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>
Signed-off-by: Andrey Konovalov <andrey.konovalov@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 e800557cf423..162c345fffac 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
@@ -95,6 +102,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 },
@@ -391,6 +409,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);
+			msleep(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);
+			msleep(10);
+			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
+			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
+		}
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -906,7 +940,7 @@ static int imx290_probe(struct i2c_client *client)
 	 */
 	imx290_entity_init_cfg(&imx290->sd, NULL);
 
-	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);
@@ -932,6 +966,11 @@ static int imx290_probe(struct i2c_client *client)
 					       INT_MAX, 1,
 					       imx290_calc_pixel_rate(imx290));
 
+	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 v3 07/10] media: i2c: imx290: Add RAW12 mode support

[Andrey Konovalov]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/imx290.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 162c345fffac..6e70ff22bc5f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -71,6 +71,7 @@ struct imx290 {
 	struct clk *xclk;
 	struct regmap *regmap;
 	u8 nlanes;
+	u8 bpp;
 
 	struct v4l2_subdev sd;
 	struct v4l2_fwnode_endpoint ep;
@@ -90,10 +91,12 @@ struct imx290 {
 
 struct imx290_pixfmt {
 	u32 code;
+	u8 bpp;
 };
 
 static const struct imx290_pixfmt imx290_formats[] = {
-	{ MEDIA_BUS_FMT_SRGGB10_1X10 },
+	{ MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
+	{ MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
 };
 
 static const struct regmap_config imx290_regmap_config = {
@@ -261,6 +264,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_2lanes[] = {
 	891000000, /* 1920x1080 -  2 lane */
@@ -421,7 +436,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 		} else {
 			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
 			msleep(10);
-			imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
+			if (imx290->bpp == 10)
+				imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
+						 0x3c);
+			else /* 12 bits per pixel */
+				imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
+						 0xf0);
 			imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
 		}
 		break;
@@ -496,7 +516,7 @@ static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
 	u8 nlanes = imx290->nlanes;
 
 	/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	return (link_freq * 2 * nlanes / 10);
+	return (link_freq * 2 * nlanes / imx290->bpp);
 }
 
 static int imx290_set_fmt(struct v4l2_subdev *sd,
@@ -533,6 +553,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
 	} else {
 		format = &imx290->current_format;
 		imx290->current_mode = mode;
+		imx290->bpp = imx290_formats[i].bpp;
 
 		if (imx290->link_freq)
 			__v4l2_ctrl_s_ctrl(imx290->link_freq,
@@ -577,6 +598,15 @@ static int imx290_write_current_format(struct imx290 *imx290)
 			return ret;
 		}
 		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;
+		}
+		break;
 	default:
 		dev_err(imx290->dev, "Unknown pixel format\n");
 		return -EINVAL;
-- 
2.17.1

[PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all frame sizes

[Andrey Konovalov]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

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>
Signed-off-by: Andrey Konovalov <andrey.konovalov@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 6e70ff22bc5f..88850f3b1427 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -471,6 +471,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)
@@ -850,6 +869,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 v3 09/10] media: i2c: imx290: Move the settle time delay out of loop

[Andrey Konovalov]

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

The 10ms settle time is needed only at the end of all consecutive
register writes. So move the delay to outside of the for loop of
imx290_set_register_array().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/imx290.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 88850f3b1427..ee5c95cf64f3 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -347,11 +347,11 @@ static int imx290_set_register_array(struct imx290 *imx290,
 		ret = imx290_write_reg(imx290, settings->reg, settings->val);
 		if (ret < 0)
 			return ret;
-
-		/* Settle time is 10ms for all registers */
-		msleep(10);
 	}
 
+	/* Provide 10ms settle time */
+	msleep(10);
+
 	return 0;
 }
 
-- 
2.17.1

[PATCH v3 10/10] media: i2c: imx290: set bus_type before calling v4l2_fwnode_endpoint_alloc_parse()

[Andrey Konovalov]

The bus_type field of v4l2_fwnode_endpoint structure passed as the argument
to v4l2_fwnode_endpoint_alloc_parse() function must be initiaized.
Set it to V4L2_MBUS_CSI2_DPHY, and check for -ENXIO which is returned
when the requested media bus type doesn't match the fwnode.

Also remove v4l2_fwnode_endpoint field from struct imx290 as it is only
needed in the probe function: use the local variable for this purpose.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 drivers/media/i2c/imx290.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index ee5c95cf64f3..05a3d897614e 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -74,7 +74,6 @@ struct imx290 {
 	u8 bpp;
 
 	struct v4l2_subdev sd;
-	struct v4l2_fwnode_endpoint ep;
 	struct media_pad pad;
 	struct v4l2_mbus_framefmt current_format;
 	const struct imx290_mode *current_mode;
@@ -887,6 +886,10 @@ static int imx290_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
 	struct fwnode_handle *endpoint;
+	/* Only CSI2 is supported for now: */
+	struct v4l2_fwnode_endpoint ep = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
 	struct imx290 *imx290;
 	u32 xclk_freq;
 	int ret;
@@ -908,15 +911,18 @@ static int imx290_probe(struct i2c_client *client)
 		return -EINVAL;
 	}
 
-	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &imx290->ep);
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
 	fwnode_handle_put(endpoint);
-	if (ret) {
+	if (ret == -ENXIO) {
+		dev_err(dev, "Unsupported bus type, should be CSI2\n");
+		goto free_err;
+	} else if (ret) {
 		dev_err(dev, "Parsing endpoint node failed\n");
 		goto free_err;
 	}
 
 	/* Get number of data lanes */
-	imx290->nlanes = imx290->ep.bus.mipi_csi2.num_data_lanes;
+	imx290->nlanes = 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;
@@ -925,19 +931,12 @@ static int imx290_probe(struct i2c_client *client)
 
 	dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
 
-	if (!imx290->ep.nr_of_link_frequencies) {
+	if (!ep.nr_of_link_frequencies) {
 		dev_err(dev, "link-frequency property not found in DT\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");
-		ret = -EINVAL;
-		goto free_err;
-	}
-
 	/* get system clock (xclk) */
 	imx290->xclk = devm_clk_get(dev, "xclk");
 	if (IS_ERR(imx290->xclk)) {
@@ -1063,7 +1062,7 @@ static int imx290_probe(struct i2c_client *client)
 	pm_runtime_enable(dev);
 	pm_runtime_idle(dev);
 
-	v4l2_fwnode_endpoint_free(&imx290->ep);
+	v4l2_fwnode_endpoint_free(&ep);
 
 	return 0;
 
@@ -1073,7 +1072,7 @@ static int imx290_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(&imx290->ctrls);
 	mutex_destroy(&imx290->lock);
 free_err:
-	v4l2_fwnode_endpoint_free(&imx290->ep);
+	v4l2_fwnode_endpoint_free(&ep);
 
 	return ret;
 }
-- 
2.17.1

Re: [PATCH v3 04/10] media: i2c: imx290: Add support for 2 data lanes

[Sakari Ailus]

Hi Andrey,

On Sun, May 24, 2020 at 10:24:59PM +0300, Andrey Konovalov wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> 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>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 133 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 124 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 7b1de1f0c8b7..a361c9ac8bd5 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;
> +	u8 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;

I think it'd be nicer to put this where the mode definitions are, to avoid
scattering the configuration around the driver.

> +		}
> +
> +		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);
> @@ -631,6 +730,9 @@ static int imx290_power_on(struct device *dev)
>  	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
>  	usleep_range(30000, 31000);
>  
> +	/* Set data lane count */
> +	imx290_set_data_lanes(imx290);
> +
>  	return 0;
>  }
>  
> @@ -703,6 +805,16 @@ static int imx290_probe(struct i2c_client *client)
>  		goto free_err;
>  	}
>  
> +	/* Get number of data lanes */

While at it, could you set the PHY type in the V4L2 fwnode endpoint before
parsing the data using v4l2_fwnode_endpoint_alloc_parse()?

> +	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;
> @@ -823,6 +935,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);

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate

[Sakari Ailus]

Hi Andrey,

On Sun, May 24, 2020 at 10:25:00PM +0300, Andrey Konovalov wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> 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>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 100 ++++++++++++++++++++++++-------------
>  1 file changed, 66 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index a361c9ac8bd5..e800557cf423 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -38,8 +38,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",
> @@ -56,8 +54,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;
> @@ -248,8 +244,13 @@ static const struct imx290_regval imx290_10bit_settings[] = {
>  };
>  
>  /* supported link frequencies */
> -static const s64 imx290_link_freq[] = {
> -	IMX290_DEFAULT_LINK_FREQ,
> +static const s64 imx290_link_freq_2lanes[] = {
> +	891000000, /* 1920x1080 -  2 lane */
> +	594000000, /* 1280x720  -  2 lane */
> +};
> +static const s64 imx290_link_freq_4lanes[] = {
> +	445500000, /* 1920x1080 -  4 lane */
> +	297000000, /* 1280x720  -  4 lane */
>  };
>  
>  /* Mode configs */
> @@ -259,16 +260,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,
>  	},
>  };
>  
> @@ -442,6 +439,32 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static u8 imx290_get_link_freq_index(struct imx290 *imx290)
> +{
> +	const struct imx290_mode *cur_mode = imx290->current_mode;
> +
> +	return (cur_mode->width == 1920) ? 0 : 1;

Could you use (imx290->current_mode - imx290_modes) / sizeof(*imx290_modes)
or something like that? It'd have fewer chances of breaking if new modes
are added.

> +}
> +
> +static s64 imx290_get_link_freq(struct imx290 *imx290)
> +{
> +	u8 index = imx290_get_link_freq_index(imx290);
> +
> +	if (imx290->nlanes == 4)
> +		return imx290_link_freq_4lanes[index];
> +	else
> +		return imx290_link_freq_2lanes[index];

Or even better: store the link frequencies to the modes themselves. They
are a property of the modes after all.

> +}
> +
> +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 / 10);
> +}
> +
>  static int imx290_set_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_pad_config *cfg,
>  		      struct v4l2_subdev_format *fmt)
> @@ -475,10 +498,14 @@ 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;
> +
> +		if (imx290->link_freq)
> +			__v4l2_ctrl_s_ctrl(imx290->link_freq,
> +					   imx290_get_link_freq_index(imx290));
> +		if (imx290->pixel_rate)
> +			__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
> +						 imx290_calc_pixel_rate(imx290));
>  	}
>  
>  	*format = fmt->format;
> @@ -502,12 +529,11 @@ 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)
> +static int imx290_write_current_format(struct imx290 *imx290)
>  {
>  	int ret;
>  
> -	switch (format->code) {
> +	switch (imx290->current_format.code) {
>  	case MEDIA_BUS_FMT_SRGGB10_1X10:
>  		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
>  						ARRAY_SIZE(
> @@ -558,8 +584,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
>  		return ret;
>  	}
>  
> -	/* Set current frame format */
> -	ret = imx290_write_current_format(imx290, &imx290->current_format);
> +	/* Apply the register values related to current frame format */
> +	ret = imx290_write_current_format(imx290);
>  	if (ret < 0) {
>  		dev_err(imx290->dev, "Could not set frame format\n");
>  		return ret;
> @@ -821,12 +847,6 @@ static int imx290_probe(struct i2c_client *client)
>  		goto free_err;
>  	}
>  
> -	if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {

This check needs to be modified to correspond to the driver's new
capabilities, not removed.

> -		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");
> @@ -879,23 +899,38 @@ static int imx290_probe(struct i2c_client *client)
>  
>  	mutex_init(&imx290->lock);
>  
> +	/*
> +	 * Initialize the frame format. In particular, imx290->current_mode
> +	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
> +	 * below relies on these fields.
> +	 */
> +	imx290_entity_init_cfg(&imx290->sd, NULL);
> +
>  	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
>  
>  	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>  			  V4L2_CID_GAIN, 0, 72, 1, 0);
> -	imx290->link_freq =
> -		v4l2_ctrl_new_int_menu(&imx290->ctrls,
> -				       &imx290_ctrl_ops,
> -				       V4L2_CID_LINK_FREQ,
> -				       ARRAY_SIZE(imx290_link_freq) - 1,
> -				       0, imx290_link_freq);
> +	if (imx290->nlanes == 4)
> +		imx290->link_freq =
> +			v4l2_ctrl_new_int_menu(&imx290->ctrls,
> +					       &imx290_ctrl_ops,
> +					       V4L2_CID_LINK_FREQ,
> +					       ARRAY_SIZE(imx290_link_freq_4lanes) - 1,
> +					       0, imx290_link_freq_4lanes);
> +	else
> +		imx290->link_freq =
> +			v4l2_ctrl_new_int_menu(&imx290->ctrls,
> +					       &imx290_ctrl_ops,
> +					       V4L2_CID_LINK_FREQ,
> +					       ARRAY_SIZE(imx290_link_freq_2lanes) - 1,
> +					       0, imx290_link_freq_2lanes);
>  	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));
>  
>  	imx290->sd.ctrl_handler = &imx290->ctrls;
>  
> @@ -919,9 +954,6 @@ static int imx290_probe(struct i2c_client *client)
>  		goto free_ctrl;
>  	}
>  
> -	/* Initialize the frame format (this also sets imx290->current_mode) */
> -	imx290_entity_init_cfg(&imx290->sd, NULL);
> -
>  	ret = v4l2_async_register_subdev(&imx290->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "Could not register v4l2 device\n");

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 04/10] media: i2c: imx290: Add support for 2 data lanes

[Andrey Konovalov]

Hi Sakari,

Thank you for the review

On 26.05.2020 12:01, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Sun, May 24, 2020 at 10:24:59PM +0300, Andrey Konovalov wrote:
>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> 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>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/i2c/imx290.c | 133 ++++++++++++++++++++++++++++++++++---
>>   1 file changed, 124 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
>> index 7b1de1f0c8b7..a361c9ac8bd5 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;
>> +	u8 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;
> 
> I think it'd be nicer to put this where the mode definitions are, to avoid
> scattering the configuration around the driver.

Would it be OK if I move this inside imx290_write_current_format()?

>> +		}
>> +
>> +		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);
>> @@ -631,6 +730,9 @@ static int imx290_power_on(struct device *dev)
>>   	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
>>   	usleep_range(30000, 31000);
>>   
>> +	/* Set data lane count */
>> +	imx290_set_data_lanes(imx290);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -703,6 +805,16 @@ static int imx290_probe(struct i2c_client *client)
>>   		goto free_err;
>>   	}
>>   
>> +	/* Get number of data lanes */
> 
> While at it, could you set the PHY type in the V4L2 fwnode endpoint before
> parsing the data using v4l2_fwnode_endpoint_alloc_parse()?

This is currently done in "[PATCH v3 10/10] media: i2c: imx290: set bus_type
before calling v4l2_fwnode_endpoint_alloc_parse()" (along with some more
clean-ups for the probe()). I can merge the PHY type in the V4L2 fwnode endpoint
change into this patch.

Thanks,
Andrey

>> +	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;
>> @@ -823,6 +935,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);
> 

Re: [PATCH v3 04/10] media: i2c: imx290: Add support for 2 data lanes

[Sakari Ailus]

Hi Andrey,

On Tue, May 26, 2020 at 12:14:33PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> Thank you for the review
> 
> On 26.05.2020 12:01, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > On Sun, May 24, 2020 at 10:24:59PM +0300, Andrey Konovalov wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > 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>
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   drivers/media/i2c/imx290.c | 133 ++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 124 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index 7b1de1f0c8b7..a361c9ac8bd5 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;
> > > +	u8 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;
> > 
> > I think it'd be nicer to put this where the mode definitions are, to avoid
> > scattering the configuration around the driver.
> 
> Would it be OK if I move this inside imx290_write_current_format()?

It'd still be separated from the mode there. My point was that it is
specific to the mode, and should be associated with it.

> 
> > > +		}
> > > +
> > > +		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);
> > > @@ -631,6 +730,9 @@ static int imx290_power_on(struct device *dev)
> > >   	gpiod_set_value_cansleep(imx290->rst_gpio, 0);
> > >   	usleep_range(30000, 31000);
> > > +	/* Set data lane count */
> > > +	imx290_set_data_lanes(imx290);
> > > +
> > >   	return 0;
> > >   }
> > > @@ -703,6 +805,16 @@ static int imx290_probe(struct i2c_client *client)
> > >   		goto free_err;
> > >   	}
> > > +	/* Get number of data lanes */
> > 
> > While at it, could you set the PHY type in the V4L2 fwnode endpoint before
> > parsing the data using v4l2_fwnode_endpoint_alloc_parse()?
> 
> This is currently done in "[PATCH v3 10/10] media: i2c: imx290: set bus_type
> before calling v4l2_fwnode_endpoint_alloc_parse()" (along with some more
> clean-ups for the probe()). I can merge the PHY type in the V4L2 fwnode endpoint
> change into this patch.

Ack, I hadn't gotten that far yet. It's fine to keep it as-is.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all frame sizes

[Sakari Ailus]

Hi Andrey,

On Sun, May 24, 2020 at 10:25:03PM +0300, Andrey Konovalov wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> 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>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@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 6e70ff22bc5f..88850f3b1427 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -471,6 +471,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;

Please skip the modes that do not have the code specified by the user. They
should not be enumerated here.

> +
> +	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)
> @@ -850,6 +869,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 v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate

[Andrey Konovalov]

Hi Sakari,

Thank you for the review!

On 26.05.2020 12:12, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Sun, May 24, 2020 at 10:25:00PM +0300, Andrey Konovalov wrote:
>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> 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>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/i2c/imx290.c | 100 ++++++++++++++++++++++++-------------
>>   1 file changed, 66 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
>> index a361c9ac8bd5..e800557cf423 100644
>> --- a/drivers/media/i2c/imx290.c
>> +++ b/drivers/media/i2c/imx290.c
>> @@ -38,8 +38,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",
>> @@ -56,8 +54,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;
>> @@ -248,8 +244,13 @@ static const struct imx290_regval imx290_10bit_settings[] = {
>>   };
>>   
>>   /* supported link frequencies */
>> -static const s64 imx290_link_freq[] = {
>> -	IMX290_DEFAULT_LINK_FREQ,
>> +static const s64 imx290_link_freq_2lanes[] = {
>> +	891000000, /* 1920x1080 -  2 lane */
>> +	594000000, /* 1280x720  -  2 lane */
>> +};
>> +static const s64 imx290_link_freq_4lanes[] = {
>> +	445500000, /* 1920x1080 -  4 lane */
>> +	297000000, /* 1280x720  -  4 lane */
>>   };
>>   
>>   /* Mode configs */
>> @@ -259,16 +260,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,
>>   	},
>>   };
>>   
>> @@ -442,6 +439,32 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
>>   	return 0;
>>   }
>>   
>> +static u8 imx290_get_link_freq_index(struct imx290 *imx290)
>> +{
>> +	const struct imx290_mode *cur_mode = imx290->current_mode;
>> +
>> +	return (cur_mode->width == 1920) ? 0 : 1;
> 
> Could you use (imx290->current_mode - imx290_modes) / sizeof(*imx290_modes)
> or something like that? It'd have fewer chances of breaking if new modes
> are added.
> 
>> +}
>> +
>> +static s64 imx290_get_link_freq(struct imx290 *imx290)
>> +{
>> +	u8 index = imx290_get_link_freq_index(imx290);
>> +
>> +	if (imx290->nlanes == 4)
>> +		return imx290_link_freq_4lanes[index];
>> +	else
>> +		return imx290_link_freq_2lanes[index];
> 
> Or even better: store the link frequencies to the modes themselves. They
> are a property of the modes after all.

Then we will get two sets (for 2 lanes and for 4 lanes) of two modes (1080p and 720p), right?

>> +}
>> +
>> +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 / 10);
>> +}
>> +
>>   static int imx290_set_fmt(struct v4l2_subdev *sd,
>>   			  struct v4l2_subdev_pad_config *cfg,
>>   		      struct v4l2_subdev_format *fmt)
>> @@ -475,10 +498,14 @@ 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;
>> +
>> +		if (imx290->link_freq)
>> +			__v4l2_ctrl_s_ctrl(imx290->link_freq,
>> +					   imx290_get_link_freq_index(imx290));
>> +		if (imx290->pixel_rate)
>> +			__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
>> +						 imx290_calc_pixel_rate(imx290));
>>   	}
>>   
>>   	*format = fmt->format;
>> @@ -502,12 +529,11 @@ 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)
>> +static int imx290_write_current_format(struct imx290 *imx290)
>>   {
>>   	int ret;
>>   
>> -	switch (format->code) {
>> +	switch (imx290->current_format.code) {
>>   	case MEDIA_BUS_FMT_SRGGB10_1X10:
>>   		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
>>   						ARRAY_SIZE(
>> @@ -558,8 +584,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
>>   		return ret;
>>   	}
>>   
>> -	/* Set current frame format */
>> -	ret = imx290_write_current_format(imx290, &imx290->current_format);
>> +	/* Apply the register values related to current frame format */
>> +	ret = imx290_write_current_format(imx290);
>>   	if (ret < 0) {
>>   		dev_err(imx290->dev, "Could not set frame format\n");
>>   		return ret;
>> @@ -821,12 +847,6 @@ static int imx290_probe(struct i2c_client *client)
>>   		goto free_err;
>>   	}
>>   
>> -	if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {
> 
> This check needs to be modified to correspond to the driver's new
> capabilities, not removed.

Agreed.
Do I understand correct that as the driver uses two link frequencies
for a given number of lanes now, it must check that *the both* frequencies
(for the given number of lanes) are listed in the device tree node?

Thanks,
Andrey

>> -		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");
>> @@ -879,23 +899,38 @@ static int imx290_probe(struct i2c_client *client)
>>   
>>   	mutex_init(&imx290->lock);
>>   
>> +	/*
>> +	 * Initialize the frame format. In particular, imx290->current_mode
>> +	 * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
>> +	 * below relies on these fields.
>> +	 */
>> +	imx290_entity_init_cfg(&imx290->sd, NULL);
>> +
>>   	v4l2_ctrl_handler_init(&imx290->ctrls, 3);
>>   
>>   	v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
>>   			  V4L2_CID_GAIN, 0, 72, 1, 0);
>> -	imx290->link_freq =
>> -		v4l2_ctrl_new_int_menu(&imx290->ctrls,
>> -				       &imx290_ctrl_ops,
>> -				       V4L2_CID_LINK_FREQ,
>> -				       ARRAY_SIZE(imx290_link_freq) - 1,
>> -				       0, imx290_link_freq);
>> +	if (imx290->nlanes == 4)
>> +		imx290->link_freq =
>> +			v4l2_ctrl_new_int_menu(&imx290->ctrls,
>> +					       &imx290_ctrl_ops,
>> +					       V4L2_CID_LINK_FREQ,
>> +					       ARRAY_SIZE(imx290_link_freq_4lanes) - 1,
>> +					       0, imx290_link_freq_4lanes);
>> +	else
>> +		imx290->link_freq =
>> +			v4l2_ctrl_new_int_menu(&imx290->ctrls,
>> +					       &imx290_ctrl_ops,
>> +					       V4L2_CID_LINK_FREQ,
>> +					       ARRAY_SIZE(imx290_link_freq_2lanes) - 1,
>> +					       0, imx290_link_freq_2lanes);
>>   	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));
>>   
>>   	imx290->sd.ctrl_handler = &imx290->ctrls;
>>   
>> @@ -919,9 +954,6 @@ static int imx290_probe(struct i2c_client *client)
>>   		goto free_ctrl;
>>   	}
>>   
>> -	/* Initialize the frame format (this also sets imx290->current_mode) */
>> -	imx290_entity_init_cfg(&imx290->sd, NULL);
>> -
>>   	ret = v4l2_async_register_subdev(&imx290->sd);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Could not register v4l2 device\n");
> 

Re: [PATCH v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate

[Sakari Ailus]

Hi Andrey,

On Tue, May 26, 2020 at 12:27:17PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> Thank you for the review!

You're welcome!

> 
> On 26.05.2020 12:12, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > On Sun, May 24, 2020 at 10:25:00PM +0300, Andrey Konovalov wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > 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>
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> > > ---
> > >   drivers/media/i2c/imx290.c | 100 ++++++++++++++++++++++++-------------
> > >   1 file changed, 66 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > index a361c9ac8bd5..e800557cf423 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -38,8 +38,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",
> > > @@ -56,8 +54,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;
> > > @@ -248,8 +244,13 @@ static const struct imx290_regval imx290_10bit_settings[] = {
> > >   };
> > >   /* supported link frequencies */
> > > -static const s64 imx290_link_freq[] = {
> > > -	IMX290_DEFAULT_LINK_FREQ,
> > > +static const s64 imx290_link_freq_2lanes[] = {
> > > +	891000000, /* 1920x1080 -  2 lane */
> > > +	594000000, /* 1280x720  -  2 lane */
> > > +};
> > > +static const s64 imx290_link_freq_4lanes[] = {
> > > +	445500000, /* 1920x1080 -  4 lane */
> > > +	297000000, /* 1280x720  -  4 lane */
> > >   };
> > >   /* Mode configs */
> > > @@ -259,16 +260,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,
> > >   	},
> > >   };
> > > @@ -442,6 +439,32 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
> > >   	return 0;
> > >   }
> > > +static u8 imx290_get_link_freq_index(struct imx290 *imx290)
> > > +{
> > > +	const struct imx290_mode *cur_mode = imx290->current_mode;
> > > +
> > > +	return (cur_mode->width == 1920) ? 0 : 1;
> > 
> > Could you use (imx290->current_mode - imx290_modes) / sizeof(*imx290_modes)
> > or something like that? It'd have fewer chances of breaking if new modes
> > are added.
> > 
> > > +}
> > > +
> > > +static s64 imx290_get_link_freq(struct imx290 *imx290)
> > > +{
> > > +	u8 index = imx290_get_link_freq_index(imx290);
> > > +
> > > +	if (imx290->nlanes == 4)
> > > +		return imx290_link_freq_4lanes[index];
> > > +	else
> > > +		return imx290_link_freq_2lanes[index];
> > 
> > Or even better: store the link frequencies to the modes themselves. They
> > are a property of the modes after all.
> 
> Then we will get two sets (for 2 lanes and for 4 lanes) of two modes (1080p and 720p), right?

Correct.

> 
> > > +}
> > > +
> > > +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 / 10);
> > > +}
> > > +
> > >   static int imx290_set_fmt(struct v4l2_subdev *sd,
> > >   			  struct v4l2_subdev_pad_config *cfg,
> > >   		      struct v4l2_subdev_format *fmt)
> > > @@ -475,10 +498,14 @@ 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;
> > > +
> > > +		if (imx290->link_freq)
> > > +			__v4l2_ctrl_s_ctrl(imx290->link_freq,
> > > +					   imx290_get_link_freq_index(imx290));
> > > +		if (imx290->pixel_rate)
> > > +			__v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
> > > +						 imx290_calc_pixel_rate(imx290));
> > >   	}
> > >   	*format = fmt->format;
> > > @@ -502,12 +529,11 @@ 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)
> > > +static int imx290_write_current_format(struct imx290 *imx290)
> > >   {
> > >   	int ret;
> > > -	switch (format->code) {
> > > +	switch (imx290->current_format.code) {
> > >   	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > >   		ret = imx290_set_register_array(imx290, imx290_10bit_settings,
> > >   						ARRAY_SIZE(
> > > @@ -558,8 +584,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> > >   		return ret;
> > >   	}
> > > -	/* Set current frame format */
> > > -	ret = imx290_write_current_format(imx290, &imx290->current_format);
> > > +	/* Apply the register values related to current frame format */
> > > +	ret = imx290_write_current_format(imx290);
> > >   	if (ret < 0) {
> > >   		dev_err(imx290->dev, "Could not set frame format\n");
> > >   		return ret;
> > > @@ -821,12 +847,6 @@ static int imx290_probe(struct i2c_client *client)
> > >   		goto free_err;
> > >   	}
> > > -	if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {
> > 
> > This check needs to be modified to correspond to the driver's new
> > capabilities, not removed.
> 
> Agreed.
> Do I understand correct that as the driver uses two link frequencies
> for a given number of lanes now, it must check that *the both* frequencies
> (for the given number of lanes) are listed in the device tree node?

Yes. The smiapp driver does this, for example.

-- 
Sakari Ailus

Re: [PATCH v3 07/10] media: i2c: imx290: Add RAW12 mode support

[Dave Stevenson]

Hi Andrey

Thanks for the patch.

On Sun, 24 May 2020 at 20:26, Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> 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>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/media/i2c/imx290.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 162c345fffac..6e70ff22bc5f 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -71,6 +71,7 @@ struct imx290 {
>         struct clk *xclk;
>         struct regmap *regmap;
>         u8 nlanes;
> +       u8 bpp;
>
>         struct v4l2_subdev sd;
>         struct v4l2_fwnode_endpoint ep;
> @@ -90,10 +91,12 @@ struct imx290 {
>
>  struct imx290_pixfmt {
>         u32 code;
> +       u8 bpp;
>  };
>
>  static const struct imx290_pixfmt imx290_formats[] = {
> -       { MEDIA_BUS_FMT_SRGGB10_1X10 },
> +       { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
> +       { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
>  };
>
>  static const struct regmap_config imx290_regmap_config = {
> @@ -261,6 +264,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_2lanes[] = {
>         891000000, /* 1920x1080 -  2 lane */
> @@ -421,7 +436,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>                 } else {
>                         imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
>                         msleep(10);
> -                       imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
> +                       if (imx290->bpp == 10)
> +                               imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
> +                                                0x3c);
> +                       else /* 12 bits per pixel */
> +                               imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
> +                                                0xf0);
>                         imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
>                 }
>                 break;
> @@ -496,7 +516,7 @@ static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
>         u8 nlanes = imx290->nlanes;
>
>         /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
> -       return (link_freq * 2 * nlanes / 10);
> +       return (link_freq * 2 * nlanes / imx290->bpp);

This doesn't link on a 32bit system as it's a 64bit divide:
ERROR: "__aeabi_ldivmod" [drivers/media/i2c/imx290.ko] undefined!
It ought to be using do_div().

Admittedly it didn't compile before as you still had a s64 divide by
10, but I hadn't tried that :-)

  Dave

>  }
>
>  static int imx290_set_fmt(struct v4l2_subdev *sd,
> @@ -533,6 +553,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>         } else {
>                 format = &imx290->current_format;
>                 imx290->current_mode = mode;
> +               imx290->bpp = imx290_formats[i].bpp;
>
>                 if (imx290->link_freq)
>                         __v4l2_ctrl_s_ctrl(imx290->link_freq,
> @@ -577,6 +598,15 @@ static int imx290_write_current_format(struct imx290 *imx290)
>                         return ret;
>                 }
>                 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;
> +               }
> +               break;
>         default:
>                 dev_err(imx290->dev, "Unknown pixel format\n");
>                 return -EINVAL;
> --
> 2.17.1
>

Re: [PATCH v3 07/10] media: i2c: imx290: Add RAW12 mode support

[Andrey Konovalov]

Hi Dave,

On 26.05.2020 19:05, Dave Stevenson wrote:
> Hi Andrey
> 
> Thanks for the patch.
> 
> On Sun, 24 May 2020 at 20:26, Andrey Konovalov
> <andrey.konovalov@linaro.org> wrote:
>>
>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> 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>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   drivers/media/i2c/imx290.c | 36 +++++++++++++++++++++++++++++++++---
>>   1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
>> index 162c345fffac..6e70ff22bc5f 100644
>> --- a/drivers/media/i2c/imx290.c
>> +++ b/drivers/media/i2c/imx290.c
>> @@ -71,6 +71,7 @@ struct imx290 {
>>          struct clk *xclk;
>>          struct regmap *regmap;
>>          u8 nlanes;
>> +       u8 bpp;
>>
>>          struct v4l2_subdev sd;
>>          struct v4l2_fwnode_endpoint ep;
>> @@ -90,10 +91,12 @@ struct imx290 {
>>
>>   struct imx290_pixfmt {
>>          u32 code;
>> +       u8 bpp;
>>   };
>>
>>   static const struct imx290_pixfmt imx290_formats[] = {
>> -       { MEDIA_BUS_FMT_SRGGB10_1X10 },
>> +       { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
>> +       { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
>>   };
>>
>>   static const struct regmap_config imx290_regmap_config = {
>> @@ -261,6 +264,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_2lanes[] = {
>>          891000000, /* 1920x1080 -  2 lane */
>> @@ -421,7 +436,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
>>                  } else {
>>                          imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
>>                          msleep(10);
>> -                       imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
>> +                       if (imx290->bpp == 10)
>> +                               imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
>> +                                                0x3c);
>> +                       else /* 12 bits per pixel */
>> +                               imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
>> +                                                0xf0);
>>                          imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
>>                  }
>>                  break;
>> @@ -496,7 +516,7 @@ static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
>>          u8 nlanes = imx290->nlanes;
>>
>>          /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
>> -       return (link_freq * 2 * nlanes / 10);
>> +       return (link_freq * 2 * nlanes / imx290->bpp);
> 
> This doesn't link on a 32bit system as it's a 64bit divide:
> ERROR: "__aeabi_ldivmod" [drivers/media/i2c/imx290.ko] undefined!
> It ought to be using do_div().

Nice catch, thanks!
I'll fix this in the next version of the patchset.

Thanks,
Andrey

> Admittedly it didn't compile before as you still had a s64 divide by
> 10, but I hadn't tried that :-)
> 
>    Dave
> 
>>   }
>>
>>   static int imx290_set_fmt(struct v4l2_subdev *sd,
>> @@ -533,6 +553,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
>>          } else {
>>                  format = &imx290->current_format;
>>                  imx290->current_mode = mode;
>> +               imx290->bpp = imx290_formats[i].bpp;
>>
>>                  if (imx290->link_freq)
>>                          __v4l2_ctrl_s_ctrl(imx290->link_freq,
>> @@ -577,6 +598,15 @@ static int imx290_write_current_format(struct imx290 *imx290)
>>                          return ret;
>>                  }
>>                  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;
>> +               }
>> +               break;
>>          default:
>>                  dev_err(imx290->dev, "Unknown pixel format\n");
>>                  return -EINVAL;
>> --
>> 2.17.1
>>

Re: [PATCH v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1467 bytes --]

Hi Andrey,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Andrey-Konovalov/Improvements-to-IMX290-CMOS-driver/20200525-032909
base:   git://linuxtv.org/media_tree.git master
config: microblaze-randconfig-c023-20200527 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

microblaze-linux-ld: drivers/media/i2c/imx290.o: in function `imx290_calc_pixel_rate':
>> drivers/media/i2c/imx290.c:465: undefined reference to `__divdi3'

vim +465 drivers/media/i2c/imx290.c

   458	
   459	static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
   460	{
   461		s64 link_freq = imx290_get_link_freq(imx290);
   462		u8 nlanes = imx290->nlanes;
   463	
   464		/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
 > 465		return (link_freq * 2 * nlanes / 10);
   466	}
   467	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32872 bytes --]

Re: [PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all frame sizes

[Andrey Konovalov]

Hi Sakari,

Thank you for the review!

On 26.05.2020 12:17, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Sun, May 24, 2020 at 10:25:03PM +0300, Andrey Konovalov wrote:
>> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>
>> 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>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@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 6e70ff22bc5f..88850f3b1427 100644
>> --- a/drivers/media/i2c/imx290.c
>> +++ b/drivers/media/i2c/imx290.c
>> @@ -471,6 +471,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;
> 
> Please skip the modes that do not have the code specified by the user. They
> should not be enumerated here.

I've double checked this part of the code, and it doesn't seem to need changes.
The reason is that for the both codes the set of the modes and the frame sizes is
exactly the same. And the fse->code check above just guards against the codes not
supported by the driver at all.

Thanks,
Andrey

>> +
>> +	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)
>> @@ -850,6 +869,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,
>>   };
> 

Re: [PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all frame sizes

[Sakari Ailus]

On Sun, Jun 07, 2020 at 07:28:56PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> Thank you for the review!
> 
> On 26.05.2020 12:17, Sakari Ailus wrote:
> > Hi Andrey,
> > 
> > On Sun, May 24, 2020 at 10:25:03PM +0300, Andrey Konovalov wrote:
> > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > 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>
> > > Signed-off-by: Andrey Konovalov <andrey.konovalov@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 6e70ff22bc5f..88850f3b1427 100644
> > > --- a/drivers/media/i2c/imx290.c
> > > +++ b/drivers/media/i2c/imx290.c
> > > @@ -471,6 +471,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;
> > 
> > Please skip the modes that do not have the code specified by the user. They
> > should not be enumerated here.
> 
> I've double checked this part of the code, and it doesn't seem to need changes.
> The reason is that for the both codes the set of the modes and the frame sizes is
> exactly the same. And the fse->code check above just guards against the codes not
> supported by the driver at all.

Indeed. Please then ignore the comment.

-- 
Sakari Ailus