[PATCH 0/4] media: mt9m111 features

[PATCH 0/4] media: mt9m111 features

[Marco Felsch]

Hello,

the purpose of this series is to support the pixclk polarity and the
framerate selection. I picked the patches form Michael and Enrico,
ported them to 4.19 and did some adjustments.

I tested the framrate and pixckl selection on a custom arm based board.

Enrico Scholz (1):
  media: mt9m111: allow to setup pixclk polarity

Marco Felsch (1):
  media: mt9m111: add s_stream callback

Michael Grzeschik (2):
  media: mt9m111: add streaming check to set_fmt
  media: mt9m111: add support to select formats and fps for {Q,SXGA}

 drivers/media/i2c/mt9m111.c | 221 +++++++++++++++++++++++++++++++++++-
 1 file changed, 220 insertions(+), 1 deletion(-)

-- 
2.19.0

[PATCH 1/4] media: mt9m111: add s_stream callback

[Marco Felsch]

Add callback to check if we are already streaming. Now other callbacks
can check the state and return -EBUSY if we already streaming.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index efda1aa95ca0..78d08a81e0e2 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -217,6 +217,7 @@ struct mt9m111 {
 	int power_count;
 	const struct mt9m111_datafmt *fmt;
 	int lastpage;	/* PageMap cache value */
+	bool is_streaming;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pad;
 #endif
@@ -880,6 +881,14 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+
+	mt9m111->is_streaming = !!enable;
+	return 0;
+}
+
 static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
@@ -893,6 +902,7 @@ static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
 
 static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = {
 	.g_mbus_config	= mt9m111_g_mbus_config,
+	.s_stream	= mt9m111_s_stream,
 };
 
 static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = {
-- 
2.19.0

[PATCH 2/4] media: mt9m111: add streaming check to set_fmt

[Marco Felsch]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

Currently set_fmt don't care about the streaming status, so the format
can be changed during streaming. This can lead into wrong behaviours.

Check if the device is already streaming and return -EBUSY to avoid
wrong behaviours.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 78d08a81e0e2..d060075a670b 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -561,6 +561,9 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 	bool bayer;
 	int ret;
 
+	if (mt9m111->is_streaming)
+		return -EBUSY;
+
 	if (format->pad)
 		return -EINVAL;
 
-- 
2.19.0

[PATCH 3/4] media: mt9m111: add support to select formats and fps for {Q,SXGA}

[Marco Felsch]

From: Michael Grzeschik <m.grzeschik@pengutronix.de>

This patch implements the framerate selection using the skipping and
readout power-modi features. The power-modi cut the framerate by half
and each context has an independent selection bit. The same applies to
the 2x skipping feature.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 156 ++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index d060075a670b..13080c6c1ba3 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -204,6 +204,22 @@ static const struct mt9m111_datafmt mt9m111_colour_fmts[] = {
 	{MEDIA_BUS_FMT_SBGGR10_2X8_PADHI_LE, V4L2_COLORSPACE_SRGB},
 };
 
+enum mt9m111_mode_id {
+	MT9M111_MODE_SXGA_8FPS,
+	MT9M111_MODE_SXGA_15FPS,
+	MT9M111_MODE_QSXGA_30FPS,
+	MT9M111_NUM_MODES,
+};
+
+struct mt9m111_mode_info {
+	unsigned int sensor_w;
+	unsigned int sensor_h;
+	unsigned int max_image_w;
+	unsigned int max_image_h;
+	unsigned int max_fps;
+	unsigned int reg_val;
+};
+
 struct mt9m111 {
 	struct v4l2_subdev subdev;
 	struct v4l2_ctrl_handler hdl;
@@ -213,6 +229,8 @@ struct mt9m111 {
 	struct v4l2_clk *clk;
 	unsigned int width;	/* output */
 	unsigned int height;	/* sizes */
+	struct v4l2_fract frame_interval;
+	const struct mt9m111_mode_info *current_mode;
 	struct mutex power_lock; /* lock to protect power_count */
 	int power_count;
 	const struct mt9m111_datafmt *fmt;
@@ -223,6 +241,34 @@ struct mt9m111 {
 #endif
 };
 
+static const struct mt9m111_mode_info mt9m111_mode_data[MT9M111_NUM_MODES] = {
+	[MT9M111_MODE_SXGA_8FPS] = {
+		.sensor_w = 1280,
+		.sensor_h = 1024,
+		.max_image_w = 1280,
+		.max_image_h = 1024,
+		.max_fps = 8,
+		.reg_val = MT9M111_RM_LOW_POWER_RD,
+	},
+	[MT9M111_MODE_SXGA_15FPS] = {
+		.sensor_w = 1280,
+		.sensor_h = 1024,
+		.max_image_w = 1280,
+		.max_image_h = 1024,
+		.max_fps = 15,
+		.reg_val = MT9M111_RM_FULL_POWER_RD,
+	},
+	[MT9M111_MODE_QSXGA_30FPS] = {
+		.sensor_w = 1280,
+		.sensor_h = 1024,
+		.max_image_w = 640,
+		.max_image_h = 512,
+		.max_fps = 30,
+		.reg_val = MT9M111_RM_LOW_POWER_RD | MT9M111_RM_COL_SKIP_2X |
+			   MT9M111_RM_ROW_SKIP_2X,
+	},
+};
+
 /* Find a data format by a pixel code */
 static const struct mt9m111_datafmt *mt9m111_find_datafmt(struct mt9m111 *mt9m111,
 						u32 code)
@@ -616,6 +662,62 @@ static int mt9m111_set_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static const struct mt9m111_mode_info *
+mt9m111_find_mode(struct mt9m111 *mt9m111, unsigned int req_fps,
+		  unsigned int width, unsigned int height)
+{
+	const struct mt9m111_mode_info *mode;
+	struct v4l2_rect *sensor_rect = &mt9m111->rect;
+	unsigned int gap, gap_best = (unsigned int) -1;
+	int i, best_gap_idx = 1;
+
+	/* find best matched fps */
+	for (i = 0; i < MT9M111_NUM_MODES; i++) {
+		unsigned int fps = mt9m111_mode_data[i].max_fps;
+
+		gap = abs(fps - req_fps);
+		if (gap < gap_best) {
+			best_gap_idx = i;
+			gap_best = gap;
+		}
+	}
+
+	/*
+	 * Use context a/b default timing values instead of calculate blanking
+	 * timing values.
+	 */
+	mode = &mt9m111_mode_data[best_gap_idx];
+	mt9m111->ctx = (best_gap_idx == MT9M111_MODE_QSXGA_30FPS) ? &context_a :
+								    &context_b;
+
+	/*
+	 * Check if current settings support the fps because fps selection is
+	 * based on the row/col skipping mechanism which has some restriction.
+	 */
+	if (sensor_rect->width != mode->sensor_w ||
+	    sensor_rect->height != mode->sensor_h ||
+	    width > mode->max_image_w ||
+	    height > mode->max_image_h) {
+		/* reset sensor window size */
+		mt9m111->rect.left = MT9M111_MIN_DARK_COLS;
+		mt9m111->rect.top = MT9M111_MIN_DARK_ROWS;
+		mt9m111->rect.width = mode->sensor_w;
+		mt9m111->rect.height = mode->sensor_h;
+
+		/* reset image size */
+		mt9m111->width = mode->max_image_w;
+		mt9m111->height = mode->max_image_h;
+
+		dev_warn(mt9m111->subdev.dev,
+			 "Warning: update image size %dx%d[%dx%d] -> %dx%d[%dx%d]\n",
+			 sensor_rect->width, sensor_rect->height, width, height,
+			 mode->sensor_w, mode->sensor_h, mode->max_image_w,
+			 mode->max_image_h);
+	}
+
+	return mode;
+}
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int mt9m111_g_register(struct v4l2_subdev *sd,
 			      struct v4l2_dbg_register *reg)
@@ -776,11 +878,15 @@ static int mt9m111_suspend(struct mt9m111 *mt9m111)
 
 static void mt9m111_restore_state(struct mt9m111 *mt9m111)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9m111->subdev);
+
 	mt9m111_set_context(mt9m111, mt9m111->ctx);
 	mt9m111_set_pixfmt(mt9m111, mt9m111->fmt->code);
 	mt9m111_setup_geometry(mt9m111, &mt9m111->rect,
 			mt9m111->width, mt9m111->height, mt9m111->fmt->code);
 	v4l2_ctrl_handler_setup(&mt9m111->hdl);
+	mt9m111_reg_write(client, mt9m111->ctx->read_mode,
+			  mt9m111->current_mode->reg_val);
 }
 
 static int mt9m111_resume(struct mt9m111 *mt9m111)
@@ -873,6 +979,50 @@ static const struct v4l2_subdev_core_ops mt9m111_subdev_core_ops = {
 #endif
 };
 
+static int mt9m111_g_frame_interval(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_frame_interval *fi)
+{
+	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+
+	fi->interval = mt9m111->frame_interval;
+
+	return 0;
+}
+
+static int mt9m111_s_frame_interval(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_frame_interval *fi)
+{
+	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+	const struct mt9m111_mode_info *mode;
+	struct v4l2_fract *fract = &fi->interval;
+	int fps;
+
+	if (mt9m111->is_streaming)
+		return -EBUSY;
+
+	if (fi->pad != 0)
+		return -EINVAL;
+
+	if (fract->numerator == 0) {
+		fract->denominator = 30;
+		fract->numerator = 1;
+	}
+
+	fps = DIV_ROUND_CLOSEST(fract->denominator, fract->numerator);
+
+	/* find best fitting mode */
+	mode = mt9m111_find_mode(mt9m111, fps, mt9m111->width, mt9m111->height);
+	if (mode->max_fps != fps) {
+		fract->denominator = mode->max_fps;
+		fract->numerator = 1;
+	}
+
+	mt9m111->current_mode = mode;
+	mt9m111->frame_interval = fi->interval;
+
+	return 0;
+}
+
 static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 		struct v4l2_subdev_pad_config *cfg,
 		struct v4l2_subdev_mbus_code_enum *code)
@@ -906,6 +1056,8 @@ static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
 static const struct v4l2_subdev_video_ops mt9m111_subdev_video_ops = {
 	.g_mbus_config	= mt9m111_g_mbus_config,
 	.s_stream	= mt9m111_s_stream,
+	.g_frame_interval = mt9m111_g_frame_interval,
+	.s_frame_interval = mt9m111_s_frame_interval,
 };
 
 static const struct v4l2_subdev_pad_ops mt9m111_subdev_pad_ops = {
@@ -1022,6 +1174,10 @@ static int mt9m111_probe(struct i2c_client *client,
 		goto out_hdlfree;
 #endif
 
+	mt9m111->current_mode = &mt9m111_mode_data[MT9M111_MODE_SXGA_15FPS];
+	mt9m111->frame_interval.numerator = 1;
+	mt9m111->frame_interval.denominator = mt9m111->current_mode->max_fps;
+
 	/* Second stage probe - when a capture adapter is there */
 	mt9m111->rect.left	= MT9M111_MIN_DARK_COLS;
 	mt9m111->rect.top	= MT9M111_MIN_DARK_ROWS;
-- 
2.19.0

[PATCH 4/4] media: mt9m111: allow to setup pixclk polarity

[Marco Felsch]

From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>

The chip can be configured to output data transitions on the
rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
falling edge.

Parsing the fw-node is made in a subfunction to bundle all (future)
dt-parsing / fw-parsing stuff.

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
(m.grzeschik@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
per default. Set bit to 0 (enable mask bit without value) to enable
falling edge sampling.)
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
(m.felsch@pengutronix.de: use fwnode helpers)
(m.felsch@pengutronix.de: mv of parsing into own function)
(m.felsch@pengutronix.de: adapt commit msg)
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/i2c/mt9m111.c | 52 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 13080c6c1ba3..5d45bc9ea0cb 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -15,12 +15,14 @@
 #include <linux/delay.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/module.h>
+#include <linux/of_graph.h>
 
 #include <media/v4l2-async.h>
 #include <media/v4l2-clk.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 /*
  * MT9M111, MT9M112 and MT9M131:
@@ -236,6 +238,8 @@ struct mt9m111 {
 	const struct mt9m111_datafmt *fmt;
 	int lastpage;	/* PageMap cache value */
 	bool is_streaming;
+	/* user point of view - 0: falling 1: rising edge */
+	unsigned int pclk_sample:1;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pad;
 #endif
@@ -586,6 +590,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
 		return -EINVAL;
 	}
 
+	/* receiver samples on falling edge, chip-hw default is rising */
+	if (mt9m111->pclk_sample == 0)
+		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
+
 	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
 			       data_outfmt2, mask_outfmt2);
 	if (!ret)
@@ -1045,9 +1053,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
 static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
 				struct v4l2_mbus_config *cfg)
 {
-	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
+	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
+
+	cfg->flags = V4L2_MBUS_MASTER |
 		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
 		V4L2_MBUS_DATA_ACTIVE_HIGH;
+
+	cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_FALLING :
+		V4L2_MBUS_PCLK_SAMPLE_RISING;
+
 	cfg->type = V4L2_MBUS_PARALLEL;
 
 	return 0;
@@ -1117,6 +1131,33 @@ static int mt9m111_video_probe(struct i2c_client *client)
 	return ret;
 }
 
+#ifdef CONFIG_OF
+static int mt9m111_probe_of(struct i2c_client *client, struct mt9m111 *mt9m111)
+{
+	struct v4l2_fwnode_endpoint *bus_cfg;
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
+	if (!np)
+		return -EINVAL;
+
+	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np));
+	if (IS_ERR(bus_cfg)) {
+		ret = PTR_ERR(bus_cfg);
+		goto out_of_put;
+	}
+
+	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+				  V4L2_MBUS_PCLK_SAMPLE_RISING);
+
+	v4l2_fwnode_endpoint_free(bus_cfg);
+out_of_put:
+	of_node_put(np);
+	return ret;
+}
+#endif
+
 static int mt9m111_probe(struct i2c_client *client,
 			 const struct i2c_device_id *did)
 {
@@ -1141,6 +1182,15 @@ static int mt9m111_probe(struct i2c_client *client,
 	/* Default HIGHPOWER context */
 	mt9m111->ctx = &context_b;
 
+	if (IS_ENABLED(CONFIG_OF)) {
+		ret = mt9m111_probe_of(client, mt9m111);
+		if (ret)
+			return ret;
+	} else {
+		/* use default chip hardware values */
+		mt9m111->pclk_sample = 1;
+	}
+
 	v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
 	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-- 
2.19.0

Re: [PATCH 4/4] media: mt9m111: allow to setup pixclk polarity

[kbuild test robot]

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

Hi Enrico,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.19-rc8 next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Marco-Felsch/media-mt9m111-features/20181020-022716
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x000-201841 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/media//i2c/mt9m111.c: In function 'mt9m111_probe':
>> drivers/media//i2c/mt9m111.c:1185:9: error: implicit declaration of function 'mt9m111_probe_of'; did you mean 'mt9m111_probe'? [-Werror=implicit-function-declaration]
      ret = mt9m111_probe_of(client, mt9m111);
            ^~~~~~~~~~~~~~~~
            mt9m111_probe
   cc1: some warnings being treated as errors

vim +1185 drivers/media//i2c/mt9m111.c

  1159	
  1160	static int mt9m111_probe(struct i2c_client *client,
  1161				 const struct i2c_device_id *did)
  1162	{
  1163		struct mt9m111 *mt9m111;
  1164		struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
  1165		int ret;
  1166	
  1167		if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
  1168			dev_warn(&adapter->dev,
  1169				 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
  1170			return -EIO;
  1171		}
  1172	
  1173		mt9m111 = devm_kzalloc(&client->dev, sizeof(struct mt9m111), GFP_KERNEL);
  1174		if (!mt9m111)
  1175			return -ENOMEM;
  1176	
  1177		mt9m111->clk = v4l2_clk_get(&client->dev, "mclk");
  1178		if (IS_ERR(mt9m111->clk))
  1179			return PTR_ERR(mt9m111->clk);
  1180	
  1181		/* Default HIGHPOWER context */
  1182		mt9m111->ctx = &context_b;
  1183	
  1184		if (IS_ENABLED(CONFIG_OF)) {
> 1185			ret = mt9m111_probe_of(client, mt9m111);
  1186			if (ret)
  1187				return ret;
  1188		} else {
  1189			/* use default chip hardware values */
  1190			mt9m111->pclk_sample = 1;
  1191		}
  1192	
  1193		v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
  1194		mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
  1195	
  1196		v4l2_ctrl_handler_init(&mt9m111->hdl, 5);
  1197		v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
  1198				V4L2_CID_VFLIP, 0, 1, 1, 0);
  1199		v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
  1200				V4L2_CID_HFLIP, 0, 1, 1, 0);
  1201		v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
  1202				V4L2_CID_AUTO_WHITE_BALANCE, 0, 1, 1, 1);
  1203		mt9m111->gain = v4l2_ctrl_new_std(&mt9m111->hdl, &mt9m111_ctrl_ops,
  1204				V4L2_CID_GAIN, 0, 63 * 2 * 2, 1, 32);
  1205		v4l2_ctrl_new_std_menu(&mt9m111->hdl,
  1206				&mt9m111_ctrl_ops, V4L2_CID_EXPOSURE_AUTO, 1, 0,
  1207				V4L2_EXPOSURE_AUTO);
  1208		v4l2_ctrl_new_std_menu_items(&mt9m111->hdl,
  1209				&mt9m111_ctrl_ops, V4L2_CID_TEST_PATTERN,
  1210				ARRAY_SIZE(mt9m111_test_pattern_menu) - 1, 0, 0,
  1211				mt9m111_test_pattern_menu);
  1212		mt9m111->subdev.ctrl_handler = &mt9m111->hdl;
  1213		if (mt9m111->hdl.error) {
  1214			ret = mt9m111->hdl.error;
  1215			goto out_clkput;
  1216		}
  1217	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

Re: [PATCH 4/4] media: mt9m111: allow to setup pixclk polarity

[Sakari Ailus]

Hi Marco,

Thanks for the patchset.

On Fri, Oct 19, 2018 at 05:50:27PM +0200, Marco Felsch wrote:
> From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> 
> The chip can be configured to output data transitions on the
> rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> falling edge.
> 
> Parsing the fw-node is made in a subfunction to bundle all (future)
> dt-parsing / fw-parsing stuff.

Could you rebase this on current mediatree master, please?

> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> (m.grzeschik@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> per default. Set bit to 0 (enable mask bit without value) to enable
> falling edge sampling.)
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> (m.felsch@pengutronix.de: use fwnode helpers)
> (m.felsch@pengutronix.de: mv of parsing into own function)
> (m.felsch@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/i2c/mt9m111.c | 52 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 13080c6c1ba3..5d45bc9ea0cb 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -15,12 +15,14 @@
>  #include <linux/delay.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/module.h>
> +#include <linux/of_graph.h>
>  
>  #include <media/v4l2-async.h>
>  #include <media/v4l2-clk.h>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
>  
>  /*
>   * MT9M111, MT9M112 and MT9M131:
> @@ -236,6 +238,8 @@ struct mt9m111 {
>  	const struct mt9m111_datafmt *fmt;
>  	int lastpage;	/* PageMap cache value */
>  	bool is_streaming;
> +	/* user point of view - 0: falling 1: rising edge */
> +	unsigned int pclk_sample:1;
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_pad pad;
>  #endif
> @@ -586,6 +590,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
>  		return -EINVAL;
>  	}
>  
> +	/* receiver samples on falling edge, chip-hw default is rising */

Could you add DT binding documentation that would cover this? The existing
documentation is, well, rather vague. Which properties are relevant for the
hardware, are they mandatory or optional and if they're optional, then are
there relevant default values?

> +	if (mt9m111->pclk_sample == 0)
> +		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> +
>  	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
>  			       data_outfmt2, mask_outfmt2);
>  	if (!ret)
> @@ -1045,9 +1053,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
>  static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
>  				struct v4l2_mbus_config *cfg)
>  {
> -	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> +	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> +
> +	cfg->flags = V4L2_MBUS_MASTER |
>  		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
>  		V4L2_MBUS_DATA_ACTIVE_HIGH;
> +
> +	cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_FALLING :
> +		V4L2_MBUS_PCLK_SAMPLE_RISING;
> +
>  	cfg->type = V4L2_MBUS_PARALLEL;
>  
>  	return 0;
> @@ -1117,6 +1131,33 @@ static int mt9m111_video_probe(struct i2c_client *client)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_OF
> +static int mt9m111_probe_of(struct i2c_client *client, struct mt9m111 *mt9m111)
> +{
> +	struct v4l2_fwnode_endpoint *bus_cfg;
> +	struct device_node *np;
> +	int ret = 0;
> +
> +	np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> +	if (!np)
> +		return -EINVAL;
> +
> +	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np));
> +	if (IS_ERR(bus_cfg)) {
> +		ret = PTR_ERR(bus_cfg);
> +		goto out_of_put;
> +	}
> +
> +	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> +				  V4L2_MBUS_PCLK_SAMPLE_RISING);
> +
> +	v4l2_fwnode_endpoint_free(bus_cfg);
> +out_of_put:
> +	of_node_put(np);
> +	return ret;
> +}
> +#endif
> +
>  static int mt9m111_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *did)
>  {
> @@ -1141,6 +1182,15 @@ static int mt9m111_probe(struct i2c_client *client,
>  	/* Default HIGHPOWER context */
>  	mt9m111->ctx = &context_b;
>  
> +	if (IS_ENABLED(CONFIG_OF)) {
> +		ret = mt9m111_probe_of(client, mt9m111);
> +		if (ret)
> +			return ret;
> +	} else {
> +		/* use default chip hardware values */
> +		mt9m111->pclk_sample = 1;
> +	}
> +
>  	v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
>  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 4/4] media: mt9m111: allow to setup pixclk polarity

[Marco Felsch]

Hi Sakari,

On 18-10-20 00:24, Sakari Ailus wrote:
> Hi Marco,
> 
> Thanks for the patchset.
> 
> On Fri, Oct 19, 2018 at 05:50:27PM +0200, Marco Felsch wrote:
> > From: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> > 
> > The chip can be configured to output data transitions on the
> > rising or falling edge of PIXCLK (Datasheet R58:1[9]), default is on the
> > falling edge.
> > 
> > Parsing the fw-node is made in a subfunction to bundle all (future)
> > dt-parsing / fw-parsing stuff.
> 
> Could you rebase this on current mediatree master, please?

Of course.

> > 
> > Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> > (m.grzeschik@pengutronix.de: Fix inverting clock. INV_PIX_CLOCK bit is set
> > per default. Set bit to 0 (enable mask bit without value) to enable
> > falling edge sampling.)
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > (m.felsch@pengutronix.de: use fwnode helpers)
> > (m.felsch@pengutronix.de: mv of parsing into own function)
> > (m.felsch@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/media/i2c/mt9m111.c | 52 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 13080c6c1ba3..5d45bc9ea0cb 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -15,12 +15,14 @@
> >  #include <linux/delay.h>
> >  #include <linux/v4l2-mediabus.h>
> >  #include <linux/module.h>
> > +#include <linux/of_graph.h>
> >  
> >  #include <media/v4l2-async.h>
> >  #include <media/v4l2-clk.h>
> >  #include <media/v4l2-common.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> >  
> >  /*
> >   * MT9M111, MT9M112 and MT9M131:
> > @@ -236,6 +238,8 @@ struct mt9m111 {
> >  	const struct mt9m111_datafmt *fmt;
> >  	int lastpage;	/* PageMap cache value */
> >  	bool is_streaming;
> > +	/* user point of view - 0: falling 1: rising edge */
> > +	unsigned int pclk_sample:1;
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >  	struct media_pad pad;
> >  #endif
> > @@ -586,6 +590,10 @@ static int mt9m111_set_pixfmt(struct mt9m111 *mt9m111,
> >  		return -EINVAL;
> >  	}
> >  
> > +	/* receiver samples on falling edge, chip-hw default is rising */
> 
> Could you add DT binding documentation that would cover this? The existing
> documentation is, well, rather vague. Which properties are relevant for the
> hardware, are they mandatory or optional and if they're optional, then are
> there relevant default values?

Okay, I will ad a dt-binding patch.

Regards,
Marco

> > +	if (mt9m111->pclk_sample == 0)
> > +		mask_outfmt2 |= MT9M111_OUTFMT_INV_PIX_CLOCK;
> > +
> >  	ret = mt9m111_reg_mask(client, context_a.output_fmt_ctrl2,
> >  			       data_outfmt2, mask_outfmt2);
> >  	if (!ret)
> > @@ -1045,9 +1053,15 @@ static int mt9m111_s_stream(struct v4l2_subdev *sd, int enable)
> >  static int mt9m111_g_mbus_config(struct v4l2_subdev *sd,
> >  				struct v4l2_mbus_config *cfg)
> >  {
> > -	cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
> > +	struct mt9m111 *mt9m111 = container_of(sd, struct mt9m111, subdev);
> > +
> > +	cfg->flags = V4L2_MBUS_MASTER |
> >  		V4L2_MBUS_HSYNC_ACTIVE_HIGH | V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> >  		V4L2_MBUS_DATA_ACTIVE_HIGH;
> > +
> > +	cfg->flags |= mt9m111->pclk_sample ? V4L2_MBUS_PCLK_SAMPLE_FALLING :
> > +		V4L2_MBUS_PCLK_SAMPLE_RISING;
> > +
> >  	cfg->type = V4L2_MBUS_PARALLEL;
> >  
> >  	return 0;
> > @@ -1117,6 +1131,33 @@ static int mt9m111_video_probe(struct i2c_client *client)
> >  	return ret;
> >  }
> >  
> > +#ifdef CONFIG_OF
> > +static int mt9m111_probe_of(struct i2c_client *client, struct mt9m111 *mt9m111)
> > +{
> > +	struct v4l2_fwnode_endpoint *bus_cfg;
> > +	struct device_node *np;
> > +	int ret = 0;
> > +
> > +	np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> > +	if (!np)
> > +		return -EINVAL;
> > +
> > +	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(of_fwnode_handle(np));
> > +	if (IS_ERR(bus_cfg)) {
> > +		ret = PTR_ERR(bus_cfg);
> > +		goto out_of_put;
> > +	}
> > +
> > +	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> > +				  V4L2_MBUS_PCLK_SAMPLE_RISING);
> > +
> > +	v4l2_fwnode_endpoint_free(bus_cfg);
> > +out_of_put:
> > +	of_node_put(np);
> > +	return ret;
> > +}
> > +#endif
> > +
> >  static int mt9m111_probe(struct i2c_client *client,
> >  			 const struct i2c_device_id *did)
> >  {
> > @@ -1141,6 +1182,15 @@ static int mt9m111_probe(struct i2c_client *client,
> >  	/* Default HIGHPOWER context */
> >  	mt9m111->ctx = &context_b;
> >  
> > +	if (IS_ENABLED(CONFIG_OF)) {
> > +		ret = mt9m111_probe_of(client, mt9m111);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		/* use default chip hardware values */
> > +		mt9m111->pclk_sample = 1;
> > +	}
> > +
> >  	v4l2_i2c_subdev_init(&mt9m111->subdev, client, &mt9m111_subdev_ops);
> >  	mt9m111->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >  
> 
> -- 
> Regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com
>