Skip to contentSkip to conversation

media: ov5640: initialize mode data structs by name

PATCH In review Daniel Mack 3 participants 11 messages Apr 20, 2018 – Apr 26, 2018

Subsystems (4)
Device tree bindings Media (V4L/DVB) V4L2 camera sensor drivers Omnivision OV5640 sensor driver
lore
Share & tools
lore

Daniel Mack

author
#0 lore raw

PATCH 1/3 media: ov5640: initialize mode data structs by name

This patch initializes the members of struct ov5640_mode_info by name for better readability. This makes later additions to this struct easier.

No functional change intended.

Signed-off-by Daniel Mack <[email protected]>
---
 drivers/media/i2c/ov5640.c | 207 +++++++++++++++++++++++++++++++++------------
 1 file changed, 152 insertions(+), 55 deletions(-)
drivers/media/i2c/ov5640.c +152 -55
index 852026baa2e7..96f1564abdf5 100644
@@ -728,67 +728,164 @@ static const struct reg_value ov5640_setting_15fps_QSXGA_2592_1944[] = {
 
 /* power-on sensor init reg table */
 static const struct ov5640_mode_info ov5640_mode_init_data = {
-	0, SUBSAMPLING, 640, 480, ov5640_init_setting_30fps_VGA,
-	ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
+	.id		= 0,
+	.dn_mode	= SUBSAMPLING,
+	.width		= 640,
+	.height		= 480,
+	.reg_data	= ov5640_init_setting_30fps_VGA,
+	.reg_data_size	= ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };
 
 static const struct ov5640_mode_info
 ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
+{
+	{
+		.id		= OV5640_MODE_QCIF_176_144,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 176,
+		.height		= 144,
+		.reg_data	= ov5640_setting_15fps_QCIF_176_144,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
+	},
+	{
+		.id		= OV5640_MODE_QVGA_320_240,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 320,
+		.height		= 240,
+		.reg_data	= ov5640_setting_15fps_QVGA_320_240,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
+	},
 	{
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING, 176, 144,
-		 ov5640_setting_15fps_QCIF_176_144,
-		 ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING, 320,  240,
-		 ov5640_setting_15fps_QVGA_320_240,
-		 ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING, 640,  480,
-		 ov5640_setting_15fps_VGA_640_480,
-		 ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING, 720, 480,
-		 ov5640_setting_15fps_NTSC_720_480,
-		 ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING, 720, 576,
-		 ov5640_setting_15fps_PAL_720_576,
-		 ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING, 1024, 768,
-		 ov5640_setting_15fps_XGA_1024_768,
-		 ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 720,
-		 ov5640_setting_15fps_720P_1280_720,
-		 ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING, 1920, 1080,
-		 ov5640_setting_15fps_1080P_1920_1080,
-		 ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, SCALING, 2592, 1944,
-		 ov5640_setting_15fps_QSXGA_2592_1944,
-		 ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944)},
-	}, {
-		{OV5640_MODE_QCIF_176_144, SUBSAMPLING, 176, 144,
-		 ov5640_setting_30fps_QCIF_176_144,
-		 ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144)},
-		{OV5640_MODE_QVGA_320_240, SUBSAMPLING, 320,  240,
-		 ov5640_setting_30fps_QVGA_320_240,
-		 ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240)},
-		{OV5640_MODE_VGA_640_480, SUBSAMPLING, 640,  480,
-		 ov5640_setting_30fps_VGA_640_480,
-		 ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480)},
-		{OV5640_MODE_NTSC_720_480, SUBSAMPLING, 720, 480,
-		 ov5640_setting_30fps_NTSC_720_480,
-		 ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480)},
-		{OV5640_MODE_PAL_720_576, SUBSAMPLING, 720, 576,
-		 ov5640_setting_30fps_PAL_720_576,
-		 ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576)},
-		{OV5640_MODE_XGA_1024_768, SUBSAMPLING, 1024, 768,
-		 ov5640_setting_30fps_XGA_1024_768,
-		 ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768)},
-		{OV5640_MODE_720P_1280_720, SUBSAMPLING, 1280, 720,
-		 ov5640_setting_30fps_720P_1280_720,
-		 ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720)},
-		{OV5640_MODE_1080P_1920_1080, SCALING, 1920, 1080,
-		 ov5640_setting_30fps_1080P_1920_1080,
-		 ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080)},
-		{OV5640_MODE_QSXGA_2592_1944, -1, 0, 0, NULL, 0},
+		.id		= OV5640_MODE_VGA_640_480,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 640,
+		.height		= 480,
+		.reg_data	= ov5640_setting_15fps_VGA_640_480,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)
 	},
+	{
+		.id		= OV5640_MODE_NTSC_720_480,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 720,
+		.height		= 480,
+		.reg_data	= ov5640_setting_15fps_NTSC_720_480,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480),
+	},
+	{
+		.id		= OV5640_MODE_PAL_720_576,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 720,
+		.height		= 576,
+		.reg_data	= ov5640_setting_15fps_PAL_720_576,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576),
+	},
+	{
+		.id		= OV5640_MODE_XGA_1024_768,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 1024,
+		.height		= 768,
+		.reg_data	= ov5640_setting_15fps_XGA_1024_768,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768),
+	},
+	{
+		.id		= OV5640_MODE_720P_1280_720,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 1280,
+		.height		= 720,
+		.reg_data	= ov5640_setting_15fps_720P_1280_720,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720),
+	},
+	{
+		.id		= OV5640_MODE_1080P_1920_1080,
+		.dn_mode	= SCALING,
+		.width		= 1920,
+		.height		= 1080,
+		.reg_data	= ov5640_setting_15fps_1080P_1920_1080,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080),
+	},
+	{
+		.id		= OV5640_MODE_QSXGA_2592_1944,
+		.dn_mode	= SCALING,
+		.width		= 2592,
+		.height		= 1944,
+		.reg_data	= ov5640_setting_15fps_QSXGA_2592_1944,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944),
+	},
+},
+{
+	{
+		.id		= OV5640_MODE_QCIF_176_144,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 176,
+		.height		= 144,
+		.reg_data	= ov5640_setting_30fps_QCIF_176_144,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144),
+	},
+	{
+		.id		= OV5640_MODE_QVGA_320_240,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 320,
+		.height		= 240,
+		.reg_data	= ov5640_setting_30fps_QVGA_320_240,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240),
+	},
+	{
+		.id		= OV5640_MODE_VGA_640_480,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 640,
+		.height		= 480,
+		.reg_data	= ov5640_setting_30fps_VGA_640_480,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480),
+	},
+	{
+		.id		= OV5640_MODE_NTSC_720_480,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 720,
+		.height		= 480,
+		.reg_data	= ov5640_setting_30fps_NTSC_720_480,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480),
+	},
+	{
+		.id		= OV5640_MODE_PAL_720_576,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 720,
+		.height		= 576,
+		.reg_data	= ov5640_setting_30fps_PAL_720_576,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576),
+	},
+	{
+		.id		= OV5640_MODE_XGA_1024_768,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 1024,
+		.height		= 768,
+		.reg_data	= ov5640_setting_30fps_XGA_1024_768,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768),
+	},
+	{
+		.id		= OV5640_MODE_720P_1280_720,
+		.dn_mode	= SUBSAMPLING,
+		.width		= 1280,
+		.height		= 720,
+		.reg_data	= ov5640_setting_30fps_720P_1280_720,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720),
+	},
+	{
+		.id		= OV5640_MODE_1080P_1920_1080,
+		.dn_mode	= SCALING,
+		.width		= 1920,
+		.height		= 1080,
+		.reg_data	= ov5640_setting_30fps_1080P_1920_1080,
+		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080),
+	},
+	{
+		.id		= OV5640_MODE_QSXGA_2592_1944,
+		.dn_mode	= -1,
+		.width		= 0,
+		.height		= 0,
+		.reg_data	= NULL,
+		.reg_data_size	= 0,
+	}
+}
 };
 
 static int ov5640_init_slave_id(struct ov5640_dev *sensor)
Signature · 1 line
2.14.3
author
#1 lore raw

PATCH 2/3 media: ov5640: add PIXEL_RATE and LINK_FREQ controls

Add v4l2 controls to report the pixel and MIPI link rates of each mode. The camss camera subsystem needs them to set up the correct hardware clocks.

Tested on msm8016 based hardware.

Signed-off-by Daniel Mack <[email protected]>
---
 drivers/media/i2c/ov5640.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/media/i2c/ov5640.c +77 -0
index 96f1564abdf5..78669ed386cd 100644
@@ -91,6 +91,20 @@
 #define OV5640_REG_SDE_CTRL5		0x5585
 #define OV5640_REG_AVG_READOUT		0x56a1
 
+#define OV5640_LINK_FREQ_111	0
+#define OV5640_LINK_FREQ_166	1
+#define OV5640_LINK_FREQ_222	2
+#define OV5640_LINK_FREQ_333	3
+#define OV5640_LINK_FREQ_666	4
+
+static const s64 link_freq_menu_items[] = {
+	111066666,
+	166600000,
+	222133333,
+	332200000,
+	666400000,
+};
+
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
 	OV5640_MODE_QVGA_320_240,

@@ -167,12 +181,18 @@ struct ov5640_mode_info {
 	enum ov5640_downsize_mode dn_mode;
 	u32 width;
 	u32 height;
+	u32 pixel_rate;
+	u32 link_freq_idx;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
 
 struct ov5640_ctrls {
 	struct v4l2_ctrl_handler handler;
+	struct {
+		struct v4l2_ctrl *link_freq;
+		struct v4l2_ctrl *pixel_rate;
+	};
 	struct {
 		struct v4l2_ctrl *auto_exp;
 		struct v4l2_ctrl *exposure;

@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 	.dn_mode	= SUBSAMPLING,
 	.width		= 640,
 	.height		= 480,
+	.pixel_rate	= 27766666,
+	.link_freq_idx	= OV5640_LINK_FREQ_111,
 	.reg_data	= ov5640_init_setting_30fps_VGA,
 	.reg_data_size	= ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };

@@ -744,6 +766,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 176,
 		.height		= 144,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_QCIF_176_144,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
 	},

@@ -752,6 +776,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 320,
 		.height		= 240,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_QVGA_320_240,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
 	},

@@ -760,6 +786,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 640,
 		.height		= 480,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_VGA_640_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)
 	},

@@ -768,6 +796,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 480,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_NTSC_720_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480),
 	},

@@ -776,6 +806,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 576,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_PAL_720_576,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576),
 	},

@@ -784,6 +816,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1024,
 		.height		= 768,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_XGA_1024_768,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768),
 	},

@@ -792,6 +826,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1280,
 		.height		= 720,
+		.pixel_rate	= 41650000,
+		.link_freq_idx	= OV5640_LINK_FREQ_166,
 		.reg_data	= ov5640_setting_15fps_720P_1280_720,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720),
 	},

@@ -800,6 +836,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SCALING,
 		.width		= 1920,
 		.height		= 1080,
+		.pixel_rate	= 83300000,
+		.link_freq_idx	= OV5640_LINK_FREQ_333,
 		.reg_data	= ov5640_setting_15fps_1080P_1920_1080,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080),
 	},

@@ -808,6 +846,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SCALING,
 		.width		= 2592,
 		.height		= 1944,
+		.pixel_rate	= 83300000,
+		.link_freq_idx	= OV5640_LINK_FREQ_333,
 		.reg_data	= ov5640_setting_15fps_QSXGA_2592_1944,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944),
 	},

@@ -818,6 +858,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 176,
 		.height		= 144,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_30fps_QCIF_176_144,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144),
 	},

@@ -826,6 +868,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 320,
 		.height		= 240,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_30fps_QVGA_320_240,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240),
 	},

@@ -834,6 +878,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 640,
 		.height		= 480,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_30fps_VGA_640_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480),
 	},

@@ -842,6 +888,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 480,
+		.pixel_rate	= 55533333,
+		.link_freq_idx	= OV5640_LINK_FREQ_222,
 		.reg_data	= ov5640_setting_30fps_NTSC_720_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480),
 	},

@@ -850,6 +898,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 576,
+		.pixel_rate	= 55533333,
+		.link_freq_idx	= OV5640_LINK_FREQ_222,
 		.reg_data	= ov5640_setting_30fps_PAL_720_576,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576),
 	},

@@ -858,6 +908,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1024,
 		.height		= 768,
+		.pixel_rate	= 55533333,
+		.link_freq_idx	= OV5640_LINK_FREQ_222,
 		.reg_data	= ov5640_setting_30fps_XGA_1024_768,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768),
 	},

@@ -866,6 +918,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1280,
 		.height		= 720,
+		.pixel_rate	= 83300000,
+		.link_freq_idx	= OV5640_LINK_FREQ_333,
 		.reg_data	= ov5640_setting_30fps_720P_1280_720,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720),
 	},

@@ -874,6 +928,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SCALING,
 		.width		= 1920,
 		.height		= 1080,
+		.pixel_rate	= 166600000,
+		.link_freq_idx	= OV5640_LINK_FREQ_666,
 		.reg_data	= ov5640_setting_30fps_1080P_1920_1080,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080),
 	},

@@ -882,6 +938,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= -1,
 		.width		= 0,
 		.height		= 0,
+		.pixel_rate	= 0,
+		.link_freq_idx	= 0,
 		.reg_data	= NULL,
 		.reg_data_size	= 0,
 	}

@@ -2024,6 +2082,11 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 	sensor->current_mode = new_mode;
 	sensor->fmt = *mbus_fmt;
 	sensor->pending_mode_change = true;
+
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, new_mode->link_freq_idx);
+	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
+				 new_mode->pixel_rate);
+
 out:
 	mutex_unlock(&sensor->lock);
 	return ret;

@@ -2350,6 +2413,20 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
 	/* we can use our own mutex for the ctrl lock */
 	hdl->lock = &sensor->lock;
 
+	/* Clock related controls */
+	ctrls->link_freq =
+		v4l2_ctrl_new_int_menu(hdl, ops,
+				       V4L2_CID_LINK_FREQ,
+				       ARRAY_SIZE(link_freq_menu_items) - 1,
+				       0, link_freq_menu_items);
+	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	ctrls->pixel_rate =
+		v4l2_ctrl_new_std(hdl, ops,
+				  V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
+				  ov5640_mode_init_data.pixel_rate);
+	ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	/* Auto/manual white balance */
 	ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
 					   V4L2_CID_AUTO_WHITE_BALANCE,
Signature · 1 line
2.14.3
#2 lore raw

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

Hi,

Quoted 56 lines · Daniel Mack

Add v4l2 controls to report the pixel and MIPI link rates of each mode. The camss camera subsystem needs them to set up the correct hardware clocks.

Tested on msm8016 based hardware.

Signed-off-by Daniel Mack <[email protected]>
---
 drivers/media/i2c/ov5640.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/media/i2c/ov5640.c +20 -0
index 96f1564abdf5..78669ed386cd 100644
@@ -91,6 +91,20 @@
 #define OV5640_REG_SDE_CTRL5		0x5585
 #define OV5640_REG_AVG_READOUT		0x56a1
 
+#define OV5640_LINK_FREQ_111	0
+#define OV5640_LINK_FREQ_166	1
+#define OV5640_LINK_FREQ_222	2
+#define OV5640_LINK_FREQ_333	3
+#define OV5640_LINK_FREQ_666	4
+
+static const s64 link_freq_menu_items[] = {
+	111066666,
+	166600000,
+	222133333,
+	332200000,
+	666400000,
+};
+
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
 	OV5640_MODE_QVGA_320_240,

@@ -167,12 +181,18 @@ struct ov5640_mode_info {
 	enum ov5640_downsize_mode dn_mode;
 	u32 width;
 	u32 height;
+	u32 pixel_rate;
+	u32 link_freq_idx;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
 
 struct ov5640_ctrls {
 	struct v4l2_ctrl_handler handler;
+	struct {
+		struct v4l2_ctrl *link_freq;
+		struct v4l2_ctrl *pixel_rate;
+	};
 	struct {
 		struct v4l2_ctrl *auto_exp;
 		struct v4l2_ctrl *exposure;

drivers/media/i2c/ov5640.c static const struct ov5640_mode_info ov5640_mode_init_data = { · line 756

Quoted 5 lines
drivers/media/i2c/ov5640.c +2 -0
@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 	.dn_mode	= SUBSAMPLING,
 	.width		= 640,
 	.height		= 480,
+	.pixel_rate	= 27766666,
+	.link_freq_idx	= OV5640_LINK_FREQ_111,

I'm not sure where this is coming from, but on a parallel sensor I have a quite different pixel rate.

I have a serie ongoing that tries to deal with this, hopefully in order to get rid of all the clock setup done in the initialiasation array.

See https://patchwork.linuxtv.org/patch/48710/ for the patch and
https://www.spinics.net/lists/linux-media/msg132201.html for a discussion on what the clock tree might look like on a MIPI-CSI bus.

Feel free to step in the discussion.
Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
author
#3 lore raw

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

Hi,

Quoted 12 lines · Maxime Ripard
Quoted 10 lines · Daniel Mack

struct ov5640_ctrls {

drivers/media/i2c/ov5640.c +4 -0
 	struct v4l2_ctrl_handler handler;
+	struct {
+		struct v4l2_ctrl *link_freq;
+		struct v4l2_ctrl *pixel_rate;
+	};
 	struct {
 		struct v4l2_ctrl *auto_exp;
 		struct v4l2_ctrl *exposure;

@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
Quoted 8 lines

drivers/media/i2c/ov5640.c static const struct ov5640_mode_info ov5640_mode_init_data = { · line 756

Quoted 5 lines
drivers/media/i2c/ov5640.c +2 -0
 	.dn_mode	= SUBSAMPLING,
 	.width		= 640,
 	.height		= 480,
+	.pixel_rate	= 27766666,
+	.link_freq_idx	= OV5640_LINK_FREQ_111,

I'm not sure where this is coming from, but on a parallel sensor I have a quite different pixel rate.

Ah, interesting. What exactly do you mean by 'parallel'? What kind of module is that, and what are your pixel rates?

Quoted 7 lines

I have a serie ongoing that tries to deal with this, hopefully in order to get rid of all the clock setup done in the initialiasation array.

See https://patchwork.linuxtv.org/patch/48710/ for the patch and
https://www.spinics.net/lists/linux-media/msg132201.html for a discussion on what the clock tree might look like on a MIPI-CSI bus.

Okay, nice. Even better if this patch isn't needed in the end.

Thanks!
Daniel

#4 lore raw

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

Quoted 24 lines · Daniel Mack

Hi,

Quoted 12 lines · Maxime Ripard
Quoted 10 lines · Daniel Mack

struct ov5640_ctrls {

drivers/media/i2c/ov5640.c +4 -0
 	struct v4l2_ctrl_handler handler;
+	struct {
+		struct v4l2_ctrl *link_freq;
+		struct v4l2_ctrl *pixel_rate;
+	};
 	struct {
 		struct v4l2_ctrl *auto_exp;
 		struct v4l2_ctrl *exposure;

@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
Quoted 8 lines

drivers/media/i2c/ov5640.c static const struct ov5640_mode_info ov5640_mode_init_data = { · line 756

Quoted 5 lines
drivers/media/i2c/ov5640.c +2 -0
 	.dn_mode	= SUBSAMPLING,
 	.width		= 640,
 	.height		= 480,
+	.pixel_rate	= 27766666,
+	.link_freq_idx	= OV5640_LINK_FREQ_111,

I'm not sure where this is coming from, but on a parallel sensor I have a quite different pixel rate.

Quoted 2 lines

Ah, interesting. What exactly do you mean by 'parallel'? What kind of module is that, and what are your pixel rates?

An RGB bus, or MIPI-DPI, or basically a pixel clock + 1 line for each bit. The sensor can operate using both that bus and a MIPI-CSI2 one.

You have the list of pixel rates in the patch I've linked before:
https://patchwork.linuxtv.org/patch/48710/

There's one pixel sent per clock cycle, so the pixel rate is the same than the clock rate.

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#5 lore raw

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

In reply to Daniel Mack

Hi Daniel,

Quoted 7 lines · Daniel Mack

Add v4l2 controls to report the pixel and MIPI link rates of each mode. The camss camera subsystem needs them to set up the correct hardware clocks.

Tested on msm8016 based hardware.

Signed-off-by Daniel Mack <[email protected]>

Maxime has written a number of patches against the driver that seem very much related; could you rebase these on his set (v2)?

<URL:https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640>

Quoted 232 lines
---
 drivers/media/i2c/ov5640.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
drivers/media/i2c/ov5640.c +63 -0
index 96f1564abdf5..78669ed386cd 100644
@@ -91,6 +91,20 @@
 #define OV5640_REG_SDE_CTRL5		0x5585
 #define OV5640_REG_AVG_READOUT		0x56a1
 
+#define OV5640_LINK_FREQ_111	0
+#define OV5640_LINK_FREQ_166	1
+#define OV5640_LINK_FREQ_222	2
+#define OV5640_LINK_FREQ_333	3
+#define OV5640_LINK_FREQ_666	4
+
+static const s64 link_freq_menu_items[] = {
+	111066666,
+	166600000,
+	222133333,
+	332200000,
+	666400000,
+};
+
 enum ov5640_mode_id {
 	OV5640_MODE_QCIF_176_144 = 0,
 	OV5640_MODE_QVGA_320_240,

@@ -167,12 +181,18 @@ struct ov5640_mode_info {
 	enum ov5640_downsize_mode dn_mode;
 	u32 width;
 	u32 height;
+	u32 pixel_rate;
+	u32 link_freq_idx;
 	const struct reg_value *reg_data;
 	u32 reg_data_size;
 };
 
 struct ov5640_ctrls {
 	struct v4l2_ctrl_handler handler;
+	struct {
+		struct v4l2_ctrl *link_freq;
+		struct v4l2_ctrl *pixel_rate;
+	};
 	struct {
 		struct v4l2_ctrl *auto_exp;
 		struct v4l2_ctrl *exposure;

@@ -732,6 +752,8 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
 	.dn_mode	= SUBSAMPLING,
 	.width		= 640,
 	.height		= 480,
+	.pixel_rate	= 27766666,
+	.link_freq_idx	= OV5640_LINK_FREQ_111,
 	.reg_data	= ov5640_init_setting_30fps_VGA,
 	.reg_data_size	= ARRAY_SIZE(ov5640_init_setting_30fps_VGA),
 };

@@ -744,6 +766,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 176,
 		.height		= 144,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_QCIF_176_144,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QCIF_176_144),
 	},

@@ -752,6 +776,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 320,
 		.height		= 240,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_QVGA_320_240,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QVGA_320_240),
 	},

@@ -760,6 +786,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 640,
 		.height		= 480,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_VGA_640_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_VGA_640_480)
 	},

@@ -768,6 +796,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 480,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_NTSC_720_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_NTSC_720_480),
 	},

@@ -776,6 +806,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 576,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_PAL_720_576,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_PAL_720_576),
 	},

@@ -784,6 +816,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1024,
 		.height		= 768,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_15fps_XGA_1024_768,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_XGA_1024_768),
 	},

@@ -792,6 +826,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1280,
 		.height		= 720,
+		.pixel_rate	= 41650000,
+		.link_freq_idx	= OV5640_LINK_FREQ_166,
 		.reg_data	= ov5640_setting_15fps_720P_1280_720,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_720P_1280_720),
 	},

@@ -800,6 +836,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SCALING,
 		.width		= 1920,
 		.height		= 1080,
+		.pixel_rate	= 83300000,
+		.link_freq_idx	= OV5640_LINK_FREQ_333,
 		.reg_data	= ov5640_setting_15fps_1080P_1920_1080,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_1080P_1920_1080),
 	},

@@ -808,6 +846,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SCALING,
 		.width		= 2592,
 		.height		= 1944,
+		.pixel_rate	= 83300000,
+		.link_freq_idx	= OV5640_LINK_FREQ_333,
 		.reg_data	= ov5640_setting_15fps_QSXGA_2592_1944,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_15fps_QSXGA_2592_1944),
 	},

@@ -818,6 +858,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 176,
 		.height		= 144,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_30fps_QCIF_176_144,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_QCIF_176_144),
 	},

@@ -826,6 +868,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 320,
 		.height		= 240,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_30fps_QVGA_320_240,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_QVGA_320_240),
 	},

@@ -834,6 +878,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 640,
 		.height		= 480,
+		.pixel_rate	= 27766666,
+		.link_freq_idx	= OV5640_LINK_FREQ_111,
 		.reg_data	= ov5640_setting_30fps_VGA_640_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_VGA_640_480),
 	},

@@ -842,6 +888,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 480,
+		.pixel_rate	= 55533333,
+		.link_freq_idx	= OV5640_LINK_FREQ_222,
 		.reg_data	= ov5640_setting_30fps_NTSC_720_480,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_NTSC_720_480),
 	},

@@ -850,6 +898,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 720,
 		.height		= 576,
+		.pixel_rate	= 55533333,
+		.link_freq_idx	= OV5640_LINK_FREQ_222,
 		.reg_data	= ov5640_setting_30fps_PAL_720_576,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_PAL_720_576),
 	},

@@ -858,6 +908,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1024,
 		.height		= 768,
+		.pixel_rate	= 55533333,
+		.link_freq_idx	= OV5640_LINK_FREQ_222,
 		.reg_data	= ov5640_setting_30fps_XGA_1024_768,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_XGA_1024_768),
 	},

@@ -866,6 +918,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SUBSAMPLING,
 		.width		= 1280,
 		.height		= 720,
+		.pixel_rate	= 83300000,
+		.link_freq_idx	= OV5640_LINK_FREQ_333,
 		.reg_data	= ov5640_setting_30fps_720P_1280_720,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_720P_1280_720),
 	},

@@ -874,6 +928,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= SCALING,
 		.width		= 1920,
 		.height		= 1080,
+		.pixel_rate	= 166600000,
+		.link_freq_idx	= OV5640_LINK_FREQ_666,
 		.reg_data	= ov5640_setting_30fps_1080P_1920_1080,
 		.reg_data_size	= ARRAY_SIZE(ov5640_setting_30fps_1080P_1920_1080),
 	},

@@ -882,6 +938,8 @@ ov5640_mode_data[OV5640_NUM_FRAMERATES][OV5640_NUM_MODES] = {
 		.dn_mode	= -1,
 		.width		= 0,
 		.height		= 0,
+		.pixel_rate	= 0,
+		.link_freq_idx	= 0,
 		.reg_data	= NULL,
 		.reg_data_size	= 0,
 	}

@@ -2024,6 +2082,11 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
 	sensor->current_mode = new_mode;
 	sensor->fmt = *mbus_fmt;
 	sensor->pending_mode_change = true;
+
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.link_freq, new_mode->link_freq_idx);
+	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixel_rate,
+				 new_mode->pixel_rate);
+
 out:
 	mutex_unlock(&sensor->lock);
 	return ret;

drivers/media/i2c/ov5640.c static int ov5640_init_controls(struct ov5640_dev *sensor) · line 2422

Quoted 10 lines
drivers/media/i2c/ov5640.c +7 -0
@@ -2350,6 +2413,20 @@ static int ov5640_init_controls(struct ov5640_dev *sensor)
 	/* we can use our own mutex for the ctrl lock */
 	hdl->lock = &sensor->lock;
 
+	/* Clock related controls */
+	ctrls->link_freq =
+		v4l2_ctrl_new_int_menu(hdl, ops,
+				       V4L2_CID_LINK_FREQ,
+				       ARRAY_SIZE(link_freq_menu_items) - 1,
+				       0, link_freq_menu_items);
+	ctrls->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;

Do make sure creating the control succeeds, i.e. check that ctrls->link_freq is non-NULL before referencing it. Same below.

Quoted 10 lines
drivers/media/i2c/ov5640.c +7 -0
+
+	ctrls->pixel_rate =
+		v4l2_ctrl_new_std(hdl, ops,
+				  V4L2_CID_PIXEL_RATE, 0, INT_MAX, 1,
+				  ov5640_mode_init_data.pixel_rate);
+	ctrls->pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	/* Auto/manual white balance */
 	ctrls->auto_wb = v4l2_ctrl_new_std(hdl, ops,
 					   V4L2_CID_AUTO_WHITE_BALANCE,
Signature · 4 lines
Kind regards,

Sakari Ailus
e-mail: [email protected]
author
#6 lore raw

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

Hi,

Quoted 5 lines · Sakari Ailus
Quoted 4 lines · Daniel Mack

Add v4l2 controls to report the pixel and MIPI link rates of each mode. The camss camera subsystem needs them to set up the correct hardware clocks.

Quoted 8 lines
Quoted 3 lines

Tested on msm8016 based hardware.

Signed-off-by Daniel Mack <[email protected]>

Maxime has written a number of patches against the driver that seem very much related; could you rebase these on his set (v2)?

<URL:https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640>

I didn't know about the ongoing work in this area, so I think both this and 3/3 are not needed. If you want, you can still pick the 1st patch in this series, but that's just a cosmetic cleanup.

Thanks,
Daniel

#7 lore raw

Re: [PATCH 2/3] media: ov5640: add PIXEL_RATE and LINK_FREQ controls

Quoted 12 lines · Daniel Mack

Hi,

Quoted 9 lines · Sakari Ailus
Quoted 7 lines · Daniel Mack

Add v4l2 controls to report the pixel and MIPI link rates of each mode. The camss camera subsystem needs them to set up the correct hardware clocks.

Tested on msm8016 based hardware.

Signed-off-by Daniel Mack <[email protected]>
Quoted 8 lines
Quoted 4 lines

Maxime has written a number of patches against the driver that seem very much related; could you rebase these on his set (v2)?

<URL:https://patchwork.linuxtv.org/project/linux-media/list/?submitter=Maxime+Ripard&state=*&q=ov5640>

I didn't know about the ongoing work in this area, so I think both this and 3/3 are not needed. If you want, you can still pick the 1st patch in this series, but that's just a cosmetic cleanup.

That patch, too, would effectively need a rebase.

I'd also suggest adding a macro that constructs the entries in the array --- much of what changes from entry to entry are fps, width, height and whether subsampling or scaling has been used. That information would likely fit nicely on a single line.

The resolution names are also redundant as the size is explicitly part of the mode list variable names.

Signature · 2 lines
Sakari Ailus
e-mail: [email protected]
author
#8 lore raw

PATCH 3/3 media: ov5640: add support for xclk frequency control

In reply to Daniel Mack

Allow setting the xclk rate via an optional 'clock-frequency' property in the device tree node.

Signed-off-by Daniel Mack <[email protected]>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.txt |  2 ++
 drivers/media/i2c/ov5640.c                             | 10 ++++++++++
 2 files changed, 12 insertions(+)
Documentation/devicetree/bindings/media/i2c/ov5640.txt +2 -0
index 8e36da0d8406..584bbc944978 100644
@@ -13,6 +13,8 @@ Optional Properties:
 	       This is an active low signal to the OV5640.
 - powerdown-gpios: reference to the GPIO connected to the powerdown pin,
 		   if any. This is an active high signal to the OV5640.
+- clock-frequency: frequency to set on the xclk input clock. The clock
+		   is left untouched if this property is missing.
 
 The device node must contain one 'port' child node for its digital output
 video port, in accordance with the video interface bindings defined in
drivers/media/i2c/ov5640.c +10 -0
index 78669ed386cd..2d94d6dbda5d 100644
@@ -2685,6 +2685,7 @@ static int ov5640_probe(struct i2c_client *client,
 	struct fwnode_handle *endpoint;
 	struct ov5640_dev *sensor;
 	struct v4l2_mbus_framefmt *fmt;
+	u32 freq;
 	int ret;
 
 	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);

@@ -2731,6 +2732,15 @@ static int ov5640_probe(struct i2c_client *client,
 		return PTR_ERR(sensor->xclk);
 	}
 
+	ret = of_property_read_u32(dev->of_node, "clock-frequency", &freq);
+	if (ret == 0) {
+		ret = clk_set_rate(sensor->xclk, freq);
+		if (ret) {
+			dev_err(dev, "could not set xclk frequency\n");
+			return ret;
+		}
+	}
+
 	sensor->xclk_freq = clk_get_rate(sensor->xclk);
 	if (sensor->xclk_freq < OV5640_XCLK_MIN ||
 	    sensor->xclk_freq > OV5640_XCLK_MAX) {
Signature · 1 line
2.14.3
#9 lore raw

Re: [PATCH 3/3] media: ov5640: add support for xclk frequency control

Hi,

Quoted 14 lines · Daniel Mack

Allow setting the xclk rate via an optional 'clock-frequency' property in the device tree node.

Signed-off-by Daniel Mack <[email protected]>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.txt |  2 ++
 drivers/media/i2c/ov5640.c                             | 10 ++++++++++
 2 files changed, 12 insertions(+)
Documentation/devicetree/bindings/media/i2c/ov5640.txt +0 -0
index 8e36da0d8406..584bbc944978 100644

Documentation/devicetree/bindings/media/i2c/ov5640.txt Optional Properties: · line 17

Quoted 5 lines
Documentation/devicetree/bindings/media/i2c/ov5640.txt +2 -0
@@ -13,6 +13,8 @@ Optional Properties:
 	       This is an active low signal to the OV5640.
 - powerdown-gpios: reference to the GPIO connected to the powerdown pin,
 		   if any. This is an active high signal to the OV5640.
+- clock-frequency: frequency to set on the xclk input clock. The clock
+		   is left untouched if this property is missing.

This can be done through assigned-clocks, right?

Quoted 17 lines
Documentation/devicetree/bindings/media/i2c/ov5640.txt +0 -0
 The device node must contain one 'port' child node for its digital output
 video port, in accordance with the video interface bindings defined in
drivers/media/i2c/ov5640.c +1 -0
index 78669ed386cd..2d94d6dbda5d 100644
@@ -2685,6 +2685,7 @@ static int ov5640_probe(struct i2c_client *client,
 	struct fwnode_handle *endpoint;
 	struct ov5640_dev *sensor;
 	struct v4l2_mbus_framefmt *fmt;
+	u32 freq;
 	int ret;
 
 	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);

@@ -2731,6 +2732,15 @@ static int ov5640_probe(struct i2c_client *client,
 		return PTR_ERR(sensor->xclk);
 	}

drivers/media/i2c/ov5640.c static int ov5640_probe(struct i2c_client *client, · line 2743

Quoted 10 lines
drivers/media/i2c/ov5640.c +9 -0
 
+	ret = of_property_read_u32(dev->of_node, "clock-frequency", &freq);
+	if (ret == 0) {
+		ret = clk_set_rate(sensor->xclk, freq);
+		if (ret) {
+			dev_err(dev, "could not set xclk frequency\n");
+			return ret;
+		}
+	}
+

I'm wondering what the use case for that would be. The clock rate is subject to various changes depending on the resolution and framerate used, so that's very likely to change. Wouldn't we be better off to simply try to change the rate at runtime, depending on those factors?

Maxime

Signature · 3 lines
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
#10 lore raw

Re: [PATCH 3/3] media: ov5640: add support for xclk frequency control

In reply to Daniel Mack
Quoted 35 lines · Daniel Mack

Allow setting the xclk rate via an optional 'clock-frequency' property in the device tree node.

Signed-off-by Daniel Mack <[email protected]>
---
 Documentation/devicetree/bindings/media/i2c/ov5640.txt |  2 ++
 drivers/media/i2c/ov5640.c                             | 10 ++++++++++
 2 files changed, 12 insertions(+)
Documentation/devicetree/bindings/media/i2c/ov5640.txt +2 -0
index 8e36da0d8406..584bbc944978 100644
@@ -13,6 +13,8 @@ Optional Properties:
 	       This is an active low signal to the OV5640.
 - powerdown-gpios: reference to the GPIO connected to the powerdown pin,
 		   if any. This is an active high signal to the OV5640.
+- clock-frequency: frequency to set on the xclk input clock. The clock
+		   is left untouched if this property is missing.
 
 The device node must contain one 'port' child node for its digital output
 video port, in accordance with the video interface bindings defined in
drivers/media/i2c/ov5640.c +1 -0
index 78669ed386cd..2d94d6dbda5d 100644
@@ -2685,6 +2685,7 @@ static int ov5640_probe(struct i2c_client *client,
 	struct fwnode_handle *endpoint;
 	struct ov5640_dev *sensor;
 	struct v4l2_mbus_framefmt *fmt;
+	u32 freq;
 	int ret;
 
 	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);

drivers/media/i2c/ov5640.c static int ov5640_probe(struct i2c_client *client, · line 2735

Quoted 4 lines
drivers/media/i2c/ov5640.c +1 -0
@@ -2731,6 +2732,15 @@ static int ov5640_probe(struct i2c_client *client,
 		return PTR_ERR(sensor->xclk);
 	}
 
+	ret = of_property_read_u32(dev->of_node, "clock-frequency", &freq);

Please use

	ret = fwnode_property_read_u32(dev_fwnode(dev), ...);

Thanks.

Quoted 14 lines
drivers/media/i2c/ov5640.c +8 -0
+	if (ret == 0) {
+		ret = clk_set_rate(sensor->xclk, freq);
+		if (ret) {
+			dev_err(dev, "could not set xclk frequency\n");
+			return ret;
+		}
+	}
+
 	sensor->xclk_freq = clk_get_rate(sensor->xclk);
 	if (sensor->xclk_freq < OV5640_XCLK_MIN ||
 	    sensor->xclk_freq > OV5640_XCLK_MAX) {
Signature · 1 line
2.14.3
Signature · 2 lines
Sakari Ailus
e-mail: [email protected]