[PATCH v8 0/4] media: i2c: imx334: support lower bandwidth mode

[PATCH v8 0/4] media: i2c: imx334: support lower bandwidth mode

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Hi

This patch series is for imx334 sensor driver support for lower bandwidth

Some platforms may not be capable of supporting the bandwidth
required for 12 bit or 3840x2160@60 resolutions.

Add support for dynamically selecting 10 bit and 1920x1080@30
resolutions while leaving the existing configuration as default

V7 -> V8
-patch drop "mimx334-odify-link-frequency" as per the commnets
linkfrquncy will be half of the line bandwidth

-changed 1920x1080@30 mode link frequency from (891000000Mbps) 
to (445500000Mbps). linkfrquncy will be half of the line bandwidth


V6 -> V7
Reloved: kernel test robot warning
"drivers/media/i2c/imx334.c:767:15: warning: unused variable 'i' "

V5 -> V6
-Drop the dt-binding patch
-Optimize the code to avoid duplicating the lines
-Added proper mutex while imx334_mbus_codes array
-Modified Function __v4l2_ctrl_modify_range arguments as per the review
commants
-Added hblank dummy set ctrl
-Removed Redundant comment
-corrected code alignment 
-All commit msgs are re-written

V4 -> V5
-Added 5 more patchs as per the review comments witch has below updates
-Updated 1782000000Mbps link frequency for 3840x2160@60 as per the mode
values
-Updated 1782000000Mbps link frequency in dt-bindings also
-Updated 3840x2160@60 mode array with default(reset) values

-Updated hblank __v4l2_ctrl_s_ctrl() to __v4l2_ctrl_modify_range()
Suggested-by: Jacopo Mondi <jacopo@jmondi.org>

-Current mode update only when we try to set V4L2_SUBDEV_FORMAT_ACTIVE
-Added link frequency (891000000Mbps) and pixel rate (74250000) to
1920x1080@30 mode
Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>

-Updated commit message

V3 -> V4
- Make the 12 bit and 3840x2160 as default
- Set bus code SRGGB12 if set format fails

V2 -> V3
- Fixed the warning reported by kernel test robot

V1 -> V2
- Addressed the review comment given by Jacopo Mondi,
  Which has bug in imx334_enum_frame_size() loop function,
- Renamed array codes[] to imx334_mbus_codes[]


Shravan Chippa (5):
  media: i2c: imx334: modify link frequency as for the configureation
  media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to
    __v4l2_ctrl_modify_range
  media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]



Shravan Chippa (4):
  media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to
    __v4l2_ctrl_modify_range
  media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]
  media: i2c: imx334: support lower bandwidth mode
  media: i2c: imx334: update pixel and link frequency

 drivers/media/i2c/imx334.c | 335 ++++++++++++++++++++++++++++++++++---
 1 file changed, 308 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH v8 1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

For evry mode we will get new set of values for hbalnk so use
__v4l2_ctrl_modify_range() to support multi modes for hblank.

The hblank value is readonly in the driver. because of this the function
returns error if we try to change. so added dumy return case in
imx334_set_ctrl function

Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 7b0a9086447d..ebacba3059b3 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334 *imx334,
 	if (ret)
 		return ret;
 
-	ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
+	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
+				       mode->hblank, 1, mode->hblank);
 	if (ret)
 		return ret;
 
@@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		pm_runtime_put(imx334->dev);
 
+		break;
+	case V4L2_CID_HBLANK:
+		ret = 0;
 		break;
 	default:
 		dev_err(imx334->dev, "Invalid control %d", ctrl->id);
-- 
2.34.1

[PATCH v8 2/4] media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

There are some missing reset reg_mode values for the 3840x2160@60
resolution. The camera sensor still works in 3840x2160@60 resolution mode
because of the register reset values. This is an issue when we change the
modes dynamically. As an example, when we change the mode from 1920x1080@30
 resolution to 3840x2160@60 resoultion then the mode values will be written
to the registers from the array mode_3840x2160_regs[] which gives the wrong
output which is incorrect resolution.

So add the missing reset values to the mode_3840x2160_regs[].

Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index ebacba3059b3..a4549d194cae 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -166,6 +166,7 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3288, 0x21},
 	{0x328a, 0x02},
 	{0x302c, 0x3c},
+	{0x302d, 0x00},
 	{0x302e, 0x00},
 	{0x302f, 0x0f},
 	{0x3076, 0x70},
@@ -240,7 +241,26 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3794, 0x7a},
 	{0x3796, 0xa1},
 	{0x3e04, 0x0e},
+	{0x319e, 0x00},
 	{0x3a00, 0x01},
+	{0x3A18, 0xBF},
+	{0x3A19, 0x00},
+	{0x3A1A, 0x67},
+	{0x3A1B, 0x00},
+	{0x3A1C, 0x6F},
+	{0x3A1D, 0x00},
+	{0x3A1E, 0xD7},
+	{0x3A1F, 0x01},
+	{0x3A20, 0x6F},
+	{0x3A21, 0x00},
+	{0x3A22, 0xCF},
+	{0x3A23, 0x00},
+	{0x3A24, 0x6F},
+	{0x3A25, 0x00},
+	{0x3A26, 0xB7},
+	{0x3A27, 0x00},
+	{0x3A28, 0x5F},
+	{0x3A29, 0x00},
 };
 
 /* Supported sensor mode configurations */
-- 
2.34.1

[PATCH v8 3/4] media: i2c: imx334: support lower bandwidth mode

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Some platforms may not be capable of supporting the bandwidth
required for 12 bit or 3840x2160@60 resolutions.

Add support for dynamically selecting 10 bit and 1920x1080@30
resolutions while leaving the existing configuration as default

CC: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 300 +++++++++++++++++++++++++++++++++----
 1 file changed, 274 insertions(+), 26 deletions(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index a4549d194cae..0315e1c9541d 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -79,7 +79,6 @@ struct imx334_reg_list {
  * struct imx334_mode - imx334 sensor mode structure
  * @width: Frame width
  * @height: Frame height
- * @code: Format code
  * @hblank: Horizontal blanking in lines
  * @vblank: Vertical blanking in lines
  * @vblank_min: Minimal vertical blanking in lines
@@ -91,7 +90,6 @@ struct imx334_reg_list {
 struct imx334_mode {
 	u32 width;
 	u32 height;
-	u32 code;
 	u32 hblank;
 	u32 vblank;
 	u32 vblank_min;
@@ -119,6 +117,7 @@ struct imx334_mode {
  * @vblank: Vertical blanking in lines
  * @cur_mode: Pointer to current selected sensor mode
  * @mutex: Mutex for serializing sensor controls
+ * @cur_code: current selected format code
  * @streaming: Flag indicating streaming state
  */
 struct imx334 {
@@ -140,6 +139,7 @@ struct imx334 {
 	u32 vblank;
 	const struct imx334_mode *cur_mode;
 	struct mutex mutex;
+	u32 cur_code;
 	bool streaming;
 };
 
@@ -147,6 +147,169 @@ static const s64 link_freq[] = {
 	IMX334_LINK_FREQ,
 };
 
+/* Sensor mode registers */
+static const struct imx334_reg mode_1920x1080_regs[] = {
+	{0x3000, 0x01},
+	{0x3018, 0x04},
+	{0x3030, 0xCA},
+	{0x3031, 0x08},
+	{0x3032, 0x00},
+	{0x3034, 0x4C},
+	{0x3035, 0x04},
+	{0x302C, 0xF0},
+	{0x302D, 0x03},
+	{0x302E, 0x80},
+	{0x302F, 0x07},
+	{0x3074, 0xCC},
+	{0x3075, 0x02},
+	{0x308E, 0xCD},
+	{0x308F, 0x02},
+	{0x3076, 0x38},
+	{0x3077, 0x04},
+	{0x3090, 0x38},
+	{0x3091, 0x04},
+	{0x3308, 0x38},
+	{0x3309, 0x04},
+	{0x30C6, 0x00},
+	{0x30C7, 0x00},
+	{0x30CE, 0x00},
+	{0x30CF, 0x00},
+	{0x30D8, 0x18},
+	{0x30D9, 0x0A},
+	{0x304C, 0x00},
+	{0x304E, 0x00},
+	{0x304F, 0x00},
+	{0x3050, 0x00},
+	{0x30B6, 0x00},
+	{0x30B7, 0x00},
+	{0x3116, 0x08},
+	{0x3117, 0x00},
+	{0x31A0, 0x20},
+	{0x31A1, 0x0F},
+	{0x300C, 0x3B},
+	{0x300D, 0x29},
+	{0x314C, 0x29},
+	{0x314D, 0x01},
+	{0x315A, 0x06},
+	{0x3168, 0xA0},
+	{0x316A, 0x7E},
+	{0x319E, 0x02},
+	{0x3199, 0x00},
+	{0x319D, 0x00},
+	{0x31DD, 0x03},
+	{0x3300, 0x00},
+	{0x341C, 0xFF},
+	{0x341D, 0x01},
+	{0x3A01, 0x03},
+	{0x3A18, 0x7F},
+	{0x3A19, 0x00},
+	{0x3A1A, 0x37},
+	{0x3A1B, 0x00},
+	{0x3A1C, 0x37},
+	{0x3A1D, 0x00},
+	{0x3A1E, 0xF7},
+	{0x3A1F, 0x00},
+	{0x3A20, 0x3F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x6F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x3F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x5F},
+	{0x3A21, 0x00},
+	{0x3A20, 0x2F},
+	{0x3A21, 0x00},
+	{0x3078, 0x02},
+	{0x3079, 0x00},
+	{0x307A, 0x00},
+	{0x307B, 0x00},
+	{0x3080, 0x02},
+	{0x3081, 0x00},
+	{0x3082, 0x00},
+	{0x3083, 0x00},
+	{0x3088, 0x02},
+	{0x3094, 0x00},
+	{0x3095, 0x00},
+	{0x3096, 0x00},
+	{0x309B, 0x02},
+	{0x309C, 0x00},
+	{0x309D, 0x00},
+	{0x309E, 0x00},
+	{0x30A4, 0x00},
+	{0x30A5, 0x00},
+	{0x3288, 0x21},
+	{0x328A, 0x02},
+	{0x3414, 0x05},
+	{0x3416, 0x18},
+	{0x35AC, 0x0E},
+	{0x3648, 0x01},
+	{0x364A, 0x04},
+	{0x364C, 0x04},
+	{0x3678, 0x01},
+	{0x367C, 0x31},
+	{0x367E, 0x31},
+	{0x3708, 0x02},
+	{0x3714, 0x01},
+	{0x3715, 0x02},
+	{0x3716, 0x02},
+	{0x3717, 0x02},
+	{0x371C, 0x3D},
+	{0x371D, 0x3F},
+	{0x372C, 0x00},
+	{0x372D, 0x00},
+	{0x372E, 0x46},
+	{0x372F, 0x00},
+	{0x3730, 0x89},
+	{0x3731, 0x00},
+	{0x3732, 0x08},
+	{0x3733, 0x01},
+	{0x3734, 0xFE},
+	{0x3735, 0x05},
+	{0x375D, 0x00},
+	{0x375E, 0x00},
+	{0x375F, 0x61},
+	{0x3760, 0x06},
+	{0x3768, 0x1B},
+	{0x3769, 0x1B},
+	{0x376A, 0x1A},
+	{0x376B, 0x19},
+	{0x376C, 0x18},
+	{0x376D, 0x14},
+	{0x376E, 0x0F},
+	{0x3776, 0x00},
+	{0x3777, 0x00},
+	{0x3778, 0x46},
+	{0x3779, 0x00},
+	{0x377A, 0x08},
+	{0x377B, 0x01},
+	{0x377C, 0x45},
+	{0x377D, 0x01},
+	{0x377E, 0x23},
+	{0x377F, 0x02},
+	{0x3780, 0xD9},
+	{0x3781, 0x03},
+	{0x3782, 0xF5},
+	{0x3783, 0x06},
+	{0x3784, 0xA5},
+	{0x3788, 0x0F},
+	{0x378A, 0xD9},
+	{0x378B, 0x03},
+	{0x378C, 0xEB},
+	{0x378D, 0x05},
+	{0x378E, 0x87},
+	{0x378F, 0x06},
+	{0x3790, 0xF5},
+	{0x3792, 0x43},
+	{0x3794, 0x7A},
+	{0x3796, 0xA1},
+	{0x37B0, 0x37},
+	{0x3E04, 0x0E},
+	{0x30E8, 0x50},
+	{0x30E9, 0x00},
+	{0x3E04, 0x0E},
+	{0x3002, 0x00},
+};
+
 /* Sensor mode registers */
 static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3000, 0x01},
@@ -263,20 +426,53 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
 	{0x3A29, 0x00},
 };
 
+static const struct imx334_reg raw10_framefmt_regs[] = {
+	{0x3050, 0x00},
+	{0x319D, 0x00},
+	{0x341C, 0xFF},
+	{0x341D, 0x01},
+};
+
+static const struct imx334_reg raw12_framefmt_regs[] = {
+	{0x3050, 0x01},
+	{0x319D, 0x01},
+	{0x341C, 0x47},
+	{0x341D, 0x00},
+};
+
+static const u32 imx334_mbus_codes[] = {
+	MEDIA_BUS_FMT_SRGGB12_1X12,
+	MEDIA_BUS_FMT_SRGGB10_1X10,
+};
+
 /* Supported sensor mode configurations */
-static const struct imx334_mode supported_mode = {
-	.width = 3840,
-	.height = 2160,
-	.hblank = 560,
-	.vblank = 2340,
-	.vblank_min = 90,
-	.vblank_max = 132840,
-	.pclk = 594000000,
-	.link_freq_idx = 0,
-	.code = MEDIA_BUS_FMT_SRGGB12_1X12,
-	.reg_list = {
-		.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
-		.regs = mode_3840x2160_regs,
+static const struct imx334_mode supported_modes[] = {
+	{
+		.width = 3840,
+		.height = 2160,
+		.hblank = 560,
+		.vblank = 2340,
+		.vblank_min = 90,
+		.vblank_max = 132840,
+		.pclk = 594000000,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
+			.regs = mode_3840x2160_regs,
+		},
+	}, {
+		.width = 1920,
+		.height = 1080,
+		.hblank = 280,
+		.vblank = 1170,
+		.vblank_min = 90,
+		.vblank_max = 132840,
+		.pclk = 74250000,
+		.link_freq_idx = 0,
+		.reg_list = {
+			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
+			.regs = mode_1920x1080_regs,
+		},
 	},
 };
 
@@ -518,6 +714,23 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
 	.s_ctrl = imx334_set_ctrl,
 };
 
+static int imx334_get_format_code(struct imx334 *imx334, u32 code)
+{
+	unsigned int i;
+
+	lockdep_assert_held(&imx334->mutex);
+
+	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
+		if (imx334_mbus_codes[i] == code)
+			break;
+	}
+
+	if (i >= ARRAY_SIZE(imx334_mbus_codes))
+		i = 0;
+
+	return imx334_mbus_codes[i];
+}
+
 /**
  * imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
  * @sd: pointer to imx334 V4L2 sub-device structure
@@ -530,10 +743,10 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->index > 0)
+	if (code->index >= ARRAY_SIZE(imx334_mbus_codes))
 		return -EINVAL;
 
-	code->code = supported_mode.code;
+	code->code = imx334_mbus_codes[code->index];
 
 	return 0;
 }
@@ -550,15 +763,21 @@ static int imx334_enum_frame_size(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_frame_size_enum *fsize)
 {
-	if (fsize->index > 0)
+	struct imx334 *imx334 = to_imx334(sd);
+	u32 code;
+
+	if (fsize->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	if (fsize->code != supported_mode.code)
+	mutex_lock(&imx334->mutex);
+	code = imx334_get_format_code(imx334, fsize->code);
+	mutex_unlock(&imx334->mutex);
+	if (fsize->code != code)
 		return -EINVAL;
 
-	fsize->min_width = supported_mode.width;
+	fsize->min_width = supported_modes[fsize->index].width;
 	fsize->max_width = fsize->min_width;
-	fsize->min_height = supported_mode.height;
+	fsize->min_height = supported_modes[fsize->index].height;
 	fsize->max_height = fsize->min_height;
 
 	return 0;
@@ -577,7 +796,6 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
 {
 	fmt->format.width = mode->width;
 	fmt->format.height = mode->height;
-	fmt->format.code = mode->code;
 	fmt->format.field = V4L2_FIELD_NONE;
 	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
 	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -633,7 +851,13 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&imx334->mutex);
 
-	mode = &supported_mode;
+	fmt->format.code = imx334_get_format_code(imx334, fmt->format.code);
+
+	mode = v4l2_find_nearest_size(supported_modes,
+				      ARRAY_SIZE(supported_modes),
+				      width, height,
+				      fmt->format.width, fmt->format.height);
+
 	imx334_fill_pad_format(imx334, mode, fmt);
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -641,7 +865,8 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
 
 		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
 		*framefmt = fmt->format;
-	} else {
+	} else if (imx334->cur_mode != mode || imx334->cur_code != fmt->format.code) {
+		imx334->cur_code = fmt->format.code;
 		ret = imx334_update_controls(imx334, mode);
 		if (!ret)
 			imx334->cur_mode = mode;
@@ -666,11 +891,26 @@ static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
 	struct v4l2_subdev_format fmt = { 0 };
 
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	imx334_fill_pad_format(imx334, &supported_mode, &fmt);
+	imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
 
 	return imx334_set_pad_format(sd, sd_state, &fmt);
 }
 
+static int imx334_set_framefmt(struct imx334 *imx334)
+{
+	switch (imx334->cur_code) {
+	case MEDIA_BUS_FMT_SRGGB10_1X10:
+		return imx334_write_regs(imx334, raw10_framefmt_regs,
+					ARRAY_SIZE(raw10_framefmt_regs));
+
+	case MEDIA_BUS_FMT_SRGGB12_1X12:
+		return imx334_write_regs(imx334, raw12_framefmt_regs,
+					ARRAY_SIZE(raw12_framefmt_regs));
+	}
+
+	return -EINVAL;
+}
+
 /**
  * imx334_start_streaming() - Start sensor stream
  * @imx334: pointer to imx334 device
@@ -691,6 +931,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
 		return ret;
 	}
 
+	ret = imx334_set_framefmt(imx334);
+	if (ret) {
+		dev_err(imx334->dev, "%s failed to set frame format: %d\n",
+			__func__, ret);
+		return ret;
+	}
+
 	/* Setup handler will write actual exposure and gain */
 	ret =  __v4l2_ctrl_handler_setup(imx334->sd.ctrl_handler);
 	if (ret) {
@@ -1061,7 +1308,8 @@ static int imx334_probe(struct i2c_client *client)
 	}
 
 	/* Set default mode to max resolution */
-	imx334->cur_mode = &supported_mode;
+	imx334->cur_mode = &supported_modes[0];
+	imx334->cur_code = imx334_mbus_codes[0];
 	imx334->vblank = imx334->cur_mode->vblank;
 
 	ret = imx334_init_controls(imx334);
-- 
2.34.1

[PATCH v8 4/4] media: i2c: imx334: update pixel and link frequency

[shravan kumar]

From: Shravan Chippa <shravan.chippa@microchip.com>

Update pixel_rate and link frequency for 1920x1080@30
while changing mode.

Add dummy ctrl cases for pixel_rate and link frequency
to avoid error while changing the modes dynamically

Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 0315e1c9541d..8c3ba660abae 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -50,6 +50,7 @@
 
 /* CSI2 HW configuration */
 #define IMX334_LINK_FREQ	891000000
+#define IMX334_LINK_FREQ_445M	445500000
 #define IMX334_NUM_DATA_LANES	4
 
 #define IMX334_REG_MIN		0x00
@@ -145,6 +146,7 @@ struct imx334 {
 
 static const s64 link_freq[] = {
 	IMX334_LINK_FREQ,
+	IMX334_LINK_FREQ_445M,
 };
 
 /* Sensor mode registers */
@@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[] = {
 		.vblank_min = 90,
 		.vblank_max = 132840,
 		.pclk = 74250000,
-		.link_freq_idx = 0,
+		.link_freq_idx = 1,
 		.reg_list = {
 			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
 			.regs = mode_1920x1080_regs,
@@ -598,6 +600,11 @@ static int imx334_update_controls(struct imx334 *imx334,
 	if (ret)
 		return ret;
 
+	ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
+				       mode->pclk, 1, mode->pclk);
+	if (ret)
+		return ret;
+
 	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
 				       mode->hblank, 1, mode->hblank);
 	if (ret)
@@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
 		pm_runtime_put(imx334->dev);
 
 		break;
+	case V4L2_CID_PIXEL_RATE:
+	case V4L2_CID_LINK_FREQ:
 	case V4L2_CID_HBLANK:
 		ret = 0;
 		break;
-- 
2.34.1

Re: [PATCH v8 1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range

[Jacopo Mondi]

Hi Sharavan,

I'm a bit confused here

On Fri, Jan 06, 2023 at 12:59:28PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> For evry mode we will get new set of values for hbalnk so use
> __v4l2_ctrl_modify_range() to support multi modes for hblank.
>
> The hblank value is readonly in the driver. because of this the function
> returns error if we try to change. so added dumy return case in
> imx334_set_ctrl function
>
> Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 7b0a9086447d..ebacba3059b3 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334 *imx334,
>  	if (ret)
>  		return ret;
>
> -	ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> +	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> +				       mode->hblank, 1, mode->hblank);
>  	if (ret)
>  		return ret;
>
> @@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
>
>  		pm_runtime_put(imx334->dev);
>
> +		break;
> +	case V4L2_CID_HBLANK:
> +		ret = 0;

Hblank is said to be read-only

	if (imx334->hblank_ctrl)
		imx334->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;

So you shouldn't need this safety measure here.

However I see that __v4l2_ctrl_modify_range() can call s_ctrl() if the
current value has to be adjusted to the new limits.

Hans, how does this work ? Do we need the above even if the control is
said to be RO ?

Sharavan: have you experienced failures here, or is this just for
safety ?


>  		break;
>  	default:
>  		dev_err(imx334->dev, "Invalid control %d", ctrl->id);
> --
> 2.34.1
>

Re: [PATCH v8 2/4] media: i2c: imx334: add missing reset values for mode 3840x2160_regs[]

[Jacopo Mondi]

Hi Shravan

On Fri, Jan 06, 2023 at 12:59:29PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> There are some missing reset reg_mode values for the 3840x2160@60
> resolution. The camera sensor still works in 3840x2160@60 resolution mode
> because of the register reset values. This is an issue when we change the
> modes dynamically. As an example, when we change the mode from 1920x1080@30
>  resolution to 3840x2160@60 resoultion then the mode values will be written
 ^ rogue space

> to the registers from the array mode_3840x2160_regs[] which gives the wrong
> output which is incorrect resolution.
>
> So add the missing reset values to the mode_3840x2160_regs[].
>

Are you resetting the registers to their default values, or are they
actually tuned for 3840x2160 operations ?

> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index ebacba3059b3..a4549d194cae 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -166,6 +166,7 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3288, 0x21},
>  	{0x328a, 0x02},
>  	{0x302c, 0x3c},
> +	{0x302d, 0x00},
>  	{0x302e, 0x00},
>  	{0x302f, 0x0f},
>  	{0x3076, 0x70},
> @@ -240,7 +241,26 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3794, 0x7a},
>  	{0x3796, 0xa1},
>  	{0x3e04, 0x0e},
> +	{0x319e, 0x00},
>  	{0x3a00, 0x01},
> +	{0x3A18, 0xBF},
> +	{0x3A19, 0x00},
> +	{0x3A1A, 0x67},
> +	{0x3A1B, 0x00},
> +	{0x3A1C, 0x6F},
> +	{0x3A1D, 0x00},
> +	{0x3A1E, 0xD7},
> +	{0x3A1F, 0x01},
> +	{0x3A20, 0x6F},
> +	{0x3A21, 0x00},
> +	{0x3A22, 0xCF},
> +	{0x3A23, 0x00},
> +	{0x3A24, 0x6F},
> +	{0x3A25, 0x00},
> +	{0x3A26, 0xB7},
> +	{0x3A27, 0x00},
> +	{0x3A28, 0x5F},
> +	{0x3A29, 0x00},

Nit: this is a small bunch of registers, and all the rest of the table
uses lowercase. Please do the same for sake of consistency.

Thanks
  j


>  };
>
>  /* Supported sensor mode configurations */
> --
> 2.34.1
>

Re: [PATCH v8 3/4] media: i2c: imx334: support lower bandwidth mode

[Jacopo Mondi]

Hi Shravan

On Fri, Jan 06, 2023 at 12:59:30PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> Some platforms may not be capable of supporting the bandwidth
> required for 12 bit or 3840x2160@60 resolutions.
>
> Add support for dynamically selecting 10 bit and 1920x1080@30
> resolutions while leaving the existing configuration as default
>
> CC: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Prakash Battu <Prakash.Battu@microchip.com>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 300 +++++++++++++++++++++++++++++++++----
>  1 file changed, 274 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index a4549d194cae..0315e1c9541d 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -79,7 +79,6 @@ struct imx334_reg_list {
>   * struct imx334_mode - imx334 sensor mode structure
>   * @width: Frame width
>   * @height: Frame height
> - * @code: Format code
>   * @hblank: Horizontal blanking in lines
>   * @vblank: Vertical blanking in lines
>   * @vblank_min: Minimal vertical blanking in lines
> @@ -91,7 +90,6 @@ struct imx334_reg_list {
>  struct imx334_mode {
>  	u32 width;
>  	u32 height;
> -	u32 code;
>  	u32 hblank;
>  	u32 vblank;
>  	u32 vblank_min;
> @@ -119,6 +117,7 @@ struct imx334_mode {
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
>   * @mutex: Mutex for serializing sensor controls
> + * @cur_code: current selected format code
>   * @streaming: Flag indicating streaming state
>   */
>  struct imx334 {
> @@ -140,6 +139,7 @@ struct imx334 {
>  	u32 vblank;
>  	const struct imx334_mode *cur_mode;
>  	struct mutex mutex;
> +	u32 cur_code;
>  	bool streaming;
>  };
>
> @@ -147,6 +147,169 @@ static const s64 link_freq[] = {
>  	IMX334_LINK_FREQ,
>  };
>
> +/* Sensor mode registers */
> +static const struct imx334_reg mode_1920x1080_regs[] = {
> +	{0x3000, 0x01},
> +	{0x3018, 0x04},
> +	{0x3030, 0xCA},
> +	{0x3031, 0x08},
> +	{0x3032, 0x00},
> +	{0x3034, 0x4C},
> +	{0x3035, 0x04},
> +	{0x302C, 0xF0},
> +	{0x302D, 0x03},
> +	{0x302E, 0x80},
> +	{0x302F, 0x07},
> +	{0x3074, 0xCC},
> +	{0x3075, 0x02},
> +	{0x308E, 0xCD},
> +	{0x308F, 0x02},
> +	{0x3076, 0x38},
> +	{0x3077, 0x04},
> +	{0x3090, 0x38},
> +	{0x3091, 0x04},
> +	{0x3308, 0x38},
> +	{0x3309, 0x04},
> +	{0x30C6, 0x00},
> +	{0x30C7, 0x00},
> +	{0x30CE, 0x00},
> +	{0x30CF, 0x00},
> +	{0x30D8, 0x18},
> +	{0x30D9, 0x0A},
> +	{0x304C, 0x00},
> +	{0x304E, 0x00},
> +	{0x304F, 0x00},
> +	{0x3050, 0x00},
> +	{0x30B6, 0x00},
> +	{0x30B7, 0x00},
> +	{0x3116, 0x08},
> +	{0x3117, 0x00},
> +	{0x31A0, 0x20},
> +	{0x31A1, 0x0F},
> +	{0x300C, 0x3B},
> +	{0x300D, 0x29},
> +	{0x314C, 0x29},
> +	{0x314D, 0x01},
> +	{0x315A, 0x06},
> +	{0x3168, 0xA0},
> +	{0x316A, 0x7E},
> +	{0x319E, 0x02},
> +	{0x3199, 0x00},
> +	{0x319D, 0x00},
> +	{0x31DD, 0x03},
> +	{0x3300, 0x00},
> +	{0x341C, 0xFF},
> +	{0x341D, 0x01},
> +	{0x3A01, 0x03},
> +	{0x3A18, 0x7F},
> +	{0x3A19, 0x00},
> +	{0x3A1A, 0x37},
> +	{0x3A1B, 0x00},
> +	{0x3A1C, 0x37},
> +	{0x3A1D, 0x00},
> +	{0x3A1E, 0xF7},
> +	{0x3A1F, 0x00},
> +	{0x3A20, 0x3F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x6F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x3F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x5F},
> +	{0x3A21, 0x00},
> +	{0x3A20, 0x2F},
> +	{0x3A21, 0x00},
> +	{0x3078, 0x02},
> +	{0x3079, 0x00},
> +	{0x307A, 0x00},
> +	{0x307B, 0x00},
> +	{0x3080, 0x02},
> +	{0x3081, 0x00},
> +	{0x3082, 0x00},
> +	{0x3083, 0x00},
> +	{0x3088, 0x02},
> +	{0x3094, 0x00},
> +	{0x3095, 0x00},
> +	{0x3096, 0x00},
> +	{0x309B, 0x02},
> +	{0x309C, 0x00},
> +	{0x309D, 0x00},
> +	{0x309E, 0x00},
> +	{0x30A4, 0x00},
> +	{0x30A5, 0x00},
> +	{0x3288, 0x21},
> +	{0x328A, 0x02},
> +	{0x3414, 0x05},
> +	{0x3416, 0x18},
> +	{0x35AC, 0x0E},
> +	{0x3648, 0x01},
> +	{0x364A, 0x04},
> +	{0x364C, 0x04},
> +	{0x3678, 0x01},
> +	{0x367C, 0x31},
> +	{0x367E, 0x31},
> +	{0x3708, 0x02},
> +	{0x3714, 0x01},
> +	{0x3715, 0x02},
> +	{0x3716, 0x02},
> +	{0x3717, 0x02},
> +	{0x371C, 0x3D},
> +	{0x371D, 0x3F},
> +	{0x372C, 0x00},
> +	{0x372D, 0x00},
> +	{0x372E, 0x46},
> +	{0x372F, 0x00},
> +	{0x3730, 0x89},
> +	{0x3731, 0x00},
> +	{0x3732, 0x08},
> +	{0x3733, 0x01},
> +	{0x3734, 0xFE},
> +	{0x3735, 0x05},
> +	{0x375D, 0x00},
> +	{0x375E, 0x00},
> +	{0x375F, 0x61},
> +	{0x3760, 0x06},
> +	{0x3768, 0x1B},
> +	{0x3769, 0x1B},
> +	{0x376A, 0x1A},
> +	{0x376B, 0x19},
> +	{0x376C, 0x18},
> +	{0x376D, 0x14},
> +	{0x376E, 0x0F},
> +	{0x3776, 0x00},
> +	{0x3777, 0x00},
> +	{0x3778, 0x46},
> +	{0x3779, 0x00},
> +	{0x377A, 0x08},
> +	{0x377B, 0x01},
> +	{0x377C, 0x45},
> +	{0x377D, 0x01},
> +	{0x377E, 0x23},
> +	{0x377F, 0x02},
> +	{0x3780, 0xD9},
> +	{0x3781, 0x03},
> +	{0x3782, 0xF5},
> +	{0x3783, 0x06},
> +	{0x3784, 0xA5},
> +	{0x3788, 0x0F},
> +	{0x378A, 0xD9},
> +	{0x378B, 0x03},
> +	{0x378C, 0xEB},
> +	{0x378D, 0x05},
> +	{0x378E, 0x87},
> +	{0x378F, 0x06},
> +	{0x3790, 0xF5},
> +	{0x3792, 0x43},
> +	{0x3794, 0x7A},
> +	{0x3796, 0xA1},
> +	{0x37B0, 0x37},
> +	{0x3E04, 0x0E},
> +	{0x30E8, 0x50},
> +	{0x30E9, 0x00},
> +	{0x3E04, 0x0E},
> +	{0x3002, 0x00},
> +};
> +
>  /* Sensor mode registers */
>  static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3000, 0x01},
> @@ -263,20 +426,53 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
>  	{0x3A29, 0x00},
>  };
>
> +static const struct imx334_reg raw10_framefmt_regs[] = {
> +	{0x3050, 0x00},
> +	{0x319D, 0x00},
> +	{0x341C, 0xFF},
> +	{0x341D, 0x01},
> +};
> +
> +static const struct imx334_reg raw12_framefmt_regs[] = {
> +	{0x3050, 0x01},
> +	{0x319D, 0x01},
> +	{0x341C, 0x47},
> +	{0x341D, 0x00},
> +};
> +
> +static const u32 imx334_mbus_codes[] = {
> +	MEDIA_BUS_FMT_SRGGB12_1X12,
> +	MEDIA_BUS_FMT_SRGGB10_1X10,
> +};
> +
>  /* Supported sensor mode configurations */
> -static const struct imx334_mode supported_mode = {
> -	.width = 3840,
> -	.height = 2160,
> -	.hblank = 560,
> -	.vblank = 2340,
> -	.vblank_min = 90,
> -	.vblank_max = 132840,
> -	.pclk = 594000000,
> -	.link_freq_idx = 0,
> -	.code = MEDIA_BUS_FMT_SRGGB12_1X12,
> -	.reg_list = {
> -		.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
> -		.regs = mode_3840x2160_regs,
> +static const struct imx334_mode supported_modes[] = {
> +	{
> +		.width = 3840,
> +		.height = 2160,
> +		.hblank = 560,
> +		.vblank = 2340,
> +		.vblank_min = 90,
> +		.vblank_max = 132840,
> +		.pclk = 594000000,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
> +			.regs = mode_3840x2160_regs,
> +		},
> +	}, {
> +		.width = 1920,
> +		.height = 1080,
> +		.hblank = 280,
> +		.vblank = 1170,
> +		.vblank_min = 90,
> +		.vblank_max = 132840,
> +		.pclk = 74250000,
> +		.link_freq_idx = 0,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> +			.regs = mode_1920x1080_regs,
> +		},
>  	},
>  };
>
> @@ -518,6 +714,23 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
>  	.s_ctrl = imx334_set_ctrl,
>  };
>
> +static int imx334_get_format_code(struct imx334 *imx334, u32 code)
> +{
> +	unsigned int i;
> +
> +	lockdep_assert_held(&imx334->mutex);
> +
> +	for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
> +		if (imx334_mbus_codes[i] == code)
> +			break;
> +	}
> +
> +	if (i >= ARRAY_SIZE(imx334_mbus_codes))

i can't be larger than ARRAY_SIZE(imx334_mbus_codes)

> +		i = 0;
> +
> +	return imx334_mbus_codes[i];
> +}
> +
>  /**
>   * imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
>   * @sd: pointer to imx334 V4L2 sub-device structure
> @@ -530,10 +743,10 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
>  				 struct v4l2_subdev_state *sd_state,
>  				 struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	if (code->index > 0)
> +	if (code->index >= ARRAY_SIZE(imx334_mbus_codes))
>  		return -EINVAL;
>
> -	code->code = supported_mode.code;
> +	code->code = imx334_mbus_codes[code->index];
>
>  	return 0;
>  }
> @@ -550,15 +763,21 @@ static int imx334_enum_frame_size(struct v4l2_subdev *sd,
>  				  struct v4l2_subdev_state *sd_state,
>  				  struct v4l2_subdev_frame_size_enum *fsize)
>  {
> -	if (fsize->index > 0)
> +	struct imx334 *imx334 = to_imx334(sd);
> +	u32 code;
> +
> +	if (fsize->index >= ARRAY_SIZE(supported_modes))
>  		return -EINVAL;
>
> -	if (fsize->code != supported_mode.code)
> +	mutex_lock(&imx334->mutex);
> +	code = imx334_get_format_code(imx334, fsize->code);
> +	mutex_unlock(&imx334->mutex);

What is this critical section protecting ?

I guess what you want here is just to verify that fsize->code is
supported, right ? imx334_get_format_code() doesn't use run-time
information but just the static list of supported formats, so I guess
you can remove the mutex

This means the lockdep_assert_held() in imx334_get_format_code()
should probably be removed ?

> +	if (fsize->code != code)
>  		return -EINVAL;
>
> -	fsize->min_width = supported_mode.width;
> +	fsize->min_width = supported_modes[fsize->index].width;
>  	fsize->max_width = fsize->min_width;
> -	fsize->min_height = supported_mode.height;
> +	fsize->min_height = supported_modes[fsize->index].height;
>  	fsize->max_height = fsize->min_height;
>
>  	return 0;
> @@ -577,7 +796,6 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
>  {
>  	fmt->format.width = mode->width;
>  	fmt->format.height = mode->height;
> -	fmt->format.code = mode->code;

I guss this breaks get_fmt(), doesn't it ? I might have missed where
fmt->format.code is set in the imx334_get_pad_format() code path


>  	fmt->format.field = V4L2_FIELD_NONE;
>  	fmt->format.colorspace = V4L2_COLORSPACE_RAW;
>  	fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> @@ -633,7 +851,13 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
>
>  	mutex_lock(&imx334->mutex);
>
> -	mode = &supported_mode;
> +	fmt->format.code = imx334_get_format_code(imx334, fmt->format.code);

Maybe you can move this to imx334_fill_pad_format() ? or just assign
fmt->format.code = imx334->cur_code; in get_pad_format() ?

> +
> +	mode = v4l2_find_nearest_size(supported_modes,
> +				      ARRAY_SIZE(supported_modes),
> +				      width, height,
> +				      fmt->format.width, fmt->format.height);
> +
>  	imx334_fill_pad_format(imx334, mode, fmt);
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> @@ -641,7 +865,8 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
>
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>  		*framefmt = fmt->format;
> -	} else {
> +	} else if (imx334->cur_mode != mode || imx334->cur_code != fmt->format.code) {
> +		imx334->cur_code = fmt->format.code;
>  		ret = imx334_update_controls(imx334, mode);
>  		if (!ret)
>  			imx334->cur_mode = mode;
> @@ -666,11 +891,26 @@ static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_subdev_format fmt = { 0 };
>
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> -	imx334_fill_pad_format(imx334, &supported_mode, &fmt);
> +	imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
>
>  	return imx334_set_pad_format(sd, sd_state, &fmt);
>  }
>
> +static int imx334_set_framefmt(struct imx334 *imx334)
> +{
> +	switch (imx334->cur_code) {
> +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> +		return imx334_write_regs(imx334, raw10_framefmt_regs,
> +					ARRAY_SIZE(raw10_framefmt_regs));

                                        ^ This should be aligned to
                                          the open (

my checkpatch doesn't complain, I'm surprised

The rest looks good to me

Thanks
  j

> +
> +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> +		return imx334_write_regs(imx334, raw12_framefmt_regs,
> +					ARRAY_SIZE(raw12_framefmt_regs));
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /**
>   * imx334_start_streaming() - Start sensor stream
>   * @imx334: pointer to imx334 device
> @@ -691,6 +931,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
>  		return ret;
>  	}
>
> +	ret = imx334_set_framefmt(imx334);
> +	if (ret) {
> +		dev_err(imx334->dev, "%s failed to set frame format: %d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
>  	/* Setup handler will write actual exposure and gain */
>  	ret =  __v4l2_ctrl_handler_setup(imx334->sd.ctrl_handler);
>  	if (ret) {
> @@ -1061,7 +1308,8 @@ static int imx334_probe(struct i2c_client *client)
>  	}
>
>  	/* Set default mode to max resolution */
> -	imx334->cur_mode = &supported_mode;
> +	imx334->cur_mode = &supported_modes[0];
> +	imx334->cur_code = imx334_mbus_codes[0];
>  	imx334->vblank = imx334->cur_mode->vblank;
>
>  	ret = imx334_init_controls(imx334);
> --
> 2.34.1
>

Re: [PATCH v8 4/4] media: i2c: imx334: update pixel and link frequency

[Jacopo Mondi]

Hi Shravan

On Fri, Jan 06, 2023 at 12:59:31PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> Update pixel_rate and link frequency for 1920x1080@30
> while changing mode.
>
> Add dummy ctrl cases for pixel_rate and link frequency
> to avoid error while changing the modes dynamically
>
> Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  drivers/media/i2c/imx334.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 0315e1c9541d..8c3ba660abae 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -50,6 +50,7 @@
>
>  /* CSI2 HW configuration */
>  #define IMX334_LINK_FREQ	891000000

I guess you want to rename this one to  IMX334_LINK_FREQ_891M

Give our previous discussion this seems correct for the following mode

	{
		.width = 3840,
		.height = 2160,
		.hblank = 560,
		.vblank = 2340,
		.vblank_min = 90,
		.vblank_max = 132840,
		.pclk = 594000000,
		.link_freq_idx = 0,
		.reg_list = {
			.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
			.regs = mode_3840x2160_regs,
		},
	}, {

        duration: (3840+560) * (2160+2340)  / 594000000 = 33sec = 30FPS
        link_freq (3840+560) * (2160+2340)  * 30 * 12 / 8 = 891000000

Which works well if we use min_vblank = 90 for 60FPS

        duration: (3840+560) * (2160+90)  / 594000000 = 0.16 = 60 FPS
        link_freq (3840+560) * (2160+90)  * 60 * 12 / 8 = 891000000


> +#define IMX334_LINK_FREQ_445M	445500000

But this doesn't work well for me

	{
		.width = 1920,
		.height = 1080,
		.hblank = 280,
		.vblank = 1170,
		.vblank_min = 90,
		.vblank_max = 132840,
		.pclk = 74250000,
		.link_freq_idx = 1,
		.reg_list = {
			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
			.regs = mode_1920x1080_regs,
		},
	},

        duration: (1920+280) * (1080+1170) / 74250000 = 66msec = 16FPS
        link_freq = (1920+280) * (1080+1170) * 60 * 10 / 8 = 371250000

Do you agree with the above or have I missed something ?

I understand you get 30 FPS with the 1920*1080 mode so could you
please check in the newly introduce mode register table what are the
actual values for the blankings and compute the pixel_rate and
link_freq accordingly ?

>  #define IMX334_NUM_DATA_LANES	4
>
>  #define IMX334_REG_MIN		0x00
> @@ -145,6 +146,7 @@ struct imx334 {
>
>  static const s64 link_freq[] = {
>  	IMX334_LINK_FREQ,
> +	IMX334_LINK_FREQ_445M,
>  };
>
>  /* Sensor mode registers */
> @@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[] = {
>  		.vblank_min = 90,
>  		.vblank_max = 132840,
>  		.pclk = 74250000,
> -		.link_freq_idx = 0,
> +		.link_freq_idx = 1,
>  		.reg_list = {
>  			.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>  			.regs = mode_1920x1080_regs,
> @@ -598,6 +600,11 @@ static int imx334_update_controls(struct imx334 *imx334,
>  	if (ret)
>  		return ret;
>
> +	ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> +				       mode->pclk, 1, mode->pclk);
> +	if (ret)
> +		return ret;
> +
>  	ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
>  				       mode->hblank, 1, mode->hblank);
>  	if (ret)
> @@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
>  		pm_runtime_put(imx334->dev);
>
>  		break;
> +	case V4L2_CID_PIXEL_RATE:
> +	case V4L2_CID_LINK_FREQ:
>  	case V4L2_CID_HBLANK:

Same question as for patch 1/4: Do we need these safety checks for
read-only controls ?

Thanks
  j

>  		ret = 0;
>  		break;
> --
> 2.34.1
>

RE: [PATCH v8 1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range

[Shravan.Chippa]

> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Sent: 06 January 2023 03:15 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>; Hans Verkuil
> <hverkuil@xs4all.nl>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH v8 1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to
> __v4l2_ctrl_modify_range
> 
> [You don't often get email from jacopo.mondi@ideasonboard.com. Learn
> why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Sharavan,
> 
> I'm a bit confused here
> 
> On Fri, Jan 06, 2023 at 12:59:28PM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > For evry mode we will get new set of values for hbalnk so use
> > __v4l2_ctrl_modify_range() to support multi modes for hblank.
> >
> > The hblank value is readonly in the driver. because of this the
> > function returns error if we try to change. so added dumy return case
> > in imx334_set_ctrl function
> >
> > Suggested-by: Jacopo Mondi <jacopo@jmondi.org>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 7b0a9086447d..ebacba3059b3 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334
> *imx334,
> >       if (ret)
> >               return ret;
> >
> > -     ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank);
> > +     ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> > +                                    mode->hblank, 1, mode->hblank);
> >       if (ret)
> >               return ret;
> >
> > @@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> >
> >               pm_runtime_put(imx334->dev);
> >
> > +             break;
> > +     case V4L2_CID_HBLANK:
> > +             ret = 0;
> 
> Hblank is said to be read-only

Yes, read-only.
> 
>         if (imx334->hblank_ctrl)
>                 imx334->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> 
> So you shouldn't need this safety measure here.
> 
> However I see that __v4l2_ctrl_modify_range() can call s_ctrl() if the current
> value has to be adjusted to the new limits.
> 
> Hans, how does this work ? Do we need the above even if the control is said
> to be RO ?
> 
> Sharavan: have you experienced failures here, or is this just for safety ?

The value is changing if we change the mode. 
While changing mode. I have experienced failures.

Thanks,
Shravan.

> 
> 
> >               break;
> >       default:
> >               dev_err(imx334->dev, "Invalid control %d", ctrl->id);
> > --
> > 2.34.1
> >

RE: [PATCH v8 4/4] media: i2c: imx334: update pixel and link frequency

[Shravan.Chippa]

> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Sent: 06 January 2023 04:35 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org; Sakari Ailus <sakari.ailus@iki.fi>
> Subject: Re: [PATCH v8 4/4] media: i2c: imx334: update pixel and link frequency
> 
> [You don't often get email from jacopo.mondi@ideasonboard.com. Learn why
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Shravan
> 
> On Fri, Jan 06, 2023 at 12:59:31PM +0530, shravan kumar wrote:
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > Update pixel_rate and link frequency for 1920x1080@30 while changing
> > mode.
> >
> > Add dummy ctrl cases for pixel_rate and link frequency to avoid error
> > while changing the modes dynamically
> >
> > Suggested-by: Sakari Ailus <sakari.ailus@iki.fi>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  drivers/media/i2c/imx334.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 0315e1c9541d..8c3ba660abae 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -50,6 +50,7 @@
> >
> >  /* CSI2 HW configuration */
> >  #define IMX334_LINK_FREQ     891000000
> 
> I guess you want to rename this one to  IMX334_LINK_FREQ_891M
> 
> Give our previous discussion this seems correct for the following mode
> 
>         {
>                 .width = 3840,
>                 .height = 2160,
>                 .hblank = 560,
>                 .vblank = 2340,
>                 .vblank_min = 90,
>                 .vblank_max = 132840,
>                 .pclk = 594000000,
>                 .link_freq_idx = 0,
>                 .reg_list = {
>                         .num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
>                         .regs = mode_3840x2160_regs,
>                 },
>         }, {
> 
>         duration: (3840+560) * (2160+2340)  / 594000000 = 33sec = 30FPS
>         link_freq (3840+560) * (2160+2340)  * 30 * 12 / 8 = 891000000
> 
> Which works well if we use min_vblank = 90 for 60FPS
> 
>         duration: (3840+560) * (2160+90)  / 594000000 = 0.16 = 60 FPS
>         link_freq (3840+560) * (2160+90)  * 60 * 12 / 8 = 891000000
> 
> 
> > +#define IMX334_LINK_FREQ_445M        445500000
> 
> But this doesn't work well for me
> 
>         {
>                 .width = 1920,
>                 .height = 1080,
>                 .hblank = 280,
>                 .vblank = 1170,
>                 .vblank_min = 90,
>                 .vblank_max = 132840,
>                 .pclk = 74250000,
>                 .link_freq_idx = 1,
>                 .reg_list = {
>                         .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
>                         .regs = mode_1920x1080_regs,
>                 },
>         },
> 
>         duration: (1920+280) * (1080+1170) / 74250000 = 66msec = 16FPS
>         link_freq = (1920+280) * (1080+1170) * 60 * 10 / 8 = 371250000
> 
> Do you agree with the above or have I missed something ?
> 
> I understand you get 30 FPS with the 1920*1080 mode so could you please
> check in the newly introduce mode register table what are the actual values for
> the blankings and compute the pixel_rate and link_freq accordingly ?

I will try to correct the hblank and vblank_min.

Thanks,
Shravan

> 
> >  #define IMX334_NUM_DATA_LANES        4
> >
> >  #define IMX334_REG_MIN               0x00
> > @@ -145,6 +146,7 @@ struct imx334 {
> >
> >  static const s64 link_freq[] = {
> >       IMX334_LINK_FREQ,
> > +     IMX334_LINK_FREQ_445M,
> >  };
> >
> >  /* Sensor mode registers */
> > @@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[]
> = {
> >               .vblank_min = 90,
> >               .vblank_max = 132840,
> >               .pclk = 74250000,
> > -             .link_freq_idx = 0,
> > +             .link_freq_idx = 1,
> >               .reg_list = {
> >                       .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> >                       .regs = mode_1920x1080_regs, @@ -598,6 +600,11
> > @@ static int imx334_update_controls(struct imx334 *imx334,
> >       if (ret)
> >               return ret;
> >
> > +     ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> > +                                    mode->pclk, 1, mode->pclk);
> > +     if (ret)
> > +             return ret;
> > +
> >       ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> >                                      mode->hblank, 1, mode->hblank);
> >       if (ret)
> > @@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> >               pm_runtime_put(imx334->dev);
> >
> >               break;
> > +     case V4L2_CID_PIXEL_RATE:
> > +     case V4L2_CID_LINK_FREQ:
> >       case V4L2_CID_HBLANK:
> 
> Same question as for patch 1/4: Do we need these safety checks for read-only
> controls ?
> 
> Thanks
>   j
> 
> >               ret = 0;
> >               break;
> > --
> > 2.34.1
> >