[PATCH v3 0/6] media: mt9m111 features

[PATCH v3 0/6] media: mt9m111 features

[Marco Felsch]

Hi,

this v3 integrate the review  of my v2 [1]. I reordered the series as
mentioned by Sakari.

The patches are rebased on top of the actual media-tree/master.

[1] https://www.mail-archive.com/linux-media@vger.kernel.org/msg135932.html

Regards,
Marco


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

Marco Felsch (3):
  media: mt9m111: add s_stream callback
  dt-bindings: media: mt9m111: adapt documentation to be more clear
  dt-bindings: media: mt9m111: add pclk-sample property

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

 .../devicetree/bindings/media/i2c/mt9m111.txt |  13 +-
 drivers/media/i2c/mt9m111.c                   | 224 +++++++++++++++++-
 2 files changed, 232 insertions(+), 5 deletions(-)

-- 
2.19.1

[PATCH v3 1/6] 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 58d134dcdf44..03559669de9f 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -220,6 +220,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
@@ -910,6 +911,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)
 {
@@ -923,6 +932,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.1

[PATCH v3 2/6] 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 03559669de9f..9b0a3689fa98 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -563,6 +563,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.1

[PATCH v3 3/6] 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>

---
Changelog

v3:
- check if sensor window size is set to default and return if not
- check if requested fps is supported by image size and do not update
  the image size if requested fps is not supported by selected image
  size
- update fps mode only if a mode was found

v2:
- fix updating read mode register, use mt9m111_reg_mask() to update the
  relevant bits only. For this purpose add reg_mask field to
  struct mt9m111_mode_info.

 drivers/media/i2c/mt9m111.c | 165 ++++++++++++++++++++++++++++++++++++
 1 file changed, 165 insertions(+)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 9b0a3689fa98..f97fd32181ed 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -129,6 +129,8 @@
 #define MT9M111_OUTFMT_SWAP_YCbCr_Cb_Cr_RGB_R_B	(1 << 0)
 #define MT9M111_TPG_SEL_MASK		GENMASK(2, 0)
 #define MT9M111_EFFECTS_MODE_MASK	GENMASK(2, 0)
+#define MT9M111_RM_PWR_MASK		BIT(10)
+#define MT9M111_RM_SKIP2_MASK		GENMASK(3, 2)
 
 /*
  * Camera control register addresses (0x200..0x2ff not implemented)
@@ -207,6 +209,23 @@ 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;
+	unsigned int reg_mask;
+};
+
 struct mt9m111 {
 	struct v4l2_subdev subdev;
 	struct v4l2_ctrl_handler hdl;
@@ -216,6 +235,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;
@@ -226,6 +247,37 @@ 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,
+		.reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
+	},
+	[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,
+		.reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
+	},
+	[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,
+		.reg_mask = MT9M111_RM_PWR_MASK | MT9M111_RM_SKIP2_MASK,
+	},
+};
+
 /* Find a data format by a pixel code */
 static const struct mt9m111_datafmt *mt9m111_find_datafmt(struct mt9m111 *mt9m111,
 						u32 code)
@@ -618,6 +670,61 @@ 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 = MT9M111_MODE_SXGA_15FPS;
+	bool skip_30fps = false;
+
+	/*
+	 * The fps selection is based on the row, column skipping mechanism.
+	 * So ensure that the sensor window is set to default else the fps
+	 * aren't calculated correctly within the sensor hw.
+	 */
+	if (sensor_rect->width != MT9M111_MAX_WIDTH ||
+	    sensor_rect->height != MT9M111_MAX_HEIGHT) {
+		dev_info(mt9m111->subdev.dev,
+			 "Framerate selection is not supported for cropped "
+			 "images\n");
+		return NULL;
+	}
+
+	/* 30fps only supported for images not exceeding 640x512 */
+	if (width > MT9M111_MAX_WIDTH / 2 || height > MT9M111_MAX_HEIGHT / 2) {
+		dev_dbg(mt9m111->subdev.dev,
+			"Framerates > 15fps are supported only for images "
+			"not exceeding 640x512\n");
+		skip_30fps = true;
+	}
+
+	/* find best matched fps */
+	for (i = 0; i < MT9M111_NUM_MODES; i++) {
+		unsigned int fps = mt9m111_mode_data[i].max_fps;
+
+		if (fps == 30 && skip_30fps)
+			continue;
+
+		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;
+	return mode;
+}
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 static int mt9m111_g_register(struct v4l2_subdev *sd,
 			      struct v4l2_dbg_register *reg)
@@ -803,11 +910,16 @@ 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_mask(client, mt9m111->ctx->read_mode,
+			 mt9m111->current_mode->reg_val,
+			 mt9m111->current_mode->reg_mask);
 }
 
 static int mt9m111_resume(struct mt9m111 *mt9m111)
@@ -903,6 +1015,53 @@ 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. Do not update the mode if no one was found. */
+	mode = mt9m111_find_mode(mt9m111, fps, mt9m111->width, mt9m111->height);
+	if (!mode)
+		return 0;
+
+	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)
@@ -936,6 +1095,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 = {
@@ -1061,6 +1222,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.1

[PATCH v3 4/6] dt-bindings: media: mt9m111: adapt documentation to be more clear

[Marco Felsch]

Replace the vague binding by a more verbose. Remove the remote property
from the example since the driver don't support such a property. Also
remove the bus-width property from the endpoint since the driver don't
take care of it.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>

---
Changelog:

v3:
- drop remote-endpoint docu, since it is documented by
  video-interfaces.txt.

v2:
- initial commit

 Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
index 6b910036b57e..a431fb45704b 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
@@ -9,8 +9,10 @@ Required Properties:
 - clocks: reference to the master clock.
 - clock-names: shall be "mclk".
 
-For further reading on port node refer to
-Documentation/devicetree/bindings/media/video-interfaces.txt.
+The device node must contain one 'port' child node with one 'endpoint' child
+sub-node for its digital output video port, in accordance with the video
+interface bindings defined in:
+Documentation/devicetree/bindings/media/video-interfaces.txt
 
 Example:
 
@@ -21,10 +23,8 @@ Example:
 			clocks = <&mclk>;
 			clock-names = "mclk";
 
-			remote = <&pxa_camera>;
 			port {
 				mt9m111_1: endpoint {
-					bus-width = <8>;
 					remote-endpoint = <&pxa_camera>;
 				};
 			};
-- 
2.19.1

[PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

[Marco Felsch]

Add the pclk-sample property to the list of optional properties
for the mt9m111 camera sensor.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
index a431fb45704b..d0bed6fa901a 100644
--- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
+++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
@@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance with the video
 interface bindings defined in:
 Documentation/devicetree/bindings/media/video-interfaces.txt
 
+Optional endpoint properties:
+- pclk-sample: For information see ../video-interfaces.txt. The value is set to
+  0 if it isn't specified.
+
 Example:
 
 	i2c_master {
@@ -26,6 +30,7 @@ Example:
 			port {
 				mt9m111_1: endpoint {
 					remote-endpoint = <&pxa_camera>;
+					pclk-sample = <1>;
 				};
 			};
 		};
-- 
2.19.1

[PATCH v3 6/6] 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 fw parsing into own function)
(m.felsch@pengutronix.de: adapt commit msg)
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

---
Changelog:

v3:
- call mt9m111_probe_fw() before v4l2_clk_get() to avoid error handling

v2:
- make use of fwnode_*() to drop OF dependency and ifdef's
- mt9m111_g_mbus_config: fix pclk_sample logic which I made due the
  conversion from Enrico's patch.
---
 drivers/media/i2c/mt9m111.c | 46 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index f97fd32181ed..2ef332b9b914 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/module.h>
+#include <linux/property.h>
 
 #include <media/v4l2-async.h>
 #include <media/v4l2-clk.h>
@@ -22,6 +23,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
 
 /*
  * MT9M111, MT9M112 and MT9M131:
@@ -242,6 +244,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
@@ -594,6 +598,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)
@@ -1084,9 +1092,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_RISING :
+		V4L2_MBUS_PCLK_SAMPLE_FALLING;
+
 	cfg->type = V4L2_MBUS_PARALLEL;
 
 	return 0;
@@ -1156,6 +1170,32 @@ static int mt9m111_video_probe(struct i2c_client *client)
 	return ret;
 }
 
+static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
+{
+	struct v4l2_fwnode_endpoint *bus_cfg;
+	struct fwnode_handle *np;
+	int ret = 0;
+
+	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
+	if (!np)
+		return -EINVAL;
+
+	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
+	if (IS_ERR(bus_cfg)) {
+		ret = PTR_ERR(bus_cfg);
+		goto out_put_fw;
+	}
+
+	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+				  V4L2_MBUS_PCLK_SAMPLE_RISING);
+
+	v4l2_fwnode_endpoint_free(bus_cfg);
+
+out_put_fw:
+	fwnode_handle_put(np);
+	return ret;
+}
+
 static int mt9m111_probe(struct i2c_client *client,
 			 const struct i2c_device_id *did)
 {
@@ -1173,6 +1213,10 @@ static int mt9m111_probe(struct i2c_client *client,
 	if (!mt9m111)
 		return -ENOMEM;
 
+	ret = mt9m111_probe_fw(client, mt9m111);
+	if (ret)
+		return ret;
+
 	mt9m111->clk = v4l2_clk_get(&client->dev, "mclk");
 	if (IS_ERR(mt9m111->clk))
 		return PTR_ERR(mt9m111->clk);
-- 
2.19.1

Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

[Sakari Ailus]

Hi Marco,

On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> Add the pclk-sample property to the list of optional properties
> for the mt9m111 camera sensor.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> index a431fb45704b..d0bed6fa901a 100644
> --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> @@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance with the video
>  interface bindings defined in:
>  Documentation/devicetree/bindings/media/video-interfaces.txt
>  
> +Optional endpoint properties:
> +- pclk-sample: For information see ../video-interfaces.txt. The value is set to
> +  0 if it isn't specified.

How about the data-active, hsync-active and vsync-active properties? Does
the hardware have a fixed configuration, or can this be set? It appears the
driver assumes active high for all.

If there's something to change, this should be a separate patch IMO.

> +
>  Example:
>  
>  	i2c_master {
> @@ -26,6 +30,7 @@ Example:
>  			port {
>  				mt9m111_1: endpoint {
>  					remote-endpoint = <&pxa_camera>;
> +					pclk-sample = <1>;
>  				};
>  			};
>  		};

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Sakari Ailus]

Hi Marco,

On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> 
> 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 fw parsing into own function)
> (m.felsch@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

Applied with the following diff:

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 2ef332b9b914..b6011bfddde8 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client)
 
 static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
 {
-	struct v4l2_fwnode_endpoint *bus_cfg;
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_PARALLEL
+	};
 	struct fwnode_handle *np;
-	int ret = 0;
+	int ret;
 
 	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
 	if (!np)
 		return -EINVAL;
 
-	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
-	if (IS_ERR(bus_cfg)) {
-		ret = PTR_ERR(bus_cfg);
+	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);
+	if (ret)
 		goto out_put_fw;
-	}
 
-	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
+	mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
 				  V4L2_MBUS_PCLK_SAMPLE_RISING);
 
-	v4l2_fwnode_endpoint_free(bus_cfg);
+	v4l2_fwnode_endpoint_free(&bus_cfg);
 
 out_put_fw:
 	fwnode_handle_put(np);

Please base on current media tree master on the next time. Thanks.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Philipp Zabel]

Hi Sakari,

On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > 
> > 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 fw parsing into own function)
> > (m.felsch@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> Applied with the following diff:
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 2ef332b9b914..b6011bfddde8 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client)
>  
>  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
>  {
> -	struct v4l2_fwnode_endpoint *bus_cfg;
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_PARALLEL
> +	};
>  	struct fwnode_handle *np;
> -	int ret = 0;
> +	int ret;
>  
>  	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
>  	if (!np)
>  		return -EINVAL;
>  
> -	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> -	if (IS_ERR(bus_cfg)) {
> -		ret = PTR_ERR(bus_cfg);
> +	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);

Should that be

+	ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg);

intead?

> +	if (ret)
>  		goto out_put_fw;
> -	}
>  
> -	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> +	mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
>  				  V4L2_MBUS_PCLK_SAMPLE_RISING);
>  
> -	v4l2_fwnode_endpoint_free(bus_cfg);
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
>  
>  out_put_fw:
>  	fwnode_handle_put(np);
> 
> Please base on current media tree master on the next time. Thanks.

regards
Philipp

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

[Marco Felsch]

Hi Sakari,

On 18-11-27 15:19, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > 
> > 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 fw parsing into own function)
> > (m.felsch@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> Applied with the following diff:
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 2ef332b9b914..b6011bfddde8 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client)
>  
>  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
>  {
> -	struct v4l2_fwnode_endpoint *bus_cfg;
> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_PARALLEL
> +	};
>  	struct fwnode_handle *np;
> -	int ret = 0;
> +	int ret;
>  
>  	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
>  	if (!np)
>  		return -EINVAL;
>  
> -	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> -	if (IS_ERR(bus_cfg)) {
> -		ret = PTR_ERR(bus_cfg);
> +	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);
> +	if (ret)
>  		goto out_put_fw;
> -	}
>  
> -	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> +	mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
>  				  V4L2_MBUS_PCLK_SAMPLE_RISING);
>  
> -	v4l2_fwnode_endpoint_free(bus_cfg);
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
>  
>  out_put_fw:
>  	fwnode_handle_put(np);
> 
> Please base on current media tree master on the next time. Thanks.

Sorry, thanks for the inline fix :)

Kind regards,
Marco

> -- 
> Kind regards,
> 
> Sakari Ailus
> sakari.ailus@linux.intel.com

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

[Sakari Ailus]

Hi Philipp,

On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> Hi Sakari,
> 
> On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > > 
> > > 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 fw parsing into own function)
> > > (m.felsch@pengutronix.de: adapt commit msg)
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > 
> > Applied with the following diff:
> > 
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 2ef332b9b914..b6011bfddde8 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client)
> >  
> >  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
> >  {
> > -	struct v4l2_fwnode_endpoint *bus_cfg;
> > +	struct v4l2_fwnode_endpoint bus_cfg = {
> > +		.bus_type = V4L2_MBUS_PARALLEL
> > +	};
> >  	struct fwnode_handle *np;
> > -	int ret = 0;
> > +	int ret;
> >  
> >  	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> >  	if (!np)
> >  		return -EINVAL;
> >  
> > -	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > -	if (IS_ERR(bus_cfg)) {
> > -		ret = PTR_ERR(bus_cfg);
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);
> 
> Should that be
> 
> +	ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg);
> 
> intead?

Could be. I'd expect the driver to need the link frequency at some point
after which you'd need the variable size properties anyway. But that's not
the case now.

With the change, the v4l2_fwnode_endpoint_free() becomes redundant. So the
diff on that diff:

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index b6011bfddde8..d639b9bcf64a 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -1182,15 +1182,13 @@ static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
 	if (!np)
 		return -EINVAL;
 
-	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);
+	ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg);
 	if (ret)
 		goto out_put_fw;
 
 	mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
 				  V4L2_MBUS_PCLK_SAMPLE_RISING);
 
-	v4l2_fwnode_endpoint_free(&bus_cfg);
-
 out_put_fw:
 	fwnode_handle_put(np);
 	return ret;


> 
> > +	if (ret)
> >  		goto out_put_fw;
> > -	}
> >  
> > -	mt9m111->pclk_sample = !!(bus_cfg->bus.parallel.flags &
> > +	mt9m111->pclk_sample = !!(bus_cfg.bus.parallel.flags &
> >  				  V4L2_MBUS_PCLK_SAMPLE_RISING);
> >  
> > -	v4l2_fwnode_endpoint_free(bus_cfg);
> > +	v4l2_fwnode_endpoint_free(&bus_cfg);
> >  
> >  out_put_fw:
> >  	fwnode_handle_put(np);
> > 
> > Please base on current media tree master on the next time. Thanks.
> 
> regards
> Philipp

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Philipp Zabel]

Hi Sakari,

On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> > Hi Sakari,
> > 
> > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > > > 
> > > > 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 fw parsing into own function)
> > > > (m.felsch@pengutronix.de: adapt commit msg)
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > 
> > > Applied with the following diff:
> > > 
> > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > index 2ef332b9b914..b6011bfddde8 100644
> > > --- a/drivers/media/i2c/mt9m111.c
> > > +++ b/drivers/media/i2c/mt9m111.c
> > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client)
> > >  
> > >  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
> > >  {
> > > -	struct v4l2_fwnode_endpoint *bus_cfg;
> > > +	struct v4l2_fwnode_endpoint bus_cfg = {
> > > +		.bus_type = V4L2_MBUS_PARALLEL
> > > +	};
> > >  	struct fwnode_handle *np;
> > > -	int ret = 0;
> > > +	int ret;
> > >  
> > >  	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> > >  	if (!np)
> > >  		return -EINVAL;
> > >  
> > > -	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > > -	if (IS_ERR(bus_cfg)) {
> > > -		ret = PTR_ERR(bus_cfg);
> > > +	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);
> > 
> > Should that be
> > 
> > +	ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg);
> > 
> > intead?
> 
> Could be. I'd expect the driver to need the link frequency at some point
> after which you'd need the variable size properties anyway. But that's not
> the case now.

I don't think the link-frequencies property will be used, this is just a
parallel device. But Marco chose to use _alloc_parse because of what the
v4l2_fwnode_endpoint_parse() documentation says:

/*
 * NOTE: This function does not parse properties the size of which is variable
 * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() in                                                                
 * new drivers instead.
 */

So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There
is no unnecessary allocation, just a lookup of the non-existing link-
frequencies property.

regards
Philipp

Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

[Marco Felsch]

Hi Sakari,

On 18-11-27 15:13, Sakari Ailus wrote:
> Hi Marco,
> 
> On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> > Add the pclk-sample property to the list of optional properties
> > for the mt9m111 camera sensor.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > index a431fb45704b..d0bed6fa901a 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > @@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance with the video
> >  interface bindings defined in:
> >  Documentation/devicetree/bindings/media/video-interfaces.txt
> >  
> > +Optional endpoint properties:
> > +- pclk-sample: For information see ../video-interfaces.txt. The value is set to
> > +  0 if it isn't specified.
> 
> How about the data-active, hsync-active and vsync-active properties? Does
> the hardware have a fixed configuration, or can this be set? It appears the
> driver assumes active high for all.

As I understood it correctly the data-active, hsync-active and vsync-active
I/O lines are always ACTIVE_HIGH.

Kind regards,
Marco
 
> If there's something to change, this should be a separate patch IMO.
> 
> > +
> >  Example:
> >  
> >  	i2c_master {
> > @@ -26,6 +30,7 @@ Example:
> >  			port {
> >  				mt9m111_1: endpoint {
> >  					remote-endpoint = <&pxa_camera>;
> > +					pclk-sample = <1>;
> >  				};
> >  			};
> >  		};
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 5/6] dt-bindings: media: mt9m111: add pclk-sample property

[Sakari Ailus]

On Tue, Nov 27, 2018 at 03:21:05PM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 18-11-27 15:13, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Tue, Nov 27, 2018 at 11:02:52AM +0100, Marco Felsch wrote:
> > > Add the pclk-sample property to the list of optional properties
> > > for the mt9m111 camera sensor.
> > > 
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > Reviewed-by: Rob Herring <robh@kernel.org>
> > > ---
> > >  Documentation/devicetree/bindings/media/i2c/mt9m111.txt | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > index a431fb45704b..d0bed6fa901a 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > +++ b/Documentation/devicetree/bindings/media/i2c/mt9m111.txt
> > > @@ -14,6 +14,10 @@ sub-node for its digital output video port, in accordance with the video
> > >  interface bindings defined in:
> > >  Documentation/devicetree/bindings/media/video-interfaces.txt
> > >  
> > > +Optional endpoint properties:
> > > +- pclk-sample: For information see ../video-interfaces.txt. The value is set to
> > > +  0 if it isn't specified.
> > 
> > How about the data-active, hsync-active and vsync-active properties? Does
> > the hardware have a fixed configuration, or can this be set? It appears the
> > driver assumes active high for all.
> 
> As I understood it correctly the data-active, hsync-active and vsync-active
> I/O lines are always ACTIVE_HIGH.

Ack. Thanks!

> 
> Kind regards,
> Marco
>  
> > If there's something to change, this should be a separate patch IMO.
> > 
> > > +
> > >  Example:
> > >  
> > >  	i2c_master {
> > > @@ -26,6 +30,7 @@ Example:
> > >  			port {
> > >  				mt9m111_1: endpoint {
> > >  					remote-endpoint = <&pxa_camera>;
> > > +					pclk-sample = <1>;
> > >  				};
> > >  			};
> > >  		};
> > 
> > -- 
> > Sakari Ailus
> > sakari.ailus@linux.intel.com
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Sakari Ailus]

On Tue, Nov 27, 2018 at 03:12:29PM +0100, Philipp Zabel wrote:
> Hi Sakari,
> 
> On Tue, 2018-11-27 at 15:50 +0200, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > On Tue, Nov 27, 2018 at 02:39:27PM +0100, Philipp Zabel wrote:
> > > Hi Sakari,
> > > 
> > > On Tue, 2018-11-27 at 15:19 +0200, Sakari Ailus wrote:
> > > > Hi Marco,
> > > > 
> > > > On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > > > > 
> > > > > 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 fw parsing into own function)
> > > > > (m.felsch@pengutronix.de: adapt commit msg)
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > 
> > > > Applied with the following diff:
> > > > 
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index 2ef332b9b914..b6011bfddde8 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -1172,24 +1172,24 @@ static int mt9m111_video_probe(struct i2c_client *client)
> > > >  
> > > >  static int mt9m111_probe_fw(struct i2c_client *client, struct mt9m111 *mt9m111)
> > > >  {
> > > > -	struct v4l2_fwnode_endpoint *bus_cfg;
> > > > +	struct v4l2_fwnode_endpoint bus_cfg = {
> > > > +		.bus_type = V4L2_MBUS_PARALLEL
> > > > +	};
> > > >  	struct fwnode_handle *np;
> > > > -	int ret = 0;
> > > > +	int ret;
> > > >  
> > > >  	np = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> > > >  	if (!np)
> > > >  		return -EINVAL;
> > > >  
> > > > -	bus_cfg = v4l2_fwnode_endpoint_alloc_parse(np);
> > > > -	if (IS_ERR(bus_cfg)) {
> > > > -		ret = PTR_ERR(bus_cfg);
> > > > +	ret = v4l2_fwnode_endpoint_alloc_parse(np, &bus_cfg);
> > > 
> > > Should that be
> > > 
> > > +	ret = v4l2_fwnode_endpoint_parse(np, &bus_cfg);
> > > 
> > > intead?
> > 
> > Could be. I'd expect the driver to need the link frequency at some point
> > after which you'd need the variable size properties anyway. But that's not
> > the case now.
> 
> I don't think the link-frequencies property will be used, this is just a
> parallel device. But Marco chose to use _alloc_parse because of what the
> v4l2_fwnode_endpoint_parse() documentation says:
> 
> /*
>  * NOTE: This function does not parse properties the size of which is variable
>  * without a low fixed limit. Please use v4l2_fwnode_endpoint_alloc_parse() in                                                                
>  * new drivers instead.
>  */
> 
> So maybe we want to use v4l2_fwnode_endpoint_alloc_parse() always. There
> is no unnecessary allocation, just a lookup of the non-existing link-
> frequencies property.

There could be other properties in the future.

When I wrote that, I guess I ignored that the link frequency might not be
relevant for some devices such as CSI-2 receivers. I think it'd make sense
to remove the latter sentence; I can send a patch. The first sentence that
tells the limitations of the function is enough IMO.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Sakari Ailus]

On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> 
> 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 fw parsing into own function)
> (m.felsch@pengutronix.de: adapt commit msg)
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>

This one as well:

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 005fc2bd0d05..902c3cabf44c 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -859,6 +859,7 @@ config VIDEO_MT9M032
 config VIDEO_MT9M111
 	tristate "mt9m111, mt9m112 and mt9m131 support"
 	depends on I2C && VIDEO_V4L2
+	select V4L2_FWNODE
 	help
 	  This driver supports MT9M111, MT9M112 and MT9M131 cameras from
 	  Micron/Aptina

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Marco Felsch]

On 18-11-27 23:15, Sakari Ailus wrote:
> On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > 
> > 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 fw parsing into own function)
> > (m.felsch@pengutronix.de: adapt commit msg)
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> 
> This one as well:

Sorry for that, I forget to adapt the Kconfig to often. Thanks for your
fix.

Kind regards,
Marco

> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 005fc2bd0d05..902c3cabf44c 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -859,6 +859,7 @@ config VIDEO_MT9M032
>  config VIDEO_MT9M111
>  	tristate "mt9m111, mt9m112 and mt9m131 support"
>  	depends on I2C && VIDEO_V4L2
> +	select V4L2_FWNODE
>  	help
>  	  This driver supports MT9M111, MT9M112 and MT9M131 cameras from
>  	  Micron/Aptina
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Sakari Ailus]

On Wed, Nov 28, 2018 at 09:29:01AM +0100, Marco Felsch wrote:
> On 18-11-27 23:15, Sakari Ailus wrote:
> > On Tue, Nov 27, 2018 at 11:02:53AM +0100, 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.
> > > 
> > > 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 fw parsing into own function)
> > > (m.felsch@pengutronix.de: adapt commit msg)
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > 
> > This one as well:
> 
> Sorry for that, I forget to adapt the Kconfig to often. Thanks for your
> fix.

No worries. I hope we'll have automated compile testing in not too distant
future...

-- 
Sakari Ailus
sakari.ailus@linux.intel.com