[PATCH v1 00/12] rcar: vin: Add digital input to Gen3

[PATCH v1 00/12] rcar: vin: Add digital input to Gen3

[Jacopo Mondi]

Hello,
   this patch series implements support for parsing OF nodes describing
digital video inputs such as parallel camera sensor connected to a Gen3 VIN
instance. It also supports linking the camera sub-device to media controller
graph.

Sending as RFC because work is far from being complete, but before everything
I would like to validate some implementation choices I made.

First of all, the DT layout proposed in patches 01 and 02.
I am now registering digital input pins inside port@2 node, while all CSI input
are currently enumerated in port@1. While I found no mention of this being a bad
practice in device tree binding documentation, I would like to validate if having
input of different nature (serial CSI-2 and parallel) in 2 different subnodes is
acceptable.

Patches from 05 to 12 assume this device tree layout and parse the additional
parallel input making use of digital_graph_* routines, used for Gen2 input.
(In future, the distinction between Gen2 and Gen3 use cases can probably be
reduced in the driver, unifying most of those functions, but that's for later).
The digital input subdevice media entity is then linked to the VIN instance it
is connected to.

Patches 03 and 04 are specific to the camera module I am using for testing,
and are required to have the device probe and having it linked in the media
controller graph.

A few words on HW setup: I have an mt9m111 camera sensor module connected to
Salvator-X M3-W EXIO-D connector, through a camera shield designed for another
board (grpeach-audiocamera shield). The camera board provides pull up resistors
for I2c lines and routes 3v3 VCC and GND to its camera connector. The camera
sensor module (designed to fit that connector) has an on-board 27Mhz oscillator
to generate clock fed to the camera sensor, and provides to the camera sensor
the required digital and analog input voltages generated from a single 3v3 input.
The 8 video digital output pins from camera module to the digital input port are
connected to VIN4[15:8] pins, exposed on EXIO-D connector pins.

After the device has probed and has been asynchronously registered by the VIN
driver, it appears in media controller graph, linked to VIN4 instance.

$ ./media-ctl -p -d /dev/media0

...

- entity 95: rcar_vin e6ef4000.video (1 pad, 3 links)
             type Node subtype V4L flags 0
             device node name /dev/video4
        pad0: Sink
                <- "rcar_csi2 feaa0000.csi2":1 [ENABLED]
                <- "rcar_csi2 fea80000.csi2":1 []
                <- "mt9m111 8-0048":0 [ENABLED]
...

- entity 139: mt9m111 8-0048 (1 pad, 1 link)
              type V4L2 subdev subtype Sensor flags 0
              device node name /dev/v4l-subdev4
        pad0: Source
                [fmt:YUYV8_2X8/0x0 field:none
                 crop.bounds:(26,8)/1280x1024
                 crop:(26,8)/1280x1024]
                -> "rcar_vin e6ef4000.video":0 [ENABLED]

Sending to linux-renesas-soc only, as before expanding to other lists/people
at least basic streaming needs to be validated.

Thanks
   j

Jacopo Mondi (12):
  arm64: boot: dts: salvator-x: Add camera module
  arm64: boot: dts: Salvator-X: Add VIN parallel input
  media: i2c: mt9m111: Skip chid identification
  media: i2c: mt9m111: Add source pad
  media: rcar: vin: Prepare to parse Gen3 digital input
  media: rcar: vin: Accept parallel input on Gen3
  media: rcar: vin: Add ID for parallel input on Gen3
  media: rcar: vin: Parse digital input after CSI on Gen3
  media: rcar: vin: Install notifier for digital input
  media: rcar: vin: Add digital input mask to vin_dev
  media: rcar: vin: Store VIN instance id
  media: rcar: vin: Link digital subdev to VIN instance

 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts |  58 ++++++++++
 drivers/media/i2c/mt9m111.c                        |  36 +++---
 drivers/media/platform/rcar-vin/rcar-core.c        | 127 ++++++++++++++-------
 drivers/media/platform/rcar-vin/rcar-vin.h         |   8 ++
 4 files changed, 166 insertions(+), 63 deletions(-)

--
2.7.4

[PATCH v1 01/12] arm64: boot: dts: salvator-x: Add camera module

[Jacopo Mondi]

Add camera module to Salvator-X M3W device tree.
The camera module sits on a i2c-gpio interface and it connected to VIN
channel #4 parallel video input port.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
index ca4eb98..41c94c3 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
@@ -103,6 +103,41 @@
 		states = <3300000 1
 			  1800000 0>;
 	};
+
+	camera_module {
+		compatible = "i2c-gpio";
+		gpios = <&gpio1 9 GPIO_ACTIVE_HIGH
+			 &gpio1 11 GPIO_ACTIVE_HIGH
+			>;
+
+		i2c-gpio,delay-us = <4>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		mclk: xclk {
+			compatible = "fixed-clock";
+			#clock-cells = <1>;
+			clock-frequency  = <27000000>;
+			clock-output-names = "mclk";
+		};
+
+		camera: mt9m111@48 {
+			compatible = "micron,mt9m111";
+			reg = <0x48>;
+
+			clocks = <&mclk 0>;
+			clock-names = "mclk";
+
+			port {
+				mt9m111_out: endpoint {
+					bus_width = <8>;
+					remote-endpoint = <&vin4_in>;
+				};
+			};
+		};
+	};
+
 };
 
 &pfc {
-- 
2.7.4

[PATCH v1 02/12] arm64: boot: dts: Salvator-X: Add VIN parallel input

[Jacopo Mondi]

Add parallel video input port to VIN channel #4 and link it to camera
module remote endpoint.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
index 41c94c3..b69fe4a 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
@@ -379,6 +379,29 @@
 
 &vin4 {
 	status = "okay";
+
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	ports{
+		port@2 {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			reg = <2>;
+
+			vin4_in: endpoint@0 {
+				reg = <0>;
+
+				bus_width = <8>;
+				hsync-active = <1>;
+				vsync-active = <1>;
+				data-shift = <8>;
+
+				remote-endpoint = <&mt9m111_out>;
+			};
+		};
+	};
 };
 
 &vin5 {
-- 
2.7.4

[PATCH v1 03/12] media: i2c: mt9m111: Skip chid identification

[Jacopo Mondi]

Reads of chip identification code (both on registers 0x00 and 0xff)
always return 0x00.
Skip chip identification to have the device complete probing.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/mt9m111.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 72e71b7..8e86d51 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -890,31 +890,12 @@ static struct v4l2_subdev_ops mt9m111_subdev_ops = {
 static int mt9m111_video_probe(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	s32 data;
 	int ret;
 
 	ret = mt9m111_s_power(&mt9m111->subdev, 1);
 	if (ret < 0)
 		return ret;
 
-	data = reg_read(CHIP_VERSION);
-
-	switch (data) {
-	case 0x143a: /* MT9M111 or MT9M131 */
-		dev_info(&client->dev,
-			"Detected a MT9M111/MT9M131 chip ID %x\n", data);
-		break;
-	case 0x148c: /* MT9M112 */
-		dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", data);
-		break;
-	default:
-		dev_err(&client->dev,
-			"No MT9M111/MT9M112/MT9M131 chip detected register read %x\n",
-			data);
-		ret = -ENODEV;
-		goto done;
-	}
-
 	ret = mt9m111_init(mt9m111);
 	if (ret)
 		goto done;
-- 
2.7.4

[PATCH v1 04/12] media: i2c: mt9m111: Add source pad

[Jacopo Mondi]

Add a single source pad to mt9m111 media entity.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/i2c/mt9m111.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 8e86d51..7e70969 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -141,6 +141,8 @@
 #define MT9M111_MAX_HEIGHT	1024
 #define MT9M111_MAX_WIDTH	1280
 
+#define MT9M111_MEDIA_PAD	1
+
 struct mt9m111_context {
 	u16 read_mode;
 	u16 blanking_h;
@@ -215,6 +217,8 @@ struct mt9m111 {
 	int power_count;
 	const struct mt9m111_datafmt *fmt;
 	int lastpage;	/* PageMap cache value */
+
+	struct media_pad pad;
 };
 
 /* Find a data format by a pixel code */
@@ -963,13 +967,23 @@ static int mt9m111_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto out_hdlfree;
 
+	mt9m111->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_pads_init(&mt9m111->subdev.entity, MT9M111_MEDIA_PAD,
+				     &mt9m111->pad);
+	if (ret)
+		goto out_hdlfree;
+
 	mt9m111->subdev.dev = &client->dev;
+	mt9m111->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	mt9m111->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 	ret = v4l2_async_register_subdev(&mt9m111->subdev);
 	if (ret < 0)
-		goto out_hdlfree;
+		goto out_mediafree;
 
 	return 0;
 
+out_mediafree:
+	media_entity_cleanup(&mt9m111->subdev.entity);
 out_hdlfree:
 	v4l2_ctrl_handler_free(&mt9m111->hdl);
 out_clkput:
@@ -982,6 +996,7 @@ static int mt9m111_remove(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 
+	media_entity_cleanup(&mt9m111->subdev.entity);
 	v4l2_async_unregister_subdev(&mt9m111->subdev);
 	v4l2_clk_put(mt9m111->clk);
 	v4l2_ctrl_handler_free(&mt9m111->hdl);
-- 
2.7.4

[PATCH v1 05/12] media: rcar: vin: Prepare to parse Gen3 digital input

[Jacopo Mondi]

Support parsing digital input on configurable port id and reg
properties. Also make the function return -ENOENT when no subdevice is
found.
This change is needed to support parsing digital input on Gen3.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 175f138..ef61bcc 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
 	return 0;
 }
 
-static int rvin_digital_graph_parse(struct rvin_dev *vin)
+static int rvin_digital_graph_parse(struct rvin_dev *vin,
+				    unsigned int port,
+				    unsigned int reg)
 {
 	struct device_node *ep, *np;
 	int ret;
@@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev *vin)
 	vin->digital.asd.match.fwnode.fwnode = NULL;
 	vin->digital.subdev = NULL;
 
-	/*
-	 * Port 0 id 0 is local digital input, try to get it.
-	 * Not all instances can or will have this, that is OK
-	 */
-	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
+	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg);
 	if (!ep)
-		return 0;
+		return -ENOENT;
 
 	np = of_graph_get_remote_port_parent(ep);
 	if (!np) {
@@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
 	if (ret)
 		return ret;
 
-	ret = rvin_digital_graph_parse(vin);
-	if (ret)
+	/*
+	 * Port 0 id 0 is local digital input, try to get it.
+	 * Not all instances can or will have this, that is OK
+	 */
+	ret = rvin_digital_graph_parse(vin, 0, 0);
+	if (ret && ret != -ENOENT)
 		return ret;
 
-	if (!vin->digital.asd.match.fwnode.fwnode) {
+	if (ret == -ENOENT) {
 		vin_dbg(vin, "No digital subdevice found\n");
 		return -ENODEV;
 	}
-- 
2.7.4

[PATCH v1 06/12] media: rcar: vin: Accept parallel input on Gen3

[Jacopo Mondi]

Accept parallel/BT.565 bus types when parsing OF on Gen3.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 37 +++++++++++++----------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index ef61bcc..6738825 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -360,29 +360,24 @@ static int rvin_parse_v4l2(struct rvin_dev *vin,
 	}
 
 	mbus_cfg->type = v4l2_ep.bus_type;
+	mbus_cfg->flags = 0;
 
-	if (vin->info->chip == RCAR_GEN3) {
-		switch (mbus_cfg->type) {
-		case V4L2_MBUS_CSI2:
-			vin_dbg(vin, "Found CSI-2 media bus\n");
-			mbus_cfg->flags = 0;
-			return 0;
-		default:
-			break;
-		}
-	} else {
-		switch (mbus_cfg->type) {
-		case V4L2_MBUS_PARALLEL:
-			vin_dbg(vin, "Found PARALLEL media bus\n");
-			mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
-			return 0;
-		case V4L2_MBUS_BT656:
-			vin_dbg(vin, "Found BT656 media bus\n");
-			mbus_cfg->flags = 0;
-			return 0;
-		default:
+	switch (mbus_cfg->type) {
+	case V4L2_MBUS_CSI2:
+		if (vin->info->chip < RCAR_GEN3)
 			break;
-		}
+
+		vin_dbg(vin, "Found CSI-2 media bus\n");
+		return 0;
+	case V4L2_MBUS_PARALLEL:
+		vin_dbg(vin, "Found PARALLEL media bus\n");
+		mbus_cfg->flags = v4l2_ep.bus.parallel.flags;
+		return 0;
+	case V4L2_MBUS_BT656:
+		vin_dbg(vin, "Found BT656 media bus\n");
+		return 0;
+	default:
+		break;
 	}
 
 	vin_err(vin, "Unknown media bus type\n");
-- 
2.7.4

[PATCH v1 07/12] media: rcar: vin: Add ID for parallel input on Gen3

[Jacopo Mondi]

Add an identifier for parallel input on Gen3.
Also define the maximum number of available inputs on Gen3 boards.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index e7e600f..4361de9 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -51,6 +51,8 @@ enum rvin_csi_id {
 	RVIN_CSI40,
 	RVIN_CSI41,
 	RVIN_CSI_MAX,
+	RVIN_PARALLEL_IN,
+	RVIN_INPUT_MAX,
 	RVIN_NC, /* Not Connected */
 };
 
-- 
2.7.4

[PATCH v1 08/12] media: rcar: vin: Parse digital input after CSI on Gen3

[Jacopo Mondi]

When parsing available inputs on Gen3 boards, add parsing of digital
input enpoints to the already in-place CSI-2 node parsing.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 6738825..78ca232 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -777,7 +777,9 @@ static int rvin_group_graph_parse(struct rvin_dev *vin, unsigned long *bitmap)
 	vin_dbg(vin, "I'm VIN number %u", val);
 	vin->group->vin[val] = vin;
 
-	/* Parse all CSI-2 nodes */
+	/* Parse all endpoints on Port@1 (CSI-2) and eventually on Port@2 */
+	vin->mbus_cfg.type = 0;
+	vin->mbus_cfg.flags = 0;
 	for (i = 0; i < RVIN_CSI_MAX; i++) {
 
 		/* Check if instance is connected to the CSI-2 */
@@ -804,15 +806,21 @@ static int rvin_group_graph_parse(struct rvin_dev *vin, unsigned long *bitmap)
 		vin->group->csi[i].asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
 
 		*bitmap |= BIT(i);
+		vin->mbus_cfg.type = V4L2_MBUS_CSI2;
 
 		vin_dbg(vin, "Handle CSI-2 %s\n", of_node_full_name(csi));
 	}
 
-	/* All our sources are CSI-2 */
-	vin->mbus_cfg.type = V4L2_MBUS_CSI2;
-	vin->mbus_cfg.flags = 0;
+	/* Done with at least one CSI-2 port registered */
+	if (vin->mbus_cfg.type)
+		return 0;
 
-	return 0;
+	/* Check if any digital input is connected to this instance's Port@2 */
+	ret = rvin_digital_graph_parse(vin, 2, -1);
+	if (!ret)
+		*bitmap |=  BIT(RVIN_PARALLEL_IN);
+
+	return (!ret || (ret == -ENOENT)) ? 0 : ret;
 }
 
 /* group lock should be held when calling this function */
-- 
2.7.4

[PATCH v1 09/12] media: rcar: vin: Install notifier for digital input

[Jacopo Mondi]

Install async notifier for digital input on Gen3 when no other CSI-2
input has been found connected.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 78ca232..6e5d84a 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -866,17 +866,28 @@ static int rvin_group_graph_init(struct rvin_dev *vin)
 	}
 
 	i = 0;
-	for_each_set_bit(bit, &bitmap, RVIN_CSI_MAX) {
-		subdevs[i++] = &vin->group->csi[bit].asd;
+	for_each_set_bit(bit, &bitmap, RVIN_INPUT_MAX) {
+		if (bit < RVIN_CSI_MAX)
+			subdevs[i++] = &vin->group->csi[bit].asd;
+		else if (bit == RVIN_PARALLEL_IN) {
+			subdevs[0] = &vin->digital.asd;
+			vin->notifier.num_subdevs = 1;
+		}
 	}
 
 	vin_dbg(vin, "Claimed %d subdevices for group\n", count);
 
-	vin->notifier.num_subdevs = count;
 	vin->notifier.subdevs = subdevs;
-	vin->notifier.bound = rvin_group_notify_bound;
-	vin->notifier.unbind = rvin_group_notify_unbind;
-	vin->notifier.complete = rvin_group_notify_complete;
+	if (!vin->notifier.num_subdevs) {
+		vin->notifier.num_subdevs = count;
+		vin->notifier.bound = rvin_group_notify_bound;
+		vin->notifier.unbind = rvin_group_notify_unbind;
+		vin->notifier.complete = rvin_group_notify_complete;
+	} else {
+		vin->notifier.bound = rvin_digital_notify_bound;
+		vin->notifier.unbind = rvin_digital_notify_unbind;
+		vin->notifier.complete = rvin_digital_notify_complete;
+	}
 
 	mutex_unlock(&vin->group->lock);
 
-- 
2.7.4

[PATCH v1 10/12] media: rcar: vin: Add digital input mask to vin_dev

[Jacopo Mondi]

Add bitmask to enumerate VIN channel identifiers that support digital
video inputs.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 4 ++++
 drivers/media/platform/rcar-vin/rcar-vin.h  | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 6e5d84a..1d0b492 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -1028,6 +1028,8 @@ static const struct rvin_info rcar_info_r8a7795 = {
 			{ .csi = RVIN_CSI21, .chan = 3 },
 		},
 	},
+
+	.din_mask = BIT(4) | BIT(5),
 };
 
 static const struct rvin_info rcar_info_r8a7796 = {
@@ -1088,6 +1090,8 @@ static const struct rvin_info rcar_info_r8a7796 = {
 			{ .csi = RVIN_CSI20, .chan = 3 },
 		},
 	},
+
+	.din_mask = BIT(4) | BIT(5),
 };
 
 static const struct of_device_id rvin_of_id_table[] = {
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 4361de9..333ef3b 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -119,6 +119,8 @@ struct rvin_group_chsel {
  *
  * num_chsels:		number of possible chsel values for this VIN
  * chsels:		routing table VIN <-> CSI-2 for the chsel values
+ *
+ * din_mask:		mask of VIN channels IDs supporting digital input
  */
 struct rvin_info {
 	enum chip_id chip;
@@ -129,6 +131,8 @@ struct rvin_info {
 
 	unsigned int num_chsels;
 	struct rvin_group_chsel chsels[RCAR_VIN_NUM][RCAR_CHSEL_MAX];
+
+	unsigned int din_mask;
 };
 
 /**
-- 
2.7.4

[PATCH v1 11/12] media: rcar: vin: Store VIN instance id

[Jacopo Mondi]

Store VIN instance id in rvin_dev.
The instance id is reported by "renesas,id" OF property.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 1 +
 drivers/media/platform/rcar-vin/rcar-vin.h  | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 1d0b492..67494ee 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -776,6 +776,7 @@ static int rvin_group_graph_parse(struct rvin_dev *vin, unsigned long *bitmap)
 
 	vin_dbg(vin, "I'm VIN number %u", val);
 	vin->group->vin[val] = vin;
+	vin->id = val;
 
 	/* Parse all endpoints on Port@1 (CSI-2) and eventually on Port@2 */
 	vin->mbus_cfg.type = 0;
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 333ef3b..eaae927 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -139,6 +139,7 @@ struct rvin_info {
  * struct rvin_dev - Renesas VIN device structure
  * @dev:		(OF) device
  * @base:		device I/O register space remapped to virtual memory
+ * @id:			VIN instance number
  * @info:		info about VIN instance
  *
  * @vdev:		V4L2 video device associated with VIN
@@ -171,6 +172,7 @@ struct rvin_info {
 struct rvin_dev {
 	struct device *dev;
 	void __iomem *base;
+	unsigned int id;
 	const struct rvin_info *info;
 
 	struct video_device vdev;
-- 
2.7.4

[PATCH v1 12/12] media: rcar: vin: Link digital subdev to VIN instance

[Jacopo Mondi]

Link digital subdevice to the current VIN instance if the instance
support it and we are currently using media controller API.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 67494ee..503c905 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -26,6 +26,12 @@
 
 #include "rcar-vin.h"
 
+static int rvin_group_add_link(struct rvin_dev *vin,
+			       struct media_entity *source,
+			       unsigned int source_idx,
+			       struct media_entity *sink,
+			       unsigned int sink_idx,
+			       u32 flags);
 /* -----------------------------------------------------------------------------
  * Media Controller link notification
  */
@@ -418,6 +424,7 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 {
 	struct rvin_dev *vin = notifier_to_vin(notifier);
 	struct v4l2_subdev *sd = vin_to_source(vin);
+	struct media_entity *source, *sink;
 	int ret;
 
 	/* Verify subdevices mbus format */
@@ -464,7 +471,20 @@ static int rvin_digital_notify_complete(struct v4l2_async_notifier *notifier)
 		v4l2_disable_ioctl(&vin->vdev, VIDIOC_ENUMSTD);
 	}
 
-	return rvin_reset_format(vin);
+	ret = rvin_reset_format(vin);
+	if (ret)
+		return ret;
+
+	/* Link the digital subdev with the VIN instance, if supported */
+	if (vin->info->use_mc && (vin->info->din_mask & BIT(vin->id))) {
+		source = &vin->digital.subdev->entity;
+		sink = &vin->group->vin[vin->id]->vdev.entity;
+
+		ret = rvin_group_add_link(vin, source, 0, sink, 0,
+					  MEDIA_LNK_FL_ENABLED);
+	}
+
+	return ret;
 }
 
 static void rvin_digital_notify_unbind(struct v4l2_async_notifier *notifier,
-- 
2.7.4

Re: [PATCH v1 02/12] arm64: boot: dts: Salvator-X: Add VIN parallel input

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2017-06-19 19:04:39 +0200, Jacopo Mondi wrote:
> Add parallel video input port to VIN channel #4 and link it to camera
> module remote endpoint.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 23 ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> index 41c94c3..b69fe4a 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> @@ -379,6 +379,29 @@
>  
>  &vin4 {
>  	status = "okay";
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	ports{
> +		port@2 {

The Gen3 code is not yet upstream so I don't blame you for not reading 
rcar_vin.txt [1] :-) I did try to prepare the DT bindings for the 
digital input and that it should be as similar as possible to the Gen2 
case. There for please use port@0 for the digital input, at least until 
someone finds a flaw in the proposed DT bindings for Gen3.

1. Documentation/devicetree/bindings/media/rcar_vin.txt from the patch 
   series '[PATCH v4 00/27] rcar-vin: Add Gen3 with media controller 
   support'.

> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			reg = <2>;
> +
> +			vin4_in: endpoint@0 {
> +				reg = <0>;
> +
> +				bus_width = <8>;
> +				hsync-active = <1>;
> +				vsync-active = <1>;
> +				data-shift = <8>;
> +
> +				remote-endpoint = <&mt9m111_out>;
> +			};
> +		};
> +	};
>  };
>  
>  &vin5 {
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v1 05/12] media: rcar: vin: Prepare to parse Gen3 digital input

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2017-06-19 19:04:42 +0200, Jacopo Mondi wrote:
> Support parsing digital input on configurable port id and reg
> properties. Also make the function return -ENOENT when no subdevice is
> found.
> This change is needed to support parsing digital input on Gen3.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 175f138..ef61bcc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
>  	return 0;
>  }
>  
> -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> +static int rvin_digital_graph_parse(struct rvin_dev *vin,
> +				    unsigned int port,
> +				    unsigned int reg)
>  {
>  	struct device_node *ep, *np;
>  	int ret;
> @@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev *vin)
>  	vin->digital.asd.match.fwnode.fwnode = NULL;
>  	vin->digital.subdev = NULL;
>  
> -	/*
> -	 * Port 0 id 0 is local digital input, try to get it.
> -	 * Not all instances can or will have this, that is OK
> -	 */
> -	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> +	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg);

I don't think it's necessary to supply the port and reg from the caller.  
If the DT proposed in rcar_vin.txt is used.

>  	if (!ep)
> -		return 0;
> +		return -ENOENT;

I'm not saying this is wrong, but why do you change this? I can't see a 
clear advantage in doing so. And if do it I think it should be done in a 
separate patch explaining why.

>  
>  	np = of_graph_get_remote_port_parent(ep);
>  	if (!np) {
> @@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
>  	if (ret)
>  		return ret;
>  
> -	ret = rvin_digital_graph_parse(vin);
> -	if (ret)
> +	/*
> +	 * Port 0 id 0 is local digital input, try to get it.
> +	 * Not all instances can or will have this, that is OK
> +	 */
> +	ret = rvin_digital_graph_parse(vin, 0, 0);
> +	if (ret && ret != -ENOENT)
>  		return ret;
>  
> -	if (!vin->digital.asd.match.fwnode.fwnode) {
> +	if (ret == -ENOENT) {
>  		vin_dbg(vin, "No digital subdevice found\n");
>  		return -ENODEV;
>  	}
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v1 07/12] media: rcar: vin: Add ID for parallel input on Gen3

[Niklas Söderlund]

Hi Jacopo,

Thanks for your patch.

On 2017-06-19 19:04:44 +0200, Jacopo Mondi wrote:
> Add an identifier for parallel input on Gen3.
> Also define the maximum number of available inputs on Gen3 boards.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index e7e600f..4361de9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -51,6 +51,8 @@ enum rvin_csi_id {
>  	RVIN_CSI40,
>  	RVIN_CSI41,
>  	RVIN_CSI_MAX,
> +	RVIN_PARALLEL_IN,
> +	RVIN_INPUT_MAX,

It feels a bit odd to add the parallel enum to the CSI-2 enums. If this 
is needed maybe the whole enum needs to be renamed?

>  	RVIN_NC, /* Not Connected */
>  };
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v1 09/12] media: rcar: vin: Install notifier for digital input

[Niklas Söderlund]

Hi Jacopo,

On 2017-06-19 19:04:46 +0200, Jacopo Mondi wrote:
> Install async notifier for digital input on Gen3 when no other CSI-2
> input has been found connected.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 78ca232..6e5d84a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -866,17 +866,28 @@ static int rvin_group_graph_init(struct rvin_dev *vin)
>  	}
>  
>  	i = 0;
> -	for_each_set_bit(bit, &bitmap, RVIN_CSI_MAX) {
> -		subdevs[i++] = &vin->group->csi[bit].asd;
> +	for_each_set_bit(bit, &bitmap, RVIN_INPUT_MAX) {
> +		if (bit < RVIN_CSI_MAX)
> +			subdevs[i++] = &vin->group->csi[bit].asd;
> +		else if (bit == RVIN_PARALLEL_IN) {
> +			subdevs[0] = &vin->digital.asd;
> +			vin->notifier.num_subdevs = 1;
> +		}
>  	}
>  
>  	vin_dbg(vin, "Claimed %d subdevices for group\n", count);
>  
> -	vin->notifier.num_subdevs = count;
>  	vin->notifier.subdevs = subdevs;
> -	vin->notifier.bound = rvin_group_notify_bound;
> -	vin->notifier.unbind = rvin_group_notify_unbind;
> -	vin->notifier.complete = rvin_group_notify_complete;
> +	if (!vin->notifier.num_subdevs) {
> +		vin->notifier.num_subdevs = count;
> +		vin->notifier.bound = rvin_group_notify_bound;
> +		vin->notifier.unbind = rvin_group_notify_unbind;
> +		vin->notifier.complete = rvin_group_notify_complete;
> +	} else {
> +		vin->notifier.bound = rvin_digital_notify_bound;
> +		vin->notifier.unbind = rvin_digital_notify_unbind;
> +		vin->notifier.complete = rvin_digital_notify_complete;
> +	}

Hum, if there is a digital subdevice you ignore to look for CSI-2 
devices? This feels wrong, it probably works (for now) since all CSI-2 
subdevs are found by VIN0 since it's probed first and the digital input 
is only valid for VIN4 and VIN5 right? I hope to remedy this before the 
VIN Gen3 code is ready for upstream and the goal is that each VIN should 
look for the subdevices that effects it self. With that change this will 
break :-(

>  
>  	mutex_unlock(&vin->group->lock);
>  
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v1 01/12] arm64: boot: dts: salvator-x: Add camera module

[Geert Uytterhoeven]

Hi Jacopo,

On Mon, Jun 19, 2017 at 7:04 PM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add camera module to Salvator-X M3W device tree.
> The camera module sits on a i2c-gpio interface and it connected to VIN

is connected

> channel #4 parallel video input port.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> index ca4eb98..41c94c3 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> @@ -103,6 +103,41 @@
>                 states = <3300000 1
>                           1800000 0>;
>         };
> +
> +       camera_module {
> +               compatible = "i2c-gpio";
> +               gpios = <&gpio1 9 GPIO_ACTIVE_HIGH
> +                        &gpio1 11 GPIO_ACTIVE_HIGH
> +                       >;
> +
> +               i2c-gpio,delay-us = <4>;
> +
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               mclk: xclk {
> +                       compatible = "fixed-clock";
> +                       #clock-cells = <1>;
> +                       clock-frequency  = <27000000>;
> +                       clock-output-names = "mclk";

Please the drop clock-output-names for clock with a single output, as it is
deprecated.  The clock name will then be taken from the node name, so you
may want to adjust that.

> +               };

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v1 05/12] media: rcar: vin: Prepare to parse Gen3 digital input

[jmondi]

Hi Niklas,
    thanks for review

On Mon, Jun 19, 2017 at 09:39:46PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2017-06-19 19:04:42 +0200, Jacopo Mondi wrote:
> > Support parsing digital input on configurable port id and reg
> > properties. Also make the function return -ENOENT when no subdevice is
> > found.
> > This change is needed to support parsing digital input on Gen3.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 175f138..ef61bcc 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -511,7 +511,9 @@ static int rvin_digital_notify_bound(struct v4l2_async_notifier *notifier,
> >  	return 0;
> >  }
> >
> > -static int rvin_digital_graph_parse(struct rvin_dev *vin)
> > +static int rvin_digital_graph_parse(struct rvin_dev *vin,
> > +				    unsigned int port,
> > +				    unsigned int reg)
> >  {
> >  	struct device_node *ep, *np;
> >  	int ret;
> > @@ -519,13 +521,9 @@ static int rvin_digital_graph_parse(struct rvin_dev *vin)
> >  	vin->digital.asd.match.fwnode.fwnode = NULL;
> >  	vin->digital.subdev = NULL;
> >
> > -	/*
> > -	 * Port 0 id 0 is local digital input, try to get it.
> > -	 * Not all instances can or will have this, that is OK
> > -	 */
> > -	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, 0, 0);
> > +	ep = of_graph_get_endpoint_by_regs(vin->dev->of_node, port, reg);
>
> I don't think it's necessary to supply the port and reg from the caller.
> If the DT proposed in rcar_vin.txt is used.
>
> >  	if (!ep)
> > -		return 0;
> > +		return -ENOENT;
>
> I'm not saying this is wrong, but why do you change this? I can't see a
> clear advantage in doing so. And if do it I think it should be done in a
> separate patch explaining why.

For both questions, as I parse entries in port@2 and
rvin_digital_notify_bound was intended to use port@0 by default I made
port and reg parameters. I use -ENOENT to distinguish the case where
no endpoint has been found

>
> >
> >  	np = of_graph_get_remote_port_parent(ep);
> >  	if (!np) {
> > @@ -555,11 +553,15 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
> >  	if (ret)
> >  		return ret;
> >
> > -	ret = rvin_digital_graph_parse(vin);
> > -	if (ret)
> > +	/*
> > +	 * Port 0 id 0 is local digital input, try to get it.
> > +	 * Not all instances can or will have this, that is OK
> > +	 */
> > +	ret = rvin_digital_graph_parse(vin, 0, 0);
> > +	if (ret && ret != -ENOENT)
> >  		return ret;
> >
> > -	if (!vin->digital.asd.match.fwnode.fwnode) {
> > +	if (ret == -ENOENT) {
> >  		vin_dbg(vin, "No digital subdevice found\n");
> >  		return -ENODEV;
> >  	}
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH v1 09/12] media: rcar: vin: Install notifier for digital input

[jmondi]

Hi Niklas,

On Mon, Jun 19, 2017 at 09:51:15PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2017-06-19 19:04:46 +0200, Jacopo Mondi wrote:
> > Install async notifier for digital input on Gen3 when no other CSI-2
> > input has been found connected.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 78ca232..6e5d84a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -866,17 +866,28 @@ static int rvin_group_graph_init(struct rvin_dev *vin)
> >  	}
> >
> >  	i = 0;
> > -	for_each_set_bit(bit, &bitmap, RVIN_CSI_MAX) {
> > -		subdevs[i++] = &vin->group->csi[bit].asd;
> > +	for_each_set_bit(bit, &bitmap, RVIN_INPUT_MAX) {
> > +		if (bit < RVIN_CSI_MAX)
> > +			subdevs[i++] = &vin->group->csi[bit].asd;
> > +		else if (bit == RVIN_PARALLEL_IN) {
> > +			subdevs[0] = &vin->digital.asd;
> > +			vin->notifier.num_subdevs = 1;
> > +		}
> >  	}
> >
> >  	vin_dbg(vin, "Claimed %d subdevices for group\n", count);
> >
> > -	vin->notifier.num_subdevs = count;
> >  	vin->notifier.subdevs = subdevs;
> > -	vin->notifier.bound = rvin_group_notify_bound;
> > -	vin->notifier.unbind = rvin_group_notify_unbind;
> > -	vin->notifier.complete = rvin_group_notify_complete;
> > +	if (!vin->notifier.num_subdevs) {
> > +		vin->notifier.num_subdevs = count;
> > +		vin->notifier.bound = rvin_group_notify_bound;
> > +		vin->notifier.unbind = rvin_group_notify_unbind;
> > +		vin->notifier.complete = rvin_group_notify_complete;
> > +	} else {
> > +		vin->notifier.bound = rvin_digital_notify_bound;
> > +		vin->notifier.unbind = rvin_digital_notify_unbind;
> > +		vin->notifier.complete = rvin_digital_notify_complete;
> > +	}
>
> Hum, if there is a digital subdevice you ignore to look for CSI-2
> devices? This feels wrong, it probably works (for now) since all CSI-2
> subdevs are found by VIN0 since it's probed first and the digital input
> is only valid for VIN4 and VIN5 right? I hope to remedy this before the
> VIN Gen3 code is ready for upstream and the goal is that each VIN should
> look for the subdevices that effects it self. With that change this will
> break :-(

This is something I probably did not get: I thought parallel input and
CSI-2 input where mutually exclusive. In particular, looking at chsel
tables (from Table 26.14 on), where routing between CSI inputs and VIN
channel is described, I did not get how parallel input fit in that
picture. My best assumption now is that when a channel is NC it means
it can be used for digital input?

>
> >
> >  	mutex_unlock(&vin->group->lock);
> >
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

Re: [PATCH v1 01/12] arm64: boot: dts: salvator-x: Add camera module

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Monday 19 Jun 2017 19:04:38 Jacopo Mondi wrote:
> Add camera module to Salvator-X M3W device tree.
> The camera module sits on a i2c-gpio interface and it connected to VIN
> channel #4 parallel video input port.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts index ca4eb98..41c94c3
> 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts
> @@ -103,6 +103,41 @@
>  		states = <3300000 1
>  			  1800000 0>;
>  	};
> +
> +	camera_module {

This node describes an I2C master controller, it should be called accordingly.

> +		compatible = "i2c-gpio";
> +		gpios = <&gpio1 9 GPIO_ACTIVE_HIGH
> +			 &gpio1 11 GPIO_ACTIVE_HIGH
> +			>;
> +
> +		i2c-gpio,delay-us = <4>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		mclk: xclk {
> +			compatible = "fixed-clock";
> +			#clock-cells = <1>;
> +			clock-frequency  = <27000000>;
> +			clock-output-names = "mclk";
> +		};

This node describes a fixed oscillator not connected to the I2C bus, so it 
should be moved to the top-level of the device tree instead of being a child 
of the I2C controller.

> +		camera: mt9m111@48 {

No need for a label, and you should call the node according to its function, 
not the device model. Maybe something like camera-sensor@48 ? Or just 
camera@48 ?

> +			compatible = "micron,mt9m111";
> +			reg = <0x48>;
> +
> +			clocks = <&mclk 0>;
> +			clock-names = "mclk";
> +
> +			port {
> +				mt9m111_out: endpoint {
> +					bus_width = <8>;
> +					remote-endpoint = <&vin4_in>;
> +				};
> +			};
> +		};
> +	};
> +
>  };
> 
>  &pfc {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 03/12] media: i2c: mt9m111: Skip chid identification

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Monday 19 Jun 2017 19:04:40 Jacopo Mondi wrote:
> Reads of chip identification code (both on registers 0x00 and 0xff)
> always return 0x00.

This shouldn't be the case. It might mean that your I2C master controller 
doesn't handle reads correctly. I don't think this patch is correct.

> Skip chip identification to have the device complete probing.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/mt9m111.c | 19 -------------------
>  1 file changed, 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 72e71b7..8e86d51 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -890,31 +890,12 @@ static struct v4l2_subdev_ops mt9m111_subdev_ops = {
>  static int mt9m111_video_probe(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> -	s32 data;
>  	int ret;
> 
>  	ret = mt9m111_s_power(&mt9m111->subdev, 1);
>  	if (ret < 0)
>  		return ret;
> 
> -	data = reg_read(CHIP_VERSION);
> -
> -	switch (data) {
> -	case 0x143a: /* MT9M111 or MT9M131 */
> -		dev_info(&client->dev,
> -			"Detected a MT9M111/MT9M131 chip ID %x\n", data);
> -		break;
> -	case 0x148c: /* MT9M112 */
> -		dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n", 
data);
> -		break;
> -	default:
> -		dev_err(&client->dev,
> -			"No MT9M111/MT9M112/MT9M131 chip detected register 
read %x\n",
> -			data);
> -		ret = -ENODEV;
> -		goto done;
> -	}
> -
>  	ret = mt9m111_init(mt9m111);
>  	if (ret)
>  		goto done;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 04/12] media: i2c: mt9m111: Add source pad

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Monday 19 Jun 2017 19:04:41 Jacopo Mondi wrote:
> Add a single source pad to mt9m111 media entity.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/media/i2c/mt9m111.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> index 8e86d51..7e70969 100644
> --- a/drivers/media/i2c/mt9m111.c
> +++ b/drivers/media/i2c/mt9m111.c
> @@ -141,6 +141,8 @@
>  #define MT9M111_MAX_HEIGHT	1024
>  #define MT9M111_MAX_WIDTH	1280
> 
> +#define MT9M111_MEDIA_PAD	1
> +
>  struct mt9m111_context {
>  	u16 read_mode;
>  	u16 blanking_h;
> @@ -215,6 +217,8 @@ struct mt9m111 {
>  	int power_count;
>  	const struct mt9m111_datafmt *fmt;
>  	int lastpage;	/* PageMap cache value */
> +
> +	struct media_pad pad;
>  };
> 
>  /* Find a data format by a pixel code */
> @@ -963,13 +967,23 @@ static int mt9m111_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		goto out_hdlfree;
> 
> +	mt9m111->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_pads_init(&mt9m111->subdev.entity, 
MT9M111_MEDIA_PAD,

The second argument is the number of pads, not a pad number. The macro name is 
quite confusing. Given that you use it once only, you can drop it and hardcode 
the value.

> +				     &mt9m111->pad);
> +	if (ret)
> +		goto out_hdlfree;
> +
>  	mt9m111->subdev.dev = &client->dev;
> +	mt9m111->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	mt9m111->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  	ret = v4l2_async_register_subdev(&mt9m111->subdev);
>  	if (ret < 0)
> -		goto out_hdlfree;
> +		goto out_mediafree;
> 
>  	return 0;
> 
> +out_mediafree:

Maybe out_media_cleanup ?

> +	media_entity_cleanup(&mt9m111->subdev.entity);
>  out_hdlfree:
>  	v4l2_ctrl_handler_free(&mt9m111->hdl);
>  out_clkput:
> @@ -982,6 +996,7 @@ static int mt9m111_remove(struct i2c_client *client)
>  {
>  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> 
> +	media_entity_cleanup(&mt9m111->subdev.entity);
>  	v4l2_async_unregister_subdev(&mt9m111->subdev);
>  	v4l2_clk_put(mt9m111->clk);
>  	v4l2_ctrl_handler_free(&mt9m111->hdl);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 03/12] media: i2c: mt9m111: Skip chid identification

[jmondi]

Hi Laurent,

On Tue, Jun 20, 2017 at 10:48:21AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Monday 19 Jun 2017 19:04:40 Jacopo Mondi wrote:
> > Reads of chip identification code (both on registers 0x00 and 0xff)
> > always return 0x00.
>
> This shouldn't be the case. It might mean that your I2C master controller
> doesn't handle reads correctly. I don't think this patch is correct.

Indeed this patch was not meant for inclusion (as it is part of an RFC series)
but I still fail to read the chip identifier even when using a tool to
sniff the bus or talk directly to the chip (I'm using bus pirate
for this purpose).

What puzzles me is that I get back reads from some registers that match the
defaults reported by the data sheet, while other registers return
meaningful values but different from what I have indicated as defaults
in the sensor manual, while chip identifier reads always back as 0x00.

I am pasting some examples down here of some interactions with the
chip using bus pirate serial control console [1]

I wonder if I am working with a different chip revision from the one
the driver and the datasheet have as reference, but it really seems
unlikely to me..

Thanks
   j


[1]
** Read register 0x106: expected value 0x700E -> got it!
I2C>[0x90+0xf0+0x00+0x1]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0xF0 ACK
WRITE: 0x00 ACK
WRITE: 0x01 ACK
I2C STOP BIT
I2C>[0x90+0x06+[0x91+rr]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0x06 ACK
I2C START BIT
WRITE: 0x91 ACK
READ: 0x70
READ:  ACK 0x0E
NACK
I2C STOP BIT

** Read register 0x108: expected value 0x0080 -> got 0xc800
I2C>[0x90+0xf0+0x00+0x1]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0xF0 ACK
WRITE: 0x00 ACK
WRITE: 0x01 ACK
I2C STOP BIT
I2C>[0x90+0x08+[0x91+rr]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0x08 ACK
I2C START BIT
WRITE: 0x91 ACK
READ: 0xC8
READ:  ACK 0x00
NACK
I2C STOP BIT

** Read Chip ID: expected value 0x143a -> got 0x0000
I2C>[0x90+0xf0+0x00+0x0]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0xF0 ACK
WRITE: 0x00 ACK
WRITE: 0x00 ACK
I2C STOP BIT
I2C>[0x90+0x00+[0x91+rr]
I2C START BIT
WRITE: 0x90 ACK
WRITE: 0x00 ACK
I2C START BIT
WRITE: 0x91 ACK
READ: 0x00
READ:  ACK 0x00
NACK
I2C STOP BIT

>
> > Skip chip identification to have the device complete probing.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/media/i2c/mt9m111.c | 19 -------------------
> >  1 file changed, 19 deletions(-)
> >
> > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > index 72e71b7..8e86d51 100644
> > --- a/drivers/media/i2c/mt9m111.c
> > +++ b/drivers/media/i2c/mt9m111.c
> > @@ -890,31 +890,12 @@ static struct v4l2_subdev_ops mt9m111_subdev_ops = {
> >  static int mt9m111_video_probe(struct i2c_client *client)
> >  {
> >  	struct mt9m111 *mt9m111 = to_mt9m111(client);
> > -	s32 data;
> >  	int ret;
> >
> >  	ret = mt9m111_s_power(&mt9m111->subdev, 1);
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	data = reg_read(CHIP_VERSION);
> > -
> > -	switch (data) {
> > -	case 0x143a: /* MT9M111 or MT9M131 */
> > -		dev_info(&client->dev,
> > -			"Detected a MT9M111/MT9M131 chip ID %x\n", data);
> > -		break;
> > -	case 0x148c: /* MT9M112 */
> > -		dev_info(&client->dev, "Detected a MT9M112 chip ID %x\n",
> data);
> > -		break;
> > -	default:
> > -		dev_err(&client->dev,
> > -			"No MT9M111/MT9M112/MT9M131 chip detected register
> read %x\n",
> > -			data);
> > -		ret = -ENODEV;
> > -		goto done;
> > -	}
> > -
> >  	ret = mt9m111_init(mt9m111);
> >  	if (ret)
> >  		goto done;
>
> --
> Regards,
>
> Laurent Pinchart
>